From 8a8350dd360b8cb230235868c75b74220cb94eba Mon Sep 17 00:00:00 2001 From: Daniel Friedman Date: Mon, 21 Sep 2015 17:02:42 +0000 Subject: [PATCH] Add next/previous buttons to video previews AN-5609 --- Makefile | 2 +- .../courses/presenters/__init__.py | 42 ++++ .../courses/_module_description.html | 33 +-- .../courses/_video_module_controls.html | 32 +++ .../courses/base_grouped_content.html | 2 +- .../base_performance_answer_distribution.html | 4 +- .../courses/engagement_video_timeline.html | 6 +- .../courses/tests/test_presenters.py | 189 +++++++++++++++++- .../tests/test_views/test_engagement.py | 2 +- .../courses/views/engagement.py | 6 +- .../static/sass/_developer.scss | 11 +- analytics_dashboard/static/sass/_mixins.scss | 7 + common/tests/course_fixtures.py | 49 +++-- 13 files changed, 329 insertions(+), 56 deletions(-) create mode 100644 analytics_dashboard/courses/templates/courses/_video_module_controls.html diff --git a/Makefile b/Makefile index 9d151c53f..853f04bd1 100644 --- a/Makefile +++ b/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 diff --git a/analytics_dashboard/courses/presenters/__init__.py b/analytics_dashboard/courses/presenters/__init__.py index 8da6f3332..56234b449 100644 --- a/analytics_dashboard/courses/presenters/__init__.py +++ b/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. """ diff --git a/analytics_dashboard/courses/templates/courses/_module_description.html b/analytics_dashboard/courses/templates/courses/_module_description.html index 5af4ca6c2..5a6ec9a88 100644 --- a/analytics_dashboard/courses/templates/courses/_module_description.html +++ b/analytics_dashboard/courses/templates/courses/_module_description.html @@ -1,20 +1,21 @@ {% load i18n %} -{% if view_live_url or description %} -
-
-

{{ description }}

-
- -
- {% if view_live_url %} - - {# Translators: This text will be a direct link to the a specific module/problem. #} - {% trans "View Live" %} - - {% endif %} +
+
+
+
+

{{ description }}

+
+
+ {% if view_live_url %} + + {# Translators: This text will be a direct link to the a specific module/problem. #} + {% trans "View Live" %} + + {% endif %} +
-
-{% endif %} +
+
diff --git a/analytics_dashboard/courses/templates/courses/_video_module_controls.html b/analytics_dashboard/courses/templates/courses/_video_module_controls.html new file mode 100644 index 000000000..bd14d6b91 --- /dev/null +++ b/analytics_dashboard/courses/templates/courses/_video_module_controls.html @@ -0,0 +1,32 @@ +{% load i18n %} + +
+
+
+
+ + {% trans 'Previous Video' %} + +
+
+ {% if view_live_url %} +
+ + {# Translators: This text will be a direct link to the a specific module/problem. #} + {% trans "View Live" %} + +
+ {% endif %} +
+
+ + {% trans 'Next Video' %} + +
+
+
+
+
diff --git a/analytics_dashboard/courses/templates/courses/base_grouped_content.html b/analytics_dashboard/courses/templates/courses/base_grouped_content.html index dffa7ed71..05d38e0bd 100644 --- a/analytics_dashboard/courses/templates/courses/base_grouped_content.html +++ b/analytics_dashboard/courses/templates/courses/base_grouped_content.html @@ -7,7 +7,7 @@ {% block content_nav %} {% endblock %} - {% block module_description %} + {% block module_meta %} {% endblock %}
diff --git a/analytics_dashboard/courses/templates/courses/base_performance_answer_distribution.html b/analytics_dashboard/courses/templates/courses/base_performance_answer_distribution.html index 7aa06b3c4..445e30dc4 100644 --- a/analytics_dashboard/courses/templates/courses/base_performance_answer_distribution.html +++ b/analytics_dashboard/courses/templates/courses/base_performance_answer_distribution.html @@ -19,9 +19,9 @@ {% block content_nav %} {% endblock %} - {% block module_description %} +
{% include "courses/_module_description.html" with description=problem_part_description view_live_url=view_live_url %} - {% endblock %} +
diff --git a/analytics_dashboard/courses/templates/courses/engagement_video_timeline.html b/analytics_dashboard/courses/templates/courses/engagement_video_timeline.html index eb4e03523..819d84dcb 100644 --- a/analytics_dashboard/courses/templates/courses/engagement_video_timeline.html +++ b/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 %} +
+ {% include "courses/_video_module_controls.html" with view_live_url=view_live_url %} +
{% endblock %} {% block metric_cards %} diff --git a/analytics_dashboard/courses/tests/test_presenters.py b/analytics_dashboard/courses/tests/test_presenters.py index 454996c9c..d6f372cbb 100644 --- a/analytics_dashboard/courses/tests/test_presenters.py +++ b/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 diff --git a/analytics_dashboard/courses/tests/test_views/test_engagement.py b/analytics_dashboard/courses/tests/test_views/test_engagement.py index c998f5c4c..5a152fe40 100644 --- a/analytics_dashboard/courses/tests/test_views/test_engagement.py +++ b/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) diff --git a/analytics_dashboard/courses/views/engagement.py b/analytics_dashboard/courses/views/engagement.py index 61dca9d4a..5bc5bae4b 100644 --- a/analytics_dashboard/courses/views/engagement.py +++ b/analytics_dashboard/courses/views/engagement.py @@ -144,13 +144,17 @@ def get_context_data(self, **kwargs): timeline = self.presenter.get_video_timeline(video_module) videos = self.presenter.subsection_children(self.section_id, self.subsection_id) + next_video = self.presenter.next_block(video_data_id) + previous_video = self.presenter.previous_block(video_data_id) self.set_primary_content(context, videos) context.update({ 'video': self.presenter.block(self.video_id), 'summary_metrics': video_module, 'view_live_url': self.presenter.build_view_live_url(settings.LMS_COURSE_SHORTCUT_BASE_URL, self.video_id), - 'page_data': self.get_page_data(context) + 'page_data': self.get_page_data(context), + 'next_video_url': next_video['url'] if next_video is not None else None, + 'previous_video_url': previous_video['url'] if previous_video is not None else None }) context['js_data']['course'].update({ diff --git a/analytics_dashboard/static/sass/_developer.scss b/analytics_dashboard/static/sass/_developer.scss index c93e0d396..b8fee3a8b 100644 --- a/analytics_dashboard/static/sass/_developer.scss +++ b/analytics_dashboard/static/sass/_developer.scss @@ -585,7 +585,7 @@ table.dataTable thead th.sorting_desc:after { white-space: normal; } -.module-description { +.module-meta { border: $description-border; border-bottom: none; padding: $padding-small-horizontal 0; @@ -595,6 +595,10 @@ table.dataTable thead th.sorting_desc:after { // shifts it down so that it's even with the adjacent button padding-top: 8px; } + + a.disabled { + @extend disabled-link; + } } .grading-policy { @@ -692,10 +696,7 @@ table.dataTable thead th.sorting_desc:after { } > .disabled > a { - &, &:hover, &:focus { - color: $navbar-default-link-disabled-color; - background-color: $navbar-default-link-disabled-bg; - } + @include disabled-link; } } } diff --git a/analytics_dashboard/static/sass/_mixins.scss b/analytics_dashboard/static/sass/_mixins.scss index c001332d4..4b653f317 100644 --- a/analytics_dashboard/static/sass/_mixins.scss +++ b/analytics_dashboard/static/sass/_mixins.scss @@ -28,3 +28,10 @@ color: $gray; background-color: $gray-d2; } + +@mixin disabled-link { + &, &:hover, &:focus { + color: $navbar-default-link-disabled-color; + background-color: $navbar-default-link-disabled-bg; + } +} diff --git a/common/tests/course_fixtures.py b/common/tests/course_fixtures.py index 1862d3eac..0e2b2a85f 100644 --- a/common/tests/course_fixtures.py +++ b/common/tests/course_fixtures.py @@ -8,13 +8,16 @@ def __init__(self, *args, **kwargs): self._uuid = uuid.uuid4().hex self._type = None self._display_name = kwargs.get('display_name', '') - self._graded = False + self.graded = False self._assignment_type = None self._children = [] # Course data - self._org = None - self._course = None - self._run = None + self.org = None + self.course = None + self.run = None + + def __repr__(self): + return self.id @property def children(self): @@ -26,46 +29,52 @@ def to_dict(self): 'id': self.id, 'type': self._type, 'display_name': self._display_name, - 'graded': self._graded, + 'graded': self.graded, 'format': self._assignment_type, 'children': [child.id for child in self._children] } def add_children(self, *children): """Add children to this block""" + def _add_course_info(parent, child): + """ + Children should inherit and cache org, course, and run for ID generation. + 'graded' is also inherited. + """ + child.org = parent.org + child.course = parent.course + child.run = parent.run + child.graded = parent.graded + self._children += children + self.pre_order(visit_fn=_add_course_info) return self - def pre_order(self): + def pre_order(self, visit_fn=None): """Return the fixture rooted at `self` as a list visited in pre-order.""" visited = [self] for child in self._children: - # Children should inherit and cache org, course, and run for ID generation. - # 'graded' is also inherited. - # pylint: disable=protected-access - child._org = self._org - child._course = self._course - child._run = self._run - child._graded = self._graded - visited += child.pre_order() + if visit_fn: + visit_fn(self, child) + visited += child.pre_order(visit_fn=visit_fn) return visited @property def id(self): """Uniquely identifies this block in the format used by the Course Structure API.""" return 'i4x://{org}/{course}/{type}/{_uuid}'.format( - org=self._org, course=self._course, type=self._type, _uuid=self._uuid + org=self.org, course=self.course, type=self._type, _uuid=self._uuid ) class CourseFixture(CourseStructureAPIFixtureMixin): """Represents a course as returned by the Course Structure API.""" - def __init__(self, org=None, course=None, run=None, *args, **kwargs): + def __init__(self, org='test_org', course='test_course', run='test_run', *args, **kwargs): super(CourseFixture, self).__init__(*args, **kwargs) self._type = 'course' - self._org = org - self._course = course - self._run = run + self.org = org + self.course = course + self.run = run self._uuid = run # The org/course/run triple uniquely identifies the course def course_structure(self): @@ -90,7 +99,7 @@ class SequentialFixture(CourseStructureAPIFixtureMixin): """Represents a sequential as returned by the Course Structure API.""" def __init__(self, graded=False, assignment_type=None, *args, **kwargs): super(SequentialFixture, self).__init__(*args, **kwargs) - self._graded = graded + self.graded = graded self._assignment_type = assignment_type self._type = 'sequential'