From e2333214b1e95acc4da0fb17390432a1edc83e17 Mon Sep 17 00:00:00 2001 From: muhammad-ammar Date: Wed, 9 Dec 2015 18:03:30 +0500 Subject: [PATCH 01/12] paginate edxnotes views TNL-3840 --- lms/djangoapps/edxnotes/helpers.py | 116 +++++++-- lms/djangoapps/edxnotes/tests.py | 349 +++++++++++++++++---------- lms/djangoapps/edxnotes/urls.py | 2 +- lms/djangoapps/edxnotes/views.py | 109 +++++++-- lms/templates/edxnotes/edxnotes.html | 2 +- 5 files changed, 407 insertions(+), 171 deletions(-) diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index 61cd678192..497b57355b 100644 --- a/lms/djangoapps/edxnotes/helpers.py +++ b/lms/djangoapps/edxnotes/helpers.py @@ -1,11 +1,12 @@ """ Helper methods related to EdxNotes. """ - import json import logging from json import JSONEncoder from uuid import uuid4 +import urlparse +from urllib import urlencode import requests from datetime import datetime @@ -34,6 +35,8 @@ HIGHLIGHT_TAG = "span" HIGHLIGHT_CLASS = "note-highlight" # OAuth2 Client name for edxnotes CLIENT_NAME = "edx-notes" +DEFAULT_PAGE = 1 +DEFAULT_PAGE_SIZE = 10 class NoteJSONEncoder(JSONEncoder): @@ -63,19 +66,32 @@ def get_token_url(course_id): }) -def send_request(user, course_id, path="", query_string=None): +def send_request(user, course_id, page, page_size, path="", text=None): """ - Sends a request with appropriate parameters and headers. + Sends a request to notes api with appropriate parameters and headers. + + Arguments: + user: Current logged in user + course_id: Course id + page: requested or default page number + page_size: requested or default page size + path: `search` or `annotations`. This is used to calculate notes api endpoint. + text: text to search. + + Returns: + Response received from notes api """ url = get_internal_endpoint(path) params = { "user": anonymous_id_for_user(user, None), "course_id": unicode(course_id).encode("utf-8"), + "page": page, + "page_size": page_size, } - if query_string: + if text: params.update({ - "text": query_string, + "text": text, "highlight": True, "highlight_tag": HIGHLIGHT_TAG, "highlight_class": HIGHLIGHT_CLASS, @@ -239,39 +255,89 @@ def get_index(usage_key, children): return children.index(usage_key) -def search(user, course, query_string): +def construct_pagination_urls(request, course_id, api_next_url, api_previous_url): """ - Returns search results for the `query_string(str)`. + Construct next and previous urls for LMS. `api_next_url` and `api_previous_url` + are returned from notes api but we need to transform them according to LMS notes + views by removing and replacing extra information. + + Arguments: + request: HTTP request object + course_id: course id + api_next_url: notes api next url + api_previous_url: notes api previous url + + Returns: + next_url: lms notes next url + previous_url: lms notes previous url """ - response = send_request(user, course.id, "search", query_string) - try: - content = json.loads(response.content) - collection = content["rows"] - except (ValueError, KeyError): - log.warning("invalid JSON: %s", response.content) - raise EdxNotesParseError(_("Server error. Please try again in a few minutes.")) + def lms_url(url): + """ + Create lms url from api url. + """ + if url is None: + return None - content.update({ - "rows": preprocess_collection(user, course, collection) - }) + keys = ('page', 'page_size', 'text') + parsed = urlparse.urlparse(url) + query_params = urlparse.parse_qs(parsed.query) - return json.dumps(content, cls=NoteJSONEncoder) + encoded_query_params = urlencode({key: query_params.get(key)[0] for key in keys if key in query_params}) + return "{}?{}".format(request.build_absolute_uri(base_url), encoded_query_params) + + base_url = reverse("notes", kwargs={"course_id": course_id}) + next_url = lms_url(api_next_url) + previous_url = lms_url(api_previous_url) + + return next_url, previous_url -def get_notes(user, course): +def get_notes(request, course, page=DEFAULT_PAGE, page_size=DEFAULT_PAGE_SIZE, text=None): """ - Returns all notes for the user. + Returns paginated list of notes for the user. + + Arguments: + request: HTTP request object + course: Course descriptor + page: requested or default page number + page_size: requested or default page size + text: text to search. If None then return all results for the current logged in user. + + Returns: + Paginated dictionary with these key: + start: start of the current page + current_page: current page number + next: url for next page + previous: url for previous page + count: total number of notes available for the sent query + num_pages: number of pages available + results: list with notes info dictionary. each item in this list will be a dict """ - response = send_request(user, course.id, "annotations") + path = 'search' if text else 'annotations' + response = send_request(request.user, course.id, page, page_size, path, text) + try: collection = json.loads(response.content) except ValueError: - return None + raise EdxNotesParseError(_("Invalid response received from notes api.")) - if not collection: - return None + # Verify response dict structure + expected_keys = ['count', 'results', 'num_pages', 'start', 'next', 'previous', 'current_page'] + keys = collection.keys() + if not keys or not all(key in expected_keys for key in keys): + raise EdxNotesParseError(_("Invalid response received from notes api.")) - return json.dumps(preprocess_collection(user, course, collection), cls=NoteJSONEncoder) + filtered_results = preprocess_collection(request.user, course, collection['results']) + collection['results'] = filtered_results + + collection['next'], collection['previous'] = construct_pagination_urls( + request, + course.id, + collection['next'], + collection['previous'] + ) + + return json.dumps(collection, cls=NoteJSONEncoder) def get_endpoint(api_url, path=""): diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index be7637667f..f1669df012 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -8,6 +8,7 @@ import jwt from mock import patch, MagicMock from unittest import skipUnless from datetime import datetime +import urlparse from edxmako.shortcuts import render_to_string from edxnotes import helpers @@ -31,6 +32,17 @@ from courseware.tabs import get_course_tab_list from student.tests.factories import UserFactory, CourseEnrollmentFactory +NOTES_API_EMPTY_RESPONSE = { + "count": 0, + "results": [], + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 0, +} + + def enable_edxnotes_for_the_course(course, user_id): """ Enable EdxNotes for the course. @@ -191,6 +203,9 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): self.user = UserFactory.create(username="Joe", email="joe@example.com", password="edx") self.client.login(username=self.user.username, password="edx") + self.request = RequestFactory().request() + self.request.user = self.user + def _get_unit_url(self, course, chapter, section, position=1): """ Returns `jump_to_id` url for the `vertical`. @@ -280,71 +295,91 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): """ Tests the result if correct data is received. """ - mock_get.return_value.content = json.dumps([ + mock_get.return_value.content = json.dumps( { - u"quote": u"quote text", - u"text": u"text", - u"usage_id": unicode(self.html_module_1.location), - u"updated": datetime(2014, 11, 19, 8, 5, 16, 00000).isoformat(), - }, - { - u"quote": u"quote text", - u"text": u"text", - u"usage_id": unicode(self.html_module_2.location), - u"updated": datetime(2014, 11, 19, 8, 6, 16, 00000).isoformat(), + "count": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, + "results": [ + { + u"quote": u"quote text", + u"text": u"text", + u"usage_id": unicode(self.html_module_1.location), + u"updated": datetime(2014, 11, 19, 8, 5, 16, 00000).isoformat(), + }, + { + u"quote": u"quote text", + u"text": u"text", + u"usage_id": unicode(self.html_module_2.location), + u"updated": datetime(2014, 11, 19, 8, 6, 16, 00000).isoformat(), + } + ] } - ]) + ) self.assertItemsEqual( - [ - { - u"quote": u"quote text", - u"text": u"text", - u"chapter": { - u"display_name": self.chapter.display_name_with_default_escaped, - u"index": 0, - u"location": unicode(self.chapter.location), - u"children": [unicode(self.sequential.location)] + { + "count": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, + "results": [ + { + u"quote": u"quote text", + u"text": u"text", + u"chapter": { + u"display_name": self.chapter.display_name_with_default_escaped, + u"index": 0, + u"location": unicode(self.chapter.location), + u"children": [unicode(self.sequential.location)] + }, + u"section": { + u"display_name": self.sequential.display_name_with_default_escaped, + u"location": unicode(self.sequential.location), + u"children": [ + unicode(self.vertical.location), unicode(self.vertical_with_container.location) + ] + }, + u"unit": { + u"url": self._get_unit_url(self.course, self.chapter, self.sequential), + u"display_name": self.vertical.display_name_with_default_escaped, + u"location": unicode(self.vertical.location), + }, + u"usage_id": unicode(self.html_module_2.location), + u"updated": "Nov 19, 2014 at 08:06 UTC", }, - u"section": { - u"display_name": self.sequential.display_name_with_default_escaped, - u"location": unicode(self.sequential.location), - u"children": [unicode(self.vertical.location), unicode(self.vertical_with_container.location)] + { + u"quote": u"quote text", + u"text": u"text", + u"chapter": { + u"display_name": self.chapter.display_name_with_default_escaped, + u"index": 0, + u"location": unicode(self.chapter.location), + u"children": [unicode(self.sequential.location)] + }, + u"section": { + u"display_name": self.sequential.display_name_with_default_escaped, + u"location": unicode(self.sequential.location), + u"children": [ + unicode(self.vertical.location), + unicode(self.vertical_with_container.location)] + }, + u"unit": { + u"url": self._get_unit_url(self.course, self.chapter, self.sequential), + u"display_name": self.vertical.display_name_with_default_escaped, + u"location": unicode(self.vertical.location), + }, + u"usage_id": unicode(self.html_module_1.location), + u"updated": "Nov 19, 2014 at 08:05 UTC", }, - u"unit": { - u"url": self._get_unit_url(self.course, self.chapter, self.sequential), - u"display_name": self.vertical.display_name_with_default_escaped, - u"location": unicode(self.vertical.location), - }, - u"usage_id": unicode(self.html_module_2.location), - u"updated": "Nov 19, 2014 at 08:06 UTC", - }, - { - u"quote": u"quote text", - u"text": u"text", - u"chapter": { - u"display_name": self.chapter.display_name_with_default_escaped, - u"index": 0, - u"location": unicode(self.chapter.location), - u"children": [unicode(self.sequential.location)] - }, - u"section": { - u"display_name": self.sequential.display_name_with_default_escaped, - u"location": unicode(self.sequential.location), - u"children": [ - unicode(self.vertical.location), - unicode(self.vertical_with_container.location)] - }, - u"unit": { - u"url": self._get_unit_url(self.course, self.chapter, self.sequential), - u"display_name": self.vertical.display_name_with_default_escaped, - u"location": unicode(self.vertical.location), - }, - u"usage_id": unicode(self.html_module_1.location), - u"updated": "Nov 19, 2014 at 08:05 UTC", - }, - ], - json.loads(helpers.get_notes(self.user, self.course)) + ] + }, + json.loads(helpers.get_notes(self.request, self.course)) ) @patch("edxnotes.helpers.requests.get", autospec=True) @@ -353,15 +388,15 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): Tests the result if incorrect json is received. """ mock_get.return_value.content = "Error" - self.assertIsNone(helpers.get_notes(self.user, self.course)) + self.assertRaises(EdxNotesParseError, helpers.get_notes, self.request, self.course) @patch("edxnotes.helpers.requests.get", autospec=True) def test_get_notes_empty_collection(self, mock_get): """ - Tests the result if an empty collection is received. + Tests the result if an empty response is received. """ - mock_get.return_value.content = json.dumps([]) - self.assertIsNone(helpers.get_notes(self.user, self.course)) + mock_get.return_value.content = json.dumps({}) + self.assertRaises(EdxNotesParseError, helpers.get_notes, self.request, self.course) @patch("edxnotes.helpers.requests.get", autospec=True) def test_search_correct_data(self, mock_get): @@ -369,8 +404,13 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): Tests the result if correct data is received. """ mock_get.return_value.content = json.dumps({ - "total": 2, - "rows": [ + "count": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, + "results": [ { u"quote": u"quote text", u"text": u"text", @@ -388,8 +428,13 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): self.assertItemsEqual( { - "total": 2, - "rows": [ + "count": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, + "results": [ { u"quote": u"quote text", u"text": u"text", @@ -440,7 +485,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): }, ] }, - json.loads(helpers.search(self.user, self.course, "test")) + json.loads(helpers.get_notes(self.request, self.course)) ) @patch("edxnotes.helpers.requests.get", autospec=True) @@ -449,7 +494,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): Tests the result if incorrect json is received. """ mock_get.return_value.content = "Error" - self.assertRaises(EdxNotesParseError, helpers.search, self.user, self.course, "test") + self.assertRaises(EdxNotesParseError, helpers.get_notes, self.request, self.course) @patch("edxnotes.helpers.requests.get", autospec=True) def test_search_wrong_data_format(self, mock_get): @@ -457,23 +502,17 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): Tests the result if incorrect data structure is received. """ mock_get.return_value.content = json.dumps({"1": 2}) - self.assertRaises(EdxNotesParseError, helpers.search, self.user, self.course, "test") + self.assertRaises(EdxNotesParseError, helpers.get_notes, self.request, self.course) @patch("edxnotes.helpers.requests.get", autospec=True) def test_search_empty_collection(self, mock_get): """ Tests no results. """ - mock_get.return_value.content = json.dumps({ - "total": 0, - "rows": [] - }) + mock_get.return_value.content = json.dumps(NOTES_API_EMPTY_RESPONSE) self.assertItemsEqual( - { - "total": 0, - "rows": [] - }, - json.loads(helpers.search(self.user, self.course, "test")) + NOTES_API_EMPTY_RESPONSE, + json.loads(helpers.get_notes(self.request, self.course)) ) def test_preprocess_collection_escaping(self): @@ -693,14 +732,19 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): @patch("edxnotes.helpers.anonymous_id_for_user", autospec=True) @patch("edxnotes.helpers.get_edxnotes_id_token", autospec=True) @patch("edxnotes.helpers.requests.get", autospec=True) - def test_send_request_with_query_string(self, mock_get, mock_get_id_token, mock_anonymous_id_for_user): + def test_send_request_with_text_param(self, mock_get, mock_get_id_token, mock_anonymous_id_for_user): """ Tests that requests are send with correct information. """ mock_get_id_token.return_value = "test_token" mock_anonymous_id_for_user.return_value = "anonymous_id" helpers.send_request( - self.user, self.course.id, path="test", query_string="text" + self.user, + self.course.id, + path="test", + text="text", + page=helpers.DEFAULT_PAGE, + page_size=helpers.DEFAULT_PAGE_SIZE ) mock_get.assert_called_with( "http://example.com/test/", @@ -714,6 +758,8 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): "highlight": True, "highlight_tag": "span", "highlight_class": "note-highlight", + 'page': 1, + 'page_size': 10, } ) @@ -722,14 +768,14 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): @patch("edxnotes.helpers.anonymous_id_for_user", autospec=True) @patch("edxnotes.helpers.get_edxnotes_id_token", autospec=True) @patch("edxnotes.helpers.requests.get", autospec=True) - def test_send_request_without_query_string(self, mock_get, mock_get_id_token, mock_anonymous_id_for_user): + def test_send_request_without_text_param(self, mock_get, mock_get_id_token, mock_anonymous_id_for_user): """ Tests that requests are send with correct information. """ mock_get_id_token.return_value = "test_token" mock_anonymous_id_for_user.return_value = "anonymous_id" helpers.send_request( - self.user, self.course.id, path="test" + self.user, self.course.id, path="test", page=1, page_size=10 ) mock_get.assert_called_with( "http://example.com/test/", @@ -739,6 +785,8 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): params={ "user": "anonymous_id", "course_id": unicode(self.course.id), + 'page': helpers.DEFAULT_PAGE, + 'page_size': helpers.DEFAULT_PAGE_SIZE, } ) @@ -808,8 +856,69 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): self.assertEqual(0, helpers.get_index(unicode(self.vertical.location), children)) self.assertEqual(1, helpers.get_index(unicode(self.vertical_with_container.location), children)) + @ddt.unpack + @ddt.data( + {'previous_api_url': None, 'next_api_url': None}, + {'previous_api_url': None, 'next_api_url': 'edxnotes/?course_id=abc&page=2&page_size=10&user=123'}, + {'previous_api_url': 'edxnotes.org/?course_id=abc&page=2&page_size=10&user=123', 'next_api_url': None}, + { + 'previous_api_url': 'edxnotes.org/?course_id=abc&page_size=10&user=123', + 'next_api_url': 'edxnotes.org/?course_id=abc&page=3&page_size=10&user=123' + }, + { + 'previous_api_url': 'edxnotes.org/?course_id=abc&page=2&page_size=10&text=wow&user=123', + 'next_api_url': 'edxnotes.org/?course_id=abc&page=4&page_size=10&text=wow&user=123' + }, + ) + def test_construct_url(self, previous_api_url, next_api_url): + """ + Verify that `construct_url` works correctly. + """ + # make absolute url + # pylint: disable=no-member + if self.request.is_secure(): + host = 'https://' + self.request.get_host() + else: + host = 'http://' + self.request.get_host() + notes_url = host + reverse("notes", args=[unicode(self.course.id)]) + + def verify_url(constructed, expected): + """ + Verify that constructed url is correct. + """ + # if api url is None then constructed url should also be None + if expected is None: + self.assertEqual(expected, constructed) + else: + # constructed url should startswith notes view url instead of api view url + self.assertTrue(constructed.startswith(notes_url)) + + # constructed url should not contain extra params + self.assertNotIn('user', constructed) + + # constructed url should only has these params if present in api url + allowed_params = ('page', 'page_size', 'text') + + # extract query params from constructed url + parsed = urlparse.urlparse(constructed) + params = urlparse.parse_qs(parsed.query) + + # verify that constructed url has only correct params and params have correct values + for param, value in params.items(): + self.assertIn(param, allowed_params) + self.assertIn('{}={}'.format(param, value[0]), expected) + + next_url, previous_url = helpers.construct_pagination_urls( + self.request, + self.course.id, + next_api_url, previous_api_url + ) + verify_url(next_url, next_api_url) + verify_url(previous_url, previous_api_url) + @skipUnless(settings.FEATURES["ENABLE_EDXNOTES"], "EdxNotes feature needs to be enabled.") +@ddt.ddt class EdxNotesViewsTest(ModuleStoreTestCase): """ Tests for EdxNotes views. @@ -822,7 +931,7 @@ class EdxNotesViewsTest(ModuleStoreTestCase): CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) self.client.login(username=self.user.username, password="edx") self.notes_page_url = reverse("edxnotes", args=[unicode(self.course.id)]) - self.search_url = reverse("search_notes", args=[unicode(self.course.id)]) + self.notes_url = reverse("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)]) @@ -858,7 +967,7 @@ class EdxNotesViewsTest(ModuleStoreTestCase): # pylint: disable=unused-argument @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.get_notes", return_value=[]) + @patch("edxnotes.views.get_notes", return_value=json.dumps({'results': []})) def test_edxnotes_view_is_enabled(self, mock_get_notes): """ Tests that appropriate view is received if EdxNotes feature is enabled. @@ -876,75 +985,57 @@ class EdxNotesViewsTest(ModuleStoreTestCase): self.assertEqual(response.status_code, 404) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.get_notes", autospec=True) - def test_edxnotes_view_404_service_unavailable(self, mock_get_notes): + @ddt.unpack + @ddt.data( + {'side_effect': EdxNotesServiceUnavailable}, + {'side_effect': EdxNotesServiceUnavailable}, + ) + def test_edxnotes_view_500_error(self, side_effect): """ - Tests that 404 status code is received if EdxNotes service is unavailable. + Tests that 500 status code is received for EdxNotesServiceUnavailable or EdxNotesServiceUnavailable exceptions. """ - mock_get_notes.side_effect = EdxNotesServiceUnavailable - enable_edxnotes_for_the_course(self.course, self.user.id) - response = self.client.get(self.notes_page_url) - self.assertEqual(response.status_code, 404) + with patch("edxnotes.views.get_notes", autospec=True) as mock_get_notes: + mock_get_notes.side_effect = side_effect + enable_edxnotes_for_the_course(self.course, self.user.id) + response = self.client.get(self.notes_page_url) + self.assertEqual(response.status_code, 500) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.search", autospec=True) + @patch("edxnotes.views.get_notes", autospec=True) def test_search_notes_successfully_respond(self, mock_search): """ - Tests that `search_notes` successfully respond if EdxNotes feature is enabled. + Tests that search notes successfully respond if EdxNotes feature is enabled. """ - mock_search.return_value = json.dumps({ - "total": 0, - "rows": [], - }) + mock_search.return_value = json.dumps(NOTES_API_EMPTY_RESPONSE) enable_edxnotes_for_the_course(self.course, self.user.id) - response = self.client.get(self.search_url, {"text": "test"}) - self.assertEqual(json.loads(response.content), { - "total": 0, - "rows": [], - }) + response = self.client.get(self.notes_url, {"text": "test"}) + self.assertEqual(json.loads(response.content), NOTES_API_EMPTY_RESPONSE) self.assertEqual(response.status_code, 200) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False}) - @patch("edxnotes.views.search", autospec=True) + @patch("edxnotes.views.get_notes", autospec=True) def test_search_notes_is_disabled(self, mock_search): """ Tests that 404 status code is received if EdxNotes feature is disabled. """ - mock_search.return_value = json.dumps({ - "total": 0, - "rows": [], - }) - response = self.client.get(self.search_url, {"text": "test"}) + mock_search.return_value = json.dumps(NOTES_API_EMPTY_RESPONSE) + response = self.client.get(self.notes_url, {"text": "test"}) self.assertEqual(response.status_code, 404) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.search", autospec=True) - def test_search_404_service_unavailable(self, mock_search): + @patch("edxnotes.views.get_notes", autospec=True) + def test_search_500_service_unavailable(self, mock_search): """ - Tests that 404 status code is received if EdxNotes service is unavailable. + Tests that 500 status code is received if EdxNotes service is unavailable. """ mock_search.side_effect = EdxNotesServiceUnavailable enable_edxnotes_for_the_course(self.course, self.user.id) - response = self.client.get(self.search_url, {"text": "test"}) + response = self.client.get(self.notes_url, {"text": "test"}) self.assertEqual(response.status_code, 500) self.assertIn("error", response.content) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.search", autospec=True) - def test_search_notes_without_required_parameters(self, mock_search): - """ - Tests that 400 status code is received if the required parameters were not sent. - """ - mock_search.return_value = json.dumps({ - "total": 0, - "rows": [], - }) - enable_edxnotes_for_the_course(self.course, self.user.id) - response = self.client.get(self.search_url) - self.assertEqual(response.status_code, 400) - - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.search", autospec=True) + @patch("edxnotes.views.get_notes", autospec=True) def test_search_notes_exception(self, mock_search): """ Tests that 500 status code is received if invalid data was received from @@ -952,7 +1043,7 @@ class EdxNotesViewsTest(ModuleStoreTestCase): """ mock_search.side_effect = EdxNotesParseError enable_edxnotes_for_the_course(self.course, self.user.id) - response = self.client.get(self.search_url, {"text": "test"}) + response = self.client.get(self.notes_url, {"text": "test"}) self.assertEqual(response.status_code, 500) self.assertIn("error", response.content) diff --git a/lms/djangoapps/edxnotes/urls.py b/lms/djangoapps/edxnotes/urls.py index 17dc36b5e5..544d42c711 100644 --- a/lms/djangoapps/edxnotes/urls.py +++ b/lms/djangoapps/edxnotes/urls.py @@ -7,7 +7,7 @@ from django.conf.urls import patterns, url urlpatterns = patterns( "edxnotes.views", url(r"^/$", "edxnotes", name="edxnotes"), - url(r"^/search/$", "search_notes", name="search_notes"), + url(r"^/notes/$", "notes", name="notes"), url(r"^/token/$", "get_token", name="get_token"), url(r"^/visibility/$", "edxnotes_visibility", name="edxnotes_visibility"), ) diff --git a/lms/djangoapps/edxnotes/views.py b/lms/djangoapps/edxnotes/views.py index 9a1e5eb64c..eae04311f5 100644 --- a/lms/djangoapps/edxnotes/views.py +++ b/lms/djangoapps/edxnotes/views.py @@ -7,6 +7,7 @@ from django.contrib.auth.decorators import login_required from django.http import HttpResponse, HttpResponseBadRequest, Http404 from django.conf import settings from django.core.urlresolvers import reverse +from django.views.decorators.http import require_GET from edxmako.shortcuts import render_to_response from opaque_keys.edx.keys import CourseKey from courseware.courses import get_course_with_access @@ -18,8 +19,9 @@ from edxnotes.helpers import ( get_edxnotes_id_token, get_notes, is_feature_enabled, - search, get_course_position, + DEFAULT_PAGE, + DEFAULT_PAGE_SIZE, ) @@ -30,6 +32,13 @@ log = logging.getLogger(__name__) def edxnotes(request, course_id): """ Displays the EdxNotes page. + + Arguments: + request: HTTP request object + course_id: course id + + Returns: + Rendered HTTP response. """ course_key = CourseKey.from_string(course_id) course = get_course_with_access(request.user, "load", course_key) @@ -38,19 +47,19 @@ def edxnotes(request, course_id): raise Http404 try: - notes = get_notes(request.user, course) - except EdxNotesServiceUnavailable: - raise Http404 + notes_info = get_notes(request, course) + except (EdxNotesParseError, EdxNotesServiceUnavailable) as err: + return JsonResponseBadRequest({"error": err.message}, status=500) context = { "course": course, - "search_endpoint": reverse("search_notes", kwargs={"course_id": course_id}), - "notes": notes, + "notes_endpoint": reverse("notes", kwargs={"course_id": course_id}), + "notes": notes_info, "debug": json.dumps(settings.DEBUG), 'position': None, } - if not notes: + if len(json.loads(notes_info)['results']) == 0: field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course.id, request.user, course, depth=2 ) @@ -66,27 +75,97 @@ def edxnotes(request, course_id): return render_to_response("edxnotes/edxnotes.html", context) +@require_GET @login_required -def search_notes(request, course_id): +def notes(request, course_id): """ - Handles search requests. + Notes view to handle list and search requests. + + Query parameters: + page: page number to get + page_size: number of items in the page + text: text string to search. If `text` param is missing then get all the + notes for the current user for this course else get only those notes + which contain the `text` value. + + Arguments: + request: HTTP request object + course_id: course id + + Returns: + Paginated response as JSON. A sample response is below. + { + "count": 101, + "num_pages": 11, + "current_page": 1, + "results": [ + { + "chapter": { + "index": 4, + "display_name": "About Exams and Certificates", + "location": "i4x://org/course/category/name@revision", + "children": [ + "i4x://org/course/category/name@revision" + ] + }, + "updated": "Dec 09, 2015 at 09:31 UTC", + "tags": ["shadow","oil"], + "quote": "foo bar baz", + "section": { + "display_name": "edX Exams", + "location": "i4x://org/course/category/name@revision", + "children": [ + "i4x://org/course/category/name@revision", + "i4x://org/course/category/name@revision", + ] + }, + "created": "2015-12-09T09:31:17.338305Z", + "ranges": [ + { + "start": "/div[1]/p[1]", + "end": "/div[1]/p[1]", + "startOffset": 0, + "endOffset": 6 + } + ], + "user": "50cf92f9a3d8489df95e583549b919df", + "text": "first angry height hungry structure", + "course_id": "edx/DemoX/Demo", + "id": "1231", + "unit": { + "url": "/courses/edx%2FDemoX%2FDemo/courseware/1414ffd5143b4b508f739b563ab468b7/workflow/1", + "display_name": "EdX Exams", + "location": "i4x://org/course/category/name@revision" + }, + "usage_id": "i4x://org/course/category/name@revision" + } ], + "next": "http://0.0.0.0:8000/courses/edx%2FDemoX%2FDemo/edxnotes/notes/?page=2&page_size=10", + "start": 0, + "previous": null + } """ course_key = CourseKey.from_string(course_id) - course = get_course_with_access(request.user, "load", course_key) + course = get_course_with_access(request.user, 'load', course_key) if not is_feature_enabled(course): raise Http404 - if "text" not in request.GET: - return HttpResponseBadRequest() + page = request.GET.get('page') or DEFAULT_PAGE + page_size = request.GET.get('page_size') or DEFAULT_PAGE_SIZE + text = request.GET.get('text') - query_string = request.GET["text"] try: - search_results = search(request.user, course, query_string) + notes_info = get_notes( + request, + course, + page=page, + page_size=page_size, + text=text + ) except (EdxNotesParseError, EdxNotesServiceUnavailable) as err: return JsonResponseBadRequest({"error": err.message}, status=500) - return HttpResponse(search_results) + return HttpResponse(notes_info, content_type="application/json") # pylint: disable=unused-argument diff --git a/lms/templates/edxnotes/edxnotes.html b/lms/templates/edxnotes/edxnotes.html index 17c384a32d..0226a86eb6 100644 --- a/lms/templates/edxnotes/edxnotes.html +++ b/lms/templates/edxnotes/edxnotes.html @@ -26,7 +26,7 @@ import json % if notes: