From 8087a1d165649b65907df5ed12df5940d8ff3462 Mon Sep 17 00:00:00 2001 From: Jillian Vogel Date: Wed, 28 Jun 2017 19:06:24 +0930 Subject: [PATCH] Adds settings.FEATURES.ENABLE_SEPARATE_ARCHIVED_COURSES Separates archived courses into a different tab on the Studio index page. Data is rendered synchronously, but tests are added to ensure that no new sql or mongo queries were required. --- cms/djangoapps/contentstore/views/course.py | 39 +++-- .../views/tests/test_course_index.py | 135 +++++++++++++++++- cms/static/js/index.js | 2 + cms/static/sass/views/_dashboard.scss | 6 +- cms/templates/index.html | 45 +++++- common/lib/xmodule/xmodule/course_module.py | 17 ++- .../modulestore/tests/test_semantics.py | 2 +- 7 files changed, 225 insertions(+), 21 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 87904bc8fc..5a84ea0911 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -518,11 +518,13 @@ def course_listing(request): u'can_edit': has_studio_write_access(request.user, library.location.library_key), } - courses_iter = _remove_in_process_courses(courses_iter, in_process_course_actions) + split_archived = settings.FEATURES.get(u'ENABLE_SEPARATE_ARCHIVED_COURSES', False) + active_courses, archived_courses = _process_courses_list(courses_iter, in_process_course_actions, split_archived) in_process_course_actions = [format_in_process_course_view(uca) for uca in in_process_course_actions] return render_to_response(u'index.html', { - u'courses': list(courses_iter), + u'courses': active_courses, + u'archived_courses': archived_courses, u'in_process_course_actions': in_process_course_actions, u'libraries_enabled': LIBRARIES_ENABLED, u'libraries': [format_library_for_view(lib) for lib in libraries], @@ -639,7 +641,6 @@ def get_courses_accessible_to_user(request, org=None): the courses returned. A value of None will have no effect (all courses returned), an empty string will result in no courses, and otherwise only courses with the specified org will be returned. The default value is None. - """ if GlobalStaff().has_user(request.user): # user has global access so no need to get courses from django groups @@ -654,10 +655,15 @@ def get_courses_accessible_to_user(request, org=None): return courses, in_process_course_actions -def _remove_in_process_courses(courses_iter, in_process_course_actions): +def _process_courses_list(courses_iter, in_process_course_actions, split_archived=False): """ - removes any in-process courses in courses list. in-process actually refers to courses - that are in the process of being generated for re-run + Iterates over the list of courses to be displayed to the user, and: + + * Removes any in-process courses from the courses list. "In-process" refers to courses + that are in the process of being generated for re-run. + * If split_archived=True, removes any archived courses and returns them in a separate list. + Archived courses have has_ended() == True. + * Formats the returned courses (in both lists) to prepare them for rendering to the view. """ def format_course_for_view(course): """ @@ -675,11 +681,20 @@ def _remove_in_process_courses(courses_iter, in_process_course_actions): } in_process_action_course_keys = {uca.course_key for uca in in_process_course_actions} - return ( - format_course_for_view(course) - for course in courses_iter - if not isinstance(course, ErrorDescriptor) and (course.id not in in_process_action_course_keys) - ) + active_courses = [] + archived_courses = [] + + for course in courses_iter: + if isinstance(course, ErrorDescriptor) or (course.id in in_process_action_course_keys): + continue + + formatted_course = format_course_for_view(course) + if split_archived and course.has_ended(): + archived_courses.append(formatted_course) + else: + active_courses.append(formatted_course) + + return active_courses, archived_courses def course_outline_initial_state(locator_to_show, course_structure): @@ -1035,7 +1050,7 @@ def settings_handler(request, course_key_string): # exclude current course from the list of available courses courses = (course for course in courses if course.id != course_key) if courses: - courses = _remove_in_process_courses(courses, in_process_course_actions) + courses, __ = _process_courses_list(courses, in_process_course_actions) settings_context.update({'possible_pre_requisite_courses': list(courses)}) if credit_eligibility_enabled: diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index 83636e0e6b..27ebb554b2 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -10,6 +10,7 @@ import mock import pytz from django.conf import settings from django.core.exceptions import PermissionDenied +from django.test.utils import override_settings from django.utils.translation import ugettext as _ from opaque_keys.edx.locator import CourseLocator from search.api import perform_search @@ -26,13 +27,13 @@ from contentstore.views.item import VisibilityState, create_xblock_info from course_action_state.managers import CourseRerunUIStateManager from course_action_state.models import CourseRerunState from student.auth import has_course_author_access -from student.roles import LibraryUserRole +from student.roles import CourseStaffRole, GlobalStaff, LibraryUserRole from student.tests.factories import UserFactory from util.date_utils import get_default_time_display from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, LibraryFactory, check_mongo_calls class TestCourseIndex(CourseTestCase): @@ -328,6 +329,136 @@ class TestCourseIndex(CourseTestCase): self.assertIn('display_course_number: ""', response.content) +@ddt.ddt +class TestCourseIndexArchived(CourseTestCase): + """ + Unit tests for testing the course index list when there are archived courses. + """ + NOW = datetime.datetime.now(pytz.utc) + DAY = datetime.timedelta(days=1) + YESTERDAY = NOW - DAY + TOMORROW = NOW + DAY + + ORG = 'MyOrg' + + ENABLE_SEPARATE_ARCHIVED_COURSES = settings.FEATURES.copy() + ENABLE_SEPARATE_ARCHIVED_COURSES['ENABLE_SEPARATE_ARCHIVED_COURSES'] = True + DISABLE_SEPARATE_ARCHIVED_COURSES = settings.FEATURES.copy() + DISABLE_SEPARATE_ARCHIVED_COURSES['ENABLE_SEPARATE_ARCHIVED_COURSES'] = False + + def setUp(self): + """ + Add courses with the end date set to various values + """ + super(TestCourseIndexArchived, self).setUp() + + # Base course has no end date (so is active) + self.course.end = None + self.course.display_name = 'Active Course 1' + self.ORG = self.course.location.org + self.save_course() + + # Active course has end date set to tomorrow + self.active_course = CourseFactory.create( + display_name='Active Course 2', + org=self.ORG, + end=self.TOMORROW, + ) + + # Archived course has end date set to yesterday + self.archived_course = CourseFactory.create( + display_name='Archived Course', + org=self.ORG, + end=self.YESTERDAY, + ) + + # Base user has global staff access + self.assertTrue(GlobalStaff().has_user(self.user)) + + # Staff user just has course staff access + self.staff, self.staff_password = self.create_non_staff_user() + for course in (self.course, self.active_course, self.archived_course): + CourseStaffRole(course.id).add_users(self.staff) + + def check_index_page_with_query_count(self, separate_archived_courses, org, mongo_queries, sql_queries): + """ + Checks the index page, and ensures the number of database queries is as expected. + """ + with self.assertNumQueries(sql_queries): + with check_mongo_calls(mongo_queries): + self.check_index_page(separate_archived_courses=separate_archived_courses, org=org) + + def check_index_page(self, separate_archived_courses, org): + """ + Ensure that the index page displays the archived courses as expected. + """ + index_url = '/home/' + index_params = {} + if org is not None: + index_params['org'] = org + index_response = self.client.get(index_url, index_params, HTTP_ACCEPT='text/html') + self.assertEquals(index_response.status_code, 200) + + parsed_html = lxml.html.fromstring(index_response.content) + course_tab = parsed_html.find_class('courses') + self.assertEqual(len(course_tab), 1) + course_links = course_tab[0].find_class('course-link') + course_titles = course_tab[0].find_class('course-title') + archived_course_tab = parsed_html.find_class('archived-courses') + + if separate_archived_courses: + # Archived courses should be separated from the main course list + self.assertEqual(len(archived_course_tab), 1) + archived_course_links = archived_course_tab[0].find_class('course-link') + archived_course_titles = archived_course_tab[0].find_class('course-title') + self.assertEqual(len(archived_course_links), 1) + self.assertEqual(len(archived_course_titles), 1) + self.assertEqual(archived_course_titles[0].text, 'Archived Course') + + self.assertEqual(len(course_links), 2) + self.assertEqual(len(course_titles), 2) + self.assertEqual(course_titles[0].text, 'Active Course 1') + self.assertEqual(course_titles[1].text, 'Active Course 2') + else: + # Archived courses should be included in the main course list + self.assertEqual(len(archived_course_tab), 0) + self.assertEqual(len(course_links), 3) + self.assertEqual(len(course_titles), 3) + self.assertEqual(course_titles[0].text, 'Active Course 1') + self.assertEqual(course_titles[1].text, 'Active Course 2') + self.assertEqual(course_titles[2].text, 'Archived Course') + + @ddt.data( + # Staff user has course staff access + (True, 'staff', None, 4, 20), + (False, 'staff', None, 4, 20), + # Base user has global staff access + (True, 'user', ORG, 3, 16), + (False, 'user', ORG, 3, 16), + (True, 'user', None, 3, 16), + (False, 'user', None, 3, 16), + ) + @ddt.unpack + def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): + """ + Ensure that archived courses are shown as expected for all user types, when the feature is enabled/disabled. + Also ensure that enabling the feature does not adversely affect the database query count. + """ + # Authenticate the requested user + user = getattr(self, username) + password = getattr(self, username + '_password') + self.client.login(username=user, password=password) + + # Enable/disable the feature before viewing the index page. + features = settings.FEATURES.copy() + features['ENABLE_SEPARATE_ARCHIVED_COURSES'] = separate_archived_courses + with override_settings(FEATURES=features): + self.check_index_page_with_query_count(separate_archived_courses=separate_archived_courses, + org=org, + mongo_queries=mongo_queries, + sql_queries=sql_queries) + + @ddt.ddt class TestCourseOutline(CourseTestCase): """ diff --git a/cms/static/js/index.js b/cms/static/js/index.js index ae88a24dcc..70def59cad 100644 --- a/cms/static/js/index.js +++ b/cms/static/js/index.js @@ -161,6 +161,7 @@ define(['domReady', 'jquery', 'underscore', 'js/utils/cancel_on_escape', 'js/vie return function(e) { e.preventDefault(); $('.courses-tab').toggleClass('active', tab === 'courses'); + $('.archived-courses-tab').toggleClass('active', tab === 'archived-courses'); $('.libraries-tab').toggleClass('active', tab === 'libraries'); // Also toggle this course-related notice shown below the course tab, if it is present: @@ -179,6 +180,7 @@ define(['domReady', 'jquery', 'underscore', 'js/utils/cancel_on_escape', 'js/vie $('.action-reload').bind('click', ViewUtils.reload); $('#course-index-tabs .courses-tab').bind('click', showTab('courses')); + $('#course-index-tabs .archived-courses-tab').bind('click', showTab('archived-courses')); $('#course-index-tabs .libraries-tab').bind('click', showTab('libraries')); }; diff --git a/cms/static/sass/views/_dashboard.scss b/cms/static/sass/views/_dashboard.scss index c5c9372fe1..af144c0d75 100644 --- a/cms/static/sass/views/_dashboard.scss +++ b/cms/static/sass/views/_dashboard.scss @@ -318,7 +318,7 @@ } // ELEM: course listings - .courses-tab, .libraries-tab { + .courses-tab, .archived-courses-tab, .libraries-tab { display: none; &.active { @@ -326,7 +326,7 @@ } } - .courses, .libraries { + .courses, .libraries, .archived-courses { .title { @extend %t-title6; margin-bottom: $baseline; @@ -342,7 +342,7 @@ padding-bottom: ($baseline/2); color: $gray-l2; } - } + } .list-courses { border-radius: 3px; diff --git a/cms/templates/index.html b/cms/templates/index.html index 9a226aced3..e19078b656 100644 --- a/cms/templates/index.html +++ b/cms/templates/index.html @@ -308,10 +308,15 @@ from openedx.core.djangolib.markup import HTML, Text %endif - % if libraries_enabled: + % if libraries_enabled or archived_courses: % endif @@ -467,6 +472,44 @@ from openedx.core.djangolib.markup import HTML, Text % endif + %if archived_courses: +
+ +
+ %endif + %if len(libraries) > 0: