MA-844 Support sorting parameters in get_videos_for_course.

edxval/api.py

@@ -236,21 +236,57 @@ def get_url_for_profile(edx_video_id, profile):
     return get_urls_for_profiles(edx_video_id, [profile])[profile]
 
 
-def get_videos_for_course(course_id):
+def _get_videos_for_filter(
+        video_filter,
+        sort_field=None,
+        sort_dir=SortDirection.asc
+):
     """
-    Returns an iterator of videos for the given course id
+    Returns a generator expression that contains the videos found, sorted by
+    the given field and direction, with ties broken by edx_video_id to ensure a
+    total order.
     """
-    videos = Video.objects.filter(courses__course_id=unicode(course_id))
+    videos = Video.objects.filter(**video_filter)
+    if sort_field:
+        # Refining by edx_video_id ensures a total order
+        videos = videos.order_by(sort_field.value, "edx_video_id")
+        if sort_dir == SortDirection.desc:
+            videos = videos.reverse()
     return (VideoSerializer(video).data for video in videos)
 
 
+def get_videos_for_course(
+    course_id,
+    sort_field=None,
+    sort_dir=SortDirection.asc,
+):
+    """
+    Returns an iterator of videos for the given course id.
+
+    Args:
+        course_id (String)
+        sort_field (VideoSortField)
+        sort_dir (SortDirection)
+
+    Returns:
+        A generator expression that contains the videos found, sorted by the
+        given field and direction, with ties broken by edx_video_id to ensure a
+        total order.
+    """
+    return _get_videos_for_filter(
+        {"courses__course_id":unicode(course_id)},
+        sort_field,
+        sort_dir,
+    )
+
+
 def get_videos_for_ids(
         edx_video_ids,
         sort_field=None,
         sort_dir=SortDirection.asc
 ):
     """
-    Returns an iterator of videos that match the given list of ids
+    Returns an iterator of videos that match the given list of ids.
 
     Args:
         edx_video_ids (list)
@@ -262,13 +298,11 @@ def get_videos_for_ids(
         given field and direction, with ties broken by edx_video_id to ensure a
         total order
     """
-    videos = Video.objects.filter(edx_video_id__in=edx_video_ids)
-    if sort_field:
-        # Refining by edx_video_id ensures a total order
-        videos = videos.order_by(sort_field.value, "edx_video_id")
-        if sort_dir == SortDirection.desc:
-            videos = videos.reverse()
-    return (VideoSerializer(video).data for video in videos)
+    return _get_videos_for_filter(
+        {"edx_video_id__in":edx_video_ids},
+        sort_field,
+        sort_dir,
+    )
 
 
 def get_video_info_for_course_and_profiles(course_id, profiles):

edxval/tests/test_api.py

