Merge pull request #10960 from edx/renzo/run-org-filtering

Modify Course API to filter visible courses by org

lms/djangoapps/branding/__init__.py

@@ -17,14 +17,29 @@ from django.contrib.staticfiles.storage import staticfiles_storage
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 
 
-def get_visible_courses():
+def get_visible_courses(org=None):
     """
-    Return the set of CourseDescriptors that should be visible in this branded instance
+    Return the set of CourseOverviews that should be visible in this branded instance
     """
-    filtered_by_org = microsite.get_value('course_org_filter')
-    courses = CourseOverview.get_all_courses(org=filtered_by_org)
+    microsite_org = microsite.get_value('course_org_filter')
+
+    if org and microsite_org:
+        # When called in the context of a microsite, return an empty result if the org
+        # passed by the caller does not match the designated microsite org.
+        courses = CourseOverview.get_all_courses(org=org) if org == microsite_org else []
+    else:
+        # We only make it to this point if one of org or microsite_org is defined.
+        # If both org and microsite_org were defined, the code would have fallen into the
+        # first branch of the conditional above, wherein an equality check is performed.
+        target_org = org or microsite_org
+        courses = CourseOverview.get_all_courses(org=target_org)
+
     courses = sorted(courses, key=lambda course: course.number)
 
+    # When called in the context of a microsite, filtering can stop here.
+    if microsite_org:
+        return courses
+
     # See if we have filtered course listings in this domain
     filtered_visible_ids = None
 
@@ -35,15 +50,12 @@ def get_visible_courses():
             [SlashSeparatedCourseKey.from_deprecated_string(c) for c in settings.COURSE_LISTINGS[subdomain]]
         )
 
-    if filtered_by_org:
-        return [course for course in courses if course.location.org == filtered_by_org]
     if filtered_visible_ids:
         return [course for course in courses if course.id in filtered_visible_ids]
     else:
-        # Let's filter out any courses in an "org" that has been declared to be
-        # in a Microsite
-        org_filter_out_set = microsite.get_all_orgs()
-        return [course for course in courses if course.location.org not in org_filter_out_set]
+        # Filter out any courses belonging to a microsite, to avoid leaking these.
+        microsite_orgs = microsite.get_all_orgs()
+        return [course for course in courses if course.location.org not in microsite_orgs]
 
 
 def get_university_for_request():

lms/djangoapps/course_api/api.py

@@ -10,7 +10,6 @@ from lms.djangoapps.courseware.courses import (
     get_course_overview_with_access,
     get_permission_for_course_about,
 )
-
 from .permissions import can_view_courses_for_username
 
 
@@ -43,7 +42,7 @@ def course_detail(request, username, course_key):
         course_key (CourseKey): Identifies the course of interest
 
     Return value:
-        `CourseDescriptor` object representing the requested course
+        `CourseOverview` object representing the requested course
     """
     user = get_effective_user(request.user, username)
     return get_course_overview_with_access(
@@ -53,7 +52,7 @@ def course_detail(request, username, course_key):
     )
 
 
-def list_courses(request, username):
+def list_courses(request, username, org=None):
     """
     Return a list of available courses.
 
@@ -69,9 +68,14 @@ def list_courses(request, username):
             The name of the user the logged-in user would like to be
             identified as
 
+    Keyword Arguments:
+        org (string):
+            If specified, visible `CourseOverview` objects are filtered
+            such that only those belonging to the organization with the provided
+            org code (e.g., "HarvardX") are returned. Case-insensitive.
 
     Return value:
