diff --git a/common/djangoapps/terrain/stubs/edxnotes.py b/common/djangoapps/terrain/stubs/edxnotes.py index 3e697f51e9..c5b4510558 100644 --- a/common/djangoapps/terrain/stubs/edxnotes.py +++ b/common/djangoapps/terrain/stubs/edxnotes.py @@ -7,11 +7,12 @@ import re from uuid import uuid4 from datetime import datetime from copy import deepcopy +from math import ceil +from urllib import urlencode from .http import StubHttpRequestHandler, StubHttpService -# pylint: disable=invalid-name class StubEdxNotesServiceHandler(StubHttpRequestHandler): """ Handler for EdxNotes requests. @@ -165,7 +166,7 @@ class StubEdxNotesServiceHandler(StubHttpRequestHandler): """ Return the note by note id. """ - notes = self.server.get_notes() + notes = self.server.get_all_notes() result = self.server.filter_by_id(notes, note_id) if result: self.respond(content=result[0]) @@ -191,6 +192,53 @@ class StubEdxNotesServiceHandler(StubHttpRequestHandler): else: self.respond(404, "404 Not Found") + @staticmethod + def _get_next_prev_url(url_path, query_params, page_num, page_size): + """ + makes url with the query params including pagination params + for pagination next and previous urls + """ + query_params = deepcopy(query_params) + query_params.update({ + "page": page_num, + "page_size": page_size + }) + return url_path + "?" + urlencode(query_params) + + def _get_paginated_response(self, notes, page_num, page_size): + """ + Returns a paginated response of notes. + """ + start = (page_num - 1) * page_size + end = start + page_size + total_notes = len(notes) + url_path = "http://{server_address}:{port}{path}".format( + server_address=self.client_address[0], + port=self.server.port, + path=self.path_only + ) + + next_url = None if end >= total_notes else self._get_next_prev_url( + url_path, self.get_params, page_num + 1, page_size + ) + prev_url = None if page_num == 1 else self._get_next_prev_url( + url_path, self.get_params, page_num - 1, page_size) + + # Get notes from range + notes = deepcopy(notes[start:end]) + + paginated_response = { + 'total': total_notes, + 'num_pages': int(ceil(float(total_notes) / page_size)), + 'current_page': page_num, + 'rows': notes, + 'next': next_url, + 'start': start, + 'previous': prev_url + } + + return paginated_response + def _search(self): """ Search for a notes by user id, course_id and usage_id. @@ -199,32 +247,35 @@ class StubEdxNotesServiceHandler(StubHttpRequestHandler): usage_id = self.get_params.get("usage_id", None) course_id = self.get_params.get("course_id", None) text = self.get_params.get("text", None) + page = int(self.get_params.get("page", 1)) + page_size = int(self.get_params.get("page_size", 2)) if user is None: self.respond(400, "Bad Request") return - notes = self.server.get_notes() + 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 text: notes = self.server.search(notes, text) - self.respond(content={ - "total": len(notes), - "rows": notes, - }) + self.respond(content=self._get_paginated_response(notes, page, page_size)) def _collection(self): """ Return all notes for the user. """ user = self.get_params.get("user", None) + page = int(self.get_params.get("page", 1)) + page_size = int(self.get_params.get("page_size", 2)) + notes = self.server.get_all_notes() + if user is None: self.send_response(400, content="Bad Request") return - notes = self.server.get_notes() + notes = self._get_paginated_response(notes, page, page_size) self.respond(content=notes) def _cleanup(self): @@ -245,9 +296,9 @@ class StubEdxNotesService(StubHttpService): super(StubEdxNotesService, self).__init__(*args, **kwargs) self.notes = list() - def get_notes(self): + def get_all_notes(self): """ - Returns a list of all notes. + Returns a list of all notes without pagination """ notes = deepcopy(self.notes) notes.reverse() diff --git a/common/djangoapps/terrain/stubs/http.py b/common/djangoapps/terrain/stubs/http.py index 9d44af24f9..5262390975 100644 --- a/common/djangoapps/terrain/stubs/http.py +++ b/common/djangoapps/terrain/stubs/http.py @@ -3,6 +3,7 @@ Stub implementation of an HTTP service. """ from BaseHTTPServer import HTTPServer, BaseHTTPRequestHandler +import urllib import urlparse import threading import json @@ -217,6 +218,8 @@ class StubHttpRequestHandler(BaseHTTPRequestHandler, object): `format_str` is a string with old-style Python format escaping; `args` is an array of values to fill into the string. """ + if not args: + format_str = urllib.unquote(format_str) return u"{0} - - [{1}] {2}\n".format( self.client_address[0], self.log_date_time_string(), diff --git a/common/djangoapps/terrain/stubs/tests/test_edxnotes.py b/common/djangoapps/terrain/stubs/tests/test_edxnotes.py index 6225a559a7..19aa0969eb 100644 --- a/common/djangoapps/terrain/stubs/tests/test_edxnotes.py +++ b/common/djangoapps/terrain/stubs/tests/test_edxnotes.py @@ -1,7 +1,7 @@ """ Unit tests for stub EdxNotes implementation. """ - +import urlparse import json import unittest import requests @@ -19,7 +19,7 @@ class StubEdxNotesServiceTest(unittest.TestCase): """ super(StubEdxNotesServiceTest, self).setUp() self.server = StubEdxNotesService() - dummy_notes = self._get_dummy_notes(count=2) + dummy_notes = self._get_dummy_notes(count=5) self.server.add_notes(dummy_notes) self.addCleanup(self.server.shutdown) @@ -99,17 +99,48 @@ class StubEdxNotesServiceTest(unittest.TestCase): self.assertEqual(response.status_code, 404) def test_search(self): + # Without user + response = requests.get(self._get_url("api/v1/search")) + self.assertEqual(response.status_code, 400) + + # 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", }) - notes = self._get_notes() - self.assertTrue(response.ok) - self.assertDictEqual({"total": 2, "rows": notes}, response.json()) - response = requests.get(self._get_url("api/v1/search")) - self.assertEqual(response.status_code, 400) + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=5, + num_pages=3, + notes_per_page=2, + start=0, + current_page=1, + next_page=2, + previous_page=None + ) + + # 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" + }) + + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=0, + num_pages=0, + notes_per_page=0, + start=0, + current_page=1, + next_page=None, + previous_page=None + ) def test_delete(self): notes = self._get_notes() @@ -119,7 +150,7 @@ class StubEdxNotesServiceTest(unittest.TestCase): for note in notes: response = requests.delete(self._get_url("api/v1/annotations/" + note["id"])) self.assertEqual(response.status_code, 204) - remaining_notes = self.server.get_notes() + remaining_notes = self.server.get_all_notes() self.assertNotIn(note["id"], [note["id"] for note in remaining_notes]) self.assertEqual(len(remaining_notes), 0) @@ -139,24 +170,149 @@ class StubEdxNotesServiceTest(unittest.TestCase): response = requests.get(self._get_url("api/v1/annotations/does_not_exist")) self.assertEqual(response.status_code, 404) - def test_notes_collection(self): - response = requests.get(self._get_url("api/v1/annotations"), params={"user": "dummy-user-id"}) - self.assertTrue(response.ok) - self.assertEqual(len(response.json()), 2) + # pylint: disable=too-many-arguments + def _verify_pagination_info( + self, + response, + total_notes, + num_pages, + notes_per_page, + current_page, + previous_page, + next_page, + start + ): + """ + Verify the pagination information. + Argument: + response: response from api + total_notes: total notes in the response + num_pages: total number of pages in response + notes_per_page: number of notes in the response + current_page: current page number + previous_page: previous page number + next_page: next page number + start: start of the current page + """ + def get_page_value(url): + """ + Return page value extracted from url. + """ + if url is None: + return None + + parsed = urlparse.urlparse(url) + query_params = urlparse.parse_qs(parsed.query) + + page = query_params["page"][0] + return page if page is None else int(page) + + self.assertEqual(response["total"], total_notes) + self.assertEqual(response["num_pages"], num_pages) + self.assertEqual(len(response["rows"]), notes_per_page) + self.assertEqual(response["current_page"], current_page) + self.assertEqual(get_page_value(response["previous"]), previous_page) + self.assertEqual(get_page_value(response["next"]), next_page) + self.assertEqual(response["start"], start) + + def test_notes_collection(self): + """ + Test paginated response of notes api + """ + + # Without user response = requests.get(self._get_url("api/v1/annotations")) self.assertEqual(response.status_code, 400) + # Without any pagination parameters + response = requests.get(self._get_url("api/v1/annotations"), params={"user": "dummy-user-id"}) + + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=5, + num_pages=3, + notes_per_page=2, + start=0, + current_page=1, + next_page=2, + previous_page=None + ) + + # With pagination parameters + response = requests.get(self._get_url("api/v1/annotations"), params={ + "user": "dummy-user-id", + "page": 2, + "page_size": 3 + }) + + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=5, + num_pages=2, + notes_per_page=2, + start=3, + current_page=2, + next_page=None, + previous_page=1 + ) + + def test_notes_collection_next_previous_with_one_page(self): + """ + Test next and previous urls of paginated response of notes api + when number of pages are 1 + """ + response = requests.get(self._get_url("api/v1/annotations"), params={ + "user": "dummy-user-id", + "page_size": 10 + }) + + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=5, + num_pages=1, + notes_per_page=5, + start=0, + current_page=1, + next_page=None, + previous_page=None + ) + + def test_notes_collection_when_no_notes(self): + """ + Test paginated response of notes api when there's no note present + """ + + # Delete all notes + self.test_cleanup() + + # Get default page + response = requests.get(self._get_url("api/v1/annotations"), params={"user": "dummy-user-id"}) + self.assertTrue(response.ok) + self._verify_pagination_info( + response=response.json(), + total_notes=0, + num_pages=0, + notes_per_page=0, + start=0, + current_page=1, + next_page=None, + previous_page=None + ) + def test_cleanup(self): response = requests.put(self._get_url("cleanup")) self.assertTrue(response.ok) - self.assertEqual(len(self.server.get_notes()), 0) + self.assertEqual(len(self.server.get_all_notes()), 0) def test_create_notes(self): dummy_notes = self._get_dummy_notes(count=2) response = requests.post(self._get_url("create_notes"), data=json.dumps(dummy_notes)) self.assertTrue(response.ok) - self.assertEqual(len(self._get_notes()), 4) + self.assertEqual(len(self._get_notes()), 7) response = requests.post(self._get_url("create_notes")) self.assertEqual(response.status_code, 400) @@ -177,7 +333,7 @@ class StubEdxNotesServiceTest(unittest.TestCase): """ Return a list of notes from the stub EdxNotes service. """ - notes = self.server.get_notes() + notes = self.server.get_all_notes() self.assertGreater(len(notes), 0, "Notes are empty.") return notes diff --git a/common/test/acceptance/pages/common/paging.py b/common/test/acceptance/pages/common/paging.py index 8594a23584..a34673aeef 100644 --- a/common/test/acceptance/pages/common/paging.py +++ b/common/test/acceptance/pages/common/paging.py @@ -63,3 +63,8 @@ class PaginatedUIMixin(object): def is_enabled(self, css): """Return whether the given element is not disabled.""" return 'is-disabled' not in self.q(css=css).attrs('class')[0] + + @property + def footer_visible(self): + """ Return True if footer is visible else False""" + return self.q(css='.pagination.bottom').visible diff --git a/common/test/acceptance/pages/lms/edxnotes.py b/common/test/acceptance/pages/lms/edxnotes.py index 5d40875f62..4cbd83fc5e 100644 --- a/common/test/acceptance/pages/lms/edxnotes.py +++ b/common/test/acceptance/pages/lms/edxnotes.py @@ -1,6 +1,7 @@ from bok_choy.page_object import PageObject, PageLoadError, unguarded from bok_choy.promise import BrokenPromise, EmptyPromise from .course_page import CoursePage +from ..common.paging import PaginatedUIMixin from ...tests.helpers import disable_animations from selenium.webdriver.common.action_chains import ActionChains @@ -114,7 +115,7 @@ class EdxNotesPageItem(NoteChild): """ BODY_SELECTOR = ".note" UNIT_LINK_SELECTOR = "a.reference-unit-link" - TAG_SELECTOR = "a.reference-tags" + TAG_SELECTOR = "span.reference-tags" def go_to_unit(self, unit_page=None): self.q(css=self._bounded_selector(self.UNIT_LINK_SELECTOR)).click() @@ -242,7 +243,7 @@ class SearchResultsView(EdxNotesPageView): TAB_SELECTOR = ".tab#view-search-results" -class EdxNotesPage(CoursePage): +class EdxNotesPage(CoursePage, PaginatedUIMixin): """ EdxNotes page. """ @@ -348,6 +349,10 @@ class EdxNotesPage(CoursePage): children = self.q(css='.note-group') return [EdxNotesTagsGroup(self.browser, child.get_attribute("id")) for child in children] + def count(self): + """ Returns the total number of notes in the list """ + return len(self.q(css='div.wrapper-note-excerpts').results) + class EdxNotesPageNoContent(CoursePage): """ diff --git a/common/test/acceptance/tests/lms/test_lms_edxnotes.py b/common/test/acceptance/tests/lms/test_lms_edxnotes.py index 0efa293145..fe4314c7ba 100644 --- a/common/test/acceptance/tests/lms/test_lms_edxnotes.py +++ b/common/test/acceptance/tests/lms/test_lms_edxnotes.py @@ -1,17 +1,18 @@ """ Test LMS Notes """ +from unittest import skip +import random from uuid import uuid4 from datetime import datetime from nose.plugins.attrib import attr -from ..helpers import UniqueCourseTest +from ..helpers import UniqueCourseTest, EventsTestMixin from ...fixtures.course import CourseFixture, XBlockFixtureDesc from ...pages.lms.auto_auth import AutoAuthPage from ...pages.lms.course_nav import CourseNavPage from ...pages.lms.courseware import CoursewarePage from ...pages.lms.edxnotes import EdxNotesUnitPage, EdxNotesPage, EdxNotesPageNoContent from ...fixtures.edxnotes import EdxNotesFixture, Note, Range -from ..helpers import EventsTestMixin class EdxNotesTestMixin(UniqueCourseTest): @@ -345,9 +346,10 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.edxnotes_fixture.create_notes(notes_list) self.edxnotes_fixture.install() - def _add_default_notes(self, tags=None): + def _add_default_notes(self, tags=None, extra_notes=0): """ - Creates 5 test notes. If tags are not specified, will populate the notes with some test tag data. + Creates 5 test notes by default & number of extra_notes will be created if specified. + If tags are not specified, will populate the notes with some test tag data. If tags are specified, they will be used for each of the 3 notes that have tags. """ xblocks = self.course_fixture.get_nested_xblocks(category="html") @@ -398,6 +400,19 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): updated=datetime(2015, 1, 1, 1, 1, 1, 1).isoformat() ), ] + if extra_notes > 0: + for __ in range(extra_notes): + self.raw_note_list.append( + Note( + usage_id=xblocks[random.choice([0, 1, 2, 3, 4, 5])].locator, + user=self.username, + course_id=self.course_fixture._course_key, # pylint: disable=protected-access + text="Fourth note", + quote="", + updated=datetime(2014, 1, 1, 1, 1, 1, 1).isoformat(), + tags=["review"] if tags is None else tags + ) + ) self._add_notes(self.raw_note_list) def assertNoteContent(self, item, text=None, quote=None, unit_name=None, time_updated=None, tags=None): @@ -469,6 +484,44 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): ] self.assert_events_match(expected_events, actual_events) + def _verify_pagination_info( + self, + notes_count_on_current_page, + header_text, + previous_button_enabled, + next_button_enabled, + current_page_number, + total_pages + ): + """ + Verify pagination info + """ + self.assertEqual(self.notes_page.count(), notes_count_on_current_page) + self.assertEqual(self.notes_page.get_pagination_header_text(), header_text) + + if total_pages > 1: + self.assertEqual(self.notes_page.footer_visible, True) + self.assertEqual(self.notes_page.is_previous_page_button_enabled(), previous_button_enabled) + self.assertEqual(self.notes_page.is_next_page_button_enabled(), next_button_enabled) + self.assertEqual(self.notes_page.get_current_page_number(), current_page_number) + self.assertEqual(self.notes_page.get_total_pages, total_pages) + else: + self.assertEqual(self.notes_page.footer_visible, False) + + def search_and_verify(self): + """ + Add, search and verify notes. + """ + self._add_default_notes(extra_notes=22) + self.notes_page.visit() + # Run the search + self.notes_page.search("note") + # No error message appears + self.assertFalse(self.notes_page.is_error_visible) + self.assertIn(u"Search Results", self.notes_page.tabs) + + self.assertEqual(self.notes_page.get_total_pages, 2) + def test_no_content(self): """ Scenario: User can see `No content` message. @@ -482,6 +535,57 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): "You have not made any notes in this course yet. Other students in this course are using notes to:", notes_page_empty.no_content_text) + def test_notes_works_correctly_with_xss(self): + """ + Scenario: Note text & tags should be HTML and JS escaped + Given I am enrolled in a course with notes enabled + When I visit the Notes page, with a Notes text and tag containing HTML characters like < and > + Then the text and tags appear as expected due to having been properly escaped + """ + xblocks = self.course_fixture.get_nested_xblocks(category="html") + self._add_notes([ + Note( + usage_id=xblocks[0].locator, + user=self.username, + course_id=self.course_fixture._course_key, # pylint: disable=protected-access + text='', + quote="quote", + updated=datetime(2014, 1, 1, 1, 1, 1, 1).isoformat(), + tags=[''] + ), + Note( + usage_id=xblocks[1].locator, + user=self.username, + course_id=self.course_fixture._course_key, # pylint: disable=protected-access + text='bold', + quote="quote", + updated=datetime(2014, 2, 1, 1, 1, 1, 1).isoformat(), + tags=['bold'] + ) + ]) + self.notes_page.visit() + + notes = self.notes_page.notes + self.assertEqual(len(notes), 2) + + self.assertNoteContent( + notes[0], + quote=u"quote", + text='bold', + unit_name="Test Unit 1", + time_updated="Feb 01, 2014 at 01:01 UTC", + tags=['bold'] + ) + + self.assertNoteContent( + notes[1], + quote=u"quote", + text='', + unit_name="Test Unit 1", + time_updated="Jan 01, 2014 at 01:01 UTC", + tags=[''] + ) + def test_recent_activity_view(self): """ Scenario: User can view all notes by recent activity. @@ -850,6 +954,7 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.assert_viewed_event('Search Results') self.assert_search_event('note', 4) + @skip("scroll to tag functionality is disabled") def test_scroll_to_tag_recent_activity(self): """ Scenario: Can scroll to a tag group from the Recent Activity view (default view) @@ -861,6 +966,7 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.notes_page.visit() self._scroll_to_tag_and_verify("pear", 3) + @skip("scroll to tag functionality is disabled") def test_scroll_to_tag_course_structure(self): """ Scenario: Can scroll to a tag group from the Course Structure view @@ -872,6 +978,7 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.notes_page.visit().switch_to_tab("structure") self._scroll_to_tag_and_verify("squash", 5) + @skip("scroll to tag functionality is disabled") def test_scroll_to_tag_search(self): """ Scenario: Can scroll to a tag group from the Search Results view @@ -884,6 +991,7 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): self.notes_page.visit().search("note") self._scroll_to_tag_and_verify("pumpkin", 4) + @skip("scroll to tag functionality is disabled") def test_scroll_to_tag_from_tag_view(self): """ Scenario: Can scroll to a tag group from the Tags view @@ -1005,6 +1113,291 @@ class EdxNotesPageTest(EventsTestMixin, EdxNotesTestMixin): note = self.note_unit_page.notes[0] self.assertFalse(note.is_visible) + def test_page_size_limit(self): + """ + Scenario: Verify that we can't get notes more than default page size. + + Given that I am a registered user + And I have a course with 11 notes + When I open Notes page + Then I can see notes list contains 10 items + And I should see paging header and footer with correct data + And I should see disabled previous button + And I should also see enabled next button + """ + self._add_default_notes(extra_notes=21) + self.notes_page.visit() + + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + def test_pagination_with_single_page(self): + """ + Scenario: Notes list pagination works as expected for single page + Given that I am a registered user + And I have a course with 5 notes + When I open Notes page + Then I can see notes list contains 5 items + And I should see paging header and footer with correct data + And I should see disabled previous and next buttons + """ + self._add_default_notes() + self.notes_page.visit() + self._verify_pagination_info( + notes_count_on_current_page=5, + header_text='Showing 1-5 out of 5 total', + previous_button_enabled=False, + next_button_enabled=False, + current_page_number=1, + total_pages=1 + ) + + def test_next_and_previous_page_button(self): + """ + Scenario: Next & Previous buttons are working as expected for notes list pagination + + Given that I am a registered user + And I have a course with 26 notes + When I open Notes page + Then I can see notes list contains 25 items + And I should see paging header and footer with correct data + And I should see disabled previous button + And I should see enabled next button + + When I click on next page button in footer + Then I should be navigated to second page + And I should see a list with 1 item + And I should see paging header and footer with correct info + And I should see enabled previous button + And I should also see disabled next button + + When I click on previous page button in footer + Then I should be navigated to first page + And I should see a list with 25 items + And I should see paging header and footer with correct info + And I should see disabled previous button + And I should also see enabled next button + """ + self._add_default_notes(extra_notes=21) + self.notes_page.visit() + + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + self.notes_page.press_next_page_button() + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + self.notes_page.press_previous_page_button() + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + def test_pagination_with_valid_and_invalid_page_number(self): + """ + Scenario: Notes list pagination works as expected for valid & invalid page number + + Given that I am a registered user + And I have a course with 26 notes + When I open Notes page + Then I can see notes list contains 25 items + And I should see paging header and footer with correct data + And I should see total page value is 2 + When I enter 2 in the page number input + Then I should be navigated to page 2 + + When I enter 3 in the page number input + Then I should not be navigated away from page 2 + """ + self._add_default_notes(extra_notes=21) + self.notes_page.visit() + + self.assertEqual(self.notes_page.get_total_pages, 2) + + # test pagination with valid page number + self.notes_page.go_to_page(2) + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + + # test pagination with invalid page number + self.notes_page.go_to_page(3) + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + + def test_search_behaves_correctly_with_pagination(self): + """ + Scenario: Searching behaves correctly with pagination. + + Given that I am a registered user + And I have a course with 27 notes + When I open Notes page + Then I can see notes list with 25 items + And I should see paging header and footer with correct data + And previous button is disabled + And next button is enabled + When I run the search with "note" query + Then I see no error message + And I see that "Search Results" tab appears with 26 notes found + And an event has fired indicating that the Search Results view was selected + And an event has fired recording the search that was performed + """ + self.search_and_verify() + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + self.assert_viewed_event('Search Results') + self.assert_search_event('note', 26) + + def test_search_with_next_and_prev_page_button(self): + """ + Scenario: Next & Previous buttons are working as expected for search + + Given that I am a registered user + And I have a course with 27 notes + When I open Notes page + Then I can see notes list with 25 items + And I should see paging header and footer with correct data + And previous button is disabled + And next button is enabled + + When I run the search with "note" query + Then I see that "Search Results" tab appears with 26 notes found + And an event has fired indicating that the Search Results view was selected + And an event has fired recording the search that was performed + + When I click on next page button in footer + Then I should be navigated to second page + And I should see a list with 1 item + And I should see paging header and footer with correct info + And I should see enabled previous button + And I should also see disabled next button + + When I click on previous page button in footer + Then I should be navigated to first page + And I should see a list with 25 items + And I should see paging header and footer with correct info + And I should see disabled previous button + And I should also see enabled next button + """ + self.search_and_verify() + + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + self.assert_viewed_event('Search Results') + self.assert_search_event('note', 26) + + self.notes_page.press_next_page_button() + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + self.notes_page.press_previous_page_button() + self._verify_pagination_info( + notes_count_on_current_page=25, + header_text='Showing 1-25 out of 26 total', + previous_button_enabled=False, + next_button_enabled=True, + current_page_number=1, + total_pages=2 + ) + + def test_search_with_valid_and_invalid_page_number(self): + """ + Scenario: Notes list pagination works as expected for valid & invalid page number + + Given that I am a registered user + And I have a course with 27 notes + When I open Notes page + Then I can see notes list contains 25 items + And I should see paging header and footer with correct data + And I should see total page value is 2 + + When I run the search with "note" query + Then I see that "Search Results" tab appears with 26 notes found + And an event has fired indicating that the Search Results view was selected + And an event has fired recording the search that was performed + + When I enter 2 in the page number input + Then I should be navigated to page 2 + + When I enter 3 in the page number input + Then I should not be navigated away from page 2 + """ + self.search_and_verify() + + # test pagination with valid page number + self.notes_page.go_to_page(2) + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + + # test pagination with invalid page number + self.notes_page.go_to_page(3) + self._verify_pagination_info( + notes_count_on_current_page=1, + header_text='Showing 26-26 out of 26 total', + previous_button_enabled=True, + next_button_enabled=False, + current_page_number=2, + total_pages=2 + ) + @attr('shard_4') class EdxNotesToggleSingleNoteTest(EdxNotesTestMixin): diff --git a/lms/djangoapps/edxnotes/helpers.py b/lms/djangoapps/edxnotes/helpers.py index 61cd678192..a4ad3dd09e 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 @@ -19,7 +20,7 @@ from django.core.urlresolvers import reverse from django.utils.translation import ugettext as _ from edxnotes.exceptions import EdxNotesParseError, EdxNotesServiceUnavailable -from capa.util import sanitize_html +from edxnotes.plugins import EdxNotesTab from courseware.views import get_current_child from courseware.access import has_access from openedx.core.lib.token_utils import get_id_token @@ -30,10 +31,10 @@ from xmodule.modulestore.exceptions import ItemNotFoundError log = logging.getLogger(__name__) -HIGHLIGHT_TAG = "span" -HIGHLIGHT_CLASS = "note-highlight" # OAuth2 Client name for edxnotes CLIENT_NAME = "edx-notes" +DEFAULT_PAGE = 1 +DEFAULT_PAGE_SIZE = 25 class NoteJSONEncoder(JSONEncoder): @@ -63,22 +64,33 @@ 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, - "highlight": True, - "highlight_tag": HIGHLIGHT_TAG, - "highlight_class": HIGHLIGHT_CLASS, + "text": text, + "highlight": True }) try: @@ -90,6 +102,7 @@ def send_request(user, course_id, path="", query_string=None): params=params ) except RequestException: + log.error("Failed to connect to edx-notes-api: url=%s, params=%s", url, str(params)) raise EdxNotesServiceUnavailable(_("EdxNotes Service is unavailable. Please try again in a few minutes.")) return response @@ -125,15 +138,13 @@ def preprocess_collection(user, course, collection): store = modulestore() filtered_collection = list() cache = {} + include_path_info = ('course_structure' not in settings.NOTES_DISABLED_TABS) with store.bulk_operations(course.id): for model in collection: update = { - u"text": sanitize_html(model["text"]), - u"quote": sanitize_html(model["quote"]), u"updated": dateutil_parse(model["updated"]), } - if "tags" in model: - update[u"tags"] = [sanitize_html(tag) for tag in model["tags"]] + model.update(update) usage_id = model["usage_id"] if usage_id in cache: @@ -160,42 +171,46 @@ def preprocess_collection(user, course, collection): log.debug("Unit not found: %s", usage_key) continue - section = unit.get_parent() - if not section: - log.debug("Section not found: %s", usage_key) - continue - if section in cache: - usage_context = cache[section] - usage_context.update({ - "unit": get_module_context(course, unit), - }) - model.update(usage_context) - cache[usage_id] = cache[unit] = usage_context - filtered_collection.append(model) - continue + if include_path_info: + section = unit.get_parent() + if not section: + log.debug("Section not found: %s", usage_key) + continue + if section in cache: + usage_context = cache[section] + usage_context.update({ + "unit": get_module_context(course, unit), + }) + model.update(usage_context) + cache[usage_id] = cache[unit] = usage_context + filtered_collection.append(model) + continue - chapter = section.get_parent() - if not chapter: - log.debug("Chapter not found: %s", usage_key) - continue - if chapter in cache: - usage_context = cache[chapter] - usage_context.update({ - "unit": get_module_context(course, unit), - "section": get_module_context(course, section), - }) - model.update(usage_context) - cache[usage_id] = cache[unit] = cache[section] = usage_context - filtered_collection.append(model) - continue + chapter = section.get_parent() + if not chapter: + log.debug("Chapter not found: %s", usage_key) + continue + if chapter in cache: + usage_context = cache[chapter] + usage_context.update({ + "unit": get_module_context(course, unit), + "section": get_module_context(course, section), + }) + model.update(usage_context) + cache[usage_id] = cache[unit] = cache[section] = usage_context + filtered_collection.append(model) + continue usage_context = { "unit": get_module_context(course, unit), - "section": get_module_context(course, section), - "chapter": get_module_context(course, chapter), + "section": get_module_context(course, section) if include_path_info else {}, + "chapter": get_module_context(course, chapter) if include_path_info else {}, } model.update(usage_context) - cache[usage_id] = cache[unit] = cache[section] = cache[chapter] = usage_context + if include_path_info: + cache[section] = cache[chapter] = usage_context + + cache[usage_id] = cache[unit] = usage_context filtered_collection.append(model) return filtered_collection @@ -239,39 +254,97 @@ 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 + log.error("Invalid JSON response received from notes api: response_content=%s", response.content) + raise EdxNotesParseError(_("Invalid JSON response received from notes api.")) - if not collection: - return None + # Verify response dict structure + expected_keys = ['total', 'rows', 'num_pages', 'start', 'next', 'previous', 'current_page'] + keys = collection.keys() + if not keys or not all(key in expected_keys for key in keys): + log.error("Incorrect data received from notes api: collection_data=%s", str(collection)) + raise EdxNotesParseError(_("Incorrect data received from notes api.")) - return json.dumps(preprocess_collection(user, course, collection), cls=NoteJSONEncoder) + filtered_results = preprocess_collection(request.user, course, collection['rows']) + # Notes API is called from: + # 1. The annotatorjs in courseware. It expects these attributes to be named "total" and "rows". + # 2. The Notes tab Javascript proxied through LMS. It expects these attributes to be called "count" and "results". + collection['count'] = collection['total'] + del collection['total'] + collection['results'] = filtered_results + del collection['rows'] + + collection['next'], collection['previous'] = construct_pagination_urls( + request, + course.id, + collection['next'], + collection['previous'] + ) + + return collection def get_endpoint(api_url, path=""): @@ -354,26 +427,6 @@ def generate_uid(): def is_feature_enabled(course): """ - Returns True if Student Notes feature is enabled for the course, - False otherwise. - - In order for the application to be enabled it must be: - 1) enabled globally via FEATURES. - 2) present in the course tab configuration. - 3) Harvard Annotation Tool must be disabled for the course. + Returns True if Student Notes feature is enabled for the course, False otherwise. """ - return (settings.FEATURES.get("ENABLE_EDXNOTES") - and [t for t in course.tabs if t["type"] == "edxnotes"] # tab found - and not is_harvard_notes_enabled(course)) - - -def is_harvard_notes_enabled(course): - """ - Returns True if Harvard Annotation Tool is enabled for the course, - False otherwise. - - Checks for 'textannotation', 'imageannotation', 'videoannotation' in the list - of advanced modules of the course. - """ - modules = set(['textannotation', 'imageannotation', 'videoannotation']) - return bool(modules.intersection(course.advanced_modules)) + return EdxNotesTab.is_enabled(course) diff --git a/lms/djangoapps/edxnotes/plugins.py b/lms/djangoapps/edxnotes/plugins.py index e2cdf8a29a..68519fe549 100644 --- a/lms/djangoapps/edxnotes/plugins.py +++ b/lms/djangoapps/edxnotes/plugins.py @@ -1,9 +1,8 @@ """ Registers the "edX Notes" feature for the edX platform. """ - +from django.conf import settings from django.utils.translation import ugettext_noop - from courseware.tabs import EnrolledTab @@ -27,4 +26,20 @@ class EdxNotesTab(EnrolledTab): """ if not super(EdxNotesTab, cls).is_enabled(course, user=user): return False + + if not settings.FEATURES.get("ENABLE_EDXNOTES") or is_harvard_notes_enabled(course): + return False + return course.edxnotes + + +def is_harvard_notes_enabled(course): + """ + Returns True if Harvard Annotation Tool is enabled for the course, + False otherwise. + + Checks for 'textannotation', 'imageannotation', 'videoannotation' in the list + of advanced modules of the course. + """ + modules = set(['textannotation', 'imageannotation', 'videoannotation']) + return bool(modules.intersection(course.advanced_modules)) diff --git a/lms/djangoapps/edxnotes/tests.py b/lms/djangoapps/edxnotes/tests.py index be7637667f..76010b4e2d 100644 --- a/lms/djangoapps/edxnotes/tests.py +++ b/lms/djangoapps/edxnotes/tests.py @@ -8,11 +8,13 @@ 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 from edxnotes.decorators import edxnotes from edxnotes.exceptions import EdxNotesParseError, EdxNotesServiceUnavailable +from edxnotes.plugins import EdxNotesTab from django.conf import settings from django.core.urlresolvers import reverse from django.core.exceptions import ImproperlyConfigured @@ -31,6 +33,29 @@ from courseware.tabs import get_course_tab_list from student.tests.factories import UserFactory, CourseEnrollmentFactory +FEATURES = settings.FEATURES.copy() + +NOTES_API_EMPTY_RESPONSE = { + "total": 0, + "rows": [], + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 0, +} + +NOTES_VIEW_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. @@ -118,6 +143,7 @@ class EdxNotesDecoratorTest(ModuleStoreTestCase): Tests that get_html is wrapped when feature flag is on, but edxnotes are disabled for the course. """ + self.course.edxnotes = False self.assertEqual("original_get_html", self.problem.get_html()) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False}) @@ -191,6 +217,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`. @@ -202,14 +231,6 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): 'position': position, }) - def test_edxnotes_not_enabled(self): - """ - Tests that edxnotes are disabled when the course tab configuration does NOT - contain a tab with type "edxnotes." - """ - self.course.tabs = [] - self.assertFalse(helpers.is_feature_enabled(self.course)) - def test_edxnotes_harvard_notes_enabled(self): """ Tests that edxnotes are disabled when Harvard Annotation Tool is enabled. @@ -226,15 +247,17 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): self.course.advanced_modules = ["textannotation", "videoannotation", "imageannotation"] self.assertFalse(helpers.is_feature_enabled(self.course)) - def test_edxnotes_enabled(self): + @ddt.unpack + @ddt.data( + {'_edxnotes': True}, + {'_edxnotes': False} + ) + def test_is_feature_enabled(self, _edxnotes): """ - Tests that edxnotes are enabled when the course tab configuration contains - a tab with type "edxnotes." + Tests that is_feature_enabled shows correct behavior. """ - self.course.tabs = [{"type": "foo"}, - {"name": "Notes", "type": "edxnotes"}, - {"type": "bar"}] - self.assertTrue(helpers.is_feature_enabled(self.course)) + self.course.edxnotes = _edxnotes + self.assertEqual(helpers.is_feature_enabled(self.course), _edxnotes) @ddt.data( helpers.get_public_endpoint, @@ -280,71 +303,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(), + "total": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, + "rows": [ + { + 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)) + ] + }, + helpers.get_notes(self.request, self.course) ) @patch("edxnotes.helpers.requests.get", autospec=True) @@ -353,15 +396,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): @@ -370,6 +413,11 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): """ mock_get.return_value.content = json.dumps({ "total": 2, + "current_page": 1, + "start": 0, + "next": None, + "previous": None, + "num_pages": 1, "rows": [ { u"quote": u"quote text", @@ -388,8 +436,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 +493,7 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): }, ] }, - json.loads(helpers.search(self.user, self.course, "test")) + helpers.get_notes(self.request, self.course) ) @patch("edxnotes.helpers.requests.get", autospec=True) @@ -449,7 +502,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,60 +510,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")) - ) - - def test_preprocess_collection_escaping(self): - """ - Tests the result if appropriate module is not found. - """ - initial_collection = [{ - u"quote": u"test ", - u"text": u"text \"<>&'", - u"usage_id": unicode(self.html_module_1.location), - u"updated": datetime(2014, 11, 19, 8, 5, 16, 00000).isoformat() - }] - - self.assertItemsEqual( - [{ - u"quote": u"test <script>alert('test')</script>", - 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": datetime(2014, 11, 19, 8, 5, 16, 00000), - }], - helpers.preprocess_collection(self.user, self.course, initial_collection) + NOTES_VIEW_EMPTY_RESPONSE, + helpers.get_notes(self.request, self.course) ) def test_preprocess_collection_no_item(self): @@ -625,6 +635,59 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): [], helpers.preprocess_collection(self.user, self.course, initial_collection) ) + @override_settings(NOTES_DISABLED_TABS=['course_structure', 'tags']) + def test_preprocess_collection_with_disabled_tabs(self, ): + """ + Tests that preprocess collection returns correct data if `course_structure` and `tags` are disabled. + """ + initial_collection = [ + { + u"quote": u"quote text1", + u"text": u"text1", + u"usage_id": unicode(self.html_module_1.location), + u"updated": datetime(2016, 01, 26, 8, 5, 16, 00000).isoformat(), + }, + { + u"quote": u"quote text2", + u"text": u"text2", + u"usage_id": unicode(self.html_module_2.location), + u"updated": datetime(2016, 01, 26, 9, 6, 17, 00000).isoformat(), + }, + ] + + self.assertItemsEqual( + [ + { + + 'section': {}, + 'chapter': {}, + "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'text': u'text1', + u'quote': u'quote text1', + u'usage_id': unicode(self.html_module_1.location), + u'updated': datetime(2016, 01, 26, 8, 5, 16) + }, + { + 'section': {}, + 'chapter': {}, + "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'text': u'text2', + u'quote': u'quote text2', + u'usage_id': unicode(self.html_module_2.location), + u'updated': datetime(2016, 01, 26, 9, 6, 17) + } + ], + helpers.preprocess_collection(self.user, self.course, initial_collection) + ) + def test_get_parent_unit(self): """ Tests `get_parent_unit` method for the successful result. @@ -693,14 +756,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/", @@ -712,8 +780,8 @@ class EdxNotesHelpersTest(ModuleStoreTestCase): "course_id": unicode(self.course.id), "text": "text", "highlight": True, - "highlight_tag": "span", - "highlight_class": "note-highlight", + 'page': 1, + 'page_size': 25, } ) @@ -722,14 +790,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=25 ) mock_get.assert_called_with( "http://example.com/test/", @@ -739,6 +807,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 +878,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 +953,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,14 +989,14 @@ 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={'results': []}) def test_edxnotes_view_is_enabled(self, mock_get_notes): """ Tests that appropriate view is received if EdxNotes feature is enabled. """ enable_edxnotes_for_the_course(self.course, self.user.id) response = self.client.get(self.notes_page_url) - self.assertContains(response, 'Highlights and notes you\'ve made in course content') + self.assertContains(response, 'Highlights and notes you've made in course content') @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False}) def test_edxnotes_view_is_disabled(self): @@ -877,74 +1008,38 @@ class EdxNotesViewsTest(ModuleStoreTestCase): @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): - """ - Tests that 404 status code is received if EdxNotes service is unavailable. - """ - 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) - - @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": True}) - @patch("edxnotes.views.search", 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 = NOTES_VIEW_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_VIEW_EMPTY_RESPONSE) self.assertEqual(response.status_code, 200) @patch.dict("django.conf.settings.FEATURES", {"ENABLE_EDXNOTES": False}) - @patch("edxnotes.views.search", autospec=True) - def test_search_notes_is_disabled(self, mock_search): + def test_search_notes_is_disabled(self): """ 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"}) + 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 +1047,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) @@ -1022,3 +1117,50 @@ class EdxNotesViewsTest(ModuleStoreTestCase): content_type="application/json", ) self.assertEqual(response.status_code, 400) + + +@skipUnless(settings.FEATURES["ENABLE_EDXNOTES"], "EdxNotes feature needs to be enabled.") +@ddt.ddt +class EdxNotesPluginTest(ModuleStoreTestCase): + """ + EdxNotesTab tests. + """ + + def setUp(self): + super(EdxNotesPluginTest, self).setUp() + self.course = CourseFactory.create(edxnotes=True) + self.user = UserFactory.create(username="ma", email="ma@ma.info", password="edx") + CourseEnrollmentFactory.create(user=self.user, course_id=self.course.id) + + def test_edxnotes_tab_with_unauthorized_user(self): + """ + Verify EdxNotesTab visibility when user is unauthroized. + """ + user = UserFactory.create(username="ma1", email="ma1@ma1.info", password="edx") + self.assertFalse(EdxNotesTab.is_enabled(self.course, user=user)) + + @ddt.unpack + @ddt.data( + {'enable_edxnotes': False}, + {'enable_edxnotes': True} + ) + def test_edxnotes_tab_with_feature_flag(self, enable_edxnotes): + """ + Verify EdxNotesTab visibility when ENABLE_EDXNOTES feature flag is enabled/disabled. + """ + FEATURES['ENABLE_EDXNOTES'] = enable_edxnotes + with override_settings(FEATURES=FEATURES): + self.assertEqual(EdxNotesTab.is_enabled(self.course), enable_edxnotes) + + @ddt.unpack + @ddt.data( + {'harvard_notes_enabled': False}, + {'harvard_notes_enabled': True} + ) + def test_edxnotes_tab_with_harvard_notes(self, harvard_notes_enabled): + """ + Verify EdxNotesTab visibility when harvard notes feature is enabled/disabled. + """ + with patch("edxnotes.plugins.is_harvard_notes_enabled") as mock_harvard_notes_enabled: + mock_harvard_notes_enabled.return_value = harvard_notes_enabled + self.assertEqual(EdxNotesTab.is_enabled(self.course), not harvard_notes_enabled) 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..a4d0a5bb1b 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,10 @@ from edxnotes.helpers import ( get_edxnotes_id_token, get_notes, is_feature_enabled, - search, get_course_position, + DEFAULT_PAGE, + DEFAULT_PAGE_SIZE, + NoteJSONEncoder, ) @@ -30,6 +33,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) @@ -37,20 +47,20 @@ def edxnotes(request, course_id): if not is_feature_enabled(course): raise Http404 - try: - notes = get_notes(request.user, course) - except EdxNotesServiceUnavailable: - raise Http404 - + notes_info = get_notes(request, course) + has_notes = (len(notes_info.get('results')) > 0) context = { "course": course, - "search_endpoint": reverse("search_notes", kwargs={"course_id": course_id}), - "notes": notes, - "debug": json.dumps(settings.DEBUG), + "notes_endpoint": reverse("notes", kwargs={"course_id": course_id}), + "notes": notes_info, + "page_size": DEFAULT_PAGE_SIZE, + "debug": settings.DEBUG, 'position': None, + 'disabled_tabs': settings.NOTES_DISABLED_TABS, + 'has_notes': has_notes, } - if not notes: + if not has_notes: field_data_cache = FieldDataCache.cache_for_descriptor_descendents( course.id, request.user, course, depth=2 ) @@ -66,27 +76,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(json.dumps(notes_info, cls=NoteJSONEncoder), content_type="application/json") # pylint: disable=unused-argument diff --git a/lms/envs/bok_choy.py b/lms/envs/bok_choy.py index cb74f9a87f..d461d4bbf3 100644 --- a/lms/envs/bok_choy.py +++ b/lms/envs/bok_choy.py @@ -91,6 +91,8 @@ XQUEUE_INTERFACE['url'] = 'http://localhost:8040' EDXNOTES_PUBLIC_API = 'http://localhost:8042/api/v1' EDXNOTES_INTERNAL_API = 'http://localhost:8042/api/v1' +NOTES_DISABLED_TABS = [] + # Silence noisy logs import logging LOG_OVERRIDES = [ diff --git a/lms/envs/common.py b/lms/envs/common.py index de98686bce..6b02eb0a63 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2542,6 +2542,9 @@ ENROLLMENT_COURSE_DETAILS_CACHE_TIMEOUT = 60 if FEATURES['ENABLE_EDXNOTES']: OAUTH_ID_TOKEN_EXPIRATION = 60 * 60 +# These tabs are currently disabled +NOTES_DISABLED_TABS = ['course_structure', 'tags'] + # Configuration used for generating PDF Receipts/Invoices PDF_RECEIPT_TAX_ID = 'add here' PDF_RECEIPT_FOOTER_TEXT = 'add your own specific footer text here' diff --git a/lms/envs/test.py b/lms/envs/test.py index 330034d39d..cee2425db6 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -516,6 +516,8 @@ MONGODB_LOG = { 'db': 'xlog', } +NOTES_DISABLED_TABS = [] + # Enable EdxNotes for tests. FEATURES['ENABLE_EDXNOTES'] = True diff --git a/lms/static/js/dashboard/track_events.js b/lms/static/js/dashboard/track_events.js index 4667b6b4bb..b61a0d2768 100644 --- a/lms/static/js/dashboard/track_events.js +++ b/lms/static/js/dashboard/track_events.js @@ -34,6 +34,7 @@ var edx = edx || {}; // Emit an event when the 'course title link' is clicked. edx.dashboard.trackCourseTitleClicked = function($courseTitleLink, properties){ var trackProperty = properties || edx.dashboard.generateTrackProperties; + window.analytics.trackLink( $courseTitleLink, 'edx.bi.dashboard.course_title.clicked', @@ -102,6 +103,7 @@ var edx = edx || {}; }; edx.dashboard.xseriesTrackMessages = function() { + $('.xseries-action .btn').each(function(i, element) { var data = edx.dashboard.generateProgramProperties($(element)); @@ -110,6 +112,9 @@ var edx = edx || {}; }; $(document).ready(function() { + if (!window.analytics) { + return; + } edx.dashboard.trackCourseTitleClicked($('.course-title > a')); edx.dashboard.trackCourseImageLinkClicked($('.cover')); edx.dashboard.trackEnterCourseLinkClicked($('.enter-course')); diff --git a/lms/static/js/edxnotes/collections/notes.js b/lms/static/js/edxnotes/collections/notes.js index dfc7d06665..b5a6f002ba 100644 --- a/lms/static/js/edxnotes/collections/notes.js +++ b/lms/static/js/edxnotes/collections/notes.js @@ -1,11 +1,21 @@ -;(function (define, undefined) { +;(function (define) { 'use strict'; define([ - 'backbone', 'js/edxnotes/models/note' -], function (Backbone, NoteModel) { - var NotesCollection = Backbone.Collection.extend({ + 'underscore', 'common/js/components/collections/paging_collection', 'js/edxnotes/models/note' +], function (_, PagingCollection, NoteModel) { + return PagingCollection.extend({ model: NoteModel, + initialize: function(models, options) { + PagingCollection.prototype.initialize.call(this); + + this.perPage = options.perPage; + this.server_api = _.pick(PagingCollection.prototype.server_api, "page", "page_size"); + if (options.text) { + this.server_api.text = options.text; + } + }, + /** * Returns course structure from the list of notes. * @return {Object} @@ -17,30 +27,26 @@ define([ sections = {}, units = {}; - if (!courseStructure) { - this.each(function (note) { - var chapter = note.get('chapter'), - section = note.get('section'), - unit = note.get('unit'); + this.each(function (note) { + var chapter = note.get('chapter'), + section = note.get('section'), + unit = note.get('unit'); - chapters[chapter.location] = chapter; - sections[section.location] = section; - units[unit.location] = units[unit.location] || []; - units[unit.location].push(note); - }); + chapters[chapter.location] = chapter; + sections[section.location] = section; + units[unit.location] = units[unit.location] || []; + units[unit.location].push(note); + }); - courseStructure = { - chapters: _.sortBy(_.toArray(chapters), function (c) {return c.index;}), - sections: sections, - units: units - }; - } + courseStructure = { + chapters: _.sortBy(_.toArray(chapters), function (c) {return c.index;}), + sections: sections, + units: units + }; return courseStructure; }; }()) }); - - return NotesCollection; }); }).call(this, define || RequireJS.define); diff --git a/lms/static/js/edxnotes/models/note.js b/lms/static/js/edxnotes/models/note.js index 9d48b1c61b..60ce8e11da 100644 --- a/lms/static/js/edxnotes/models/note.js +++ b/lms/static/js/edxnotes/models/note.js @@ -51,10 +51,6 @@ define(['backbone', 'js/edxnotes/utils/utils', 'underscore.string'], function (B } return message; - }, - - getText: function () { - return Utils.nl2br(this.get('text')); } }); diff --git a/lms/static/js/edxnotes/plugins/store_error_handler.js b/lms/static/js/edxnotes/plugins/store_error_handler.js new file mode 100644 index 0000000000..9c1d01ff7d --- /dev/null +++ b/lms/static/js/edxnotes/plugins/store_error_handler.js @@ -0,0 +1,33 @@ +(function (define, undefined) { + 'use strict'; + + define(['annotator_1.2.9'], function (Annotator) { + /** + * Modifies Annotator.Plugin.Store.prototype._onError to show custom error message + * if sent by server + */ + var originalErrorHandler = Annotator.Plugin.Store.prototype._onError; + Annotator.Plugin.Store.prototype._onError = function (xhr) { + var serverResponse; + + // Try to parse json + if (xhr.responseText) { + try { + serverResponse = JSON.parse(xhr.responseText); + } catch (exception) { + serverResponse = null; + } + } + + // if response includes an error message it will take precedence + if (serverResponse && serverResponse.error_msg) { + Annotator.showNotification(serverResponse.error_msg, Annotator.Notification.ERROR); + return console.error(Annotator._t("API request failed:") + (" '" + xhr.status + "'")); + } + + // Delegate to original error handler + originalErrorHandler(xhr); + }; + }); +}).call(this, define || RequireJS.define); + diff --git a/lms/static/js/edxnotes/views/note_item.js b/lms/static/js/edxnotes/views/note_item.js index c6a01e0549..1f5fd27709 100644 --- a/lms/static/js/edxnotes/views/note_item.js +++ b/lms/static/js/edxnotes/views/note_item.js @@ -31,8 +31,7 @@ define([ getContext: function () { return $.extend({}, this.model.toJSON(), { - message: this.model.getQuote(), - text: this.model.getText() + message: this.model.getQuote() }); }, @@ -60,7 +59,9 @@ define([ tagHandler: function (event) { event.preventDefault(); - this.options.scrollToTag(event.currentTarget.text); + if (!_.isUndefined(this.options.scrollToTag)) { + this.options.scrollToTag(event.currentTarget.text); + } }, redirectTo: function (uri) { diff --git a/lms/static/js/edxnotes/views/notes_factory.js b/lms/static/js/edxnotes/views/notes_factory.js index 4b1d64811d..f1e091b3a9 100644 --- a/lms/static/js/edxnotes/views/notes_factory.js +++ b/lms/static/js/edxnotes/views/notes_factory.js @@ -4,7 +4,8 @@ define([ 'jquery', 'underscore', 'annotator_1.2.9', 'js/edxnotes/utils/logger', 'js/edxnotes/views/shim', 'js/edxnotes/plugins/scroller', 'js/edxnotes/plugins/events', 'js/edxnotes/plugins/accessibility', - 'js/edxnotes/plugins/caret_navigation' + 'js/edxnotes/plugins/caret_navigation', + 'js/edxnotes/plugins/store_error_handler' ], function ($, _, Annotator, NotesLogger) { var plugins = ['Auth', 'Store', 'Scroller', 'Events', 'Accessibility', 'CaretNavigation', 'Tags'], getOptions, setupPlugins, getAnnotator; diff --git a/lms/static/js/edxnotes/views/notes_page.js b/lms/static/js/edxnotes/views/notes_page.js index 4c43dd1542..9034578897 100644 --- a/lms/static/js/edxnotes/views/notes_page.js +++ b/lms/static/js/edxnotes/views/notes_page.js @@ -15,17 +15,19 @@ define([ this.options = options; this.tabsCollection = new TabsCollection(); - // Must create the Tags view first to get the "scrollToTag" method. - this.tagsView = new TagsView({ - el: this.el, - collection: this.collection, - tabsCollection: this.tabsCollection - }); + if (!_.contains(this.options.disabledTabs, 'tags')) { + // Must create the Tags view first to get the "scrollToTag" method. + this.tagsView = new TagsView({ + el: this.el, + collection: this.collection, + tabsCollection: this.tabsCollection + }); - scrollToTag = this.tagsView.scrollToTag; + scrollToTag = this.tagsView.scrollToTag; - // Remove the Tags model from the tabs collection because it should not appear first. - tagsModel = this.tabsCollection.shift(); + // Remove the Tags model from the tabs collection because it should not appear first. + tagsModel = this.tabsCollection.shift(); + } this.recentActivityView = new RecentActivityView({ el: this.el, @@ -34,20 +36,25 @@ define([ scrollToTag: scrollToTag }); - this.courseStructureView = new CourseStructureView({ - el: this.el, - collection: this.collection, - tabsCollection: this.tabsCollection, - scrollToTag: scrollToTag - }); - - // Add the Tags model after the Course Structure model. - this.tabsCollection.push(tagsModel); + if (!_.contains(this.options.disabledTabs, 'course_structure')) { + this.courseStructureView = new CourseStructureView({ + el: this.el, + collection: this.collection, + tabsCollection: this.tabsCollection, + scrollToTag: scrollToTag + }); + } + + if (!_.contains(this.options.disabledTabs, 'tags')) { + // Add the Tags model after the Course Structure model. + this.tabsCollection.push(tagsModel); + } this.searchResultsView = new SearchResultsView({ el: this.el, tabsCollection: this.tabsCollection, debug: this.options.debug, + perPage: this.options.perPage, createTabOnInitialization: false, scrollToTag: scrollToTag }); diff --git a/lms/static/js/edxnotes/views/page_factory.js b/lms/static/js/edxnotes/views/page_factory.js index 43ab4be3c3..142b0c18fd 100644 --- a/lms/static/js/edxnotes/views/page_factory.js +++ b/lms/static/js/edxnotes/views/page_factory.js @@ -6,19 +6,29 @@ define([ /** * Factory method for the Notes page. * @param {Object} params Params for the Notes page. - * @param {Array} params.notesList A list of note models. + * @param {List} params.disabledTabs Names of disabled tabs, these tabs will not be shown. + * @param {Object} params.notes Paginated notes info. + * @param {Number} params.pageSize Number of notes per page. * @param {Boolean} params.debugMode Enable the flag to see debug information. * @param {String} params.endpoint The endpoint of the store. * @return {Object} An instance of NotesPageView. */ return function (params) { - var collection = new NotesCollection(params.notesList); + var collection = new NotesCollection( + params.notes, + { + url: params.notesEndpoint, + perPage: params.pageSize, + parse: true + } + ); return new NotesPageView({ el: $('.wrapper-student-notes').get(0), collection: collection, debug: params.debugMode, - endpoint: params.endpoint + perPage: params.pageSize, + disabledTabs: params.disabledTabs }); }; }); diff --git a/lms/static/js/edxnotes/views/search_box.js b/lms/static/js/edxnotes/views/search_box.js index c50eace24f..8be666b17e 100644 --- a/lms/static/js/edxnotes/views/search_box.js +++ b/lms/static/js/edxnotes/views/search_box.js @@ -29,9 +29,15 @@ define([ this.logger = NotesLogger.getLogger('search_box', this.options.debug); this.$el.removeClass('is-hidden'); this.isDisabled = false; + this.searchInput = this.$el.find('#search-notes-input'); this.logger.log('initialized'); }, + clearInput: function() { + // clear the search input box + this.searchInput.val(''); + }, + submitHandler: function (event) { event.preventDefault(); this.search(); @@ -43,15 +49,12 @@ define([ * @return {Array} */ prepareData: function (data) { - var collection; - - if (!(data && _.has(data, 'total') && _.has(data, 'rows'))) { + if (!(data && _.has(data, 'count') && _.has(data, 'results'))) { this.logger.log('Wrong data', data, this.searchQuery); return null; } - collection = new NotesCollection(data.rows); - return [collection, data.total, this.searchQuery]; + return [this.collection, this.searchQuery]; }, /** @@ -99,8 +102,8 @@ define([ if (args) { this.options.search.apply(this, args); this.logger.emit('edx.course.student_notes.searched', { - 'number_of_results': args[1], - 'search_string': args[2] + 'number_of_results': args[0].totalCount, + 'search_string': args[1] }); } else { this.options.error(this.errorMessage, this.searchQuery); @@ -144,15 +147,15 @@ define([ * @return {jQuery.Deferred} */ sendRequest: function (text) { - var settings = { - url: this.el.action, - type: this.el.method, - dataType: 'json', - data: {text: text} - }; - - this.logger.log(settings); - return $.ajax(settings); + this.collection = new NotesCollection( + [], + { + text: text, + perPage: this.options.perPage, + url: this.el.action + } + ); + return this.collection.goTo(1); } }); diff --git a/lms/static/js/edxnotes/views/tab_panel.js b/lms/static/js/edxnotes/views/tab_panel.js index c9ac5a5905..c27eb442c6 100644 --- a/lms/static/js/edxnotes/views/tab_panel.js +++ b/lms/static/js/edxnotes/views/tab_panel.js @@ -1,7 +1,8 @@ ;(function (define, undefined) { 'use strict'; -define(['gettext', 'underscore', 'backbone', 'js/edxnotes/views/note_item'], -function (gettext, _, Backbone, NoteItemView) { +define(['gettext', 'underscore', 'backbone', 'js/edxnotes/views/note_item', + 'common/js/components/views/paging_header', 'common/js/components/views/paging_footer'], +function (gettext, _, Backbone, NoteItemView, PagingHeaderView, PagingFooterView) { var TabPanelView = Backbone.View.extend({ tagName: 'section', className: 'tab-panel', @@ -13,14 +14,30 @@ function (gettext, _, Backbone, NoteItemView) { initialize: function () { this.children = []; + if (this.options.createHeaderFooter) { + this.pagingHeaderView = new PagingHeaderView({collection: this.collection}); + this.pagingFooterView = new PagingFooterView({collection: this.collection, hideWhenOnePage: true}); + } + if (this.hasOwnProperty('collection')) { + this.listenTo(this.collection, 'page_changed', this.render); + } }, render: function () { this.$el.html(this.getTitle()); + this.renderView(this.pagingHeaderView); this.renderContent(); + this.renderView(this.pagingFooterView); return this; }, + renderView: function(view) { + if (this.options.createHeaderFooter && this.collection.models.length) { + this.$el.append(view.render().el); + view.delegateEvents(); + } + }, + renderContent: function () { return this; }, diff --git a/lms/static/js/edxnotes/views/tab_view.js b/lms/static/js/edxnotes/views/tab_view.js index baa8a79801..19883abe3f 100644 --- a/lms/static/js/edxnotes/views/tab_view.js +++ b/lms/static/js/edxnotes/views/tab_view.js @@ -14,7 +14,8 @@ define([ initialize: function (options) { _.bindAll(this, 'showLoadingIndicator', 'hideLoadingIndicator'); this.options = _.defaults(options || {}, { - createTabOnInitialization: true + createTabOnInitialization: true, + createHeaderFooter: true }); if (this.options.createTabOnInitialization) { @@ -64,7 +65,13 @@ define([ getSubView: function () { var collection = this.getCollection(); - return new this.PanelConstructor({collection: collection, scrollToTag: this.options.scrollToTag}); + return new this.PanelConstructor( + { + collection: collection, + scrollToTag: this.options.scrollToTag, + createHeaderFooter: this.options.createHeaderFooter + } + ); }, destroySubView: function () { diff --git a/lms/static/js/edxnotes/views/tabs/search_results.js b/lms/static/js/edxnotes/views/tabs/search_results.js index 2351528ff6..6a35ff2b20 100644 --- a/lms/static/js/edxnotes/views/tabs/search_results.js +++ b/lms/static/js/edxnotes/views/tabs/search_results.js @@ -58,6 +58,7 @@ define([ this.searchBox = new SearchBoxView({ el: document.getElementById('search-notes-form'), debug: this.options.debug, + perPage: this.options.perPage, beforeSearchStart: this.onBeforeSearchStart, search: this.onSearch, error: this.onSearchError @@ -81,7 +82,8 @@ define([ return new this.PanelConstructor({ collection: collection, searchQuery: this.searchResults.searchQuery, - scrollToTag: this.options.scrollToTag + scrollToTag: this.options.scrollToTag, + createHeaderFooter: this.options.createHeaderFooter }); } else { return new this.NoResultsViewConstructor({ @@ -103,6 +105,7 @@ define([ onClose: function () { this.searchResults = null; + this.searchBox.clearInput(); }, onBeforeSearchStart: function () { @@ -122,10 +125,9 @@ define([ } }, - onSearch: function (collection, total, searchQuery) { + onSearch: function (collection, searchQuery) { this.searchResults = { collection: collection, - total: total, searchQuery: searchQuery }; diff --git a/lms/static/js/spec/edxnotes/collections/notes_spec.js b/lms/static/js/spec/edxnotes/collections/notes_spec.js index f5dc0206a8..c1d928c10f 100644 --- a/lms/static/js/spec/edxnotes/collections/notes_spec.js +++ b/lms/static/js/spec/edxnotes/collections/notes_spec.js @@ -6,7 +6,7 @@ define([ var notes = Helpers.getDefaultNotes(); beforeEach(function () { - this.collection = new NotesCollection(notes); + this.collection = new NotesCollection(notes, {perPage: 10, parse: true}); }); it('can return correct course structure', function () { @@ -23,11 +23,22 @@ define([ 'i4x://section/2': Helpers.getSection('First Section', 2, [3]) }); - expect(structure.units).toEqual({ + var compareUnits = function (structureUnits, collectionUnits) { + expect(structureUnits.length === collectionUnits.length).toBeTruthy(); + for(var i = 0; i < structureUnits.length; i++) { + expect(structureUnits[i].attributes).toEqual(collectionUnits[i].attributes); + } + }; + + var units = { 'i4x://unit/0': [this.collection.at(0), this.collection.at(1)], 'i4x://unit/1': [this.collection.at(2)], 'i4x://unit/2': [this.collection.at(3)], 'i4x://unit/3': [this.collection.at(4)] + }; + + _.each(units, function(value, key){ + compareUnits(structure.units[key], value); }); }); }); diff --git a/lms/static/js/spec/edxnotes/helpers.js b/lms/static/js/spec/edxnotes/helpers.js index 81bf7c4daa..3edaf1024b 100644 --- a/lms/static/js/spec/edxnotes/helpers.js +++ b/lms/static/js/spec/edxnotes/helpers.js @@ -1,8 +1,10 @@ -define(['underscore'], function(_) { +define(['underscore', 'URI', 'common/js/spec_helpers/ajax_helpers'], function(_, URI, AjaxHelpers) { 'use strict'; var B64 = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/=", LONG_TEXT, PRUNED_TEXT, TRUNCATED_TEXT, SHORT_TEXT, - base64Encode, makeToken, getChapter, getSection, getUnit, getDefaultNotes; + base64Encode, makeToken, getChapter, getSection, getUnit, getDefaultNotes, + verifyUrl, verifyRequestParams, createNotesData, respondToRequest, + verifyPaginationInfo, verifyPageData; LONG_TEXT = [ 'Adipisicing elit, sed do eiusmod tempor incididunt ', @@ -106,57 +108,134 @@ define(['underscore'], function(_) { getDefaultNotes = function () { // Note that the server returns notes in reverse chronological order (newest first). - return [ - { - chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), - section: getSection('Third Section', 0, ['w_n', 1, 0]), - unit: getUnit('Fourth Unit', 0), - created: 'December 11, 2014 at 11:12AM', - updated: 'December 11, 2014 at 11:12AM', - text: 'Third added model', - quote: 'Note 4', - tags: ['Pumpkin', 'pumpkin', 'yummy'] - }, - { - chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), - section: getSection('Third Section', 0, ['w_n', 1, 0]), - unit: getUnit('Fourth Unit', 0), - created: 'December 11, 2014 at 11:11AM', - updated: 'December 11, 2014 at 11:11AM', - text: 'Third added model', - quote: 'Note 5' - }, - { - chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), - section: getSection('Third Section', 0, ['w_n', 1, 0]), - unit: getUnit('Third Unit', 1), - created: 'December 11, 2014 at 11:11AM', - updated: 'December 11, 2014 at 11:11AM', - text: 'Second added model', - quote: 'Note 3', - tags: ['yummy'] - }, - { - chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), - section: getSection('Second Section', 1, [2]), - unit: getUnit('Second Unit', 2), - created: 'December 11, 2014 at 11:10AM', - updated: 'December 11, 2014 at 11:10AM', - text: 'First added model', - quote: 'Note 2', - tags: ['PUMPKIN', 'pie'] - }, - { - chapter: getChapter('First Chapter', 1, 0, [2]), - section: getSection('First Section', 2, [3]), - unit: getUnit('First Unit', 3), - created: 'December 11, 2014 at 11:10AM', - updated: 'December 11, 2014 at 11:10AM', - text: 'First added model', - quote: 'Note 1', - tags: ['pie', 'pumpkin'] - } - ]; + return { + 'count': 5, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [ + { + chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), + section: getSection('Third Section', 0, ['w_n', 1, 0]), + unit: getUnit('Fourth Unit', 0), + created: 'December 11, 2014 at 11:12AM', + updated: 'December 11, 2014 at 11:12AM', + text: 'Third added model', + quote: 'Note 4', + tags: ['Pumpkin', 'pumpkin', 'yummy'] + }, + { + chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), + section: getSection('Third Section', 0, ['w_n', 1, 0]), + unit: getUnit('Fourth Unit', 0), + created: 'December 11, 2014 at 11:11AM', + updated: 'December 11, 2014 at 11:11AM', + text: 'Third added model', + quote: 'Note 5' + }, + { + chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), + section: getSection('Third Section', 0, ['w_n', 1, 0]), + unit: getUnit('Third Unit', 1), + created: 'December 11, 2014 at 11:11AM', + updated: 'December 11, 2014 at 11:11AM', + text: 'Second added model', + quote: 'Note 3', + tags: ['yummy'] + }, + { + chapter: getChapter('Second Chapter', 0, 1, [1, 'w_n', 0]), + section: getSection('Second Section', 1, [2]), + unit: getUnit('Second Unit', 2), + created: 'December 11, 2014 at 11:10AM', + updated: 'December 11, 2014 at 11:10AM', + text: 'First added model', + quote: 'Note 2', + tags: ['PUMPKIN', 'pie'] + }, + { + chapter: getChapter('First Chapter', 1, 0, [2]), + section: getSection('First Section', 2, [3]), + unit: getUnit('First Unit', 3), + created: 'December 11, 2014 at 11:10AM', + updated: 'December 11, 2014 at 11:10AM', + text: 'First added model', + quote: 'Note 1', + tags: ['pie', 'pumpkin'] + } + ] + }; + }; + + verifyUrl = function (requestUrl, expectedUrl, expectedParams) { + expect(requestUrl.slice(0, expectedUrl.length) === expectedUrl).toBeTruthy(); + verifyRequestParams(requestUrl, expectedParams); + }; + + verifyRequestParams = function (requestUrl, expectedParams) { + var urlParams = (new URI(requestUrl)).query(true); + _.each(expectedParams, function (value, key) { + expect(urlParams[key]).toBe(value); + }); + }; + + createNotesData = function (options) { + + var data = { + count: options.count || 0, + num_pages: options.num_pages || 1, + current_page: options.current_page || 1, + start: options.start || 0, + results: [] + }; + + for(var i = 0; i < options.numNotesToCreate; i++) { + var notesInfo = { + chapter: getChapter('First Chapter__' + i, 1, 0, [2]), + section: getSection('First Section__' + i, 2, [3]), + unit: getUnit('First Unit__' + i, 3), + created: new Date().toISOString(), + updated: new Date().toISOString(), + text: 'text__' + i, + quote: 'Note__' + i, + tags: ['tag__' + i, 'tag__' + i+1] + }; + + data.results.push(notesInfo); + } + + return data; + }; + + respondToRequest = function(requests, responseJson, respondToEvent) { + // Respond to the analytics event + if (respondToEvent) { + AjaxHelpers.respondWithNoContent(requests); + } + // Now process the actual request + AjaxHelpers.respondWithJson(requests, responseJson); + }; + + verifyPaginationInfo = function (view, headerMessage, footerHidden, currentPage, totalPages) { + expect(view.$('.search-count.listing-count').text().trim()).toBe(headerMessage); + expect(view.$('.pagination.bottom').parent().hasClass('hidden')).toBe(footerHidden); + if (!footerHidden) { + expect(parseInt(view.$('.pagination span.current-page').text().trim())).toBe(currentPage); + expect(parseInt(view.$('.pagination span.total-pages').text().trim())).toBe(totalPages); + } + }; + + verifyPageData = function (view, tabsCollection, tabInfo, tabId, notes) { + expect(tabsCollection).toHaveLength(1); + expect(tabsCollection.at(0).toJSON()).toEqual(tabInfo); + expect(view.$(tabId)).toExist(); + expect(view.$('.note')).toHaveLength(notes.results.length); + _.each(view.$('.note'), function(element, index) { + expect($('.note-comments', element)).toContainText(notes.results[index].text); + expect($('.note-excerpt', element)).toContainText(notes.results[index].quote); + }); }; return { @@ -169,6 +248,12 @@ define(['underscore'], function(_) { getChapter: getChapter, getSection: getSection, getUnit: getUnit, - getDefaultNotes: getDefaultNotes + getDefaultNotes: getDefaultNotes, + verifyUrl: verifyUrl, + verifyRequestParams: verifyRequestParams, + createNotesData: createNotesData, + respondToRequest: respondToRequest, + verifyPaginationInfo: verifyPaginationInfo, + verifyPageData: verifyPageData }; }); diff --git a/lms/static/js/spec/edxnotes/models/note_spec.js b/lms/static/js/spec/edxnotes/models/note_spec.js index 4a491c43e5..3a79f32596 100644 --- a/lms/static/js/spec/edxnotes/models/note_spec.js +++ b/lms/static/js/spec/edxnotes/models/note_spec.js @@ -4,10 +4,23 @@ define([ 'use strict'; describe('EdxNotes NoteModel', function() { beforeEach(function () { - this.collection = new NotesCollection([ - {quote: Helpers.LONG_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'}, - {quote: Helpers.SHORT_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'} - ]); + this.collection = new NotesCollection( + { + 'count': 2, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [ + {quote: Helpers.LONG_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'}, + {quote: Helpers.SHORT_TEXT, text: 'text\n with\r\nline\n\rbreaks \r'} + ] + }, + { + perPage: 10, parse: true + } + ); }); it('has correct values on initialization', function () { @@ -33,7 +46,7 @@ define([ it('can return appropriate `text`', function () { var model = this.collection.at(0); - expect(model.getText()).toBe('text
with
line
breaks
'); + expect(model.get('text')).toBe('text\n with\r\nline\n\rbreaks \r'); }); }); }); 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 new file mode 100644 index 0000000000..53bcce802d --- /dev/null +++ b/lms/static/js/spec/edxnotes/plugins/store_error_handler_spec.js @@ -0,0 +1,36 @@ +define([ + 'jquery', 'underscore', 'annotator_1.2.9', + 'common/js/spec_helpers/ajax_helpers', + 'js/spec/edxnotes/helpers', + 'js/edxnotes/views/notes_factory' +], function ($, _, Annotator, AjaxHelpers, Helpers, NotesFactory) { + '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'); + }); + + afterEach(function () { + _.invoke(Annotator._instances, 'destroy'); + }); + + 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' + }); + + var errorMsg = 'can\'t create more notes'; + AjaxHelpers.respondWithError(requests, 400, {error_msg: errorMsg}); + expect(Annotator.showNotification).toHaveBeenCalledWith(errorMsg, Annotator.Notification.ERROR); + }); + }); +}); diff --git a/lms/static/js/spec/edxnotes/views/note_item_spec.js b/lms/static/js/spec/edxnotes/views/note_item_spec.js index 6478ad356a..84ea75a39f 100644 --- a/lms/static/js/spec/edxnotes/views/note_item_spec.js +++ b/lms/static/js/spec/edxnotes/views/note_item_spec.js @@ -9,14 +9,14 @@ define([ ) { 'use strict'; describe('EdxNotes NoteItemView', function() { - var getView = function (model, scrollToTag) { + var getView = function (model, scrollToTag, formattedText) { model = new NoteModel(_.defaults(model || {}, { id: 'id-123', user: 'user-123', usage_id: 'usage_id-123', created: 'December 11, 2014 at 11:12AM', updated: 'December 11, 2014 at 11:12AM', - text: 'Third added model', + text: formattedText || 'Third added model', quote: Helpers.LONG_TEXT, unit: { url: 'http://example.com/' @@ -67,12 +67,42 @@ define([ var view = getView({tags: ["First", "Second"]}); expect(view.$('.reference-title').length).toBe(3); expect(view.$('.reference-title')[2]).toContainText('Tags:'); - expect(view.$('a.reference-tags').length).toBe(2); - expect(view.$('a.reference-tags')[0]).toContainText('First'); - expect(view.$('a.reference-tags')[1]).toContainText('Second'); + expect(view.$('span.reference-tags').length).toBe(2); + expect(view.$('span.reference-tags')[0]).toContainText('First'); + expect(view.$('span.reference-tags')[1]).toContainText('Second'); }); - it('should handle a click event on the tag', function() { + it('should highlight tags & text if they have elasticsearch formatter', function() { + var view = getView({ + tags: ["First", "{elasticsearch_highlight_start}Second{elasticsearch_highlight_end}"] + }, {}, "{elasticsearch_highlight_start}Sample{elasticsearch_highlight_end}"); + expect(view.$('.reference-title').length).toBe(3); + expect(view.$('.reference-title')[2]).toContainText('Tags:'); + expect(view.$('span.reference-tags').length).toBe(2); + expect(view.$('span.reference-tags')[0]).toContainText('First'); + // highlighted tag & text + expect($.trim($(view.$('span.reference-tags')[1]).html())).toBe( + 'Second' + ); + expect($.trim(view.$('.note-comment-p').html())).toBe('Sample'); + }); + + it('should escape html for tags & comments', function() { + var view = getView({ + tags: ["First", "Second", "ȗnicode"] + }, {}, "Sample"); + expect(view.$('.reference-title').length).toBe(3); + expect(view.$('.reference-title')[2]).toContainText('Tags:'); + expect(view.$('span.reference-tags').length).toBe(3); + expect(view.$('span.reference-tags')[0]).toContainText('First'); + expect($.trim($(view.$('span.reference-tags')[1]).html())).toBe( + '<b>Second</b>' + ); + expect($.trim($(view.$('span.reference-tags')[2]).html())).toBe('ȗnicode'); + expect($.trim(view.$('.note-comment-p').html())).toBe('<b>Sample</b>'); + }); + + xit('should handle a click event on the tag', function() { var scrollToTagSpy = { scrollToTag: function (tagName){} }; diff --git a/lms/static/js/spec/edxnotes/views/notes_page_spec.js b/lms/static/js/spec/edxnotes/views/notes_page_spec.js index 43ab8a38e9..2c2ad249bf 100644 --- a/lms/static/js/spec/edxnotes/views/notes_page_spec.js +++ b/lms/static/js/spec/edxnotes/views/notes_page_spec.js @@ -13,7 +13,7 @@ define([ TemplateHelpers.installTemplates([ 'templates/edxnotes/note-item', 'templates/edxnotes/tab-item' ]); - this.view = new NotesFactory({notesList: notes}); + this.view = new NotesFactory({notes: notes, pageSize: 10}); }); @@ -35,8 +35,13 @@ define([ this.view.$('.search-notes-input').val('test_query'); this.view.$('.search-notes-submit').click(); AjaxHelpers.respondWithJson(requests, { - total: 0, - rows: [] + 'count': 0, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [] }); expect(this.view.$('#view-search-results')).toHaveClass('is-active'); expect(this.view.$('#view-recent-activity')).toExist(); diff --git a/lms/static/js/spec/edxnotes/views/search_box_spec.js b/lms/static/js/spec/edxnotes/views/search_box_spec.js index 907d250d2f..3b1e780fcd 100644 --- a/lms/static/js/spec/edxnotes/views/search_box_spec.js +++ b/lms/static/js/spec/edxnotes/views/search_box_spec.js @@ -1,14 +1,25 @@ define([ 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers', 'js/edxnotes/views/search_box', - 'js/edxnotes/collections/notes', 'js/spec/edxnotes/custom_matchers', 'jasmine-jquery' -], function($, _, AjaxHelpers, SearchBoxView, NotesCollection, customMatchers) { + 'js/edxnotes/collections/notes', 'js/spec/edxnotes/custom_matchers', 'js/spec/edxnotes/helpers', 'jasmine-jquery' +], function($, _, AjaxHelpers, SearchBoxView, NotesCollection, customMatchers, Helpers) { 'use strict'; describe('EdxNotes SearchBoxView', function() { - var getSearchBox, submitForm, assertBoxIsEnabled, assertBoxIsDisabled; + var getSearchBox, submitForm, assertBoxIsEnabled, assertBoxIsDisabled, searchResponse; + + searchResponse = { + 'count': 2, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [null, null] + }; getSearchBox = function (options) { options = _.defaults(options || {}, { el: $('#search-notes-form').get(0), + perPage: 10, beforeSearchStart: jasmine.createSpy(), search: jasmine.createSpy(), error: jasmine.createSpy(), @@ -50,7 +61,11 @@ define([ submitForm(this.searchBox, 'test_text'); request = requests[0]; expect(request.method).toBe(form.method.toUpperCase()); - expect(request.url).toBe(form.action + '?' + $.param({text: 'test_text'})); + Helpers.verifyUrl( + request.url, + form.action, + {text: 'test_text', page: '1', page_size: '10'} + ); }); it('returns success result', function () { @@ -60,13 +75,10 @@ define([ 'test_text' ); assertBoxIsDisabled(this.searchBox); - AjaxHelpers.respondWithJson(requests, { - total: 2, - rows: [null, null] - }); + AjaxHelpers.respondWithJson(requests, searchResponse); assertBoxIsEnabled(this.searchBox); expect(this.searchBox.options.search).toHaveBeenCalledWith( - jasmine.any(NotesCollection), 2, 'test_text' + jasmine.any(NotesCollection), 'test_text' ); expect(this.searchBox.options.complete).toHaveBeenCalledWith( 'test_text' @@ -76,10 +88,7 @@ define([ it('should log the edx.course.student_notes.searched event properly', function () { var requests = AjaxHelpers.requests(this); submitForm(this.searchBox, 'test_text'); - AjaxHelpers.respondWithJson(requests, { - total: 2, - rows: [null, null] - }); + AjaxHelpers.respondWithJson(requests, searchResponse); expect(Logger.log).toHaveBeenCalledWith('edx.course.student_notes.searched', { 'number_of_results': 2, @@ -140,10 +149,7 @@ define([ submitForm(this.searchBox, 'test_text'); assertBoxIsDisabled(this.searchBox); submitForm(this.searchBox, 'another_text'); - AjaxHelpers.respondWithJson(requests, { - total: 2, - rows: [null, null] - }); + AjaxHelpers.respondWithJson(requests, searchResponse); assertBoxIsEnabled(this.searchBox); expect(requests).toHaveLength(1); }); @@ -158,5 +164,11 @@ define([ ' ' ); }); + + it('can clear its input box', function () { + this.searchBox.$('.search-notes-input').val('search me'); + this.searchBox.clearInput(); + expect(this.searchBox.$('#search-notes-input').val()).toEqual(''); + }); }); }); diff --git a/lms/static/js/spec/edxnotes/views/tabs/course_structure_spec.js b/lms/static/js/spec/edxnotes/views/tabs/course_structure_spec.js index 05471765ce..39b01573bd 100644 --- a/lms/static/js/spec/edxnotes/views/tabs/course_structure_spec.js +++ b/lms/static/js/spec/edxnotes/views/tabs/course_structure_spec.js @@ -40,7 +40,7 @@ define([ 'templates/edxnotes/note-item', 'templates/edxnotes/tab-item' ]); - this.collection = new NotesCollection(notes); + this.collection = new NotesCollection(notes, {perPage: 10, parse: true}); this.tabsCollection = new TabsCollection(); }); diff --git a/lms/static/js/spec/edxnotes/views/tabs/recent_activity_spec.js b/lms/static/js/spec/edxnotes/views/tabs/recent_activity_spec.js index 5f513842fa..fa18680809 100644 --- a/lms/static/js/spec/edxnotes/views/tabs/recent_activity_spec.js +++ b/lms/static/js/spec/edxnotes/views/tabs/recent_activity_spec.js @@ -1,32 +1,40 @@ define([ - 'jquery', 'common/js/spec_helpers/template_helpers', 'js/edxnotes/collections/notes', - 'js/edxnotes/collections/tabs', 'js/edxnotes/views/tabs/recent_activity', - 'js/spec/edxnotes/custom_matchers', 'jasmine-jquery' + 'jquery', 'common/js/spec_helpers/template_helpers', 'common/js/spec_helpers/ajax_helpers', + 'js/edxnotes/collections/notes', 'js/edxnotes/collections/tabs', 'js/edxnotes/views/tabs/recent_activity', + 'js/spec/edxnotes/custom_matchers', 'js/spec/edxnotes/helpers', 'jasmine-jquery' ], function( - $, TemplateHelpers, NotesCollection, TabsCollection, RecentActivityView, customMatchers + $, TemplateHelpers, AjaxHelpers, NotesCollection, TabsCollection, RecentActivityView, customMatchers, Helpers ) { 'use strict'; describe('EdxNotes RecentActivityView', function() { - var notes = [ - { - created: 'December 11, 2014 at 11:12AM', - updated: 'December 11, 2014 at 11:12AM', - text: 'Third added model', - quote: 'Should be listed first' - }, - { - created: 'December 11, 2014 at 11:11AM', - updated: 'December 11, 2014 at 11:11AM', - text: 'Second added model', - quote: 'Should be listed second' - }, - { - created: 'December 11, 2014 at 11:10AM', - updated: 'December 11, 2014 at 11:10AM', - text: 'First added model', - quote: 'Should be listed third' - } - ], getView; + var notes = { + 'count': 3, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [ + { + created: 'December 11, 2014 at 11:12AM', + updated: 'December 11, 2014 at 11:12AM', + text: 'Third added model', + quote: 'Should be listed first' + }, + { + created: 'December 11, 2014 at 11:11AM', + updated: 'December 11, 2014 at 11:11AM', + text: 'Second added model', + quote: 'Should be listed second' + }, + { + created: 'December 11, 2014 at 11:10AM', + updated: 'December 11, 2014 at 11:10AM', + text: 'First added model', + quote: 'Should be listed third' + } + ] + }, getView, tabInfo, recentActivityTabId; getView = function (collection, tabsCollection, options) { var view; @@ -35,6 +43,7 @@ define([ el: $('.wrapper-student-notes'), collection: collection, tabsCollection: tabsCollection, + createHeaderFooter: true }); view = new RecentActivityView(options); @@ -43,6 +52,17 @@ define([ return view; }; + tabInfo = { + name: 'Recent Activity', + identifier: 'view-recent-activity', + icon: 'fa fa-clock-o', + is_active: true, + is_closable: false, + view: 'Recent Activity' + }; + + recentActivityTabId = '#recent-panel'; + beforeEach(function () { customMatchers(this); loadFixtures('js/fixtures/edxnotes/edxnotes.html'); @@ -50,28 +70,136 @@ define([ 'templates/edxnotes/note-item', 'templates/edxnotes/tab-item' ]); - this.collection = new NotesCollection(notes); + this.collection = new NotesCollection(notes, {perPage: 10, parse: true}); this.tabsCollection = new TabsCollection(); }); it('displays a tab and content with proper data and order', function () { var view = getView(this.collection, this.tabsCollection); + Helpers.verifyPaginationInfo(view, "Showing 1-3 out of 3 total", true, 1, 1); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, notes); + }); - expect(this.tabsCollection).toHaveLength(1); - expect(this.tabsCollection.at(0).toJSON()).toEqual({ - name: 'Recent Activity', - identifier: 'view-recent-activity', - icon: 'fa fa-clock-o', - is_active: true, - is_closable: false, - view: 'Recent Activity' - }); - expect(view.$('#recent-panel')).toExist(); - expect(view.$('.note')).toHaveLength(3); - _.each(view.$('.note'), function(element, index) { - expect($('.note-comments', element)).toContainText(notes[index].text); - expect($('.note-excerpt', element)).toContainText(notes[index].quote); - }); + it("will not render header and footer if there are no notes", function () { + var notes = { + 'count': 0, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [] + }; + var collection = new NotesCollection(notes, {perPage: 10, parse: true}); + var view = getView(collection, this.tabsCollection); + expect(view.$('.search-tools.listing-tools')).toHaveLength(0); + expect(view.$('.pagination.pagination-full.bottom')).toHaveLength(0); + }); + + it("can go to a page number", function () { + var requests = AjaxHelpers.requests(this); + var notes = Helpers.createNotesData( + { + numNotesToCreate: 10, + count: 12, + num_pages: 2, + current_page: 1, + start: 0 + } + ); + + var collection = new NotesCollection(notes, {perPage: 10, parse: true}); + var view = getView(collection, this.tabsCollection); + + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 12 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, notes); + + view.$('input#page-number-input').val('2'); + view.$('input#page-number-input').trigger('change'); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '2', page_size: '10'} + ); + + notes = Helpers.createNotesData( + { + numNotesToCreate: 2, + count: 12, + num_pages: 2, + current_page: 2, + start: 10 + } + ); + Helpers.respondToRequest(requests, notes, true); + Helpers.verifyPaginationInfo(view, "Showing 11-12 out of 12 total", false, 2, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, notes); + }); + + it("can navigate forward and backward", function () { + var requests = AjaxHelpers.requests(this); + var page1Notes = Helpers.createNotesData( + { + numNotesToCreate: 10, + count: 15, + num_pages: 2, + current_page: 1, + start: 0 + } + ); + var collection = new NotesCollection(page1Notes, {perPage: 10, parse: true}); + var view = getView(collection, this.tabsCollection); + + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 15 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, page1Notes); + + view.$('.pagination .next-page-link').click(); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '2', page_size: '10'} + ); + var page2Notes = Helpers.createNotesData( + { + numNotesToCreate: 5, + count: 15, + num_pages: 2, + current_page: 2, + start: 10 + } + ); + Helpers.respondToRequest(requests, page2Notes, true); + Helpers.verifyPaginationInfo(view, "Showing 11-15 out of 15 total", false, 2, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, page2Notes); + + view.$('.pagination .previous-page-link').click(); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '1', page_size: '10'} + ); + Helpers.respondToRequest(requests, page1Notes); + + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 15 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, recentActivityTabId, page1Notes); + }); + + it("sends correct page size value", function () { + var requests = AjaxHelpers.requests(this); + var notes = Helpers.createNotesData( + { + numNotesToCreate: 5, + count: 7, + num_pages: 2, + current_page: 1, + start: 0 + } + ); + var collection = new NotesCollection(notes, {perPage: 5, parse: true}); + var view = getView(collection, this.tabsCollection); + + view.$('.pagination .next-page-link').click(); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '2', page_size: '5'} + ); }); }); }); diff --git a/lms/static/js/spec/edxnotes/views/tabs/search_results_spec.js b/lms/static/js/spec/edxnotes/views/tabs/search_results_spec.js index f828e2d63a..58286bf1e8 100644 --- a/lms/static/js/spec/edxnotes/views/tabs/search_results_spec.js +++ b/lms/static/js/spec/edxnotes/views/tabs/search_results_spec.js @@ -1,10 +1,10 @@ define([ 'jquery', 'underscore', 'common/js/spec_helpers/template_helpers', 'common/js/spec_helpers/ajax_helpers', 'logger', 'js/edxnotes/collections/tabs', 'js/edxnotes/views/tabs/search_results', - 'js/spec/edxnotes/custom_matchers', 'jasmine-jquery' + 'js/spec/edxnotes/custom_matchers', 'js/spec/edxnotes/helpers', 'jasmine-jquery' ], function( $, _, TemplateHelpers, AjaxHelpers, Logger, TabsCollection, SearchResultsView, - customMatchers + customMatchers, Helpers ) { 'use strict'; describe('EdxNotes SearchResultsView', function() { @@ -29,18 +29,25 @@ define([ } ], responseJson = { - total: 3, - rows: notes + 'count': 3, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': notes }, - getView, submitForm, respondToSearch; + getView, submitForm, tabInfo, searchResultsTabId; - getView = function (tabsCollection, options) { + getView = function (tabsCollection, perPage, options) { options = _.defaults(options || {}, { el: $('.wrapper-student-notes'), tabsCollection: tabsCollection, user: 'test_user', courseId: 'course_id', - createTabOnInitialization: false + createTabOnInitialization: false, + createHeaderFooter: true, + perPage: perPage || 10 }); return new SearchResultsView(options); }; @@ -50,14 +57,17 @@ define([ searchBox.$('.search-notes-submit').click(); }; - respondToSearch = function(requests, responseJson) { - // First respond to the analytics event - AjaxHelpers.respondWithNoContent(requests); - - // Now process the search request - AjaxHelpers.respondWithJson(requests, responseJson); + tabInfo = { + name: 'Search Results', + identifier: 'view-search-results', + icon: 'fa fa-search', + is_active: true, + is_closable: true, + view: 'Search Results' }; + searchResultsTabId = "#search-results-panel"; + beforeEach(function () { customMatchers(this); loadFixtures('js/fixtures/edxnotes/edxnotes.html'); @@ -79,23 +89,9 @@ define([ requests = AjaxHelpers.requests(this); submitForm(view.searchBox, 'second'); - respondToSearch(requests, responseJson); - - expect(this.tabsCollection).toHaveLength(1); - expect(this.tabsCollection.at(0).toJSON()).toEqual({ - name: 'Search Results', - identifier: 'view-search-results', - icon: 'fa fa-search', - is_active: true, - is_closable: true, - view: 'Search Results' - }); - expect(view.$('#search-results-panel')).toExist(); - expect(view.$('#search-results-panel')).toBeFocused(); - expect(view.$('.note')).toHaveLength(3); - view.searchResults.collection.each(function (model, index) { - expect(model.get('text')).toBe(notes[index].text); - }); + Helpers.respondToRequest(requests, responseJson, true); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, responseJson); + Helpers.verifyPaginationInfo(view, "Showing 1-3 out of 3 total", true, 1, 1); }); it('displays loading indicator when search is running', function () { @@ -108,7 +104,7 @@ define([ expect(this.tabsCollection).toHaveLength(1); expect(view.searchResults).toBeNull(); expect(view.$('.tab-panel')).not.toExist(); - respondToSearch(requests, responseJson); + Helpers.respondToRequest(requests, responseJson, true); expect(view.$('.ui-loading')).toHaveClass('is-hidden'); }); @@ -117,10 +113,7 @@ define([ requests = AjaxHelpers.requests(this); submitForm(view.searchBox, 'some text'); - respondToSearch(requests, { - total: 0, - rows: [] - }); + Helpers.respondToRequest(requests, _.extend(_.clone(responseJson), {count: 0, results: []}), true); expect(view.$('#search-results-panel')).not.toExist(); expect(view.$('#no-results-panel')).toBeFocused(); @@ -153,12 +146,14 @@ define([ it('can clear search results if tab is closed', function () { var view = getView(this.tabsCollection), requests = AjaxHelpers.requests(this); + spyOn(view.searchBox, 'clearInput').andCallThrough(); submitForm(view.searchBox, 'test_query'); - respondToSearch(requests, responseJson); + Helpers.respondToRequest(requests, responseJson, true); expect(view.searchResults).toBeDefined(); this.tabsCollection.at(0).destroy(); expect(view.searchResults).toBeNull(); + expect(view.searchBox.clearInput).toHaveBeenCalled(); }); it('can correctly show/hide error messages', function () { @@ -195,20 +190,140 @@ define([ }]; submitForm(view.searchBox, 'test_query'); - respondToSearch(requests, responseJson); + Helpers.respondToRequest(requests, responseJson, true); expect(view.$('.note')).toHaveLength(3); submitForm(view.searchBox, 'new_test_query'); - respondToSearch(requests, { - total: 1, - rows: newNotes - }); + Helpers.respondToRequest(requests, { + 'count': 1, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': newNotes + }, true); expect(view.$('.note').length).toHaveLength(1); view.searchResults.collection.each(function (model, index) { expect(model.get('text')).toBe(newNotes[index].text); }); }); + + it("will not render header and footer if there are no notes", function () { + var view = getView(this.tabsCollection), + requests = AjaxHelpers.requests(this), + notes = { + 'count': 0, + 'current_page': 1, + 'num_pages': 1, + 'start': 0, + 'next': null, + 'previous': null, + 'results': [] + }; + submitForm(view.searchBox, 'awesome'); + Helpers.respondToRequest(requests, notes, true); + expect(view.$('.search-tools.listing-tools')).toHaveLength(0); + expect(view.$('.pagination.pagination-full.bottom')).toHaveLength(0); + }); + + it("can go to a page number", function () { + var view = getView(this.tabsCollection), + requests = AjaxHelpers.requests(this), + notes = Helpers.createNotesData( + { + numNotesToCreate: 10, + count: 12, + num_pages: 2, + current_page: 1, + start: 0 + } + ); + + submitForm(view.searchBox, 'awesome'); + Helpers.respondToRequest(requests, notes, true); + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 12 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, notes); + + view.$('input#page-number-input').val('2'); + view.$('input#page-number-input').trigger('change'); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '2', page_size: '10'} + ); + + notes = Helpers.createNotesData( + { + numNotesToCreate: 2, + count: 12, + num_pages: 2, + current_page: 2, + start: 10 + } + ); + Helpers.respondToRequest(requests, notes, true); + Helpers.verifyPaginationInfo(view, "Showing 11-12 out of 12 total", false, 2, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, notes); + }); + + it("can navigate forward and backward", function () { + var requests = AjaxHelpers.requests(this), + page1Notes = Helpers.createNotesData( + { + numNotesToCreate: 10, + count: 15, + num_pages: 2, + current_page: 1, + start: 0 + } + ), + view = getView(this.tabsCollection); + + submitForm(view.searchBox, 'awesome'); + Helpers.respondToRequest(requests, page1Notes, true); + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 15 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, page1Notes); + + view.$('.pagination .next-page-link').click(); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '2', page_size: '10'} + ); + var page2Notes = Helpers.createNotesData( + { + numNotesToCreate: 5, + count: 15, + num_pages: 2, + current_page: 2, + start: 10 + } + ); + Helpers.respondToRequest(requests, page2Notes, true); + Helpers.verifyPaginationInfo(view, "Showing 11-15 out of 15 total", false, 2, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, page2Notes); + + view.$('.pagination .previous-page-link').click(); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '1', page_size: '10'} + ); + Helpers.respondToRequest(requests, page1Notes); + + Helpers.verifyPaginationInfo(view, "Showing 1-10 out of 15 total", false, 1, 2); + Helpers.verifyPageData(view, this.tabsCollection, tabInfo, searchResultsTabId, page1Notes); + }); + + it("sends correct page size value", function () { + var requests = AjaxHelpers.requests(this), + view = getView(this.tabsCollection, 5); + + submitForm(view.searchBox, 'awesome'); + Helpers.verifyRequestParams( + requests[requests.length - 1].url, + {page: '1', page_size: '5'} + ); + }); }); }); diff --git a/lms/static/js/spec/edxnotes/views/tabs/tags_spec.js b/lms/static/js/spec/edxnotes/views/tabs/tags_spec.js index 2fbb467db4..811167f589 100644 --- a/lms/static/js/spec/edxnotes/views/tabs/tags_spec.js +++ b/lms/static/js/spec/edxnotes/views/tabs/tags_spec.js @@ -44,7 +44,7 @@ define([ 'templates/edxnotes/note-item', 'templates/edxnotes/tab-item' ]); - this.collection = new NotesCollection(notes); + this.collection = new NotesCollection(notes, {perPage: 10, parse: true}); this.tabsCollection = new TabsCollection(); }); diff --git a/lms/static/js/spec/main.js b/lms/static/js/spec/main.js index e473ea0a9a..498e3542b7 100644 --- a/lms/static/js/spec/main.js +++ b/lms/static/js/spec/main.js @@ -705,6 +705,7 @@ 'lms/include/js/spec/edxnotes/plugins/events_spec.js', 'lms/include/js/spec/edxnotes/plugins/scroller_spec.js', 'lms/include/js/spec/edxnotes/plugins/caret_navigation_spec.js', + 'lms/include/js/spec/edxnotes/plugins/store_error_handler_spec.js', 'lms/include/js/spec/edxnotes/collections/notes_spec.js', 'lms/include/js/spec/search/search_spec.js', 'lms/include/js/spec/navigation_spec.js', diff --git a/lms/static/sass/course/_student-notes.scss b/lms/static/sass/course/_student-notes.scss index 21c3937624..c739917989 100644 --- a/lms/static/sass/course/_student-notes.scss +++ b/lms/static/sass/course/_student-notes.scss @@ -189,6 +189,10 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4; background: transparent; } + .note-comment-p { + word-wrap: break-word; + } + .note-comment-ul, .note-comment-ol { padding: auto; @@ -233,29 +237,29 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4; color: $m-gray-d2; } - // CASE: tag matches a search query - .reference-meta.reference-tags .note-highlight { - // needed because .note-highlight is a span, which overrides the color - @extend %shame-link-text; - background-color: $result-highlight-color-base; - } + .reference-meta.reference-tags { + word-wrap: break-word; + // CASE: tag matches a search query + .note-highlight { + background-color: $result-highlight-color-base; + } + } // Put commas between tags. - a.reference-meta.reference-tags:after { + span.reference-meta.reference-tags:after { content: ","; color: $m-gray-d2; } // But not after the last tag. - a.reference-meta.reference-tags:last-child:after { + span.reference-meta.reference-tags:last-child:after { content: ""; } // needed for poor base LMS styling scope a.reference-meta { - @extend %shame-link-text; + @extend %shame-link-text; } - } } @@ -285,6 +289,15 @@ $divider-visual-tertiary: ($baseline/20) solid $gray-l4; .tab-panel, .inline-error, .ui-loading { @extend %no-outline; + border-top: $divider-visual-primary; + + .listing-tools { + @include margin($baseline $baseline (-$baseline/2) 0); + } + + .note-group:first-of-type { + border-top: none; + } } .tab-panel.note-group { diff --git a/lms/templates/edxnotes/edxnotes.html b/lms/templates/edxnotes/edxnotes.html index 17c384a32d..533646c5cd 100644 --- a/lms/templates/edxnotes/edxnotes.html +++ b/lms/templates/edxnotes/edxnotes.html @@ -1,8 +1,11 @@ +<%page expression_filter="h"/> <%inherit file="/main.html" /> <%namespace name='static' file='/static_content.html'/> + <%! from django.utils.translation import ugettext as _ -import json +from edxnotes.helpers import NoteJSONEncoder +from openedx.core.djangolib.js_utils import dump_js_escaped_json, js_escaped_string %> <%block name="bodyclass">view-student-notes is-in-course course @@ -12,6 +15,7 @@ import json <%include file="/courseware/course_navigation.html" args="active_page='edxnotes'" /> +
@@ -24,9 +28,9 @@ import json
- % if notes: + % if has_notes:
-
- % if notes: + % if has_notes:
@@ -103,12 +107,15 @@ import json % endfor -% if notes: +% if has_notes: <%block name="js_extra"> <%static:require_module module_name="js/edxnotes/views/page_factory" class_name="NotesPageFactory"> NotesPageFactory({ - notesList: ${notes if notes is not None else []}, - debugMode: ${debug} + disabledTabs: ${disabled_tabs | n, dump_js_escaped_json}, + notes: ${dump_js_escaped_json(notes, NoteJSONEncoder) | n}, + notesEndpoint: ${notes_endpoint | n, dump_js_escaped_json}, + pageSize: ${page_size | n, dump_js_escaped_json}, + debugMode: ${debug | n, dump_js_escaped_json} }); diff --git a/lms/templates/edxnotes/note-item.underscore b/lms/templates/edxnotes/note-item.underscore index 7ec7425c4d..9aaceb98d1 100644 --- a/lms/templates/edxnotes/note-item.underscore +++ b/lms/templates/edxnotes/note-item.underscore @@ -1,7 +1,7 @@
<% if (message) { %>
-

<%= message %> +

<%- message %> <% if (show_link) { %> <% if (is_expanded) { %> <%- gettext('Less') %> @@ -17,7 +17,12 @@

  1. <%- gettext("You commented...") %>

    -

    <%= text %>

    +

    + <%= interpolate_text(_.escape(text), { + elasticsearch_highlight_start: '', + elasticsearch_highlight_end: '' + })%> +

<% } %> @@ -38,7 +43,12 @@ <% if (tags.length > 0) { %>

<%- gettext("Tags:") %>

<% for (var i = 0; i < tags.length; i++) { %> - <%= tags[i] %> + + <%= interpolate_text(_.escape(tags[i]), { + elasticsearch_highlight_start: '', + elasticsearch_highlight_end: '' + })%> + <% } %> <% } %>