Merge pull request #15548 from edx/robrap/LEARNER-613-handle-start-dates

LEARNER-613: Redirect Course Home for course that hasn't started.

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)

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)

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):

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)]))

openedx/features/course_experience/tests/views/test_course_home.py

+# 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)