Merge pull request #6141 from edx/jmclaus/toggle-all-notes

TNL-661: Toggle all markup visibility and ability to add new notes.

cms/djangoapps/models/settings/course_metadata.py

@@ -32,6 +32,7 @@ class CourseMetadata(object):
         'name',  # from xblock
         'tags',  # from xblock
         'visible_to_staff_only',
+        'edxnotes_visibility',
     ]
 
     @classmethod

common/lib/xmodule/xmodule/modulestore/inheritance.py

@@ -169,6 +169,12 @@ class InheritanceMixin(XBlockMixin):
         default=False,
         scope=Scope.settings
     )
+    edxnotes_visibility = Boolean(
+        display_name=_("Enable visibility of Notes"),
+        help=_("Enter true or false. If true, Notes for HTML components will be visible."),
+        default=True,
+        scope=Scope.user_info
+    )
 
 
 def compute_inherited_metadata(descriptor):

common/templates/edxnotes_wrapper.html

@@ -9,9 +9,9 @@
 </div>
 <script type="text/javascript">
     (function (require) {
-        require(['js/edxnotes/views/notes_factory'], function (Notes) {
+        require(['js/edxnotes/views/visibility_decorator'], function(EdxnotesVisibilityDecorator) {
             var element = document.getElementById('edx-notes-wrapper-${uid}');
-            Notes.factory(element, ${json.dumps(params)});
+            EdxnotesVisibilityDecorator.factory(element, ${json.dumps(params)}, ${edxnotes_visibility});
         });
     }).call(this, require || RequireJS.require);
 </script>

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

@@ -251,6 +251,13 @@ def click(self, selector):
         self.q(css=selector).first.click()
         return self
 
+    def toggle_visibility(self):
+        """
+        Clicks on the "Show notes" checkbox.
+        """
+        self.q(css=".action-toggle-notes").first.click()
+        return self
+
     @property
     def components(self):
         """

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

@@ -39,7 +39,7 @@ def setUp(self):
 
         self.course_fixture.add_children(
             XBlockFixtureDesc("chapter", "Test Section").add_children(
-                XBlockFixtureDesc("sequential", "Test Subsection").add_children(
+                XBlockFixtureDesc("sequential", "Test Subsection 1").add_children(
                     XBlockFixtureDesc("vertical", "Test Vertical").add_children(
                         XBlockFixtureDesc(
                             "html",
@@ -61,6 +61,18 @@ def setUp(self):
                         data="""<p><span class="{}">Annotate this text!</span></p>""".format(self.selector)
                     ),
                 ),
+                XBlockFixtureDesc("sequential", "Test Subsection 2").add_children(
+                    XBlockFixtureDesc("vertical", "Test Vertical").add_children(
+                        XBlockFixtureDesc(
+                            "html",
+                            "Test HTML 4",
+                            data="""
+                                <p><span class="{0}">Annotate</span> this text!</p>
+                                <p>Annotate this text</p>
+                            """.format(self.selector)
+                        ),
+                    ),
+                ),
             )).install()
 
         AutoAuthPage(self.browser, username=self.username, email=self.email, course_id=self.course_id).visit()
@@ -521,3 +533,60 @@ def test_interaction_between_notes(self):
 
         note_2.click_on_highlight()
         self.assertTrue(note_2.is_visible)
+
+
+class EdxNotesToggleNotesTest(EdxNotesTestMixin):
+    """
+    Tests for toggling visibility of all notes.
+    """
+
+    def setUp(self):
+        super(EdxNotesToggleNotesTest, self).setUp()
+        self._add_notes()
+        self.note_unit_page.visit()
+
+    def test_can_disable_all_notes(self):
+        """
+        Scenario: User can disable all notes.
+        Given I have a course with components with notes
+        And I open the unit with annotatable components
+        When I click on "Show notes" checkbox
+        Then I do not see any notes on the sequential position
+        When I change sequential position to "2"
+        Then I still do not see any notes on the sequential position
+        When I go to "Test Subsection 2" subsection
+        Then I do not see any notes on the subsection
+        """
+        # Disable all notes
+        self.note_unit_page.toggle_visibility()
+        self.assertEqual(len(self.note_unit_page.notes), 0)
+        self.course_nav.go_to_sequential_position(2)
+        self.assertEqual(len(self.note_unit_page.notes), 0)
+        self.course_nav.go_to_section(u"Test Section", u"Test Subsection 2")
+        self.assertEqual(len(self.note_unit_page.notes), 0)
+
+    def test_can_reenable_all_notes(self):
+        """
+        Scenario: User can toggle notes visibility.
+        Given I have a course with components with notes
+        And I open the unit with annotatable components
+        When I click on "Show notes" checkbox
+        Then I do not see any notes on the sequential position
+        When I click on "Show notes" checkbox again
+        Then I see that all notes appear
+        When I change sequential position to "2"
+        Then I still can see all notes on the sequential position
+        When I go to "Test Subsection 2" subsection
+        Then I can see all notes on the subsection
+        """
+        # Disable notes
+        self.note_unit_page.toggle_visibility()
+        self.assertEqual(len(self.note_unit_page.notes), 0)
+        # Enable notes to make sure that I can enable notes without refreshing
+        # the page.
+        self.note_unit_page.toggle_visibility()
+        self.assertGreater(len(self.note_unit_page.notes), 0)
+        self.course_nav.go_to_sequential_position(2)
+        self.assertGreater(len(self.note_unit_page.notes), 0)
+        self.course_nav.go_to_section(u"Test Section", u"Test Subsection 2")
+        self.assertGreater(len(self.note_unit_page.notes), 0)

lms/djangoapps/edxnotes/decorators.py

@@ -2,6 +2,7 @@
 Decorators related to edXNotes.
 """
 from django.conf import settings
+import json
 from edxnotes.helpers import (
     get_endpoint,
     get_id_token,
@@ -33,6 +34,9 @@ def get_html(self, *args, **kwargs):
             return render_to_string("edxnotes_wrapper.html", {
                 "content": original_get_html(self, *args, **kwargs),
                 "uid": generate_uid(),
+                "edxnotes_visibility": json.dumps(
+                    getattr(self, 'edxnotes_visibility', course.edxnotes_visibility)
+                ),
                 "params": {
                     # Use camelCase to name keys.
                     "usageId": unicode(self.scope_ids.usage_id).encode("utf-8"),

lms/djangoapps/edxnotes/helpers.py

@@ -22,6 +22,7 @@
 import oauth2_provider.oidc as oidc
 from provider.utils import now
 from .exceptions import EdxNotesParseError
+
 log = logging.getLogger(__name__)
 
 

lms/djangoapps/edxnotes/tests.py

@@ -14,11 +14,12 @@
 from django.core.exceptions import ImproperlyConfigured
 from oauth2_provider.tests.factories import ClientFactory
 from provider.oauth2.models import Client
-
 from xmodule.tabs import EdxNotesTab
 from xmodule.modulestore.django import modulestore
 from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.exceptions import ItemNotFoundError
+from courseware.model_data import FieldDataCache
+from courseware.module_render import get_module_for_descriptor
 from student.tests.factories import UserFactory
 
 from .exceptions import EdxNotesParseError
@@ -86,6 +87,7 @@ def test_edxnotes_enabled(self, mock_generate_uid, mock_get_id_token, mock_get_t
         expected_context = {
             "content": "original_get_html",
             "uid": "uid",
+            "edxnotes_visibility": "true",
             "params": {
                 "usageId": u"test_usage_id",
                 "courseId": unicode(self.course.id).encode("utf-8"),
@@ -520,6 +522,14 @@ def setUp(self):
         self.notes_page_url = reverse("edxnotes", args=[unicode(self.course.id)])
         self.search_url = reverse("search_notes", args=[unicode(self.course.id)])
         self.get_token_url = reverse("get_token", args=[unicode(self.course.id)])
+        self.visibility_url = reverse("edxnotes_visibility", args=[unicode(self.course.id)])
+
+    def _get_course_module(self):
+        """
+        Returns the course module.
+        """
+        field_data_cache = FieldDataCache([self.course], self.course.id, self.user)
+        return get_module_for_descriptor(self.user, MagicMock(), self.course, field_data_cache, self.course.id)
 
     # pylint: disable=unused-argument
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True})
@@ -532,7 +542,6 @@ def test_edxnotes_view_is_enabled(self, mock_get_notes):
         response = self.client.get(self.notes_page_url)
         self.assertContains(response, "<h1>Notes</h1>")
 
-    # pylint: disable=unused-argument
     @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False})
     def test_edxnotes_view_is_disabled(self):
         """
@@ -617,3 +626,51 @@ def test_get_id_token_anonymous(self):
         self.client.logout()
         response = self.client.get(self.get_token_url)
         self.assertEqual(response.status_code, 302)
+
+    def test_edxnotes_visibility(self):
+        """
+        Can update edxnotes_visibility value successfully.
+        """
+        enable_edxnotes_for_the_course(self.course, self.user.id)
+        response = self.client.post(
+            self.visibility_url,
+            data=json.dumps({"visibility": False}),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, 200)
+        course_module = self._get_course_module()
+        self.assertFalse(course_module.edxnotes_visibility)
+
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False})
+    def test_edxnotes_visibility_if_feature_is_disabled(self):
+        """
+        Tests that 404 response is received if EdxNotes feature is disabled.
+        """
+        response = self.client.post(self.visibility_url)
+        self.assertEqual(response.status_code, 404)
+
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True})
+    def test_edxnotes_visibility_invalid_json(self):
+        """
+        Tests that 400 response is received if invalid JSON is sent.
+        """
+        enable_edxnotes_for_the_course(self.course, self.user.id)
+        response = self.client.post(
+            self.visibility_url,
+            data="string",
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, 400)
+
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True})
+    def test_edxnotes_visibility_key_error(self):
+        """
+        Tests that 400 response is received if invalid data structure is sent.
+        """
+        enable_edxnotes_for_the_course(self.course, self.user.id)
+        response = self.client.post(
+            self.visibility_url,
+            data=json.dumps({'test_key': 1}),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, 400)

lms/djangoapps/edxnotes/urls.py

@@ -9,4 +9,5 @@
     url(r"^/$", "edxnotes", name="edxnotes"),
     url(r"^/search/$", "search_notes", name="search_notes"),
     url(r"^/token/$", "get_token", name="get_token"),
+    url(r"^/visibility/$", "edxnotes_visibility", name="edxnotes_visibility"),
 )

lms/djangoapps/edxnotes/views.py

@@ -2,14 +2,17 @@
 Views related to EdxNotes.
 """
 import json
+import logging
 from django.contrib.auth.decorators import login_required
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse, HttpResponseBadRequest, Http404
 from django.conf import settings
-from util.json_request import JsonResponseBadRequest
 from edxmako.shortcuts import render_to_response
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 from courseware.courses import get_course_with_access
+from courseware.model_data import FieldDataCache
+from courseware.module_render import get_module_for_descriptor
+from util.json_request import JsonResponse, JsonResponseBadRequest
 from edxnotes.exceptions import EdxNotesParseError
 from edxnotes.helpers import (
     get_notes,
@@ -18,6 +21,8 @@
     search
 )
 
+log = logging.getLogger(__name__)
+
 
 @login_required
 def edxnotes(request, course_id):
@@ -71,3 +76,27 @@ def get_token(request, course_id):
     Get JWT ID-Token, in case you need new one.
     """
     return HttpResponse(get_id_token(request.user), content_type='text/plain')
+
+
+def edxnotes_visibility(request, course_id):
+    """
+    Handle ajax call from "Show notes" checkbox.
+    """
+    course_key = SlashSeparatedCourseKey.from_deprecated_string(course_id)
+    course = get_course_with_access(request.user, "load", course_key)
+    field_data_cache = FieldDataCache([course], course_key, request.user)
+    course_module = get_module_for_descriptor(request.user, request, course, field_data_cache, course_key)
+
+    if not is_feature_enabled(course):
+        raise Http404
+
+    try:
+        visibility = json.loads(request.body)["visibility"]
+        course_module.edxnotes_visibility = visibility
+        course_module.save()
+        return JsonResponse(status=200)
+    except (ValueError, KeyError):
+        log.warning(
+            "Could not decode request body as JSON and find a boolean visibility field: '{0}'".format(request.body)
+        )
+        return JsonResponseBadRequest()

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

@@ -13,13 +13,21 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
      * so we add it here if necessary.
      **/
     if (!$.fn.addBack) {
-        $.fn.addBack = function(selector) {
-            return this.add(selector === null ?
-                    this.prevObject : this.prevObject.filter(selector)
+        $.fn.addBack = function (selector) {
+            return this.add(
+              selector === null ? this.prevObject : this.prevObject.filter(selector)
             );
         };
     }
 
+    /**
+     * The original _setupDynamicStyle uses a very expensive call to
+     * Util.maxZIndex(...) that sets the z-index of .annotator-adder,
+     * .annotator-outer, .annotator-notice, .annotator-filter. We set these
+     * values in annotator.min.css instead and do nothing here.
+     */
+    Annotator.prototype._setupDynamicStyle = function() { };
+
     Annotator.frozenSrc = null;
 
     /**
@@ -90,17 +98,17 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
      **/
     Annotator.Viewer.prototype.html.item = [
         '<li class="annotator-annotation annotator-item">',
-          '<span class="annotator-controls">',
-            '<a href="#" title="', _t('View as webpage'), '" class="annotator-link">',
-                _t('View as webpage'),
-            '</a>',
-            '<button title="', _t('Edit'), '" class="annotator-edit">',
-                _t('Edit'),
-            '</button>',
-            '<button title="', _t('Delete'), '" class="annotator-delete">',
-                _t('Delete'),
-            '</button>',
-          '</span>',
+            '<span class="annotator-controls">',
+                '<a href="#" title="', _t('View as webpage'), '" class="annotator-link">',
+                    _t('View as webpage'),
+                '</a>',
+                '<button title="', _t('Edit'), '" class="annotator-edit">',
+                    _t('Edit'),
+                '</button>',
+                '<button title="', _t('Delete'), '" class="annotator-delete">',
+                    _t('Delete'),
+                '</button>',
+            '</span>',
         '</li>'
     ].join('');
 
@@ -133,7 +141,7 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
             }
         },
 
-        freeze: function() {
+        freeze: function () {
             if (!this.isFrozen) {
                 // Remove default events
                 this.removeEvents();
@@ -144,7 +152,7 @@ define(['jquery', 'underscore', 'annotator'], function ($, _, Annotator) {
             }
         },
 
-        unfreeze: function() {
+        unfreeze: function () {
             if (this.isFrozen) {
                 // Add default events
                 this.addEvents();