Merge pull request #365 from edx/dan-f/prevent-links-to-empty-modules

Disable next/previous links to videos with no data

analytics_dashboard/courses/presenters/engagement.py

@@ -4,7 +4,6 @@
 import logging
 from waffle import switch_is_active
 
-from opaque_keys.edx.keys import UsageKey
 from analyticsclient.client import Client
 import analyticsclient.constants.activity_type as AT
 from analyticsclient.exceptions import NotFoundError
@@ -138,6 +137,14 @@ def attach_computed_data(self, video):
             'start_only_percent': utils.math.calculate_percent(start_only_users, total),
         })
 
+    def video_has_data(self, video_or_data):
+        """
+        Return True if and only if `video_or_data` represents a video that has been viewed.
+
+        `video_or_data` is either a video block annotated with data or the data itself.
+        """
+        return video_or_data.get('users_at_start', 0) > 0 or video_or_data.get('users_at_end', 0) > 0
+
     def attach_aggregated_data_to_parent(self, index, parent, url_func=None):
         children = parent['children']
         total_start_users = sum(child.get('users_at_start', 0) for child in children)
@@ -153,7 +160,7 @@ def attach_aggregated_data_to_parent(self, index, parent, url_func=None):
         # calculates the percentages too
         self.attach_computed_data(parent)
 
-        has_views = total_start_users > 0 or total_end_users > 0
+        has_views = any(self.video_has_data(video) for video in children)
 
         if has_views and parent['num_modules']:
             num_modules = float(parent['num_modules'])
@@ -195,7 +202,7 @@ def build_url(parent, video):
         return build_url
 
     def post_process_adding_data_to_blocks(self, data, parent_block, child_block, url_func=None):
-        if url_func:
+        if url_func and self.video_has_data(data):
             data['url'] = url_func(parent_block, child_block)
 
     @property
