From be239bdbcac3702ab0136898c1bf0362afbcb38e Mon Sep 17 00:00:00 2001 From: "Dave St.Germain" Date: Mon, 10 Mar 2014 09:43:56 -0400 Subject: [PATCH 1/8] remove old version of codemirror, to prevent confusion --- cms/envs/common.py | 2 +- .../vendor/CodeMirror/codemirror-3.21.0.css | 263 --------------- .../js/vendor/CodeMirror/codemirror.css | 301 +++++++++++++----- lms/djangoapps/course_wiki/editors.py | 4 +- 4 files changed, 229 insertions(+), 341 deletions(-) delete mode 100644 common/static/js/vendor/CodeMirror/codemirror-3.21.0.css diff --git a/cms/envs/common.py b/cms/envs/common.py index 03c1df3c22..a5baa347e0 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -300,7 +300,7 @@ PIPELINE_CSS = { 'css/vendor/normalize.css', 'css/vendor/font-awesome.css', 'css/vendor/html5-input-polyfills/number-polyfill.css', - 'js/vendor/CodeMirror/codemirror-3.21.0.css', + 'js/vendor/CodeMirror/codemirror.css', 'css/vendor/ui-lightness/jquery-ui-1.8.22.custom.css', 'css/vendor/jquery.qtip.min.css', 'js/vendor/markitup/skins/simple/style.css', diff --git a/common/static/js/vendor/CodeMirror/codemirror-3.21.0.css b/common/static/js/vendor/CodeMirror/codemirror-3.21.0.css deleted file mode 100644 index 23eaf74d44..0000000000 --- a/common/static/js/vendor/CodeMirror/codemirror-3.21.0.css +++ /dev/null @@ -1,263 +0,0 @@ -/* BASICS */ - -.CodeMirror { - /* Set height, width, borders, and global font properties here */ - font-family: monospace; - height: 300px; -} -.CodeMirror-scroll { - /* Set scrolling behaviour here */ - overflow: auto; -} - -/* PADDING */ - -.CodeMirror-lines { - padding: 4px 0; /* Vertical padding around content */ -} -.CodeMirror pre { - padding: 0 4px; /* Horizontal padding of content */ -} - -.CodeMirror-scrollbar-filler, .CodeMirror-gutter-filler { - background-color: white; /* The little square between H and V scrollbars */ -} - -/* GUTTER */ - -.CodeMirror-gutters { - border-right: 1px solid #ddd; - background-color: #f7f7f7; - white-space: nowrap; -} -.CodeMirror-linenumbers {} -.CodeMirror-linenumber { - padding: 0 3px 0 5px; - min-width: 20px; - text-align: right; - color: #999; -} - -/* CURSOR */ - -.CodeMirror div.CodeMirror-cursor { - border-left: 1px solid black; - z-index: 3; -} -/* Shown when moving in bi-directional text */ -.CodeMirror div.CodeMirror-secondarycursor { - border-left: 1px solid silver; -} -.CodeMirror.cm-keymap-fat-cursor div.CodeMirror-cursor { - width: auto; - border: 0; - background: #7e7; - z-index: 1; -} -/* Can style cursor different in overwrite (non-insert) mode */ -.CodeMirror div.CodeMirror-cursor.CodeMirror-overwrite {} - -.cm-tab { display: inline-block; } - -/* DEFAULT THEME */ - -.cm-s-default .cm-keyword {color: #708;} -.cm-s-default .cm-atom {color: #219;} -.cm-s-default .cm-number {color: #164;} -.cm-s-default .cm-def {color: #00f;} -.cm-s-default .cm-variable {color: black;} -.cm-s-default .cm-variable-2 {color: #05a;} -.cm-s-default .cm-variable-3 {color: #085;} -.cm-s-default .cm-property {color: black;} -.cm-s-default .cm-operator {color: black;} -.cm-s-default .cm-comment {color: #a50;} -.cm-s-default .cm-string {color: #a11;} -.cm-s-default .cm-string-2 {color: #f50;} -.cm-s-default .cm-meta {color: #555;} -.cm-s-default .cm-qualifier {color: #555;} -.cm-s-default .cm-builtin {color: #30a;} -.cm-s-default .cm-bracket {color: #997;} -.cm-s-default .cm-tag {color: #170;} -.cm-s-default .cm-attribute {color: #00c;} -.cm-s-default .cm-header {color: blue;} -.cm-s-default .cm-quote {color: #090;} -.cm-s-default .cm-hr {color: #999;} -.cm-s-default .cm-link {color: #00c;} - -.cm-negative {color: #d44;} -.cm-positive {color: #292;} -.cm-header, .cm-strong {font-weight: bold;} -.cm-em {font-style: italic;} -.cm-link {text-decoration: underline;} - -.cm-s-default .cm-error {color: #f00;} -.cm-invalidchar {color: #f00;} - -div.CodeMirror span.CodeMirror-matchingbracket {color: #0f0;} -div.CodeMirror span.CodeMirror-nonmatchingbracket {color: #f22;} -.CodeMirror-activeline-background {background: #e8f2ff;} - -/* STOP */ - -/* The rest of this file contains styles related to the mechanics of - the editor. You probably shouldn't touch them. */ - -.CodeMirror { - line-height: 1; - position: relative; - overflow: hidden; - background: white; - color: black; -} - -.CodeMirror-scroll { - /* 30px is the magic margin used to hide the element's real scrollbars */ - /* See overflow: hidden in .CodeMirror */ - margin-bottom: -30px; margin-right: -30px; - padding-bottom: 30px; padding-right: 30px; - height: 100%; - outline: none; /* Prevent dragging from highlighting the element */ - position: relative; - -moz-box-sizing: content-box; - box-sizing: content-box; -} -.CodeMirror-sizer { - position: relative; -} - -/* The fake, visible scrollbars. Used to force redraw during scrolling - before actuall scrolling happens, thus preventing shaking and - flickering artifacts. */ -.CodeMirror-vscrollbar, .CodeMirror-hscrollbar, .CodeMirror-scrollbar-filler, .CodeMirror-gutter-filler { - position: absolute; - z-index: 6; - display: none; -} -.CodeMirror-vscrollbar { - right: 0; top: 0; - overflow-x: hidden; - overflow-y: scroll; -} -.CodeMirror-hscrollbar { - bottom: 0; left: 0; - overflow-y: hidden; - overflow-x: scroll; -} -.CodeMirror-scrollbar-filler { - right: 0; bottom: 0; -} -.CodeMirror-gutter-filler { - left: 0; bottom: 0; -} - -.CodeMirror-gutters { - position: absolute; left: 0; top: 0; - padding-bottom: 30px; - z-index: 3; -} -.CodeMirror-gutter { - white-space: normal; - height: 100%; - -moz-box-sizing: content-box; - box-sizing: content-box; - padding-bottom: 30px; - margin-bottom: -32px; - display: inline-block; - /* Hack to make IE7 behave */ - *zoom:1; - *display:inline; -} -.CodeMirror-gutter-elt { - position: absolute; - cursor: default; - z-index: 4; -} - -.CodeMirror-lines { - cursor: text; -} -.CodeMirror pre { - /* Reset some styles that the rest of the page might have set */ - -moz-border-radius: 0; -webkit-border-radius: 0; border-radius: 0; - border-width: 0; - background: transparent; - font-family: inherit; - font-size: inherit; - margin: 0; - white-space: pre; - word-wrap: normal; - line-height: inherit; - color: inherit; - z-index: 2; - position: relative; - overflow: visible; -} -.CodeMirror-wrap pre { - word-wrap: break-word; - white-space: pre-wrap; - word-break: normal; -} -.CodeMirror-code pre { - border-right: 30px solid transparent; - width: -webkit-fit-content; - width: -moz-fit-content; - width: fit-content; -} -.CodeMirror-wrap .CodeMirror-code pre { - border-right: none; - width: auto; -} -.CodeMirror-linebackground { - position: absolute; - left: 0; right: 0; top: 0; bottom: 0; - z-index: 0; -} - -.CodeMirror-linewidget { - position: relative; - z-index: 2; - overflow: auto; -} - -.CodeMirror-widget {} - -.CodeMirror-wrap .CodeMirror-scroll { - overflow-x: hidden; -} - -.CodeMirror-measure { - position: absolute; - width: 100%; - height: 0; - overflow: hidden; - visibility: hidden; -} -.CodeMirror-measure pre { position: static; } - -.CodeMirror div.CodeMirror-cursor { - position: absolute; - visibility: hidden; - border-right: none; - width: 0; -} -.CodeMirror-focused div.CodeMirror-cursor { - visibility: visible; -} - -.CodeMirror-selected { background: #d9d9d9; } -.CodeMirror-focused .CodeMirror-selected { background: #d7d4f0; } - -.cm-searching { - background: #ffa; - background: rgba(255, 255, 0, .4); -} - -/* IE7 hack to prevent it from returning funny offsetTops on the spans */ -.CodeMirror span { *vertical-align: text-bottom; } - -@media print { - /* Hide the cursor when printing */ - .CodeMirror div.CodeMirror-cursor { - visibility: hidden; - } -} diff --git a/common/static/js/vendor/CodeMirror/codemirror.css b/common/static/js/vendor/CodeMirror/codemirror.css index 2d79f4aa79..23eaf74d44 100644 --- a/common/static/js/vendor/CodeMirror/codemirror.css +++ b/common/static/js/vendor/CodeMirror/codemirror.css @@ -1,112 +1,263 @@ +/* BASICS */ + .CodeMirror { - line-height: 1em; + /* Set height, width, borders, and global font properties here */ font-family: monospace; + height: 300px; +} +.CodeMirror-scroll { + /* Set scrolling behaviour here */ + overflow: auto; +} + +/* PADDING */ + +.CodeMirror-lines { + padding: 4px 0; /* Vertical padding around content */ +} +.CodeMirror pre { + padding: 0 4px; /* Horizontal padding of content */ +} + +.CodeMirror-scrollbar-filler, .CodeMirror-gutter-filler { + background-color: white; /* The little square between H and V scrollbars */ +} + +/* GUTTER */ + +.CodeMirror-gutters { + border-right: 1px solid #ddd; + background-color: #f7f7f7; + white-space: nowrap; +} +.CodeMirror-linenumbers {} +.CodeMirror-linenumber { + padding: 0 3px 0 5px; + min-width: 20px; + text-align: right; + color: #999; +} + +/* CURSOR */ + +.CodeMirror div.CodeMirror-cursor { + border-left: 1px solid black; + z-index: 3; +} +/* Shown when moving in bi-directional text */ +.CodeMirror div.CodeMirror-secondarycursor { + border-left: 1px solid silver; +} +.CodeMirror.cm-keymap-fat-cursor div.CodeMirror-cursor { + width: auto; + border: 0; + background: #7e7; + z-index: 1; +} +/* Can style cursor different in overwrite (non-insert) mode */ +.CodeMirror div.CodeMirror-cursor.CodeMirror-overwrite {} + +.cm-tab { display: inline-block; } + +/* DEFAULT THEME */ + +.cm-s-default .cm-keyword {color: #708;} +.cm-s-default .cm-atom {color: #219;} +.cm-s-default .cm-number {color: #164;} +.cm-s-default .cm-def {color: #00f;} +.cm-s-default .cm-variable {color: black;} +.cm-s-default .cm-variable-2 {color: #05a;} +.cm-s-default .cm-variable-3 {color: #085;} +.cm-s-default .cm-property {color: black;} +.cm-s-default .cm-operator {color: black;} +.cm-s-default .cm-comment {color: #a50;} +.cm-s-default .cm-string {color: #a11;} +.cm-s-default .cm-string-2 {color: #f50;} +.cm-s-default .cm-meta {color: #555;} +.cm-s-default .cm-qualifier {color: #555;} +.cm-s-default .cm-builtin {color: #30a;} +.cm-s-default .cm-bracket {color: #997;} +.cm-s-default .cm-tag {color: #170;} +.cm-s-default .cm-attribute {color: #00c;} +.cm-s-default .cm-header {color: blue;} +.cm-s-default .cm-quote {color: #090;} +.cm-s-default .cm-hr {color: #999;} +.cm-s-default .cm-link {color: #00c;} + +.cm-negative {color: #d44;} +.cm-positive {color: #292;} +.cm-header, .cm-strong {font-weight: bold;} +.cm-em {font-style: italic;} +.cm-link {text-decoration: underline;} + +.cm-s-default .cm-error {color: #f00;} +.cm-invalidchar {color: #f00;} + +div.CodeMirror span.CodeMirror-matchingbracket {color: #0f0;} +div.CodeMirror span.CodeMirror-nonmatchingbracket {color: #f22;} +.CodeMirror-activeline-background {background: #e8f2ff;} + +/* STOP */ + +/* The rest of this file contains styles related to the mechanics of + the editor. You probably shouldn't touch them. */ + +.CodeMirror { + line-height: 1; + position: relative; + overflow: hidden; + background: white; + color: black; } .CodeMirror-scroll { - overflow: auto; - height: 300px; - /* This is needed to prevent an IE[67] bug where the scrolled content - is visible outside of the scrolling box. */ - position: relative; - outline: none; -} - -.CodeMirror-gutter { - position: absolute; left: 0; top: 0; - z-index: 10; - background-color: #f7f7f7; - border-right: 1px solid #eee; - min-width: 2em; + /* 30px is the magic margin used to hide the element's real scrollbars */ + /* See overflow: hidden in .CodeMirror */ + margin-bottom: -30px; margin-right: -30px; + padding-bottom: 30px; padding-right: 30px; height: 100%; + outline: none; /* Prevent dragging from highlighting the element */ + position: relative; + -moz-box-sizing: content-box; + box-sizing: content-box; } -.CodeMirror-gutter-text { - color: #aaa; - text-align: right; - padding: .4em .2em .4em .4em; - white-space: pre !important; -} -.CodeMirror-lines { - padding: .4em; - white-space: pre; +.CodeMirror-sizer { + position: relative; } +/* The fake, visible scrollbars. Used to force redraw during scrolling + before actuall scrolling happens, thus preventing shaking and + flickering artifacts. */ +.CodeMirror-vscrollbar, .CodeMirror-hscrollbar, .CodeMirror-scrollbar-filler, .CodeMirror-gutter-filler { + position: absolute; + z-index: 6; + display: none; +} +.CodeMirror-vscrollbar { + right: 0; top: 0; + overflow-x: hidden; + overflow-y: scroll; +} +.CodeMirror-hscrollbar { + bottom: 0; left: 0; + overflow-y: hidden; + overflow-x: scroll; +} +.CodeMirror-scrollbar-filler { + right: 0; bottom: 0; +} +.CodeMirror-gutter-filler { + left: 0; bottom: 0; +} + +.CodeMirror-gutters { + position: absolute; left: 0; top: 0; + padding-bottom: 30px; + z-index: 3; +} +.CodeMirror-gutter { + white-space: normal; + height: 100%; + -moz-box-sizing: content-box; + box-sizing: content-box; + padding-bottom: 30px; + margin-bottom: -32px; + display: inline-block; + /* Hack to make IE7 behave */ + *zoom:1; + *display:inline; +} +.CodeMirror-gutter-elt { + position: absolute; + cursor: default; + z-index: 4; +} + +.CodeMirror-lines { + cursor: text; +} .CodeMirror pre { - -moz-border-radius: 0; - -webkit-border-radius: 0; - -o-border-radius: 0; - border-radius: 0; - border-width: 0; margin: 0; padding: 0; background: transparent; + /* Reset some styles that the rest of the page might have set */ + -moz-border-radius: 0; -webkit-border-radius: 0; border-radius: 0; + border-width: 0; + background: transparent; font-family: inherit; font-size: inherit; - padding: 0; margin: 0; + margin: 0; white-space: pre; word-wrap: normal; + line-height: inherit; + color: inherit; + z-index: 2; + position: relative; + overflow: visible; } - .CodeMirror-wrap pre { word-wrap: break-word; white-space: pre-wrap; + word-break: normal; } +.CodeMirror-code pre { + border-right: 30px solid transparent; + width: -webkit-fit-content; + width: -moz-fit-content; + width: fit-content; +} +.CodeMirror-wrap .CodeMirror-code pre { + border-right: none; + width: auto; +} +.CodeMirror-linebackground { + position: absolute; + left: 0; right: 0; top: 0; bottom: 0; + z-index: 0; +} + +.CodeMirror-linewidget { + position: relative; + z-index: 2; + overflow: auto; +} + +.CodeMirror-widget {} + .CodeMirror-wrap .CodeMirror-scroll { overflow-x: hidden; } -.CodeMirror textarea { - outline: none !important; +.CodeMirror-measure { + position: absolute; + width: 100%; + height: 0; + overflow: hidden; + visibility: hidden; } +.CodeMirror-measure pre { position: static; } -.CodeMirror pre.CodeMirror-cursor { - z-index: 10; +.CodeMirror div.CodeMirror-cursor { position: absolute; visibility: hidden; - border-left: 1px solid black; - border-right:none; - width:0; + border-right: none; + width: 0; } -.CodeMirror pre.CodeMirror-cursor.CodeMirror-overwrite {} -.CodeMirror-focused pre.CodeMirror-cursor { +.CodeMirror-focused div.CodeMirror-cursor { visibility: visible; } -div.CodeMirror-selected { background: #d9d9d9; } -.CodeMirror-focused div.CodeMirror-selected { background: #d7d4f0; } +.CodeMirror-selected { background: #d9d9d9; } +.CodeMirror-focused .CodeMirror-selected { background: #d7d4f0; } -.CodeMirror-searching { +.cm-searching { background: #ffa; background: rgba(255, 255, 0, .4); } -/* Default theme */ +/* IE7 hack to prevent it from returning funny offsetTops on the spans */ +.CodeMirror span { *vertical-align: text-bottom; } -.cm-s-default span.cm-keyword {color: #708;} -.cm-s-default span.cm-atom {color: #219;} -.cm-s-default span.cm-number {color: #164;} -.cm-s-default span.cm-def {color: #00f;} -.cm-s-default span.cm-variable {color: black;} -.cm-s-default span.cm-variable-2 {color: #05a;} -.cm-s-default span.cm-variable-3 {color: #085;} -.cm-s-default span.cm-property {color: black;} -.cm-s-default span.cm-operator {color: black;} -.cm-s-default span.cm-comment {color: #a50;} -.cm-s-default span.cm-string {color: #a11;} -.cm-s-default span.cm-string-2 {color: #f50;} -.cm-s-default span.cm-meta {color: #555;} -.cm-s-default span.cm-error {color: #f00;} -.cm-s-default span.cm-qualifier {color: #555;} -.cm-s-default span.cm-builtin {color: #30a;} -.cm-s-default span.cm-bracket {color: #cc7;} -.cm-s-default span.cm-tag {color: #170;} -.cm-s-default span.cm-attribute {color: #00c;} -.cm-s-default span.cm-header {color: #a0a;} -.cm-s-default span.cm-quote {color: #090;} -.cm-s-default span.cm-hr {color: #999;} -.cm-s-default span.cm-link {color: #00c;} - -span.cm-header, span.cm-strong {font-weight: bold;} -span.cm-em {font-style: italic;} -span.cm-emstrong {font-style: italic; font-weight: bold;} -span.cm-link {text-decoration: underline;} - -div.CodeMirror span.CodeMirror-matchingbracket {color: #0f0;} -div.CodeMirror span.CodeMirror-nonmatchingbracket {color: #f22;} +@media print { + /* Hide the cursor when printing */ + .CodeMirror div.CodeMirror-cursor { + visibility: hidden; + } +} diff --git a/lms/djangoapps/course_wiki/editors.py b/lms/djangoapps/course_wiki/editors.py index d5d0d2c3f3..129457cc35 100644 --- a/lms/djangoapps/course_wiki/editors.py +++ b/lms/djangoapps/course_wiki/editors.py @@ -56,8 +56,8 @@ class CodeMirror(BaseEditor): 'all': ("js/vendor/CodeMirror/codemirror.css",) } js = ("js/vendor/CodeMirror/codemirror.js", - "js/vendor/CodeMirror/xml.js", - "js/vendor/CodeMirror/edx_markdown.js", + "js/vendor/CodeMirror/addons/xml.js", + "js/vendor/CodeMirror/addons/edx_markdown.js", "js/wiki/accessible.js", "js/wiki/CodeMirror.init.js", ) From e3a70ce6df4936ff425699e33d605895244915d7 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Tue, 11 Mar 2014 10:00:42 -0400 Subject: [PATCH 2/8] Fix for CourseFactory bug - course object not saved after setattr called --- common/lib/xmodule/xmodule/modulestore/tests/factories.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 6fe138e05c..8f4cf1ffa8 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -56,6 +56,8 @@ class CourseFactory(XModuleFactory): for k, v in kwargs.iteritems(): setattr(new_course, k, v) + # Save the attributes we just set + new_course.save() # Update the data in the mongo datastore store.update_item(new_course) return new_course @@ -156,6 +158,7 @@ class ItemFactory(XModuleFactory): for attr, val in kwargs.items(): setattr(module, attr, val) + # Save the attributes we just set module.save() store.update_item(module) From 119ffd4677ffc66b51cf9cc2a9a8e021bfce2701 Mon Sep 17 00:00:00 2001 From: Sarina Canelake Date: Tue, 11 Mar 2014 10:03:33 -0400 Subject: [PATCH 3/8] Bugfix for split test on sequential save Bug would occur when comparing Groups or UserPartitions - because these types lacked comparator methods, different objects would come up as different when their values were equal. --- .../xmodule/xmodule/partitions/partitions.py | 23 +++++---- .../courseware/tests/test_split_module.py | 49 +++++++++++++++++++ 2 files changed, 60 insertions(+), 12 deletions(-) diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index e5d78b79e8..cd6b7cbcfa 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -1,11 +1,11 @@ """Defines ``Group`` and ``UserPartition`` models for partitioning""" - +from collections import namedtuple # We use ``id`` in this file as the IDs of our Groups and UserPartitions, # which Pylint disapproves of. # pylint: disable=invalid-name, redefined-builtin -class Group(object): +class Group(namedtuple("Group", "id name")): """ An id and name for a group of students. The id should be unique within the UserPartition this group appears in. @@ -14,9 +14,9 @@ class Group(object): # for deserializing old versions. (This will be serialized in courses) VERSION = 1 - def __init__(self, id, name): - self.id = int(id) - self.name = name + def __new__(cls, id, name): + # pylint: disable=super-on-old-class + return super(Group, cls).__new__(cls, int(id), name) def to_json(self): """ @@ -25,6 +25,7 @@ class Group(object): Returns: a dictionary with keys for the properties of the group. """ + # pylint: disable=no-member return { "id": self.id, "name": self.name, @@ -53,7 +54,7 @@ class Group(object): return Group(value["id"], value["name"]) -class UserPartition(object): +class UserPartition(namedtuple("UserPartition", "id name description groups")): """ A named way to partition users into groups, primarily intended for running experiments. It is expected that each user will be in at most one group in a @@ -65,12 +66,9 @@ class UserPartition(object): """ VERSION = 1 - def __init__(self, id, name, description, groups): - - self.id = int(id) - self.name = name - self.description = description - self.groups = groups + def __new__(cls, id, name, description, groups): + # pylint: disable=super-on-old-class + return super(UserPartition, cls).__new__(cls, int(id), name, description, groups) def to_json(self): """ @@ -79,6 +77,7 @@ class UserPartition(object): Returns: a dictionary with keys for the properties of the partition. """ + # pylint: disable=no-member return { "id": self.id, "name": self.name, diff --git a/lms/djangoapps/courseware/tests/test_split_module.py b/lms/djangoapps/courseware/tests/test_split_module.py index 958712476e..6c7731fa23 100644 --- a/lms/djangoapps/courseware/tests/test_split_module.py +++ b/lms/djangoapps/courseware/tests/test_split_module.py @@ -3,9 +3,12 @@ Test for split test XModule """ from django.core.urlresolvers import reverse from django.test.utils import override_settings +from mock import MagicMock from student.tests.factories import UserFactory, CourseEnrollmentFactory from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE +from courseware.module_render import get_module_for_descriptor +from courseware.model_data import FieldDataCache from xmodule.modulestore.tests.factories import ItemFactory, CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase @@ -267,3 +270,49 @@ class TestSplitTestVert(SplitTestBase): ) video1 = self._video(cond1vert, 1) html1 = self._html(cond1vert, 1) + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class SplitTestPosition(ModuleStoreTestCase): + """ + Check that we can change positions in a course with partitions defined + """ + def setUp(self): + self.partition = UserPartition( + 0, + 'first_partition', + 'First Partition', + [ + Group(0, 'alpha'), + Group(1, 'beta') + ] + ) + + self.course = CourseFactory.create( + user_partitions=[self.partition] + ) + + self.chapter = ItemFactory.create( + parent_location=self.course.location, + category="chapter", + display_name="test chapter", + ) + + self.student = UserFactory.create() + CourseEnrollmentFactory.create(user=self.student, course_id=self.course.id) + self.client.login(username=self.student.username, password='test') + + def test_changing_position_works(self): + # Make a mock FieldDataCache for this course, so we can get the course module + mock_field_data_cache = FieldDataCache([self.course], self.course.id, self.student) + course = get_module_for_descriptor( + self.student, + MagicMock(name='request'), + self.course, + mock_field_data_cache, + self.course.id + ) + + # Now that we have the course, change the position and save, nothing should explode! + course.position = 2 + course.save() From cb2243a495930dda0771531f657b366c505a9998 Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Tue, 11 Mar 2014 20:45:14 +0500 Subject: [PATCH 4/8] Do not remap the wiki_slug if reimporting into the same course. LMS-2136 --- .../contentstore/tests/test_contentstore.py | 25 ++++++++++++++++--- .../xmodule/modulestore/xml_importer.py | 25 ++++++++++--------- 2 files changed, 35 insertions(+), 15 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 8c321f03dd..d52330f5ff 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1763,6 +1763,26 @@ class ContentStoreTest(ModuleStoreTestCase): def test_import_into_new_course_id_wiki_slug_renamespacing(self): module_store = modulestore('direct') + + # If reimporting into the same course do not change the wiki_slug. + target_location = Location('i4x', 'edX', 'toy', 'course', '2012_Fall') + course_data = { + 'org': target_location.org, + 'number': target_location.course, + 'display_name': 'Robot Super Course', + 'run': target_location.name + } + _create_course(self, course_data) + course_module = module_store.get_instance(target_location.course_id, target_location) + course_module.wiki_slug = 'toy' + course_module.save() + + # Import a course with wiki_slug == location.course + import_from_xml(module_store, 'common/test/data/', ['toy'], target_location_namespace=target_location) + course_module = module_store.get_instance(target_location.course_id, target_location) + self.assertEquals(course_module.wiki_slug, 'toy') + + # But change the wiki_slug if it is a different course. target_location = Location('i4x', 'MITx', '999', 'course', '2013_Spring') course_data = { 'org': target_location.org, @@ -1770,7 +1790,6 @@ class ContentStoreTest(ModuleStoreTestCase): 'display_name': 'Robot Super Course', 'run': target_location.name } - target_course_id = '{0}/{1}/{2}'.format(target_location.org, target_location.course, target_location.name) _create_course(self, course_data) # Import a course with wiki_slug == location.course @@ -1778,9 +1797,9 @@ class ContentStoreTest(ModuleStoreTestCase): course_module = module_store.get_instance(target_location.course_id, target_location) self.assertEquals(course_module.wiki_slug, 'MITx.999.2013_Spring') - # Now try importing a course with wiki_slug == '{0}{1}{2}'.format(location.org, location.course, location.name) + # Now try importing a course with wiki_slug == '{0}.{1}.{2}'.format(location.org, location.course, location.name) import_from_xml(module_store, 'common/test/data/', ['two_toys'], target_location_namespace=target_location) - course_module = module_store.get_instance(target_course_id, target_location) + course_module = module_store.get_instance(target_location.course_id, target_location) self.assertEquals(course_module.wiki_slug, 'MITx.999.2013_Spring') def test_import_metadata_with_attempts_empty_string(self): diff --git a/common/lib/xmodule/xmodule/modulestore/xml_importer.py b/common/lib/xmodule/xmodule/modulestore/xml_importer.py index cd82f97aec..d398f5d931 100644 --- a/common/lib/xmodule/xmodule/modulestore/xml_importer.py +++ b/common/lib/xmodule/xmodule/modulestore/xml_importer.py @@ -515,19 +515,20 @@ def remap_namespace(module, target_location_namespace): ) # Original wiki_slugs had value location.course. To make them unique this was changed to 'org.course.name'. - # If the wiki_slug is equal to either of these default values then remap that so that the wiki does not point - # to the old wiki. - original_unique_wiki_slug = '{0}.{1}.{2}'.format( - original_location.org, - original_location.course, - original_location.name - ) - if module.wiki_slug == original_unique_wiki_slug or module.wiki_slug == original_location.course: - module.wiki_slug = '{0}.{1}.{2}'.format( - target_location_namespace.org, - target_location_namespace.course, - target_location_namespace.name, + # If we are importing into a course with a different course_id and wiki_slug is equal to either of these default + # values then remap it so that the wiki does not point to the old wiki. + if original_location.course_id != target_location_namespace.course_id: + original_unique_wiki_slug = '{0}.{1}.{2}'.format( + original_location.org, + original_location.course, + original_location.name ) + if module.wiki_slug == original_unique_wiki_slug or module.wiki_slug == original_location.course: + module.wiki_slug = '{0}.{1}.{2}'.format( + target_location_namespace.org, + target_location_namespace.course, + target_location_namespace.name, + ) module.save() From 2f2ce555c1b5e8ec562860cfd249f1f5c6c7fadc Mon Sep 17 00:00:00 2001 From: marco Date: Mon, 10 Mar 2014 16:22:32 -0400 Subject: [PATCH 5/8] adjusted visual styling for editing state of xblocks, html, and video components which had extra padding applied to it from the container page xblock styling STUD-1410 --- cms/static/sass/views/_unit.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cms/static/sass/views/_unit.scss b/cms/static/sass/views/_unit.scss index 61b1428b33..0a315e6799 100644 --- a/cms/static/sass/views/_unit.scss +++ b/cms/static/sass/views/_unit.scss @@ -1403,3 +1403,9 @@ body.unit .component { margin-top: ($baseline*1.5); } } + +body.unit .component.editing { + .xmodule_DiscussionModule, .xmodule_HtmlModule, .xblock { + margin-top: 0; + } +} From e117957e2ac5e1693fc97beeb6004da749588d3c Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Mon, 10 Mar 2014 19:22:57 +0500 Subject: [PATCH 6/8] Fixed module urls creation in legacy instructor dashboard. LMS-2327 --- lms/djangoapps/instructor/views/legacy.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/legacy.py b/lms/djangoapps/instructor/views/legacy.py index ba013b1067..12b0d6f9f7 100644 --- a/lms/djangoapps/instructor/views/legacy.py +++ b/lms/djangoapps/instructor/views/legacy.py @@ -173,7 +173,7 @@ def instructor_dashboard(request, course_id): # complete the url using information about the current course: parts = Location.parse_course_id(course_id) parts['url'] = urlname - return u"i4x://{org}/{name}/{url}".format(**parts) + return u"i4x://{org}/{course}/{url}".format(**parts) def get_student_from_identifier(unique_student_identifier): """Gets a student object using either an email address or username""" From ddf41ce9c54599efffa8f4fa54534971f0fde883 Mon Sep 17 00:00:00 2001 From: polesye Date: Tue, 11 Mar 2014 17:38:29 +0200 Subject: [PATCH 7/8] BLD-915: Fix issues with different videoIDs. --- CHANGELOG.rst | 3 + common/djangoapps/terrain/start_stubs.py | 6 +- common/djangoapps/terrain/stubs/youtube.py | 3 +- .../xmodule/js/spec/video/initialize_spec.js | 53 ++++++-- .../js/spec/video/video_caption_spec.js | 41 ++++-- .../js/spec/video/video_player_spec.js | 9 +- .../xmodule/js/src/video/01_initialize.js | 13 +- .../xmodule/js/src/video/03_video_player.js | 121 +++++++++--------- .../js/src/video/06_video_progress_slider.js | 7 +- .../xmodule/js/src/video/09_video_caption.js | 6 +- .../data/uploads/subs_b7xgknqkQk8.srt.sjson | 11 ++ .../courseware/features/video.feature | 22 +++- lms/djangoapps/courseware/features/video.py | 83 +++++++++--- 13 files changed, 256 insertions(+), 122 deletions(-) create mode 100644 common/test/data/uploads/subs_b7xgknqkQk8.srt.sjson diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c768388866..7c0b652204 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Blades: Fix issues related to videos that have separate YouTube IDs for the +different video speeds. BLD-915, BLD-901. + Blades: Add .txt and .srt options to the "download transcript" button. BLD-844. Blades: Fix bug when transcript cutting off view in full view mode. BLD-852. diff --git a/common/djangoapps/terrain/start_stubs.py b/common/djangoapps/terrain/start_stubs.py index ad9ffbbce3..3e663364e7 100644 --- a/common/djangoapps/terrain/start_stubs.py +++ b/common/djangoapps/terrain/start_stubs.py @@ -15,8 +15,8 @@ SERVICES = { } -@before.all -def start_stubs(): +@before.each_scenario +def start_stubs(_): """ Start each stub service running on a local port. """ @@ -25,7 +25,7 @@ def start_stubs(): setattr(world, name, fake_server) -@after.all +@after.each_scenario def stop_stubs(_): """ Shut down each stub service. diff --git a/common/djangoapps/terrain/stubs/youtube.py b/common/djangoapps/terrain/stubs/youtube.py index 9b16e830df..64841dd5c4 100644 --- a/common/djangoapps/terrain/stubs/youtube.py +++ b/common/djangoapps/terrain/stubs/youtube.py @@ -5,7 +5,6 @@ Stub implementation of YouTube for acceptance tests. from .http import StubHttpRequestHandler, StubHttpService import json import time -import requests from urlparse import urlparse from collections import OrderedDict @@ -80,7 +79,7 @@ class StubYouTubeHandler(StubHttpRequestHandler): 'data': OrderedDict({ 'id': youtube_id, 'message': message, - 'duration': 60, + 'duration': 60 if youtube_id == 'OEoXaMPEzfM' else 77, }) }) response = "{cb}({data})".format(cb=callback, data=json.dumps(data)) diff --git a/common/lib/xmodule/xmodule/js/spec/video/initialize_spec.js b/common/lib/xmodule/xmodule/js/spec/video/initialize_spec.js index 020de1f384..31ffe79a11 100644 --- a/common/lib/xmodule/xmodule/js/spec/video/initialize_spec.js +++ b/common/lib/xmodule/xmodule/js/spec/video/initialize_spec.js @@ -233,25 +233,42 @@ function (Initialize) { '1.0': 'testId', '1.50': 'videoId' }, - youtubeId: Initialize.prototype.youtubeId + youtubeId: Initialize.prototype.youtubeId, + isFlashMode: jasmine.createSpy().andReturn(false) }; }); - it('returns duration for current video', function () { - var expected = Initialize.prototype.getDuration.call(state); - - expect(expected).toEqual(100); - }); - - var msg = 'returns duration for the 1.0 speed as a fallback'; + var msg = 'returns duration for the 1.0 speed if speed is not 1.0'; it(msg, function () { var expected; - state.speed = '2.0'; + state.speed = '1.50'; expected = Initialize.prototype.getDuration.call(state); expect(expected).toEqual(400); }); + + describe('Flash mode', function () { + it('returns duration for current video', function () { + var expected; + + state.isFlashMode.andReturn(true); + expected = Initialize.prototype.getDuration.call(state); + + expect(expected).toEqual(100); + }); + + var msg = 'returns duration for the 1.0 speed as a fall-back'; + it(msg, function () { + var expected; + + state.isFlashMode.andReturn(true); + state.speed = '2.0'; + expected = Initialize.prototype.getDuration.call(state); + + expect(expected).toEqual(400); + }); + }); }); describe('youtubeId', function () { @@ -262,7 +279,8 @@ function (Initialize) { '0.50': '7tqY6eQzVhE', '1.0': 'cogebirgzzM', '1.50': 'abcdefghijkl' - } + }, + isFlashMode: jasmine.createSpy().andReturn(false) }; }); @@ -278,14 +296,25 @@ function (Initialize) { }); }); - describe('without speed', function () { + describe('without speed for flash mode', function () { it('return the video id for current speed', function () { - var expected = Initialize.prototype.youtubeId.call(state); + var expected; + + state.isFlashMode.andReturn(true); + expected = Initialize.prototype.youtubeId.call(state); expect(expected).toEqual('abcdefghijkl'); }); }); + describe('without speed for youtube html5 mode', function () { + it('return the video id for 1.0x speed', function () { + var expected = Initialize.prototype.youtubeId.call(state); + + expect(expected).toEqual('cogebirgzzM'); + }); + }); + describe('speed is absent in the list of video speeds', function () { it('return the video id for 1.0x speed', function () { var expected = Initialize.prototype.youtubeId.call(state, '0.0'); 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 f57cde4d1c..b1d2b7b2b5 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 @@ -55,11 +55,7 @@ }); waitsFor(function () { - if (state.videoCaption.loaded === true) { - return true; - } - - return false; + return state.videoCaption.loaded; }, 'Expect captions to be loaded.', WAIT_TIMEOUT); runs(function () { @@ -77,17 +73,15 @@ }); }); - it('fetch the caption in Youtube mode', function () { + it('fetch the caption in Flash mode', function () { runs(function () { state = jasmine.initializePlayerYouTube(); + spyOn(state, 'isFlashMode').andReturn(true); + state.videoCaption.fetchCaption(); }); waitsFor(function () { - if (state.videoCaption.loaded === true) { - return true; - } - - return false; + return state.videoCaption.loaded; }, 'Expect captions to be loaded.', WAIT_TIMEOUT); runs(function () { @@ -106,6 +100,31 @@ }); }); + it('fetch the caption in Youtube mode', function () { + runs(function () { + state = jasmine.initializePlayerYouTube(); + }); + + waitsFor(function () { + return state.videoCaption.loaded; + }, 'Expect captions to be loaded.', WAIT_TIMEOUT); + + runs(function () { + expect($.ajaxWithPrefix).toHaveBeenCalledWith({ + url: '/transcript/translation', + notifyOnError: false, + data: jasmine.any(Object), + success: jasmine.any(Function), + error: jasmine.any(Function) + }); + expect($.ajaxWithPrefix.mostRecentCall.args[0].data) + .toEqual({ + language: 'en', + videoId: 'cogebirgzzM' + }); + }); + }); + it('bind the hide caption button', function () { state = jasmine.initializePlayer(); expect($('.hide-subtitles')).toHandleWith( 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 51b1bd5287..c25abc34c1 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 @@ -105,6 +105,7 @@ function (VideoPlayer) { it('create Flash player', function () { var player; + spyOn($.fn, 'trigger'); state = jasmine.initializePlayerYouTube(); state.videoEl = state.el.find('video, iframe').width(100); player = state.videoPlayer.player; @@ -715,7 +716,8 @@ function (VideoPlayer) { }, currentPlayerMode: 'html5', trigger: function () {}, - browserIsFirefox: false + browserIsFirefox: false, + isFlashMode: jasmine.createSpy().andReturn(false) }; }); @@ -1015,7 +1017,8 @@ function (VideoPlayer) { updatePlayTime: jasmine.createSpy(), setPlaybackRate: jasmine.createSpy(), player: jasmine.createSpyObj('player', ['setPlaybackRate']) - } + }, + isFlashMode: jasmine.createSpy().andReturn(false) }; }); @@ -1033,7 +1036,7 @@ function (VideoPlayer) { }); it('convert the current time to the new speed', function () { - state.currentPlayerMode = 'flash'; + state.isFlashMode.andReturn(true); VideoPlayer.prototype.onSpeedChange.call(state, '0.75', false); expect(state.videoPlayer.currentTime).toBe('120.000'); }); diff --git a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js index 8be4bf9e06..cddd0d4aa1 100644 --- a/common/lib/xmodule/xmodule/js/src/video/01_initialize.js +++ b/common/lib/xmodule/xmodule/js/src/video/01_initialize.js @@ -65,6 +65,7 @@ function (VideoPlayer, VideoStorage) { getDuration: getDuration, getVideoMetadata: getVideoMetadata, initialize: initialize, + isFlashMode: isFlashMode, parseSpeed: parseSpeed, parseVideoSources: parseVideoSources, parseYoutubeStreams: parseYoutubeStreams, @@ -550,7 +551,7 @@ function (VideoPlayer, VideoStorage) { _this.videos[speed] = video[1]; }); - return true; + return _.isString(this.videos['1.0']); } // function parseVideoSources(, mp4Source, webmSource, oggSource) @@ -702,7 +703,11 @@ function (VideoPlayer, VideoStorage) { } function youtubeId(speed) { - return this.videos[speed || this.speed] || this.videos['1.0']; + var currentSpeed = this.isFlashMode() ? this.speed : '1.0'; + + return this.videos[speed] || + this.videos[currentSpeed] || + this.videos['1.0']; } function getDuration() { @@ -713,6 +718,10 @@ function (VideoPlayer, VideoStorage) { } } + function isFlashMode() { + return this.currentPlayerMode === 'flash'; + } + function getCurrentLanguage() { var keys = _.keys(this.config.transcriptLanguages); diff --git a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js index 0a5acd87da..d16f9b73b3 100644 --- a/common/lib/xmodule/xmodule/js/src/video/03_video_player.js +++ b/common/lib/xmodule/xmodule/js/src/video/03_video_player.js @@ -73,7 +73,7 @@ function (HTML5Video, Resizer) { state.videoPlayer.ready = _.once(function () { $(window).on('unload', state.saveState); - if (state.currentPlayerMode !== 'flash') { + if (!state.isFlashMode()) { state.videoPlayer.setPlaybackRate(state.speed); } state.videoPlayer.player.setVolume(state.currentVolume); @@ -100,7 +100,7 @@ function (HTML5Video, Resizer) { modestbranding: 1 }; - if (state.currentPlayerMode !== 'flash') { + if (!state.isFlashMode()) { state.videoPlayer.playerVars.html5 = 1; } @@ -140,11 +140,8 @@ function (HTML5Video, Resizer) { }, false); } else { // if (state.videoType === 'youtube') { - if (state.currentPlayerMode === 'flash') { - youTubeId = state.youtubeId(); - } else { - youTubeId = state.youtubeId('1.0'); - } + youTubeId = state.youtubeId(); + state.videoPlayer.player = new YT.Player(state.id, { playerVars: state.videoPlayer.playerVars, videoId: youTubeId, @@ -162,22 +159,7 @@ function (HTML5Video, Resizer) { videoHeight = player.attr('height') || player.height(); _resize(state, videoWidth, videoHeight); - - // After initialization, update the VCR with total time. - // At this point only the metadata duration is available (not - // very precise), but it is better than having 00:00:00 for - // total time. - if (state.youtubeMetadataReceived) { - // Metadata was already received, and is available. - _updateVcrAndRegion(state); - } else { - // We wait for metadata to arrive, before we request the update - // of the VCR video time, and of the start-end time region. - // Metadata contains duration of the video. - state.el.on('metadata_received', function () { - _updateVcrAndRegion(state); - }); - } + _updateVcrAndRegion(state, true); }); } @@ -185,36 +167,53 @@ function (HTML5Video, Resizer) { dfd.resolve(); } } + function _updateVcrAndRegion(state, isYoutube) { + var update = function (state) { + var duration = state.videoPlayer.duration(), + time; - function _updateVcrAndRegion(state) { - var duration = state.videoPlayer.duration(), - time; + time = state.videoPlayer.figureOutStartingTime(duration); - time = state.videoPlayer.figureOutStartingTime(duration); + // Update the VCR. + state.trigger( + 'videoControl.updateVcrVidTime', + { + time: time, + duration: duration + } + ); - // Update the VCR. - state.trigger( - 'videoControl.updateVcrVidTime', - { - time: time, - duration: duration - } - ); + // Update the time slider. + state.trigger( + 'videoProgressSlider.updateStartEndTimeRegion', + { + duration: duration + } + ); + state.trigger( + 'videoProgressSlider.updatePlayTime', + { + time: time, + duration: duration + } + ); + }; - // Update the time slider. - state.trigger( - 'videoProgressSlider.updateStartEndTimeRegion', - { - duration: duration - } - ); - state.trigger( - 'videoProgressSlider.updatePlayTime', - { - time: time, - duration: duration - } - ); + // After initialization, update the VCR with total time. + // At this point only the metadata duration is available (not + // very precise), but it is better than having 00:00:00 for + // total time. + if (state.youtubeMetadataReceived || !isYoutube) { + // Metadata was already received, and is available. + update(state); + } else { + // We wait for metadata to arrive, before we request the update + // of the VCR video time, and of the start-end time region. + // Metadata contains duration of the video. + state.el.on('metadata_received', function () { + update(state); + }); + } } function _resize(state, videoWidth, videoHeight) { @@ -271,6 +270,8 @@ function (HTML5Video, Resizer) { } }); + _updateVcrAndRegion(state, true); + state.trigger('videoCaption.fetchCaption', null); state.resizer.setElement(state.el.find('iframe')).align(); } @@ -348,7 +349,7 @@ function (HTML5Video, Resizer) { // where in Firefox speed switching to 1.0 in HTML5 player mode is // handled incorrectly by YouTube API. methodName = 'cueVideoById'; - youtubeId = this.youtubeId(); + youtubeId = this.youtubeId(newSpeed); if (this.videoPlayer.isPlaying()) { methodName = 'loadVideoById'; @@ -360,10 +361,9 @@ function (HTML5Video, Resizer) { } function onSpeedChange(newSpeed) { - var time = this.videoPlayer.currentTime, - isFlash = this.currentPlayerMode === 'flash'; + var time = this.videoPlayer.currentTime; - if (isFlash) { + if (this.isFlashMode()) { this.videoPlayer.currentTime = Time.convert( time, parseFloat(this.speed), @@ -605,6 +605,11 @@ function (HTML5Video, Resizer) { } } + if (this.isFlashMode()) { + this.setSpeed(this.speed); + this.trigger('videoSpeedControl.setSpeed', this.speed); + } + if (this.currentPlayerMode === 'html5') { this.videoPlayer.player.setPlaybackRate(this.speed); } @@ -653,7 +658,7 @@ function (HTML5Video, Resizer) { videoPlayer.startTime = this.config.startTime; if (videoPlayer.startTime >= duration) { videoPlayer.startTime = 0; - } else if (this.currentPlayerMode === 'flash') { + } else if (this.isFlashMode()) { videoPlayer.startTime /= Number(this.speed); } @@ -664,7 +669,7 @@ function (HTML5Video, Resizer) { ) { videoPlayer.stopAtEndTime = false; videoPlayer.endTime = null; - } else if (this.currentPlayerMode === 'flash') { + } else if (this.isFlashMode()) { videoPlayer.endTime /= Number(this.speed); } } @@ -749,11 +754,7 @@ function (HTML5Video, Resizer) { // HTML5 video sources play fine from start-time in both Chrome // and Firefox. if (this.browserIsFirefox && this.videoType === 'youtube') { - if (this.currentPlayerMode === 'flash') { - youTubeId = this.youtubeId(); - } else { - youTubeId = this.youtubeId('1.0'); - } + youTubeId = this.youtubeId(); // When we will call cueVideoById() for some strange reason // an ENDED event will be fired. It really does no damage diff --git a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js index 05f0f47205..fadc23cb91 100644 --- a/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js +++ b/common/lib/xmodule/xmodule/js/src/video/06_video_progress_slider.js @@ -104,8 +104,7 @@ function () { // whole slider). Remember that endTime === null means the end-time // is set to the end of video by default. function updateStartEndTimeRegion(params) { - var isFlashMode = this.currentPlayerMode === 'flash', - left, width, start, end, duration, rangeParams; + var left, width, start, end, duration, rangeParams; // We must have a duration in order to determine the area of range. // It also must be non-zero. @@ -120,7 +119,7 @@ function () { if (start > duration) { start = 0; - } else if (isFlashMode) { + } else if (this.isFlashMode()) { start /= Number(this.speed); } @@ -128,7 +127,7 @@ function () { // video, then we set it to the end of the video. if (end === null || end > duration) { end = duration; - } else if (isFlashMode) { + } else if (this.isFlashMode()) { end /= Number(this.speed); } diff --git a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js index 13917defd2..0bba40d7db 100644 --- a/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js +++ b/common/lib/xmodule/xmodule/js/src/video/09_video_caption.js @@ -155,7 +155,7 @@ function () { } this.el.on('speedchange', function () { - if (self.currentPlayerMode === 'flash') { + if (self.isFlashMode()) { Caption.fetchCaption(); } }); @@ -628,7 +628,7 @@ function () { if (this.videoCaption.loaded) { // Current mode === 'flash' can only be for YouTube videos. So, we // don't have to also check for videoType === 'youtube'. - if (this.currentPlayerMode === 'flash') { + if (this.isFlashMode()) { // Total play time changes with speed change. Also there is // a 250 ms delay we have to take into account. time = Math.round( @@ -670,7 +670,7 @@ function () { // Current mode === 'flash' can only be for YouTube videos. So, we // don't have to also check for videoType === 'youtube'. - if (this.currentPlayerMode === 'flash') { + if (this.isFlashMode()) { // Total play time changes with speed change. Also there is // a 250 ms delay we have to take into account. time = Math.round( diff --git a/common/test/data/uploads/subs_b7xgknqkQk8.srt.sjson b/common/test/data/uploads/subs_b7xgknqkQk8.srt.sjson new file mode 100644 index 0000000000..a016152f3b --- /dev/null +++ b/common/test/data/uploads/subs_b7xgknqkQk8.srt.sjson @@ -0,0 +1,11 @@ +{ + "start": [ + 1000 + ], + "end": [ + 2000 + ], + "text": [ + "Equal transcripts" + ] +} \ No newline at end of file diff --git a/lms/djangoapps/courseware/features/video.feature b/lms/djangoapps/courseware/features/video.feature index 721e4630a7..d318b264aa 100644 --- a/lms/djangoapps/courseware/features/video.feature +++ b/lms/djangoapps/courseware/features/video.feature @@ -62,12 +62,12 @@ Feature: LMS Video component Given I am registered for the course "test_course" And it has a video "A" in "Youtube" mode in position "1" of sequential And a video "B" in "Youtube" mode in position "2" of sequential - And a video "C" in "Youtube" mode in position "3" of sequential + And a video "C" in "HTML5" mode in position "3" of sequential And I open the section with videos And I select the "2.0" speed on video "A" And I select the "0.50" speed on video "B" When I open video "C" - Then video "C" should start playing at speed "0.50" + Then video "C" should start playing at speed "0.75" When I open video "A" Then video "A" should start playing at speed "2.0" And I reload the page @@ -94,7 +94,7 @@ Feature: LMS Video component # 11 Scenario: CC button works correctly w/o english transcript in HTML5 mode of Video component Given the course has a Video component in HTML5 mode: - | transcripts | + | transcripts | | {"zh": "chinese_transcripts.srt"} | And I make sure captions are opened Then I see "好 各位同学" text in the captions @@ -112,13 +112,13 @@ Feature: LMS Video component # 13 Scenario: CC button works correctly w/o english transcript in Youtube mode of Video component Given the course has a Video component in Youtube mode: - | transcripts | + | transcripts | | {"zh": "chinese_transcripts.srt"} | And I make sure captions are opened Then I see "好 各位同学" text in the captions # 14 - Scenario: CC button works correctly if transcripts and sub fields are empty, but transcript file exists is assets (Youtube mode of Video component) + Scenario: CC button works correctly if transcripts and sub fields are empty, but transcript file exists in assets (Youtube mode of Video component) Given I am registered for the course "test_course" And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets And it has a video in "Youtube" mode @@ -176,3 +176,15 @@ Feature: LMS Video component Then I can download transcript in "txt" format When I open video "C" Then menu "download_transcript" doesn't exist + + # 20 + Scenario: Youtube video has correct transcript if fields for other speeds are filled. + Given the course has a Video component in Youtube mode: + | sub | youtube_id_1_5 | + | OEoXaMPEzfM | b7xgknqkQk8 | + And I make sure captions are opened + Then I see "Hi, welcome to Edx." text in the captions + And I select the "1.50" speed + And I reload the page + Then I see "Hi, welcome to Edx." text in the captions + And I see duration "1:00" diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index 2cd36b5641..ba7bfbaced 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -5,6 +5,7 @@ from lettuce import world, step import json import os import requests +import time from common import i_am_registered_for_the_course, section_location, visit_scenario_item from django.utils.translation import ugettext as _ from django.conf import settings @@ -94,6 +95,21 @@ def add_video_to_course(course, player_mode, hashes, display_name='Video'): 'metadata': {}, } + course_location =world.scenario_dict['COURSE'].location + + if hashes: + kwargs['metadata'].update(hashes[0]) + + conversions = { + 'transcripts': json.loads, + 'download_track': json.loads, + 'download_video': json.loads, + } + + for key in kwargs['metadata']: + if key in conversions: + kwargs['metadata'][key] = conversions[key](kwargs['metadata'][key]) + if player_mode == 'html5': kwargs['metadata'].update({ 'youtube_id_1_0': '', @@ -119,28 +135,18 @@ def add_video_to_course(course, player_mode, hashes, display_name='Video'): 'html5_sources': HTML5_SOURCES_INCORRECT }) - if hashes: - kwargs['metadata'].update(hashes[0]) - course_location =world.scenario_dict['COURSE'].location - - conversions = { - 'transcripts': json.loads, - 'download_track': json.loads, - 'download_video': json.loads, - } - - for key in kwargs['metadata']: - if key in conversions: - kwargs['metadata'][key] = conversions[key](kwargs['metadata'][key]) - - if 'sub' in kwargs['metadata']: + if kwargs['metadata'].get('sub'): filename = _get_sjson_filename(kwargs['metadata']['sub'], 'en') _upload_file(filename, course_location) - if 'transcripts' in kwargs['metadata']: + if kwargs['metadata'].get('transcripts'): for lang, filename in kwargs['metadata']['transcripts'].items(): _upload_file(filename, course_location) + if kwargs['metadata'].get('youtube_id_1_5'): + filename = _get_sjson_filename(kwargs['metadata']['youtube_id_1_5'], 'en') + _upload_file(filename, course_location) + world.scenario_dict['VIDEO'] = world.ItemFactory.create(**kwargs) @@ -208,6 +214,36 @@ def _set_window_dimensions(width, height): world.wait(0.2) +def _duration(): + """ + Total duration of the video, in seconds. + """ + elapsed_time, duration = _video_time() + return duration + + +def _video_time(): + """ + Return a tuple `(elapsed_time, duration)`, each in seconds. + """ + # The full time has the form "0:32 / 3:14" + full_time = world.css_text('div.vidtime') + + # Split the time at the " / ", to get ["0:32", "3:14"] + elapsed_str, duration_str = full_time.split(' / ') + + # Convert each string to seconds + return (_parse_time_str(elapsed_str), _parse_time_str(duration_str)) + + +def _parse_time_str(time_str): + """ + Parse a string of the form 1:23 into seconds (int). + """ + time_obj = time.strptime(time_str, '%M:%S') + return time_obj.tm_min * 60 + time_obj.tm_sec + + @step('when I view the (.*) it does not have autoplay enabled$') def does_not_autoplay(_step, video_type): assert(world.css_find('.%s' % video_type)[0]['data-autoplay'] == 'False') @@ -237,8 +273,13 @@ def visit_video_section(_step): visit_scenario_item('SECTION') +@step('I select the "([^"]*)" speed$') +def change_video_speed(_step, speed): + _change_video_speed(speed) + + @step('I select the "([^"]*)" speed on video "([^"]*)"$') -def change_video_speed(_step, speed, player_id): +def change_video_speed_on_video(_step, speed, player_id): _navigate_to_an_item_in_a_sequence(sequence[player_id]) _change_video_speed(speed) @@ -355,6 +396,14 @@ def start_playing_video_from_n_seconds(_step, position): ) +@step('I see duration "([^"]*)"$') +def i_see_duration(_step, position): + world.wait_for( + func=lambda _: _duration() == _parse_time_str(position), + timeout=5 + ) + + @step('I seek video to "([^"]*)" seconds$') def seek_video_to_n_seconds(_step, seconds): time = float(seconds.strip()) From 40c1090ca8acbe5da78f67f7fce5a1b598af09ff Mon Sep 17 00:00:00 2001 From: polesye Date: Tue, 11 Mar 2014 18:41:24 +0200 Subject: [PATCH 8/8] Upload files explicitly in tests. --- .../courseware/features/video.feature | 29 ++++++++++++++----- lms/djangoapps/courseware/features/video.py | 14 --------- 2 files changed, 22 insertions(+), 21 deletions(-) diff --git a/lms/djangoapps/courseware/features/video.feature b/lms/djangoapps/courseware/features/video.feature index d318b264aa..c5df8c23e4 100644 --- a/lms/djangoapps/courseware/features/video.feature +++ b/lms/djangoapps/courseware/features/video.feature @@ -81,8 +81,11 @@ Feature: LMS Video component # 10 Scenario: Language menu works correctly in Video component - Given the course has a Video component in Youtube mode: - | transcripts | sub | + Given I am registered for the course "test_course" + And I have a "chinese_transcripts.srt" transcript file in assets + And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets + And it has a video in "Youtube" mode: + | transcripts | sub | | {"zh": "chinese_transcripts.srt"} | OEoXaMPEzfM | And I make sure captions are closed And I see video menu "language" with correct items @@ -93,7 +96,9 @@ Feature: LMS Video component # 11 Scenario: CC button works correctly w/o english transcript in HTML5 mode of Video component - Given the course has a Video component in HTML5 mode: + Given I am registered for the course "test_course" + And I have a "chinese_transcripts.srt" transcript file in assets + And it has a video in "HTML5" mode: | transcripts | | {"zh": "chinese_transcripts.srt"} | And I make sure captions are opened @@ -111,7 +116,9 @@ Feature: LMS Video component # 13 Scenario: CC button works correctly w/o english transcript in Youtube mode of Video component - Given the course has a Video component in Youtube mode: + Given I am registered for the course "test_course" + And I have a "chinese_transcripts.srt" transcript file in assets + And it has a video in "Youtube" mode: | transcripts | | {"zh": "chinese_transcripts.srt"} | And I make sure captions are opened @@ -132,7 +139,9 @@ Feature: LMS Video component # 16 Scenario: Video is aligned correctly if transcript is visible in fullscreen mode - Given the course has a Video component in HTML5 mode: + Given I am registered for the course "test_course" + And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets + And it has a video in "HTML5" mode: | sub | | OEoXaMPEzfM | And I make sure captions are opened @@ -147,7 +156,9 @@ Feature: LMS Video component # 18 Scenario: Video is aligned correctly on transcript toggle in fullscreen mode - Given the course has a Video component in Youtube mode: + Given I am registered for the course "test_course" + And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets + And it has a video in "Youtube" mode: | sub | | OEoXaMPEzfM | And I make sure captions are opened @@ -159,6 +170,7 @@ Feature: LMS Video component # 19 Scenario: Download Transcript button works correctly in Video component Given I am registered for the course "test_course" + And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets And it has a video "A" in "Youtube" mode in position "1" of sequential: | sub | download_track | | OEoXaMPEzfM | true | @@ -179,7 +191,10 @@ Feature: LMS Video component # 20 Scenario: Youtube video has correct transcript if fields for other speeds are filled. - Given the course has a Video component in Youtube mode: + Given I am registered for the course "test_course" + And I have a "subs_OEoXaMPEzfM.srt.sjson" transcript file in assets + And I have a "subs_b7xgknqkQk8.srt.sjson" transcript file in assets + And it has a video in "Youtube" mode: | sub | youtube_id_1_5 | | OEoXaMPEzfM | b7xgknqkQk8 | And I make sure captions are opened diff --git a/lms/djangoapps/courseware/features/video.py b/lms/djangoapps/courseware/features/video.py index ba7bfbaced..530333c711 100644 --- a/lms/djangoapps/courseware/features/video.py +++ b/lms/djangoapps/courseware/features/video.py @@ -95,8 +95,6 @@ def add_video_to_course(course, player_mode, hashes, display_name='Video'): 'metadata': {}, } - course_location =world.scenario_dict['COURSE'].location - if hashes: kwargs['metadata'].update(hashes[0]) @@ -135,18 +133,6 @@ def add_video_to_course(course, player_mode, hashes, display_name='Video'): 'html5_sources': HTML5_SOURCES_INCORRECT }) - if kwargs['metadata'].get('sub'): - filename = _get_sjson_filename(kwargs['metadata']['sub'], 'en') - _upload_file(filename, course_location) - - if kwargs['metadata'].get('transcripts'): - for lang, filename in kwargs['metadata']['transcripts'].items(): - _upload_file(filename, course_location) - - if kwargs['metadata'].get('youtube_id_1_5'): - filename = _get_sjson_filename(kwargs['metadata']['youtube_id_1_5'], 'en') - _upload_file(filename, course_location) - world.scenario_dict['VIDEO'] = world.ItemFactory.create(**kwargs)