Merge pull request #15431 from open-craft/jill/studio-filter-archived-courses

Separate Archived courses into a separate list in Studio

cms/djangoapps/contentstore/views/course.py

@@ -524,11 +524,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],
@@ -645,7 +647,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
@@ -660,10 +661,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):
         """
@@ -681,11 +687,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):
@@ -1041,7 +1056,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:

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):
@@ -329,6 +330,136 @@ class TestCourseIndex(CourseTestCase):
 
 
 @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):
     """
     Unit tests for the course outline.

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'));
         };
 

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;

cms/templates/index.html

@@ -308,10 +308,15 @@ from openedx.core.djangolib.markup import HTML, Text
       </div>
       %endif
 
-      % if libraries_enabled:
+      % if libraries_enabled or archived_courses:
       <ul id="course-index-tabs">
         <li class="courses-tab active"><a>${_("Courses")}</a></li>
+        % if archived_courses:
+        <li class="archived-courses-tab"><a>${_("Archived Courses")}</a></li>
+        % endif
+        % if libraries_enabled:
         <li class="libraries-tab"><a>${_("Libraries")}</a></li>
+        % endif
       </ul>
       % endif
 
@@ -467,6 +472,44 @@ from openedx.core.djangolib.markup import HTML, Text
       </div>
       % endif
 
+      %if archived_courses:
+      <div class="archived-courses archived-courses-tab">
+        <ul class="list-courses">
+          %for course_info in sorted(archived_courses, key=lambda s: s['display_name'].lower() if s['display_name'] is not None else ''):
+          <li class="course-item" data-course-key="${course_info['course_key']}">
+            <a class="course-link" href="${course_info['url']}">
+              <h3 class="course-title">${course_info['display_name']}</h3>
+
+              <div class="course-metadata">
+                <span class="course-org metadata-item">
+                  <span class="label">${_("Organization:")}</span> <span class="value">${course_info['org']}</span>
+                </span>
+                <span class="course-num metadata-item">
+                  <span class="label">${_("Course Number:")}</span>
+                  <span class="value">${course_info['number']}</span>
+                </span>
+                <span class="course-run metadata-item">
+                  <span class="label">${_("Course Run:")}</span> <span class="value">${course_info['run']}</span>
+                </span>
+              </div>
+            </a>
+
+            <ul  class="item-actions course-actions">
+              % if allow_course_reruns and rerun_creator_status and course_creator_status=='granted':
+              <li class="action action-rerun">
+                <a href="${course_info['rerun_link']}" class="button rerun-button">${_("Re-run Course")}</a>
+              </li>
+              % endif
+              <li class="action action-view">
+                <a href="${course_info['lms_link']}" rel="external" class="button view-button">${_("View Live")}</a>
+              </li>
+            </ul>
+          </li>
+          %endfor
+        </ul>
+      </div>
+      %endif
+
       %if len(libraries) > 0:
       <div class="libraries libraries-tab">
         <ul class="list-courses">

common/lib/xmodule/xmodule/course_module.py

@@ -5,6 +5,7 @@ import json
 import logging
 from cStringIO import StringIO
 from datetime import datetime, timedelta
+import dateutil.parser
 
 import requests
 from lazy import lazy
@@ -1393,9 +1394,10 @@ class CourseSummary(object):
     A lightweight course summary class, which constructs split/mongo course summary without loading
     the course. It is used at cms for listing courses to global staff user.
     """
-    course_info_fields = ['display_name', 'display_coursenumber', 'display_organization']
+    course_info_fields = ['display_name', 'display_coursenumber', 'display_organization', 'end']
 
-    def __init__(self, course_locator, display_name=u"Empty", display_coursenumber=None, display_organization=None):
+    def __init__(self, course_locator, display_name=u"Empty", display_coursenumber=None, display_organization=None,
+                 end=None):
         """
         Initialize and construct course summary
 
@@ -1412,6 +1414,8 @@ class CourseSummary(object):
 
         display_organization (unicode|None): Course organization that is specified & appears in the courseware
 
+        end (unicode|None): Course end date.  Must contain timezone.
+
         """
         self.display_coursenumber = display_coursenumber
         self.display_organization = display_organization
@@ -1419,6 +1423,9 @@ class CourseSummary(object):
 
         self.id = course_locator  # pylint: disable=invalid-name
         self.location = course_locator.make_usage_key('course', 'course')
+        self.end = end
+        if end is not None and not isinstance(end, datetime):
+            self.end = dateutil.parser.parse(end)
 
     @property
     def display_org_with_default(self):
@@ -1439,3 +1446,9 @@ class CourseSummary(object):
         if self.display_coursenumber:
             return self.display_coursenumber
         return self.location.course
+
+    def has_ended(self):
+        """
+        Returns whether the course has ended.
+        """
+        return course_metadata_utils.has_course_ended(self.end)

common/lib/xmodule/xmodule/modulestore/tests/test_semantics.py

@@ -211,7 +211,7 @@ class DirectOnlyCategorySemantics(PureModulestoreTestCase):
         """
         def verify_course_summery_fields(course_summary):
             """ Verify that every `course_summary` object has all the required fields """
-            expected_fields = CourseSummary.course_info_fields + ['id', 'location']
+            expected_fields = CourseSummary.course_info_fields + ['id', 'location', 'has_ended']
             return all([hasattr(course_summary, field) for field in expected_fields])
 
         self.assertTrue(all(verify_course_summery_fields(course_summary) for course_summary in course_summaries))