-        List of `CourseDescriptor` objects representing the collection of courses.
+        List of `CourseOverview` objects representing the collection of courses.
     """
     user = get_effective_user(request.user, username)
-    return get_courses(user)
+    return get_courses(user, org=org)

lms/djangoapps/course_api/tests/test_api.py

 """
 Test for course API
 """
+from hashlib import md5
 
 from django.contrib.auth.models import AnonymousUser
 from django.http import Http404
+from opaque_keys.edx.keys import CourseKey
 from rest_framework.exceptions import PermissionDenied
 from rest_framework.request import Request
 from rest_framework.test import APIRequestFactory
 
-from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.content.course_overviews.models import CourseOverview
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase, ModuleStoreTestCase
 from xmodule.modulestore.tests.factories import check_mongo_calls
-
-from ..api import course_detail, list_courses
 from .mixins import CourseApiFactoryMixin
+from ..api import course_detail, list_courses
 
 
 class CourseApiTestMixin(CourseApiFactoryMixin):
@@ -87,7 +87,7 @@ class CourseListTestMixin(CourseApiTestMixin):
     """
     Common behavior for list_courses tests
     """
-    def _make_api_call(self, requesting_user, specified_user):
+    def _make_api_call(self, requesting_user, specified_user, org=None):
         """
         Call the list_courses api endpoint to get information about
         `specified_user` on behalf of `requesting_user`.
@@ -95,7 +95,7 @@ class CourseListTestMixin(CourseApiTestMixin):
         request = Request(self.request_factory.get('/'))
         request.user = requesting_user
         with check_mongo_calls(0):
-            return list_courses(request, specified_user.username)
+            return list_courses(request, specified_user.username, org=org)
 
     def verify_courses(self, courses):
         """
@@ -113,7 +113,7 @@ class TestGetCourseList(CourseListTestMixin, SharedModuleStoreTestCase):
     @classmethod
     def setUpClass(cls):
         super(TestGetCourseList, cls).setUpClass()
-        cls.create_course()
+        cls.course = cls.create_course()
         cls.staff_user = cls.create_user("staff", is_staff=True)
         cls.honor_user = cls.create_user("honor", is_staff=False)
 
@@ -144,11 +144,35 @@ class TestGetCourseList(CourseListTestMixin, SharedModuleStoreTestCase):
         with self.assertRaises(PermissionDenied):
             self._make_api_call(anonuser, self.staff_user)
 
+    @SharedModuleStoreTestCase.modifies_courseware
     def test_multiple_courses(self):
         self.create_course(course='second')
         courses = self._make_api_call(self.honor_user, self.honor_user)
         self.assertEqual(len(courses), 2)
 
+    @SharedModuleStoreTestCase.modifies_courseware
+    def test_filter_by_org(self):
+        """Verify that courses are filtered by the provided org key."""
+        # Create a second course to be filtered out of queries.
+        alternate_course = self.create_course(
+            org=md5(self.course.org).hexdigest()
+        )
+
+        self.assertNotEqual(alternate_course.org, self.course.org)
+
+        # No filtering.
+        unfiltered_courses = self._make_api_call(self.staff_user, self.staff_user)
+        for org in [self.course.org, alternate_course.org]:
+            self.assertTrue(
+                any(course.org == org for course in unfiltered_courses)
+            )
+
+        # With filtering.
+        filtered_courses = self._make_api_call(self.staff_user, self.staff_user, org=self.course.org)
+        self.assertTrue(
+            all(course.org == self.course.org for course in filtered_courses)
+        )
+
 
 class TestGetCourseListExtras(CourseListTestMixin, ModuleStoreTestCase):
     """

lms/djangoapps/course_api/tests/test_views.py

 """
-Tests for Blocks Views
+Tests for Course API views.
 """
+from hashlib import md5
 
 from django.core.urlresolvers import reverse
 from django.test import RequestFactory
 
 from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase
-
 from .mixins import CourseApiFactoryMixin, TEST_PASSWORD
-
 from ..views import CourseDetailView
 
 
@@ -78,6 +77,31 @@ class CourseListViewTestCase(CourseApiTestViewMixin, SharedModuleStoreTestCase):
         self.setup_user(self.honor_user)
         self.verify_response(expected_status_code=403, params={'username': self.staff_user.username})
 
