From 2167584e5ec0dee5c22fc657c96429808d8dcc56 Mon Sep 17 00:00:00 2001 From: Andy Armstrong Date: Thu, 27 Apr 2017 16:17:49 -0400 Subject: [PATCH] Unify the home and course tabs LEARNER-609 --- common/djangoapps/student/views.py | 7 +-- .../tests/lms/test_lms_course_home.py | 2 +- .../tests/test_field_override_performance.py | 54 +++++++++---------- lms/djangoapps/courseware/tabs.py | 20 +++---- .../courseware/tests/test_course_info.py | 4 +- lms/djangoapps/courseware/tests/test_tabs.py | 23 ++++++++ lms/djangoapps/courseware/tests/test_views.py | 10 ++-- lms/djangoapps/courseware/views/views.py | 18 +++++-- .../dashboard/_dashboard_course_listing.html | 3 +- .../registration_code_receipt.html | 4 +- lms/tests.py | 3 +- .../views/course_bookmarks.py | 4 +- .../features/course_experience/__init__.py | 28 +++++++++- .../course_experience/views/course_updates.py | 4 +- 14 files changed, 120 insertions(+), 64 deletions(-) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 381c51a9a3..0dd4f3cf52 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -116,6 +116,7 @@ from student.models import anonymous_id_for_user, UserAttribute, EnrollStatusCha from shoppingcart.models import DonationConfiguration, CourseRegistrationCode from openedx.core.djangoapps.embargo import api as embargo_api +from openedx.features.course_experience import course_home_url_name from openedx.features.enterprise_support.api import get_dashboard_consent_notification import analytics @@ -2194,12 +2195,12 @@ def auto_auth(request): # Redirect to specific page if specified if redirect_to: redirect_url = redirect_to - # Redirect to course info page if course_id is known + # Redirect to course home page if course_id is known elif course_id: try: - # redirect to course info page in LMS + # redirect to course home page in LMS redirect_url = reverse( - 'info', + course_home_url_name(request), kwargs={'course_id': course_id} ) except NoReverseMatch: diff --git a/common/test/acceptance/tests/lms/test_lms_course_home.py b/common/test/acceptance/tests/lms/test_lms_course_home.py index 2fe3184a7e..b5118f21df 100644 --- a/common/test/acceptance/tests/lms/test_lms_course_home.py +++ b/common/test/acceptance/tests/lms/test_lms_course_home.py @@ -64,7 +64,7 @@ class CourseHomeTest(CourseHomeBaseTest): def test_course_home(self): """ - Smoke test of course outline, breadcrumbs to and from cours outline, and bookmarks. + Smoke test of course outline, breadcrumbs to and from course outline, and bookmarks. """ self.course_home_page.visit() diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 08eb0387d4..0fba797f97 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -231,18 +231,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): # # of sql queries to default, # # of mongo queries, # ) - ('no_overrides', 1, True, False): (23, 1), - ('no_overrides', 2, True, False): (23, 1), - ('no_overrides', 3, True, False): (23, 1), - ('ccx', 1, True, False): (23, 1), - ('ccx', 2, True, False): (23, 1), - ('ccx', 3, True, False): (23, 1), - ('no_overrides', 1, False, False): (23, 1), - ('no_overrides', 2, False, False): (23, 1), - ('no_overrides', 3, False, False): (23, 1), - ('ccx', 1, False, False): (23, 1), - ('ccx', 2, False, False): (23, 1), - ('ccx', 3, False, False): (23, 1), + ('no_overrides', 1, True, False): (24, 1), + ('no_overrides', 2, True, False): (24, 1), + ('no_overrides', 3, True, False): (24, 1), + ('ccx', 1, True, False): (24, 1), + ('ccx', 2, True, False): (24, 1), + ('ccx', 3, True, False): (24, 1), + ('no_overrides', 1, False, False): (24, 1), + ('no_overrides', 2, False, False): (24, 1), + ('no_overrides', 3, False, False): (24, 1), + ('ccx', 1, False, False): (24, 1), + ('ccx', 2, False, False): (24, 1), + ('ccx', 3, False, False): (24, 1), } @@ -254,19 +254,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True TEST_DATA = { - ('no_overrides', 1, True, False): (23, 3), - ('no_overrides', 2, True, False): (23, 3), - ('no_overrides', 3, True, False): (23, 3), - ('ccx', 1, True, False): (23, 3), - ('ccx', 2, True, False): (23, 3), - ('ccx', 3, True, False): (23, 3), - ('ccx', 1, True, True): (24, 3), - ('ccx', 2, True, True): (24, 3), - ('ccx', 3, True, True): (24, 3), - ('no_overrides', 1, False, False): (23, 3), - ('no_overrides', 2, False, False): (23, 3), - ('no_overrides', 3, False, False): (23, 3), - ('ccx', 1, False, False): (23, 3), - ('ccx', 2, False, False): (23, 3), - ('ccx', 3, False, False): (23, 3), + ('no_overrides', 1, True, False): (24, 3), + ('no_overrides', 2, True, False): (24, 3), + ('no_overrides', 3, True, False): (24, 3), + ('ccx', 1, True, False): (24, 3), + ('ccx', 2, True, False): (24, 3), + ('ccx', 3, True, False): (24, 3), + ('ccx', 1, True, True): (25, 3), + ('ccx', 2, True, True): (25, 3), + ('ccx', 3, True, True): (25, 3), + ('no_overrides', 1, False, False): (24, 3), + ('no_overrides', 2, False, False): (24, 3), + ('no_overrides', 3, False, False): (24, 3), + ('ccx', 1, False, False): (24, 3), + ('ccx', 2, False, False): (24, 3), + ('ccx', 3, False, False): (24, 3), } diff --git a/lms/djangoapps/courseware/tabs.py b/lms/djangoapps/courseware/tabs.py index c8df7669f9..2e175a15ac 100644 --- a/lms/djangoapps/courseware/tabs.py +++ b/lms/djangoapps/courseware/tabs.py @@ -10,7 +10,7 @@ from django.utils.translation import ugettext as _, ugettext_noop from courseware.access import has_access from courseware.entrance_exams import user_can_skip_entrance_exam from openedx.core.lib.course_tabs import CourseTabPluginManager -from openedx.features.course_experience import UNIFIED_COURSE_VIEW_FLAG +from openedx.features.course_experience import defaut_course_url_name, UNIFIED_COURSE_EXPERIENCE_FLAG from request_cache.middleware import RequestCache from student.models import CourseEnrollment from xmodule.tabs import CourseTab, CourseTabList, key_checker, link_reverse_func @@ -39,23 +39,13 @@ class CoursewareTab(EnrolledTab): is_default = False supports_preview_menu = True - @staticmethod - def main_course_url_name(request): - """ - Returns the main course URL for the current user. - """ - if waffle.flag_is_active(request, UNIFIED_COURSE_VIEW_FLAG): - return 'openedx.course_experience.course_home' - else: - return 'courseware' - @property def link_func(self): """ Returns a function that computes the URL for this tab. """ request = RequestCache.get_current_request() - url_name = self.main_course_url_name(request) + url_name = defaut_course_url_name(request) return link_reverse_func(url_name) @@ -73,7 +63,11 @@ class CourseInfoTab(CourseTab): @classmethod def is_enabled(cls, course, user=None): - return True + """ + The "Home" tab is not shown for the new unified course experience. + """ + request = RequestCache.get_current_request() + return not waffle.flag_is_active(request, UNIFIED_COURSE_EXPERIENCE_FLAG) class SyllabusTab(EnrolledTab): diff --git a/lms/djangoapps/courseware/tests/test_course_info.py b/lms/djangoapps/courseware/tests/test_course_info.py index 3e35bd0efe..85b892983e 100644 --- a/lms/djangoapps/courseware/tests/test_course_info.py +++ b/lms/djangoapps/courseware/tests/test_course_info.py @@ -367,7 +367,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, 18, 4) + self.fetch_course_info_with_queries(self.instructor_paced_course, 20, 4) def test_num_queries_self_paced(self): - self.fetch_course_info_with_queries(self.self_paced_course, 18, 4) + self.fetch_course_info_with_queries(self.self_paced_course, 20, 4) diff --git a/lms/djangoapps/courseware/tests/test_tabs.py b/lms/djangoapps/courseware/tests/test_tabs.py index 70f4f8454b..c6157ac40e 100644 --- a/lms/djangoapps/courseware/tests/test_tabs.py +++ b/lms/djangoapps/courseware/tests/test_tabs.py @@ -2,6 +2,8 @@ Test cases for tabs. """ +import waffle + from django.core.urlresolvers import reverse from django.http import Http404 from mock import MagicMock, Mock, patch @@ -16,6 +18,7 @@ from courseware.tests.helpers import LoginEnrollmentTestCase from courseware.tests.factories import InstructorFactory, StaffFactory from courseware.views.views import get_static_tab_fragment, StaticCourseTabView from openedx.core.djangolib.testing.utils import get_mock_request +from openedx.features.course_experience import UNIFIED_COURSE_EXPERIENCE_FLAG from student.models import CourseEnrollment from student.tests.factories import UserFactory from util.milestones_helpers import ( @@ -766,6 +769,26 @@ class StaticTabTestCase(TabTestCase): self.check_get_and_set_method_for_key(tab, 'url_slug') +@attr(shard=1) +class CourseInfoTabTestCase(TabTestCase): + """Test cases for the course info tab.""" + def setUp(self): + self.user = self.create_mock_user() + self.request = get_mock_request(self.user) + + def test_default_tab(self): + # Verify that the course info tab is the first tab + tabs = get_course_tab_list(self.request, self.course) + self.assertEqual(tabs[0].type, 'course_info') + + @patch('waffle.flag_is_active') + def test_default_tab_for_new_course_experience(self, patched_flag_is_active): + # Verify that the unified course experience hides the course info tab + patched_flag_is_active.return_value = True + tabs = get_course_tab_list(self.request, self.course) + self.assertEqual(tabs[0].type, 'courseware') + + @attr(shard=1) class DiscussionLinkTestCase(TabTestCase): """Test cases for discussion link tab.""" diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 64022d42dc..4c3a2b7413 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -206,8 +206,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 141), - (ModuleStoreEnum.Type.split, 4, 141), + (ModuleStoreEnum.Type.mongo, 10, 142), + (ModuleStoreEnum.Type.split, 4, 142), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1408,12 +1408,12 @@ class ProgressPageTests(ModuleStoreTestCase): """Test that query counts remain the same for self-paced and instructor-paced courses.""" SelfPacedConfiguration(enabled=self_paced_enabled).save() self.setup_course(self_paced=self_paced) - with self.assertNumQueries(39), check_mongo_calls(1): + with self.assertNumQueries(40), check_mongo_calls(1): self._get_progress_page() @ddt.data( - (False, 39, 25), - (True, 32, 21) + (False, 40, 26), + (True, 33, 22) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 91bb15e3df..5e24a5466c 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -91,11 +91,15 @@ from openedx.core.djangoapps.programs.utils import ProgramMarketingDataExtender from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.self_paced.models import SelfPacedConfiguration from openedx.core.djangoapps.monitoring_utils import set_custom_metrics_for_course_key +from openedx.features.course_experience import ( + course_home_url_name, + UNIFIED_COURSE_EXPERIENCE_FLAG, + UNIFIED_COURSE_VIEW_FLAG, +) from openedx.features.enterprise_support.api import data_sharing_consent_required from shoppingcart.models import CourseRegistrationCode from shoppingcart.utils import is_shopping_cart_enabled from student.models import UserTestGroup, CourseEnrollment -from student.roles import GlobalStaff from survey.utils import must_answer_survey from util.cache import cache, cache_if_anonymous from util.date_utils import strftime_localized @@ -226,7 +230,7 @@ def jump_to(request, course_id, location): except InvalidKeyError: raise Http404(u"Invalid course_key or usage_key") try: - unified_course_view = waffle.flag_is_active(request, 'unified_course_view') + unified_course_view = waffle.flag_is_active(request, UNIFIED_COURSE_VIEW_FLAG) redirect_url = get_redirect_url(course_key, usage_key, unified_course_view=unified_course_view) except ItemNotFoundError: raise Http404(u"No data at this location: {0}".format(usage_key)) @@ -245,6 +249,10 @@ def course_info(request, course_id): Assumes the course_id is in a valid format. """ + # If the unified course experience is enabled, redirect to the "Course" tab + if waffle.flag_is_active(request, UNIFIED_COURSE_EXPERIENCE_FLAG): + return redirect(reverse(course_home_url_name(request), args=[course_id])) + course_key = CourseKey.from_string(course_id) with modulestore().bulk_operations(course_key): course = get_course_by_id(course_key, depth=2) @@ -636,7 +644,7 @@ def course_about(request, course_id): modes = CourseMode.modes_for_course_dict(course_key) if configuration_helpers.get_value('ENABLE_MKTG_SITE', settings.FEATURES.get('ENABLE_MKTG_SITE', False)): - return redirect(reverse('info', args=[course.id.to_deprecated_string()])) + return redirect(reverse(course_home_url_name(request), args=[unicode(course.id)])) registered = registered_for_course(course, request.user) @@ -644,7 +652,7 @@ def course_about(request, course_id): studio_url = get_studio_url(course, 'settings/details') if has_access(request.user, 'load', course): - course_target = reverse('info', args=[course.id.to_deprecated_string()]) + course_target = reverse(course_home_url_name(request), args=[course.id.to_deprecated_string()]) else: course_target = reverse('about_course', args=[course.id.to_deprecated_string()]) @@ -1208,7 +1216,7 @@ def course_survey(request, course_id): course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, 'load', course_key) - redirect_url = reverse('info', args=[course_id]) + redirect_url = reverse(course_home_url_name(request), args=[course_id]) # if there is no Survey associated with this course, # then redirect to the course instead diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 419d7aff45..53e178f89c 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -10,6 +10,7 @@ from course_modes.models import CourseMode from course_modes.helpers import enrollment_mode_display from openedx.core.djangolib.js_utils import dump_js_escaped_json, js_escaped_string from openedx.core.djangolib.markup import HTML, Text +from openedx.features.course_experience import course_home_url_name from student.helpers import ( VERIFY_STATUS_NEED_TO_VERIFY, VERIFY_STATUS_SUBMITTED, @@ -55,7 +56,7 @@ from util.course import get_link_for_about_page, get_encoded_course_sharing_utm_ % endif
- <% course_target = reverse('info', args=[unicode(course_overview.id)]) %> + <% course_target = reverse(course_home_url_name(), args=[unicode(course_overview.id)]) %>

