Merge pull request #355 from edx/dan-f/next-previous-video-buttons

Add next/previous buttons to video engagement timelines

Makefile

@@ -4,7 +4,7 @@ ROOT = $(shell echo "$$PWD")
 COVERAGE = $(ROOT)/build/coverage
 NODE_BIN=./node_modules/.bin
 
-DJANGO_SETTINGS_MODULE := "analytics_dashboard.settings.local"
+DJANGO_SETTINGS_MODULE ?= "analytics_dashboard.settings.local"
 
 .PHONY: requirements clean
 

analytics_dashboard/courses/presenters/__init__.py

@@ -306,6 +306,48 @@ def block(self, block_id):
         block['name'] = block.get('display_name')
         return block
 
+    def sibling_block(self, block_id, sibling_offset):
+        """
+        Returns a sibling block of the same type as the one denoted by
+        `block_id`, where order is course ordering.  The sibling is chosen by
+        `sibling_offset` which is the difference in index between the block and
+        its requested sibling.  Returns `None` if no such sibling is found.
+        Only siblings with data are returned.
+        """
+        sections = self.sections()
+        siblings = [
+            component
+            for section in sections
+            for subsection in section['children']
+            for component in subsection['children']
+            if component.get('url')  # Only consider siblings with data, hence with URLs
+        ]
+        try:
+            block_index = (index for index, sibling in enumerate(siblings) if sibling['id'] == block_id).next()
+            sibling_index = block_index + sibling_offset
+            if sibling_index < 0:
+                return None
+            else:
+                return siblings[sibling_index]
+        except (StopIteration, IndexError):
+            # StopIteration: requested video not found in the course structure
+            # IndexError: No such video with the requested offset
+            return None
+
+    def next_block(self, block_id):
+        """
+        Get the next block in the course with the same block type as the block
+        denoted by `block_id`.
+        """
+        return self.sibling_block(block_id, 1)
+
+    def previous_block(self, block_id):
+        """
+        Get the previous block in the course with the same block type as the
+        block denoted by `block_id`.
+        """
+        return self.sibling_block(block_id, -1)
+
     @abc.abstractmethod
     def blocks_have_data(self, blocks):
         """ Returns whether blocks contains any displayable data. """

analytics_dashboard/courses/templates/courses/_module_description.html

@@ -1,20 +1,21 @@
 {% load i18n %}
 