+    @SharedModuleStoreTestCase.modifies_courseware
+    def test_filter_by_org(self):
+        """Verify that CourseOverviews are filtered by the provided org key."""
+        self.setup_user(self.staff_user)
+
+        # Create a second course to be filtered out of queries.
+        alternate_course = self.create_course(
+            org=md5(self.course.org).hexdigest()
+        )
+
+        self.assertNotEqual(alternate_course.org, self.course.org)
+
+        # No filtering.
+        unfiltered_response = self.verify_response()
+        for org in [self.course.org, alternate_course.org]:
+            self.assertTrue(
+                any(course['org'] == org for course in unfiltered_response.data['results'])  # pylint: disable=no-member
+            )
+
+        # With filtering.
+        filtered_response = self.verify_response(params={'org': self.course.org})
+        self.assertTrue(
+            all(course['org'] == self.course.org for course in filtered_response.data['results'])  # pylint: disable=no-member
+        )
+
     def test_not_logged_in(self):
         self.client.logout()
         self.verify_response()

lms/djangoapps/course_api/views.py

@@ -3,13 +3,11 @@ Course API Views
 """
 
 from django.http import Http404
-from rest_framework.generics import ListAPIView, RetrieveAPIView
-
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import CourseKey
+from rest_framework.generics import ListAPIView, RetrieveAPIView
 
 from openedx.core.lib.api.paginators import NamespacedPageNumberPagination
-
 from .api import course_detail, list_courses
 from .serializers import CourseSerializer
 
@@ -127,6 +125,11 @@ class CourseListView(ListAPIView):
             The username of the specified user whose visible courses we
             want to see.  Defaults to the current user.
 
+        org (optional):
+            If specified, visible `CourseOverview` objects are filtered
+            such that only those belonging to the organization with the provided
+            org code (e.g., "HarvardX") are returned. Case-insensitive.
+
     **Returns**
 
         * 200 on success, with a list of course discovery objects as returned
@@ -170,4 +173,6 @@ class CourseListView(ListAPIView):
         Return a list of courses visible to the user.
         """
         username = self.request.query_params.get('username', self.request.user.username)
-        return list_courses(self.request, username)
+        org = self.request.query_params.get('org')
+
+        return list_courses(self.request, username, org=org)

lms/djangoapps/courseware/courses.py

@@ -373,11 +373,12 @@ def get_course_syllabus_section(course, section_key):
     raise KeyError("Invalid about key " + str(section_key))
 
 
