From 753d78210a4d6ae31328a392d384a77ed4d15b39 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Fri, 28 Oct 2016 16:49:42 +0500 Subject: [PATCH] single search request to fetch notes for HTML components per unit TNL-4163 --- .../js/edxnotes/plugins/search_override.js | 13 ++ .../js/edxnotes/utils/notes_collector.js | 113 ++++++++++++++++++ lms/static/js/edxnotes/views/notes_factory.js | 12 +- .../plugins/store_error_handler_spec.js | 27 +++-- .../edxnotes/utils/notes_collector_spec.js | 42 +++++++ .../spec/edxnotes/views/notes_factory_spec.js | 36 +++--- .../views/notes_visibility_factory_spec.js | 62 +++++----- lms/static/lms/js/spec/main.js | 1 + 8 files changed, 244 insertions(+), 62 deletions(-) create mode 100644 lms/static/js/edxnotes/plugins/search_override.js create mode 100644 lms/static/js/edxnotes/utils/notes_collector.js create mode 100644 lms/static/js/spec/edxnotes/utils/notes_collector_spec.js diff --git a/lms/static/js/edxnotes/plugins/search_override.js b/lms/static/js/edxnotes/plugins/search_override.js new file mode 100644 index 0000000000..4499b6863e --- /dev/null +++ b/lms/static/js/edxnotes/plugins/search_override.js @@ -0,0 +1,13 @@ +(function(define) { + 'use strict'; + + define(['annotator_1.2.9'], function(Annotator) { + // + // Override Annotator.Plugin.Store.prototype._getAnnotations. We don't want AnnotatorJS to search notes. + // + // eslint-disable-next-line no-param-reassign, no-underscore-dangle + Annotator.Plugin.Store.prototype._getAnnotations = function() { + // Do Nothing + }; + }); +}).call(this, define || RequireJS.define); diff --git a/lms/static/js/edxnotes/utils/notes_collector.js b/lms/static/js/edxnotes/utils/notes_collector.js new file mode 100644 index 0000000000..b45f1d0e82 --- /dev/null +++ b/lms/static/js/edxnotes/utils/notes_collector.js @@ -0,0 +1,113 @@ +(function(define) { + 'use strict'; + define(['jquery', 'underscore', 'annotator_1.2.9'], function($, _, Annotator) { + var cleanup, + renderNotes, + fetchNotesWhenReady, + storeNotesRequestData, + searchRequestsData = []; + + /** + * Clears the searchRequestsData. + */ + cleanup = function() { + searchRequestsData = []; + }; + + /** + * Store requests data for each annotatable component and fetch + * notes for them when request for each component is stored. + * + * @param {object} data Request data for each annotatable component + * @param {Number} totalNotesWrappers Total number of edx notes wrappers present + */ + storeNotesRequestData = function(data, totalNotesWrappers) { + searchRequestsData.push(data); + fetchNotesWhenReady(totalNotesWrappers); + }; + + /** + * Fetch notes for annotatable components only when desired + * number of requests are stored. + * + * @param {Number} totalNotesWrappers Total number of edx notes wrappers present + */ + fetchNotesWhenReady = function(totalNotesWrappers) { + var settings, + usageIds, + searchEndpoint; + + if (totalNotesWrappers !== searchRequestsData.length) { + return; + } + + // `user` and `course_id` values are same for every annotatable + // component so we pick these from first `searchRequestsData` item + settings = { + data: { + user: searchRequestsData[0].params.user, + course_id: searchRequestsData[0].params.courseId + }, + type: 'GET', + dataType: 'json', + headers: {'x-annotator-auth-token': searchRequestsData[0].params.token} + }; + searchEndpoint = searchRequestsData[0].params.endpoint + 'search/?'; + usageIds = _.map(searchRequestsData, function(item) { + return 'usage_id=' + encodeURIComponent(item.params.usageId); + }); + + // Search endpoint expects the below format for query params + // /api/v1/search/?course_id={course_id}&user={user_id}&usage_id={usage_id}&usage_id={usage_id} ... + searchEndpoint += usageIds.join('&'); + + $.ajax(searchEndpoint, settings) + .done(function(jqXHR) { + renderNotes(jqXHR); + }) + .fail(function(jqXHR) { + // `_action` is used by AnnotatorJS to construct error message + jqXHR._action = 'search'; // eslint-disable-line no-underscore-dangle, no-param-reassign + Annotator.Plugin.Store.prototype._onError(jqXHR); // eslint-disable-line no-underscore-dangle + }) + .always(function() { + cleanup(); + }); + }; + + /** + * Pass notes to AnnotatorJS for rendering + * + * @param {Array} notes Notes data received from server + */ + renderNotes = function(notes) { + var edxNotes = {}; + + // AnnotatorJS expects notes to be present in an array named as `rows` + _.each(searchRequestsData, function(item) { + edxNotes[item.params.usageId] = {rows: []}; + }); + + // Place the received notes in the format below + // edxNotes = { + // 'usage_id1': [noteObject, noteObject, noteObject], + // 'usage_id2': [noteObject, noteObject] + // } + _.each(notes, function(note) { + edxNotes[note.usage_id].rows.push(note); + }); + + // Render the notes for each annotatable component using its associated AnnotatorJS instance + _.each(searchRequestsData, function(item) { + item.annotator.plugins.Store._onLoadAnnotationsFromSearch( // eslint-disable-line no-underscore-dangle + edxNotes[item.params.usageId] + ); + }); + }; + + return { + storeNotesRequestData: storeNotesRequestData, + cleanup: cleanup + }; + }); +}).call(this, define || RequireJS.define); diff --git a/lms/static/js/edxnotes/views/notes_factory.js b/lms/static/js/edxnotes/views/notes_factory.js index 5c7bfd0c75..543d8a712b 100644 --- a/lms/static/js/edxnotes/views/notes_factory.js +++ b/lms/static/js/edxnotes/views/notes_factory.js @@ -1,12 +1,14 @@ (function(define, undefined) { 'use strict'; define([ - 'jquery', 'underscore', 'annotator_1.2.9', 'js/edxnotes/utils/logger', + 'jquery', 'underscore', 'annotator_1.2.9', + 'js/edxnotes/utils/logger', 'js/edxnotes/utils/notes_collector', 'js/edxnotes/views/shim', 'js/edxnotes/plugins/scroller', 'js/edxnotes/plugins/events', 'js/edxnotes/plugins/accessibility', 'js/edxnotes/plugins/caret_navigation', - 'js/edxnotes/plugins/store_error_handler' - ], function($, _, Annotator, NotesLogger) { + 'js/edxnotes/plugins/store_error_handler', + 'js/edxnotes/plugins/search_override' + ], function($, _, Annotator, NotesLogger, NotesCollector) { var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation', 'Tags'], getOptions, setupPlugins, getAnnotator; @@ -84,6 +86,10 @@ annotator = el.annotator(options).data('annotator'); setupPlugins(annotator, plugins, options); + NotesCollector.storeNotesRequestData( + {annotator: annotator, params: params}, + $('.edx-notes-wrapper').length + ); annotator.logger = logger; logger.log({'element': element, 'options': options}); return annotator; diff --git a/lms/static/js/spec/edxnotes/plugins/store_error_handler_spec.js b/lms/static/js/spec/edxnotes/plugins/store_error_handler_spec.js index 78cf4f73f4..779aefb019 100644 --- a/lms/static/js/spec/edxnotes/plugins/store_error_handler_spec.js +++ b/lms/static/js/spec/edxnotes/plugins/store_error_handler_spec.js @@ -2,30 +2,35 @@ define([ 'jquery', 'underscore', 'annotator_1.2.9', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers', 'js/spec/edxnotes/helpers', - 'js/edxnotes/views/notes_factory' -], function($, _, Annotator, AjaxHelpers, Helpers, NotesFactory) { + 'js/edxnotes/views/notes_factory', + 'js/edxnotes/utils/notes_collector' +], function($, _, Annotator, AjaxHelpers, Helpers, NotesFactory, NotesCollector) { 'use strict'; describe('Store Error Handler Custom Message', function() { beforeEach(function() { spyOn(Annotator, 'showNotification'); loadFixtures('js/fixtures/edxnotes/edxnotes_wrapper.html'); - this.wrapper = document.getElementById('edx-notes-wrapper-123'); + NotesCollector.cleanup(); }); afterEach(function() { - _.invoke(Annotator._instances, 'destroy'); + while (Annotator._instances.length > 0) { // eslint-disable-line no-underscore-dangle + Annotator._instances[0].destroy(); // eslint-disable-line no-underscore-dangle + } }); it('can handle custom error if sent from server', function() { var requests = AjaxHelpers.requests(this); var token = Helpers.makeToken(); - NotesFactory.factory(this.wrapper, { - endpoint: '/test_endpoint', - user: 'a user', - usageId: 'an usage', - courseId: 'a course', - token: token, - tokenUrl: '/test_token_url' + _.each($('.edx-notes-wrapper'), function(wrapper) { + NotesFactory.factory(wrapper, { + endpoint: '/test_endpoint', + user: 'a user', + usageId: 'an usage', + courseId: 'a course', + token: token, + tokenUrl: '/test_token_url' + }); }); var errorMsg = 'can\'t create more notes'; diff --git a/lms/static/js/spec/edxnotes/utils/notes_collector_spec.js b/lms/static/js/spec/edxnotes/utils/notes_collector_spec.js new file mode 100644 index 0000000000..506fa768e6 --- /dev/null +++ b/lms/static/js/spec/edxnotes/utils/notes_collector_spec.js @@ -0,0 +1,42 @@ +define([ + 'jquery', 'underscore', 'annotator_1.2.9', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers', + 'js/edxnotes/views/notes_factory', 'js/edxnotes/utils/notes_collector', 'js/spec/edxnotes/helpers' +], function( + $, _, Annotator, AjaxHelpers, NotesFactory, NotesCollector, Helpers +) { + 'use strict'; + describe('EdxNotes NotesCollector', function() { + beforeEach(function() { + loadFixtures('js/fixtures/edxnotes/edxnotes_wrapper.html'); + NotesCollector.cleanup(); + }); + + afterEach(function() { + while (Annotator._instances.length > 0) { // eslint-disable-line no-underscore-dangle + Annotator._instances[0].destroy(); // eslint-disable-line no-underscore-dangle + } + NotesCollector.cleanup(); + }); + + it('sends single search request to fetch notes for all HTML components', function() { + var requests = AjaxHelpers.requests(this), + token = Helpers.makeToken(); + + _.each($('.edx-notes-wrapper'), function(wrapper, index) { + NotesFactory.factory(wrapper, { + endpoint: '/test_endpoint/', + user: 'a user', + usageId: 'usage ' + index, + courseId: 'a course', + token: token, + tokenUrl: '/test_token_url' + }); + }); + + expect(requests.length).toBe(1); + AjaxHelpers.expectJsonRequest(requests, 'GET', + '/test_endpoint/search/?usage_id=usage%200&usage_id=usage%201&user=a+user&course_id=a+course' + ); + }); + }); +}); diff --git a/lms/static/js/spec/edxnotes/views/notes_factory_spec.js b/lms/static/js/spec/edxnotes/views/notes_factory_spec.js index f9e68f172c..5472d1046e 100644 --- a/lms/static/js/spec/edxnotes/views/notes_factory_spec.js +++ b/lms/static/js/spec/edxnotes/views/notes_factory_spec.js @@ -1,12 +1,12 @@ define([ - 'annotator_1.2.9', 'js/edxnotes/views/notes_factory', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers', - 'js/spec/edxnotes/helpers' -], function(Annotator, NotesFactory, AjaxHelpers, Helpers) { + 'jquery', 'underscore', 'annotator_1.2.9', 'js/edxnotes/views/notes_factory', + 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers', 'js/edxnotes/utils/notes_collector', 'js/spec/edxnotes/helpers' +], function($, _, Annotator, NotesFactory, AjaxHelpers, NotesCollector, Helpers) { 'use strict'; describe('EdxNotes NotesFactory', function() { beforeEach(function() { loadFixtures('js/fixtures/edxnotes/edxnotes_wrapper.html'); - this.wrapper = document.getElementById('edx-notes-wrapper-123'); + NotesCollector.cleanup(); }); afterEach(function() { @@ -15,30 +15,36 @@ define([ } }); - it('can initialize annotator correctly', function() { + it('can initialize annotator correctly', function(done) { var requests = AjaxHelpers.requests(this), token = Helpers.makeToken(), options = { user: 'a user', usage_id: 'an usage', course_id: 'a course' - }, - annotator = NotesFactory.factory(this.wrapper, { + }; + + _.each($('.edx-notes-wrapper'), function(wrapper) { + var annotator = NotesFactory.factory(wrapper, { endpoint: '/test_endpoint', user: 'a user', usageId: 'an usage', courseId: 'a course', token: token, tokenUrl: '/test_token_url' - }), - request = requests[0]; + }); - expect(requests).toHaveLength(1); - expect(request.requestHeaders['x-annotator-auth-token']).toBe(token); - expect(annotator.options.auth.tokenUrl).toBe('/test_token_url'); - expect(annotator.options.store.prefix).toBe('/test_endpoint'); - expect(annotator.options.store.annotationData).toEqual(options); - expect(annotator.options.store.loadFromSearch).toEqual(options); + expect(annotator.options.auth.tokenUrl).toBe('/test_token_url'); + expect(annotator.options.store.prefix).toBe('/test_endpoint'); + expect(annotator.options.store.annotationData).toEqual(options); + expect(annotator.options.store.loadFromSearch).toEqual(options); + }); + jasmine.waitUntil(function() { + return requests.length === 1; + }).done(function() { + expect(requests[0].requestHeaders['x-annotator-auth-token']).toBe(token); + done(); + }); }); }); }); diff --git a/lms/static/js/spec/edxnotes/views/notes_visibility_factory_spec.js b/lms/static/js/spec/edxnotes/views/notes_visibility_factory_spec.js index 0d34846c5b..8f5cdf4789 100644 --- a/lms/static/js/spec/edxnotes/views/notes_visibility_factory_spec.js +++ b/lms/static/js/spec/edxnotes/views/notes_visibility_factory_spec.js @@ -1,16 +1,16 @@ define([ 'jquery', 'underscore', 'annotator_1.2.9', 'edx-ui-toolkit/js/utils/spec-helpers/ajax-helpers', - 'js/edxnotes/views/notes_visibility_factory', 'js/spec/edxnotes/helpers' + 'js/edxnotes/views/notes_visibility_factory', 'js/edxnotes/utils/notes_collector', 'js/spec/edxnotes/helpers' ], function( - $, _, Annotator, AjaxHelpers, NotesVisibilityFactory, Helpers + $, _, Annotator, AjaxHelpers, NotesVisibilityFactory, NotesCollector, Helpers ) { 'use strict'; describe('EdxNotes ToggleNotesFactory', function() { var params = { - endpoint: '/test_endpoint', - user: 'a user', - usageId: 'an usage', - courseId: 'a course', + endpoint: '/test_endpoint/', + user: 'user12345', + usageId: 'usageid777', + courseId: 'courseid000', token: Helpers.makeToken(), tokenUrl: '/test_token_url' }; @@ -27,10 +27,11 @@ define([ document.getElementById('edx-notes-wrapper-456'), params, true ); this.toggleNotes = NotesVisibilityFactory.ToggleVisibilityView(true, '/test_url'); - this.button = $('.action-toggle-notes'); - this.label = this.button.find('.utility-control-label'); + this.toggleVisibilityButton = $('.action-toggle-notes'); + this.label = this.toggleVisibilityButton.find('.utility-control-label'); this.toggleMessage = $('.action-toggle-message'); spyOn(this.toggleNotes, 'toggleHandler').and.callThrough(); + NotesCollector.cleanup(); }); afterEach(function() { @@ -39,49 +40,45 @@ define([ Annotator._instances[0].destroy(); } $('.annotator-notice').remove(); + NotesCollector.cleanup(); }); it('can toggle notes', function() { var requests = AjaxHelpers.requests(this); - expect(this.button).not.toHaveClass('is-disabled'); + expect(this.toggleVisibilityButton).not.toHaveClass('is-disabled'); expect(this.label).toContainText('Hide notes'); - expect(this.button).toHaveClass('is-active'); - expect(this.button).toHaveAttr('aria-pressed', 'true'); + expect(this.toggleVisibilityButton).toHaveClass('is-active'); + expect(this.toggleVisibilityButton).toHaveAttr('aria-pressed', 'true'); expect(this.toggleMessage).not.toHaveClass('is-fleeting'); expect(this.toggleMessage).toContainText('Notes visible'); - this.button.click(); + this.toggleVisibilityButton.click(); expect(this.label).toContainText('Show notes'); - expect(this.button).not.toHaveClass('is-active'); + expect(this.toggleVisibilityButton).not.toHaveClass('is-active'); expect(this.toggleMessage).toHaveClass('is-fleeting'); expect(this.toggleMessage).toContainText('Notes hidden'); expect(Annotator._instances).toHaveLength(0); AjaxHelpers.expectJsonRequest(requests, 'PUT', '/test_url', { - 'visibility': false + visibility: false }); AjaxHelpers.respondWithJson(requests, {}); - this.button.click(); + this.toggleVisibilityButton.click(); expect(this.label).toContainText('Hide notes'); - expect(this.button).toHaveClass('is-active'); + expect(this.toggleVisibilityButton).toHaveClass('is-active'); expect(this.toggleMessage).toHaveClass('is-fleeting'); expect(this.toggleMessage).toContainText('Notes visible'); expect(Annotator._instances).toHaveLength(2); - // TODO: why is the same search request made twice? AjaxHelpers.expectJsonRequest(requests, 'GET', - '/test_endpoint/search/?user=a+user&usage_id=an+usage&course_id=a+course' + '/test_endpoint/search/?usage_id=usageid777&usage_id=usageid777&user=user12345&course_id=courseid000' ); - AjaxHelpers.respondWithJson(requests, {}); - AjaxHelpers.expectJsonRequest(requests, 'GET', - '/test_endpoint/search/?user=a+user&usage_id=an+usage&course_id=a+course' - ); - AjaxHelpers.respondWithJson(requests, {}); + AjaxHelpers.respondWithJson(requests, []); AjaxHelpers.expectJsonRequest(requests, 'PUT', '/test_url', { - 'visibility': true + visibility: true }); AjaxHelpers.respondWithJson(requests, {}); }); @@ -90,7 +87,7 @@ define([ var requests = AjaxHelpers.requests(this), $errorContainer = $('.annotator-notice'); - this.button.click(); + this.toggleVisibilityButton.click(); AjaxHelpers.respondWithError(requests); expect($errorContainer).toContainText( 'An error has occurred. Make sure that you are connected to the Internet, ' + @@ -100,19 +97,18 @@ define([ expect($errorContainer).toHaveClass('annotator-notice-show'); expect($errorContainer).toHaveClass('annotator-notice-error'); - this.button.click(); + this.toggleVisibilityButton.click(); - // TODO: why is the same search request made twice? AjaxHelpers.expectJsonRequest(requests, 'GET', - '/test_endpoint/search/?user=a+user&usage_id=an+usage&course_id=a+course' - ); - AjaxHelpers.respondWithJson(requests, {}); - AjaxHelpers.expectJsonRequest(requests, 'GET', - '/test_endpoint/search/?user=a+user&usage_id=an+usage&course_id=a+course' + '/test_endpoint/search/?usage_id=usageid777&usage_id=usageid777&user=user12345&course_id=courseid000' ); + AjaxHelpers.respondWithJson(requests, []); + + AjaxHelpers.expectJsonRequest(requests, 'PUT', '/test_url', { + visibility: true + }); AjaxHelpers.respondWithJson(requests, {}); - AjaxHelpers.respondWithJson(requests, {}); expect($errorContainer).not.toHaveClass('annotator-notice-show'); }); diff --git a/lms/static/lms/js/spec/main.js b/lms/static/lms/js/spec/main.js index 2a4b77f506..67e4f40cc5 100644 --- a/lms/static/lms/js/spec/main.js +++ b/lms/static/lms/js/spec/main.js @@ -701,6 +701,7 @@ 'js/spec/edxnotes/plugins/scroller_spec.js', 'js/spec/edxnotes/plugins/store_error_handler_spec.js', 'js/spec/edxnotes/utils/logger_spec.js', + 'js/spec/edxnotes/utils/notes_collector_spec.js', 'js/spec/edxnotes/views/note_item_spec.js', 'js/spec/edxnotes/views/notes_factory_spec.js', 'js/spec/edxnotes/views/notes_page_spec.js',