Merge pull request #7823 from edx/christina/notes-tagging

Add tags plugin.

cms/envs/bok_choy.py

@@ -65,6 +65,9 @@ FEATURES['MILESTONES_APP'] = True
 # Enable pre-requisite course
 FEATURES['ENABLE_PREREQUISITE_COURSES'] = True
 
+# Enable student notes
+FEATURES['ENABLE_EDXNOTES'] = True
+
 ########################### Entrance Exams #################################
 FEATURES['ENTRANCE_EXAMS'] = True
 

common/test/acceptance/pages/lms/edxnotes.py

 from bok_choy.page_object import PageObject, PageLoadError, unguarded
-from bok_choy.promise import BrokenPromise
+from bok_choy.promise import BrokenPromise, EmptyPromise
 from .course_page import CoursePage
 from ...tests.helpers import disable_animations
 from selenium.webdriver.common.action_chains import ActionChains
@@ -523,7 +523,7 @@ class EdxNoteHighlight(NoteChild):
         Returns text of the note.
         """
         self.show()
-        element = self.q(css=self._bounded_selector(".annotator-annotation > div"))
+        element = self.q(css=self._bounded_selector(".annotator-annotation > div.annotator-note"))
         if element:
             text = element.text[0].strip()
         else:
@@ -538,3 +538,47 @@ class EdxNoteHighlight(NoteChild):
         Sets text for the note.
         """
         self.q(css=self._bounded_selector(".annotator-item textarea")).first.fill(value)
+
+    @property
+    def tags(self):
+        """
+        Returns the tags associated with the note.
+
+        Tags are returned as a list of strings, with each tag as an individual string.
+        """
+        tag_text = []
+        self.show()
+        tags = self.q(css=self._bounded_selector(".annotator-annotation > div.annotator-tags > span.annotator-tag"))
+        if tags:
+            for tag in tags:
+                tag_text.append(tag.text)
+        self.q(css="body").first.click()
+        self.wait_for_notes_invisibility()
+        return tag_text
+
+    @tags.setter
+    def tags(self, tags):
+        """
+        Sets tags for the note. Tags should be supplied as a list of strings, with each tag as an individual string.
+        """
+        self.q(css=self._bounded_selector(".annotator-item input")).first.fill(" ".join(tags))
+
+    def has_sr_label(self, sr_index, field_index, expected_text):
+        """
+        Returns true iff a screen reader label (of index sr_index) exists for the annotator field with
+        the specified field_index and text.
+        """
+        label_exists = False
+        EmptyPromise(
+            lambda: len(self.q(css=self._bounded_selector("li.annotator-item > label.sr"))) > sr_index,
+            "Expected more than '{}' sr labels".format(sr_index)
+        ).fulfill()
+        annotator_field_label = self.q(css=self._bounded_selector("li.annotator-item > label.sr"))[sr_index]
+        for_attrib_correct = annotator_field_label.get_attribute("for") == "annotator-field-" + str(field_index)
+        if for_attrib_correct and (annotator_field_label.text == expected_text):
+            label_exists = True
+
+        self.q(css="body").first.click()
+        self.wait_for_notes_invisibility()
+
+        return label_exists

common/test/acceptance/pages/studio/settings_advanced.py

@@ -177,6 +177,7 @@ class AdvancedSettingsPage(CoursePage):
             'discussion_topics',
             'due',
             'due_date_display_format',
+            'edxnotes',
             'use_latex_compiler',
             'video_speed_optimizations',
             'enrollment_domain',

common/test/acceptance/tests/lms/test_lms_edxnotes.py

 import os
 from uuid import uuid4
 from datetime import datetime
-from unittest import skipUnless
 from ..helpers import UniqueCourseTest
 from ...fixtures.course import CourseFixture, XBlockFixtureDesc
 from ...pages.lms.auto_auth import AutoAuthPage
@@ -11,7 +10,6 @@ from ...pages.lms.edxnotes import EdxNotesUnitPage, EdxNotesPage
 from ...fixtures.edxnotes import EdxNotesFixture, Note, Range
 
 