-def get_courses(user, domain=None):
-    '''
-    Returns a list of courses available, sorted by course.number
-    '''
-    courses = branding.get_visible_courses()
+def get_courses(user, domain=None, org=None):  # pylint: disable=unused-argument
+    """
+    Returns a list of courses available, sorted by course.number and optionally
+    filtered by org code (case-insensitive).
+    """
+    courses = branding.get_visible_courses(org=org)
 
     permission_name = microsite.get_value(
         'COURSE_CATALOG_VISIBILITY_PERMISSION',
@@ -386,8 +387,6 @@ def get_courses(user, domain=None):
 
     courses = [c for c in courses if has_access(user, permission_name, c)]
 
-    courses = sorted(courses, key=lambda course: course.number)
-
     return courses
 
 

lms/djangoapps/courseware/tests/test_courses.py

@@ -2,23 +2,27 @@
 """
 Tests for course access
 """
-import ddt
 import itertools
-import mock
-from nose.plugins.attrib import attr
 
+import ddt
 from django.conf import settings
 from django.test.utils import override_settings
 from django.core.urlresolvers import reverse
 from django.test.client import RequestFactory
+import mock
+from nose.plugins.attrib import attr
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
 from courseware.courses import (
-    get_course_by_id, get_cms_course_link,
-    get_course_info_section, get_course_about_section, get_cms_block_link
+    get_cms_block_link,
+    get_cms_course_link,
+    get_courses,
+    get_course_about_section,
+    get_course_by_id,
+    get_course_info_section,
+    get_course_overview_with_access,
+    get_course_with_access,
 )
-
-from courseware.courses import get_course_with_access, get_course_overview_with_access
 from courseware.module_render import get_module_for_descriptor
 from courseware.tests.helpers import get_request_for_user
 from courseware.model_data import FieldDataCache
@@ -80,6 +84,54 @@ class CoursesTest(ModuleStoreTestCase):
         with check_mongo_calls(num_mongo_calls):
             course_access_func(user, 'load', course.id)
 
+    def test_get_courses(self):
+        """
+        Verify that org filtering performs as expected, and that an empty result
+        is returned if the org passed by the caller does not match the designated
+        microsite org.
+        """
+        primary = 'primary'
+        alternate = 'alternate'
+
+        def _fake_get_value(value, default=None):
+            """Used to stub out microsite.get_value()."""
+            if value == 'course_org_filter':
+                return alternate
+
+            return default
+
+        user = UserFactory.create()
+
+        # Pass `emit_signals=True` so that these courses are cached with CourseOverviews.
+        primary_course = CourseFactory.create(org=primary, emit_signals=True)
+        alternate_course = CourseFactory.create(org=alternate, emit_signals=True)
+
+        self.assertNotEqual(primary_course.org, alternate_course.org)
+
+        unfiltered_courses = get_courses(user)
+        for org in [primary_course.org, alternate_course.org]:
+            self.assertTrue(
+                any(course.org == org for course in unfiltered_courses)
+            )
+
+        filtered_courses = get_courses(user, org=primary)
+        self.assertTrue(
+            all(course.org == primary_course.org for course in filtered_courses)
+        )
+
+        with mock.patch('microsite_configuration.microsite.get_value', autospec=True) as mock_get_value:
+            mock_get_value.side_effect = _fake_get_value
+
+            # Request filtering for an org distinct from the designated microsite org.
+            no_courses = get_courses(user, org=primary)
+            self.assertEqual(no_courses, [])
+
+            # Request filtering for an org matching the designated microsite org.
+            microsite_courses = get_courses(user, org=alternate)
+            self.assertTrue(
+                all(course.org == alternate_course.org for course in microsite_courses)
+            )
+
 
 @attr('shard_1')
 class ModuleStoreBranchSettingTest(ModuleStoreTestCase):

openedx/core/djangoapps/content/course_overviews/models.py

@@ -457,10 +457,15 @@ class CourseOverview(TimeStampedModel):
         """
         # Note: If a newly created course is not returned in this QueryList,
         # make sure the "publish" signal was emitted when the course was
-        # created.  For tests using CourseFactory, use emit_signals=True.
+        # created. For tests using CourseFactory, use emit_signals=True.
         course_overviews = CourseOverview.objects.all()
+
         if org:
-            course_overviews = course_overviews.filter(org=org)
+            # In rare cases, courses belonging to the same org may be accidentally assigned
+            # an org code with a different casing (e.g., Harvardx as opposed to HarvardX).
+            # Case-insensitive exact matching allows us to deal with this kind of dirty data.
+            course_overviews = course_overviews.filter(org__iexact=org)
+
         return course_overviews
 
     @classmethod

openedx/core/djangoapps/content/course_overviews/tests.py

@@ -444,14 +444,14 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
     def test_get_select_courses(self):
         course_ids = [CourseFactory.create().id for __ in range(3)]
         select_course_ids = course_ids[:len(course_ids) - 1]  # all items except the last
-        self.assertSetEqual(
+        self.assertEqual(
             {course_overview.id for course_overview in CourseOverview.get_select_courses(select_course_ids)},
             set(select_course_ids),
         )
 
     def test_get_all_courses(self):
         course_ids = [CourseFactory.create(emit_signals=True).id for __ in range(3)]
-        self.assertSetEqual(
+        self.assertEqual(
             {course_overview.id for course_overview in CourseOverview.get_all_courses()},
             set(course_ids),
         )
@@ -470,12 +470,18 @@ class CourseOverviewTestCase(ModuleStoreTestCase):
                 for __ in range(3)
             ])
 
-        self.assertSetEqual(
+        self.assertEqual(
+            {c.id for c in CourseOverview.get_all_courses()},
+            {c.id for c in org_courses[0] + org_courses[1]},
+        )
+
+        self.assertEqual(
             {c.id for c in CourseOverview.get_all_courses(org='test_org_1')},
             {c.id for c in org_courses[1]},
         )
 
-        self.assertSetEqual(
-            {c.id for c in CourseOverview.get_all_courses()},
-            {c.id for c in org_courses[0] + org_courses[1]},
+        # Test case-insensitivity.
+        self.assertEqual(
+            {c.id for c in CourseOverview.get_all_courses(org='TEST_ORG_1')},
+            {c.id for c in org_courses[1]},
         )