@@ -23,6 +23,57 @@ from edxval.api import (
 from edxval.tests import constants, APIAuthTestCase
 
 
+class SortedVideoTestMixin(object):
+    """
+    Test Mixin for testing api functions that sort the returned videos.
+    """
+    def _check_sort(self, api_func, sort_field, expected_ids_for_asc):
+        """
+        Assert that sorting by given field returns videos in the expected
+        order (checking both ascending and descending)
+        """
+        def check_direction(sort_direction, expected_ids):
+            """Assert that the given videos match the expected ids"""
+
+            # Make sure it's not just returning the order given
+            actual_videos = api_func(list(reversed(expected_ids)), sort_field, sort_direction)
+            actual_ids = [video["edx_video_id"] for video in actual_videos]
+            self.assertEqual(actual_ids, expected_ids)
+
+        check_direction(SortDirection.asc, expected_ids_for_asc)
+        check_direction(
+            SortDirection.desc,
+            list(reversed(expected_ids_for_asc))
+        )
+
+    def check_sort_params_of_api(self, api_func):
+        """
+        Verifies the given API function handles the sort parameters correctly for a list of videos.
+
+        Args:
+            api_func: A function with parameters (list-of-video-ids, sort_field, sort_direction)
+                and returns a list of videos.
+        """
+        fish_id = constants.VIDEO_DICT_FISH["edx_video_id"]
+        star_id = constants.VIDEO_DICT_STAR["edx_video_id"]
+        other_id = "other-video"
+        star_video = Video.objects.create(**constants.VIDEO_DICT_STAR)
+
+        # This is made to sort with the other videos differently by each field
+        other_video = Video.objects.create(
+            client_video_id="other video",
+            duration=555.0,
+            edx_video_id=other_id
+        )
+        CourseVideo.objects.create(video=star_video, course_id=self.course_id)
+        CourseVideo.objects.create(video=other_video, course_id=self.course_id)
+
+        self._check_sort(api_func, VideoSortField.client_video_id, [fish_id, star_id, other_id])
+        self._check_sort(api_func, VideoSortField.edx_video_id, [star_id, other_id, fish_id])
+        # Check a field with a tie
+        self._check_sort(api_func, VideoSortField.duration, [star_id, fish_id, other_id])
+
+
 @ddt
 class CreateVideoTest(TestCase):
     """
@@ -75,12 +126,12 @@ class CreateProfileTest(TestCase):
         """
         Tests the creation of a profile
         """
-        result = api.create_profile(constants.PROFILE_DESKTOP)
+        api.create_profile(constants.PROFILE_DESKTOP)
         profiles = list(Profile.objects.all())
         self.assertEqual(len(profiles), 6)
-        self.assertEqual(
-            profiles[-1].profile_name,
-            constants.PROFILE_DESKTOP
+        self.assertIn(
+            constants.PROFILE_DESKTOP,
+            [unicode(profile) for profile in profiles],
         )
         self.assertEqual(len(profiles), 6)
 
@@ -132,16 +183,6 @@ class GetVideoInfoTest(TestCase):
             )
         )
 
-    def test_get_videos_for_course(self):
-        """
-        Tests retrieving videos for a course id
-        """
-        videos = list(api.get_videos_for_course(self.course_id))
-        self.assertEqual(len(videos), 1)
-        self.assertEqual(videos[0]['edx_video_id'], constants.VIDEO_DICT_FISH['edx_video_id'])
-        videos = list(api.get_videos_for_course('unknown'))
-        self.assertEqual(len(videos), 0)
-
     def test_no_such_video(self):
         """
         Tests searching for a video that does not exist
@@ -443,7 +484,55 @@ class GetVideoForCourseProfiles(TestCase):
         self.assertEqual(videos, expected_dict)
 
 
-class GetVideosForIds(TestCase):
+class GetVideosForCourseTest(TestCase, SortedVideoTestMixin):
+    """
+    Tests for our get_videos_for_course function in api.py
+    """
+
+    def setUp(self):
+        """
+        Creates EncodedVideo objects in database
+        """
+        Profile.objects.create(profile_name=constants.PROFILE_MOBILE)
+        Profile.objects.create(profile_name=constants.PROFILE_DESKTOP)
+
+        # create video in the test course
+        self.course_id = 'test-course'
+        video = Video.objects.create(**constants.VIDEO_DICT_FISH)
+        CourseVideo.objects.create(video=video, course_id=self.course_id)
+
+        # create another video in a different course (to make sure it's not returned)
+        video_in_other_course = Video.objects.create(
+            client_video_id="video in another course",
+            duration=111.0,
+            edx_video_id="video-in-another-course",
+        )
+        CourseVideo.objects.create(video=video_in_other_course, course_id="other-course")
+
+    def test_get_videos_for_course(self):
+        """
+        Tests retrieving videos for a course id
+        """
+        videos = list(api.get_videos_for_course(self.course_id))
+        self.assertEqual(len(videos), 1)
+        self.assertEqual(videos[0]['edx_video_id'], constants.VIDEO_DICT_FISH['edx_video_id'])
+        videos = list(api.get_videos_for_course('unknown'))
+        self.assertEqual(len(videos), 0)
+
+    def test_get_videos_for_course_sort(self):
+        """
+        Tests retrieving videos for a course id according to sort
+        """
+        def api_func(_expected_ids, sort_field, sort_direction):
+            return api.get_videos_for_course(
+                self.course_id,
+                sort_field,
+                sort_direction,
+            )
+        self.check_sort_params_of_api(api_func)
+
+
+class GetVideosForIdsTest(TestCase, SortedVideoTestMixin):
     """
     Tests the get_videos_for_ids function in api.py
     """
@@ -509,42 +598,13 @@ class GetVideosForIds(TestCase):
         self.assertEqual(len(videos), 1)
 
     def test_get_videos_for_ids_sort(self):
-        fish_id = constants.VIDEO_DICT_FISH["edx_video_id"]
-        star_id = constants.VIDEO_DICT_STAR["edx_video_id"]
-        other_id = "other-video"
-        Video.objects.create(**constants.VIDEO_DICT_STAR)
-        # This is made to sort with the other videos differently by each field
-        Video.objects.create(
-            client_video_id="other video",
-            duration=555.0,
-            edx_video_id=other_id
-        )
-
-        def check_sort(sort_field, expected_ids_for_asc):
-            """
-            Assert that sorting by given field returns videos in the expected
-            order (checking both ascending and descending)
-            """
-            def check_direction(sort_dir, expected_ids):
-                """Assert that the given videos match the expected ids"""
-                actual_videos = api.get_videos_for_ids(
-                    # Make sure it's not just returning the order given
-                    list(reversed(expected_ids)),
+        def api_func(expected_ids, sort_field, sort_direction):
+            return api.get_videos_for_ids(
+                expected_ids,
                 sort_field,
-                    sort_dir
+                sort_direction,
             )
-                actual_ids = [video["edx_video_id"] for video in actual_videos]
-                self.assertEqual(actual_ids, expected_ids)
-            check_direction(SortDirection.asc, expected_ids_for_asc)
-            check_direction(
-                SortDirection.desc,
-                list(reversed(expected_ids_for_asc))
-            )
-
-        check_sort(VideoSortField.client_video_id, [fish_id, star_id, other_id])
-        check_sort(VideoSortField.edx_video_id, [star_id, other_id, fish_id])
-        # Check a field with a tie
-        check_sort(VideoSortField.duration, [star_id, fish_id, other_id])
+        self.check_sort_params_of_api(api_func)
 
 
 class GetVideoInfoTestWithHttpCalls(APIAuthTestCase):

setup.py

@@ -37,7 +37,7 @@ def load_requirements(*requirements_paths):
 
 setup(
     name='edxval',
-    version='0.0.4',
+    version='0.0.5',
     author='edX',
     url='http://github.com/edx/edx-val',
     description='edx-val',