@@ -213,7 +220,7 @@ def module_id_to_data_id(self, module):
         The data api only has the encoded module ID.  This converts the course structure ID
         to the encoded form.
         """
-        return UsageKey.from_string(module['id']).html_id()
+        return utils.get_encoded_module_id(module['id'])
 
     def get_video_timeline(self, video_module):
         """ Returns the video timeline with gaps in the beginning and end filled in with zeros. """

analytics_dashboard/courses/tests/test_presenters.py

@@ -189,13 +189,13 @@ def test_graded_modes(self, grade_status):
         """
         Ensure that video structure will be retrieved for both graded and ungraded.
         """
-        factory = CourseEngagementDataFactory()
         course_structure_fixtures = self._create_graded_and_ungraded_course_structure_fixtures()
         course_fixture = course_structure_fixtures['course']
         chapter_fixture = course_structure_fixtures['chapter']
 
         with mock.patch('slumber.Resource.get', mock.Mock(return_value=course_fixture.course_structure())):
-            with mock.patch('analyticsclient.course.Course.videos', mock.Mock(return_value=factory.videos())):
+            with mock.patch('analyticsclient.course.Course.videos',
+                            mock.Mock(return_value=utils.get_mock_video_data(course_fixture))):
                 # check that we get results for both graded and ungraded
                 sequential_fixture = course_structure_fixtures[grade_status]['sequential']
                 video_id = sequential_fixture.children[0].children[0].id
@@ -208,10 +208,10 @@ def test_graded_modes(self, grade_status):
                                            'subsection_id': sequential_fixture.id,
                                            'video_id': video_id
                                        })
-                expected = [{'index': 1, 'name': None, 'end_percent': 0, 'url': expected_url, 'start_only_percent': 0,
-                             'id': video_id, 'users_at_start': 0, 'start_only_users': 0, 'users_at_end': 0,
-                             'children': []}]
-                self.assertListEqual(actual_videos, expected)
+                expected = [{'id': utils.get_encoded_module_id(video_id), 'url': expected_url}]
+                self.assertEqual(len(actual_videos), len(expected))
+                for index, actual_video in enumerate(actual_videos):
+                    self.assertDictContainsSubset(expected[index], actual_video)
 
     def test_module_id_to_data_id(self):
         opaque_key_id = 'i4x-edX-DemoX-video-0b9e39477cf34507a7a48f74be381fdd'
@@ -405,7 +405,7 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_1.id, 0, VIDEO_1.id),
+        ), VIDEO_1, 0, VIDEO_1),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -415,7 +415,7 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_1.id, 1, VIDEO_2.id),
+        ), VIDEO_1, 1, VIDEO_2),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -425,7 +425,7 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_2.id, -1, VIDEO_1.id),
+        ), VIDEO_2, -1, VIDEO_1),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -437,7 +437,7 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_1.id, 1, VIDEO_2.id),
+        ), VIDEO_1, 1, VIDEO_2),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -451,7 +451,7 @@ def test_build_live_url(self):
                     ),
                 )
             )
-        ), VIDEO_1.id, 1, VIDEO_2.id),
+        ), VIDEO_1, 1, VIDEO_2),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -467,7 +467,7 @@ def test_build_live_url(self):
                     ),
                 ),
             )
-        ), VIDEO_1.id, 1, VIDEO_2.id),
+        ), VIDEO_1, 1, VIDEO_2),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -490,7 +490,7 @@ def test_build_live_url(self):
                     ),
                 ),
             )
-        ), VIDEO_1.id, 2, VIDEO_3.id),
+        ), VIDEO_1, 2, VIDEO_3),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -499,7 +499,7 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_1.id, -1, None),
+        ), VIDEO_1, -1, None),
         (CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -508,36 +508,35 @@ def test_build_live_url(self):
                     )
                 )
             )
-        ), VIDEO_1.id, 1, None),
+        ), VIDEO_1, 1, None),
     )
     @unpack
     @override_settings(CACHES={
         'default': {
             'BACKEND': 'django.core.cache.backends.dummy.DummyCache',
         }
     })
-    @mock.patch('analyticsclient.course.Course.videos')
-    def test_sibling(self, fixture, start_id, offset, expected_sibling_id, mock_videos):
+    def test_sibling(self, fixture, start_block, offset, expected_sibling_block):
         """Tests the _sibling method of the `CourseAPIPresenterMixin`."""
-        mock_videos.return_value = []
-        with mock.patch('slumber.Resource.get', mock.Mock(return_value=fixture.course_structure())):
-            sibling = self.presenter.sibling_block(start_id, offset)
-            if expected_sibling_id is None:
-                self.assertIsNone(sibling)
-            else:
-                self.assertEqual(sibling['id'], expected_sibling_id)
+        with mock.patch(
+            'analyticsclient.course.Course.videos', mock.Mock(return_value=utils.get_mock_video_data(fixture))
+        ):
+            with mock.patch('slumber.Resource.get', mock.Mock(return_value=fixture.course_structure())):
+                sibling = self.presenter.sibling_block(utils.get_encoded_module_id(start_block['id']), offset)
+                if expected_sibling_block is None:
+                    self.assertIsNone(sibling)
+                else:
+                    self.assertEqual(sibling['id'], utils.get_encoded_module_id(expected_sibling_block['id']))
 
     @override_settings(CACHES={
         'default': {
             'BACKEND': 'django.core.cache.backends.dummy.DummyCache',
         }
     })
-    @mock.patch('analyticsclient.course.Course.videos')
-    def test_sibling_no_data(self, mock_videos):
+    def test_sibling_no_data(self):
         """
         Verify that _sibling() skips over siblings with no data (no associated URL).
         """
-        mock_videos.return_value = []
         fixture = CourseFixture().add_children(
             ChapterFixture().add_children(
                 SequentialFixture().add_children(
@@ -549,18 +548,13 @@ def test_sibling_no_data(self, mock_videos):
                 )
             )
         )
-        with mock.patch('slumber.Resource.get', mock.Mock(return_value=fixture.course_structure())):
-            # pylint: disable=unused-argument
-            def build_module_url_func(parent, video):
-                if video['id'] == self.VIDEO_2.id:
-                    return None
-                else:
-                    return 'dummy_url'
-
-            with mock.patch.object(CourseEngagementVideoPresenter, 'build_module_url_func',
-                                   mock.Mock(return_value=build_module_url_func)):
-                sibling = self.presenter.sibling_block(self.VIDEO_1.id, 1)
-                self.assertEqual(sibling['id'], self.VIDEO_3.id)
+        with mock.patch(
+            'analyticsclient.course.Course.videos',
+            mock.Mock(return_value=utils.get_mock_video_data(fixture, excluded_module_ids=[self.VIDEO_2['id']]))
+        ):
+            with mock.patch('slumber.Resource.get', mock.Mock(return_value=fixture.course_structure())):
+                sibling = self.presenter.sibling_block(utils.get_encoded_module_id(self.VIDEO_1['id']), 1)
+                self.assertEqual(sibling['id'], utils.get_encoded_module_id(self.VIDEO_3['id']))
 
     @data('http://example.com', 'http://example.com/')
     def test_build_render_xblock_url(self, xblock_render_base):

analytics_dashboard/courses/tests/utils.py

@@ -11,6 +11,7 @@
 
 from courses.permissions import set_user_course_permissions
 from courses.presenters.performance import AnswerDistributionEntry
+from courses.utils import get_encoded_module_id
 
 
 CREATED_DATETIME = datetime.datetime(year=2014, month=2, day=2)
@@ -651,3 +652,31 @@ def get_presenter_answer_distribution(course_id, problem_part_id):
 
 def mock_course_name(course_id):
     return 'Test ' + course_id
+
+
+def get_mock_video_data(course_fixture, excluded_module_ids=None):
+    """
+    Given a `course_fixture` (instance of `CourseFixture`), return a
+    list of mock video data for each video in the course.
+
+    If `excluded_module_ids` is provided, don't generate data for any
+    module IDs in the list.
+    """
+    if excluded_module_ids is None:
+        excluded_module_ids = list()
+    return [
+        {
+            "pipeline_video_id": '{org}/{course}/{run}|{encoded_module_id}'.format(
+                org=course_fixture.org, course=course_fixture.course, run=course_fixture.run,
+                encoded_module_id=get_encoded_module_id(module_id)
+            ),
+            "encoded_module_id": get_encoded_module_id(module_id),
+            "duration": 129,
+            "segment_length": 5,
+            "users_at_start": 1,
+            "users_at_end": 1,
+            "created": "2015-10-03T195620"
+        }
+        for module_id, module_block in course_fixture.course_structure()['blocks'].items()
+        if module_block['type'] == 'video' and module_id not in excluded_module_ids
+    ]

analytics_dashboard/courses/utils.py

@@ -1,6 +1,8 @@
 import re
 from waffle import switch_is_active
 
+from opaque_keys.edx.keys import UsageKey
+
 
 def is_feature_enabled(item):
     switch_name = item.get('switch', None)
@@ -11,6 +13,11 @@ def is_feature_enabled(item):
     return True
 
 
+def get_encoded_module_id(module_id):
+    """Return an encoded module ID representing `module_id`"""
+    return UsageKey.from_string(module_id).html_id()
+
+
 class number(object):
     @staticmethod
     def is_number(word):

common/tests/course_fixtures.py

@@ -19,6 +19,13 @@ def __init__(self, *args, **kwargs):
     def __repr__(self):
         return self.id
 
+    def __getitem__(self, item):
+        """
+        Allows course structure fixture objects to be treated like the
+        dict data structures they represent.
+        """
+        return self.to_dict().__getitem__(item)
+
     @property
     def children(self):
         return self._children