Commit 71cc602d by Muhammad Ammar Committed by GitHub

Merge pull request #13863 from edx/ammar/tnl-4163-reduce-annotatorjs-search-calls

Single search request to fetch notes per unit
parents 3224c027 237ef00a
......@@ -243,8 +243,9 @@ class StubEdxNotesServiceHandler(StubHttpRequestHandler):
"""
Search for a notes by user id, course_id and usage_id.
"""
search_with_usage_id = False
user = self.get_params.get("user", None)
usage_id = self.get_params.get("usage_id", None)
usage_ids = self.get_params.get("usage_id", [])
course_id = self.get_params.get("course_id", None)
text = self.get_params.get("text", None)
page = int(self.get_params.get("page", 1))
......@@ -257,11 +258,14 @@ class StubEdxNotesServiceHandler(StubHttpRequestHandler):
notes = self.server.get_all_notes()
if course_id is not None:
notes = self.server.filter_by_course_id(notes, course_id)
if usage_id is not None:
notes = self.server.filter_by_usage_id(notes, usage_id)
if len(usage_ids) > 0:
search_with_usage_id = True
notes = self.server.filter_by_usage_id(notes, usage_ids)
if text:
notes = self.server.search(notes, text)
self.respond(content=self._get_paginated_response(notes, page, page_size))
if not search_with_usage_id:
notes = self._get_paginated_response(notes, page, page_size)
self.respond(content=notes)
def _collection(self):
"""
......@@ -356,11 +360,13 @@ class StubEdxNotesService(StubHttpService):
"""
return self.filter_by(data, "user", user)
def filter_by_usage_id(self, data, usage_id):
def filter_by_usage_id(self, data, usage_ids):
"""
Filters provided `data(list)` by the `usage_id(str)`.
"""
return self.filter_by(data, "usage_id", usage_id)
if not isinstance(usage_ids, list):
usage_ids = [usage_ids]
return self.filter_by_list(data, "usage_id", usage_ids)
def filter_by_course_id(self, data, course_id):
"""
......@@ -374,6 +380,12 @@ class StubEdxNotesService(StubHttpService):
"""
return [note for note in data if note.get(field_name) == value]
def filter_by_list(self, data, field_name, values):
"""
Filters provided `data(list)` by the `field_name(str)` in values.
"""
return [note for note in data if note.get(field_name) in values]
def search(self, data, query):
"""
Search the `query(str)` text in the provided `data(list)`.
......
"""
Unit tests for stub EdxNotes implementation.
"""
import ddt
import urlparse
import json
import unittest
......@@ -9,6 +10,7 @@ from uuid import uuid4
from ..edxnotes import StubEdxNotesService
@ddt.ddt
class StubEdxNotesServiceTest(unittest.TestCase):
"""
Test cases for the stub EdxNotes service.
......@@ -27,9 +29,9 @@ class StubEdxNotesServiceTest(unittest.TestCase):
"""
Returns a list of dummy notes.
"""
return [self._get_dummy_note() for i in xrange(count)] # pylint: disable=unused-variable
return [self._get_dummy_note(i) for i in xrange(count)] # pylint: disable=unused-variable
def _get_dummy_note(self):
def _get_dummy_note(self, uid=0):
"""
Returns a single dummy note.
"""
......@@ -39,7 +41,7 @@ class StubEdxNotesServiceTest(unittest.TestCase):
"created": "2014-10-31T10:05:00.000000",
"updated": "2014-10-31T10:50:00.101010",
"user": "dummy-user-id",
"usage_id": "dummy-usage-id",
"usage_id": "dummy-usage-id-" + str(uid),
"course_id": "dummy-course-id",
"text": "dummy note text " + nid,
"quote": "dummy note quote",
......@@ -106,7 +108,6 @@ class StubEdxNotesServiceTest(unittest.TestCase):
# get response with default page and page size
response = requests.get(self._get_url("api/v1/search"), params={
"user": "dummy-user-id",
"usage_id": "dummy-usage-id",
"course_id": "dummy-course-id",
})
......@@ -125,7 +126,6 @@ class StubEdxNotesServiceTest(unittest.TestCase):
# search notes with text that don't exist
response = requests.get(self._get_url("api/v1/search"), params={
"user": "dummy-user-id",
"usage_id": "dummy-usage-id",
"course_id": "dummy-course-id",
"text": "world war 2"
})
......@@ -142,6 +142,28 @@ class StubEdxNotesServiceTest(unittest.TestCase):
previous_page=None
)
@ddt.data(
'?usage_id=dummy-usage-id-0',
'?usage_id=dummy-usage-id-0&usage_id=dummy-usage-id-1&dummy-usage-id-2&dummy-usage-id-3&dummy-usage-id-4'
)
def test_search_usage_ids(self, usage_ids):
"""
Test search with usage ids.
"""
url = self._get_url('api/v1/search') + usage_ids
response = requests.get(url, params={
'user': 'dummy-user-id',
'course_id': 'dummy-course-id'
})
self.assertTrue(response.ok)
response = response.json()
parsed = urlparse.urlparse(url)
query_params = urlparse.parse_qs(parsed.query)
query_params['usage_id'].reverse()
self.assertEqual(len(response), len(query_params['usage_id']))
for index, usage_id in enumerate(query_params['usage_id']):
self.assertEqual(response[index]['usage_id'], usage_id)
def test_delete(self):
notes = self._get_notes()
response = requests.delete(self._get_url("api/v1/annotations/does_not_exist"))
......
(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);
(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);
(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;
......
......@@ -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';
......
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'
);
});
});
});
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();
});
});
});
});
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'
'/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
});
AjaxHelpers.respondWithJson(requests, {});
expect($errorContainer).not.toHaveClass('annotator-notice-show');
});
......
......@@ -712,6 +712,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',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment