From 3c47d19a52f2ed24a823ddb8e44bccc6b0714154 Mon Sep 17 00:00:00 2001
From: Matthew Piatetsky <mpiatetsky@edx.org>
Date: Wed, 17 Oct 2018 13:00:40 -0400
Subject: [PATCH] add redirect behavior when accessing expired course and add
tests
---
common/djangoapps/student/tests/test_views.py | 40 +++++++++++++++++--
common/djangoapps/student/views/dashboard.py | 2 +
lms/djangoapps/courseware/access_response.py | 19 ++++++---
lms/djangoapps/courseware/courses.py | 10 +++++
.../dashboard/_dashboard_course_listing.html | 22 +++++-----
.../features/course_duration_limits/access.py | 19 +++++++--
.../tests/views/test_course_home.py | 30 ++++++++++++++
7 files changed, 119 insertions(+), 23 deletions(-)
diff --git a/common/djangoapps/student/tests/test_views.py b/common/djangoapps/student/tests/test_views.py
index 4766cca92de..7e6c321db2e 100644
--- a/common/djangoapps/student/tests/test_views.py
+++ b/common/djangoapps/student/tests/test_views.py
@@ -27,7 +27,10 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOvervi
from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory
from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration_context
from pyquery import PyQuery as pq
+from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG
+from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory
from openedx.core.djangoapps.user_authn.cookies import _get_user_info_cookie_data
+from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag
from student.helpers import DISABLE_UNENROLL_CERT_STATES
from student.models import CourseEnrollment, UserProfile
from student.signals import REFUND_ORDER
@@ -306,7 +309,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin,
'type': 'verified'
}
response = self.client.get(self.path)
- self.assertIn('class="enter-course hidden"', response.content)
+ self.assertIn('class="course-target-link enter-course hidden"', response.content)
self.assertIn('You must select a session to access the course.', response.content)
self.assertIn('<div class="course-entitlement-selection-container ">', response.content)
self.assertIn('Related Programs:', response.content)
@@ -580,7 +583,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin,
def _get_html_for_view_course_button(course_key_string, course_run_string):
return '''
<a href="/courses/{course_key}/course/"
- class="enter-course "
+ class="course-target-link enter-course"
data-course-key="{course_key}">
View Course
<span class="sr">
@@ -593,7 +596,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin,
def _get_html_for_resume_course_button(course_key_string, resume_block_key_string, course_run_string):
return '''
<a href="/courses/{course_key}/jump_to/{url_to_block}"
- class="enter-course "
+ class="course-target-link enter-course"
data-course-key="{course_key}">
Resume Course
<span class="sr">
@@ -718,6 +721,37 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin,
dashboard_html
)
+ @override_waffle_flag(COURSE_UPDATE_WAFFLE_FLAG, True)
+ def test_content_gating_course_card_changes(self):
+ """
+ When a course is expired, the links on the course card should be removed.
+ Links will be removed from the course title, course image and button (View Course/Resume Course).
+ The course card should have an access expired message.
+ """
+ self.override_waffle_switch(True)
+
+ course = CourseFactory.create(start=self.THREE_YEARS_AGO)
+ enrollment = CourseEnrollmentFactory.create(
+ user=self.user,
+ course_id=course.id
+ )
+ schedule = ScheduleFactory(start=self.THREE_YEARS_AGO, enrollment=enrollment)
+
+ response = self.client.get(reverse('dashboard'))
+ dashboard_html = self._remove_whitespace_from_html_string(response.content)
+ access_expired_substring = 'Accessexpired'
+ course_link_class = 'course-target-link'
+
+ self.assertNotIn(
+ course_link_class,
+ dashboard_html
+ )
+
+ self.assertIn(
+ access_expired_substring,
+ dashboard_html
+ )
+
def test_dashboard_with_resume_buttons_and_view_buttons(self):
'''
The Test creates a four-course-card dashboard. The user completes course
diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py
index 722b717fbd8..8a18d52e491 100644
--- a/common/djangoapps/student/views/dashboard.py
+++ b/common/djangoapps/student/views/dashboard.py
@@ -767,6 +767,8 @@ def student_dashboard(request):
redirect_message = _("The course you are looking for is closed for enrollment as of {date}.").format(
date=request.GET['course_closed']
)
+ elif 'expired_message' in request.GET:
+ redirect_message = request.GET['expired_message']
else:
redirect_message = ''
diff --git a/lms/djangoapps/courseware/access_response.py b/lms/djangoapps/courseware/access_response.py
index 4f254887e86..a12dfa05a71 100644
--- a/lms/djangoapps/courseware/access_response.py
+++ b/lms/djangoapps/courseware/access_response.py
@@ -9,7 +9,8 @@ from xmodule.course_metadata_utils import DEFAULT_START_DATE
class AccessResponse(object):
"""Class that represents a response from a has_access permission check."""
- def __init__(self, has_access, error_code=None, developer_message=None, user_message=None, user_fragment=None):
+ def __init__(self, has_access, error_code=None, developer_message=None, user_message=None,
+ detailed_user_message=None, user_fragment=None):
"""
Creates an AccessResponse object.
@@ -21,13 +22,16 @@ class AccessResponse(object):
to show the developer
user_message (String): optional - default is None. Message to
show the user
+ detailed_user_message (String): optional - default is None. Message to
+ show the user when additional context like the course name is necessary
user_fragment (:py:class:`~web_fragments.fragment.Fragment`): optional -
- An html fragment to display to the user if their access is denied.
+ An html fragment to display to the user if their access is denied
"""
self.has_access = has_access
self.error_code = error_code
self.developer_message = developer_message
self.user_message = user_message
+ self.detailed_user_message = detailed_user_message
self.user_fragment = user_fragment
if has_access:
assert error_code is None
@@ -58,15 +62,17 @@ class AccessResponse(object):
"error_code": self.error_code,
"developer_message": self.developer_message,
"user_message": self.user_message,
+ "detailed_user_message": self.detailed_user_message,
"user_fragment": self.user_fragment,
}
def __repr__(self):
- return "AccessResponse({!r}, {!r}, {!r}, {!r}, {!r})".format(
+ return "AccessResponse({!r}, {!r}, {!r}, {!r}, {!r}, {!r})".format(
self.has_access,
self.error_code,
self.developer_message,
self.user_message,
+ self.detailed_user_message,
self.user_fragment,
)
@@ -79,6 +85,7 @@ class AccessResponse(object):
self.error_code == other.error_code and
self.developer_message == other.developer_message and
self.user_message == other.user_message and
+ self.detailed_user_message == other.detailed_user_message and
self.user_fragment == other.user_fragment
)
@@ -89,7 +96,7 @@ class AccessError(AccessResponse):
denial in has_access. Contains the error code, user and developer
messages. Subclasses represent specific errors.
"""
- def __init__(self, error_code, developer_message, user_message, user_fragment=None):
+ def __init__(self, error_code, developer_message, user_message, detailed_user_message=None, user_fragment=None):
"""
Creates an AccessError object.
@@ -100,10 +107,12 @@ class AccessError(AccessResponse):
error_code (String): unique identifier for the specific type of
error developer_message (String): message to show the developer
user_message (String): message to show the user
+ detailed_user_message (String): message to show the user with additional context like the course name
user_fragment (:py:class:`~web_fragments.fragment.Fragment`): HTML to show the user
"""
- super(AccessError, self).__init__(False, error_code, developer_message, user_message, user_fragment)
+ super(AccessError, self).__init__(False, error_code, developer_message, user_message,
+ detailed_user_message, user_fragment)
class StartDateError(AccessError):
diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py
index b18820d14d1..a9d131b3922 100644
--- a/lms/djangoapps/courseware/courses.py
+++ b/lms/djangoapps/courseware/courses.py
@@ -8,6 +8,7 @@ from datetime import datetime
import branding
import pytz
+from openedx.features.course_duration_limits.access import AuditExpiredError
from courseware.access import has_access
from courseware.access_response import StartDateError, MilestoneAccessError
from courseware.date_summary import (
@@ -143,6 +144,15 @@ def check_course_access(course, user, action, check_if_enrolled=False, check_sur
params=params.urlencode()
), access_response)
+ # Redirect if AuditExpiredError
+ if isinstance(access_response, AuditExpiredError):
+ params = QueryDict(mutable=True)
+ params['expired_message'] = access_response.detailed_user_message
+ raise CourseAccessRedirect('{dashboard_url}?{params}'.format(
+ dashboard_url=reverse('dashboard'),
+ params=params.urlencode()
+ ), access_response)
+
# Redirect if the user must answer a survey before entering the course.
if isinstance(access_response, MilestoneAccessError):
raise CourseAccessRedirect('{dashboard_url}'.format(
diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html
index e1472f3a8fc..def15e99e4f 100644
--- a/lms/templates/dashboard/_dashboard_course_listing.html
+++ b/lms/templates/dashboard/_dashboard_course_listing.html
@@ -34,7 +34,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_
cert_name_long = settings.CERT_NAME_LONG
billing_email = settings.PAYMENT_SUPPORT_EMAIL
- is_course_expired = getattr(show_courseware_link, 'error_code') == 'audit_expired'
+ is_course_expired = hasattr(show_courseware_link, 'error_code') and show_courseware_link.error_code == 'audit_expired'
%>
<%namespace name='static' file='../static_content.html'/>
@@ -68,7 +68,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_
<div class="wrapper-course-image" aria-hidden="true">
% if show_courseware_link and not is_unfulfilled_entitlement:
% if not is_course_blocked and not is_course_expired:
- <a href="${course_target}" data-course-key="${enrollment.course_id}" class="cover" tabindex="-1">
+ <a href="${course_target}" data-course-key="${enrollment.course_id}" class="cover course-target-link" tabindex="-1">
<img src="${course_overview.image_urls['small']}" class="course-image" alt="${_('{course_number} {course_name} Home Page').format(course_number=course_overview.number, course_name=course_overview.display_name_with_default)}" />
</a>
% else:
@@ -95,7 +95,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_
<h3 class="course-title" id="course-title-${enrollment.course_id}">
% if show_courseware_link and not is_unfulfilled_entitlement:
% if not is_course_blocked and not is_course_expired:
- <a data-course-key="${enrollment.course_id}" href="${course_target}">${course_overview.display_name_with_default}</a>
+ <a data-course-key="${enrollment.course_id}" href="${course_target}" class="course-target-link">${course_overview.display_name_with_default}</a>
% else:
<a class="disable-look" data-course-key="${enrollment.course_id}">${course_overview.display_name_with_default}</a>
% endif
@@ -130,7 +130,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_
%>
<span class="info-date-block-container">
- % if is_course_expired:
+ % if not is_unfulfilled_entitlement and is_course_expired:
<span class="info-date-block" data-course-key="${enrollment.course_id}">
${show_courseware_link.user_message}
<span class="sr">
@@ -174,27 +174,27 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_
</div>
<div class="wrapper-course-actions">
<div class="course-actions">
- % if show_courseware_link or is_unfulfilled_entitlement:
+ % if (show_courseware_link or is_unfulfilled_entitlement) and not is_course_expired:
% if course_overview.has_ended():
- % if not is_course_blocked and not is_course_expired:
- <a href="${course_target}" class="enter-course archived" data-course-key="${enrollment.course_id}">${_('View Archived Course')}<span class="sr"> ${course_overview.display_name_with_default}</span></a>
+ % if not is_course_blocked:
+ <a href="${course_target}" class="enter-course archived course-target-link" data-course-key="${enrollment.course_id}">${_('View Archived Course')}<span class="sr"> ${course_overview.display_name_with_default}</span></a>
% else:
- <a class="enter-course-blocked archived" data-course-key="${enrollment.course_id}">${_('View Archived Course')}<span class="sr"> ${course_overview.display_name_with_default}</span></a>
+ <a class="enter-course-blocked archived course-target-link" data-course-key="${enrollment.course_id}">${_('View Archived Course')}<span class="sr"> ${course_overview.display_name_with_default}</span></a>
% endif
% else:
% if resume_button_url != '':
<a href="${resume_button_url}"
- class="enter-course ${'hidden' if is_unfulfilled_entitlement else ''}"
+ class="course-target-link enter-course ${'hidden' if is_unfulfilled_entitlement else ''}"
data-course-key="${enrollment.course_id}">
${_('Resume Course')}
<span class="sr">
${course_overview.display_name_with_default}
</span>
</a>
- % elif not is_course_blocked and not is_course_expired:
+ % elif not is_course_blocked:
<a href="${course_target}"
- class="enter-course ${'hidden' if is_unfulfilled_entitlement else ''}"
+ class="course-target-link enter-course ${'hidden' if is_unfulfilled_entitlement else ''}"
data-course-key="${enrollment.course_id}">
${_('View Course')}
<span class="sr">
diff --git a/openedx/features/course_duration_limits/access.py b/openedx/features/course_duration_limits/access.py
index 27fad2fe2f3..0ce4b236868 100644
--- a/openedx/features/course_duration_limits/access.py
+++ b/openedx/features/course_duration_limits/access.py
@@ -9,9 +9,10 @@ from django.apps import apps
from django.utils import timezone
from django.utils.translation import ugettext as _
-
+from util.date_utils import DEFAULT_SHORT_DATE_FORMAT, strftime_localized
from lms.djangoapps.courseware.access_response import AccessError
from lms.djangoapps.courseware.access_utils import ACCESS_GRANTED
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
class AuditExpiredError(AccessError):
@@ -21,9 +22,19 @@ class AuditExpiredError(AccessError):
def __init__(self, user, course, expiration_date):
error_code = "audit_expired"
developer_message = "User {} had access to {} until {}".format(user, course, expiration_date)
- # TODO: Translate the expiration_date
- user_message = _("Course access expired on ") + expiration_date.strftime("%B %d, %Y")
- super(AuditExpiredError, self).__init__(error_code, developer_message, user_message)
+ expiration_date = strftime_localized(expiration_date, DEFAULT_SHORT_DATE_FORMAT)
+ user_message = _("Access expired on {expiration_date}").format(expiration_date=expiration_date)
+ try:
+ course_name = CourseOverview.get_from_id(course.id).display_name_with_default
+ detailed_user_message = _("Access to {course_name} expired on {expiration_date}").format(
+ course_name=course_name,
+ expiration_date=expiration_date
+ )
+ except CourseOverview.DoesNotExist:
+ detailed_user_message = _("Access to the course you were looking for expired on {expiration_date}").format(
+ expiration_date=expiration_date
+ )
+ super(AuditExpiredError, self).__init__(error_code, developer_message, user_message, detailed_user_message)
def get_user_course_expiration_date(user, course):
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 de380d2968a..3cf29180072 100644
--- a/openedx/features/course_experience/tests/views/test_course_home.py
+++ b/openedx/features/course_experience/tests/views/test_course_home.py
@@ -20,6 +20,8 @@ from courseware.tests.factories import StaffFactory
from lms.djangoapps.commerce.models import CommerceConfiguration
from lms.djangoapps.commerce.utils import EcommerceService
from lms.djangoapps.course_goals.api import add_course_goal, remove_course_goal
+from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
+from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory
from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES, override_waffle_flag
from openedx.features.course_experience import (
SHOW_REVIEWS_TOOL_FLAG,
@@ -317,6 +319,34 @@ class TestCourseHomePageAccess(CourseHomePageTestCase):
)
self.assertRedirects(response, expected_url)
+ @mock.patch.dict(settings.FEATURES, {'DISABLE_START_DATES': False})
+ def test_expired_course(self):
+ """
+ Ensure that a user accessing an expired course sees a redirect to
+ the student dashboard, not a 404.
+ """
+ three_years_ago = now() - timedelta(days=(365 * 3))
+ course = CourseFactory.create(start=three_years_ago)
+ user = self.create_user_for_course(course, CourseUserType.ENROLLED)
+ enrollment = CourseEnrollment.get_enrollment(user, course.id)
+ ScheduleFactory(start=three_years_ago, enrollment=enrollment)
+
+ url = course_home_url(course)
+ response = self.client.get(url)
+
+ expiration_date = strftime_localized(course.start + timedelta(weeks=8), 'SHORT_DATE')
+ expected_params = QueryDict(mutable=True)
+ course_name = CourseOverview.get_from_id(course.id).display_name_with_default
+ expected_params['expired_message'] = 'Access to {run} expired on {expiration_date}'.format(
+ run=course_name,
+ expiration_date=expiration_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):
--
GitLab