-@skipUnless(os.environ.get("FEATURE_EDXNOTES"), "Requires Student Notes feature to be enabled")
 class EdxNotesTestMixin(UniqueCourseTest):
     """
     Creates a course with initial data and contains useful helper methods.
@@ -140,6 +138,16 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin):
                 note.text = "TEST TEXT {}".format(index)
                 index += 1
 
+    def edit_tags_in_notes(self, components, tags):
+        self.assertGreater(len(components), 0)
+        index = 0
+        for component in components:
+            self.assertGreater(len(component.notes), 0)
+            for note in component.edit_note():
+                note.tags = tags[index]
+                index += 1
+        self.assertEqual(index, len(tags), "Number of supplied tags did not match components")
+
     def remove_notes(self, components):
         self.assertGreater(len(components), 0)
         for component in components:
@@ -155,6 +163,11 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin):
         expected = ["TEST TEXT {}".format(i) for i in xrange(len(notes))]
         self.assertEqual(expected, actual)
 
+    def assert_tags_in_notes(self, notes, expected_tags):
+        actual = [note.tags for note in notes]
+        expected = [expected_tags[i] for i in xrange(len(notes))]
+        self.assertEqual(expected, actual)
+
     def test_can_create_notes(self):
         """
         Scenario: User can create notes.
@@ -255,6 +268,69 @@ class EdxNotesDefaultInteractionsTest(EdxNotesTestMixin):
         components = self.note_unit_page.components
         self.assert_notes_are_removed(components)
 