-{% if view_live_url or description %}
-    <div class="col-xs-12 module-description">
-        <div class="col-sm-10 col-xs-12">
-            <p>{{ description }}</p>
-        </div>
-
-        <div class="col-sm-2 col-xs-12">
-            {% if view_live_url %}
-                <a href="{{ view_live_url }}" class="btn btn-default pull-right"
-                   role="button" target="_blank">
-                    {# Translators: This text will be a direct link to the a specific module/problem. #}
-                    {% trans "View Live" %}
-                </a>
-            {% endif %}
+<div class="col-xs-12 module-description">
+    <div class="container-fluid">
+        <div class="row">
+            <div class="col-sm-10 col-xs-12">
+                <p>{{ description }}</p>
+            </div>
+            <div class="col-sm-2 col-xs-12">
+                {% if view_live_url %}
+                    <a href="{{ view_live_url }}" class="btn btn-default pull-right"
+                       role="button" target="_blank">
+                        {# Translators: This text will be a direct link to the a specific module/problem. #}
+                        {% trans "View Live" %}
+                    </a>
+                {% endif %}
+            </div>
         </div>
     </div>
-    <div class="clearfix"></div>
-{% endif %}
+</div>
+<div class="clearfix"></div>

analytics_dashboard/courses/templates/courses/_video_module_controls.html

@@ -0,0 +1,32 @@
+{% load i18n %}
+
+<div class="col-xs-12 module-controls">
+    <div class="container-fluid">
+        <div class="row">
+            <div class="col-xs-4">
+                <a href="{% firstof previous_video_url "#" %}"
+                   class="previous-link btn btn-default {% if not previous_video_url %} disabled {% endif %}">
+                    {% trans 'Previous Video' %}
+                </a>
+            </div>
+            <div class="col-xs-4">
+                {% if view_live_url %}
+                    <div class="text-center">
+                        <a href="{{ view_live_url }}" class="btn btn-default"
+                           role="button" target="_blank">
+                            {# Translators: This text will be a direct link to the a specific module/problem. #}
+                            {% trans "View Live" %}
+                        </a>
+                    </div>
+                {% endif %}
+            </div>
+            <div class="col-xs-4">
+                <a href="{% firstof next_video_url "#" %}"
+                   class="previous-link btn btn-default pull-right {% if not next_video_url %} disabled {% endif %}">
+                    {% trans 'Next Video' %}
+                </a>
+            </div>
+        </div>
+    </div>
+</div>
+<div class="clearfix"></div>

analytics_dashboard/courses/templates/courses/base_grouped_content.html

@@ -7,7 +7,7 @@
     {% block content_nav %}
     {% endblock %}
 
-    {% block module_description %}
+    {% block module_meta %}
     {% endblock %}
 
     <div class="section-content section-data-graph">

analytics_dashboard/courses/templates/courses/base_performance_answer_distribution.html

@@ -19,9 +19,9 @@
     {% block content_nav %}
     {% endblock %}
 
-    {% block module_description %}
+    <div class="module-meta">
         {% include "courses/_module_description.html" with description=problem_part_description view_live_url=view_live_url %}
-    {% endblock %}
+    </div>
 
     <div class="section-content section-data-graph">
         <div class="section-content section-data-viz">

analytics_dashboard/courses/templates/courses/engagement_video_timeline.html

@@ -27,8 +27,10 @@
     {% trans "The number of students who watched each segment of the video, and the number of replays for each segment." %}
 {% endblock %}
 
-{% block module_description %}
-    {% include "courses/_module_description.html" with view_live_url=view_live_url %}
+{% block module_meta %}
+    <div class="module-meta">
+        {% include "courses/_video_module_controls.html" with view_live_url=view_live_url %}
+    </div>
 {% endblock %}
 
 {% block metric_cards %}

analytics_dashboard/courses/tests/test_presenters.py

@@ -7,9 +7,15 @@
 from django.core.urlresolvers import reverse
 from django.test import (override_settings, TestCase)
 import mock
-from ddt import ddt, data
+from ddt import ddt, data, unpack
 
-from common.tests import course_fixtures
+from common.tests.course_fixtures import (
+    ChapterFixture,
+    CourseFixture,
+    SequentialFixture,
+    VerticalFixture,
+    VideoFixture
+)
 
 from courses.exceptions import NoVideosError
 from courses.presenters import BasePresenter
@@ -130,6 +136,9 @@ class CourseEngagementVideoPresenterTests(SwitchMixin, TestCase):
     SECTION_ID = 'i4x://edX/DemoX/chapter/9fca584977d04885bc911ea76a9ef29e'
     SUBSECTION_ID = 'i4x://edX/DemoX/sequential/07bc32474380492cb34f76e5f9d9a135'
     VIDEO_ID = 'i4x://edX/DemoX/video/0b9e39477cf34507a7a48f74be381fdd'
+    VIDEO_1 = VideoFixture()
+    VIDEO_2 = VideoFixture()
+    VIDEO_3 = VideoFixture()
 
     def setUp(self):
         super(CourseEngagementVideoPresenterTests, self).setUp()
@@ -149,17 +158,17 @@ def _create_graded_and_ungraded_course_structure_fixtures(self):
         """
         Create graded and ungraded video sections.
         """
-        chapter_fixture = course_fixtures.ChapterFixture()
+        chapter_fixture = ChapterFixture()
         # a dictionary to access the fixtures easily
         course_structure_fixtures = {
             'chapter': chapter_fixture,
-            'course': course_fixtures.CourseFixture(org='this', course='course', run='id')
+            'course': CourseFixture(org='this', course='course', run='id')
         }
 
         for grade_status in ['graded', 'ungraded']:
-            sequential_fixture = course_fixtures.SequentialFixture(graded=grade_status is 'graded').add_children(
-                course_fixtures.VerticalFixture().add_children(
-                    course_fixtures.VideoFixture()
+            sequential_fixture = SequentialFixture(graded=grade_status is 'graded').add_children(
+                VerticalFixture().add_children(
+                    VideoFixture()
                 )
             )
             course_structure_fixtures[grade_status] = {
@@ -371,6 +380,172 @@ def test_build_live_url(self):
         self.assertEqual('a-url/{}/jump_to/{}'.format(self.course_id, self.VIDEO_ID), actual_view_live_url)
         self.assertEqual(None, self.presenter.build_view_live_url(None, self.VIDEO_ID))
 
+    @data(
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1
+                    )
+                )
+            )
+        ), VIDEO_1.id, 0, VIDEO_1.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                        VIDEO_2
+                    )
+                )
+            )
+        ), VIDEO_1.id, 1, VIDEO_2.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                        VIDEO_2
+                    )
+                )
+            )
+        ), VIDEO_2.id, -1, VIDEO_1.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                    ),
+                    VerticalFixture().add_children(
+                        VIDEO_2,
+                    )
+                )
+            )
+        ), VIDEO_1.id, 1, VIDEO_2.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                    ),
+                ),
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_2,
+                    ),
+                )
+            )
+        ), VIDEO_1.id, 1, VIDEO_2.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                    ),
+                ),
+            ),
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_2,
+                    ),
+                ),
+            )
+        ), VIDEO_1.id, 1, VIDEO_2.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1,
+                    ),
+                ),
+            ),
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_2
+                    ),
+                ),
+            ),
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_3
+                    ),
+                ),
+            )
+        ), VIDEO_1.id, 2, VIDEO_3.id),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1
+                    )
+                )
+            )
+        ), VIDEO_1.id, -1, None),
+        (CourseFixture().add_children(
+            ChapterFixture().add_children(
+                SequentialFixture().add_children(
+                    VerticalFixture().add_children(
+                        VIDEO_1
+                    )
+                )
+            )
+        ), VIDEO_1.id, 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):
+        """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)
+
+    @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):
+        """
+        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(
+                    VerticalFixture().add_children(
+                        self.VIDEO_1,
+                        self.VIDEO_2,  # self.VIDEO_2 will have no data
+                        self.VIDEO_3
+                    )
+                )
+            )
+        )
+        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)
+
 
 class CourseEnrollmentPresenterTests(SwitchMixin, TestCase):
     @classmethod

analytics_dashboard/courses/tests/test_views/test_engagement.py

@@ -171,7 +171,7 @@ def assertValidContext(self, context):
         self.assertDictContainsSubset(expected, context)
 
     @httpretty.activate
-    @patch('courses.presenters.engagement.CourseEngagementVideoPresenter.sections', Mock(return_value=None))
+    @patch('courses.presenters.engagement.CourseEngagementVideoPresenter.sections', Mock(return_value=dict()))
     def test_missing_sections(self):
         """ Every video page will use sections and will return 200 if sections aren't available. """
         self.mock_course_detail(DEMO_COURSE_ID)