From 07f331bcda7339c7b6165bc2cf7ed186ecb36709 Mon Sep 17 00:00:00 2001 From: David Baumgold Date: Thu, 5 Nov 2015 10:40:13 -0500 Subject: [PATCH 001/115] Fix flaky tests related to creative commons TNL-2800 --- .../acceptance/tests/video/test_video_license.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/common/test/acceptance/tests/video/test_video_license.py b/common/test/acceptance/tests/video/test_video_license.py index cd9fd929bb..89858b0ffe 100644 --- a/common/test/acceptance/tests/video/test_video_license.py +++ b/common/test/acceptance/tests/video/test_video_license.py @@ -84,8 +84,11 @@ class VideoLicenseTest(StudioCourseTest): self.lms_courseware.visit() video = self.lms_courseware.q(css=".vert .xblock .video") self.assertTrue(video.is_present()) - video_license = self.lms_courseware.q(css=".vert .xblock.xmodule_VideoModule .xblock-license") - self.assertTrue(video_license.is_present()) + video_license_css = ".vert .xblock.xmodule_VideoModule .xblock-license" + self.lms_courseware.wait_for_element_presence( + video_license_css, "Video module license block is present" + ) + video_license = self.lms_courseware.q(css=video_license_css) self.assertEqual(video_license.text[0], "© All Rights Reserved") def test_cc_license(self): @@ -111,6 +114,9 @@ class VideoLicenseTest(StudioCourseTest): self.lms_courseware.visit() video = self.lms_courseware.q(css=".vert .xblock .video") self.assertTrue(video.is_present()) - video_license = self.lms_courseware.q(css=".vert .xblock.xmodule_VideoModule .xblock-license") - self.assertTrue(video_license.is_present()) + video_license_css = ".vert .xblock.xmodule_VideoModule .xblock-license" + self.lms_courseware.wait_for_element_presence( + video_license_css, "Video module license block is present" + ) + video_license = self.lms_courseware.q(css=video_license_css) self.assertIn("Some Rights Reserved", video_license.text[0]) From 5d422b107c721daae6f4206ec0749370295d76e3 Mon Sep 17 00:00:00 2001 From: Christine Lytwynec Date: Fri, 23 Oct 2015 10:58:52 -0400 Subject: [PATCH 002/115] remove @attr('shard_5') from mixin --- common/test/acceptance/tests/lms/test_account_settings.py | 1 - 1 file changed, 1 deletion(-) diff --git a/common/test/acceptance/tests/lms/test_account_settings.py b/common/test/acceptance/tests/lms/test_account_settings.py index 4b6b3a30a0..906ffc8643 100644 --- a/common/test/acceptance/tests/lms/test_account_settings.py +++ b/common/test/acceptance/tests/lms/test_account_settings.py @@ -14,7 +14,6 @@ from ...pages.lms.dashboard import DashboardPage from ..helpers import EventsTestMixin -@attr('shard_5') class AccountSettingsTestMixin(EventsTestMixin, WebAppTest): """ Mixin with helper methods to test the account settings page. From 5d9dc325589e19add08facc396376e50b63f844a Mon Sep 17 00:00:00 2001 From: Christine Lytwynec Date: Thu, 22 Oct 2015 13:45:37 -0400 Subject: [PATCH 003/115] add custom-a11y-rules.js from edx-custom-a11y-rules --- package.json | 3 ++- pavelib/paver_tests/test_paver_bok_choy_cmds.py | 2 ++ pavelib/utils/envs.py | 4 ++++ pavelib/utils/test/suites/bokchoy_suite.py | 2 ++ 4 files changed, 10 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 876b169897..c7f7232cb4 100644 --- a/package.json +++ b/package.json @@ -6,6 +6,7 @@ "uglify-js": "2.4.24" }, "devDependencies": { - "jshint": "^2.7.0" + "jshint": "^2.7.0", + "edx-custom-a11y-rules": "edx/edx-custom-a11y-rules" } } diff --git a/pavelib/paver_tests/test_paver_bok_choy_cmds.py b/pavelib/paver_tests/test_paver_bok_choy_cmds.py index 9df04d09e9..87a0969cd6 100644 --- a/pavelib/paver_tests/test_paver_bok_choy_cmds.py +++ b/pavelib/paver_tests/test_paver_bok_choy_cmds.py @@ -25,6 +25,7 @@ class TestPaverBokChoyCmd(unittest.TestCase): "DEFAULT_STORE={default_store} " "SCREENSHOT_DIR='{repo_dir}/test_root/log{shard_str}' " "BOK_CHOY_HAR_DIR='{repo_dir}/test_root/log{shard_str}/hars' " + "BOKCHOY_A11Y_CUSTOM_RULES_FILE='{repo_dir}/{a11y_custom_file}' " "SELENIUM_DRIVER_LOG_DIR='{repo_dir}/test_root/log{shard_str}' " "nosetests {repo_dir}/common/test/acceptance/{exp_text} " "--with-xunit " @@ -35,6 +36,7 @@ class TestPaverBokChoyCmd(unittest.TestCase): repo_dir=REPO_DIR, shard_str='/shard_' + self.shard if self.shard else '', exp_text=name, + a11y_custom_file='node_modules/edx-custom-a11y-rules/lib/custom_a11y_rules.js', ) return expected_statement diff --git a/pavelib/utils/envs.py b/pavelib/utils/envs.py index ac5b5be565..7fef62016f 100644 --- a/pavelib/utils/envs.py +++ b/pavelib/utils/envs.py @@ -32,6 +32,10 @@ class Env(object): BOK_CHOY_A11Y_REPORT_DIR = REPORT_DIR / "a11y" BOK_CHOY_COVERAGERC = BOK_CHOY_DIR / ".coveragerc" BOK_CHOY_A11Y_COVERAGERC = BOK_CHOY_DIR / ".a11ycoveragerc" + BOK_CHOY_A11Y_CUSTOM_RULES_FILE = ( + REPO_ROOT / "node_modules" / "edx-custom-a11y-rules" / + "lib" / "custom_a11y_rules.js" + ) # If set, put reports for run in "unique" directories. # The main purpose of this is to ensure that the reports can be 'slurped' diff --git a/pavelib/utils/test/suites/bokchoy_suite.py b/pavelib/utils/test/suites/bokchoy_suite.py index 7e87b15c33..2c73597a9b 100644 --- a/pavelib/utils/test/suites/bokchoy_suite.py +++ b/pavelib/utils/test/suites/bokchoy_suite.py @@ -55,6 +55,7 @@ class BokChoyTestSuite(TestSuite): self.num_processes = kwargs.get('num_processes', DEFAULT_NUM_PROCESSES) self.extra_args = kwargs.get('extra_args', '') self.har_dir = self.log_dir / 'hars' + self.a11y_file = Env.BOK_CHOY_A11Y_CUSTOM_RULES_FILE self.imports_dir = kwargs.get('imports_dir', None) self.coveragerc = kwargs.get('coveragerc', None) @@ -205,6 +206,7 @@ class BokChoyTestSuite(TestSuite): "DEFAULT_STORE={}".format(self.default_store), "SCREENSHOT_DIR='{}'".format(self.log_dir), "BOK_CHOY_HAR_DIR='{}'".format(self.har_dir), + "BOKCHOY_A11Y_CUSTOM_RULES_FILE='{}'".format(self.a11y_file), "SELENIUM_DRIVER_LOG_DIR='{}'".format(self.log_dir), "nosetests", test_spec, From a40f286ecf47b6a28404c5e5562d9736f74204b2 Mon Sep 17 00:00:00 2001 From: Christine Lytwynec Date: Wed, 4 Nov 2015 15:14:09 -0500 Subject: [PATCH 004/115] Updating a11y tests --- .../tests/lms/test_account_settings.py | 6 +++ .../tests/lms/test_learner_profile.py | 16 ++++++-- .../tests/lms/test_lms_dashboard.py | 7 ++-- .../tests/studio/test_studio_library.py | 8 +++- .../tests/video/test_studio_video_module.py | 5 +++ .../tests/video/test_video_module.py | 41 ++++++++++++++++++- 6 files changed, 75 insertions(+), 8 deletions(-) diff --git a/common/test/acceptance/tests/lms/test_account_settings.py b/common/test/acceptance/tests/lms/test_account_settings.py index 906ffc8643..dc00718b53 100644 --- a/common/test/acceptance/tests/lms/test_account_settings.py +++ b/common/test/acceptance/tests/lms/test_account_settings.py @@ -459,4 +459,10 @@ class AccountSettingsA11yTest(AccountSettingsTestMixin, WebAppTest): """ self.log_in_as_unique_user() self.visit_account_settings_page() + self.account_settings_page.a11y_audit.config.set_rules({ + 'ignore': [ + 'link-href', # TODO: AC-233, AC-230 + 'skip-link', # TODO: AC-179 + ], + }) self.account_settings_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/lms/test_learner_profile.py b/common/test/acceptance/tests/lms/test_learner_profile.py index 207a7e7bef..d32a489451 100644 --- a/common/test/acceptance/tests/lms/test_learner_profile.py +++ b/common/test/acceptance/tests/lms/test_learner_profile.py @@ -765,10 +765,12 @@ class LearnerProfileA11yTest(LearnerProfileTestMixin, WebAppTest): username, _ = self.log_in_as_unique_user() profile_page = self.visit_profile_page(username) - # TODO: There are several existing color contrast errors on this page, - # we will ignore this error in the test until we fix them. profile_page.a11y_audit.config.set_rules({ - "ignore": ['color-contrast'], + "ignore": [ + 'color-contrast', # TODO: AC-232 + 'skip-link', # TODO: AC-179 + 'link-href', # TODO: AC-231 + ], }) profile_page.a11y_audit.check_for_accessibility_errors() @@ -791,4 +793,12 @@ class LearnerProfileA11yTest(LearnerProfileTestMixin, WebAppTest): different_username, _ = self.initialize_different_user(privacy=self.PRIVACY_PUBLIC) self.log_in_as_unique_user() profile_page = self.visit_profile_page(different_username) + + profile_page.a11y_audit.config.set_rules({ + "ignore": [ + 'skip-link', # TODO: AC-179 + 'link-href', # TODO: AC-231 + ], + }) + profile_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/lms/test_lms_dashboard.py b/common/test/acceptance/tests/lms/test_lms_dashboard.py index 4e8cbf599c..ec13ddfc41 100644 --- a/common/test/acceptance/tests/lms/test_lms_dashboard.py +++ b/common/test/acceptance/tests/lms/test_lms_dashboard.py @@ -233,10 +233,11 @@ class LmsDashboardA11yTest(BaseLmsDashboardTest): course_listings = self.dashboard_page.get_course_listings() self.assertEqual(len(course_listings), 1) - # There are several existing color contrast errors on this page, - # we will ignore this error in the test until we fix them. self.dashboard_page.a11y_audit.config.set_rules({ - "ignore": ['color-contrast'], + "ignore": [ + 'skip-link', # TODO: AC-179 + 'link-href', # TODO: AC-230 + ], }) self.dashboard_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/studio/test_studio_library.py b/common/test/acceptance/tests/studio/test_studio_library.py index fed2f5b8cf..facd11b6d6 100644 --- a/common/test/acceptance/tests/studio/test_studio_library.py +++ b/common/test/acceptance/tests/studio/test_studio_library.py @@ -656,7 +656,13 @@ class StudioLibraryA11yTest(StudioLibraryTest): # There are several existing color contrast errors on this page, # we will ignore this error in the test until we fix them. lib_page.a11y_audit.config.set_rules({ - "ignore": ['color-contrast'], + "ignore": [ + 'color-contrast', # TODO: AC-225 + 'link-href', # TODO: AC-226 + 'nav-aria-label', # TODO: AC-227 + 'skip-link', # TODO: AC-228 + 'icon-aria-hidden', # TODO: AC-229 + ], }) lib_page.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/video/test_studio_video_module.py b/common/test/acceptance/tests/video/test_studio_video_module.py index ff067e9fc7..ac9dde7f8e 100644 --- a/common/test/acceptance/tests/video/test_studio_video_module.py +++ b/common/test/acceptance/tests/video/test_studio_video_module.py @@ -344,6 +344,11 @@ class CMSVideoA11yTest(CMSVideoBaseTest): def test_video_player_a11y(self): # Limit the scope of the audit to the video player only. self.outline.a11y_audit.config.set_scope(include=["div.video"]) + self.outline.a11y_audit.config.set_rules({ + "ignore": [ + 'link-href', # TODO: AC-223 + ], + }) self._create_course_unit() self.outline.a11y_audit.check_for_accessibility_errors() diff --git a/common/test/acceptance/tests/video/test_video_module.py b/common/test/acceptance/tests/video/test_video_module.py index 825a37e6c2..fb2c1fc6e9 100644 --- a/common/test/acceptance/tests/video/test_video_module.py +++ b/common/test/acceptance/tests/video/test_video_module.py @@ -3,6 +3,9 @@ """ Acceptance tests for Video. """ +import os + +from mock import patch from nose.plugins.attrib import attr from unittest import skipIf, skip from ..helpers import UniqueCourseTest, is_youtube_available, YouTubeStubConfig @@ -30,7 +33,6 @@ HTML5_SOURCES_INCORRECT = [ ] -@attr('shard_4') @skipIf(is_youtube_available() is False, 'YouTube is not available!') class VideoBaseTest(UniqueCourseTest): """ @@ -192,6 +194,7 @@ class VideoBaseTest(UniqueCourseTest): self.video.wait_for_video_player_render() +@attr('shard_4') class YouTubeVideoTest(VideoBaseTest): """ Test YouTube Video Player """ @@ -849,6 +852,7 @@ class YouTubeVideoTest(VideoBaseTest): execute_video_steps(tab1_video_names) +@attr('shard_4') class YouTubeHtml5VideoTest(VideoBaseTest): """ Test YouTube HTML5 Video Player """ @@ -870,6 +874,7 @@ class YouTubeHtml5VideoTest(VideoBaseTest): self.assertTrue(self.video.is_video_rendered('youtube')) +@attr('shard_4') class Html5VideoTest(VideoBaseTest): """ Test HTML5 Video Player """ @@ -1055,6 +1060,7 @@ class Html5VideoTest(VideoBaseTest): self.assertTrue(all([source in HTML5_SOURCES for source in self.video.sources])) +@attr('shard_4') class YouTubeQualityTest(VideoBaseTest): """ Test YouTube Video Quality Button """ @@ -1099,3 +1105,36 @@ class YouTubeQualityTest(VideoBaseTest): self.video.click_player_button('quality') self.assertTrue(self.video.is_quality_button_active) + + +@attr('a11y') +class LMSVideoModuleA11yTest(VideoBaseTest): + """ + LMS Video Accessibility Test Class + """ + + def setUp(self): + browser = os.environ.get('SELENIUM_BROWSER', 'firefox') + + # the a11y tests run in CI under phantomjs which doesn't + # support html5 video or flash player, so the video tests + # don't work in it. We still want to be able to run these + # tests in CI, so override the browser setting if it is + # phantomjs. + if browser == 'phantomjs': + browser = 'firefox' + + with patch.dict(os.environ, {'SELENIUM_BROWSER': browser}): + super(LMSVideoModuleA11yTest, self).setUp() + + def test_video_player_a11y(self): + self.navigate_to_video() + + # Limit the scope of the audit to the video player only. + self.video.a11y_audit.config.set_scope(include=["div.video"]) + self.video.a11y_audit.config.set_rules({ + "ignore": [ + 'link-href', # TODO: AC-223 + ], + }) + self.video.a11y_audit.check_for_accessibility_errors() From a8c80535d3e74967c3640b698f91a896a0659f39 Mon Sep 17 00:00:00 2001 From: Syed Hassan Raza Date: Fri, 18 Sep 2015 04:49:19 -0700 Subject: [PATCH 005/115] html5 transcript sync --- .../xmodule/video_module/video_module.py | 19 ++++- .../courseware/tests/test_video_mongo.py | 76 ++++++++++++++++++- 2 files changed, 93 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 4e7ad7108d..1da9ed8a41 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -37,7 +37,7 @@ from xmodule.raw_module import EmptyDataRawDescriptor from xmodule.xml_module import is_pointer_tag, name_to_pathname, deserialize_field from xmodule.exceptions import NotFoundError -from .transcripts_utils import VideoTranscriptsMixin +from .transcripts_utils import VideoTranscriptsMixin, Transcript, get_html5_ids from .video_utils import create_youtube_string, get_video_from_cdn, get_poster from .bumper_utils import bumperize from .video_xfields import VideoFields @@ -425,6 +425,23 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler This should be fixed too. """ metadata_was_changed_by_user = old_metadata != own_metadata(self) + + # There is an edge case when old_metadata and own_metadata are same and we are importing transcript from youtube + # then there is a syncing issue where html5_subs are not syncing with youtube sub, We can make sync better by + # checking if transcript is present for the video and if any html5_ids transcript is not present then trigger + # the manage_video_subtitles_save to create the missing transcript with particular html5_id. + if not metadata_was_changed_by_user and self.sub and hasattr(self, 'html5_sources'): + html5_ids = get_html5_ids(self.html5_sources) + for subs_id in html5_ids: + try: + Transcript.asset(self.location, subs_id) + except NotFoundError: + # If a transcript does not not exist with particular html5_id then there is no need to check other + # html5_ids because we have to create a new transcript with this missing html5_id by turning on + # metadata_was_changed_by_user flag. + metadata_was_changed_by_user = True + break + if metadata_was_changed_by_user: manage_video_subtitles_save( self, diff --git a/lms/djangoapps/courseware/tests/test_video_mongo.py b/lms/djangoapps/courseware/tests/test_video_mongo.py index 701f244167..8d13c7046b 100644 --- a/lms/djangoapps/courseware/tests/test_video_mongo.py +++ b/lms/djangoapps/courseware/tests/test_video_mongo.py @@ -4,6 +4,7 @@ import ddt import itertools import json from collections import OrderedDict +from path import Path as path from lxml import etree from mock import patch, MagicMock, Mock @@ -17,7 +18,13 @@ from xmodule.video_module import VideoDescriptor, bumper_utils, video_utils from xmodule.x_module import STUDENT_VIEW from xmodule.tests.test_video import VideoDescriptorTestBase, instantiate_descriptor from xmodule.tests.test_import import DummySystem - +from xmodule.video_module.transcripts_utils import save_to_store, Transcript +from xmodule.modulestore.inheritance import own_metadata +from xmodule.contentstore.content import StaticContent +from xmodule.exceptions import NotFoundError +from xmodule.modulestore.tests.django_utils import ( + TEST_DATA_MONGO_MODULESTORE, TEST_DATA_SPLIT_MODULESTORE +) from edxval.api import ( create_profile, create_video, get_video_info, ValCannotCreateError, ValVideoNotFoundError ) @@ -866,6 +873,73 @@ class TestVideoDescriptorInitialization(BaseTestXmodule): self.assertFalse(self.item_descriptor.download_video) +@attr('shard_1') +@ddt.ddt +class TestEditorSavedMethod(BaseTestXmodule): + """ + Make sure that `editor_saved` method works correctly. + """ + CATEGORY = "video" + DATA = SOURCE_XML + METADATA = {} + + def setUp(self): + super(TestEditorSavedMethod, self).setUp() + self.setup_course() + self.metadata = { + 'source': 'http://youtu.be/3_yD_cEKoCk', + 'html5_sources': ['http://example.org/video.mp4'], + } + # path to subs_3_yD_cEKoCk.srt.sjson file + self.file_name = 'subs_3_yD_cEKoCk.srt.sjson' + # pylint: disable=no-value-for-parameter + self.test_dir = path(__file__).abspath().dirname().dirname().dirname().dirname().dirname() + self.file_path = self.test_dir + '/common/test/data/uploads/' + self.file_name + + @ddt.data(TEST_DATA_MONGO_MODULESTORE, TEST_DATA_SPLIT_MODULESTORE) + def test_editor_saved_when_html5_sub_not_exist(self, default_store): + """ + When there is youtube_sub exist but no html5_sub present for + html5_sources, editor_saved function will generate new html5_sub + for video. + """ + self.MODULESTORE = default_store # pylint: disable=invalid-name + self.initialize_module(metadata=self.metadata) + item = self.store.get_item(self.item_descriptor.location) + with open(self.file_path, "r") as myfile: + save_to_store(myfile.read(), self.file_name, 'text/sjson', item.location) + item.sub = "3_yD_cEKoCk" + # subs_video.srt.sjson does not exist before calling editor_saved function + with self.assertRaises(NotFoundError): + Transcript.get_asset(item.location, 'subs_video.srt.sjson') + old_metadata = own_metadata(item) + # calling editor_saved will generate new file subs_video.srt.sjson for html5_sources + item.editor_saved(self.user, old_metadata, None) + self.assertIsInstance(Transcript.get_asset(item.location, 'subs_3_yD_cEKoCk.srt.sjson'), StaticContent) + self.assertIsInstance(Transcript.get_asset(item.location, 'subs_video.srt.sjson'), StaticContent) + + @ddt.data(TEST_DATA_MONGO_MODULESTORE, TEST_DATA_SPLIT_MODULESTORE) + def test_editor_saved_when_youtube_and_html5_subs_exist(self, default_store): + """ + When both youtube_sub and html5_sub already exist then no new + sub will be generated by editor_saved function. + """ + self.MODULESTORE = default_store + self.initialize_module(metadata=self.metadata) + item = self.store.get_item(self.item_descriptor.location) + with open(self.file_path, "r") as myfile: + save_to_store(myfile.read(), self.file_name, 'text/sjson', item.location) + save_to_store(myfile.read(), 'subs_video.srt.sjson', 'text/sjson', item.location) + item.sub = "3_yD_cEKoCk" + # subs_3_yD_cEKoCk.srt.sjson and subs_video.srt.sjson already exist + self.assertIsInstance(Transcript.get_asset(item.location, self.file_name), StaticContent) + self.assertIsInstance(Transcript.get_asset(item.location, 'subs_video.srt.sjson'), StaticContent) + old_metadata = own_metadata(item) + with patch('xmodule.video_module.video_module.manage_video_subtitles_save') as manage_video_subtitles_save: + item.editor_saved(self.user, old_metadata, None) + self.assertFalse(manage_video_subtitles_save.called) + + @ddt.ddt class TestVideoDescriptorStudentViewJson(TestCase): """ From 270dc22c1fafbd55089bf120f0e00ecea53be2d7 Mon Sep 17 00:00:00 2001 From: Peter Fogg Date: Thu, 5 Nov 2015 10:22:43 -0500 Subject: [PATCH 006/115] Update display of dates on Course Home page. Adds UTC time to the "Today's Date" block, and a fuzzy/relative timestamp to all other displayed dates. ECOM-2807 --- lms/djangoapps/courseware/date_summary.py | 21 ++++++++++++++++++- .../courseware/tests/test_date_summary.py | 10 +++++++-- lms/static/sass/_developer.scss | 10 ++++----- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/courseware/date_summary.py b/lms/djangoapps/courseware/date_summary.py index 6db3ca1315..c1af8c460c 100644 --- a/lms/djangoapps/courseware/date_summary.py +++ b/lms/djangoapps/courseware/date_summary.py @@ -5,8 +5,10 @@ course-run-specific date which will be displayed to the user. """ from datetime import datetime +from babel.dates import format_timedelta from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ +from django.utils.translation import to_locale, get_language from edxmako.shortcuts import render_to_string from lazy import lazy import pytz @@ -68,7 +70,23 @@ class DateSummary(object): """Return the template context used to render this summary block.""" date = '' if self.date is not None: - date = self.date.strftime(self.date_format) + # Translators: relative_date is a fuzzy description of the + # time from now until absolute_date. For example, + # absolute_date might be "Jan 01, 2020", and if today were + # December 5th, 2020, relative_date would be "1 month". + locale = to_locale(get_language()) + try: + relative_date = format_timedelta(self.date - datetime.now(pytz.UTC), locale=locale) + # Babel doesn't have translations for Esperanto, so we get + # a KeyError when testing translations with + # ?preview-lang=eo. This should not happen with any other + # languages. See https://github.com/python-babel/babel/issues/107 + except KeyError: + relative_date = format_timedelta(self.date - datetime.now(pytz.UTC)) + date = _("in {relative_date} - {absolute_date}").format( + relative_date=relative_date, + absolute_date=self.date.strftime(self.date_format), + ) return { 'title': self.title, 'date': date, @@ -110,6 +128,7 @@ class TodaysDate(DateSummary): """ css_class = 'todays-date' is_enabled = True + date_format = '%b %d, %Y (%H:%M {utc})'.format(utc=_('UTC')) # The date is shown in the title, no need to display it again. def get_context(self): diff --git a/lms/djangoapps/courseware/tests/test_date_summary.py b/lms/djangoapps/courseware/tests/test_date_summary.py index 0d0fa634f0..4dae6f4d5f 100644 --- a/lms/djangoapps/courseware/tests/test_date_summary.py +++ b/lms/djangoapps/courseware/tests/test_date_summary.py @@ -146,11 +146,11 @@ class CourseDateSummaryTest(SharedModuleStoreTestCase): block = TodaysDate(self.course, self.user) self.assertTrue(block.is_enabled) self.assertEqual(block.date, datetime.now(pytz.UTC)) - self.assertEqual(block.title, 'Today is Jan 02, 2015') + self.assertEqual(block.title, 'Today is Jan 02, 2015 (00:00 UTC)') self.assertNotIn('date-summary-date', block.render()) @freezegun.freeze_time('2015-01-02') - def test_date_render(self): + def test_todays_date_render(self): self.setup_course_and_user() block = TodaysDate(self.course, self.user) self.assertIn('Jan 02, 2015', block.render()) @@ -162,6 +162,12 @@ class CourseDateSummaryTest(SharedModuleStoreTestCase): block = CourseStartDate(self.course, self.user) self.assertEqual(block.date, self.course.start) + @freezegun.freeze_time('2015-01-02') + def test_start_date_render(self): + self.setup_course_and_user() + block = CourseStartDate(self.course, self.user) + self.assertIn('in 1 day - Jan 03, 2015', block.render()) + ## CourseEndDate def test_course_end_date_during_course(self): diff --git a/lms/static/sass/_developer.scss b/lms/static/sass/_developer.scss index 6f43b80490..b37c0b16e7 100644 --- a/lms/static/sass/_developer.scss +++ b/lms/static/sass/_developer.scss @@ -380,10 +380,6 @@ background-color: $gray-l4; @include border-left(3px solid $gray-l3); - .heading { - @include float(left); - } - .description { margin-top: $baseline/2; margin-bottom: $baseline/2; @@ -402,13 +398,17 @@ } .date { - @include float(right); color: $lighter-base-font-color; font-size: 80%; } &-todays-date { @include border-left(3px solid $blue); + + .heading { + font-weight: $font-regular; + font-size: 80%; + } } &-verified-upgrade-deadline { From 00b3051b73a2f69b8d2c201a48c156cacc95ff09 Mon Sep 17 00:00:00 2001 From: "J. Cliff Dyer" Date: Mon, 2 Nov 2015 22:17:00 +0000 Subject: [PATCH 007/115] Make sure proper logic is used for getting account privacy. --- .../teams/tests/test_serializers.py | 2 +- lms/djangoapps/teams/tests/test_views.py | 2 +- .../user_api/accounts/serializers.py | 10 ++++---- .../user_api/accounts/tests/test_api.py | 6 ++--- .../user_api/accounts/tests/test_views.py | 24 ++++++++++++------- 5 files changed, 26 insertions(+), 18 deletions(-) diff --git a/lms/djangoapps/teams/tests/test_serializers.py b/lms/djangoapps/teams/tests/test_serializers.py index 55323d638e..189b8fa349 100644 --- a/lms/djangoapps/teams/tests/test_serializers.py +++ b/lms/djangoapps/teams/tests/test_serializers.py @@ -66,7 +66,7 @@ class MembershipSerializerTestCase(SerializerTestCase): 'image_url_small': 'http://testserver/static/default_30.png', 'has_image': False }, - 'account_privacy': None + 'account_privacy': 'private' }) self.assertNotIn('membership', data['team']) diff --git a/lms/djangoapps/teams/tests/test_views.py b/lms/djangoapps/teams/tests/test_views.py index 4cdab1a37a..a498a26f30 100644 --- a/lms/djangoapps/teams/tests/test_views.py +++ b/lms/djangoapps/teams/tests/test_views.py @@ -129,7 +129,7 @@ class TestDashboard(SharedModuleStoreTestCase): team.add_user(self.user) # Check the query count on the dashboard again - with self.assertNumQueries(20): + with self.assertNumQueries(19): self.client.get(self.teams_url) def test_bad_course_id(self): diff --git a/openedx/core/djangoapps/user_api/accounts/serializers.py b/openedx/core/djangoapps/user_api/accounts/serializers.py index 024fcd3ad6..976706189d 100644 --- a/openedx/core/djangoapps/user_api/accounts/serializers.py +++ b/openedx/core/djangoapps/user_api/accounts/serializers.py @@ -91,7 +91,7 @@ class UserReadOnlySerializer(serializers.Serializer): "level_of_education": AccountLegacyProfileSerializer.convert_empty_to_None(profile.level_of_education), "mailing_address": profile.mailing_address, "requires_parental_consent": profile.requires_parental_consent(), - "account_privacy": UserPreference.get_value(user, 'account_privacy'), + "account_privacy": self._get_profile_visibility(profile, user), } return self._filter_fields( @@ -185,19 +185,19 @@ class AccountLegacyProfileSerializer(serializers.HyperlinkedModelSerializer, Rea raise serializers.ValidationError("The language_proficiencies field must consist of unique languages") return value - def transform_gender(self, user_profile, value): + def transform_gender(self, user_profile, value): # pylint: disable=unused-argument """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ return AccountLegacyProfileSerializer.convert_empty_to_None(value) - def transform_country(self, user_profile, value): + def transform_country(self, user_profile, value): # pylint: disable=unused-argument """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ return AccountLegacyProfileSerializer.convert_empty_to_None(value) - def transform_level_of_education(self, user_profile, value): + def transform_level_of_education(self, user_profile, value): # pylint: disable=unused-argument """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ return AccountLegacyProfileSerializer.convert_empty_to_None(value) - def transform_bio(self, user_profile, value): + def transform_bio(self, user_profile, value): # pylint: disable=unused-argument """ Converts empty string to None, to indicate not set. Replaced by to_representation in version 3. """ return AccountLegacyProfileSerializer.convert_empty_to_None(value) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index 7c6bc230fe..f58e1a03ab 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -25,7 +25,7 @@ from ...errors import ( from ..api import ( get_account_settings, update_account_settings, create_account, activate_account, request_password_change ) -from .. import USERNAME_MAX_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MAX_LENGTH +from .. import USERNAME_MAX_LENGTH, EMAIL_MAX_LENGTH, PASSWORD_MAX_LENGTH, PRIVATE_VISIBILITY def mock_render_to_string(template_name, context): @@ -278,7 +278,7 @@ class AccountSettingsOnCreationTest(TestCase): }, 'requires_parental_consent': True, 'language_proficiencies': [], - 'account_privacy': None + 'account_privacy': PRIVATE_VISIBILITY, }) @@ -404,7 +404,7 @@ class AccountCreationActivationAndPasswordChangeTest(TestCase): # Verify that the body of the message contains something that looks # like an activation link email_body = mail.outbox[0].body - result = re.search('(?Phttps?://[^\s]+)', email_body) + result = re.search(r'(?Phttps?://[^\s]+)', email_body) self.assertIsNot(result, None) @unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in LMS') diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 5bc49ced6b..7578eb46a7 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -241,7 +241,7 @@ class TestAccountAPI(UserAPITestCase): self.create_mock_profile(self.user) with self.assertNumQueries(9): response = self.send_get(self.different_client) - self._verify_full_shareable_account_response(response) + self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY) # Note: using getattr so that the patching works even if there is no configuration. # This is needed when testing CMS as the patching is still executed even though the @@ -256,7 +256,7 @@ class TestAccountAPI(UserAPITestCase): self.create_mock_profile(self.user) with self.assertNumQueries(9): response = self.send_get(self.different_client) - self._verify_private_account_response(response) + self._verify_private_account_response(response, account_privacy=PRIVATE_VISIBILITY) @ddt.data( ("client", "user", PRIVATE_VISIBILITY), @@ -305,7 +305,7 @@ class TestAccountAPI(UserAPITestCase): """ Internal helper to perform the actual assertions """ - with self.assertNumQueries(8): + with self.assertNumQueries(7): response = self.send_get(self.client) data = response.data self.assertEqual(16, len(data)) @@ -322,7 +322,7 @@ class TestAccountAPI(UserAPITestCase): self._verify_profile_image_data(data, False) self.assertTrue(data["requires_parental_consent"]) self.assertEqual([], data["language_proficiencies"]) - self.assertEqual(None, data["account_privacy"]) + self.assertEqual(PRIVATE_VISIBILITY, data["account_privacy"]) self.client.login(username=self.user.username, password=self.test_password) verify_get_own_information() @@ -344,7 +344,7 @@ class TestAccountAPI(UserAPITestCase): legacy_profile.save() self.client.login(username=self.user.username, password=self.test_password) - with self.assertNumQueries(8): + with self.assertNumQueries(7): response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "bio"): self.assertIsNone(response.data[empty_field]) @@ -403,6 +403,14 @@ class TestAccountAPI(UserAPITestCase): Test the behavior of patch, when using the correct content_type. """ client = self.login_client("client", "user") + + if field == 'account_privacy': + # Ensure the user has birth year set, and is over 13, so + # account_privacy behaves normally + legacy_profile = UserProfile.objects.get(id=self.user.id) + legacy_profile.year_of_birth = 2000 + legacy_profile.save() + response = self.send_patch(client, {field: value}) self.assertEqual(value, response.data[field]) @@ -687,16 +695,16 @@ class TestAccountAPI(UserAPITestCase): self.assertIsNotNone(data["date_joined"]) self._verify_profile_image_data(data, False) self.assertTrue(data["requires_parental_consent"]) - self.assertEqual(ALL_USERS_VISIBILITY, data["account_privacy"]) + self.assertEqual(PRIVATE_VISIBILITY, data["account_privacy"]) else: self._verify_private_account_response( - response, requires_parental_consent=True, account_privacy=ALL_USERS_VISIBILITY + response, requires_parental_consent=True, account_privacy=PRIVATE_VISIBILITY ) # Verify that the shared view is still private response = self.send_get(client, query_parameters='view=shared') self._verify_private_account_response( - response, requires_parental_consent=True, account_privacy=ALL_USERS_VISIBILITY + response, requires_parental_consent=True, account_privacy=PRIVATE_VISIBILITY ) From 5d8830096a6ca97b0d48cf562430d360ce101cb3 Mon Sep 17 00:00:00 2001 From: Shawn Milochik Date: Fri, 6 Nov 2015 12:49:19 -0500 Subject: [PATCH 008/115] Added max score to output of get_student_grade_summary_data. This will put the actual score and actual max score in scope for the the return_csv function, so actual scores can be dumped. The ultimate goal is to provide this data in the CSV dump that is passed to Stellar via pylmod. This is PR #10395 on edX, and issue 95 on mitocw's edx fork. https://github.com/edx/edx-platform/pull/10395 https://github.com/mitocw/edx-platform/issues/95 --- AUTHORS | 3 +- .../tests/test_legacy_raw_download_csv.py | 29 +++++++++++++++++-- lms/djangoapps/instructor/views/legacy.py | 29 ++++++++++++++++--- 3 files changed, 54 insertions(+), 7 deletions(-) diff --git a/AUTHORS b/AUTHORS index 0c07f8fcd3..28ee38920c 100644 --- a/AUTHORS +++ b/AUTHORS @@ -253,4 +253,5 @@ Justin Abrahms Arbab Nazar Douglas Hall Awais Jibran -Muhammad Rehan \ No newline at end of file +Muhammad Rehan +Shawn Milochik diff --git a/lms/djangoapps/instructor/tests/test_legacy_raw_download_csv.py b/lms/djangoapps/instructor/tests/test_legacy_raw_download_csv.py index a39e7356ec..24adeeec8a 100644 --- a/lms/djangoapps/instructor/tests/test_legacy_raw_download_csv.py +++ b/lms/djangoapps/instructor/tests/test_legacy_raw_download_csv.py @@ -69,7 +69,7 @@ class TestRawGradeCSV(TestSubmittingProblems): def get_expected_grade_data( self, get_grades=True, get_raw_scores=False, - use_offline=False): + use_offline=False, get_score_max=False): """ Return expected results from the get_student_grade_summary_data call with any options selected. @@ -79,6 +79,11 @@ class TestRawGradeCSV(TestSubmittingProblems): get_student_grade_summary_data for this function to be accurate. If kwargs are added or removed, or the functionality triggered by them changes, this function must be updated to match. + + If get_score_max is True, instead of a single score between 0 and 1, + the actual score and total possible are returned. For example, if the + student got one out of two possible points, the values (1, 2) will be + returned instead of 0.5. """ expected_data = { 'students': [self.student_user, self.student_user2], @@ -155,6 +160,10 @@ class TestRawGradeCSV(TestSubmittingProblems): u'ID', u'Username', u'Full Name', u'edX email', u'External email', u'p3', u'p2', u'p1' ] + # Strip out the single-value float scores and replace them + # with two-tuples of actual and possible scores (see docstring). + if get_score_max: + expected_data["data"][-1][-3:] = (0.0, 1), (1.0, 1.0), (0.0, 1) return expected_data @@ -197,7 +206,7 @@ class TestRawGradeCSV(TestSubmittingProblems): request, self.course, get_grades=False ) expected_data = self.get_expected_grade_data(get_grades=False) - # if get_grades == False, get_expected_grade_data does not + # if get_grades is False, get_expected_grade_data does not # add an "assignments" key. self.assertNotIn("assignments", expected_data) self.compare_data(data, expected_data) @@ -228,6 +237,22 @@ class TestRawGradeCSV(TestSubmittingProblems): ) self.compare_data(data, expected_data) + def test_grade_summary_data_get_score_max(self): + """ + Test grade summary data report generation with get_score_max set + to True (also requires get_raw_scores to be True). + """ + request = DummyRequest() + self.answer_question() + data = get_student_grade_summary_data( + request, self.course, use_offline=True, get_raw_scores=True, + get_score_max=True, + ) + expected_data = self.get_expected_grade_data( + use_offline=True, get_raw_scores=True, get_score_max=True, + ) + self.compare_data(data, expected_data) + def compare_data(self, data, expected_data): """ Compare the output of the get_student_grade_summary_data diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index ac9e07261c..f93dfd06c6 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -631,17 +631,21 @@ class GradeTable(object): self.grades = {} self._current_row = {} - def _add_grade_to_row(self, component, score): + def _add_grade_to_row(self, component, score, possible=None): """Creates component if needed, and assigns score Args: component (str): Course component being graded score (float): Score of student on component + possible (float): Max possible score for the component Returns: None """ component_index = self.components.setdefault(component, len(self.components)) + if possible is not None: + # send a tuple instead of a single value + score = (score, possible) self._current_row[component_index] = score @contextmanager @@ -681,7 +685,10 @@ class GradeTable(object): return self.components.keys() -def get_student_grade_summary_data(request, course, get_grades=True, get_raw_scores=False, use_offline=False): +def get_student_grade_summary_data( + request, course, get_grades=True, get_raw_scores=False, + use_offline=False, get_score_max=False +): """ Return data arrays with student identity and grades for specified course. @@ -697,6 +704,11 @@ def get_student_grade_summary_data(request, course, get_grades=True, get_raw_sco data = list (one per student) of lists of data corresponding to the fields If get_raw_scores=True, then instead of grade summaries, the raw grades for all graded modules are returned. + + If get_score_max is True, two values will be returned for each grade -- the + total number of points earned and the total number of points possible. For + example, if two points are possible and one is earned, (1, 2) will be + returned instead of 0.5 (the default). """ course_key = course.id enrolled_students = User.objects.filter( @@ -723,9 +735,18 @@ def get_student_grade_summary_data(request, course, get_grades=True, get_raw_sco log.debug(u'student=%s, gradeset=%s', student, gradeset) with gtab.add_row(student.id) as add_grade: if get_raw_scores: - # TODO (ichuang) encode Score as dict instead of as list, so score[0] -> score['earned'] + # The following code calls add_grade, which is an alias + # for the add_row method on the GradeTable class. This adds + # a grade for each assignment. Depending on whether + # get_score_max is True, it will return either a single + # value as a float between 0 and 1, or a two-tuple + # containing the earned score and possible score for + # the assignment (see docstring). for score in gradeset['raw_scores']: - add_grade(score.section, getattr(score, 'earned', score[0])) + if get_score_max is True: + add_grade(score.section, score.earned, score.possible) + else: + add_grade(score.section, score.earned) else: for grade_item in gradeset['section_breakdown']: add_grade(grade_item['label'], grade_item['percent']) From 73d8f064997bcf1e1997a93addf962fa1acf29cd Mon Sep 17 00:00:00 2001 From: louyihua Date: Sat, 7 Nov 2015 13:34:59 +0800 Subject: [PATCH 009/115] Fix gettext guidance violation 1. No string concatenation should be used in the gettext function. 2. Some extra parentheses should be used in coffee script, to avoid the following situation: in coffee script, the call ```gettext "text to be extracted" + "text should not be extracted"``` will be translated into ```gettext("text to be extracted" + "text should not be extracted")``` rather than ```gettext("text to be extracted") + "text should not be extracted"```. --- .../xmodule/js/src/combinedopenended/display.coffee | 2 +- .../js/student_account/views/account_settings_fields.js | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee index ff1120a5bd..86fae4d265 100644 --- a/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee +++ b/common/lib/xmodule/xmodule/js/src/combinedopenended/display.coffee @@ -325,7 +325,7 @@ class @CombinedOpenEnded @submit_button.hide() @queueing() @grader_status = @$(@grader_status_sel) - @grader_status.html("" + gettext "Your response has been submitted. Please check back later for your grade." + "") + @grader_status.html("" + (gettext "Your response has been submitted. Please check back later for your grade.") + "") else if @child_type == "selfassessment" @setup_score_selection() else if @child_state == 'post_assessment' diff --git a/lms/static/js/student_account/views/account_settings_fields.js b/lms/static/js/student_account/views/account_settings_fields.js index e2cf8e3e26..af90b9b912 100644 --- a/lms/static/js/student_account/views/account_settings_fields.js +++ b/lms/static/js/student_account/views/account_settings_fields.js @@ -11,8 +11,8 @@ successMessage: function() { return this.indicators.success + interpolate_text( gettext( - 'We\'ve sent a confirmation message to {new_email_address}. ' + - 'Click the link in the message to update your email address.' + /* jshint maxlen: false */ + 'We\'ve sent a confirmation message to {new_email_address}. Click the link in the message to update your email address.' ), {'new_email_address': this.fieldValue()} ); @@ -79,8 +79,8 @@ successMessage: function () { return this.indicators.success + interpolate_text( gettext( - 'We\'ve sent a message to {email_address}. ' + - 'Click the link in the message to reset your password.' + /* jshint maxlen: false */ + 'We\'ve sent a message to {email_address}. Click the link in the message to reset your password.' ), {'email_address': this.model.get(this.options.emailAttribute)} ); From 3102788bd1c64b8553e768ae148b7205dc050f0c Mon Sep 17 00:00:00 2001 From: Syed Hassan Raza Date: Wed, 14 Oct 2015 05:42:01 -0700 Subject: [PATCH 010/115] Delete course entry from CourseAboutSearchIndex --- .../contentstore/courseware_index.py | 19 ++++++++++++++ .../tests/test_courseware_index.py | 26 ++++++++++++++++++- .../content/course_overviews/signals.py | 4 +++ 3 files changed, 48 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index bcf26f3572..2a05caf64c 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -640,3 +640,22 @@ class CourseAboutSearchIndexer(object): "Successfully added %s course to the course discovery index", course_id ) + + @classmethod + def _get_location_info(cls, normalized_structure_key): + """ Builds location info dictionary """ + return {"course": unicode(normalized_structure_key), "org": normalized_structure_key.org} + + @classmethod + def remove_deleted_items(cls, structure_key): + """ Remove item from Course About Search_index """ + searcher = SearchEngine.get_search_engine(cls.INDEX_NAME) + if not searcher: + return + + response = searcher.search( + doc_type=cls.DISCOVERY_DOCUMENT_TYPE, + field_dictionary=cls._get_location_info(structure_key) + ) + result_ids = [result["data"]["id"] for result in response["results"]] + searcher.remove(cls.DISCOVERY_DOCUMENT_TYPE, result_ids) diff --git a/cms/djangoapps/contentstore/tests/test_courseware_index.py b/cms/djangoapps/contentstore/tests/test_courseware_index.py index e146cb914c..1b290a29d6 100644 --- a/cms/djangoapps/contentstore/tests/test_courseware_index.py +++ b/cms/djangoapps/contentstore/tests/test_courseware_index.py @@ -17,7 +17,7 @@ from django.conf import settings from course_modes.models import CourseMode from xmodule.library_tools import normalize_key_for_search from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import SignalHandler +from xmodule.modulestore.django import SignalHandler, modulestore from xmodule.modulestore.edit_info import EditInfoMixin from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.inheritance import InheritanceMixin @@ -335,6 +335,25 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): response = self.search() self.assertEqual(response["total"], 5) + def _test_delete_course_from_search_index_after_course_deletion(self, store): # pylint: disable=invalid-name + """ + Test that course will also be delete from search_index after course deletion. + """ + self.DOCUMENT_TYPE = 'course_info' # pylint: disable=invalid-name + response = self.search() + self.assertEqual(response["total"], 0) + + # index the course in search_index + self.reindex_course(store) + response = self.search() + self.assertEqual(response["total"], 1) + + # delete the course and look course in search_index + modulestore().delete_course(self.course.id, self.user_id) + self.assertIsNone(modulestore().get_course(self.course.id)) + response = self.search() + self.assertEqual(response["total"], 0) + def _test_deleting_item(self, store): """ test deleting an item """ # Publish the vertical to start with @@ -604,6 +623,11 @@ class TestCoursewareSearchIndexer(MixedWithOptionsTestCase): def test_course_location_null(self, store_type): self._perform_test_using_store(store_type, self._test_course_location_null) + @ddt.data(*WORKS_WITH_STORES) + def test_delete_course_from_search_index_after_course_deletion(self, store_type): + """ Test for removing course from CourseAboutSearchIndexer """ + self._perform_test_using_store(store_type, self._test_delete_course_from_search_index_after_course_deletion) + @patch('django.conf.settings.SEARCH_ENGINE', 'search.tests.utils.ForceRefreshElasticSearchEngine') @ddt.ddt diff --git a/openedx/core/djangoapps/content/course_overviews/signals.py b/openedx/core/djangoapps/content/course_overviews/signals.py index 26dcca6490..951ebbd65f 100644 --- a/openedx/core/djangoapps/content/course_overviews/signals.py +++ b/openedx/core/djangoapps/content/course_overviews/signals.py @@ -24,3 +24,7 @@ def _listen_for_course_delete(sender, course_key, **kwargs): # pylint: disable= invalidates the corresponding CourseOverview cache entry if one exists. """ CourseOverview.objects.filter(id=course_key).delete() + # import CourseAboutSearchIndexer inline due to cyclic import + from cms.djangoapps.contentstore.courseware_index import CourseAboutSearchIndexer + # Delete course entry from Course About Search_index + CourseAboutSearchIndexer.remove_deleted_items(course_key) From 10e8435b3ac19efd389aa4b50baf5bde380deb98 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Sun, 8 Nov 2015 21:32:11 -0500 Subject: [PATCH 011/115] Bump coverage.py to 4.0.2 --- requirements/edx/base.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 5916c173dd..65769a903f 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -123,7 +123,7 @@ astroid==1.3.8 before_after==0.1.3 bok-choy==0.4.7 chrono==1.0.2 -coverage==4.0 +coverage==4.0.2 ddt==0.8.0 diff-cover==0.8.0 django-crum==0.5 From 58e0633eaee55ddefbdf84c833b4e179347c122d Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 9 Nov 2015 06:11:42 -0500 Subject: [PATCH 012/115] This script is now in github.com/nedbat/xunit_tools --- scripts/summarize_test_results.py | 127 ------------------------------ 1 file changed, 127 deletions(-) delete mode 100755 scripts/summarize_test_results.py diff --git a/scripts/summarize_test_results.py b/scripts/summarize_test_results.py deleted file mode 100755 index 32f1879ed4..0000000000 --- a/scripts/summarize_test_results.py +++ /dev/null @@ -1,127 +0,0 @@ -#!/usr/bin/env python -"""Summarize the results of running all the tests. - -See the report_all_files docstring for details, or run this with --help. - -""" - -import collections -import os - -import click -from lxml import etree - - -@click.command() -@click.option("--errors/--no-errors", help="Show details of errors") -@click.option("--names/--no-names", help="Show all test names") -@click.option("--outcomes/--no-outcomes", help="Show pass/fail/error with names") -@click.argument("start", default="reports") -def report_all_files(errors, names, outcomes, start): - """Find all the nosetests.xml files, and report on them. - - For every nosetests.xml file found, prints a summary of the number of - tests, fails, errors, etc. If --details is used, then the error messages - from all of the fails and errors will be shown, most frequent first, with a - count of how many tests failed for that reason. - - """ - totals = TestResults() - for dirpath, _, filenames in os.walk(start): - if "nosetests.xml" in filenames: - results = report_file( - os.path.join(dirpath, "nosetests.xml"), - errors=errors, - names=names, - outcomes=outcomes, - ) - totals += results - - print "\nTotals:\n{}".format(totals) - - -class Summable(object): - """An object whose attributes can be added together easily. - - Subclass this and define `fields` on your derived class. - - """ - def __init__(self): - for name in self.fields: - setattr(self, name, 0) - - @classmethod - def from_element(cls, element): - """Construct a Summable from an xml element with the same attributes.""" - self = cls() - for name in self.fields: - setattr(self, name, int(element.get(name))) - return self - - def __add__(self, other): - result = type(self)() - for name in self.fields: - setattr(result, name, getattr(self, name) + getattr(other, name)) - return result - - -class TestResults(Summable): - """A test result, makeable from a nosetests.xml element.""" - - fields = ["tests", "errors", "failures", "skip"] - - def __str__(self): - msg = "{0.tests:4d} tests, {0.errors} errors, {0.failures} failures, {0.skip} skipped" - return msg.format(self) - - -def error_line_from_error_element(element): - """Given an element, get the important error line from it.""" - return element.get("message").splitlines()[0] - - -def report_file(path, errors, names, outcomes): - """Report on one nosetests.xml file.""" - print "\n{}".format(path) - with open(path) as xml_file: - tree = etree.parse(xml_file) # pylint: disable=no-member - suite = tree.xpath("/testsuite")[0] - - results = TestResults.from_element(suite) - print results - - if errors: - errors = collections.Counter() - for error_element in tree.xpath(".//error|.//failure"): - errors[error_line_from_error_element(error_element)] += 1 - - if errors: - print "" - for error_message, number in errors.most_common(): - print "{0:4d}: {1}".format(number, error_message) - - if names: - for testcase in tree.xpath(".//testcase"): - if outcomes: - result = testcase.xpath("*") - if result: - outcome = result[0].tag - if outcome == "system-out": - outcome = "." - else: - outcome = outcome[0].upper() - else: - outcome = "." - else: - outcome = "" - print " {outcome} {classname}.{name}".format( - outcome=outcome, - classname=testcase.get("classname"), - name=testcase.get("name"), - ) - - return results - - -if __name__ == "__main__": - report_all_files() # pylint: disable=no-value-for-parameter From 0646f9bfcfd66513533474a011dd3d7cf308c557 Mon Sep 17 00:00:00 2001 From: christopher lee Date: Fri, 6 Nov 2015 11:41:37 -0500 Subject: [PATCH 013/115] MA-1637 Discussion API now uses the view_auth_classes for permissions Added missing unit tests. --- .../discussion_api/tests/test_views.py | 8 ++++-- lms/djangoapps/discussion_api/views.py | 26 +++++++------------ 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/discussion_api/tests/test_views.py b/lms/djangoapps/discussion_api/tests/test_views.py index b2c1090440..8a64d3582f 100644 --- a/lms/djangoapps/discussion_api/tests/test_views.py +++ b/lms/djangoapps/discussion_api/tests/test_views.py @@ -71,6 +71,10 @@ class DiscussionAPIViewTestMixin(CommentsServiceMockMixin, UrlResetMixin): {"developer_message": "Authentication credentials were not provided."} ) + def test_inactive(self): + self.user.is_active = False + self.test_basic() + @mock.patch.dict("django.conf.settings.FEATURES", {"ENABLE_DISCUSSION_SERVICE": True}) class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): @@ -89,7 +93,7 @@ class CourseViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): {"developer_message": "Course not found."} ) - def test_get_success(self): + def test_basic(self): response = self.client.get(self.url) self.assert_response_correct( response, @@ -150,7 +154,7 @@ class CourseTopicsViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase): {"developer_message": "Course not found."} ) - def test_get_success(self): + def test_basic(self): response = self.client.get(self.url) self.assert_response_correct( response, diff --git a/lms/djangoapps/discussion_api/views.py b/lms/djangoapps/discussion_api/views.py index f6d07cfad4..612010c448 100644 --- a/lms/djangoapps/discussion_api/views.py +++ b/lms/djangoapps/discussion_api/views.py @@ -3,9 +3,6 @@ Discussion API views """ from django.core.exceptions import ValidationError -from rest_framework.authentication import SessionAuthentication -from rest_framework_oauth.authentication import OAuth2Authentication -from rest_framework.permissions import IsAuthenticated from rest_framework.response import Response from rest_framework.views import APIView from rest_framework.viewsets import ViewSet @@ -28,19 +25,11 @@ from discussion_api.api import ( update_thread, ) from discussion_api.forms import CommentListGetForm, ThreadListGetForm, _PaginationForm -from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin +from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes -class _ViewMixin(object): - """ - Mixin to provide common characteristics and utility functions for Discussion - API views - """ - authentication_classes = (OAuth2Authentication, SessionAuthentication) - permission_classes = (IsAuthenticated,) - - -class CourseView(_ViewMixin, DeveloperErrorViewMixin, APIView): +@view_auth_classes() +class CourseView(DeveloperErrorViewMixin, APIView): """ **Use Cases** @@ -72,7 +61,8 @@ class CourseView(_ViewMixin, DeveloperErrorViewMixin, APIView): return Response(get_course(request, course_key)) -class CourseTopicsView(_ViewMixin, DeveloperErrorViewMixin, APIView): +@view_auth_classes() +class CourseTopicsView(DeveloperErrorViewMixin, APIView): """ **Use Cases** @@ -106,7 +96,8 @@ class CourseTopicsView(_ViewMixin, DeveloperErrorViewMixin, APIView): return Response(response) -class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): +@view_auth_classes() +class ThreadViewSet(DeveloperErrorViewMixin, ViewSet): """ **Use Cases** @@ -294,7 +285,8 @@ class ThreadViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): return Response(status=204) -class CommentViewSet(_ViewMixin, DeveloperErrorViewMixin, ViewSet): +@view_auth_classes() +class CommentViewSet(DeveloperErrorViewMixin, ViewSet): """ **Use Cases** From 69b261775eccf5aa4d419fcd865e9bc74f1fea33 Mon Sep 17 00:00:00 2001 From: Alison Hodges Date: Mon, 9 Nov 2015 17:15:08 -0500 Subject: [PATCH 014/115] Update example location to split style --- .../instructor/instructor_dashboard_2/student_admin.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/templates/instructor/instructor_dashboard_2/student_admin.html b/lms/templates/instructor/instructor_dashboard_2/student_admin.html index a7d3b92279..3c15715237 100644 --- a/lms/templates/instructor/instructor_dashboard_2/student_admin.html +++ b/lms/templates/instructor/instructor_dashboard_2/student_admin.html @@ -54,7 +54,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

+ block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

${_("Next, select an action to perform for the given user and problem:")} @@ -141,7 +141,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

+ block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

${_("Then select an action")}: From f475200a16144f3ca066d8ca5cd46240042729ca Mon Sep 17 00:00:00 2001 From: Alison Hodges Date: Mon, 9 Nov 2015 17:22:00 -0500 Subject: [PATCH 015/115] Revert "Update example location to split style" This reverts commit 69b261775eccf5aa4d419fcd865e9bc74f1fea33. --- .../instructor/instructor_dashboard_2/student_admin.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/templates/instructor/instructor_dashboard_2/student_admin.html b/lms/templates/instructor/instructor_dashboard_2/student_admin.html index 3c15715237..a7d3b92279 100644 --- a/lms/templates/instructor/instructor_dashboard_2/student_admin.html +++ b/lms/templates/instructor/instructor_dashboard_2/student_admin.html @@ -54,7 +54,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

+ i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

${_("Next, select an action to perform for the given user and problem:")} @@ -141,7 +141,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

+ i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

${_("Then select an action")}: From 056225e3ac9269c0aa6cf34246d6a8b36953de13 Mon Sep 17 00:00:00 2001 From: Alison Hodges Date: Mon, 9 Nov 2015 17:26:28 -0500 Subject: [PATCH 016/115] Updates the example locations to split format --- .../instructor/instructor_dashboard_2/student_admin.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/templates/instructor/instructor_dashboard_2/student_admin.html b/lms/templates/instructor/instructor_dashboard_2/student_admin.html index a7d3b92279..3c15715237 100644 --- a/lms/templates/instructor/instructor_dashboard_2/student_admin.html +++ b/lms/templates/instructor/instructor_dashboard_2/student_admin.html @@ -54,7 +54,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

+ block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

${_("Next, select an action to perform for the given user and problem:")} @@ -141,7 +141,7 @@ ## Translators: A location (string of text) follows this sentence.

${_("You must provide the complete location of the problem. In the Staff Debug viewer, the location looks like this:")}
- i4x://edX/Open_DemoX/problem/78c98390884243b89f6023745231c525

+ block-v1:edX+DemoX+2015+type@problem+block@618c5933b8b544e4a4cc103d3e508378

${_("Then select an action")}: From ed6e46453c665c7a71d0eba41981c94ebc2867bb Mon Sep 17 00:00:00 2001 From: Afeef Janjua Date: Wed, 4 Nov 2015 18:55:57 +0500 Subject: [PATCH 017/115] allow reverification for pending status ('must_retry', 'submitted') --- AUTHORS | 1 + lms/djangoapps/verify_student/tests/test_views.py | 14 +++++++++++--- lms/djangoapps/verify_student/views.py | 6 +++++- 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/AUTHORS b/AUTHORS index 28ee38920c..c04dc2c40e 100644 --- a/AUTHORS +++ b/AUTHORS @@ -255,3 +255,4 @@ Douglas Hall Awais Jibran Muhammad Rehan Shawn Milochik +Afeef Janjua diff --git a/lms/djangoapps/verify_student/tests/test_views.py b/lms/djangoapps/verify_student/tests/test_views.py index 82127df3ce..588690dd74 100644 --- a/lms/djangoapps/verify_student/tests/test_views.py +++ b/lms/djangoapps/verify_student/tests/test_views.py @@ -1870,14 +1870,22 @@ class TestReverifyView(TestCase): # Allow the student to reverify self._assert_can_reverify() - def test_reverify_view_cannot_reverify_pending(self): + def test_reverify_view_can_reverify_pending(self): + """ Test that the user can still re-verify even if the previous photo + verification is in pending state. + + A photo verification is considered in pending state when the user has + either submitted the photo verification (status in database: 'submitted') + or photo verification submission failed (status in database: 'must_retry'). + """ + # User has submitted a verification attempt, but Software Secure has not yet responded attempt = SoftwareSecurePhotoVerification.objects.create(user=self.user) attempt.mark_ready() attempt.submit() - # Cannot reverify because an attempt has already been submitted. - self._assert_cannot_reverify() + # Can re-verify because an attempt has already been submitted. + self._assert_can_reverify() def test_reverify_view_cannot_reverify_approved(self): # Submitted attempt has been approved diff --git a/lms/djangoapps/verify_student/views.py b/lms/djangoapps/verify_student/views.py index 2710944365..46d831d523 100644 --- a/lms/djangoapps/verify_student/views.py +++ b/lms/djangoapps/verify_student/views.py @@ -1350,7 +1350,11 @@ class ReverifyView(View): Backbone views used in the initial verification flow. """ status, _ = SoftwareSecurePhotoVerification.user_status(request.user) - if status in ["must_reverify", "expired"]: + + # If the verification process is still ongoing i.e. the status for photo + # verification is either 'submitted' or 'must_retry' then its marked as + # 'pending' + if status in ["must_reverify", "expired", "pending"]: context = { "user_full_name": request.user.profile.name, "platform_name": settings.PLATFORM_NAME, From 094ed32176ca41cb4c200cf5c4efaaabfae16b3c Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Wed, 4 Nov 2015 18:18:48 +0500 Subject: [PATCH 018/115] Added ability to regenerate certificates from Instructor Dashboard --- lms/djangoapps/certificates/models.py | 26 ++++ .../instructor/tests/test_certificates.py | 78 +++++++++- lms/djangoapps/instructor/views/api.py | 39 ++++- lms/djangoapps/instructor/views/api_urls.py | 4 + .../instructor/views/instructor_dashboard.py | 7 +- lms/djangoapps/instructor_task/api.py | 24 +++ .../instructor_task/tasks_helper.py | 37 +++-- .../instructor_task/tests/test_api.py | 17 ++ .../tests/test_tasks_helper.py | 146 ++++++++++++++++++ .../js/instructor_dashboard/certificates.js | 50 +++++- .../instructor_dashboard/certificates_spec.js | 116 ++++++++++++++ lms/static/js/spec/main.js | 5 + .../sass/course/instructor/_instructor_2.scss | 20 ++- .../instructor_dashboard_2/certificates.html | 19 +++ 14 files changed, 567 insertions(+), 21 deletions(-) create mode 100644 lms/static/js/spec/instructor_dashboard/certificates_spec.js diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 5e34632c81..a9b516b4a3 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -54,6 +54,7 @@ import os from django.contrib.auth.models import User from django.core.exceptions import ValidationError from django.db import models, transaction +from django.db.models import Count from django.db.models.signals import post_save from django.dispatch import receiver from django.conf import settings @@ -187,6 +188,31 @@ class GeneratedCertificate(models.Model): return None + @classmethod + def get_unique_statuses(cls, course_key=None, flat=False): + """ + 1 - Return unique statuses as a list of dictionaries containing the following key value pairs + [ + {'status': 'status value from db', 'count': 'occurrence count of the status'}, + {...}, + ..., ] + + 2 - if flat is 'True' then return unique statuses as a list + 3 - if course_key is given then return unique statuses associated with the given course + + :param course_key: Course Key identifier + :param flat: boolean showing whether to return statuses as a list of values or a list of dictionaries. + """ + query = cls.objects + + if course_key: + query = query.filter(course_id=course_key) + + if flat: + return query.values_list('status', flat=True).distinct() + else: + return query.values('status').annotate(count=Count('status')) + @receiver(post_save, sender=GeneratedCertificate) def handle_post_cert_generated(sender, instance, **kwargs): # pylint: disable=no-self-argument, unused-argument diff --git a/lms/djangoapps/instructor/tests/test_certificates.py b/lms/djangoapps/instructor/tests/test_certificates.py index d54773c6c9..a10efa03f0 100644 --- a/lms/djangoapps/instructor/tests/test_certificates.py +++ b/lms/djangoapps/instructor/tests/test_certificates.py @@ -12,7 +12,8 @@ from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from config_models.models import cache from courseware.tests.factories import GlobalStaffFactory, InstructorFactory, UserFactory -from certificates.models import CertificateGenerationConfiguration +from certificates.tests.factories import GeneratedCertificateFactory +from certificates.models import CertificateGenerationConfiguration, CertificateStatuses from certificates import api as certs_api @@ -486,3 +487,78 @@ class CertificatesInstructorApiTest(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) res_json = json.loads(response.content) self.assertTrue(res_json['success']) + + def test_certificate_regeneration_success(self): + """ + Test certificate regeneration is successful when accessed with 'certificate_statuses' + present in GeneratedCertificate table. + """ + + # Create a generated Certificate of some user with status 'downloadable' + GeneratedCertificateFactory.create( + user=self.user, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='honor' + ) + + # Login the client and access the url with 'certificate_statuses' + self.client.login(username=self.global_staff.username, password='test') + url = reverse('start_certificate_regeneration', kwargs={'course_id': unicode(self.course.id)}) + response = self.client.post(url, data={'certificate_statuses': [CertificateStatuses.downloadable]}) + + # Assert 200 status code in response + self.assertEqual(response.status_code, 200) + res_json = json.loads(response.content) + + # Assert request is successful + self.assertTrue(res_json['success']) + + # Assert success message + self.assertEqual( + res_json['message'], + u'Certificate regeneration task has been started. You can view the status of the generation task in ' + u'the "Pending Tasks" section.' + ) + + def test_certificate_regeneration_error(self): + """ + Test certificate regeneration errors out when accessed with either empty list of 'certificate_statuses' or + the 'certificate_statuses' that are not present in GeneratedCertificate table. + """ + # Create a dummy course and GeneratedCertificate with the same status as the one we will use to access + # 'start_certificate_regeneration' but their error message should be displayed as GeneratedCertificate + # belongs to a different course + dummy_course = CourseFactory.create() + GeneratedCertificateFactory.create( + user=self.user, + course_id=dummy_course.id, + status=CertificateStatuses.generating, + mode='honor' + ) + + # Login the client and access the url without 'certificate_statuses' + self.client.login(username=self.global_staff.username, password='test') + url = reverse('start_certificate_regeneration', kwargs={'course_id': unicode(self.course.id)}) + response = self.client.post(url) + + # Assert 400 status code in response + self.assertEqual(response.status_code, 400) + res_json = json.loads(response.content) + + # Assert Error Message + self.assertEqual( + res_json['message'], + u'Please select one or more certificate statuses that require certificate regeneration.' + ) + + # Access the url passing 'certificate_statuses' that are not present in db + url = reverse('start_certificate_regeneration', kwargs={'course_id': unicode(self.course.id)}) + response = self.client.post(url, data={'certificate_statuses': [CertificateStatuses.generating]}) + + # Assert 400 status code in response + self.assertEqual(response.status_code, 400) + res_json = json.loads(response.content) + + # Assert Error Message + self.assertEqual(res_json['message'], u'Please select certificate statuses from the list only.') diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d626a45ed0..f9d2aee23f 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -92,7 +92,7 @@ from instructor.views import INVOICE_KEY from submissions import api as sub_api # installed from the edx-submissions repository from certificates import api as certs_api -from certificates.models import CertificateWhitelist +from certificates.models import CertificateWhitelist, GeneratedCertificate from bulk_email.models import CourseEmail from student.models import get_user_by_username_or_email @@ -2708,6 +2708,43 @@ def start_certificate_generation(request, course_id): return JsonResponse(response_payload) +@ensure_csrf_cookie +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_global_staff +@require_POST +def start_certificate_regeneration(request, course_id): + """ + Start regenerating certificates for students whose certificate statuses lie with in 'certificate_statuses' + entry in POST data. + """ + course_key = CourseKey.from_string(course_id) + certificates_statuses = request.POST.getlist('certificate_statuses', []) + if not certificates_statuses: + return JsonResponse( + {'message': _('Please select one or more certificate statuses that require certificate regeneration.')}, + status=400 + ) + + # Check if the selected statuses are allowed + allowed_statuses = GeneratedCertificate.get_unique_statuses(course_key=course_key, flat=True) + if not set(certificates_statuses).issubset(allowed_statuses): + return JsonResponse( + {'message': _('Please select certificate statuses from the list only.')}, + status=400 + ) + try: + instructor_task.api.regenerate_certificates(request, course_key, certificates_statuses) + except AlreadyRunningError as error: + return JsonResponse({'message': error.message}, status=400) + + response_payload = { + 'message': _('Certificate regeneration task has been started. ' + 'You can view the status of the generation task in the "Pending Tasks" section.'), + 'success': True + } + return JsonResponse(response_payload) + + @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_global_staff diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index be99ef4600..7565c4e671 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -143,6 +143,10 @@ urlpatterns = patterns( 'instructor.views.api.start_certificate_generation', name='start_certificate_generation'), + url(r'^start_certificate_regeneration', + 'instructor.views.api.start_certificate_regeneration', + name='start_certificate_regeneration'), + url(r'^create_certificate_exception/(?P[^/]*)', 'instructor.views.api.create_certificate_exception', name='create_certificate_exception'), diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index c94dc09820..8034d3d0cf 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -37,7 +37,7 @@ from student.models import CourseEnrollment from shoppingcart.models import Coupon, PaidCourseRegistration, CourseRegCodeItem from course_modes.models import CourseMode, CourseModesArchive from student.roles import CourseFinanceAdminRole, CourseSalesAdminRole -from certificates.models import CertificateGenerationConfiguration, CertificateWhitelist +from certificates.models import CertificateGenerationConfiguration, CertificateWhitelist, GeneratedCertificate from certificates import api as certs_api from util.date_utils import get_default_time_display @@ -299,6 +299,7 @@ def _section_certificates(course): 'enabled_for_course': certs_api.cert_generation_enabled(course.id), 'instructor_generation_enabled': instructor_generation_enabled, 'html_cert_enabled': html_cert_enabled, + 'certificate_statuses': GeneratedCertificate.get_unique_statuses(course_key=course.id), 'urls': { 'generate_example_certificates': reverse( 'generate_example_certificates', @@ -312,6 +313,10 @@ def _section_certificates(course): 'start_certificate_generation', kwargs={'course_id': course.id} ), + 'start_certificate_regeneration': reverse( + 'start_certificate_regeneration', + kwargs={'course_id': course.id} + ), 'list_instructor_tasks_url': reverse( 'list_instructor_tasks', kwargs={'course_id': course.id} diff --git a/lms/djangoapps/instructor_task/api.py b/lms/djangoapps/instructor_task/api.py index 602a8ab64a..703c54b4c1 100644 --- a/lms/djangoapps/instructor_task/api.py +++ b/lms/djangoapps/instructor_task/api.py @@ -512,3 +512,27 @@ def generate_certificates_for_students(request, course_key, students=None): # p task_key = "" return submit_task(request, task_type, task_class, course_key, task_input, task_key) + + +def regenerate_certificates(request, course_key, statuses_to_regenerate, students=None): + """ + Submits a task to regenerate certificates for given students enrolled in the course or + all students if argument 'students' is None. + Regenerate Certificate only if the status of the existing generated certificate is in 'statuses_to_regenerate' + list passed in the arguments. + + Raises AlreadyRunningError if certificates are currently being generated. + """ + if students: + task_type = 'regenerate_certificates_certain_student' + students = [student.id for student in students] + task_input = {'students': students} + else: + task_type = 'regenerate_certificates_all_student' + task_input = {} + + task_input.update({"statuses_to_regenerate": statuses_to_regenerate}) + task_class = generate_certificates + task_key = "" + + return submit_task(request, task_type, task_class, course_key, task_input, task_key) diff --git a/lms/djangoapps/instructor_task/tasks_helper.py b/lms/djangoapps/instructor_task/tasks_helper.py index c823abd22e..19e9ef4586 100644 --- a/lms/djangoapps/instructor_task/tasks_helper.py +++ b/lms/djangoapps/instructor_task/tasks_helper.py @@ -1414,7 +1414,8 @@ def generate_students_certificates( current_step = {'step': 'Calculating students already have certificates'} task_progress.update_task_state(extra_meta=current_step) - students_require_certs = students_require_certificate(course_id, enrolled_students) + statuses_to_regenerate = task_input.get('statuses_to_regenerate', []) + students_require_certs = students_require_certificate(course_id, enrolled_students, statuses_to_regenerate) task_progress.skipped = task_progress.total - len(students_require_certs) @@ -1523,15 +1524,31 @@ def cohort_students_and_upload(_xmodule_instance_args, _entry_id, course_id, tas return task_progress.update_task_state(extra_meta=current_step) -def students_require_certificate(course_id, enrolled_students): - """ Returns list of students where certificates needs to be generated. - Removing those students who have their certificate already generated - from total enrolled students for given course. +def students_require_certificate(course_id, enrolled_students, statuses_to_regenerate=None): + """ + Returns list of students where certificates needs to be generated. + if 'statuses_to_regenerate' is given then return students that have Generated Certificates + and the generated certificate status lies in 'statuses_to_regenerate' + + if 'statuses_to_regenerate' is not given then return all the enrolled student skipping the ones + whose certificates have already been generated. + :param course_id: :param enrolled_students: + :param statuses_to_regenerate: """ - # compute those students where certificates already generated - students_already_have_certs = User.objects.filter( - ~Q(generatedcertificate__status=CertificateStatuses.unavailable), - generatedcertificate__course_id=course_id) - return list(set(enrolled_students) - set(students_already_have_certs)) + if statuses_to_regenerate: + # Return Students that have Generated Certificates and the generated certificate status + # lies in 'statuses_to_regenerate' + return User.objects.filter( + generatedcertificate__course_id=course_id, + generatedcertificate__status__in=statuses_to_regenerate + ) + else: + # compute those students whose certificates are already generated + students_already_have_certs = User.objects.filter( + ~Q(generatedcertificate__status=CertificateStatuses.unavailable), + generatedcertificate__course_id=course_id) + + # Return all the enrolled student skipping the ones whose certificates have already been generated + return list(set(enrolled_students) - set(students_already_have_certs)) diff --git a/lms/djangoapps/instructor_task/tests/test_api.py b/lms/djangoapps/instructor_task/tests/test_api.py index 6ac8a7bb94..712b30cc0b 100644 --- a/lms/djangoapps/instructor_task/tests/test_api.py +++ b/lms/djangoapps/instructor_task/tests/test_api.py @@ -22,6 +22,7 @@ from instructor_task.api import ( submit_executive_summary_report, submit_course_survey_report, generate_certificates_for_all_students, + regenerate_certificates ) from instructor_task.api_helper import AlreadyRunningError @@ -31,6 +32,7 @@ from instructor_task.tests.test_base import (InstructorTaskTestCase, InstructorTaskModuleTestCase, TestReportMixin, TEST_COURSE_KEY) +from certificates.models import CertificateStatuses class InstructorTaskReportTest(InstructorTaskTestCase): @@ -263,3 +265,18 @@ class InstructorTaskCourseSubmitTest(TestReportMixin, InstructorTaskCourseTestCa self.course.id ) self._test_resubmission(api_call) + + def test_regenerate_certificates(self): + """ + Tests certificates regeneration task submission api + """ + def api_call(): + """ + wrapper method for regenerate_certificates + """ + return regenerate_certificates( + self.create_task_request(self.instructor), + self.course.id, + [CertificateStatuses.downloadable, CertificateStatuses.generating] + ) + self._test_resubmission(api_call) diff --git a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py index 9cc34e4827..51a81887e5 100644 --- a/lms/djangoapps/instructor_task/tests/test_tasks_helper.py +++ b/lms/djangoapps/instructor_task/tests/test_tasks_helper.py @@ -1635,3 +1635,149 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase): }, result ) + + def test_certificate_regeneration_for_students(self): + """ + Verify that certificates are regenerated for all eligible students enrolled in a course whose generated + certificate statuses lies in the list 'statuses_to_regenerate' given in task_input. + """ + # create 10 students + students = [self.create_student(username='student_{}'.format(i), email='student_{}@example.com'.format(i)) + for i in xrange(1, 11)] + + # mark 2 students to have certificates generated already + for student in students[:2]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='honor' + ) + + # mark 3 students to have certificates generated with status 'error' + for student in students[2:5]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.error, + mode='honor' + ) + + # mark 6th students to have certificates generated with status 'deleted' + for student in students[5:6]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.deleted, + mode='honor' + ) + + # white-list 7 students + for student in students[:7]: + CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True) + + current_task = Mock() + current_task.update_state = Mock() + + # Certificates should be regenerated for students having generated certificates with status + # 'downloadable' or 'error' which are total of 5 students in this test case + task_input = {'statuses_to_regenerate': [CertificateStatuses.downloadable, CertificateStatuses.error]} + + with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task: + mock_current_task.return_value = current_task + with patch('capa.xqueue_interface.XQueueInterface.send_to_queue') as mock_queue: + mock_queue.return_value = (0, "Successfully queued") + result = generate_students_certificates( + None, None, self.course.id, task_input, 'certificates generated' + ) + + self.assertDictContainsSubset( + { + 'action_name': 'certificates generated', + 'total': 10, + 'attempted': 5, + 'succeeded': 5, + 'failed': 0, + 'skipped': 5 + }, + result + ) + + def test_certificate_regeneration_with_expected_failures(self): + """ + Verify that certificates are regenerated for all eligible students enrolled in a course whose generated + certificate statuses lies in the list 'statuses_to_regenerate' given in task_input. + """ + # create 10 students + students = [self.create_student(username='student_{}'.format(i), email='student_{}@example.com'.format(i)) + for i in xrange(1, 11)] + + # mark 2 students to have certificates generated already + for student in students[:2]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.downloadable, + mode='honor' + ) + + # mark 3 students to have certificates generated with status 'error' + for student in students[2:5]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.error, + mode='honor' + ) + + # mark 6th students to have certificates generated with status 'deleted' + for student in students[5:6]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.deleted, + mode='honor' + ) + + # mark rest of the 4 students with having generated certificates with status 'generating' + # These students are not added in white-list and they have not completed grades so certificate generation + # for these students should fail other than the one student that has been added to white-list + # so from these students 3 failures and 1 success + for student in students[6:]: + GeneratedCertificateFactory.create( + user=student, + course_id=self.course.id, + status=CertificateStatuses.generating, + mode='honor' + ) + + # white-list 7 students + for student in students[:7]: + CertificateWhitelistFactory.create(user=student, course_id=self.course.id, whitelist=True) + + current_task = Mock() + current_task.update_state = Mock() + + # Regenerated certificates for students having generated certificates with status + # 'deleted' or 'generating' + task_input = {'statuses_to_regenerate': [CertificateStatuses.deleted, CertificateStatuses.generating]} + + with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task: + mock_current_task.return_value = current_task + with patch('capa.xqueue_interface.XQueueInterface.send_to_queue') as mock_queue: + mock_queue.return_value = (0, "Successfully queued") + result = generate_students_certificates( + None, None, self.course.id, task_input, 'certificates generated' + ) + + self.assertDictContainsSubset( + { + 'action_name': 'certificates generated', + 'total': 10, + 'attempted': 5, + 'succeeded': 2, + 'failed': 3, + 'skipped': 5 + }, + result + ) diff --git a/lms/static/js/instructor_dashboard/certificates.js b/lms/static/js/instructor_dashboard/certificates.js index a8712b697b..df5404f158 100644 --- a/lms/static/js/instructor_dashboard/certificates.js +++ b/lms/static/js/instructor_dashboard/certificates.js @@ -1,4 +1,5 @@ var edx = edx || {}; +var onCertificatesReady = null; (function($, gettext, _) { 'use strict'; @@ -6,7 +7,7 @@ var edx = edx || {}; edx.instructor_dashboard = edx.instructor_dashboard || {}; edx.instructor_dashboard.certificates = {}; - $(function() { + onCertificatesReady = function() { /** * Show a confirmation message before letting staff members * enable/disable self-generated certificates for a course. @@ -59,7 +60,52 @@ var edx = edx || {}; } }); }); - }); + + /** + * Start regenerating certificates for students. + */ + $section.on('click', '#btn-start-regenerating-certificates', function(event) { + if ( !confirm( gettext('Start regenerating certificates for students in this course?') ) ) { + event.preventDefault(); + return; + } + + var $btn_regenerating_certs = $(this), + $certificate_regeneration_status = $('.certificate-regeneration-status'), + url = $btn_regenerating_certs.data('endpoint'); + + $.ajax({ + type: "POST", + data: $("#certificate-regenerating-form").serializeArray(), + url: url, + success: function (data) { + $btn_regenerating_certs.attr('disabled','disabled'); + if(data.success){ + $certificate_regeneration_status.text(data.message). + removeClass('msg-error').addClass('msg-success'); + } + else{ + $certificate_regeneration_status.text(data.message). + removeClass('msg-success').addClass("msg-error"); + } + }, + error: function(jqXHR) { + try{ + var response = JSON.parse(jqXHR.responseText); + $certificate_regeneration_status.text(gettext(response.message)). + removeClass('msg-success').addClass("msg-error"); + }catch(error){ + $certificate_regeneration_status. + text(gettext('Error while regenerating certificates. Please try again.')). + removeClass('msg-success').addClass("msg-error"); + } + } + }); + }); + }; + + // Call onCertificatesReady on document.ready event + $(onCertificatesReady); var Certificates = (function() { function Certificates($section) { diff --git a/lms/static/js/spec/instructor_dashboard/certificates_spec.js b/lms/static/js/spec/instructor_dashboard/certificates_spec.js new file mode 100644 index 0000000000..9dd500fa1e --- /dev/null +++ b/lms/static/js/spec/instructor_dashboard/certificates_spec.js @@ -0,0 +1,116 @@ +/*global define, onCertificatesReady */ +define([ + 'jquery', + 'common/js/spec_helpers/ajax_helpers', + 'js/instructor_dashboard/certificates' + ], + function($, AjaxHelpers) { + 'use strict'; + describe("edx.instructor_dashboard.certificates.regenerate_certificates", function() { + var $regenerate_certificates_button = null, + $certificate_regeneration_status = null, + requests = null; + var MESSAGES = { + success_message: 'Certificate regeneration task has been started. ' + + 'You can view the status of the generation task in the "Pending Tasks" section.', + error_message: 'Please select one or more certificate statuses that require certificate regeneration.', + server_error_message: "Error while regenerating certificates. Please try again." + }; + var expected = { + error_class: 'msg-error', + success_class: 'msg-success', + url: 'test/url/', + postData : [], + selected_statuses: ['downloadable', 'error'], + body: 'certificate_statuses=downloadable&certificate_statuses=error' + }; + + var select_options = function(option_values){ + $.each(option_values, function(index, element){ + $("#certificate-statuses option[value=" + element + "]").attr('selected', 'selected'); + }); + }; + + beforeEach(function() { + var fixture = '

Regenerate Certificates

' + + '
' + + '

Select one or more certificate statuses ' + + ' below using your mouse and ctrl or command key.

' + + ' ' + + ' ' + + ' ' + + '
' + + '
'; + + setFixtures(fixture); + onCertificatesReady(); + $regenerate_certificates_button = $("#btn-start-regenerating-certificates"); + $certificate_regeneration_status = $(".certificate-regeneration-status"); + requests = AjaxHelpers.requests(this); + }); + + it("does not regenerate certificates if user cancels operation in confirm popup", function() { + spyOn(window, 'confirm').andReturn(false); + $regenerate_certificates_button.click(); + expect(window.confirm).toHaveBeenCalled(); + AjaxHelpers.expectNoRequests(requests); + }); + + it("sends regenerate certificates request if user accepts operation in confirm popup", function() { + spyOn(window, 'confirm').andReturn(true); + $regenerate_certificates_button.click(); + expect(window.confirm).toHaveBeenCalled(); + AjaxHelpers.expectRequest(requests, 'POST', expected.url); + }); + + it("sends regenerate certificates request with selected certificate statuses", function() { + spyOn(window, 'confirm').andReturn(true); + + select_options(expected.selected_statuses); + + $regenerate_certificates_button.click(); + AjaxHelpers.expectRequest(requests, 'POST', expected.url, expected.body); + }); + + it("displays error message in case of server side error", function() { + spyOn(window, 'confirm').andReturn(true); + select_options(expected.selected_statuses); + + $regenerate_certificates_button.click(); + AjaxHelpers.respondWithError(requests, 500, {message: MESSAGES.server_error_message}); + expect($certificate_regeneration_status).toHaveClass(expected.error_class); + expect($certificate_regeneration_status.text()).toEqual(MESSAGES.server_error_message); + }); + + it("displays error message returned by the server in case of unsuccessful request", function() { + spyOn(window, 'confirm').andReturn(true); + select_options(expected.selected_statuses); + + $regenerate_certificates_button.click(); + AjaxHelpers.respondWithError(requests, 400, {message: MESSAGES.error_message}); + expect($certificate_regeneration_status).toHaveClass(expected.error_class); + expect($certificate_regeneration_status.text()).toEqual(MESSAGES.error_message); + }); + + it("displays success message returned by the server in case of successful request", function() { + spyOn(window, 'confirm').andReturn(true); + select_options(expected.selected_statuses); + + $regenerate_certificates_button.click(); + AjaxHelpers.respondWithJson(requests, {message: MESSAGES.success_message, success: true}); + expect($certificate_regeneration_status).toHaveClass(expected.success_class); + expect($certificate_regeneration_status.text()).toEqual(MESSAGES.success_message); + }); + + }); + } +); diff --git a/lms/static/js/spec/main.js b/lms/static/js/spec/main.js index 06456ecd0c..bf5ee49a10 100644 --- a/lms/static/js/spec/main.js +++ b/lms/static/js/spec/main.js @@ -293,6 +293,10 @@ exports: 'coffee/src/instructor_dashboard/student_admin', deps: ['jquery', 'underscore', 'coffee/src/instructor_dashboard/util', 'string_utils'] }, + 'js/instructor_dashboard/certificates': { + exports: 'js/instructor_dashboard/certificates', + deps: ['jquery', 'gettext', 'underscore'] + }, // LMS class loaded explicitly until they are converted to use RequireJS 'js/student_account/account': { exports: 'js/student_account/account', @@ -644,6 +648,7 @@ 'lms/include/js/spec/instructor_dashboard/ecommerce_spec.js', 'lms/include/js/spec/instructor_dashboard/student_admin_spec.js', 'lms/include/js/spec/instructor_dashboard/certificates_exception_spec.js', + 'lms/include/js/spec/instructor_dashboard/certificates_spec.js', 'lms/include/js/spec/student_account/account_spec.js', 'lms/include/js/spec/student_account/access_spec.js', 'lms/include/js/spec/student_account/logistration_factory_spec.js', diff --git a/lms/static/sass/course/instructor/_instructor_2.scss b/lms/static/sass/course/instructor/_instructor_2.scss index f2cd5f61e2..744696f8c4 100644 --- a/lms/static/sass/course/instructor/_instructor_2.scss +++ b/lms/static/sass/course/instructor/_instructor_2.scss @@ -92,6 +92,20 @@ } } + .msg-success{ + border-top: 2px solid $confirm-color; + background: tint($confirm-color,95%); + color: $confirm-color; + } + + .multi-select { + min-width: 150px; + + option { + padding: ($baseline/5) $baseline ($baseline/10) ($baseline/4); + } + } + // inline copy .copy-confirm { color: $confirm-color; @@ -2125,12 +2139,6 @@ input[name="subject"] { } #certificate-white-list-editor{ - .msg-success{ - border-top: 2px solid $confirm-color; - background: tint($confirm-color,95%); - color: $confirm-color; - } - .certificate-exception-inputs{ .student-username-or-email{ width: 300px; diff --git a/lms/templates/instructor/instructor_dashboard_2/certificates.html b/lms/templates/instructor/instructor_dashboard_2/certificates.html index 897c5588a3..43f039308d 100644 --- a/lms/templates/instructor/instructor_dashboard_2/certificates.html +++ b/lms/templates/instructor/instructor_dashboard_2/certificates.html @@ -92,6 +92,25 @@ import json %endif % endif +
+

+

${_("Regenerate Certificates")}

+
+

${_('Select one or more certificate statuses below using your mouse and ctrl or command key.')}

+ + + + +
+
+ +

${_("Certificate Exceptions")}

From 44b409ebffc7f55e4479d1cd95d8651620d292a6 Mon Sep 17 00:00:00 2001 From: Amir Qayyum Khan Date: Thu, 1 Oct 2015 19:39:04 +0500 Subject: [PATCH 019/115] Added pagination on grade book. --- lms/djangoapps/ccx/tests/test_views.py | 65 ++++++---- lms/djangoapps/ccx/urls.py | 5 + lms/djangoapps/ccx/views.py | 21 +-- .../tests/views/test_instructor_dashboard.py | 39 ++++++ lms/djangoapps/instructor/views/api.py | 41 ------ lms/djangoapps/instructor/views/api_urls.py | 5 +- .../instructor/views/gradebook_api.py | 120 ++++++++++++++++++ lms/static/sass/course/_gradebook.scss | 10 ++ lms/templates/courseware/gradebook.html | 16 +++ 9 files changed, 240 insertions(+), 82 deletions(-) create mode 100644 lms/djangoapps/instructor/views/gradebook_api.py diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 1839cfd200..6dcda9e291 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -80,6 +80,40 @@ def ccx_dummy_request(): return request +def setup_students_and_grades(context): + """ + Create students and set their grades. + :param context: class reference + """ + if context.course: + context.student = student = UserFactory.create() + CourseEnrollmentFactory.create(user=student, course_id=context.course.id) + + context.student2 = student2 = UserFactory.create() + CourseEnrollmentFactory.create(user=student2, course_id=context.course.id) + + # create grades for self.student as if they'd submitted the ccx + for chapter in context.course.get_children(): + for i, section in enumerate(chapter.get_children()): + for j, problem in enumerate(section.get_children()): + # if not problem.visible_to_staff_only: + StudentModuleFactory.create( + grade=1 if i < j else 0, + max_grade=1, + student=context.student, + course_id=context.course.id, + module_state_key=problem.location + ) + + StudentModuleFactory.create( + grade=1 if i > j else 0, + max_grade=1, + student=context.student2, + course_id=context.course.id, + module_state_key=problem.location + ) + + @attr('shard_1') @ddt.ddt class TestCoachDashboard(SharedModuleStoreTestCase, LoginEnrollmentTestCase): @@ -696,28 +730,12 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): # which emulates how a student would get access. self.ccx_key = CCXLocator.from_course_locator(self._course.id, ccx.id) self.course = get_course_by_id(self.ccx_key, depth=None) - - self.student = student = UserFactory.create() - CourseEnrollmentFactory.create(user=student, course_id=self.course.id) - - # create grades for self.student as if they'd submitted the ccx - for chapter in self.course.get_children(): - for i, section in enumerate(chapter.get_children()): - for j, problem in enumerate(section.get_children()): - # if not problem.visible_to_staff_only: - StudentModuleFactory.create( - grade=1 if i < j else 0, - max_grade=1, - student=self.student, - course_id=self.course.id, - module_state_key=problem.location - ) - + setup_students_and_grades(self) self.client.login(username=coach.username, password="test") - self.addCleanup(RequestCache.clear_request_cache) @patch('ccx.views.render_to_response', intercept_renderer) + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 1) def test_gradebook(self): self.course.enable_ccx = True RequestCache.clear_request_cache() @@ -728,6 +746,8 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): ) response = self.client.get(url) self.assertEqual(response.status_code, 200) + # Max number of student per page is one. Patched setting MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 1 + self.assertEqual(len(response.mako_context['students']), 1) # pylint: disable=no-member student_info = response.mako_context['students'][0] # pylint: disable=no-member self.assertEqual(student_info['grade_summary']['percent'], 0.5) self.assertEqual( @@ -751,12 +771,11 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): response['content-disposition'], 'attachment' ) + rows = response.content.strip().split('\r') + headers = rows[0] - headers, row = ( - row.strip().split(',') for row in - response.content.strip().split('\n') - ) - data = dict(zip(headers, row)) + # picking first student records + data = dict(zip(headers.strip().split(','), rows[1].strip().split(','))) self.assertNotIn('HW 04', data) self.assertEqual(data['HW 01'], '0.75') self.assertEqual(data['HW 02'], '0.5') diff --git a/lms/djangoapps/ccx/urls.py b/lms/djangoapps/ccx/urls.py index 9a2be83e7e..f670749087 100644 --- a/lms/djangoapps/ccx/urls.py +++ b/lms/djangoapps/ccx/urls.py @@ -18,8 +18,13 @@ urlpatterns = patterns( 'ccx.views.ccx_schedule', name='ccx_schedule'), url(r'^ccx_manage_student$', 'ccx.views.ccx_student_management', name='ccx_manage_student'), + + # Grade book url(r'^ccx_gradebook$', 'ccx.views.ccx_gradebook', name='ccx_gradebook'), + url(r'^ccx_gradebook/(?P[0-9]+)$', + 'ccx.views.ccx_gradebook', name='ccx_gradebook'), + url(r'^ccx_grades.csv$', 'ccx.views.ccx_grades_csv', name='ccx_grades_csv'), url(r'^ccx_set_grading_policy$', diff --git a/lms/djangoapps/ccx/views.py b/lms/djangoapps/ccx/views.py index cabb554b63..e88fa0b623 100644 --- a/lms/djangoapps/ccx/views.py +++ b/lms/djangoapps/ccx/views.py @@ -39,8 +39,8 @@ from ccx_keys.locator import CCXLocator from student.roles import CourseCcxCoachRole from student.models import CourseEnrollment -from instructor.offline_gradecalc import student_grades from instructor.views.api import _split_input_list +from instructor.views.gradebook_api import get_grade_book_page from instructor.views.tools import get_student_from_identifier from instructor.enrollment import ( enroll_email, @@ -551,24 +551,11 @@ def ccx_gradebook(request, course, ccx=None): ccx_key = CCXLocator.from_course_locator(course.id, ccx.id) with ccx_course(ccx_key) as course: prep_course_for_grading(course, request) - - enrolled_students = User.objects.filter( - courseenrollment__course_id=ccx_key, - courseenrollment__is_active=1 - ).order_by('username').select_related("profile") - - student_info = [ - { - 'username': student.username, - 'id': student.id, - 'email': student.email, - 'grade_summary': student_grades(student, request, course), - 'realname': student.profile.name, - } - for student in enrolled_students - ] + student_info, page = get_grade_book_page(request, course, course_key=ccx_key) return render_to_response('courseware/gradebook.html', { + 'page': page, + 'page_url': reverse('ccx_gradebook', kwargs={'course_id': ccx_key}), 'students': student_info, 'course': course, 'course_id': course.id, diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index 4fed6a075e..37ea9c4e7a 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -8,10 +8,13 @@ from django.conf import settings from django.core.urlresolvers import reverse from django.test.client import RequestFactory from django.test.utils import override_settings +from edxmako.shortcuts import render_to_response +from ccx.tests.test_views import setup_students_and_grades from courseware.tabs import get_course_tab_list from courseware.tests.factories import UserFactory from courseware.tests.helpers import LoginEnrollmentTestCase +from instructor.views.gradebook_api import calculate_page_info from common.test.utils import XssTestMixin from student.tests.factories import AdminFactory @@ -23,6 +26,20 @@ from student.roles import CourseFinanceAdminRole from student.models import CourseEnrollment +def intercept_renderer(path, context): + """ + Intercept calls to `render_to_response` and attach the context dict to the + response for examination in unit tests. + """ + # I think Django already does this for you in their TestClient, except + # we're bypassing that by using edxmako. Probably edxmako should be + # integrated better with Django's rendering and event system. + response = render_to_response(path, context) + response.mako_context = context + response.mako_template = path + return response + + @ddt.ddt class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssTestMixin): """ @@ -252,3 +269,25 @@ class TestInstructorDashboard(ModuleStoreTestCase, LoginEnrollmentTestCase, XssT """ response = self.client.get(self.url) self.assertIn('D: 0.5, C: 0.57, B: 0.63, A: 0.75', response.content) + + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 2) + def test_calculate_page_info(self): + page = calculate_page_info(offset=0, total_students=2) + self.assertEqual(page["offset"], 0) + self.assertEqual(page["page_num"], 1) + self.assertEqual(page["next_offset"], None) + self.assertEqual(page["previous_offset"], None) + self.assertEqual(page["total_pages"], 1) + + @patch('instructor.views.gradebook_api.render_to_response', intercept_renderer) + @patch('instructor.views.gradebook_api.MAX_STUDENTS_PER_PAGE_GRADE_BOOK', 1) + def test_spoc_gradebook_pages(self): + setup_students_and_grades(self) + url = reverse( + 'spoc_gradebook', + kwargs={'course_id': self.course.id} + ) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + # Max number of student per page is one. Patched setting MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 1 + self.assertEqual(len(response.mako_context['students']), 1) # pylint: disable=no-member diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index d626a45ed0..f394e4166a 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -81,7 +81,6 @@ from instructor.enrollment import ( unenroll_email, ) from instructor.access import list_with_level, allow_access, revoke_access, ROLES, update_forum_role -from instructor.offline_gradecalc import student_grades import instructor_analytics.basic import instructor_analytics.distributions import instructor_analytics.csvs @@ -2625,46 +2624,6 @@ def enable_certificate_generation(request, course_id=None): return redirect(_instructor_dash_url(course_key, section='certificates')) -#---- Gradebook (shown to small courses only) ---- -@cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('staff') -def spoc_gradebook(request, course_id): - """ - Show the gradebook for this course: - - Only shown for courses with enrollment < settings.FEATURES.get("MAX_ENROLLMENT_INSTR_BUTTONS") - - Only displayed to course staff - """ - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id) - course = get_course_with_access(request.user, 'staff', course_key, depth=None) - - enrolled_students = User.objects.filter( - courseenrollment__course_id=course_key, - courseenrollment__is_active=1 - ).order_by('username').select_related("profile") - - # possible extension: implement pagination to show to large courses - - student_info = [ - { - 'username': student.username, - 'id': student.id, - 'email': student.email, - 'grade_summary': student_grades(student, request, course), - 'realname': student.profile.name, - } - for student in enrolled_students - ] - - return render_to_response('courseware/gradebook.html', { - 'students': student_info, - 'course': course, - 'course_id': course_key, - # Checked above - 'staff_access': True, - 'ordered_grades': sorted(course.grade_cutoffs.items(), key=lambda i: i[1], reverse=True), - }) - - @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) @require_level('staff') diff --git a/lms/djangoapps/instructor/views/api_urls.py b/lms/djangoapps/instructor/views/api_urls.py index be99ef4600..700e99639c 100644 --- a/lms/djangoapps/instructor/views/api_urls.py +++ b/lms/djangoapps/instructor/views/api_urls.py @@ -124,7 +124,10 @@ urlpatterns = patterns( # spoc gradebook url(r'^gradebook$', - 'instructor.views.api.spoc_gradebook', name='spoc_gradebook'), + 'instructor.views.gradebook_api.spoc_gradebook', name='spoc_gradebook'), + + url(r'^gradebook/(?P[0-9]+)$', + 'instructor.views.gradebook_api.spoc_gradebook', name='spoc_gradebook'), # Cohort management url(r'add_users_to_cohorts$', diff --git a/lms/djangoapps/instructor/views/gradebook_api.py b/lms/djangoapps/instructor/views/gradebook_api.py new file mode 100644 index 0000000000..2d9e5fd0a2 --- /dev/null +++ b/lms/djangoapps/instructor/views/gradebook_api.py @@ -0,0 +1,120 @@ +""" +Grade book view for instructor and pagination work (for grade book) +which is currently use by ccx and instructor apps. +""" +import math + +from django.contrib.auth.models import User +from django.core.urlresolvers import reverse +from django.views.decorators.cache import cache_control + +from opaque_keys.edx.keys import CourseKey + +from edxmako.shortcuts import render_to_response +from courseware.courses import get_course_with_access +from instructor.offline_gradecalc import student_grades +from instructor.views.api import require_level + + +# Grade book: max students per page +MAX_STUDENTS_PER_PAGE_GRADE_BOOK = 20 + + +def calculate_page_info(offset, total_students): + """ + Takes care of sanitizing the offset of current page also calculates offsets for next and previous page + and information like total number of pages and current page number. + + :param offset: offset for database query + :return: tuple consist of page number, query offset for next and previous pages and valid offset + """ + + # validate offset. + if not (isinstance(offset, int) or offset.isdigit()) or int(offset) < 0 or int(offset) >= total_students: + offset = 0 + else: + offset = int(offset) + + # calculate offsets for next and previous pages. + next_offset = offset + MAX_STUDENTS_PER_PAGE_GRADE_BOOK + previous_offset = offset - MAX_STUDENTS_PER_PAGE_GRADE_BOOK + + # calculate current page number. + page_num = ((offset / MAX_STUDENTS_PER_PAGE_GRADE_BOOK) + 1) + + # calculate total number of pages. + total_pages = int(math.ceil(float(total_students) / MAX_STUDENTS_PER_PAGE_GRADE_BOOK)) or 1 + + if previous_offset < 0 or offset == 0: + # We are at first page, so there's no previous page. + previous_offset = None + + if next_offset >= total_students: + # We've reached the last page, so there's no next page. + next_offset = None + + return { + "previous_offset": previous_offset, + "next_offset": next_offset, + "page_num": page_num, + "offset": offset, + "total_pages": total_pages + } + + +def get_grade_book_page(request, course, course_key): + """ + Get student records per page along with page information i.e current page, total pages and + offset information. + """ + # Unsanitized offset + current_offset = request.GET.get('offset', 0) + enrolled_students = User.objects.filter( + courseenrollment__course_id=course_key, + courseenrollment__is_active=1 + ).order_by('username').select_related("profile") + + total_students = enrolled_students.count() + page = calculate_page_info(current_offset, total_students) + offset = page["offset"] + total_pages = page["total_pages"] + + if total_pages > 1: + # Apply limit on queryset only if total number of students are greater then MAX_STUDENTS_PER_PAGE_GRADE_BOOK. + enrolled_students = enrolled_students[offset: offset + MAX_STUDENTS_PER_PAGE_GRADE_BOOK] + + student_info = [ + { + 'username': student.username, + 'id': student.id, + 'email': student.email, + 'grade_summary': student_grades(student, request, course), + 'realname': student.profile.name, + } + for student in enrolled_students + ] + return student_info, page + + +@cache_control(no_cache=True, no_store=True, must_revalidate=True) +@require_level('staff') +def spoc_gradebook(request, course_id): + """ + Show the gradebook for this course: + - Only shown for courses with enrollment < settings.FEATURES.get("MAX_ENROLLMENT_INSTR_BUTTONS") + - Only displayed to course staff + """ + course_key = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'staff', course_key, depth=None) + student_info, page = get_grade_book_page(request, course, course_key) + + return render_to_response('courseware/gradebook.html', { + 'page': page, + 'page_url': reverse('spoc_gradebook', kwargs={'course_id': unicode(course_key)}), + 'students': student_info, + 'course': course, + 'course_id': course_key, + # Checked above + 'staff_access': True, + 'ordered_grades': sorted(course.grade_cutoffs.items(), key=lambda i: i[1], reverse=True), + }) diff --git a/lms/static/sass/course/_gradebook.scss b/lms/static/sass/course/_gradebook.scss index 28bb5a87b4..1085195087 100644 --- a/lms/static/sass/course/_gradebook.scss +++ b/lms/static/sass/course/_gradebook.scss @@ -80,6 +80,16 @@ div.gradebook-wrapper { } } + .grade-book-footer { + position: relative; + top: 15px; + width: 100%; + border: 0; + box-shadow: 0; + text-align: center; + display: inline-block; + } + .grades { position: relative; float: left; diff --git a/lms/templates/courseware/gradebook.html b/lms/templates/courseware/gradebook.html index 2ec8dee0b4..ebc6bac37e 100644 --- a/lms/templates/courseware/gradebook.html +++ b/lms/templates/courseware/gradebook.html @@ -118,7 +118,23 @@ from django.core.urlresolvers import reverse
+ + %if page["previous_offset"] is not None: + + ${_('previous page')} + + %endif + ${_('Page')} ${page["page_num"]} ${_('of')} ${page["total_pages"]} + + %if page["next_offset"] is not None: + + ${_('next page')} + + %endif + %endif From cbc57aae6b3de70017e0bf4a2a0f26678ff61a36 Mon Sep 17 00:00:00 2001 From: Zia Fazal Date: Thu, 29 Oct 2015 13:43:39 +0500 Subject: [PATCH 020/115] enabled web certs by default changing default value instead of setting it via fields fixed broken test fixed quality violation following another approach --- .../tests/test_course_create_rerun.py | 23 +++++++++++++++++++ cms/djangoapps/contentstore/views/course.py | 7 ++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py index a7c5df8bc7..4db95d3bed 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -1,11 +1,15 @@ """ Test view handler for rerun (and eventually create) """ +import ddt + from django.test.client import RequestFactory from opaque_keys.edx.keys import CourseKey from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore from student.roles import CourseInstructorRole, CourseStaffRole from student.tests.factories import UserFactory from contentstore.tests.utils import AjaxEnabledTestClient, parse_json @@ -13,6 +17,7 @@ from datetime import datetime from xmodule.course_module import CourseFields +@ddt.ddt class TestCourseListing(ModuleStoreTestCase): """ Unit tests for getting the list of courses for a logged in user @@ -64,3 +69,21 @@ class TestCourseListing(ModuleStoreTestCase): self.assertEqual(dest_course_key.run, 'copy') dest_course = self.store.get_course(dest_course_key) self.assertEqual(dest_course.start, CourseFields.start.default) + + @ddt.data(ModuleStoreEnum.Type.mongo, ModuleStoreEnum.Type.split) + def test_newly_created_course_has_web_certs_enabled(self, store): + """ + Tests newly created course has web certs enabled by default. + """ + with modulestore().default_store(store): + response = self.client.ajax_post('/course/', { + 'org': 'orgX', + 'number': 'CS101', + 'display_name': 'Course with web certs enabled', + 'run': '2015_T2' + }) + self.assertEqual(response.status_code, 200) + data = parse_json(response) + new_course_key = CourseKey.from_string(data['course_key']) + course = self.store.get_course(new_course_key) + self.assertTrue(course.cert_html_view_enabled) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4ff5151e16..4e8fc5a027 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -741,8 +741,11 @@ def create_new_course_in_store(store, user, org, number, run, fields): Separated out b/c command line course creation uses this as well as the web interface. """ - # Set default language from settings - fields.update({'language': getattr(settings, 'DEFAULT_COURSE_LANGUAGE', 'en')}) + # Set default language from settings and enable web certs + fields.update({ + 'language': getattr(settings, 'DEFAULT_COURSE_LANGUAGE', 'en'), + 'cert_html_view_enabled': True, + }) with modulestore().default_store(store): # Creating the course raises DuplicateCourseError if an existing course with this org/name is found From fc2594e37be5b66c49890903263f7d3ae529cb4b Mon Sep 17 00:00:00 2001 From: Matjaz Gregoric Date: Wed, 11 Nov 2015 09:02:22 +0100 Subject: [PATCH 021/115] Remove stray quotation mark from courseware-error text. --- lms/templates/courseware/courseware-error.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/templates/courseware/courseware-error.html b/lms/templates/courseware/courseware-error.html index 8829ba3811..47400365e9 100644 --- a/lms/templates/courseware/courseware-error.html +++ b/lms/templates/courseware/courseware-error.html @@ -20,7 +20,7 @@

${_("We're sorry, this module is temporarily unavailable. Our staff is working to fix " - "it as soon as possible. Please email us at {tech_support_email}' to report any problems or downtime.").format( + "it as soon as possible. Please email us at {tech_support_email} to report any problems or downtime.").format( tech_support_email=u'{0}'.format(settings.TECH_SUPPORT_EMAIL) )}

From a62e403ed847753f14262dd0b32710c2514d390b Mon Sep 17 00:00:00 2001 From: Davorin Sego Date: Mon, 21 Sep 2015 10:06:47 +0200 Subject: [PATCH 022/115] Search optimization Remove filtering by partition, course and user Rewrite the LMS result processor to use the course blocks api --- .../acceptance/pages/lms/courseware_search.py | 2 +- .../test_lms_cohorted_courseware_search.py | 21 +- .../test_lms_split_test_courseware_search.py | 81 ++--- .../courseware_search/lms_filter_generator.py | 67 ---- .../courseware_search/lms_result_processor.py | 30 +- .../test/test_lms_filter_generator.py | 287 +----------------- .../test/test_lms_search_initializer.py | 151 --------- 7 files changed, 54 insertions(+), 585 deletions(-) delete mode 100644 lms/lib/courseware_search/test/test_lms_search_initializer.py diff --git a/common/test/acceptance/pages/lms/courseware_search.py b/common/test/acceptance/pages/lms/courseware_search.py index 8f4a57515a..640f17935c 100644 --- a/common/test/acceptance/pages/lms/courseware_search.py +++ b/common/test/acceptance/pages/lms/courseware_search.py @@ -29,7 +29,7 @@ class CoursewareSearchPage(CoursePage): def search(self): """ execute the search """ self.q(css=self.search_bar_selector + ' [type="submit"]').click() - self.wait_for_element_visibility('.search-info', 'Search results are shown') + self.wait_for_ajax() def search_for_term(self, text): """ diff --git a/common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py b/common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py index e0739b4385..76560d9d11 100644 --- a/common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py +++ b/common/test/acceptance/tests/lms/test_lms_cohorted_courseware_search.py @@ -30,8 +30,6 @@ class CoursewareSearchCohortTest(ContainerBase): """ Test courseware search. """ - USERNAME = 'STUDENT_TESTER' - EMAIL = 'student101@example.com' TEST_INDEX_FILENAME = "test_root/index_file.dat" @@ -71,6 +69,14 @@ class CoursewareSearchCohortTest(ContainerBase): self.browser, username=self.cohort_b_student_username, email=self.cohort_b_student_email, no_login=True ).visit() + # Create a student who will end up in the default cohort group + self.cohort_default_student_username = "cohort_default_student" + self.cohort_default_student_email = "cohort_default_student@example.com" + StudioAutoAuthPage( + self.browser, username=self.cohort_default_student_username, + email=self.cohort_default_student_email, no_login=True + ).visit() + self.courseware_search_page = CoursewareSearchPage(self.browser, self.course_id) # Enable Cohorting and assign cohorts and content groups @@ -183,6 +189,7 @@ class CoursewareSearchCohortTest(ContainerBase): set_visibility(1, self.content_group_a) set_visibility(2, self.content_group_b) set_visibility(3, self.content_group_a, self.content_group_b) + set_visibility(4, 'All Students and Staff') # Does not work without this container_page.publish_action.click() @@ -213,7 +220,7 @@ class CoursewareSearchCohortTest(ContainerBase): """ Make sure that the page is accessible. """ - self._auto_auth(self.USERNAME, self.EMAIL, False) + self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False) self.courseware_search_page.visit() def test_cohorted_search_user_a_a_content(self): @@ -234,20 +241,20 @@ class CoursewareSearchCohortTest(ContainerBase): self.courseware_search_page.search_for_term(self.group_a_html) assert self.group_a_html not in self.courseware_search_page.search_results.html[0] - def test_cohorted_search_user_c_ab_content(self): + def test_cohorted_search_user_default_ab_content(self): """ Test user not enrolled in any cohorts can't see any of restricted content. """ - self._auto_auth(self.USERNAME, self.EMAIL, False) + self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False) self.courseware_search_page.visit() self.courseware_search_page.search_for_term(self.group_a_and_b_html) assert self.group_a_and_b_html not in self.courseware_search_page.search_results.html[0] - def test_cohorted_search_user_c_all_content(self): + def test_cohorted_search_user_default_all_content(self): """ Test user can search public content if cohorts used on course. """ - self._auto_auth(self.USERNAME, self.EMAIL, False) + self._auto_auth(self.cohort_default_student_username, self.cohort_default_student_email, False) self.courseware_search_page.visit() self.courseware_search_page.search_for_term(self.visible_to_all_html) assert self.visible_to_all_html in self.courseware_search_page.search_results.html[0] diff --git a/common/test/acceptance/tests/lms/test_lms_split_test_courseware_search.py b/common/test/acceptance/tests/lms/test_lms_split_test_courseware_search.py index 094152d16a..e3ba27cce1 100644 --- a/common/test/acceptance/tests/lms/test_lms_split_test_courseware_search.py +++ b/common/test/acceptance/tests/lms/test_lms_split_test_courseware_search.py @@ -52,7 +52,7 @@ class SplitTestCoursewareSearchTest(ContainerBase): self.course_info['run'] ) - self._add_and_configure_split_test() + self._create_group_configuration() self._studio_reindex() def _auto_auth(self, username, email, staff): @@ -72,41 +72,24 @@ class SplitTestCoursewareSearchTest(ContainerBase): self.course_outline.start_reindex() self.course_outline.wait_for_ajax() - def _add_and_configure_split_test(self): + def _create_group_configuration(self): """ - Add a split test and a configuration to a test course fixture + Create a group configuration for course """ - # Create a new group configurations - # pylint: disable=W0212 + # pylint: disable=protected-access self.course_fixture._update_xblock(self.course_fixture._course_location, { "metadata": { u"user_partitions": [ create_user_partition_json( 0, - "Name", - "Description.", + "Configuration A/B", + "Content Group Partition.", [Group("0", "Group A"), Group("1", "Group B")] - ), - create_user_partition_json( - 456, - "Name 2", - "Description 2.", - [Group("2", "Group C"), Group("3", "Group D")] - ), - ], - }, + ) + ] + } }) - # Add a split test module to the 'Test Unit' vertical in the course tree - split_test_1 = XBlockFixtureDesc('split_test', 'Test Content Experiment 1', metadata={'user_partition_id': 0}) - split_test_1_parent_vertical = self.course_fixture.get_nested_xblocks(category="vertical")[1] - self.course_fixture.create_xblock(split_test_1_parent_vertical.locator, split_test_1) - - # Add a split test module to the 'Test 2 Unit' vertical in the course tree - split_test_2 = XBlockFixtureDesc('split_test', 'Test Content Experiment 2', metadata={'user_partition_id': 456}) - split_test_2_parent_vertical = self.course_fixture.get_nested_xblocks(category="vertical")[2] - self.course_fixture.create_xblock(split_test_2_parent_vertical.locator, split_test_2) - def populate_course_fixture(self, course_fixture): """ Populate the children of the test course fixture. @@ -116,28 +99,26 @@ class SplitTestCoursewareSearchTest(ContainerBase): }) course_fixture.add_children( - XBlockFixtureDesc('chapter', 'Content Section').add_children( - XBlockFixtureDesc('sequential', 'Content Subsection').add_children( - XBlockFixtureDesc('vertical', 'Content Unit').add_children( - XBlockFixtureDesc('html', 'VISIBLETOALLCONTENT', data='VISIBLETOALLCONTENT') - ) - ) - ), XBlockFixtureDesc('chapter', 'Test Section').add_children( XBlockFixtureDesc('sequential', 'Test Subsection').add_children( - XBlockFixtureDesc('vertical', 'Test Unit') + XBlockFixtureDesc('vertical', 'Test Unit').add_children( + XBlockFixtureDesc( + 'html', + 'VISIBLE TO A', + data='VISIBLE TO A', + metadata={"group_access": {0: [0]}} + ), + XBlockFixtureDesc( + 'html', + 'VISIBLE TO B', + data='VISIBLE TO B', + metadata={"group_access": {0: [1]}} + ) + ) ) - ), - XBlockFixtureDesc('chapter', 'X Section').add_children( - XBlockFixtureDesc('sequential', 'X Subsection').add_children( - XBlockFixtureDesc('vertical', 'X Unit') - ) - ), + ) ) - self.test_1_breadcrumb = "Test Section \xe2\x96\xb8 Test Subsection \xe2\x96\xb8 Test Unit".decode("utf-8") - self.test_2_breadcrumb = "X Section \xe2\x96\xb8 X Subsection \xe2\x96\xb8 X Unit".decode("utf-8") - def test_page_existence(self): """ Make sure that the page is accessible. @@ -145,15 +126,6 @@ class SplitTestCoursewareSearchTest(ContainerBase): self._auto_auth(self.USERNAME, self.EMAIL, False) self.courseware_search_page.visit() - def test_search_for_experiment_content_user_not_assigned(self): - """ - Test user can't search for experiment content if not assigned to a group. - """ - self._auto_auth(self.USERNAME, self.EMAIL, False) - self.courseware_search_page.visit() - self.courseware_search_page.search_for_term("Group") - assert "Sorry, no results were found." in self.courseware_search_page.search_results.html[0] - def test_search_for_experiment_content_user_assigned_to_one_group(self): """ Test user can search for experiment content restricted to his group @@ -161,8 +133,5 @@ class SplitTestCoursewareSearchTest(ContainerBase): """ self._auto_auth(self.USERNAME, self.EMAIL, False) self.courseware_search_page.visit() - self.course_navigation_page.go_to_section("Test Section", "Test Subsection") - self.courseware_search_page.search_for_term("Group") + self.courseware_search_page.search_for_term("VISIBLE TO") assert "1 result" in self.courseware_search_page.search_results.html[0] - assert self.test_1_breadcrumb in self.courseware_search_page.search_results.html[0] - assert self.test_2_breadcrumb not in self.courseware_search_page.search_results.html[0] diff --git a/lms/lib/courseware_search/lms_filter_generator.py b/lms/lib/courseware_search/lms_filter_generator.py index 8bc40ecced..bee00302a1 100644 --- a/lms/lib/courseware_search/lms_filter_generator.py +++ b/lms/lib/courseware_search/lms_filter_generator.py @@ -5,15 +5,9 @@ This file contains implementation override of SearchFilterGenerator which will a from microsite_configuration import microsite from student.models import CourseEnrollment -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey -from opaque_keys.edx.locations import SlashSeparatedCourseKey -from xmodule.modulestore.django import modulestore - from search.filter_generator import SearchFilterGenerator from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme -from courseware.access import get_user_role INCLUDE_SCHEMES = [CohortPartitionScheme, RandomUserPartitionScheme, ] @@ -31,67 +25,6 @@ class LmsSearchFilterGenerator(SearchFilterGenerator): self._user_enrollments[user] = CourseEnrollment.enrollments_for_user(user) return self._user_enrollments[user] - def filter_dictionary(self, **kwargs): - """ LMS implementation, adds filtering by user partition, course id and user """ - - def get_group_for_user_partition(user_partition, course_key, user): - """ Returns the specified user's group for user partition """ - if user_partition.scheme in SCHEME_SUPPORTS_ASSIGNMENT: - return user_partition.scheme.get_group_for_user( - course_key, - user, - user_partition, - assign=False, - ) - else: - return user_partition.scheme.get_group_for_user( - course_key, - user, - user_partition, - ) - - def get_group_ids_for_user(course, user): - """ Collect user partition group ids for user for this course """ - partition_groups = [] - for user_partition in course.user_partitions: - if user_partition.scheme in INCLUDE_SCHEMES: - group = get_group_for_user_partition(user_partition, course.id, user) - if group: - partition_groups.append(group) - partition_group_ids = [unicode(partition_group.id) for partition_group in partition_groups] - return partition_group_ids if partition_group_ids else None - - filter_dictionary = super(LmsSearchFilterGenerator, self).filter_dictionary(**kwargs) - if 'user' in kwargs: - user = kwargs['user'] - - if 'course_id' in kwargs and kwargs['course_id']: - try: - course_key = CourseKey.from_string(kwargs['course_id']) - except InvalidKeyError: - course_key = SlashSeparatedCourseKey.from_deprecated_string(kwargs['course_id']) - - # Staff user looking at course as staff user - if get_user_role(user, course_key) in ('instructor', 'staff'): - return filter_dictionary - # Need to check course exist (if course gets deleted enrollments don't get cleaned up) - course = modulestore().get_course(course_key) - if course: - filter_dictionary['content_groups'] = get_group_ids_for_user(course, user) - else: - user_enrollments = self._enrollments_for_user(user) - content_groups = [] - for enrollment in user_enrollments: - course = modulestore().get_course(enrollment.course_id) - if course: - enrollment_group_ids = get_group_ids_for_user(course, user) - if enrollment_group_ids: - content_groups.extend(enrollment_group_ids) - - filter_dictionary['content_groups'] = content_groups if content_groups else None - - return filter_dictionary - def field_dictionary(self, **kwargs): """ add course if provided otherwise add courses in which the user is enrolled in """ field_dictionary = super(LmsSearchFilterGenerator, self).field_dictionary(**kwargs) diff --git a/lms/lib/courseware_search/lms_result_processor.py b/lms/lib/courseware_search/lms_result_processor.py index 5102038178..73d6df73da 100644 --- a/lms/lib/courseware_search/lms_result_processor.py +++ b/lms/lib/courseware_search/lms_result_processor.py @@ -8,18 +8,16 @@ from django.core.urlresolvers import reverse from opaque_keys.edx.locations import SlashSeparatedCourseKey from search.result_processor import SearchResultProcessor from xmodule.modulestore.django import modulestore - -from courseware.access import has_access +from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.courseware.access import has_access class LmsSearchResultProcessor(SearchResultProcessor): - """ SearchResultProcessor for LMS Search """ _course_key = None - _course_name = None _usage_key = None _module_store = None - _module_temp_dictionary = {} + _course_blocks = {} def get_course_key(self): """ fetch course key object from string representation - retain result for subsequent uses """ @@ -39,11 +37,13 @@ class LmsSearchResultProcessor(SearchResultProcessor): self._module_store = modulestore() return self._module_store - def get_item(self, usage_key): - """ fetch item from the modulestore - don't refetch if we've already retrieved it beforehand """ - if usage_key not in self._module_temp_dictionary: - self._module_temp_dictionary[usage_key] = self.get_module_store().get_item(usage_key) - return self._module_temp_dictionary[usage_key] + def get_course_blocks(self, user): + """ fetch cached blocks for course - retain for subsequent use """ + course_key = self.get_course_key() + if course_key not in self._course_blocks: + root_block_usage_key = self.get_module_store().make_course_usage_key(course_key) + self._course_blocks[course_key] = get_course_blocks(user, root_block_usage_key) + return self._course_blocks[course_key] @property def url(self): @@ -60,10 +60,6 @@ class LmsSearchResultProcessor(SearchResultProcessor): def should_remove(self, user): """ Test to see if this result should be removed due to access restriction """ - user_has_access = has_access( - user, - "load", - self.get_item(self.get_usage_key()), - self.get_course_key() - ) - return not user_has_access + if has_access(user, 'staff', self.get_course_key()): + return False + return self.get_usage_key() not in self.get_course_blocks(user).get_block_keys() diff --git a/lms/lib/courseware_search/test/test_lms_filter_generator.py b/lms/lib/courseware_search/test/test_lms_filter_generator.py index 479b1350f7..e56bff22b2 100644 --- a/lms/lib/courseware_search/test/test_lms_filter_generator.py +++ b/lms/lib/courseware_search/test/test_lms_filter_generator.py @@ -3,20 +3,10 @@ Tests for the lms_filter_generator """ from mock import patch, Mock -from xmodule.modulestore.django import modulestore -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from student.tests.factories import UserFactory from student.models import CourseEnrollment - -from xmodule.partitions.partitions import Group, UserPartition -from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme -from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme -from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory, config_course_cohorts -from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort -from openedx.core.djangoapps.course_groups.views import link_cohort_to_partition_group -from opaque_keys import InvalidKeyError -from opaque_keys.edx.keys import CourseKey from lms.lib.courseware_search.lms_filter_generator import LmsSearchFilterGenerator @@ -58,10 +48,6 @@ class LmsSearchFilterGeneratorTestCase(ModuleStoreTestCase): publish_item=True, ) - self.groups = [Group(1, 'Group 1'), Group(2, 'Group 2')] - - self.content_groups = [1, 2] - def setUp(self): super(LmsSearchFilterGeneratorTestCase, self).setUp() self.build_courses() @@ -150,274 +136,3 @@ class LmsSearchFilterGeneratorTestCase(ModuleStoreTestCase): self.assertNotIn('org', exclude_dictionary) self.assertIn('org', field_dictionary) self.assertEqual('TestMicrosite3', field_dictionary['org']) - - -class LmsSearchFilterGeneratorGroupsTestCase(LmsSearchFilterGeneratorTestCase): - """ - Test case class to test search result processor - with content and user groups present within the course - """ - - def setUp(self): - super(LmsSearchFilterGeneratorGroupsTestCase, self).setUp() - self.user_partition = None - self.split_test_user_partition = None - self.first_cohort = None - self.second_cohort = None - - def add_seq_with_content_groups(self, groups=None): - """ - Adds sequential and two content groups to first course in courses list. - """ - config_course_cohorts(self.courses[0], is_cohorted=True) - - if groups is None: - groups = self.groups - - self.user_partition = UserPartition( - id=0, - name='Partition 1', - description='This is partition 1', - groups=groups, - scheme=CohortPartitionScheme - ) - - self.user_partition.scheme.name = "cohort" - - ItemFactory.create( - parent_location=self.chapter.location, - category='sequential', - display_name="Lesson 1", - publish_item=True, - metadata={u"user_partitions": [self.user_partition.to_json()]} - ) - - self.first_cohort, self.second_cohort = [ - CohortFactory(course_id=self.courses[0].id) for _ in range(2) - ] - - self.courses[0].user_partitions = [self.user_partition] - self.courses[0].save() - modulestore().update_item(self.courses[0], self.user.id) - - def add_user_to_cohort_group(self): - """ - adds user to cohort and links cohort to content group - """ - add_user_to_cohort(self.first_cohort, self.user.username) - - link_cohort_to_partition_group( - self.first_cohort, - self.user_partition.id, - self.groups[0].id, - ) - - self.courses[0].save() - modulestore().update_item(self.courses[0], self.user.id) - - def add_split_test(self, groups=None): - """ - Adds split test and two content groups to second course in courses list. - """ - if groups is None: - groups = self.groups - - self.split_test_user_partition = UserPartition( - id=0, - name='Partition 2', - description='This is partition 2', - groups=groups, - scheme=RandomUserPartitionScheme - ) - - self.split_test_user_partition.scheme.name = "random" - - sequential = ItemFactory.create( - parent_location=self.chapter.location, - category='sequential', - display_name="Lesson 2", - publish_item=True, - ) - - vertical = ItemFactory.create( - parent_location=sequential.location, - category='vertical', - display_name='Subsection 3', - publish_item=True, - ) - - split_test_unit = ItemFactory.create( - parent_location=vertical.location, - category='split_test', - user_partition_id=0, - display_name="Test Content Experiment 1", - ) - - condition_1_vertical = ItemFactory.create( - parent_location=split_test_unit.location, - category="vertical", - display_name="Group ID 1", - ) - - condition_2_vertical = ItemFactory.create( - parent_location=split_test_unit.location, - category="vertical", - display_name="Group ID 2", - ) - - ItemFactory.create( - parent_location=condition_1_vertical.location, - category="html", - display_name="Group A", - publish_item=True, - ) - - ItemFactory.create( - parent_location=condition_2_vertical.location, - category="html", - display_name="Group B", - publish_item=True, - ) - - self.courses[1].user_partitions = [self.split_test_user_partition] - self.courses[1].save() - modulestore().update_item(self.courses[1], self.user.id) - - def add_user_to_splittest_group(self): - """ - adds user to a random split test group - """ - self.split_test_user_partition.scheme.get_group_for_user( - CourseKey.from_string(unicode(self.courses[1].id)), - self.user, - self.split_test_user_partition, - assign=True, - ) - - self.courses[1].save() - modulestore().update_item(self.courses[1], self.user.id) - - def test_content_group_id_provided(self): - """ - Tests that we get the content group ID when course is assigned to cohort - which is assigned content group. - """ - self.add_seq_with_content_groups() - self.add_user_to_cohort_group() - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[0].id) - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[0].id), field_dictionary['course']) - self.assertEqual([unicode(self.content_groups[0])], filter_dictionary['content_groups']) - - def test_content_multiple_groups_id_provided(self): - """ - Tests that we get content groups IDs when course is assigned to cohort - which is assigned to multiple content groups. - """ - self.add_seq_with_content_groups() - self.add_user_to_cohort_group() - - # Second cohort link - link_cohort_to_partition_group( - self.second_cohort, - self.user_partition.id, - self.groups[0].id, - ) - - self.courses[0].save() - modulestore().update_item(self.courses[0], self.user.id) - - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[0].id) - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[0].id), field_dictionary['course']) - # returns only first group, relevant to current user - self.assertEqual([unicode(self.content_groups[0])], filter_dictionary['content_groups']) - - def test_content_group_id_not_provided(self): - """ - Tests that we don't get content group ID when course is assigned a cohort - but cohort is not assigned to content group. - """ - self.add_seq_with_content_groups(groups=[]) - - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[0].id) - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[0].id), field_dictionary['course']) - self.assertEqual(None, filter_dictionary['content_groups']) - - def test_content_group_with_cohort_not_provided(self): - """ - Tests that we don't get content group ID when course has no cohorts - """ - self.add_seq_with_content_groups() - - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[0].id) - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[0].id), field_dictionary['course']) - self.assertEqual(None, filter_dictionary['content_groups']) - - def test_split_test_with_user_groups_user_not_assigned(self): - """ - Tests that we don't get user group ID when user is not assigned to a split test group - """ - self.add_split_test() - - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[1].id) - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[1].id), field_dictionary['course']) - self.assertEqual(None, filter_dictionary['content_groups']) - - def test_split_test_with_user_groups_user_assigned(self): - """ - Tests that we get user group ID when user is assigned to a split test group - """ - self.add_split_test() - self.add_user_to_splittest_group() - - field_dictionary, filter_dictionary, _ = LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id=unicode(self.courses[1].id) - ) - - partition_group = self.split_test_user_partition.scheme.get_group_for_user( - CourseKey.from_string(unicode(self.courses[1].id)), - self.user, - self.split_test_user_partition, - assign=False, - ) - - self.assertTrue('start_date' in filter_dictionary) - self.assertEqual(unicode(self.courses[1].id), field_dictionary['course']) - self.assertEqual([unicode(partition_group.id)], filter_dictionary['content_groups']) - - def test_invalid_course_key(self): - """ - Test system raises an error if no course found. - """ - - self.add_seq_with_content_groups() - with self.assertRaises(InvalidKeyError): - LmsSearchFilterGenerator.generate_field_filters( - user=self.user, - course_id='this_is_false_course_id' - ) diff --git a/lms/lib/courseware_search/test/test_lms_search_initializer.py b/lms/lib/courseware_search/test/test_lms_search_initializer.py deleted file mode 100644 index 9cf04c23fa..0000000000 --- a/lms/lib/courseware_search/test/test_lms_search_initializer.py +++ /dev/null @@ -1,151 +0,0 @@ -""" -Tests for the lms_search_initializer -""" - -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.partitions.partitions import Group, UserPartition -from xmodule.modulestore.django import modulestore - -from courseware.tests.factories import UserFactory -from courseware.tests.test_masquerade import StaffMasqueradeTestCase -from courseware.masquerade import handle_ajax - -from lms.lib.courseware_search.lms_search_initializer import LmsSearchInitializer -from lms.lib.courseware_search.lms_filter_generator import LmsSearchFilterGenerator - - -class LmsSearchInitializerTestCase(StaffMasqueradeTestCase): - """ Test case class to test search initializer """ - - def build_course(self): - """ - Build up a course tree with an html control - """ - self.global_staff = UserFactory(is_staff=True) - - self.course = CourseFactory.create( - org='Elasticsearch', - course='ES101', - run='test_run', - display_name='Elasticsearch test course', - ) - self.section = ItemFactory.create( - parent=self.course, - category='chapter', - display_name='Test Section', - ) - self.subsection = ItemFactory.create( - parent=self.section, - category='sequential', - display_name='Test Subsection', - ) - self.vertical = ItemFactory.create( - parent=self.subsection, - category='vertical', - display_name='Test Unit', - ) - self.html = ItemFactory.create( - parent=self.vertical, - category='html', - display_name='Test Html control 1', - ) - self.html = ItemFactory.create( - parent=self.vertical, - category='html', - display_name='Test Html control 2', - ) - - def setUp(self): - super(LmsSearchInitializerTestCase, self).setUp() - self.build_course() - self.user_partition = UserPartition( - id=0, - name='Test User Partition', - description='', - groups=[Group(0, 'Group 1'), Group(1, 'Group 2')], - scheme_id='cohort' - ) - self.course.user_partitions.append(self.user_partition) - modulestore().update_item(self.course, self.global_staff.id) # pylint: disable=no-member - - def test_staff_masquerading_added_to_group(self): - """ - Tests that initializer sets masquerading for a staff user in a group. - """ - # Verify that there is no masquerading group initially - _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters( # pylint: disable=unused-variable - user=self.global_staff, - course_id=unicode(self.course.id) - ) - # User is staff by default, no content groups filter is set - see all - self.assertNotIn('content_groups', filter_directory) - - # Install a masquerading group - request = self._create_mock_json_request( - self.global_staff, - body='{"role": "student", "user_partition_id": 0, "group_id": 1}' - ) - handle_ajax(request, unicode(self.course.id)) - - # Call initializer - LmsSearchInitializer.set_search_enviroment( - request=request, - course_id=unicode(self.course.id) - ) - - # Verify that there is masquerading group after masquerade - _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters( # pylint: disable=unused-variable - user=self.global_staff, - course_id=unicode(self.course.id) - ) - self.assertEqual(filter_directory['content_groups'], [unicode(1)]) - - def test_staff_masquerading_as_a_staff_user(self): - """ - Tests that initializer sets masquerading for a staff user as staff. - """ - - # Install a masquerading group - request = self._create_mock_json_request( - self.global_staff, - body='{"role": "staff"}' - ) - handle_ajax(request, unicode(self.course.id)) - - # Call initializer - LmsSearchInitializer.set_search_enviroment( - request=request, - course_id=unicode(self.course.id) - ) - - # Verify that there is masquerading group after masquerade - _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters( # pylint: disable=unused-variable - user=self.global_staff, - course_id=unicode(self.course.id) - ) - self.assertNotIn('content_groups', filter_directory) - - def test_staff_masquerading_as_a_student_user(self): - """ - Tests that initializer sets masquerading for a staff user as student. - """ - - # Install a masquerading group - request = self._create_mock_json_request( - self.global_staff, - body='{"role": "student"}' - ) - handle_ajax(request, unicode(self.course.id)) - - # Call initializer - LmsSearchInitializer.set_search_enviroment( - request=request, - course_id=unicode(self.course.id) - ) - - # Verify that there is masquerading group after masquerade - _, filter_directory, _ = LmsSearchFilterGenerator.generate_field_filters( # pylint: disable=unused-variable - user=self.global_staff, - course_id=unicode(self.course.id) - ) - self.assertEqual(filter_directory['content_groups'], None) From c068bc95cf02f1309fc8b2bf2a63891c5bacfb58 Mon Sep 17 00:00:00 2001 From: Saleem Latif Date: Wed, 11 Nov 2015 18:03:45 +0500 Subject: [PATCH 023/115] Custom 500 error message should only be seen for Preview --- common/djangoapps/util/views.py | 19 +++++++++++++++---- .../certificates/tests/test_webview_views.py | 6 +++++- lms/djangoapps/certificates/views/webview.py | 5 ++++- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/util/views.py b/common/djangoapps/util/views.py index cfa37c62c9..99083db7fc 100644 --- a/common/djangoapps/util/views.py +++ b/common/djangoapps/util/views.py @@ -56,13 +56,18 @@ def jsonable_server_error(request, template_name='500.html'): return server_error(request, template_name=template_name) -def handle_500(template_path, context=None): +def handle_500(template_path, context=None, test_func=None): """ Decorator for view specific 500 error handling. + Custom handling will be skipped only if test_func is passed and it returns False - Usage:: + Usage: - @handle_500(template_path='certificates/server-error.html', context={'error-info': 'Internal Server Error'}) + @handle_500( + template_path='certificates/server-error.html', + context={'error-info': 'Internal Server Error'}, + test_func=lambda request: request.GET.get('preview', None) + ) def my_view(request): # Any unhandled exception in this view would be handled by the handle_500 decorator # ... @@ -83,9 +88,15 @@ def handle_500(template_path, context=None): if settings.DEBUG: # In debug mode let django process the 500 errors and display debug info for the developer raise - else: + elif test_func is None or test_func(request): + # Display custom 500 page if either + # 1. test_func is None (meaning nothing to test) + # 2. or test_func(request) returns True log.exception("Error in django view.") return render_to_response(template_path, context) + else: + # Do not show custom 500 error when test fails + raise return inner return decorator diff --git a/lms/djangoapps/certificates/tests/test_webview_views.py b/lms/djangoapps/certificates/tests/test_webview_views.py index 18727eb04e..362122e276 100644 --- a/lms/djangoapps/certificates/tests/test_webview_views.py +++ b/lms/djangoapps/certificates/tests/test_webview_views.py @@ -454,9 +454,13 @@ class CertificatesViewsTests(ModuleStoreTestCase, EventTrackingTestCase): user_id=self.user.id, course_id=unicode(self.course.id) ) - response = self.client.get(test_url) + response = self.client.get(test_url + "?preview=honor") self.assertIn("Invalid Certificate Configuration", response.content) + # Verify that Exception is raised when certificate is not in the preview mode + with self.assertRaises(Exception): + self.client.get(test_url) + @override_settings(FEATURES=FEATURES_WITH_CERTS_ENABLED) def test_certificate_evidence_event_emitted(self): self.client.logout() diff --git a/lms/djangoapps/certificates/views/webview.py b/lms/djangoapps/certificates/views/webview.py index 11465db16b..3d4fbb4118 100644 --- a/lms/djangoapps/certificates/views/webview.py +++ b/lms/djangoapps/certificates/views/webview.py @@ -281,7 +281,10 @@ def _update_certificate_context(context, course, user, user_certificate): ) -@handle_500(template_path="certificates/server-error.html") +@handle_500( + template_path="certificates/server-error.html", + test_func=lambda request: request.GET.get('preview', None) +) def render_html_view(request, user_id, course_id): """ This public view generates an HTML representation of the specified student's certificate From 78c6a38ca642efb5fc40db6ec3ee6945157f4f79 Mon Sep 17 00:00:00 2001 From: Chris Rodriguez Date: Tue, 29 Sep 2015 13:54:45 -0400 Subject: [PATCH 024/115] LMS: new UI for video player + AFontGarde iconfonts --- .../xmodule/xmodule/css/video/display.scss | 696 +++--- .../spec/video/video_accessible_menu_spec.js | 4 +- .../js/spec/video/video_caption_spec.js | 184 +- .../js/spec/video/video_context_menu_spec.js | 16 +- .../js/spec/video/video_full_screen_spec.js | 6 - .../video/video_play_pause_control_spec.js | 6 - .../video/video_play_skip_control_spec.js | 2 - .../js/spec/video/video_player_spec.js | 10 - .../spec/video/video_quality_control_spec.js | 4 +- .../js/spec/video/video_skip_control_spec.js | 2 - .../js/spec/video/video_speed_control_spec.js | 45 +- .../spec/video/video_volume_control_spec.js | 44 +- .../js/src/video/04_video_full_screen.js | 28 +- .../js/src/video/05_video_quality_control.js | 32 +- .../js/src/video/07_video_volume_control.js | 76 +- .../js/src/video/08_video_speed_control.js | 50 +- .../js/src/video/09_play_pause_control.js | 34 +- .../js/src/video/09_play_skip_control.js | 21 +- .../xmodule/js/src/video/09_skip_control.js | 14 +- .../xmodule/js/src/video/09_video_caption.js | 1937 +++++++++-------- .../static/images/fontawesome/arrows-alt.svg | 6 + .../static/images/fontawesome/caret-left.svg | 6 + .../static/images/fontawesome/caret-right.svg | 6 + common/static/images/fontawesome/caret-up.svg | 6 + common/static/images/fontawesome/cc.svg | 6 + common/static/images/fontawesome/compress.svg | 6 + common/static/images/fontawesome/list-alt.svg | 6 + common/static/images/fontawesome/pause.svg | 6 + common/static/images/fontawesome/play.svg | 6 + .../static/images/fontawesome/quote-left.svg | 6 + .../images/fontawesome/step-forward.svg | 6 + .../static/images/fontawesome/volume-down.svg | 6 + .../static/images/fontawesome/volume-off.svg | 6 + .../static/images/fontawesome/volume-up.svg | 6 + .../js/vendor/afontgarde/afontgarde.css | 60 + .../static/js/vendor/afontgarde/afontgarde.js | 287 +++ .../static/js/vendor/afontgarde/edx-icons.js | 3 + .../modernizr.fontface-generatedcontent.js | 4 + .../test/acceptance/pages/lms/video/video.py | 29 +- .../tests/video/test_studio_video_module.py | 4 +- .../tests/video/test_video_module.py | 8 +- lms/envs/common.py | 4 + 42 files changed, 2223 insertions(+), 1471 deletions(-) create mode 100755 common/static/images/fontawesome/arrows-alt.svg create mode 100755 common/static/images/fontawesome/caret-left.svg create mode 100755 common/static/images/fontawesome/caret-right.svg create mode 100755 common/static/images/fontawesome/caret-up.svg create mode 100755 common/static/images/fontawesome/cc.svg create mode 100755 common/static/images/fontawesome/compress.svg create mode 100755 common/static/images/fontawesome/list-alt.svg create mode 100755 common/static/images/fontawesome/pause.svg create mode 100755 common/static/images/fontawesome/play.svg create mode 100755 common/static/images/fontawesome/quote-left.svg create mode 100755 common/static/images/fontawesome/step-forward.svg create mode 100755 common/static/images/fontawesome/volume-down.svg create mode 100755 common/static/images/fontawesome/volume-off.svg create mode 100755 common/static/images/fontawesome/volume-up.svg create mode 100644 common/static/js/vendor/afontgarde/afontgarde.css create mode 100644 common/static/js/vendor/afontgarde/afontgarde.js create mode 100644 common/static/js/vendor/afontgarde/edx-icons.js create mode 100644 common/static/js/vendor/afontgarde/modernizr.fontface-generatedcontent.js diff --git a/common/lib/xmodule/xmodule/css/video/display.scss b/common/lib/xmodule/xmodule/css/video/display.scss index 5f531435bf..c54591a7ff 100644 --- a/common/lib/xmodule/xmodule/css/video/display.scss +++ b/common/lib/xmodule/xmodule/css/video/display.scss @@ -1,3 +1,66 @@ +// This is for A Font Garde +// It loads the icon font only when it's available. +// --- +// It is scoped to the video player for now, but we +// will eventually want to move this to the main font +// sheet, globally, so it applies to all use cases. + +// -------- +// Defaults: what displays if the icon font doesn't load. +// -------- + +// the html target is necessary for xblocks and xmodules, but works across the board +html:not('.afontgarde') .icon-fallback-img { + + .fa-play { + background: url('#{$static-path}/images/fontawesome/play.svg') center center no-repeat; + } + + .fa-pause { + background: url('#{$static-path}/images/fontawesome/pause.svg') center center no-repeat; + } + + .fa-step-forward { + background: url('#{$static-path}/images/fontawesome/step-forward.svg') center center no-repeat; + } + + .fa-arrows-alt { + background: url('#{$static-path}/images/fontawesome/arrows-alt.svg') center center no-repeat; + } + + .fa-caret-right { + background: url('#{$static-path}/images/fontawesome/caret-right.svg') center center no-repeat; + } + + .fa-caret-left { + background: url('#{$static-path}/images/fontawesome/caret-left.svg') center center no-repeat; + } + + .fa-caret-up { + background: url('#{$static-path}/images/fontawesome/caret-up.svg') center center no-repeat; + } + + .fa-compress { + background: url('#{$static-path}/images/fontawesome/compress.svg') center center no-repeat; + } + + .fa-quote-left { + background: url('#{$static-path}/images/fontawesome/quote-left.svg') center center no-repeat; + } + + .fa-volume-up { + background: url('#{$static-path}/images/fontawesome/volume-up.svg') center center no-repeat; + } + + .fa-volume-down { + background: url('#{$static-path}/images/fontawesome/volume-down.svg') center center no-repeat; + } + + .fa-volume-off { + background: url('#{$static-path}/images/fontawesome/volume-off.svg') center center no-repeat; + } +} + & { margin-bottom: ($baseline*1.5); } @@ -6,21 +69,25 @@ display: none; } -div.video { +.video { @include clearfix(); - background: #f3f3f3; + background: rgb(240, 243, 245); // UXPL grayscale-cool xx-light; display: block; margin: 0 -12px; padding: 12px; border-radius: 5px; outline: none; - &:focus, &:active, &:hover { + &:focus, + &:active, + &:hover { border: 0; } &.is-initialized { - article.video-wrapper { + + .video-wrapper { + .spinner { display: none; } @@ -29,12 +96,14 @@ div.video { // CASE: video pre-roll state &.is-pre-roll { + .slider { visibility: hidden; } .video-player { position: relative; + &:before { display: block; content: ""; @@ -44,12 +113,12 @@ div.video { } } - div.tc-wrapper { + .tc-wrapper { @include clearfix(); position: relative; } - div.focus_grabber { + .focus_grabber { position: relative; display: inline; width: 0px; @@ -60,7 +129,7 @@ div.video { margin: 0; padding: 0; - .video-download-button{ + .video-download-button { display: inline-block; vertical-align: top; margin: ($baseline*0.75) ($baseline/2) 0 0; @@ -75,16 +144,20 @@ div.video { padding: ($baseline*0.75); color: $lighter-base-font-color; - &:hover, &:focus { + &:hover, + &:focus { background-color: $action-primary-active-bg; color: $very-light-text; } } } + .video-tracks { + > a { border-radius: 3px 0 0 3px; } + > a.external-track { border-radius: 3px; } @@ -116,16 +189,17 @@ div.video { } } - article.video-wrapper { - float: left; - margin-right: flex-gutter(9); + .video-wrapper { + @include float(left); + @include margin-right(flex-gutter(9)); width: flex-grid(6, 9); background-color: black; position: relative; - div.video-player-pre, div.video-player-post { + .video-player-pre, + .video-player-post { height: 50px; - background-color: black; + background-color: rgb(17, 16, 16) // UXPL grayscale black; } .spinner { @@ -173,7 +247,7 @@ div.video { } } - section.video-player { + .video-player { overflow: hidden; min-height: 300px; @@ -185,7 +259,9 @@ div.video { } } - object, iframe, video { + object, + iframe, + video { display: block; border: none; width: 100%; @@ -201,285 +277,272 @@ div.video { } } - section.video-controls { + .video-controls { @include clearfix(); - background: #333; - border: 1px solid $black; - border-top: 0; - color: $gray-l3; position: relative; + border: 0; + background: rgb(40, 44, 46); // UXPL grayscale-cool x-dark + color: rgb(240, 243, 245); // UXPL grayscale-cool xx-light - &:hover, &:focus { - ul, div { + &:hover, + &:focus { + + ul, + div { opacity: 1; } } - %video-button { - @extend %ui-fake-link; - @include transition(none); - display: block; - font-weight: 700; - line-height: 46px; + %video-control { + @extend %t-strong; + @extend %t-title7; + display: inline-block; + vertical-align: middle; margin: 0; - padding: 0 0 0 15px; - overflow: hidden; - text-indent: -9999px; - -webkit-font-smoothing: antialiased; - box-shadow: 1px 0 0 #555, inset 1px 0 0 #555; - color: $white; - border-width: 0 1px; - border-style: solid; - border-color: $black; + border: 0; + border-radius: 0; + padding: ($baseline / 2) ($baseline / 1.5); + background: rgb(40, 44, 46); // UXPL grayscale-cool x-dark + box-shadow: none; + text-shadow: none; + color: rgb(207, 216, 220); // UXPL grayscale-cool light - &:hover, &:focus { - background-color: #444; - color: $white; - text-decoration: none; + &:hover, + &:focus { + background: darken(rgb(40, 44, 46), 7%); // UXPL secondary } &:active, - &:focus { - color: $white; - background-color: #444; - text-decoration: none; + &.is-active, + &.active { + color: rgb(14, 166, 236); // UXPL primary accent } } - div.slider { + .control { + @extend %video-control; + + .icon-fallback-img { + + .icon { + // if the icon font doesn't render, we need to provide dimensions for the svg's + width: 1em; + height: 1em; + + &.icon-hd { + // except when it's text, like with HD + // otherwise it's shifted to the right because "HD" is wider than 1em + width: auto; + } + } + } + } + + .slider { @include clearfix(); - @include transform(scaleY(0.5) translate3d(0, 50%, 0)); - background: #c2c2c2; - border: 1px solid $black; - border-radius: 0; - border-top: 1px solid $black; - box-shadow: inset 0 1px 0 #eee, 0 1px 0 #555; + @include transform-origin(bottom left); + @include transition(height .7s ease-in-out 0s); position: absolute; - z-index: 1; bottom: 100%; left: 0; right: 0; - height: 14px; - margin-left: -1px; - margin-right: -1px; - -webkit-transition: -webkit-transform 0.7s ease-in-out; - -moz-transition: -moz-transform 0.7s ease-in-out; - -ms-transition: -ms-transform 0.7s ease-in-out; - transition: transform 0.7s ease-in-out; + z-index: 1; + height: ($baseline / 4); + margin-left: 0; + border: 0; + border-radius: 0; + background: rgb(79, 89, 93); // UXPL grayscale-cool dark - div.ui-widget-header { - background: #777; - box-shadow: inset 0 1px 0 #999; + .ui-widget-header { + background: rgb(142, 62, 99); // UXPL secondary dark + box-shadow: none; } - div.ui-corner-all.slider-range { - background-color: #1e91d3; + .ui-corner-all.slider-range { opacity: 0.3; + background-color: #1e91d3; } - a.ui-slider-handle { + .ui-slider-handle { @extend %ui-fake-link; - @include transform(scale(.7, 1.3) translate3d(-80%, -15%, 0)); - background: $pink url('#{$static-path}/images/slider-handle.png') center center no-repeat; - background-size: 50%; - border: 1px solid darken($pink, 20%); - border-radius: 50%; - box-shadow: inset 0 1px 0 lighten($pink, 10%); - height: 20px; - margin-left: 0; + @include transform-origin(bottom left); + @include transition(all .7s ease-in-out 0s); top: 0; - -webkit-transition: -webkit-transform 0.7s ease-in-out; - -moz-transition: -moz-transform 0.7s ease-in-out; - -ms-transition: -ms-transform 0.7s ease-in-out; - transition: transform 0.7s ease-in-out; - width: 20px; + height: ($baseline / 4); + width: ($baseline / 4); + margin-left: -($baseline / 8); // center-center causes the control to be beyond the end of the sider + border: 0; + border-radius: ($baseline / 5); + background: rgb(203, 89, 141); // UXPL secondary base + box-shadow: none; - &:focus, &:hover { - background-color: lighten($pink, 10%); + &:focus, + &:hover { + background-color: rgb(219, 139, 175); // UXPL secondary light } } } .vcr { - float: left; + @include float(left); list-style: none; - margin: 0 lh() 0 0; + @include border-right(1px solid rgb(40, 44, 46)); // UXPL grayscale-cool x-dark padding: 0; @media (max-width: 1120px) { - margin-right: lh(0.5); + @include margin-right(lh(0.5)); font-size: em(14); } .video_control { - @extend %video-button; - float: left; - background-image: url('#{$static-path}/images/vcr.png'); - background-position: 15px 15px ; - background-repeat: no-repeat; - border-left: none; - padding: 0 lh(.75); - width: 14px; &:focus { - @extend %ui-depth4; position: relative; - outline: $white dotted thin; - outline-offset: -2px; - } - - &:empty { - height: 46px; - background-position: 15px 15px; - } - - &.play { - background-position: 17px -114px; - } - - &.pause { - background-position: 16px -50px; } &.skip { - background-image: none; - text-indent: 0; - width: initial; white-space: nowrap; } } - div.vidtime { + .vidtime { @extend %t-strong; - float: left; - line-height: 46px; //height of play pause buttons - -webkit-font-smoothing: antialiased; - padding-left: lh(.75); + @extend %t-title7; + @include padding-left(lh(.75)); + display: inline-block; + color: rgb(207, 216, 220); // UXPL grayscale-cool light + -webkit-font-smoothing: antialiased;; + @media (max-width: 1120px) { - padding-left: lh(0.5); + @include padding-left(lh(0.5)); } } } - div.secondary-controls { - float: right; + .secondary-controls { + @include float(right); + @include border-left(1px dotted rgb(79, 89, 93)); // UXPL grayscale-cool x-dark + + .volume, + .add-fullscreen, + .grouped-controls, + .quality-control { + @include border-left(1px dotted rgb(79, 89, 93)); // UXPL grayscale-cool x-dark + } + + .speed-button, + .volume > .control, + .add-fullscreen, + .quality-control, + .toggle-transcript { - a.speed-button, - div.volume > a, - a.add-fullscreen, - a.quality-control, - a.hide-subtitles { - // overflow is used to bypass Firefox CSS :focus outline bug - // http://johndoesdesign.com/blog/2012/css/firefox-and-its-css-focus-outline-bug/ &:focus { - @extend %ui-depth5; position: relative; - outline: $white dotted thin; - outline-offset: -2px; - overflow: auto; } } .menu-container { - float: left; position: relative; - &.is-opened { - .menu { - display: block; - opacity: 1; - padding: 0; - margin: 0; - list-style: none; - } - } - .menu { @include transition(none); @extend %ui-depth1; - box-shadow: inset 1px 0 0 #555, 0 1px 0 #444; - background-color: #444; - border: 1px solid $black; - bottom: 46px; - display: none; - opacity: 0; position: absolute; + display: none; + bottom: ($baseline * 2); + @include right(0); // right-align menus since this whole collection is on the right + width: 120px; + margin: 0; + border: none; + padding: 0; + box-shadow: none; + background-color: rgb(40, 44, 46); // UXPL grayscale-cool x-dark + list-style: none; li { @extend %ui-fake-link; - box-shadow: 0 1px 0 #555; - border-bottom: 1px solid $black; - color: $white; + color: rgb(231, 236, 238); // UXPL grayscale-cool x-light - a { - border: 0; - color: $white; + .speed-option, + .control-lang { + @include text-align(left); display: block; + width: 100%; + border: 0; + border-radius: 0; padding: lh(0.5); + background: rgb(40, 44, 46); // UXPL grayscale-cool x-dark + box-shadow: none; + color: rgb(231, 236, 238); // UXPL grayscale-cool x-light overflow: hidden; + text-shadow: none; text-overflow: ellipsis; white-space: nowrap; - &:hover, &:focus { - background-color: #666; - color: #aaa; - outline-offset: -4px; + &:hover, + &:focus { + background-color: rgb(79, 89, 93); // UXPL grayscale-cool dark + color: rgb(252, 252, 252); // UXPL grayscale white } } - &.is-active{ - a { - font-weight: bold; + &.is-active { + + .speed-option, + .control-lang { + color: rgb(14, 166, 236); // UXPL primary accent } } + } + } - &:last-child { - box-shadow: none; - border-bottom: 0; - margin-top: 0; - } + &.is-opened { + + .menu { + display: block; } } } - div.speeds { - &.is-opened { - .speed-button { - background-image: url('#{$static-path}/images/open-arrow.png'); + .speeds, + .lang, + .grouped-controls { + display: inline-block; + + .control { + + .icon-fallback-img { + @include float(left); + @include transform-origin(center center); } } + } - .menu{ - width: 131px; + .speeds { - @media (max-width: 1120px) { - width: 80px; + &.is-opened { + + .control { + + .icon { + + @include ltr { + @include transform(rotate(-90deg)); + } + + @include rtl { + @include transform(rotate(90deg)); + } + } } } .speed-button { - @extend %video-button; - @include clearfix(); - background-image: url('#{$static-path}/images/closed-arrow.png'); - background-position: 10px center; - background-repeat: no-repeat; - min-width: 116px; - text-indent: 0; - - @media (max-width: 1120px) { - min-width: 0; - width: 60px; - } .label { - float: left; - font-size: em(14); - font-weight: normal; - letter-spacing: 1px; - padding: 0 lh(0.25) 0 lh(0.5); - line-height: 46px; - text-transform: uppercase; - color: #999; + @include padding(0 ($baseline/3) 0 0); + font-family: $body-font-family; + color: rgb(231, 236, 238); // UXPL grayscale-cool x-light @media (max-width: 1120px) { display: none; @@ -487,117 +550,115 @@ div.video { } .value { - float: left; + @include padding(0, lh(0.5), 0, 0); + color: rgb(231, 236, 238); // UXPL grayscale-cool x-light font-weight: bold; - margin-bottom: 0; - padding: 0 lh(0.5) 0 0; @media (max-width: 1120px) { - padding: 0 lh(0.5) 0 lh(0.5); + padding: 0 lh(0.5); } - - line-height: 46px; - color: $white; } } } - div.volume { - float: left; + .lang { + + .language-menu { + width: $baseline; + padding: ($baseline / 2) 0; + } + + .control { + + .icon { + + @include rtl { + @include transform(rotate(-180deg)); + } + } + } + + &.is-opened { + + .control { + + .icon { + + @include ltr { + @include transform(rotate(90deg)); + } + + @include rtl { + @include transform(rotate(90deg)); + } + } + } + } + } + + .volume { + display: inline-block; position: relative; &.is-opened { + .volume-slider-container { display: block; opacity: 1; } } - &.is-muted { - & > a { - background-image: url('#{$static-path}/images/mute.png'); - } - } - - & > a { - @extend %video-button; - @include clearfix(); - background-image: url('#{$static-path}/images/volume.png'); - background-position: 10px center; - background-repeat: no-repeat; - width: 30px; - height: 46px; - } - &:not(:first-child) > a { - border-left: none; + @include border-left(none); } .volume-slider-container { @include transition(none); @extend %ui-depth1; - box-shadow: inset 1px 0 0 #555, 0 3px 0 #444; - background-color: #444; - border: 1px solid $black; - bottom: 46px; display: none; - opacity: 0; position: absolute; - width: 45px; - height: 125px; - margin-left: -1px; + bottom: ($baseline * 2); + @include right(0); + width: 41px; + height: 120px; + background-color: rgb(40, 44, 46); // UXPL grayscale-cool x-dark .volume-slider { height: 100px; - border: 0; - width: 5px; + width: ($baseline / 4); margin: 14px auto; - background: #666; - border: 1px solid $black; - box-shadow: 0 1px 0 #333; + border: 0; + background: rgb(79, 89, 93); // UXPL grayscale-cool dark - a.ui-slider-handle { + .ui-slider-handle { @extend %ui-fake-link; @include transition(height $tmg-s2 ease-in-out 0s, width $tmg-s2 ease-in-out 0s); - background: $pink url('#{$static-path}/images/slider-handle.png') center center no-repeat; - background-size: 50%; - border: 1px solid darken($pink, 20%); - border-radius: 15px; - box-shadow: inset 0 1px 0 lighten($pink, 10%); + @include left(-5px); height: 15px; - left: -6px; width: 15px; + background: rgb(203, 89, 141); // UXPL secondary base + border: 0; + border-radius: ($baseline / 5); + + &:hover, + &:focus { + background: rgb(219, 139, 175); // UXPL secondary light + } } .ui-slider-range { - background: #ddd; + background: rgb(142, 62, 99); // UXPL secondary dark } } } } - a.add-fullscreen { - @extend %video-button; - background: url('#{$static-path}/images/fullscreen.png') center no-repeat; - border-left: none; - float: left; - padding: 0 11px; - width: 30px; - } - - - a.quality-control { - @extend %video-button; - background: url('#{$static-path}/images/hd.png') center no-repeat; - border-left: none; - float: left; - padding: 0 11px; - width: 30px; + .quality-control { + font-weight: 700; + letter-spacing: -1px; &.active { - background-color: #F44; - color: #0ff; - text-decoration: none; + color: rgb(14, 166, 236); // UXPL primary accent } &.is-hidden { @@ -605,62 +666,55 @@ div.video { } } - div.lang { - & > a.hide-subtitles { - @extend %video-button; - @include transition(none); - box-shadow: inset 1px 0 0 #555; - background: url('#{$static-path}/images/cc.png') center no-repeat; - border-left: none; - border-right: none; - padding: 0 11px; - width: 30px; + .toggle-transcript { - &.off { - opacity: 0.7; + &.is-active { + color: rgb(14, 166, 236); // UXPL primary accent } - } + } - .menu.langs-list { - right: -1px; - width: 150px; + .lang { + + & > .hide-subtitles { + @include transition(none); } } } } - &:hover section.video-controls { - ul, div { - opacity: 1; - } + &:hover { - div.slider { - @include transform(scaleY(1) translate3d(0, 0, 0)); + .video-controls { - a.ui-slider-handle { - @include transform(scale(1) translate3d(-50%, -15%, 0)); + .slider { + height: ($baseline / 1.5); + + .ui-slider-handle { + height: ($baseline / 1.5); + width: ($baseline / 1.5); + } } } } } - ol.subtitles { - padding-left: 0; - float: left; - max-height: 460px; + .subtitles { + @include float(left); overflow: auto; - width: flex-grid(3, 9); margin: 0; + max-height: 460px; + width: flex-grid(3, 9); + padding: 0; font-size: 14px; list-style: none; visibility: visible; li { @extend %ui-fake-link; - border: 0; - color: rgb(29,157,217); margin-bottom: 8px; + border: 0; padding: 0; + color: #0074b5; // AA compliant line-height: lh(); &.current { @@ -673,7 +727,8 @@ div.video { outline-offset: -1px; } - &:hover, &:focus { + &:hover, + &:focus { text-decoration: underline; } @@ -685,13 +740,12 @@ div.video { &.closed { - article.video-wrapper { + .video-wrapper { width: flex-grid(9,9); - background-color: inherit; } - article.video-wrapper section.video-controls.html5 { + .video-wrapper .video-controls.html5 { bottom: 0; left: 0; right: 0; @@ -699,21 +753,22 @@ div.video { z-index: 1; } - article.video-wrapper div.video-player-pre, article.video-wrapper div.video-player-post { + .video-wrapper .video-player-pre, + .video-wrapper .video-player-post { height: 0; } - article.video-wrapper section.video-player { + .video-wrapper .video-player { h3 { color: black; } } - ol.subtitles { + .subtitles { @extend .is-hidden; } - ol.subtitles.html5 { + .subtitles.html5 { @extend %ui-depth0; background-color: rgba(243, 243, 243, 0.8); height: 100%; @@ -743,63 +798,66 @@ div.video { border-radius: 0; &.closed { - div.tc-wrapper { - article.video-wrapper { + .tc-wrapper { + .video-wrapper { width: 100%; } } } - article.video-wrapper div.video-player-pre, article.video-wrapper div.video-player-post { + .video-wrapper .video-player-pre, + .video-wrapper .video-player-post { height: 0; } - article.video-wrapper { + .video-wrapper { position: static; } - article.video-wrapper section.video-player { + .video-wrapper .video-player { h3 { color: white; } } - div.tc-wrapper { + .tc-wrapper { @include clearfix(); width: 100%; height: 100%; position: static; - article.video-wrapper { + .video-wrapper { height: 100%; width: 75%; + @include margin-right(0); vertical-align: middle; - margin-right: 0; - object, iframe, video{ + object, + iframe, + video{ position: absolute; width: auto; height: auto; } } - section.video-controls { + .video-controls { @extend %ui-depth4; + position: absolute; bottom: 0; left: 0; - position: absolute; width: 100%; } } - ol.subtitles { - @include box-sizing(border-box); - @include transition(none); - background: $black; + .subtitles { height: 100%; width: 25%; padding: lh(); + @include box-sizing(border-box); + @include transition(none); + background: $black; visibility: visible; li { @@ -813,9 +871,11 @@ div.video { } &.is-touch { - div.tc-wrapper { - article.video-wrapper { - object, iframe, video { + .tc-wrapper { + .video-wrapper { + object, + iframe, + video { width: 100%; height: 100%; } @@ -864,5 +924,3 @@ div.video { } } } - - diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_accessible_menu_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_accessible_menu_spec.js index a790f8ecae..feae0e58f3 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_accessible_menu_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_accessible_menu_spec.js @@ -260,7 +260,7 @@ state.videoSpeedControl.setSpeed(1.0); spyOn(state.videoPlayer, 'onSpeedChange').andCallThrough(); - $('li[data-speed="0.75"] a').click(); + $('li[data-speed="0.75"] .speed-link').click(); }); it('trigger speedChange event', function () { @@ -274,7 +274,7 @@ xdescribe('onSpeedChange', function () { beforeEach(function () { state = jasmine.initializePlayer(); - $('li[data-speed="1.0"] a').addClass('active'); + $('li[data-speed="1.0"] .speed-link').addClass('active'); state.videoSpeedControl.setSpeed(0.75); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js index b8d489d861..9d822bd8c1 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_caption_spec.js @@ -23,39 +23,39 @@ }); describe('constructor', function () { + describe('always', function () { + beforeEach(function () { spyOn($, 'ajaxWithPrefix').andCallThrough(); }); - it('create the caption element', function () { + it('create the transcript element', function () { state = jasmine.initializePlayer(); - expect($('.video')).toContain('ol.subtitles'); + expect($('.video')).toContain('.subtitles'); }); - it('add caption control to video player', function () { + it('add transcript control to video player', function () { state = jasmine.initializePlayer(); - expect($('.video')).toContain('a.hide-subtitles'); + expect($('.video')).toContain('.toggle-transcript'); }); - it('add ARIA attributes to caption control', function () { + it('add ARIA attributes to transcript control', function () { state = jasmine.initializePlayer(); - var captionControl = $('a.hide-subtitles'); + var captionControl = $('.toggle-transcript'); expect(captionControl).toHaveAttrs({ - 'role': 'button', - 'title': 'Turn off captions', 'aria-disabled': 'false' }); }); - it('fetch the caption in HTML5 mode', function () { + it('fetch the transcript in HTML5 mode', function () { runs(function () { state = jasmine.initializePlayer(); }); waitsFor(function () { return state.videoCaption.loaded; - }, 'Expect captions to be loaded.', WAIT_TIMEOUT); + }, 'Expect transcript to be loaded.', WAIT_TIMEOUT); runs(function () { expect($.ajaxWithPrefix).toHaveBeenCalledWith({ @@ -70,7 +70,7 @@ }); }); - it('fetch the caption in Flash mode', function () { + it('fetch the transcript in Flash mode', function () { runs(function () { state = jasmine.initializePlayerYouTube(); spyOn(state, 'isFlashMode').andReturn(true); @@ -79,7 +79,7 @@ waitsFor(function () { return state.videoCaption.loaded; - }, 'Expect captions to be loaded.', WAIT_TIMEOUT); + }, 'Expect transcript to be loaded.', WAIT_TIMEOUT); runs(function () { expect($.ajaxWithPrefix).toHaveBeenCalledWith({ @@ -96,14 +96,14 @@ }); }); - it('fetch the caption in Youtube mode', function () { + it('fetch the transcript in Youtube mode', function () { runs(function () { state = jasmine.initializePlayerYouTube(); }); waitsFor(function () { return state.videoCaption.loaded; - }, 'Expect captions to be loaded.', WAIT_TIMEOUT); + }, 'Expect transcript to be loaded.', WAIT_TIMEOUT); runs(function () { expect($.ajaxWithPrefix).toHaveBeenCalledWith({ @@ -159,7 +159,14 @@ }); describe('renderLanguageMenu', function () { + describe('is rendered', function () { + var KEY = $.ui.keyCode, + + keyPressEvent = function(key) { + return $.Event('keydown', { keyCode: key }); + }; + it('if languages more than 1', function () { state = jasmine.initializePlayer(); var transcripts = state.config.transcriptLanguages, @@ -172,7 +179,7 @@ $('.langs-list li').each(function(index) { var code = $(this).data('lang-code'), - link = $(this).find('a'), + link = $(this).find('.control'), label = link.text(); expect(code).toBeInArray(langCodes); @@ -183,7 +190,7 @@ it('when clicking on link with new language', function () { state = jasmine.initializePlayer(); var Caption = state.videoCaption, - link = $('.langs-list li[data-lang-code="de"] a'); + link = $('.langs-list li[data-lang-code="de"] .control-lang'); spyOn(Caption, 'fetchCaption'); spyOn(state.storage, 'setItem'); @@ -201,7 +208,7 @@ it('when clicking on link with current language', function () { state = jasmine.initializePlayer(); var Caption = state.videoCaption, - link = $('.langs-list li[data-lang-code="en"] a'); + link = $('.langs-list li[data-lang-code="en"] .control-lang'); spyOn(Caption, 'fetchCaption'); spyOn(state.storage, 'setItem'); @@ -223,6 +230,23 @@ $('.lang').mouseleave(); expect($('.lang')).not.toHaveClass('is-opened'); }); + + it('opens the language menu on arrow up', function() { + state = jasmine.initializePlayer(); + $('.language-menu').focus(); + $('.language-menu').trigger(keyPressEvent(KEY.UP)); + expect($('.lang')).toHaveClass('is-opened'); + expect($('.langs-list').find('li').last().find('.control-lang')).toBeFocused(); + }); + + it('closes the language menu on ESC', function() { + state = jasmine.initializePlayer(); + $('.language-menu').trigger(keyPressEvent(KEY.UP)); + expect($('.lang')).toHaveClass('is-opened'); + $('.language-menu').trigger(keyPressEvent(KEY.ESCAPE)); + expect($('.lang')).not.toHaveClass('is-opened'); + expect($('.language-menu')).toBeFocused(); + }); }); describe('is not rendered', function () { @@ -246,10 +270,10 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); - it('render the caption', function () { + it('render the transcript', function () { runs(function () { var captionsData = jasmine.stubbedCaption, items = $('.subtitles li[data-index]'); @@ -267,7 +291,7 @@ }); }); - it('add a padding element to caption', function () { + it('add a padding element to transcript', function () { runs(function () { expect($('.subtitles li:first').hasClass('spacing')) .toBe(true); @@ -277,7 +301,7 @@ }); - it('bind all the caption link', function () { + it('bind all the transcript link', function () { runs(function () { var handlerList = ['captionMouseOverOut', 'captionClick', 'captionMouseDown', 'captionFocus', 'captionBlur', @@ -323,7 +347,7 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); runs(function () { expect(state.videoCaption.rendered).toBeTruthy(); @@ -346,14 +370,14 @@ ); }); - it('show captions on play', function () { + it('show transcript on play', function () { runs(function () { state.el.trigger('play'); }); waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); runs(function () { var captionsData = jasmine.stubbedCaption, @@ -377,7 +401,7 @@ }); }); - describe('when no captions file was specified', function () { + describe('when no transcripts file was specified', function () { beforeEach(function () { state = jasmine.initializePlayer('video_all.html', { 'sub': '', @@ -385,8 +409,8 @@ }); }); - it('captions panel is not shown', function () { - expect(state.videoCaption.hideSubtitlesEl).toBeHidden(); + it('transcript panel is not shown', function () { + expect(state.videoCaption.languageChooserEl).toBeHidden(); }); }); }); @@ -403,10 +427,10 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); - describe('when cursor is outside of the caption box', function () { + describe('when cursor is outside of the transcript box', function () { it('does not set freezing timeout', function () { runs(function () { expect(state.videoCaption.frozen).toBeFalsy(); @@ -414,7 +438,7 @@ }); }); - describe('when cursor is in the caption box', function () { + describe('when cursor is in the transcript box', function () { beforeEach(function () { spyOn(state.videoCaption, 'onMouseLeave'); runs(function () { @@ -452,7 +476,7 @@ }); describe( - 'when cursor is moving out of the caption box', + 'when cursor is moving out of the transcript box', function () { beforeEach(function () { @@ -469,7 +493,7 @@ expect(window.clearTimeout).toHaveBeenCalledWith(100); }); - it('unfreeze the caption', function () { + it('unfreeze the transcript', function () { expect(state.videoCaption.frozen).toBeNull(); }); }); @@ -482,7 +506,7 @@ $('.subtitles').trigger(jQuery.Event('mouseout')); }); - it('scroll the caption', function () { + it('scroll the transcript', function () { expect($.fn.scrollTo).toHaveBeenCalled(); }); }); @@ -493,7 +517,7 @@ $('.subtitles').trigger(jQuery.Event('mouseout')); }); - it('does not scroll the caption', function () { + it('does not scroll the transcript', function () { expect($.fn.scrollTo).not.toHaveBeenCalled(); }); }); @@ -514,7 +538,7 @@ spyOn(state, 'youtubeId').andReturn('Z5KLxerq05Y'); }); - it('show caption on language change', function () { + it('show transcript on language change', function () { Caption.loaded = true; Caption.fetchCaption(); @@ -522,7 +546,7 @@ expect(Caption.hideCaptions).toHaveBeenCalledWith(false); }); - msg = 'use cookie to show/hide captions if they have not been ' + + msg = 'use cookie to show/hide transcripts if they have not been ' + 'loaded yet'; it(msg, function () { Caption.loaded = false; @@ -554,7 +578,7 @@ }); msg = 'on success: change language on touch devices when ' + - 'captions have not been rendered yet'; + 'transcripts have not been rendered yet'; it(msg, function () { state.isTouch = true; Caption.loaded = true; @@ -604,7 +628,7 @@ expect(Caption.loaded).toBeTruthy(); }); - msg = 'on error: captions are hidden if there are no transcripts'; + msg = 'on error: transcripts are hidden if there are no transcripts'; it(msg, function () { spyOn(Caption, 'fetchAvailableTranslations'); $.ajax.andCallFake(function (settings) { @@ -619,7 +643,6 @@ expect(Caption.fetchAvailableTranslations).not.toHaveBeenCalled(); expect(Caption.hideCaptions.mostRecentCall.args) .toEqual([true, false]); - expect(Caption.hideSubtitlesEl).toBeHidden(); }); msg = 'on error: for Html5 player an attempt to fetch transcript ' + @@ -667,7 +690,7 @@ msg = 'on error: fetch available translations if there are ' + 'additional transcripts'; - xit(msg, function () { + it(msg, function () { $.ajax .andCallFake(function (settings) { _.result(settings, 'error'); @@ -683,7 +706,6 @@ expect($.ajaxWithPrefix).toHaveBeenCalled(); expect(Caption.fetchAvailableTranslations).toHaveBeenCalled(); - expect(Caption.hideCaptions).not.toHaveBeenCalled(); }); }); @@ -745,7 +767,7 @@ expect(Caption.renderLanguageMenu).not.toHaveBeenCalled(); }); - msg = 'on error: captions are hidden if there are no transcript'; + msg = 'on error: transcripts are hidden if there are no transcript'; it(msg, function () { $.ajax.andCallFake(function (settings) { _.result(settings, 'error'); @@ -754,12 +776,12 @@ expect($.ajaxWithPrefix).toHaveBeenCalled(); expect(Caption.hideCaptions).toHaveBeenCalledWith(true, false); - expect(Caption.hideSubtitlesEl).toBeHidden(); + expect(Caption.subtitlesEl).toBeHidden(); }); }); describe('play', function () { - describe('when the caption was not rendered', function () { + describe('when the transcript was not rendered', function () { beforeEach(function () { window.onTouchBasedDevice.andReturn(['iPad']); @@ -770,10 +792,10 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); - it('render the caption', function () { + it('render the transcript', function () { runs(function () { var captionsData; @@ -792,7 +814,7 @@ }); - it('add a padding element to caption', function () { + it('add a padding element to transcript', function () { runs(function () { expect($('.subtitles li:first')).toBe('.spacing'); expect($('.subtitles li:last')).toBe('.spacing'); @@ -833,7 +855,7 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); describe('when the video speed is 1.0x', function () { @@ -852,7 +874,7 @@ }); describe('when the video speed is not 1.0x', function () { - it('search the caption based on 1.0x speed', function () { + it('search the transcript based on 1.0x speed', function () { runs(function () { state.videoCaption.updatePlayTime(25.000); expect(state.videoCaption.currentIndex).toEqual(5); @@ -882,14 +904,14 @@ }); }); - it('deactivate the previous caption', function () { + it('deactivate the previous transcript', function () { runs(function () { expect($('.subtitles li[data-index=1]')) .not.toHaveClass('current'); }); }); - it('activate new caption', function () { + it('activate new transcript', function () { runs(function () { expect($('.subtitles li[data-index=5]')) .toHaveClass('current'); @@ -902,7 +924,7 @@ }); }); - it('scroll caption to new position', function () { + it('scroll transcript to new position', function () { runs(function () { expect($.fn.scrollTo).toHaveBeenCalled(); }); @@ -930,7 +952,7 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); runs(function () { videoControl = state.videoControl; @@ -939,8 +961,8 @@ }); }); - describe('set the height of caption container', function () { - it('when CC button is enabled', function () { + describe('set the height of transcript container', function () { + it('when transcript button is enabled', function () { runs(function () { var realHeight = parseInt( $('.subtitles').css('maxHeight'), 10 @@ -953,7 +975,7 @@ }); }); - it('when CC button is disabled ', function () { + it('when transcript button is disabled ', function () { runs(function () { var realHeight, videoWrapperHeight, progressSliderHeight, controlHeight, shouldBeHeight; @@ -976,7 +998,7 @@ }); }); - it('set the height of caption spacing', function () { + it('set the height of transcript spacing', function () { runs(function () { var firstSpacing, lastSpacing; @@ -994,7 +1016,7 @@ }); }); - it('scroll caption to new position', function () { + it('scroll transcript to new position', function () { runs(function () { expect($.fn.scrollTo).toHaveBeenCalled(); }); @@ -1009,11 +1031,11 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); describe('when frozen', function () { - it('does not scroll the caption', function () { + it('does not scroll the transcript', function () { runs(function () { state.videoCaption.frozen = true; $('.subtitles li[data-index=1]').addClass('current'); @@ -1030,8 +1052,8 @@ }); }); - describe('when there is no current caption', function () { - it('does not scroll the caption', function () { + describe('when there is no current transcript', function () { + it('does not scroll the transcript', function () { runs(function () { state.videoCaption.scrollCaption(); expect($.fn.scrollTo).not.toHaveBeenCalled(); @@ -1039,8 +1061,8 @@ }); }); - describe('when there is a current caption', function () { - it('scroll to current caption', function () { + describe('when there is a current transcript', function () { + it('scroll to current transcript', function () { runs(function () { $('.subtitles li[data-index=1]').addClass('current'); state.videoCaption.scrollCaption(); @@ -1062,7 +1084,7 @@ isRendered = state.videoCaption.rendered; return isRendered && duration; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); describe('when the video speed is 1.0x', function () { @@ -1104,40 +1126,30 @@ $('.subtitles li[data-index=1]').addClass('current'); }); - describe('when the caption is visible', function () { + describe('when the transcript is visible', function () { beforeEach(function () { state.el.removeClass('closed'); state.videoCaption.toggle(jQuery.Event('click')); }); - it('hide the caption', function () { + it('hide the transcript', function () { expect(state.el).toHaveClass('closed'); }); - - it('changes ARIA attribute of caption control', function () { - expect($('a.hide-subtitles')) - .toHaveAttr('title', 'Turn on captions'); - }); }); - describe('when the caption is hidden', function () { + describe('when the transcript is hidden', function () { beforeEach(function () { state.el.addClass('closed'); state.videoCaption.toggle(jQuery.Event('click')); jasmine.Clock.useMock(); }); - it('show the caption', function () { + it('show the transcript', function () { expect(state.el).not.toHaveClass('closed'); }); - it('changes ARIA attribute of caption control', function () { - expect($('a.hide-subtitles')) - .toHaveAttr('title', 'Turn off captions'); - }); - // Test turned off due to flakiness (11/25/13) - xit('scroll the caption', function () { + xit('scroll the transcript', function () { // After transcripts are shown, and the video plays for a // bit. jasmine.Clock.tick(1000); @@ -1153,7 +1165,7 @@ }); }); - describe('caption accessibility', function () { + describe('transcript accessibility', function () { beforeEach(function () { runs(function () { state = jasmine.initializePlayer(); @@ -1161,7 +1173,7 @@ waitsFor(function () { return state.videoCaption.rendered; - }, 'Captions are not rendered', WAIT_TIMEOUT); + }, 'Transcripts are not rendered', WAIT_TIMEOUT); }); describe('when getting focus through TAB key', function () { @@ -1174,7 +1186,7 @@ }); }); - it('shows an outline around the caption', function () { + it('shows an outline around the transcript', function () { runs(function () { expect($('.subtitles li[data-index=0]')) .toHaveClass('focused'); @@ -1197,7 +1209,7 @@ }); }); - it('does not show an outline around the caption', function () { + it('does not show an outline around the transcript', function () { runs(function () { expect($('.subtitles li[data-index=0]')) .not.toHaveClass('focused'); @@ -1212,7 +1224,7 @@ }); describe( - 'when same caption gets the focus through mouse after ' + + 'when same transcript gets the focus through mouse after ' + 'having focus through TAB key', function () { @@ -1241,7 +1253,7 @@ }); describe( - 'when a second caption gets focus through mouse after ' + + 'when a second transcript gets focus through mouse after ' + 'first had focus through TAB key', function () { diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_context_menu_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_context_menu_spec.js index 295b151a4f..96b854340b 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_context_menu_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_context_menu_spec.js @@ -5,16 +5,16 @@ closeSubmenuKeyboard, menu, menuItems, menuSubmenuItem, submenu, submenuItems, overlay, playButton; openMenu = function () { - var container = $('div.video'); + var container = $('.video'); jasmine.Clock.useMock(); container.find('video').trigger('contextmenu'); - menu = container.children('ol.contextmenu'); - menuItems = menu.children('li.menu-item').not('.submenu-item'); - menuSubmenuItem = menu.children('li.menu-item.submenu-item'); - submenu = menuSubmenuItem.children('ol.submenu'); - submenuItems = submenu.children('li.menu-item'); - overlay = container.children('div.overlay'); - playButton = $('a.video_control.play'); + menu = container.children('.contextmenu'); + menuItems = menu.children('.menu-item').not('.submenu-item'); + menuSubmenuItem = menu.children('.menu-item.submenu-item'); + submenu = menuSubmenuItem.children('.submenu'); + submenuItems = submenu.children('.menu-item'); + overlay = container.children('.overlay'); + playButton = $('.video_control.play'); }; keyPressEvent = function(key) { diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_full_screen_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_full_screen_spec.js index 215b891f41..6aa858b73f 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_full_screen_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_full_screen_spec.js @@ -30,8 +30,6 @@ var fullScreenControl = $('.add-fullscreen'); expect(fullScreenControl).toHaveAttrs({ - 'role': 'button', - 'title': 'Fill browser', 'aria-disabled': 'false' }); }); @@ -53,14 +51,10 @@ var fullScreenControl = $('.add-fullscreen'); fullScreenControl.click(); expect(fullScreenControl).toHaveAttrs({ - 'role': 'button', - 'title': 'Exit full browser', 'aria-disabled': 'false' }); fullScreenControl.click(); expect(fullScreenControl).toHaveAttrs({ - 'role': 'button', - 'title': 'Fill browser', 'aria-disabled': 'false' }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_play_pause_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_play_pause_control_spec.js index 877dc9861e..0cbb23c1f1 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_play_pause_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_play_pause_control_spec.js @@ -25,8 +25,6 @@ it('add ARIA attributes to play control', function () { expect($('.video_control.play')).toHaveAttrs({ - 'role': 'button', - 'title': 'Play', 'aria-disabled': 'false' }); }); @@ -34,8 +32,6 @@ it('can update ARIA state on play', function () { state.el.trigger('play'); expect($('.video_control.pause')).toHaveAttrs({ - 'role': 'button', - 'title': 'Pause', 'aria-disabled': 'false' }); }); @@ -44,8 +40,6 @@ state.el.trigger('play'); state.el.trigger('ended'); expect($('.video_control.play')).toHaveAttrs({ - 'role': 'button', - 'title': 'Play', 'aria-disabled': 'false' }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_play_skip_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_play_skip_control_spec.js index 9ccea6a0ab..725bdd5c8a 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_play_skip_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_play_skip_control_spec.js @@ -27,8 +27,6 @@ it('add ARIA attributes to play control', function () { expect($('.video_control.play')).toHaveAttrs({ - 'role': 'button', - 'title': 'Play', 'aria-disabled': 'false' }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js index 843b6c3f65..e521282ec8 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_player_spec.js @@ -745,11 +745,6 @@ function (VideoPlayer) { $('.add-fullscreen').click(); }); - it('replace the full screen button tooltip', function () { - expect($('.add-fullscreen')) - .toHaveAttr('title', 'Exit full browser'); - }); - it('add the video-fullscreen class', function () { expect(state.el).toHaveClass('video-fullscreen'); }); @@ -773,11 +768,6 @@ function (VideoPlayer) { $('.add-fullscreen').click(); }); - it('replace the full screen button tooltip', function () { - expect($('.add-fullscreen')) - .toHaveAttr('title', 'Fill browser'); - }); - it('remove the video-fullscreen class', function () { expect(state.el).not.toHaveClass('video-fullscreen'); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_quality_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_quality_control_spec.js index 0bf3722a4c..5ce7375aee 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_quality_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_quality_control_spec.js @@ -33,8 +33,6 @@ it('add ARIA attributes to quality control', function () { expect(qualityControl.el).toHaveAttrs({ - 'role': 'button', - 'title': 'HD off', 'aria-disabled': 'false' }); }); @@ -117,7 +115,7 @@ it('does not contain the quality control', function () { state = jasmine.initializePlayer(); - expect(state.el.find('a.quality-control').length).toBe(0); + expect(state.el.find('.quality-control').length).toBe(0); }); }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_skip_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_skip_control_spec.js index da3a87845b..d3c0b47d52 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_skip_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_skip_control_spec.js @@ -33,8 +33,6 @@ it('add ARIA attributes to play control', function () { state.el.trigger('play'); expect($('.skip-control')).toHaveAttrs({ - 'role': 'button', - 'title': 'Do not show again', 'aria-disabled': 'false' }); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js index d5b14e6b2d..2ea4a7e365 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_speed_control_spec.js @@ -1,4 +1,5 @@ (function (undefined) { + 'use strict'; describe('VideoSpeedControl', function () { var state, oldOTBD; @@ -38,21 +39,11 @@ expect($(link)).toHaveData( 'speed', state.speeds[index] ); - expect($(link).find('a').text()).toBe( + expect($(link).find('.speed-option').text()).toBe( state.speeds[index] + 'x' ); }); }); - - it('add ARIA attributes to speed control', function () { - var speedControl = $('div.speeds>a'); - - expect(speedControl).toHaveAttrs({ - 'role': 'button', - 'title': 'Speeds', - 'aria-disabled': 'false' - }); - }); }); describe('when running on touch based device', function () { @@ -61,33 +52,17 @@ window.onTouchBasedDevice.andReturn([device]); state = jasmine.initializePlayer(); - expect(state.el.find('div.speeds')).not.toExist(); + expect(state.el.find('.speeds')).not.toExist(); }); }); }); describe('when running on non-touch based device', function () { - var speedControl, speedEntries, speedButton, + var speedControl, speedEntries, speedButton, speedsContainer, KEY = $.ui.keyCode, keyPressEvent = function(key) { return $.Event('keydown', {keyCode: key}); - }, - - // Get previous element in array or cyles back to the last - // if it is the first. - previousSpeed = function(index) { - return speedEntries.eq(index < 1 ? - speedEntries.length - 1 : - index - 1); - }, - - // Get next element in array or cyles back to the first if - // it is the last. - nextSpeed = function(index) { - return speedEntries.eq(index >= speedEntries.length-1 ? - 0 : - index + 1); }; beforeEach(function () { @@ -95,7 +70,7 @@ speedControl = $('.speeds'); speedButton = $('.speed-button'); speedsContainer = $('.video-speeds'); - speedEntries = speedsContainer.find('a'); + speedEntries = speedsContainer.find('.speed-option'); }); it('open/close the speed menu on mouseenter/mouseleave', @@ -114,11 +89,6 @@ expect(speedControl).toHaveClass('is-opened'); }); - it('close the speed menu on click', function () { - speedControl.mouseenter().click(); - expect(speedControl).not.toHaveClass('is-opened'); - }); - it('close the speed menu on outside click', function () { speedControl.trigger(keyPressEvent(KEY.ENTER)); $(window).click(); @@ -150,8 +120,7 @@ it('UP and DOWN keydown function as expected on speed entries', function () { - var lastEntry = speedEntries.length-1, - speed_0_75 = speedEntries.filter(':contains("0.75x")'), + var speed_0_75 = speedEntries.filter(':contains("0.75x")'), speed_1_0 = speedEntries.filter(':contains("1.0x")'); // First open menu @@ -226,7 +195,7 @@ it('trigger speedChange event', function () { spyOnEvent(state.el, 'speedchange'); - $('li[data-speed="0.75"] a').click(); + $('li[data-speed="0.75"] .speed-option').click(); expect('speedchange').toHaveBeenTriggeredOn(state.el); expect(state.videoSpeedControl.currentSpeed).toEqual('0.75'); }); diff --git a/common/lib/xmodule/xmodule/js/spec/video/video_volume_control_spec.js b/common/lib/xmodule/xmodule/js/spec/video/video_volume_control_spec.js index e1edb571d3..5c3f782e85 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/video_volume_control_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/video_volume_control_spec.js @@ -3,6 +3,12 @@ describe('VideoVolumeControl', function () { var state, oldOTBD, volumeControl; + var KEY = $.ui.keyCode, + + keyPressEvent = function(key) { + return $.Event('keydown', { keyCode: key }); + }; + beforeEach(function () { oldOTBD = window.onTouchBasedDevice; window.onTouchBasedDevice = jasmine.createSpy('onTouchBasedDevice') @@ -56,24 +62,20 @@ describe('VideoVolumeControl', function () { var liveRegion = $('.video-live-region'); expect(liveRegion).toHaveAttrs({ - 'role': 'status', - 'aria-live': 'polite', - 'aria-atomic': 'false' + 'aria-live': 'polite' }); }); it('add ARIA attributes to volume control', function () { - var button = $('.volume > a'); + var button = $('.volume .control'); expect(button).toHaveAttrs({ - 'role': 'button', - 'title': 'Volume', 'aria-disabled': 'false' }); }); it('bind the volume control', function () { - var button = $('.volume > a'); + var button = $('.volume .control'); expect(button).toHandle('keydown'); expect(button).toHandle('mousedown'); @@ -185,16 +187,19 @@ describe('VideoVolumeControl', function () { }); describe('increaseVolume', function () { + beforeEach(function () { state = jasmine.initializePlayer(); volumeControl = state.videoVolumeControl; }); it('volume is increased correctly', function () { + var button = $('.volume .control'); volumeControl.volume = 60; - state.el.trigger(jQuery.Event("keydown", { - keyCode: $.ui.keyCode.UP - })); + + // adjust the volume + button.focus(); + button.trigger(keyPressEvent(KEY.UP)); expect(volumeControl.volume).toEqual(80); }); @@ -206,16 +211,19 @@ describe('VideoVolumeControl', function () { }); describe('decreaseVolume', function () { + beforeEach(function () { state = jasmine.initializePlayer(); volumeControl = state.videoVolumeControl; }); it('volume is decreased correctly', function () { + var button = $('.volume .control'); volumeControl.volume = 60; - state.el.trigger(jQuery.Event("keydown", { - keyCode: $.ui.keyCode.DOWN - })); + + // adjust the volume + button.focus(); + button.trigger(keyPressEvent(KEY.DOWN)); expect(volumeControl.volume).toEqual(40); }); @@ -274,21 +282,21 @@ describe('VideoVolumeControl', function () { it('nothing happens if ALT+keyUp are pushed down', function () { assertVolumeIsNotChanged({ - keyCode: $.ui.keyCode.UP, + keyCode: KEY.UP, altKey: true }); }); it('nothing happens if SHIFT+keyUp are pushed down', function () { assertVolumeIsNotChanged({ - keyCode: $.ui.keyCode.UP, + keyCode: KEY.UP, shiftKey: true }); }); it('nothing happens if SHIFT+keyDown are pushed down', function () { assertVolumeIsNotChanged({ - keyCode: $.ui.keyCode.DOWN, + keyCode: KEY.DOWN, shiftKey: true }); }); @@ -302,8 +310,8 @@ describe('VideoVolumeControl', function () { it('nothing happens if ALT+ENTER are pushed down', function () { var isMuted = volumeControl.getMuteStatus(); - $('.volume > a').trigger(jQuery.Event("keydown", { - keyCode: $.ui.keyCode.ENTER, + $('.volume .control').trigger(jQuery.Event("keydown", { + keyCode: KEY.ENTER, altKey: true })); expect(volumeControl.getMuteStatus()).toEqual(isMuted); diff --git a/common/lib/xmodule/xmodule/js/src/video/04_video_full_screen.js b/common/lib/xmodule/xmodule/js/src/video/04_video_full_screen.js index e561852057..916f840a9f 100644 --- a/common/lib/xmodule/xmodule/js/src/video/04_video_full_screen.js +++ b/common/lib/xmodule/xmodule/js/src/video/04_video_full_screen.js @@ -2,10 +2,14 @@ 'use strict'; define('video/04_video_full_screen.js', [], function () { var template = [ - '', - gettext('Fill browser'), - '' + '' ].join(''); // VideoControl() function - what this module "exports". @@ -133,8 +137,12 @@ define('video/04_video_full_screen.js', [], function () { fullScreenClassNameEl.removeClass('video-fullscreen'); $(window).scrollTop(this.scrollPos); this.videoFullScreen.fullScreenEl - .attr('title', gettext('Fill browser')) - .text(gettext('Fill browser')); + .find('.icon') + .removeClass('fa-compress') + .addClass('fa-arrows-alt') + .find('.control-text') + .text(gettext('Fill browser')); + this.el.trigger('fullscreen', [this.isFullScreen]); } @@ -146,8 +154,12 @@ define('video/04_video_full_screen.js', [], function () { this.videoFullScreen.fullScreenState = this.isFullScreen = true; fullScreenClassNameEl.addClass('video-fullscreen'); this.videoFullScreen.fullScreenEl - .attr('title', gettext('Exit full browser')) - .text(gettext('Exit full browser')); + .find('.icon') + .removeClass('fa-arrows-alt') + .addClass('fa-compress') + .find('.control-text') + .text(gettext('Exit full browser')); + this.el.trigger('fullscreen', [this.isFullScreen]); } diff --git a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js index 11965fe31e..10e119b1ad 100644 --- a/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/05_video_quality_control.js @@ -1,15 +1,27 @@ (function (requirejs, require, define) { // VideoQualityControl module. +'use strict'; define( 'video/05_video_quality_control.js', [], function () { var template = [ - '' + '' ].join(''); // VideoQualityControl() function - what this module "exports". @@ -134,17 +146,17 @@ function () { var controlStateStr; this.videoQualityControl.quality = value; if (_.contains(this.config.availableHDQualities, value)) { - controlStateStr = gettext('HD on'); + controlStateStr = gettext('on'); this.videoQualityControl.el .addClass('active') - .attr('title', controlStateStr) - .text(controlStateStr); + .find('.control-text') + .text(controlStateStr); } else { - controlStateStr = gettext('HD off'); + controlStateStr = gettext('off'); this.videoQualityControl.el .removeClass('active') - .attr('title', controlStateStr) - .text(controlStateStr); + .find('.control-text') + .text(controlStateStr); } } diff --git a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js index 7177ee9215..aff29a4459 100644 --- a/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js +++ b/common/lib/xmodule/xmodule/js/src/video/07_video_volume_control.js @@ -38,13 +38,25 @@ function() { step: 20, template: [ - '
', - '', - '