+    def test_can_create_note_with_tags(self):
+        """
+        Scenario: a user of notes can define one with tags
+        Given I have a course with 3 annotatable components
+        And I open the unit with 2 annotatable components
+        When I add a note with tags for the first component
+        And I refresh the page
+        Then I see that note was correctly stored with its tags
+        """
+        self.note_unit_page.visit()
+
+        components = self.note_unit_page.components
+        for note in components[0].create_note(".{}".format(self.selector)):
+            note.tags = ["fruit", "tasty"]
+
+        self.note_unit_page.refresh()
+        self.assertEqual(["fruit", "tasty"], self.note_unit_page.notes[0].tags)
+
+    def test_can_change_tags(self):
+        """
+        Scenario: a user of notes can edit tags on notes
+        Given I have a course with 3 components with notes
+        When I open the unit with 2 annotatable components
+        And I edit tags on the notes for the 2 annotatable components
+        Then I see that the tags were correctly changed
+        And I again edit tags on the notes for the 2 annotatable components
+        And I refresh the page
+        Then I see that the tags were correctly changed
+        """
+        self._add_notes()
+        self.note_unit_page.visit()
+
+        components = self.note_unit_page.components
+        self.edit_tags_in_notes(components, [["hard"], ["apple", "pear"]])
+        self.assert_tags_in_notes(self.note_unit_page.notes, [["hard"], ["apple", "pear"]])
+
+        self.edit_tags_in_notes(components, [[], ["avocado"]])
+        self.assert_tags_in_notes(self.note_unit_page.notes, [[], ["avocado"]])
+
+        self.note_unit_page.refresh()
+        self.assert_tags_in_notes(self.note_unit_page.notes, [[], ["avocado"]])
+
+    def test_sr_labels(self):
+        """
+        Scenario: screen reader labels exist for text and tags fields
+        Given I have a course with 3 components with notes
+        When I open the unit with 2 annotatable components
+        And I open the editor for each note
+        Then the text and tags fields both have screen reader labels
+        """
+        self._add_notes()
+        self.note_unit_page.visit()
+
+        # First note is in the first annotatable component, will have field indexes 0 and 1.
+        for note in self.note_unit_page.components[0].edit_note():
+            self.assertTrue(note.has_sr_label(0, 0, "Note"))
+            self.assertTrue(note.has_sr_label(1, 1, "Tags (space-separated)"))
+
+        # Second note is in the second annotatable component, will have field indexes 2 and 3.
+        for note in self.note_unit_page.components[1].edit_note():
+            self.assertTrue(note.has_sr_label(0, 2, "Note"))
+            self.assertTrue(note.has_sr_label(1, 3, "Tags (space-separated)"))
+
 
 class EdxNotesPageTest(EdxNotesTestMixin):
     """

lms/envs/bok_choy.py

@@ -94,6 +94,9 @@ FEATURES['MILESTONES_APP'] = True
 # Enable pre-requisite course
 FEATURES['ENABLE_PREREQUISITE_COURSES'] = True
 
+# Enable student notes
+FEATURES['ENABLE_EDXNOTES'] = True
+
 # Unfortunately, we need to use debug mode to serve staticfiles
 DEBUG = True
 

lms/static/js/edxnotes/models/note.js

@@ -12,6 +12,7 @@ define(['backbone', 'js/edxnotes/utils/utils', 'underscore.string'], function (B
             'text': '',
             'quote': '',
             'ranges': [],
+            'tags': [],
             'unit': {
                 'display_name': '',
                 'url': '',

lms/static/js/edxnotes/plugins/accessibility.js

@@ -131,19 +131,27 @@ define(['jquery', 'underscore', 'annotator_1.2.9'], function ($, _, Annotator) {
         },
 
         getEditorTabControls: function () {
-            var editor, editorControls, textArea, saveButton, cancelButton, tabControls = [];
+            var editor, editorControls, textArea, saveButton, cancelButton, tabControls = [], annotatorItems,
+                tagInput = null;
 
             // Editor elements
             editor = this.annotator.element.find('.annotator-editor');
             editorControls = editor.find('.annotator-controls');
-            textArea = editor.find('.annotator-listing')
-                             .find('.annotator-item')
-                             .first()
-                             .children('textarea');
+            annotatorItems = editor.find('.annotator-listing').find('.annotator-item');
+            textArea = annotatorItems.first().children('textarea');
             saveButton  = editorControls.find('.annotator-save');
             cancelButton = editorControls.find('.annotator-cancel');
 
-            tabControls.push(textArea, saveButton, cancelButton);
+            // If the tags plugin is enabled, add the ability to tab into it.
+            if (annotatorItems.length > 1) {
+                tagInput = annotatorItems.first().next().children('input');
+            }
+
+            tabControls.push(textArea);
+            if (tagInput){
+                tabControls.push(tagInput);
+            }
+            tabControls.push(saveButton, cancelButton);
 
             return tabControls;
         },

lms/static/js/edxnotes/views/notes_factory.js

@@ -6,7 +6,7 @@ define([
      'js/edxnotes/plugins/events', 'js/edxnotes/plugins/accessibility',
      'js/edxnotes/plugins/caret_navigation'
 ], function ($, _, Annotator, NotesLogger) {
-    var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation'],
+    var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation', 'Tags'],
         getOptions, setupPlugins, updateHeaders, getAnnotator;
 
     /**

lms/static/js/edxnotes/views/shim.js

@@ -58,6 +58,34 @@ define([
         return (timeToExpiry > 0) ? timeToExpiry : 0;
     };
 
+
+    Annotator.Plugin.Tags.prototype.updateField = _.compose(
+        function() {
+            // Add screen reader label for edit mode. Note that the id of the tags element will not always be "1".
+            // It depends on the number of annotatable components on the page.
+            var tagsField = $("li.annotator-item >input", this.annotator.editor.element).attr('id');
+            if ($("label.sr[for='"+ tagsField + "']", this.annotator.editor.element).length === 0) {
+                $('<label class="sr" for='+ tagsField +'>' + _t('Tags (space-separated)') + '</label>').insertBefore(
+                    $('#'+tagsField, this.annotator.editor.element)
+                );
+            }
+            return this;
+        },
+        Annotator.Plugin.Tags.prototype.updateField
+    );
+
+    Annotator.Plugin.Tags.prototype.updateViewer = _.compose(
+        function() {
+            // Add ARIA information for viewing mode.
+            $('div.annotator-tags', this.wrapper).attr({
+                'role': 'region',
+                'aria-label': 'tags'
+            });
+            return this;
+        },
+        Annotator.Plugin.Tags.prototype.updateViewer
+    );
+
     /**
      * Modifies Annotator.highlightRange to add "tabindex=0" and role="link"
      * attributes to the <span class="annotator-hl"> markup that encloses the
@@ -186,25 +214,24 @@ define([
         '</div>'
     ].join('');
 
-    /**
-     * Modifies Annotator._setupEditor to add a label for textarea#annotator-field-0.
-     **/
-    Annotator.prototype._setupEditor = _.compose(
-        function () {
-            $('<label class="sr" for="annotator-field-0">Edit note</label>').insertBefore(
-                $('#annotator-field-0', this.wrapper)
-            );
-            return this;
-        },
-        Annotator.prototype._setupEditor
-    );
 
     /**
      * Modifies Annotator.Editor.show, in the case of a keydown event, to remove
      * focus from Save button and put it on form.annotator-widget instead.
+     *
+     * Also add a sr label for note textarea.
      **/
     Annotator.Editor.prototype.show = _.compose(
         function (event) {
+            // Add screen reader label for the note area. Note that the id of the tags element will not always be "0".
+            // It depends on the number of annotatable components on the page.
+            var noteField = $("li.annotator-item >textarea", this.element).attr('id');
+            if ($("label.sr[for='"+ noteField + "']", this.element).length === 0) {
+                $('<label class="sr" for='+ noteField +'>' + _t('Note') + '</label>').insertBefore(
+                    $('#'+noteField, this.element)
+                );
+            }
+
             if (event.type === 'keydown') {
                 this.element.find('.annotator-save').removeClass(this.classes.focus);
                 this.element.find('form.annotator-widget').focus();

lms/static/js/spec/edxnotes/plugins/accessibility_spec.js

@@ -168,12 +168,12 @@ define([
                 };
                 highlight.data('annotation', annotation);
                 this.annotator.viewer.load([annotation]);
-                listing = this.annotator.element.find('.annotator-listing').first(),
+                listing = this.annotator.element.find('.annotator-listing').first();
                 note = this.annotator.element.find('.annotator-note').first();
                 edit= this.annotator.element.find('.annotator-edit').first();
                 del = this.annotator.element.find('.annotator-delete').first();
                 close = this.annotator.element.find('.annotator-close').first();
-                spyOn(this.annotator.viewer, 'hide').andCallThrough();;
+                spyOn(this.annotator.viewer, 'hide').andCallThrough();
             });
 
             it('should give focus to Note on Listing TAB keydown', function () {
@@ -224,7 +224,7 @@ define([
         });
 
         describe('keydown events on editor', function () {
-            var highlight, annotation, form, textArea, save, cancel;
+            var highlight, annotation, form, annotatorItems, textArea, tags, save, cancel;
 
             beforeEach(function() {
                 highlight = $('<span class="annotator-hl" tabindex="0"/>').appendTo(this.annotator.element);
@@ -236,7 +236,9 @@ define([
                 highlight.data('annotation', annotation);
                 this.annotator.editor.show(annotation, {'left': 0, 'top': 0});
                 form = this.annotator.element.find('form.annotator-widget');
-                textArea = this.annotator.element.find('.annotator-item').first().children('textarea');
+                annotatorItems = this.annotator.element.find('.annotator-item');
+                textArea = annotatorItems.first().children('textarea');
+                tags = annotatorItems.first().next().children('input');
                 save  = this.annotator.element.find('.annotator-save');
                 cancel = this.annotator.element.find('.annotator-cancel');
                 spyOn(this.annotator.editor, 'submit').andCallThrough();
@@ -255,9 +257,11 @@ define([
                 expect(cancel).toBeFocused();
             });
 
-            it('should cycle forward through texarea, save, and cancel on TAB keydown', function () {
+            it('should cycle forward through textarea, tags, save, and cancel on TAB keydown', function () {
                 textArea.focus();
                 textArea.trigger(tabForwardEvent());
+                expect(tags).toBeFocused();
+                tags.trigger(tabForwardEvent());
                 expect(save).toBeFocused();
                 save.trigger(tabForwardEvent());
                 expect(cancel).toBeFocused();
@@ -265,13 +269,15 @@ define([
                 expect(textArea).toBeFocused();
             });
 
-            it('should cycle back through texarea, save, and cancel on SHIFT + TAB keydown', function () {
+            it('should cycle back through textarea, tags, save, and cancel on SHIFT + TAB keydown', function () {
                 textArea.focus();
                 textArea.trigger(tabBackwardEvent());
                 expect(cancel).toBeFocused();
                 cancel.trigger(tabBackwardEvent());
                 expect(save).toBeFocused();
                 save.trigger(tabBackwardEvent());
+                expect(tags).toBeFocused();
+                tags.trigger(tabBackwardEvent());
                 expect(textArea).toBeFocused();
             });
 

lms/static/js/spec/edxnotes/views/note_item_spec.js

@@ -53,8 +53,21 @@ define([
 
         it('should display update value and accompanying text', function() {
             var view = getView();
-            expect(view.$('.reference-title').last()).toContainText('Last Edited:');
-            expect(view.$('.reference-meta').last()).toContainText('December 11, 2014 at 11:12AM');
+            expect(view.$('.reference-title')[1]).toContainText('Last Edited:');
+            expect(view.$('.reference-updated-date').last()).toContainText('December 11, 2014 at 11:12AM');
+        });
+
+        it('should not display tags if there are none', function() {
+            var view = getView();
+            expect(view.$el).not.toContain('.reference-tags');
+            expect(view.$('.reference-title').length).toBe(2);
+        });
+
+        it('should display tags if they exist', function() {
+            var view = getView({tags: ["First", "Second"]});
+            expect(view.$('.reference-title').length).toBe(3);
+            expect(view.$('.reference-title')[2]).toContainText('Tags:');
+            expect(view.$('.reference-tags').last()).toContainText('First, Second');
         });
 
         it('should log the edx.student_notes.used_unit_link event properly', function () {

lms/static/js/spec/edxnotes/views/shim_spec.js

@@ -233,5 +233,37 @@ define([
                 expect(mockViewer.element.appendTo).toHaveBeenCalledWith(annotators[0].wrapper);
             });
         });
+
+        describe('TagsPlugin', function () {
+            it('should add ARIA label information to the viewer', function() {
+                var tagDiv,
+                    annotation = {
+                        id: '01',
+                        text: "Test text",
+                        tags: ["tag1", "tag2", "tag3"],
+                        highlights: [highlights[0].get(0)]
+                    };
+
+                annotators[0].viewer.load([annotation]);
+                tagDiv = annotators[0].viewer.element.find('.annotator-tags');
+                expect($(tagDiv).attr('role')).toEqual('region');
+                expect($(tagDiv).attr('aria-label')).toEqual('tags');
+
+                // Three children for the individual tags.
+                expect($(tagDiv).children().length).toEqual(3);
+            });
+
+            it('should add screen reader label to the editor', function() {
+                var srLabel, editor, inputId;
+
+                // We don't know exactly what the input ID will be (depends on number of annotatable components shown),
+                // but the sr label "for" attribute should match the ID of the element immediately following it.
+                annotators[0].showEditor({}, {});
+                editor = annotators[0].editor;
+                srLabel = editor.element.find("label.sr");
+                inputId = srLabel.next().attr('id');
+                expect(srLabel.attr('for')).toEqual(inputId);
+            });
+        });
     });
 });

lms/static/sass/course/_student-notes.scss

@@ -212,7 +212,7 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4;
           .reference-title {
             @extend %t-title8;
             @extend %t-weight3;
-            margin-top: $baseline;
+            margin-top: ($baseline/2);
             text-transform: uppercase;
             letter-spacing: ($baseline/20);
             color: $gray-l2;

lms/templates/edxnotes/note-item.underscore

@@ -34,5 +34,10 @@
 
         <h3 class="reference-title"><%- gettext("Last Edited:") %></h3>
         <span class="reference-meta reference-updated-date"><%- updated %></span>
+
+        <% if (tags.length > 0) { %>
+            <h3 class="reference-title"><%- gettext("Tags:") %></h3>
+            <span class="reference-meta reference-tags"><%- tags.join(", ") %></span>
+        <% } %>
     </div>
 </footer>