From 22c8e4c6ade70dd5a6c37cb34a28946ae8d84c71 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Wed, 12 Jul 2017 11:58:18 -0400 Subject: [PATCH] Redirect Course Home for course that hasn't started. Includes the following: - Move the redirect logic for before course that hasn't started to share between Course Info and Course Home. - Add audit comments for Course Info vs Course Home - Other minor clean-up. LEARNER-613 --- lms/djangoapps/courseware/courses.py | 27 ++++-- .../courseware/tests/test_course_info.py | 12 +-- .../tests/test_view_authentication.py | 12 ++- lms/djangoapps/courseware/views/views.py | 92 +++++++++---------- .../tests/views/test_course_home.py | 74 ++++++++++++++- 5 files changed, 146 insertions(+), 71 deletions(-) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 3fda77b7c4..610bd8fc2f 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -6,16 +6,10 @@ import logging from collections import defaultdict from datetime import datetime -import pytz -from django.conf import settings -from django.core.urlresolvers import reverse -from django.http import Http404 -from fs.errors import ResourceNotFoundError -from opaque_keys.edx.keys import UsageKey -from path import Path as path - import branding +import pytz from courseware.access import has_access +from courseware.access_response import StartDateError from courseware.date_summary import ( CourseEndDate, CourseStartDate, @@ -25,13 +19,20 @@ from courseware.date_summary import ( ) from courseware.model_data import FieldDataCache from courseware.module_render import get_module, get_module_for_descriptor +from django.conf import settings +from django.core.urlresolvers import reverse +from django.http import Http404, QueryDict from edxmako.shortcuts import render_to_string +from fs.errors import ResourceNotFoundError from lms.djangoapps.courseware.courseware_access_exception import CoursewareAccessException from lms.djangoapps.courseware.exceptions import CourseAccessRedirect +from opaque_keys.edx.keys import UsageKey from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers +from path import Path as path from static_replace import replace_static_urls from student.models import CourseEnrollment +from util.date_utils import strftime_localized from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.x_module import STUDENT_VIEW @@ -121,6 +122,16 @@ def check_course_access(course, user, action, check_if_enrolled=False): access_response = has_access(user, action, course, course.id) if not access_response: + # Redirect if StartDateError + if isinstance(access_response, StartDateError): + start_date = strftime_localized(course.start, 'SHORT_DATE') + params = QueryDict(mutable=True) + params['notlive'] = start_date + raise CourseAccessRedirect('{dashboard_url}?{params}'.format( + dashboard_url=reverse('dashboard'), + params=params.urlencode() + )) + # Deliberately return a non-specific error message to avoid # leaking info about access control settings raise CoursewareAccessException(access_response) diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 48b0775b07..99cc43187d 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -8,11 +8,10 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.http import QueryDict from django.test.utils import override_settings -from nose.plugins.attrib import attr -from pyquery import PyQuery as pq - from lms.djangoapps.ccx.tests.factories import CcxFactory +from nose.plugins.attrib import attr from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration +from pyquery import PyQuery as pq from student.models import CourseEnrollment from student.tests.factories import AdminFactory from util.date_utils import strftime_localized @@ -58,6 +57,7 @@ class CourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTestCase): resp = self.client.get(url) self.assertNotIn("You are not currently enrolled in this course", resp.content) + # TODO: LEARNER-611: If this is only tested under Course Info, does this need to move? @mock.patch('openedx.features.enterprise_support.api.get_enterprise_consent_url') def test_redirection_missing_enterprise_consent(self, mock_get_url): """ @@ -120,7 +120,7 @@ class CourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTestCase): self.assertRedirects(response, expected_url) @mock.patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False}) - @mock.patch("courseware.views.views.strftime_localized") + @mock.patch("util.date_utils.strftime_localized") def test_non_live_course_other_language(self, mock_strftime_localized): """Ensure that a user accessing a non-live course sees a redirect to the student dashboard, not a 404, even if the localized date is unicode @@ -385,7 +385,7 @@ class SelfPacedCourseInfoTestCase(LoginEnrollmentTestCase, SharedModuleStoreTest self.assertEqual(resp.status_code, 200) def test_num_queries_instructor_paced(self): - self.fetch_course_info_with_queries(self.instructor_paced_course, 26, 4) + self.fetch_course_info_with_queries(self.instructor_paced_course, 26, 3) def test_num_queries_self_paced(self): - self.fetch_course_info_with_queries(self.self_paced_course, 26, 4) + self.fetch_course_info_with_queries(self.self_paced_course, 26, 3) diff --git a/lms/djangoapps/courseware/tests/test_view_authentication.py b/lms/djangoapps/courseware/tests/test_view_authentication.py index 7fffc8bd68..bf1a7f1c8c 100644 --- a/lms/djangoapps/courseware/tests/test_view_authentication.py +++ b/lms/djangoapps/courseware/tests/test_view_authentication.py @@ -60,7 +60,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): Check that non-staff don't have access to dark urls. """ - names = ['courseware', 'instructor_dashboard', 'progress'] + names = ['courseware', 'progress'] urls = self._reverse_urls(names, course) urls.extend([ reverse('book', kwargs={'course_id': course.id.to_deprecated_string(), @@ -68,7 +68,11 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): for index, __ in enumerate(course.textbooks) ]) for url in urls: - self.assert_request_status_code(404, url) + self.assert_request_status_code(302, url) + + self.assert_request_status_code( + 404, reverse('instructor_dashboard', kwargs={'course_id': course.id.to_deprecated_string()}) + ) def _check_staff(self, course): """ @@ -97,7 +101,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): 'student_id': self.enrolled_user.id, } ) - self.assert_request_status_code(404, url) + self.assert_request_status_code(302, url) # The courseware url should redirect, not 200 url = self._reverse_urls(['courseware'], course)[0] @@ -351,9 +355,9 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase): self.enroll(self.test_course, True) # should now be able to get to everything for self.course + self._check_staff(self.course) self._check_non_staff_light(self.test_course) self._check_non_staff_dark(self.test_course) - self._check_staff(self.course) @patch.dict('courseware.access.settings.FEATURES', {'DISABLE_START_DATES': False}) def test_dark_launch_global_staff(self): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 228370288d..ec3d5e4e1f 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -8,36 +8,10 @@ from collections import OrderedDict, namedtuple from datetime import datetime, timedelta import analytics -import waffle -from django.conf import settings -from django.contrib.auth.decorators import login_required -from django.contrib.auth.models import AnonymousUser, User -from django.core.context_processors import csrf -from django.core.exceptions import PermissionDenied -from django.core.urlresolvers import reverse -from django.db import transaction -from django.db.models import Q -from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, QueryDict -from django.shortcuts import redirect -from django.utils.decorators import method_decorator -from django.utils.text import slugify -from django.utils.timezone import UTC -from django.utils.translation import ugettext as _ -from django.views.decorators.cache import cache_control -from django.views.decorators.csrf import ensure_csrf_cookie -from django.views.decorators.http import require_GET, require_http_methods, require_POST -from django.views.generic import View -from ipware.ip import get_ip -from markupsafe import escape -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey, UsageKey -from pytz import utc -from rest_framework import status -from web_fragments.fragment import Fragment - import shoppingcart import survey.utils import survey.views +import waffle from certificates import api as certs_api from certificates.models import CertificateStatuses from commerce.utils import EcommerceService @@ -64,9 +38,28 @@ from courseware.model_data import FieldDataCache from courseware.models import BaseStudentModuleHistory, StudentModule from courseware.url_helpers import get_redirect_url from courseware.user_state_client import DjangoXBlockUserStateClient +from django.conf import settings +from django.contrib.auth.decorators import login_required +from django.contrib.auth.models import AnonymousUser, User +from django.core.context_processors import csrf +from django.core.exceptions import PermissionDenied +from django.core.urlresolvers import reverse +from django.db import transaction +from django.db.models import Q +from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, QueryDict +from django.shortcuts import redirect +from django.utils.decorators import method_decorator +from django.utils.text import slugify +from django.utils.timezone import UTC +from django.utils.translation import ugettext as _ +from django.views.decorators.cache import cache_control +from django.views.decorators.csrf import ensure_csrf_cookie +from django.views.decorators.http import require_GET, require_http_methods, require_POST +from django.views.generic import View from edxmako.shortcuts import marketing_link, render_to_response, render_to_string from enrollment.api import add_enrollment from eventtracking import tracker +from ipware.ip import get_ip from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException from lms.djangoapps.ccx.utils import prep_course_for_grading from lms.djangoapps.courseware.exceptions import CourseAccessRedirect, Redirect @@ -74,6 +67,9 @@ from lms.djangoapps.grades.new.course_grade_factory import CourseGradeFactory from lms.djangoapps.instructor.enrollment import uses_shib from lms.djangoapps.instructor.views.api import require_global_staff from lms.djangoapps.verify_student.models import SoftwareSecurePhotoVerification +from markupsafe import escape +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey, UsageKey from openedx.core.djangoapps.catalog.utils import get_programs, get_programs_with_type from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credit.api import ( @@ -91,6 +87,8 @@ from openedx.features.course_experience import UNIFIED_COURSE_TAB_FLAG, course_h from openedx.features.course_experience.course_tools import CourseToolsPluginManager from openedx.features.course_experience.views.course_dates import CourseDatesFragmentView from openedx.features.enterprise_support.api import data_sharing_consent_required +from pytz import utc +from rest_framework import status from shoppingcart.utils import is_shopping_cart_enabled from student.models import CourseEnrollment, UserTestGroup from survey.utils import must_answer_survey @@ -99,6 +97,7 @@ from util.date_utils import strftime_localized from util.db import outer_atomic from util.milestones_helpers import get_prerequisite_courses_display from util.views import _record_feedback_in_zendesk, ensure_valid_course_key, ensure_valid_usage_key +from web_fragments.fragment import Fragment from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.tabs import CourseTabList @@ -233,6 +232,8 @@ def course_info(request, course_id): Assumes the course_id is in a valid format. """ + # TODO: LEARNER-611: This can be deleted with Course Info removal. The new + # Course Home is using its own processing of last accessed. def get_last_accessed_courseware(course, request, user): """ Returns the courseware module URL that the user last accessed, or None if it cannot be found. @@ -262,29 +263,13 @@ def course_info(request, course_id): return redirect(reverse(course_home_url_name(course_key), args=[course_id])) with modulestore().bulk_operations(course_key): - course = get_course_by_id(course_key, depth=2) - access_response = has_access(request.user, 'load', course, course_key) - - if not access_response: - - # The user doesn't have access to the course. If they're - # denied permission due to the course not being live yet, - # redirect to the dashboard page. - if isinstance(access_response, StartDateError): - start_date = strftime_localized(course.start, 'SHORT_DATE') - params = QueryDict(mutable=True) - params['notlive'] = start_date - return redirect('{dashboard_url}?{params}'.format( - dashboard_url=reverse('dashboard'), - params=params.urlencode() - )) - # Otherwise, give a 404 to avoid leaking info about access - # control. - raise Http404("Course not found.") + course = get_course_with_access(request.user, 'load', course_key) staff_access = has_access(request.user, 'staff', course) masquerade, user = setup_masquerade(request, course_key, staff_access, reset_masquerade_data=True) + # LEARNER-612: CCX redirect handled by new Course Home (DONE) + # TODO: LEARNER-1697: Transition banner messages to new Course Home. # if user is not enrolled in a course then app will show enroll/get register link inside course info page. user_is_enrolled = CourseEnrollment.is_enrolled(user, course.id) show_enroll_banner = request.user.is_authenticated() and not user_is_enrolled @@ -294,20 +279,24 @@ def course_info(request, course_id): if not user_is_enrolled and not can_self_enroll_in_course(course_key): return redirect(reverse('dashboard')) + # TODO: LEARNER-1865: Handle prereqs and course survey in new Course Home. # Redirect the user if they are not yet allowed to view this course check_access_to_course(request, course) + # LEARNER-170: Entrance exam is handled by new Course Outline. (DONE) # If the user needs to take an entrance exam to access this course, then we'll need # to send them to that specific course module before allowing them into other areas if not user_can_skip_entrance_exam(user, course): return redirect(reverse('courseware', args=[unicode(course.id)])) + # TODO: LEARNER-611: Remove deprecated course.bypass_home. # If the user is coming from the dashboard and bypass_home setting is set, # redirect them straight to the courseware page. is_from_dashboard = reverse('dashboard') in request.META.get('HTTP_REFERER', []) if course.bypass_home and is_from_dashboard: return redirect(reverse('courseware', args=[course_id])) + # TODO: LEARNER-1697: Transition handling of enroll links in new Course Home. # link to where the student should go to enroll in the course: # about page if there is not marketing site, SITE_NAME if there is url_to_enroll = reverse(course_about, args=[course_id]) @@ -318,7 +307,9 @@ def course_info(request, course_id): dates_fragment = None if request.user.is_authenticated(): + # TODO: LEARNER-611: Remove enable_course_home_improvements if SelfPacedConfiguration.current().enable_course_home_improvements: + # Shared code with the new Course Home (DONE) dates_fragment = CourseDatesFragmentView().render_to_fragment(request, course_id=course_id) # This local import is due to the circularity of lms and openedx references. @@ -326,9 +317,7 @@ def course_info(request, course_id): # as plugins, and to avoid the direct import. from openedx.features.course_experience.views.course_reviews import CourseReviewsModuleFragmentView - # Decide whether or not to show the reviews link in the course tools bar - show_reviews_link = CourseReviewsModuleFragmentView.is_configured() - + # Shared code with the new Course Home (DONE) # Get the course tools enabled for this user and course course_tools = CourseToolsPluginManager.get_enabled_course_tools(request, course_key) @@ -346,7 +335,6 @@ def course_info(request, course_id): 'user_is_enrolled': user_is_enrolled, 'dates_fragment': dates_fragment, 'url_to_enroll': url_to_enroll, - 'show_reviews_link': show_reviews_link, 'course_tools': course_tools, # TODO: (Experimental Code). See https://openedx.atlassian.net/wiki/display/RET/2.+In-course+Verification+Prompts @@ -358,9 +346,11 @@ def course_info(request, course_id): # Get the URL of the user's last position in order to display the 'where you were last' message context['resume_course_url'] = None + # TODO: LEARNER-611: Remove enable_course_home_improvements if SelfPacedConfiguration.current().enable_course_home_improvements: context['resume_course_url'] = get_last_accessed_courseware(course, request, user) + # LEARNER-981/LEARNER-837: Hide masquerade as necessary in Course Home (DONE) if not is_course_open_for_learner(user, course): # Disable student view button if user is staff and # course is not yet visible to students. @@ -1697,6 +1687,7 @@ def check_access_to_course(request, course): """ Raises Redirect exceptions if the user does not have course access. """ + # TODO: LEARNER-1865: Handle prereqs in new Course Home. # Redirect to the dashboard if not all prerequisites have been met if not has_access(request.user, 'view_courseware_with_prerequisites', course): log.info( @@ -1705,6 +1696,7 @@ def check_access_to_course(request, course): request.user.id, unicode(course.id)) raise CourseAccessRedirect(reverse('dashboard')) + # TODO: LEARNER-1865: Handle course surveys in new Course Home. # Redirect if the user must answer a survey before entering the course. if must_answer_survey(course, request.user): raise CourseAccessRedirect(reverse('course_survey', args=[unicode(course.id)])) diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 3413e5179e..10607783fd 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -1,15 +1,19 @@ +# coding=utf-8 """ Tests for the course home page. """ import ddt - +import mock from courseware.tests.factories import StaffFactory +from django.conf import settings from django.core.urlresolvers import reverse +from django.http import QueryDict from django.utils.http import urlquote_plus from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES, override_waffle_flag from openedx.features.course_experience import SHOW_REVIEWS_TOOL_FLAG, UNIFIED_COURSE_TAB_FLAG from student.models import CourseEnrollment from student.tests.factories import UserFactory +from util.date_utils import strftime_localized from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import CourseUserType, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls @@ -28,12 +32,25 @@ QUERY_COUNT_TABLE_BLACKLIST = WAFFLE_TABLES def course_home_url(course): """ - Returns the URL for the course's home page + Returns the URL for the course's home page. + + Arguments: + course (CourseDescriptor): The course being tested. + """ + return course_home_url_from_string(unicode(course.id)) + + +def course_home_url_from_string(course_key_string): + """ + Returns the URL for the course's home page. + + Arguments: + course_key_string (String): The course key as string. """ return reverse( 'openedx.course_experience.course_home', kwargs={ - 'course_id': unicode(course.id), + 'course_id': course_key_string, } ) @@ -211,3 +228,54 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): url = course_home_url(self.course) response = self.client.get(url) self.assertContains(response, '/login?next={url}'.format(url=urlquote_plus(url))) + + @mock.patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False}) + def test_non_live_course(self): + """ + Ensure that a user accessing a non-live course sees a redirect to + the student dashboard, not a 404. + """ + self.user = self.create_user_for_course(self.course, CourseUserType.ENROLLED) + + url = course_home_url(self.course) + response = self.client.get(url) + start_date = strftime_localized(self.course.start, 'SHORT_DATE') + expected_params = QueryDict(mutable=True) + expected_params['notlive'] = start_date + expected_url = '{url}?{params}'.format( + url=reverse('dashboard'), + params=expected_params.urlencode() + ) + self.assertRedirects(response, expected_url) + + @mock.patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False}) + @mock.patch("util.date_utils.strftime_localized") + def test_non_live_course_other_language(self, mock_strftime_localized): + """ + Ensure that a user accessing a non-live course sees a redirect to + the student dashboard, not a 404, even if the localized date is unicode + """ + self.user = self.create_user_for_course(self.course, CourseUserType.ENROLLED) + + fake_unicode_start_time = u"üñîçø∂é_ßtå®t_tîµé" + mock_strftime_localized.return_value = fake_unicode_start_time + + url = course_home_url(self.course) + response = self.client.get(url) + expected_params = QueryDict(mutable=True) + expected_params['notlive'] = fake_unicode_start_time + expected_url = u'{url}?{params}'.format( + url=reverse('dashboard'), + params=expected_params.urlencode() + ) + self.assertRedirects(response, expected_url) + + def test_nonexistent_course(self): + """ + Ensure a non-existent course results in a 404. + """ + self.user = self.create_user_for_course(self.course, CourseUserType.ANONYMOUS) + + url = course_home_url_from_string('not/a/course') + response = self.client.get(url) + self.assertEqual(response.status_code, 404)