${_('Course details')}

% if not reg_code_already_redeemed: %if redemption_success: - <% course_url = reverse('info', args=[course.id.to_deprecated_string()]) %> + <% course_url = reverse(course_home_url_name(), args=[course.id.to_deprecated_string()]) %> ${_("View Course")} %elif not registered_for_course:
diff --git a/lms/tests.py b/lms/tests.py index e6f15792cb..1e275e7acc 100644 --- a/lms/tests.py +++ b/lms/tests.py @@ -8,6 +8,7 @@ from django.core.urlresolvers import reverse from edxmako import add_lookup, LOOKUP from lms import startup +from openedx.features.course_experience import course_home_url_name from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -54,6 +55,6 @@ class HelpModalTests(ModuleStoreTestCase): Simple test to make sure that you don't get a 500 error when the modal is enabled. """ - url = reverse('info', args=[self.course.id.to_deprecated_string()]) + url = reverse(course_home_url_name(), args=[self.course.id.to_deprecated_string()]) resp = self.client.get(url) self.assertEqual(resp.status_code, 200) diff --git a/openedx/features/course_bookmarks/views/course_bookmarks.py b/openedx/features/course_bookmarks/views/course_bookmarks.py index 7d8d2a5e98..0846923a14 100644 --- a/openedx/features/course_bookmarks/views/course_bookmarks.py +++ b/openedx/features/course_bookmarks/views/course_bookmarks.py @@ -13,9 +13,9 @@ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.generic import View from courseware.courses import get_course_with_access -from lms.djangoapps.courseware.tabs import CoursewareTab from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.plugin_api.views import EdxFragmentView +from openedx.features.course_experience import defaut_course_url_name from util.views import ensure_valid_course_key from web_fragments.fragment import Fragment @@ -38,7 +38,7 @@ class CourseBookmarksView(View): """ course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) - course_url_name = CoursewareTab.main_course_url_name(request) + course_url_name = defaut_course_url_name(request) course_url = reverse(course_url_name, kwargs={'course_id': unicode(course.id)}) # Render the bookmarks list as a fragment diff --git a/openedx/features/course_experience/__init__.py b/openedx/features/course_experience/__init__.py index 800a50452d..48ac65f039 100644 --- a/openedx/features/course_experience/__init__.py +++ b/openedx/features/course_experience/__init__.py @@ -1,4 +1,10 @@ -# Unified course experience settings. +""" +Unified course experience settings and helper methods. +""" + +import waffle + +from request_cache.middleware import RequestCache # Waffle flag to enable a single unified "Course" tab. UNIFIED_COURSE_EXPERIENCE_FLAG = 'unified_course_experience' @@ -6,3 +12,23 @@ UNIFIED_COURSE_EXPERIENCE_FLAG = 'unified_course_experience' # Waffle flag to enable the full screen course content view # along with a unified course home page. UNIFIED_COURSE_VIEW_FLAG = 'unified_course_view' + + +def defaut_course_url_name(request=None): + """ + Returns the default course URL name for the current user. + """ + if waffle.flag_is_active(request or RequestCache.get_current_request(), UNIFIED_COURSE_VIEW_FLAG): + return 'openedx.course_experience.course_home' + else: + return 'courseware' + + +def course_home_url_name(request=None): + """ + Returns the course home page's URL name for the current user. + """ + if waffle.flag_is_active(request or RequestCache.get_current_request(), UNIFIED_COURSE_EXPERIENCE_FLAG): + return 'openedx.course_experience.course_home' + else: + return 'info' diff --git a/openedx/features/course_experience/views/course_updates.py b/openedx/features/course_experience/views/course_updates.py index 1080a1016f..e999633955 100644 --- a/openedx/features/course_experience/views/course_updates.py +++ b/openedx/features/course_experience/views/course_updates.py @@ -10,10 +10,10 @@ from django.utils.decorators import method_decorator from django.views.decorators.cache import cache_control from courseware.courses import get_course_info_section, get_course_with_access -from lms.djangoapps.courseware.tabs import CoursewareTab from lms.djangoapps.courseware.views.views import CourseTabView from opaque_keys.edx.keys import CourseKey from openedx.core.djangoapps.plugin_api.views import EdxFragmentView +from openedx.features.course_experience import defaut_course_url_name from web_fragments.fragment import Fragment @@ -45,7 +45,7 @@ class CourseUpdatesFragmentView(EdxFragmentView): """ course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True) - course_url_name = CoursewareTab.main_course_url_name(request) + course_url_name = defaut_course_url_name(request) course_url = reverse(course_url_name, kwargs={'course_id': unicode(course.id)}) # Fetch the updates as HTML