From be346499da1faf1f78781d6fce08c00cf0742117 Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Thu, 9 Jul 2020 13:59:57 -0700 Subject: [PATCH] AA-219: Dates Tab behavior improvements This switches the Dates Tab to be an enrolled tab allowing only enrolled learners to view. Additionally, it will now redirect logged out learners to the login page if they hit the Dates Tab directly. --- lms/djangoapps/courseware/tabs.py | 3 +- .../courseware/tests/test_date_summary.py | 33 +++++++++++----- lms/djangoapps/courseware/tests/test_tabs.py | 39 +++++++++++++------ lms/djangoapps/courseware/tests/test_views.py | 30 +++++++++++++- lms/djangoapps/courseware/views/views.py | 14 ++----- .../course-dates-fragment.html | 8 ++-- .../course_experience/views/course_dates.py | 8 ++++ 7 files changed, 98 insertions(+), 37 deletions(-) diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index 660d99be69..57962266b5 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -13,7 +13,6 @@ from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.entrance_exams import user_can_skip_entrance_exam from lms.djangoapps.course_home_api.toggles import course_home_mfe_dates_tab_is_active from lms.djangoapps.course_home_api.utils import get_microfrontend_url -from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.lib.course_tabs import CourseTabPluginManager from openedx.features.course_experience import RELATIVE_DATES_FLAG, UNIFIED_COURSE_TAB_FLAG, default_course_url_name from student.models import CourseEnrollment @@ -205,7 +204,7 @@ class LinkTab(CourseTab): """ link_value = '' - def __init__(self, tab_dict=None, name=None, link=None): + def __init__(self, tab_dict=None, link=None): self.link_value = tab_dict['link'] if tab_dict else link def link_value_func(_course, _reverse_func): diff --git a/lms/djangoapps/courseware/tests/test_date_summary.py b/lms/djangoapps/courseware/tests/test_date_summary.py index 7ea8e94914..36e821a739 100644 --- a/lms/djangoapps/courseware/tests/test_date_summary.py +++ b/lms/djangoapps/courseware/tests/test_date_summary.py @@ -663,18 +663,33 @@ class CourseDateSummaryTest(SharedModuleStoreTestCase): ) @override_waffle_flag(UNIFIED_COURSE_TAB_FLAG, active=True) def test_dates_tab_link_render(self, url_name): - with freeze_time('2015-01-02'): - course = create_course_run() - user = create_user() + """ The dates tab link should only show for enrolled or staff users """ + course = create_course_run() + html_elements = [ + 'class="dates-tab-link"', + 'View all course dates', + ] + url = reverse(url_name, args=(course.id,)) + + def assert_html_elements(assert_function, user): self.client.login(username=user.username, password=TEST_PASSWORD) - url = reverse(url_name, args=(course.id,)) response = self.client.get(url, follow=True) - html_elements = [ - 'class="dates-tab-link"', - 'View all course dates', - ] for html in html_elements: - self.assertContains(response, html) + assert_function(response, html) + self.client.logout() + + with freeze_time('2015-01-02'): + unenrolled_user = create_user() + assert_html_elements(self.assertNotContains, unenrolled_user) + + staff_user = create_user() + staff_user.is_staff = True + staff_user.save() + assert_html_elements(self.assertContains, staff_user) + + enrolled_user = create_user() + CourseEnrollmentFactory(course_id=course.id, user=enrolled_user, mode=CourseMode.VERIFIED) + assert_html_elements(self.assertContains, enrolled_user) @ddt.ddt diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index b5d621beff..fa4c88826b 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -164,7 +164,6 @@ class TabTestCase(SharedModuleStoreTestCase): if for_authenticated_users_only: user = self.create_mock_user(is_staff=False, is_enrolled=False) self.assertEqual(expected_value, self.is_tab_enabled(tab, self.course, user)) - assert False if not for_staff_only and not for_authenticated_users_only and not for_enrolled_users_only: user = AnonymousUser() self.assertEqual(expected_value, self.is_tab_enabled(tab, self.course, user)) @@ -903,9 +902,29 @@ class DiscussionLinkTestCase(TabTestCase): class DatesTabTestCase(TabListTestCase): - """Test cases for making sure no persisted dates tab is surfaced""" + """Test cases for dates tab""" - def test_singular_dates_tab(self): + @patch('lms.djangoapps.courseware.tabs.RELATIVE_DATES_FLAG') + @patch('student.models.CourseEnrollment.is_enrolled') + def test_dates_tab_disabled_if_unenrolled(self, is_enrolled, mock_flag): + mock_flag.is_enabled().return_value = True + tab = DatesTab({'type': DatesTab.type, 'name': 'dates'}) + + is_enrolled.return_value = False + unenrolled_user = self.create_mock_user(is_staff=False, is_enrolled=False) + self.assertFalse(self.is_tab_enabled(tab, self.course, unenrolled_user)) + + staff_user = self.create_mock_user(is_staff=True, is_enrolled=False) + self.assertTrue(self.is_tab_enabled(tab, self.course, staff_user)) + + is_enrolled.return_value = True + enrolled_user = self.create_mock_user(is_staff=False, is_enrolled=True) + self.assertTrue(self.is_tab_enabled(tab, self.course, enrolled_user)) + + @patch('lms.djangoapps.courseware.tabs.RELATIVE_DATES_FLAG') + def test_singular_dates_tab(self, mock_flag): + """Test cases for making sure no persisted dates tab is surfaced""" + mock_flag.is_enabled().return_value = True user = self.create_mock_user() self.course.tabs = self.all_valid_tab_list self.course.save() @@ -918,11 +937,9 @@ class DatesTabTestCase(TabListTestCase): self.assertTrue(has_dates_tab) # Verify that there is only 1 'dates' tab in the returned result from get_course_tab_list() - with patch('lms.djangoapps.courseware.tabs.RELATIVE_DATES_FLAG') as mock_flag: - mock_flag.is_enabled().return_value = True - tabs = get_course_tab_list(user, self.course) - num_dates_tabs = 0 - for tab in tabs: - if tab.type == 'dates': - num_dates_tabs += 1 - self.assertEqual(num_dates_tabs, 1) + tabs = get_course_tab_list(user, self.course) + num_dates_tabs = 0 + for tab in tabs: + if tab.type == 'dates': + num_dates_tabs += 1 + self.assertEqual(num_dates_tabs, 1) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index fa29d0b48d..f70d8f6f6e 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3098,9 +3098,37 @@ class DatesTabTestCase(ModuleStoreTestCase): ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2017, 1, 1)) def _get_response(self, course): - """ Returns the HTML for the progress page """ + """ Returns the HTML for the dates page """ return self.client.get(reverse('dates', args=[six.text_type(course.id)])) + def test_tab_redirects_if_not_logged_in(self): + self.client.logout() + response = self._get_response(self.course) + self.assertEqual(response.status_code, 302) + self.assertIn('/login?next=/courses/', response.url) + + def test_tab_redirects_if_not_enrolled_and_not_staff(self): + response = self._get_response(self.course) + self.assertEqual(response.status_code, 302) + # Beginning of redirect URL + self.assertIn('/courses/', response.url) + # End of redirect URL + self.assertIn('/course/', response.url) + + # Now check staff users can see + self.user.is_staff = True + self.user.save() + response = self._get_response(self.course) + self.assertEqual(response.status_code, 200) + + # Enrolled users can also see + self.client.logout() + enrolled_user = UserFactory() + CourseEnrollmentFactory(course_id=self.course.id, user=enrolled_user, mode=CourseMode.VERIFIED) + self.client.login(username=enrolled_user.username, password=TEST_PASSWORD) + response = self._get_response(self.course) + self.assertEqual(response.status_code, 200) + @RELATIVE_DATES_FLAG.override(active=True) @patch('edx_django_utils.monitoring.set_custom_metric') def test_defaults(self, mock_set_custom_metric): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 4858c19dc5..b795c0e6c8 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -21,7 +21,6 @@ from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpRespo from django.shortcuts import redirect from django.template.context_processors import csrf from django.urls import reverse -from django.utils import timezone from django.utils.decorators import method_decorator from django.utils.http import urlquote_plus from django.utils.text import slugify @@ -111,12 +110,7 @@ from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.mobile_utils import is_request_from_mobile_app from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.access import generate_course_expired_fragment -from openedx.features.course_experience import ( - COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, - RELATIVE_DATES_FLAG, - UNIFIED_COURSE_TAB_FLAG, - course_home_url_name -) +from openedx.features.course_experience import UNIFIED_COURSE_TAB_FLAG, course_home_url_name from openedx.features.course_experience.course_tools import CourseToolsPluginManager from openedx.features.course_experience.utils import dates_banner_should_display from openedx.features.course_experience.views.course_dates import CourseDatesFragmentView @@ -735,9 +729,6 @@ class CourseTabView(EdxFragmentView): """ Creates the context for the fragment's template. """ - from lms.urls import RESET_COURSE_DEADLINES_NAME - from openedx.features.course_experience.urls import COURSE_HOME_VIEW_NAME - can_masquerade = request.user.has_perm(MASQUERADE_AS_STUDENT, course) supports_preview_menu = tab.get('supports_preview_menu', False) uses_bootstrap = self.uses_bootstrap(request, course, tab=tab) @@ -1031,6 +1022,7 @@ def program_marketing(request, program_uuid): return render_to_response('courseware/program_marketing.html', context) +@login_required @ensure_csrf_cookie @ensure_valid_course_key def dates(request, course_id): @@ -1061,7 +1053,7 @@ def dates(request, course_id): request.user = masquerade_user user_is_enrolled = CourseEnrollment.is_enrolled(request.user, course_key) - user_is_staff = has_access(request.user, 'staff', course_key) + user_is_staff = bool(has_access(request.user, 'staff', course_key)) # Render the full content to enrolled users, as well as to course and global staff. # Unenrolled users who are not course or global staff are redirected to the Outline Tab. diff --git a/openedx/features/course_experience/templates/course_experience/course-dates-fragment.html b/openedx/features/course_experience/templates/course_experience/course-dates-fragment.html index 2ca2de0154..5787fde1e0 100644 --- a/openedx/features/course_experience/templates/course_experience/course-dates-fragment.html +++ b/openedx/features/course_experience/templates/course_experience/course-dates-fragment.html @@ -13,9 +13,11 @@ from django.utils.translation import ugettext as _ % for course_date_block in course_date_blocks: <%include file="dates-summary.html" args="course_date=course_date_block" /> % endfor -
- View all course dates -
+ % if user_enrolled: +
+ View all course dates +
+ % endif % endif <%static:require_module_async module_name="js/dateutil_factory" class_name="DateUtilFactory"> diff --git a/openedx/features/course_experience/views/course_dates.py b/openedx/features/course_experience/views/course_dates.py index f241ba8a83..3fc0ac270c 100644 --- a/openedx/features/course_experience/views/course_dates.py +++ b/openedx/features/course_experience/views/course_dates.py @@ -10,8 +10,10 @@ from django.utils.translation import get_language_bidi from opaque_keys.edx.keys import CourseKey from web_fragments.fragment import Fragment +from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.courses import get_course_date_blocks, get_course_with_access from openedx.core.djangoapps.plugin_api.views import EdxFragmentView +from student.models import CourseEnrollment class CourseDatesFragmentView(EdxFragmentView): @@ -27,10 +29,16 @@ class CourseDatesFragmentView(EdxFragmentView): course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=False) course_date_blocks = get_course_date_blocks(course, request.user, request, num_assignments=1) + # We will use this boolean to gate if we show a link to the dates tab. This same logic + # dictates if we show the tab at all. + user_enrolled = (request.user and request.user.is_authenticated and + (bool(CourseEnrollment.is_enrolled(request.user, course.id) or + has_access(request.user, 'staff', course, course.id)))) context = { 'course_date_blocks': [block for block in course_date_blocks if block.title != 'current_datetime'], 'dates_tab_link': reverse('dates', args=[course.id]), + 'user_enrolled': user_enrolled, } html = render_to_string(self.template_name, context) dates_fragment = Fragment(html)