From bb53c03e241b4d1862195138968e907f3895fd2e Mon Sep 17 00:00:00 2001 From: Nimisha Asthagiri Date: Tue, 8 Dec 2015 14:19:27 -0500 Subject: [PATCH] Optimize Course Catalog/About API with Course Overviews --- lms/djangoapps/course_api/api.py | 22 +++--- lms/djangoapps/course_api/blocks/views.py | 2 - lms/djangoapps/course_api/serializers.py | 68 ++++--------------- lms/djangoapps/course_api/tests/mixins.py | 1 + lms/djangoapps/course_api/tests/test_api.py | 20 +++--- .../course_api/tests/test_serializers.py | 22 ++++-- lms/djangoapps/course_api/views.py | 4 +- .../mobile_api/users/serializers.py | 18 +---- lms/djangoapps/mobile_api/users/tests.py | 4 +- 9 files changed, 59 insertions(+), 102 deletions(-) diff --git a/lms/djangoapps/course_api/api.py b/lms/djangoapps/course_api/api.py index 8fd36b1a2d..9f67011382 100644 --- a/lms/djangoapps/course_api/api.py +++ b/lms/djangoapps/course_api/api.py @@ -3,10 +3,13 @@ Course API """ from django.contrib.auth.models import User -from django.http import Http404 -from rest_framework.exceptions import NotFound, PermissionDenied +from rest_framework.exceptions import PermissionDenied -from lms.djangoapps.courseware.courses import get_courses, get_course_with_access +from lms.djangoapps.courseware.courses import ( + get_courses, + get_course_overview_with_access, + get_permission_for_course_about, +) from .permissions import can_view_courses_for_username @@ -43,11 +46,11 @@ def course_detail(request, username, course_key): `CourseDescriptor` object representing the requested course """ user = get_effective_user(request.user, username) - try: - course = get_course_with_access(user, 'see_exists', course_key) - except Http404: - raise NotFound() - return course + return get_course_overview_with_access( + user, + get_permission_for_course_about(), + course_key, + ) def list_courses(request, username): @@ -71,5 +74,4 @@ def list_courses(request, username): List of `CourseDescriptor` objects representing the collection of courses. """ user = get_effective_user(request.user, username) - courses = get_courses(user) - return courses + return get_courses(user) diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 1fb4376408..a3912ededa 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -39,8 +39,6 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView): * username: (string) The name of the user on whose behalf we want to see the data. - Default is the logged in user - Example: username=anjali * student_view_data: (list) Indicates for which block types to return diff --git a/lms/djangoapps/course_api/serializers.py b/lms/djangoapps/course_api/serializers.py index f891365853..3394044ccb 100644 --- a/lms/djangoapps/course_api/serializers.py +++ b/lms/djangoapps/course_api/serializers.py @@ -5,42 +5,32 @@ Course API Serializers. Representing course catalog data import urllib from django.core.urlresolvers import reverse -from django.template import defaultfilters - from rest_framework import serializers -from lms.djangoapps.courseware.courses import get_course_about_section -from openedx.core.lib.courses import course_image_url -from openedx.core.djangoapps.models.course_details import CourseDetails -from xmodule.course_module import DEFAULT_START_DATE - class _MediaSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Nested serializer to represent a media object. """ - def __init__(self, uri_parser, *args, **kwargs): + def __init__(self, uri_attribute, *args, **kwargs): super(_MediaSerializer, self).__init__(*args, **kwargs) - self.uri_parser = uri_parser + self.uri_attribute = uri_attribute uri = serializers.SerializerMethodField(source='*') - def get_uri(self, course): + def get_uri(self, course_overview): """ Get the representation for the media resource's URI """ - return self.uri_parser(course) + return getattr(course_overview, self.uri_attribute) class _CourseApiMediaCollectionSerializer(serializers.Serializer): # pylint: disable=abstract-method """ Nested serializer to represent a collection of media objects """ - course_image = _MediaSerializer(source='*', uri_parser=course_image_url) - course_video = _MediaSerializer( - source='*', - uri_parser=lambda course: CourseDetails.fetch_video_url(course.id), - ) + course_image = _MediaSerializer(source='*', uri_attribute='course_image_url') + course_video = _MediaSerializer(source='*', uri_attribute='course_video_url') class CourseSerializer(serializers.Serializer): # pylint: disable=abstract-method @@ -52,14 +42,14 @@ class CourseSerializer(serializers.Serializer): # pylint: disable=abstract-meth name = serializers.CharField(source='display_name_with_default') number = serializers.CharField(source='display_number_with_default') org = serializers.CharField(source='display_org_with_default') - short_description = serializers.SerializerMethodField() - effort = serializers.SerializerMethodField() + short_description = serializers.CharField() + effort = serializers.CharField() media = _CourseApiMediaCollectionSerializer(source='*') start = serializers.DateTimeField() - start_type = serializers.SerializerMethodField() - start_display = serializers.SerializerMethodField() + start_type = serializers.CharField() + start_display = serializers.CharField() end = serializers.DateTimeField() enrollment_start = serializers.DateTimeField() @@ -67,46 +57,12 @@ class CourseSerializer(serializers.Serializer): # pylint: disable=abstract-meth blocks_url = serializers.SerializerMethodField() - def get_start_type(self, course): - """ - Get the representation for SerializerMethodField `start_type` - """ - if course.advertised_start is not None: - return u'string' - elif course.start != DEFAULT_START_DATE: - return u'timestamp' - else: - return u'empty' - - def get_start_display(self, course): - """ - Get the representation for SerializerMethodField `start_display` - """ - if course.advertised_start is not None: - return course.advertised_start - elif course.start != DEFAULT_START_DATE: - return defaultfilters.date(course.start, "DATE_FORMAT") - else: - return None - - def get_short_description(self, course): - """ - Get the representation for SerializerMethodField `short_description` - """ - return get_course_about_section(self.context['request'], course, 'short_description').strip() - - def get_blocks_url(self, course): + def get_blocks_url(self, course_overview): """ Get the representation for SerializerMethodField `blocks_url` """ base_url = '?'.join([ reverse('blocks_in_course'), - urllib.urlencode({'course_id': course.id}), + urllib.urlencode({'course_id': course_overview.id}), ]) return self.context['request'].build_absolute_uri(base_url) - - def get_effort(self, course): - """ - Get the representation for SerializerMethodField `effort` - """ - return CourseDetails.fetch_effort(course.id) diff --git a/lms/djangoapps/course_api/tests/mixins.py b/lms/djangoapps/course_api/tests/mixins.py index bdac14f05e..fa8b848802 100644 --- a/lms/djangoapps/course_api/tests/mixins.py +++ b/lms/djangoapps/course_api/tests/mixins.py @@ -26,6 +26,7 @@ class CourseApiFactoryMixin(object): end=datetime(2015, 9, 19, 18, 0, 0), enrollment_start=datetime(2015, 6, 15, 0, 0, 0), enrollment_end=datetime(2015, 7, 15, 0, 0, 0), + emit_signals=True, **kwargs ) diff --git a/lms/djangoapps/course_api/tests/test_api.py b/lms/djangoapps/course_api/tests/test_api.py index bf77026518..7d3b12f656 100644 --- a/lms/djangoapps/course_api/tests/test_api.py +++ b/lms/djangoapps/course_api/tests/test_api.py @@ -3,13 +3,15 @@ Test for course API """ from django.contrib.auth.models import AnonymousUser -from rest_framework.exceptions import NotFound, PermissionDenied +from django.http import Http404 +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.course_module import CourseDescriptor +from xmodule.modulestore.tests.factories import check_mongo_calls from ..api import course_detail, list_courses from .mixins import CourseApiFactoryMixin @@ -23,12 +25,12 @@ class CourseApiTestMixin(CourseApiFactoryMixin): def setUpClass(cls): super(CourseApiTestMixin, cls).setUpClass() cls.request_factory = APIRequestFactory() + CourseOverview.get_all_courses() # seed the CourseOverview table def verify_course(self, course, course_id=u'edX/toy/2012_Fall'): """ Ensure that the returned course is the course we just created """ - self.assertIsInstance(course, CourseDescriptor) self.assertEqual(course_id, str(course.id)) @@ -43,7 +45,8 @@ class CourseDetailTestMixin(CourseApiTestMixin): """ request = Request(self.request_factory.get('/')) request.user = requesting_user - return course_detail(request, target_user.username, course_key) + with check_mongo_calls(0): + return course_detail(request, target_user.username, course_key) class TestGetCourseDetail(CourseDetailTestMixin, SharedModuleStoreTestCase): @@ -64,11 +67,11 @@ class TestGetCourseDetail(CourseDetailTestMixin, SharedModuleStoreTestCase): def test_get_nonexistent_course(self): course_key = CourseKey.from_string(u'edX/toy/nope') - with self.assertRaises(NotFound): + with self.assertRaises(Http404): self._make_api_call(self.honor_user, self.honor_user, course_key) def test_hidden_course_for_honor(self): - with self.assertRaises(NotFound): + with self.assertRaises(Http404): self._make_api_call(self.honor_user, self.honor_user, self.hidden_course.id) def test_hidden_course_for_staff(self): @@ -76,7 +79,7 @@ class TestGetCourseDetail(CourseDetailTestMixin, SharedModuleStoreTestCase): self.verify_course(course, course_id=u'edX/hidden/2012_Fall') def test_hidden_course_for_staff_as_honor(self): - with self.assertRaises(NotFound): + with self.assertRaises(Http404): self._make_api_call(self.staff_user, self.honor_user, self.hidden_course.id) @@ -91,7 +94,8 @@ class CourseListTestMixin(CourseApiTestMixin): """ request = Request(self.request_factory.get('/')) request.user = requesting_user - return list_courses(request, specified_user.username) + with check_mongo_calls(0): + return list_courses(request, specified_user.username) def verify_courses(self, courses): """ diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py index 093e8131d9..a7440fc8ed 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -5,6 +5,7 @@ Test data created by CourseSerializer from datetime import datetime from openedx.core.djangoapps.models.course_details import CourseDetails +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from rest_framework.test import APIRequestFactory from rest_framework.request import Request @@ -29,7 +30,7 @@ class TestCourseSerializerFields(CourseApiFactoryMixin, ModuleStoreTestCase): def _get_request(self, user=None): """ - Build a Request object for the specified user + Build a Request object for the specified user. """ if user is None: user = self.honor_user @@ -37,6 +38,13 @@ class TestCourseSerializerFields(CourseApiFactoryMixin, ModuleStoreTestCase): request.user = user return request + def _get_result(self, course): + """ + Return the CourseSerializer for the specified course. + """ + course_overview = CourseOverview.get_from_id(course.id) + return CourseSerializer(course_overview, context={'request': self._get_request()}).data + def test_basic(self): expected_data = { 'course_id': u'edX/toy/2012_Fall', @@ -55,15 +63,15 @@ class TestCourseSerializerFields(CourseApiFactoryMixin, ModuleStoreTestCase): 'start': u'2015-07-17T12:00:00Z', 'start_type': u'timestamp', 'start_display': u'July 17, 2015', - 'end': u'2015-09-19T18:00:00', - 'enrollment_start': u'2015-06-15T00:00:00', - 'enrollment_end': u'2015-07-15T00:00:00', + 'end': u'2015-09-19T18:00:00Z', + 'enrollment_start': u'2015-06-15T00:00:00Z', + 'enrollment_end': u'2015-07-15T00:00:00Z', 'blocks_url': u'http://testserver/api/courses/v1/blocks/?course_id=edX%2Ftoy%2F2012_Fall', 'effort': u'6 hours', } course = self.create_course() CourseDetails.update_about_video(course, 'test_youtube_id', self.staff_user.id) # pylint: disable=no-member - result = CourseSerializer(course, context={'request': self._get_request()}).data + result = self._get_result(course) self.assertDictEqual(result, expected_data) def test_advertised_start(self): @@ -72,14 +80,14 @@ class TestCourseSerializerFields(CourseApiFactoryMixin, ModuleStoreTestCase): start=datetime(2015, 3, 15), advertised_start=u'The Ides of March' ) - result = CourseSerializer(course, context={'request': self._get_request()}).data + result = self._get_result(course) self.assertEqual(result['course_id'], u'edX/custom/2012_Fall') self.assertEqual(result['start_type'], u'string') self.assertEqual(result['start_display'], u'The Ides of March') def test_empty_start(self): course = self.create_course(start=DEFAULT_START_DATE, course=u'custom') - result = CourseSerializer(course, context={'request': self._get_request()}).data + result = self._get_result(course) self.assertEqual(result['course_id'], u'edX/custom/2012_Fall') self.assertEqual(result['start_type'], u'empty') self.assertIsNone(result['start_display']) diff --git a/lms/djangoapps/course_api/views.py b/lms/djangoapps/course_api/views.py index 2fcb700652..22c38b38fe 100644 --- a/lms/djangoapps/course_api/views.py +++ b/lms/djangoapps/course_api/views.py @@ -2,7 +2,7 @@ Course API Views """ -from rest_framework.exceptions import NotFound +from django.http import Http404 from rest_framework.generics import ListAPIView, RetrieveAPIView from opaque_keys import InvalidKeyError @@ -102,7 +102,7 @@ class CourseDetailView(RetrieveAPIView): try: course_key = CourseKey.from_string(course_key_string) except InvalidKeyError: - raise NotFound() + raise Http404() return course_detail(self.request, username, course_key) diff --git a/lms/djangoapps/mobile_api/users/serializers.py b/lms/djangoapps/mobile_api/users/serializers.py index a28a2f392a..b2be7c81c8 100644 --- a/lms/djangoapps/mobile_api/users/serializers.py +++ b/lms/djangoapps/mobile_api/users/serializers.py @@ -4,12 +4,9 @@ Serializer for user API from rest_framework import serializers from rest_framework.reverse import reverse -from django.template import defaultfilters - from courseware.access import has_access from student.models import CourseEnrollment, User from certificates.api import certificate_downloadable_status -from xmodule.course_module import DEFAULT_START_DATE class CourseOverviewField(serializers.RelatedField): @@ -19,17 +16,6 @@ class CourseOverviewField(serializers.RelatedField): def to_representation(self, course_overview): course_id = unicode(course_overview.id) - - if course_overview.advertised_start is not None: - start_type = 'string' - start_display = course_overview.advertised_start - elif course_overview.start != DEFAULT_START_DATE: - start_type = 'timestamp' - start_display = defaultfilters.date(course_overview.start, 'DATE_FORMAT') - else: - start_type = 'empty' - start_display = None - request = self.context.get('request') return { # identifiers @@ -40,8 +26,8 @@ class CourseOverviewField(serializers.RelatedField): # dates 'start': course_overview.start, - 'start_display': start_display, - 'start_type': start_type, + 'start_display': course_overview.start_display, + 'start_type': course_overview.start_type, 'end': course_overview.end, # notification info diff --git a/lms/djangoapps/mobile_api/users/tests.py b/lms/djangoapps/mobile_api/users/tests.py index 92fdb6e989..c49d47c8c1 100644 --- a/lms/djangoapps/mobile_api/users/tests.py +++ b/lms/djangoapps/mobile_api/users/tests.py @@ -168,8 +168,10 @@ class TestUserEnrollmentApi(UrlResetMixin, MobileAPITestCase, MobileAuthUserTest @ddt.data( (NEXT_WEEK, ADVERTISED_START, ADVERTISED_START, "string"), (NEXT_WEEK, None, defaultfilters.date(NEXT_WEEK, "DATE_FORMAT"), "timestamp"), + (NEXT_WEEK, '', defaultfilters.date(NEXT_WEEK, "DATE_FORMAT"), "timestamp"), (DEFAULT_START_DATE, ADVERTISED_START, ADVERTISED_START, "string"), - (DEFAULT_START_DATE, None, None, "empty") + (DEFAULT_START_DATE, '', None, "empty"), + (DEFAULT_START_DATE, None, None, "empty"), ) @ddt.unpack @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False})