From 2cee39d535a6c6696e3c15bcc3a34a7c90cc126a Mon Sep 17 00:00:00 2001 From: Renzo Lucioni Date: Wed, 9 Dec 2015 14:55:27 -0500 Subject: [PATCH] Modify Course API to filter visible courses by org Org to filter by is provided to the Course API list view using a querystring argument. Filtering ultimately occurs at the database layer. ECOM-2761. --- lms/djangoapps/branding/__init__.py | 32 ++++++--- lms/djangoapps/course_api/api.py | 14 ++-- lms/djangoapps/course_api/tests/test_api.py | 36 ++++++++-- lms/djangoapps/course_api/tests/test_views.py | 30 ++++++++- lms/djangoapps/course_api/views.py | 13 ++-- lms/djangoapps/courseware/courses.py | 13 ++-- .../courseware/tests/test_courses.py | 66 +++++++++++++++++-- .../content/course_overviews/models.py | 9 ++- .../content/course_overviews/tests.py | 18 +++-- 9 files changed, 181 insertions(+), 50 deletions(-) diff --git a/lms/djangoapps/branding/__init__.py b/lms/djangoapps/branding/__init__.py index 3f2e64a09d..e2d97f211b 100644 --- a/lms/djangoapps/branding/__init__.py +++ b/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(): diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 9f67011382..1c6f932ffd 100644 --- a/lms/djangoapps/course_api/api.py +++ b/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) diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index 7d3b12f656..26ce210511 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -1,20 +1,20 @@ """ 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): """ diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index 55a54ff30e..881ae687cf 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -1,14 +1,13 @@ """ -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() diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 22c38b38fe..002c62d476 100644 --- a/lms/djangoapps/course_api/views.py +++ b/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) diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index a31ed93288..4227058d9a 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/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 diff --git a/lms/djangoapps/courseware/tests/test_courses.py b/lms/djangoapps/courseware/tests/test_courses.py index c87bf922ee..858576b36e 100644 --- a/lms/djangoapps/courseware/tests/test_courses.py +++ b/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): diff --git a/openedx/core/djangoapps/content/course_overviews/models.py b/openedx/core/djangoapps/content/course_overviews/models.py index a7aeff9b8e..a4149820d5 100644 --- a/openedx/core/djangoapps/content/course_overviews/models.py +++ b/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 diff --git a/openedx/core/djangoapps/content/course_overviews/tests.py b/openedx/core/djangoapps/content/course_overviews/tests.py index 286371ddb9..51666bad93 100644 --- a/openedx/core/djangoapps/content/course_overviews/tests.py +++ b/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]}, )