From 4304c66c545d626b60cc467196e4ea44b52a870d Mon Sep 17 00:00:00 2001 From: Usman Khalid <2200617@gmail.com> Date: Wed, 20 May 2015 17:14:05 +0500 Subject: [PATCH] Performance optimizations and cache to keep the bookmarks info updated. The cache uses the bookmarks.XBlockCache model. TNL-1945 --- cms/envs/common.py | 3 + .../lib/xmodule/xmodule/modulestore/search.py | 6 +- .../xmodule/modulestore/tests/django_utils.py | 6 +- lms/djangoapps/bookmarks/models.py | 77 --- lms/djangoapps/bookmarks/services.py | 88 --- lms/djangoapps/bookmarks/tests/factories.py | 25 - lms/djangoapps/bookmarks/tests/test_api.py | 230 ------- lms/djangoapps/bookmarks/tests/test_models.py | 99 ---- .../bookmarks/tests/test_services.py | 102 ---- lms/djangoapps/bookmarks/tests/test_views.py | 560 ------------------ lms/djangoapps/courseware/module_render.py | 2 +- lms/envs/common.py | 1 + lms/urls.py | 2 +- .../core}/djangoapps/bookmarks/__init__.py | 4 + .../core}/djangoapps/bookmarks/api.py | 14 +- .../bookmarks/migrations/0001_initial.py | 0 ...d_bookmark_display_name__del_field_book.py | 127 ++++ .../bookmarks/migrations/__init__.py | 0 openedx/core/djangoapps/bookmarks/models.py | 246 ++++++++ .../core}/djangoapps/bookmarks/serializers.py | 15 +- openedx/core/djangoapps/bookmarks/services.py | 138 +++++ openedx/core/djangoapps/bookmarks/signals.py | 20 + openedx/core/djangoapps/bookmarks/startup.py | 5 + openedx/core/djangoapps/bookmarks/tasks.py | 160 +++++ .../djangoapps/bookmarks/tests/__init__.py | 0 .../djangoapps/bookmarks/tests/factories.py | 39 ++ .../djangoapps/bookmarks/tests/test_api.py | 212 +++++++ .../djangoapps/bookmarks/tests/test_models.py | 490 +++++++++++++++ .../bookmarks/tests/test_services.py | 84 +++ .../djangoapps/bookmarks/tests/test_tasks.py | 166 ++++++ .../djangoapps/bookmarks/tests/test_views.py | 540 +++++++++++++++++ .../core}/djangoapps/bookmarks/urls.py | 0 .../core}/djangoapps/bookmarks/views.py | 9 +- 33 files changed, 2280 insertions(+), 1190 deletions(-) delete mode 100644 lms/djangoapps/bookmarks/models.py delete mode 100644 lms/djangoapps/bookmarks/services.py delete mode 100644 lms/djangoapps/bookmarks/tests/factories.py delete mode 100644 lms/djangoapps/bookmarks/tests/test_api.py delete mode 100644 lms/djangoapps/bookmarks/tests/test_models.py delete mode 100644 lms/djangoapps/bookmarks/tests/test_services.py delete mode 100644 lms/djangoapps/bookmarks/tests/test_views.py rename {lms => openedx/core}/djangoapps/bookmarks/__init__.py (63%) rename {lms => openedx/core}/djangoapps/bookmarks/api.py (84%) rename {lms => openedx/core}/djangoapps/bookmarks/migrations/0001_initial.py (100%) create mode 100644 openedx/core/djangoapps/bookmarks/migrations/0002_auto__add_xblockcache__del_field_bookmark_display_name__del_field_book.py rename {lms => openedx/core}/djangoapps/bookmarks/migrations/__init__.py (100%) create mode 100644 openedx/core/djangoapps/bookmarks/models.py rename {lms => openedx/core}/djangoapps/bookmarks/serializers.py (73%) create mode 100644 openedx/core/djangoapps/bookmarks/services.py create mode 100644 openedx/core/djangoapps/bookmarks/signals.py create mode 100644 openedx/core/djangoapps/bookmarks/startup.py create mode 100644 openedx/core/djangoapps/bookmarks/tasks.py rename {lms => openedx/core}/djangoapps/bookmarks/tests/__init__.py (100%) create mode 100644 openedx/core/djangoapps/bookmarks/tests/factories.py create mode 100644 openedx/core/djangoapps/bookmarks/tests/test_api.py create mode 100644 openedx/core/djangoapps/bookmarks/tests/test_models.py create mode 100644 openedx/core/djangoapps/bookmarks/tests/test_services.py create mode 100644 openedx/core/djangoapps/bookmarks/tests/test_tasks.py create mode 100644 openedx/core/djangoapps/bookmarks/tests/test_views.py rename {lms => openedx/core}/djangoapps/bookmarks/urls.py (100%) rename {lms => openedx/core}/djangoapps/bookmarks/views.py (97%) diff --git a/cms/envs/common.py b/cms/envs/common.py index af89b610c3..fc9919aff5 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -803,6 +803,9 @@ INSTALLED_APPS = ( # edX Proctoring 'edx_proctoring', + # Bookmarks + 'openedx.core.djangoapps.bookmarks', + # programs support 'openedx.core.djangoapps.programs', diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 0ee4e37d41..307ed51a05 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -6,7 +6,7 @@ from .exceptions import (ItemNotFoundError, NoPathToItem) LOGGER = getLogger(__name__) -def path_to_location(modulestore, usage_key): +def path_to_location(modulestore, usage_key, full_path=False): ''' Try to find a course_id/chapter/section[/position] path to location in modulestore. The courseware insists that the first level in the course is @@ -15,6 +15,7 @@ def path_to_location(modulestore, usage_key): Args: modulestore: which store holds the relevant objects usage_key: :class:`UsageKey` the id of the location to which to generate the path + full_path: :class:`Bool` if True, return the full path to location. Default is False. Raises ItemNotFoundError if the location doesn't exist. @@ -81,6 +82,9 @@ def path_to_location(modulestore, usage_key): if path is None: raise NoPathToItem(usage_key) + if full_path: + return path + n = len(path) course_id = path[0].course_key # pull out the location names diff --git a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py index 224a1dbc22..8d3224db2a 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/django_utils.py @@ -18,10 +18,12 @@ from openedx.core.lib.tempdir import mkdtemp_clean from xmodule.contentstore.django import _CONTENTSTORE from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore, clear_existing_modulestores +from xmodule.modulestore.django import modulestore, clear_existing_modulestores, SignalHandler from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST from xmodule.modulestore.tests.factories import XMODULE_FACTORY_LOCK +from openedx.core.djangoapps.bookmarks.signals import trigger_update_xblocks_cache_task + class StoreConstructors(object): """Enumeration of store constructor types.""" @@ -405,6 +407,8 @@ class ModuleStoreTestCase(TestCase): super(ModuleStoreTestCase, self).setUp() + SignalHandler.course_published.disconnect(trigger_update_xblocks_cache_task) + self.store = modulestore() uname = 'testuser' diff --git a/lms/djangoapps/bookmarks/models.py b/lms/djangoapps/bookmarks/models.py deleted file mode 100644 index a9148af8de..0000000000 --- a/lms/djangoapps/bookmarks/models.py +++ /dev/null @@ -1,77 +0,0 @@ -""" -Models for Bookmarks. -""" - -from django.contrib.auth.models import User -from django.db import models - -from jsonfield.fields import JSONField -from model_utils.models import TimeStampedModel - -from xmodule.modulestore.django import modulestore -from xmodule_django.models import CourseKeyField, LocationKeyField - - -class Bookmark(TimeStampedModel): - """ - Bookmarks model. - """ - user = models.ForeignKey(User, db_index=True) - course_key = CourseKeyField(max_length=255, db_index=True) - usage_key = LocationKeyField(max_length=255, db_index=True) - display_name = models.CharField(max_length=255, default='', help_text='Display name of block') - path = JSONField(help_text='Path in course tree to the block') - - @classmethod - def create(cls, bookmark_data): - """ - Create a Bookmark object. - - Arguments: - bookmark_data (dict): The data to create the object with. - - Returns: - A Bookmark object. - - Raises: - ItemNotFoundError: If no block exists for the usage_key. - """ - usage_key = bookmark_data.pop('usage_key') - usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) - - block = modulestore().get_item(usage_key) - - bookmark_data['course_key'] = usage_key.course_key - bookmark_data['display_name'] = block.display_name - bookmark_data['path'] = cls.get_path(block) - user = bookmark_data.pop('user') - - return cls.objects.get_or_create(usage_key=usage_key, user=user, defaults=bookmark_data) - - @staticmethod - def get_path(block): - """ - Returns data for the path to the block in the course tree. - - Arguments: - block (XBlock): The block whose path is required. - - Returns: - list of dicts of the form {'usage_id': , 'display_name': }. - """ - parent = block.get_parent() - parents_data = [] - - while parent is not None and parent.location.block_type not in ['course']: - parents_data.append({"display_name": parent.display_name, "usage_id": unicode(parent.location)}) - parent = parent.get_parent() - - parents_data.reverse() - return parents_data - - @property - def resource_id(self): - """ - Return the resource id: {username,usage_id}. - """ - return "{0},{1}".format(self.user.username, self.usage_key) # pylint: disable=no-member diff --git a/lms/djangoapps/bookmarks/services.py b/lms/djangoapps/bookmarks/services.py deleted file mode 100644 index d5bb446fd4..0000000000 --- a/lms/djangoapps/bookmarks/services.py +++ /dev/null @@ -1,88 +0,0 @@ -""" -Bookmarks service. -""" -import logging - -from django.core.exceptions import ObjectDoesNotExist - -from xmodule.modulestore.exceptions import ItemNotFoundError - -from . import DEFAULT_FIELDS, OPTIONAL_FIELDS, api - -log = logging.getLogger(__name__) - - -class BookmarksService(object): - """ - A service that provides access to the bookmarks API. - """ - - def __init__(self, user, **kwargs): - super(BookmarksService, self).__init__(**kwargs) - self._user = user - - def bookmarks(self, course_key): - """ - Return a list of bookmarks for the course for the current user. - - Arguments: - course_key: CourseKey of the course for which to retrieve the user's bookmarks for. - - Returns: - list of dict: - """ - return api.get_bookmarks(self._user, course_key=course_key, fields=DEFAULT_FIELDS + OPTIONAL_FIELDS) - - def is_bookmarked(self, usage_key): - """ - Return whether the block has been bookmarked by the user. - - Arguments: - usage_key: UsageKey of the block. - - Returns: - Bool - """ - try: - api.get_bookmark(user=self._user, usage_key=usage_key) - except ObjectDoesNotExist: - log.error(u'Bookmark with usage_id: %s does not exist.', usage_key) - return False - - return True - - def set_bookmarked(self, usage_key): - """ - Adds a bookmark for the block. - - Arguments: - usage_key: UsageKey of the block. - - Returns: - Bool indicating whether the bookmark was added. - """ - try: - api.create_bookmark(user=self._user, usage_key=usage_key) - except ItemNotFoundError: - log.error(u'Block with usage_id: %s not found.', usage_key) - return False - - return True - - def unset_bookmarked(self, usage_key): - """ - Removes the bookmark for the block. - - Arguments: - usage_key: UsageKey of the block. - - Returns: - Bool indicating whether the bookmark was removed. - """ - try: - api.delete_bookmark(self._user, usage_key=usage_key) - except ObjectDoesNotExist: - log.error(u'Bookmark with usage_id: %s does not exist.', usage_key) - return False - - return True diff --git a/lms/djangoapps/bookmarks/tests/factories.py b/lms/djangoapps/bookmarks/tests/factories.py deleted file mode 100644 index 50422ec21b..0000000000 --- a/lms/djangoapps/bookmarks/tests/factories.py +++ /dev/null @@ -1,25 +0,0 @@ -""" -Factories for Bookmark models. -""" - -from factory.django import DjangoModelFactory -from factory import SubFactory -from functools import partial - -from student.tests.factories import UserFactory -from opaque_keys.edx.locations import SlashSeparatedCourseKey -from ..models import Bookmark - -COURSE_KEY = SlashSeparatedCourseKey(u'edX', u'test_course', u'test') -LOCATION = partial(COURSE_KEY.make_usage_key, u'problem') - - -class BookmarkFactory(DjangoModelFactory): - """ Simple factory class for generating Bookmark """ - FACTORY_FOR = Bookmark - - user = SubFactory(UserFactory) - course_key = COURSE_KEY - usage_key = LOCATION('usage_id') - display_name = "" - path = list() diff --git a/lms/djangoapps/bookmarks/tests/test_api.py b/lms/djangoapps/bookmarks/tests/test_api.py deleted file mode 100644 index bbd867daa9..0000000000 --- a/lms/djangoapps/bookmarks/tests/test_api.py +++ /dev/null @@ -1,230 +0,0 @@ -""" -Tests for bookmarks api. -""" - -from django.core.exceptions import ObjectDoesNotExist - -from opaque_keys.edx.keys import UsageKey - -from student.tests.factories import UserFactory - -from util.testing import EventTestMixin - -from xmodule.modulestore.exceptions import ItemNotFoundError -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase - -from .factories import BookmarkFactory -from .. import api, DEFAULT_FIELDS, OPTIONAL_FIELDS -from ..models import Bookmark - - -class BookmarkApiEventTestMixin(EventTestMixin): - """ Mixin for verifying that bookmark api events were emitted during a test. """ - def setUp(self): # pylint: disable=arguments-differ - super(BookmarkApiEventTestMixin, self).setUp('lms.djangoapps.bookmarks.api.tracker') - - def assert_bookmark_event_emitted(self, event_name, course_id, bookmark_id, usage_key): - """ Assert that an event has been emitted. """ - self.assert_event_emitted( - event_name, - course_id=course_id, - bookmark_id=bookmark_id, - component_type=usage_key.category, - component_usage_id=unicode(usage_key), - ) - - -class BookmarksAPITests(BookmarkApiEventTestMixin, ModuleStoreTestCase): - """ - These tests cover the parts of the API methods. - """ - - def setUp(self): - super(BookmarksAPITests, self).setUp() - - self.user = UserFactory.create(password='test') - self.other_user = UserFactory.create(password='test') - - self.course = CourseFactory.create(display_name='An Introduction to API Testing') - self.course_id = unicode(self.course.id) - - self.chapter = ItemFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 1' - ) - self.sequential = ItemFactory.create( - parent_location=self.chapter.location, category='sequential', display_name='Lesson 1' - ) - self.vertical = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 1' - ) - self.vertical_1 = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 1.1' - ) - self.bookmark = BookmarkFactory.create( - user=self.user, - course_key=self.course_id, - usage_key=self.vertical.location, - display_name=self.vertical.display_name - ) - - self.course_2 = CourseFactory.create(display_name='An Introduction to API Testing 2') - self.chapter_2 = ItemFactory.create( - parent_location=self.course_2.location, category='chapter', display_name='Week 2' - ) - self.sequential_2 = ItemFactory.create( - parent_location=self.chapter_2.location, category='sequential', display_name='Lesson 2' - ) - self.vertical_2 = ItemFactory.create( - parent_location=self.sequential_2.location, category='vertical', display_name='Subsection 2' - ) - self.bookmark_2 = BookmarkFactory.create( - user=self.user, - course_key=self.course_2.id, - usage_key=self.vertical_2.location, - display_name=self.vertical_2.display_name - ) - self.all_fields = DEFAULT_FIELDS + OPTIONAL_FIELDS - - self.reset_tracker() - - def assert_bookmark_response(self, response_data, bookmark, optional_fields=False): - """ - Determines if the given response data (dict) matches the given bookmark. - """ - self.assertEqual(response_data['id'], '%s,%s' % (self.user.username, unicode(bookmark.usage_key))) - self.assertEqual(response_data['course_id'], unicode(bookmark.course_key)) - self.assertEqual(response_data['usage_id'], unicode(bookmark.usage_key)) - self.assertEqual(response_data['block_type'], unicode(bookmark.usage_key.block_type)) - self.assertIsNotNone(response_data['created']) - - if optional_fields: - self.assertEqual(response_data['display_name'], bookmark.display_name) - self.assertEqual(response_data['path'], bookmark.path) - - def test_get_bookmark(self): - """ - Verifies that get_bookmark returns data as expected. - """ - bookmark_data = api.get_bookmark(user=self.user, usage_key=self.vertical.location) - self.assert_bookmark_response(bookmark_data, self.bookmark) - - # With Optional fields. - bookmark_data = api.get_bookmark( - user=self.user, - usage_key=self.vertical.location, - fields=self.all_fields - ) - self.assert_bookmark_response(bookmark_data, self.bookmark, optional_fields=True) - - def test_get_bookmark_raises_error(self): - """ - Verifies that get_bookmark raises error as expected. - """ - with self.assertRaises(ObjectDoesNotExist): - api.get_bookmark(user=self.other_user, usage_key=self.vertical.location) - - def test_get_bookmarks(self): - """ - Verifies that get_bookmarks returns data as expected. - """ - # Without course key. - bookmarks_data = api.get_bookmarks(user=self.user) - self.assertEqual(len(bookmarks_data), 2) - # Assert them in ordered manner. - self.assert_bookmark_response(bookmarks_data[0], self.bookmark_2) - self.assert_bookmark_response(bookmarks_data[1], self.bookmark) - - # With course key. - bookmarks_data = api.get_bookmarks(user=self.user, course_key=self.course.id) - self.assertEqual(len(bookmarks_data), 1) - self.assert_bookmark_response(bookmarks_data[0], self.bookmark) - - # With optional fields. - bookmarks_data = api.get_bookmarks(user=self.user, course_key=self.course.id, fields=self.all_fields) - self.assertEqual(len(bookmarks_data), 1) - self.assert_bookmark_response(bookmarks_data[0], self.bookmark, optional_fields=True) - - # Without Serialized. - bookmarks = api.get_bookmarks(user=self.user, course_key=self.course.id, serialized=False) - self.assertEqual(len(bookmarks), 1) - self.assertTrue(bookmarks.model is Bookmark) # pylint: disable=no-member - self.assertEqual(bookmarks[0], self.bookmark) - - def test_create_bookmark(self): - """ - Verifies that create_bookmark create & returns data as expected. - """ - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 1) - - bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_1.location) - - self.assert_bookmark_event_emitted( - 'edx.bookmark.added', - self.course_id, - bookmark_data['id'], - self.vertical_1.location - ) - - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) - - def test_create_bookmark_do_not_create_duplicates(self): - """ - Verifies that create_bookmark do not create duplicate bookmarks. - """ - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 1) - bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_1.location) - - self.assert_bookmark_event_emitted( - 'edx.bookmark.added', - self.course_id, - bookmark_data['id'], - self.vertical_1.location - ) - - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) - - self.reset_tracker() - - bookmark_data_2 = api.create_bookmark(user=self.user, usage_key=self.vertical_1.location) - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) - self.assertEqual(bookmark_data, bookmark_data_2) - - self.assert_no_events_were_emitted() - - def test_create_bookmark_raises_error(self): - """ - Verifies that create_bookmark raises error as expected. - """ - with self.assertRaises(ItemNotFoundError): - api.create_bookmark(user=self.user, usage_key=UsageKey.from_string('i4x://brb/100/html/340ef1771a0940')) - - self.assert_no_events_were_emitted() - - def test_delete_bookmark(self): - """ - Verifies that delete_bookmark removes bookmark as expected. - """ - self.assertEqual(len(api.get_bookmarks(user=self.user)), 2) - - api.delete_bookmark(user=self.user, usage_key=self.vertical.location) - - self.assert_bookmark_event_emitted( - 'edx.bookmark.removed', - self.course_id, - self.bookmark.resource_id, - self.vertical.location - ) - - bookmarks_data = api.get_bookmarks(user=self.user) - self.assertEqual(len(bookmarks_data), 1) - self.assertNotEqual(unicode(self.vertical.location), bookmarks_data[0]['usage_id']) - - def test_delete_bookmark_raises_error(self): - """ - Verifies that delete_bookmark raises error as expected. - """ - with self.assertRaises(ObjectDoesNotExist): - api.delete_bookmark(user=self.other_user, usage_key=self.vertical.location) - - self.assert_no_events_were_emitted() diff --git a/lms/djangoapps/bookmarks/tests/test_models.py b/lms/djangoapps/bookmarks/tests/test_models.py deleted file mode 100644 index 3fa0801863..0000000000 --- a/lms/djangoapps/bookmarks/tests/test_models.py +++ /dev/null @@ -1,99 +0,0 @@ -""" -Tests for Bookmarks models. -""" - -from bookmarks.models import Bookmark -from student.tests.factories import UserFactory - -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase - - -class BookmarkModelTest(ModuleStoreTestCase): - """ - Test the Bookmark model. - """ - def setUp(self): - super(BookmarkModelTest, self).setUp() - - self.user = UserFactory.create(password='test') - - self.course = CourseFactory.create(display_name='An Introduction to API Testing') - self.course_id = unicode(self.course.id) - - self.chapter = ItemFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 1' - ) - self.sequential = ItemFactory.create( - parent_location=self.chapter.location, category='sequential', display_name='Lesson 1' - ) - self.vertical = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 1' - ) - self.vertical_2 = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 2' - ) - - self.path = [ - {'display_name': self.chapter.display_name, 'usage_id': unicode(self.chapter.location)}, - {'display_name': self.sequential.display_name, 'usage_id': unicode(self.sequential.location)} - ] - - def get_bookmark_data(self, block): - """ - Returns bookmark data for testing. - """ - return { - 'user': self.user, - 'course_key': self.course.id, - 'usage_key': block.location, - 'display_name': block.display_name, - } - - def assert_valid_bookmark(self, bookmark_object, bookmark_data): - """ - Check if the given data matches the specified bookmark. - """ - self.assertEqual(bookmark_object.user, self.user) - self.assertEqual(bookmark_object.course_key, bookmark_data['course_key']) - self.assertEqual(bookmark_object.usage_key, self.vertical.location) - self.assertEqual(bookmark_object.display_name, bookmark_data['display_name']) - self.assertEqual(bookmark_object.path, self.path) - self.assertIsNotNone(bookmark_object.created) - - def test_create_bookmark_success(self): - """ - Tests creation of bookmark. - """ - bookmark_data = self.get_bookmark_data(self.vertical) - bookmark_object, __ = Bookmark.create(bookmark_data) - self.assert_valid_bookmark(bookmark_object, bookmark_data) - - def test_get_path(self): - """ - Tests creation of path with given block. - """ - path_object = Bookmark.get_path(block=self.vertical) - self.assertEqual(path_object, self.path) - - def test_get_path_with_given_chapter_block(self): - """ - Tests path for chapter level block. - """ - path_object = Bookmark.get_path(block=self.chapter) - self.assertEqual(len(path_object), 0) - - def test_get_path_with_given_sequential_block(self): - """ - Tests path for sequential level block. - """ - path_object = Bookmark.get_path(block=self.sequential) - self.assertEqual(len(path_object), 1) - self.assertEqual(path_object[0], self.path[0]) - - def test_get_path_returns_empty_list_for_unreachable_parent(self): - """ - Tests get_path returns empty list if block has no parent. - """ - path = Bookmark.get_path(block=self.course) - self.assertEqual(path, []) diff --git a/lms/djangoapps/bookmarks/tests/test_services.py b/lms/djangoapps/bookmarks/tests/test_services.py deleted file mode 100644 index bc6cae4097..0000000000 --- a/lms/djangoapps/bookmarks/tests/test_services.py +++ /dev/null @@ -1,102 +0,0 @@ -""" -Tests for bookmark services. -""" - -from opaque_keys.edx.keys import UsageKey - -from .factories import BookmarkFactory -from ..services import BookmarksService - -from student.tests.factories import UserFactory - -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase - - -class BookmarksAPITests(ModuleStoreTestCase): - """ - Tests the Bookmarks service. - """ - - def setUp(self): - super(BookmarksAPITests, self).setUp() - - self.user = UserFactory.create(password='test') - self.other_user = UserFactory.create(password='test') - - self.course = CourseFactory.create(display_name='An Introduction to API Testing') - self.course_id = unicode(self.course.id) - - self.chapter = ItemFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 1' - ) - self.sequential = ItemFactory.create( - parent_location=self.chapter.location, category='sequential', display_name='Lesson 1' - ) - self.vertical = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 1' - ) - self.vertical_1 = ItemFactory.create( - parent_location=self.sequential.location, category='vertical', display_name='Subsection 1.1' - ) - self.bookmark = BookmarkFactory.create( - user=self.user, - course_key=self.course_id, - usage_key=self.vertical.location, - display_name=self.vertical.display_name - ) - self.bookmark_service = BookmarksService(user=self.user) - - def assert_bookmark_response(self, response_data, bookmark): - """ - Determines if the given response data (dict) matches the specified bookmark. - """ - self.assertEqual(response_data['id'], '%s,%s' % (self.user.username, unicode(bookmark.usage_key))) - self.assertEqual(response_data['course_id'], unicode(bookmark.course_key)) - self.assertEqual(response_data['usage_id'], unicode(bookmark.usage_key)) - self.assertEqual(response_data['block_type'], unicode(bookmark.usage_key.block_type)) - self.assertIsNotNone(response_data['created']) - - self.assertEqual(response_data['display_name'], bookmark.display_name) - self.assertEqual(response_data['path'], bookmark.path) - - def test_get_bookmarks(self): - """ - Verifies get_bookmarks returns data as expected. - """ - - bookmarks_data = self.bookmark_service.bookmarks(course_key=self.course.id) - - self.assertEqual(len(bookmarks_data), 1) - self.assert_bookmark_response(bookmarks_data[0], self.bookmark) - - def test_is_bookmarked(self): - """ - Verifies is_bookmarked returns Bool as expected. - """ - self.assertTrue(self.bookmark_service.is_bookmarked(usage_key=self.vertical.location)) - self.assertFalse(self.bookmark_service.is_bookmarked(usage_key=self.vertical_1.location)) - - # Get bookmark that does not exist. - bookmark_service = BookmarksService(self.other_user) - self.assertFalse(bookmark_service.is_bookmarked(usage_key=self.vertical.location)) - - def test_set_bookmarked(self): - """ - Verifies set_bookmarked returns Bool as expected. - """ - # Assert False for item that does not exist. - self.assertFalse( - self.bookmark_service.set_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive")) - ) - - self.assertTrue(self.bookmark_service.set_bookmarked(usage_key=self.vertical_1.location)) - - def test_unset_bookmarked(self): - """ - Verifies unset_bookmarked returns Bool as expected. - """ - self.assertFalse( - self.bookmark_service.unset_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive")) - ) - self.assertTrue(self.bookmark_service.unset_bookmarked(usage_key=self.vertical.location)) diff --git a/lms/djangoapps/bookmarks/tests/test_views.py b/lms/djangoapps/bookmarks/tests/test_views.py deleted file mode 100644 index cebc701ccd..0000000000 --- a/lms/djangoapps/bookmarks/tests/test_views.py +++ /dev/null @@ -1,560 +0,0 @@ -""" -Tests for bookmark views. -""" - -import ddt -import json -import urllib -from django.core.urlresolvers import reverse -from mock import patch -from rest_framework.test import APIClient - -from student.tests.factories import UserFactory -from xmodule.modulestore import ModuleStoreEnum - -from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory - -from .factories import BookmarkFactory - -# pylint: disable=no-member - - -class BookmarksViewTestsMixin(ModuleStoreTestCase): - """ - Mixin for bookmarks views tests. - """ - test_password = 'test' - - def setUp(self): - super(BookmarksViewTestsMixin, self).setUp() - - self.anonymous_client = APIClient() - self.user = UserFactory.create(password=self.test_password) - self.create_test_data() - self.client = self.login_client(user=self.user) - - def login_client(self, user): - """ - Helper method for getting the client and user and logging in. Returns client. - """ - client = APIClient() - client.login(username=user.username, password=self.test_password) - return client - - def create_test_data(self): - """ - Creates the bookmarks test data. - """ - with self.store.default_store(ModuleStoreEnum.Type.split): - - self.course = CourseFactory.create() - self.course_id = unicode(self.course.id) - - chapter_1 = ItemFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 1' - ) - sequential_1 = ItemFactory.create( - parent_location=chapter_1.location, category='sequential', display_name='Lesson 1' - ) - self.vertical_1 = ItemFactory.create( - parent_location=sequential_1.location, category='vertical', display_name='Subsection 1' - ) - self.bookmark_1 = BookmarkFactory.create( - user=self.user, - course_key=self.course_id, - usage_key=self.vertical_1.location, - display_name=self.vertical_1.display_name - ) - chapter_2 = ItemFactory.create( - parent_location=self.course.location, category='chapter', display_name='Week 2' - ) - sequential_2 = ItemFactory.create( - parent_location=chapter_2.location, category='sequential', display_name='Lesson 2' - ) - vertical_2 = ItemFactory.create( - parent_location=sequential_2.location, category='vertical', display_name='Subsection 2' - ) - self.vertical_3 = ItemFactory.create( - parent_location=sequential_2.location, category='vertical', display_name='Subsection 3' - ) - self.bookmark_2 = BookmarkFactory.create( - user=self.user, - course_key=self.course_id, - usage_key=vertical_2.location, - display_name=vertical_2.display_name - ) - - # Other Course - self.other_course = CourseFactory.create(display_name='An Introduction to API Testing 2') - other_chapter = ItemFactory.create( - parent_location=self.other_course.location, category='chapter', display_name='Other Week 1' - ) - other_sequential = ItemFactory.create( - parent_location=other_chapter.location, category='sequential', display_name='Other Lesson 1' - ) - self.other_vertical = ItemFactory.create( - parent_location=other_sequential.location, category='vertical', display_name='Other Subsection 1' - ) - self.other_bookmark = BookmarkFactory.create( - user=self.user, - course_key=unicode(self.other_course.id), - usage_key=self.other_vertical.location, - display_name=self.other_vertical.display_name - ) - - def assert_valid_bookmark_response(self, response_data, bookmark, optional_fields=False): - """ - Determines if the given response data (dict) matches the specified bookmark. - """ - self.assertEqual(response_data['id'], '%s,%s' % (self.user.username, unicode(bookmark.usage_key))) - self.assertEqual(response_data['course_id'], unicode(bookmark.course_key)) - self.assertEqual(response_data['usage_id'], unicode(bookmark.usage_key)) - self.assertEqual(response_data['block_type'], unicode(bookmark.usage_key.block_type)) - self.assertIsNotNone(response_data['created']) - - if optional_fields: - self.assertEqual(response_data['display_name'], bookmark.display_name) - self.assertEqual(response_data['path'], bookmark.path) - - def send_get(self, client, url, query_parameters=None, expected_status=200): - """ - Helper method for sending a GET to the server. Verifies the expected status and returns the response. - """ - url = url + '?' + query_parameters if query_parameters else url - response = client.get(url) - self.assertEqual(expected_status, response.status_code) - return response - - def send_post(self, client, url, data, content_type='application/json', expected_status=201): - """ - Helper method for sending a POST to the server. Verifies the expected status and returns the response. - """ - response = client.post(url, data=json.dumps(data), content_type=content_type) - self.assertEqual(expected_status, response.status_code) - return response - - def send_delete(self, client, url, expected_status=204): - """ - Helper method for sending a DELETE to the server. Verifies the expected status and returns the response. - """ - response = client.delete(url) - self.assertEqual(expected_status, response.status_code) - return response - - -@ddt.ddt -class BookmarksListViewTests(BookmarksViewTestsMixin): - """ - This contains the tests for GET & POST methods of bookmark.views.BookmarksListView class - GET /api/bookmarks/v1/bookmarks/?course_id={course_id1} - POST /api/bookmarks/v1/bookmarks - """ - def assert_bookmark_listed_event_emitted(self, mock_tracker, **kwargs): - """ Assert that edx.course.bookmark.listed event is emitted with correct data. """ - mock_tracker.assert_any_call( - 'edx.bookmark.listed', - kwargs - ) - - @ddt.data( - ('course_id={}', False), - ('course_id={}&fields=path,display_name', True), - ) - @ddt.unpack - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - def test_get_bookmarks_successfully(self, query_params, check_optionals, mock_tracker): - """ - Test that requesting bookmarks for a course returns records successfully in - expected order without optional fields. - """ - response = self.send_get( - client=self.client, - url=reverse('bookmarks'), - query_parameters=query_params.format(urllib.quote(self.course_id)) - ) - - bookmarks = response.data['results'] - - self.assertEqual(len(bookmarks), 2) - self.assertEqual(response.data['count'], 2) - self.assertEqual(response.data['num_pages'], 1) - - # As bookmarks are sorted by -created so we will compare in that order. - self.assert_valid_bookmark_response(bookmarks[0], self.bookmark_2, optional_fields=check_optionals) - self.assert_valid_bookmark_response(bookmarks[1], self.bookmark_1, optional_fields=check_optionals) - - self.assert_bookmark_listed_event_emitted( - mock_tracker, - course_id=self.course_id, - list_type='per_course', - bookmarks_count=2, - page_size=10, - page_number=1 - ) - - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - def test_get_bookmarks_with_pagination(self, mock_tracker): - """ - Test that requesting bookmarks for a course return results with pagination 200 code. - """ - query_parameters = 'course_id={}&page_size=1'.format(urllib.quote(self.course_id)) - response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) - - bookmarks = response.data['results'] - - # Pagination assertions. - self.assertEqual(response.data['count'], 2) - self.assertIn('page=2&page_size=1', response.data['next']) - self.assertEqual(response.data['num_pages'], 2) - - self.assertEqual(len(bookmarks), 1) - self.assert_valid_bookmark_response(bookmarks[0], self.bookmark_2) - - self.assert_bookmark_listed_event_emitted( - mock_tracker, - course_id=self.course_id, - list_type='per_course', - bookmarks_count=2, - page_size=1, - page_number=1 - ) - - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - def test_get_bookmarks_with_invalid_data(self, mock_tracker): - """ - Test that requesting bookmarks with invalid data returns 0 records. - """ - # Invalid course id. - response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters='course_id=invalid') - bookmarks = response.data['results'] - self.assertEqual(len(bookmarks), 0) - - self.assertFalse(mock_tracker.emit.called) # pylint: disable=maybe-no-member - - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - def test_get_all_bookmarks_when_course_id_not_given(self, mock_tracker): - """ - Test that requesting bookmarks returns all records for that user. - """ - # Without course id we would return all the bookmarks for that user. - response = self.send_get(client=self.client, url=reverse('bookmarks')) - bookmarks = response.data['results'] - self.assertEqual(len(bookmarks), 3) - self.assert_valid_bookmark_response(bookmarks[0], self.other_bookmark) - self.assert_valid_bookmark_response(bookmarks[1], self.bookmark_2) - self.assert_valid_bookmark_response(bookmarks[2], self.bookmark_1) - - self.assert_bookmark_listed_event_emitted( - mock_tracker, - list_type='all_courses', - bookmarks_count=3, - page_size=10, - page_number=1 - ) - - def test_anonymous_access(self): - """ - Test that an anonymous client (not logged in) cannot call GET or POST. - """ - query_parameters = 'course_id={}'.format(self.course_id) - self.send_get( - client=self.anonymous_client, - url=reverse('bookmarks'), - query_parameters=query_parameters, - expected_status=401 - ) - self.send_post( - client=self.anonymous_client, - url=reverse('bookmarks'), - data={'usage_id': 'test'}, - expected_status=401 - ) - - def test_post_bookmark_successfully(self): - """ - Test that posting a bookmark successfully returns newly created data with 201 code. - """ - response = self.send_post( - client=self.client, - url=reverse('bookmarks'), - data={'usage_id': unicode(self.vertical_3.location)} - ) - - # Assert Newly created bookmark. - self.assertEqual(response.data['id'], '%s,%s' % (self.user.username, unicode(self.vertical_3.location))) - self.assertEqual(response.data['course_id'], self.course_id) - self.assertEqual(response.data['usage_id'], unicode(self.vertical_3.location)) - self.assertIsNotNone(response.data['created']) - self.assertEqual(len(response.data['path']), 2) - self.assertEqual(response.data['display_name'], self.vertical_3.display_name) - - def test_post_bookmark_with_invalid_data(self): - """ - Test that posting a bookmark for a block with invalid usage id returns a 400. - Scenarios: - 1) Invalid usage id. - 2) Without usage id. - 3) With empty request.DATA - """ - # Send usage_id with invalid format. - response = self.send_post( - client=self.client, - url=reverse('bookmarks'), - data={'usage_id': 'invalid'}, - expected_status=400 - ) - self.assertEqual(response.data['user_message'], u'Invalid usage_id: invalid.') - - # Send data without usage_id. - response = self.send_post( - client=self.client, - url=reverse('bookmarks'), - data={'course_id': 'invalid'}, - expected_status=400 - ) - self.assertEqual(response.data['user_message'], u'Parameter usage_id not provided.') - self.assertEqual(response.data['developer_message'], u'Parameter usage_id not provided.') - - # Send empty data dictionary. - response = self.send_post( - client=self.client, - url=reverse('bookmarks'), - data={}, - expected_status=400 - ) - self.assertEqual(response.data['user_message'], u'No data provided.') - self.assertEqual(response.data['developer_message'], u'No data provided.') - - def test_post_bookmark_for_non_existing_block(self): - """ - Test that posting a bookmark for a block that does not exist returns a 400. - """ - response = self.send_post( - client=self.client, - url=reverse('bookmarks'), - data={'usage_id': 'i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21'}, - expected_status=400 - ) - self.assertEqual( - response.data['user_message'], - u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.' - ) - self.assertEqual( - response.data['developer_message'], - u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.' - ) - - def test_unsupported_methods(self): - """ - Test that DELETE and PUT are not supported. - """ - self.client.login(username=self.user.username, password=self.test_password) - self.assertEqual(405, self.client.put(reverse('bookmarks')).status_code) - self.assertEqual(405, self.client.delete(reverse('bookmarks')).status_code) - - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - @ddt.unpack - @ddt.data( - {'page_size': -1, 'expected_bookmarks_count': 2, 'expected_page_size': 10, 'expected_page_number': 1}, - {'page_size': 0, 'expected_bookmarks_count': 2, 'expected_page_size': 10, 'expected_page_number': 1}, - {'page_size': 999, 'expected_bookmarks_count': 2, 'expected_page_size': 500, 'expected_page_number': 1} - ) - def test_listed_event_for_different_page_size_values(self, mock_tracker, page_size, expected_bookmarks_count, - expected_page_size, expected_page_number): - """ Test that edx.course.bookmark.listed event values are as expected for different page size values """ - query_parameters = 'course_id={}&page_size={}'.format(urllib.quote(self.course_id), page_size) - - self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) - - self.assert_bookmark_listed_event_emitted( - mock_tracker, - course_id=self.course_id, - list_type='per_course', - bookmarks_count=expected_bookmarks_count, - page_size=expected_page_size, - page_number=expected_page_number - ) - - @patch('lms.djangoapps.bookmarks.views.tracker.emit') - def test_listed_event_for_page_number(self, mock_tracker): - """ Test that edx.course.bookmark.listed event values are as expected when we request a specific page number """ - self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters='page_size=2&page=2') - - self.assert_bookmark_listed_event_emitted( - mock_tracker, - list_type='all_courses', - bookmarks_count=3, - page_size=2, - page_number=2 - ) - - -@ddt.ddt -class BookmarksDetailViewTests(BookmarksViewTestsMixin): - """ - This contains the tests for GET & DELETE methods of bookmark.views.BookmarksDetailView class - """ - @ddt.data( - ('', False), - ('fields=path,display_name', True) - ) - @ddt.unpack - def test_get_bookmark_successfully(self, query_params, check_optionals): - """ - Test that requesting bookmark returns data with 200 code. - """ - response = self.send_get( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': unicode(self.vertical_1.location)} - ), - query_parameters=query_params - ) - data = response.data - self.assertIsNotNone(data) - self.assert_valid_bookmark_response(data, self.bookmark_1, optional_fields=check_optionals) - - def test_get_bookmark_that_belongs_to_other_user(self): - """ - Test that requesting bookmark that belongs to other user returns 404 status code. - """ - self.send_get( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)} - ), - expected_status=404 - ) - - def test_get_bookmark_that_does_not_exist(self): - """ - Test that requesting bookmark that does not exist returns 404 status code. - """ - response = self.send_get( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': 'i4x://arbi/100/html/340ef1771a0940'} - ), - expected_status=404 - ) - self.assertEqual( - response.data['user_message'], - 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' - ) - self.assertEqual( - response.data['developer_message'], - 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' - ) - - def test_get_bookmark_with_invalid_usage_id(self): - """ - Test that requesting bookmark with invalid usage id returns 400. - """ - response = self.send_get( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': 'i4x'} - ), - expected_status=404 - ) - self.assertEqual(response.data['user_message'], u'Invalid usage_id: i4x.') - - def test_anonymous_access(self): - """ - Test that an anonymous client (not logged in) cannot call GET or DELETE. - """ - url = reverse('bookmarks_detail', kwargs={'username': self.user.username, 'usage_id': 'i4x'}) - self.send_get( - client=self.anonymous_client, - url=url, - expected_status=401 - ) - self.send_delete( - client=self.anonymous_client, - url=url, - expected_status=401 - ) - - def test_delete_bookmark_successfully(self): - """ - Test that delete bookmark returns 204 status code with success. - """ - query_parameters = 'course_id={}'.format(urllib.quote(self.course_id)) - response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) - data = response.data - bookmarks = data['results'] - self.assertEqual(len(bookmarks), 2) - - self.send_delete( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': unicode(self.vertical_1.location)} - ) - ) - response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) - bookmarks = response.data['results'] - - self.assertEqual(len(bookmarks), 1) - - def test_delete_bookmark_that_belongs_to_other_user(self): - """ - Test that delete bookmark that belongs to other user returns 404. - """ - self.send_delete( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)} - ), - expected_status=404 - ) - - def test_delete_bookmark_that_does_not_exist(self): - """ - Test that delete bookmark that does not exist returns 404. - """ - response = self.send_delete( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': 'i4x://arbi/100/html/340ef1771a0940'} - ), - expected_status=404 - ) - self.assertEqual( - response.data['user_message'], - u'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' - ) - self.assertEqual( - response.data['developer_message'], - 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' - ) - - def test_delete_bookmark_with_invalid_usage_id(self): - """ - Test that delete bookmark with invalid usage id returns 400. - """ - response = self.send_delete( - client=self.client, - url=reverse( - 'bookmarks_detail', - kwargs={'username': self.user.username, 'usage_id': 'i4x'} - ), - expected_status=404 - ) - self.assertEqual(response.data['user_message'], u'Invalid usage_id: i4x.') - - def test_unsupported_methods(self): - """ - Test that POST and PUT are not supported. - """ - url = reverse('bookmarks_detail', kwargs={'username': self.user.username, 'usage_id': 'i4x'}) - self.client.login(username=self.user.username, password=self.test_password) - self.assertEqual(405, self.client.put(url).status_code) - self.assertEqual(405, self.client.post(url).status_code) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 08fb4c33cb..a334ad01e9 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -42,13 +42,13 @@ from courseware.entrance_exams import ( ) from edxmako.shortcuts import render_to_string from eventtracking import tracker -from lms.djangoapps.bookmarks.services import BookmarksService from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem, unquote_slashes, quote_slashes from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locations import SlashSeparatedCourseKey +from openedx.core.djangoapps.bookmarks.services import BookmarksService from openedx.core.lib.xblock_utils import ( replace_course_urls, replace_jump_to_id_urls, diff --git a/lms/envs/common.py b/lms/envs/common.py index de05b8eb23..5cd8420d46 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1909,6 +1909,7 @@ INSTALLED_APPS = ( 'xblock_django', # Bookmarks + 'openedx.core.djangoapps.bookmarks', 'bookmarks', # programs support diff --git a/lms/urls.py b/lms/urls.py index 39bd78fb60..41050a0c52 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -90,7 +90,7 @@ urlpatterns = ( url(r'^api/user/', include('openedx.core.djangoapps.user_api.urls')), # Bookmarks API endpoints - url(r'^api/bookmarks/', include('bookmarks.urls')), + url(r'^api/bookmarks/', include('openedx.core.djangoapps.bookmarks.urls')), # Profile Images API endpoints url(r'^api/profile_images/', include('openedx.core.djangoapps.profile_images.urls')), diff --git a/lms/djangoapps/bookmarks/__init__.py b/openedx/core/djangoapps/bookmarks/__init__.py similarity index 63% rename from lms/djangoapps/bookmarks/__init__.py rename to openedx/core/djangoapps/bookmarks/__init__.py index d5046012e2..11252411b1 100644 --- a/lms/djangoapps/bookmarks/__init__.py +++ b/openedx/core/djangoapps/bookmarks/__init__.py @@ -1,6 +1,8 @@ """ Bookmarks module. """ +from collections import namedtuple + DEFAULT_FIELDS = [ 'id', @@ -14,3 +16,5 @@ OPTIONAL_FIELDS = [ 'display_name', 'path', ] + +PathItem = namedtuple('PathItem', ['usage_key', 'display_name']) diff --git a/lms/djangoapps/bookmarks/api.py b/openedx/core/djangoapps/bookmarks/api.py similarity index 84% rename from lms/djangoapps/bookmarks/api.py rename to openedx/core/djangoapps/bookmarks/api.py index 330877c785..93bbe434bb 100644 --- a/lms/djangoapps/bookmarks/api.py +++ b/openedx/core/djangoapps/bookmarks/api.py @@ -22,7 +22,14 @@ def get_bookmark(user, usage_key, fields=None): Raises: ObjectDoesNotExist: If a bookmark with the parameters does not exist. """ - bookmark = Bookmark.objects.get(user=user, usage_key=usage_key) + bookmarks_queryset = Bookmark.objects + + if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: + bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') + else: + bookmarks_queryset = bookmarks_queryset.select_related('user') + + bookmark = bookmarks_queryset.get(user=user, usage_key=usage_key) return BookmarkSerializer(bookmark, context={'fields': fields}).data @@ -46,6 +53,11 @@ def get_bookmarks(user, course_key=None, fields=None, serialized=True): if course_key: bookmarks_queryset = bookmarks_queryset.filter(course_key=course_key) + if len(set(fields or []) & set(OPTIONAL_FIELDS)) > 0: + bookmarks_queryset = bookmarks_queryset.select_related('user', 'xblock_cache') + else: + bookmarks_queryset = bookmarks_queryset.select_related('user') + bookmarks_queryset = bookmarks_queryset.order_by('-created') if serialized: diff --git a/lms/djangoapps/bookmarks/migrations/0001_initial.py b/openedx/core/djangoapps/bookmarks/migrations/0001_initial.py similarity index 100% rename from lms/djangoapps/bookmarks/migrations/0001_initial.py rename to openedx/core/djangoapps/bookmarks/migrations/0001_initial.py diff --git a/openedx/core/djangoapps/bookmarks/migrations/0002_auto__add_xblockcache__del_field_bookmark_display_name__del_field_book.py b/openedx/core/djangoapps/bookmarks/migrations/0002_auto__add_xblockcache__del_field_bookmark_display_name__del_field_book.py new file mode 100644 index 0000000000..ca93d4102c --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/migrations/0002_auto__add_xblockcache__del_field_bookmark_display_name__del_field_book.py @@ -0,0 +1,127 @@ +# -*- coding: utf-8 -*- +from south.utils import datetime_utils as datetime +from south.db import db +from south.v2 import SchemaMigration +from django.db import models + + +class Migration(SchemaMigration): + + def forwards(self, orm): + # Adding model 'XBlockCache' + db.create_table('bookmarks_xblockcache', ( + ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)), + ('created', self.gf('model_utils.fields.AutoCreatedField')(default=datetime.datetime.now)), + ('modified', self.gf('model_utils.fields.AutoLastModifiedField')(default=datetime.datetime.now)), + ('course_key', self.gf('xmodule_django.models.CourseKeyField')(max_length=255, db_index=True)), + ('usage_key', self.gf('xmodule_django.models.LocationKeyField')(unique=True, max_length=255, db_index=True)), + ('display_name', self.gf('django.db.models.fields.CharField')(default='', max_length=255)), + ('_paths', self.gf('jsonfield.fields.JSONField')(default=[], db_column='paths')), + )) + db.send_create_signal('bookmarks', ['XBlockCache']) + + # Deleting field 'Bookmark.display_name' + db.delete_column('bookmarks_bookmark', 'display_name') + + # Deleting field 'Bookmark.path' + db.delete_column('bookmarks_bookmark', 'path') + + # Adding field 'Bookmark._path' + db.add_column('bookmarks_bookmark', '_path', + self.gf('jsonfield.fields.JSONField')(default='', db_column='path'), + keep_default=False) + + # Adding field 'Bookmark.xblock_cache' + db.add_column('bookmarks_bookmark', 'xblock_cache', + self.gf('django.db.models.fields.related.ForeignKey')(default=0, to=orm['bookmarks.XBlockCache']), + keep_default=False) + + # Adding unique constraint on 'Bookmark', fields ['user', 'usage_key'] + db.create_unique('bookmarks_bookmark', ['user_id', 'usage_key']) + + + def backwards(self, orm): + # Removing unique constraint on 'Bookmark', fields ['user', 'usage_key'] + db.delete_unique('bookmarks_bookmark', ['user_id', 'usage_key']) + + # Deleting model 'XBlockCache' + db.delete_table('bookmarks_xblockcache') + + # Adding field 'Bookmark.display_name' + db.add_column('bookmarks_bookmark', 'display_name', + self.gf('django.db.models.fields.CharField')(default='', max_length=255), + keep_default=False) + + # Adding field 'Bookmark.path' + db.add_column('bookmarks_bookmark', 'path', + self.gf('jsonfield.fields.JSONField')(default=''), + keep_default=False) + + # Deleting field 'Bookmark._path' + db.delete_column('bookmarks_bookmark', 'path') + + # Deleting field 'Bookmark.xblock_cache' + db.delete_column('bookmarks_bookmark', 'xblock_cache_id') + + + models = { + 'auth.group': { + 'Meta': {'object_name': 'Group'}, + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '80'}), + 'permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}) + }, + 'auth.permission': { + 'Meta': {'ordering': "('content_type__app_label', 'content_type__model', 'codename')", 'unique_together': "(('content_type', 'codename'),)", 'object_name': 'Permission'}, + 'codename': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'content_type': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['contenttypes.ContentType']"}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '50'}) + }, + 'auth.user': { + 'Meta': {'object_name': 'User'}, + 'date_joined': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'email': ('django.db.models.fields.EmailField', [], {'max_length': '75', 'blank': 'True'}), + 'first_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'groups': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Group']", 'symmetrical': 'False', 'blank': 'True'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'is_active': ('django.db.models.fields.BooleanField', [], {'default': 'True'}), + 'is_staff': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'is_superuser': ('django.db.models.fields.BooleanField', [], {'default': 'False'}), + 'last_login': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now'}), + 'last_name': ('django.db.models.fields.CharField', [], {'max_length': '30', 'blank': 'True'}), + 'password': ('django.db.models.fields.CharField', [], {'max_length': '128'}), + 'user_permissions': ('django.db.models.fields.related.ManyToManyField', [], {'to': "orm['auth.Permission']", 'symmetrical': 'False', 'blank': 'True'}), + 'username': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '30'}) + }, + 'bookmarks.bookmark': { + 'Meta': {'unique_together': "(('user', 'usage_key'),)", 'object_name': 'Bookmark'}, + '_path': ('jsonfield.fields.JSONField', [], {'db_column': "'path'"}), + 'course_key': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}), + 'usage_key': ('xmodule_django.models.LocationKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"}), + 'xblock_cache': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['bookmarks.XBlockCache']"}) + }, + 'bookmarks.xblockcache': { + 'Meta': {'object_name': 'XBlockCache'}, + '_paths': ('jsonfield.fields.JSONField', [], {'default': '[]', 'db_column': "'paths'"}), + 'course_key': ('xmodule_django.models.CourseKeyField', [], {'max_length': '255', 'db_index': 'True'}), + 'created': ('model_utils.fields.AutoCreatedField', [], {'default': 'datetime.datetime.now'}), + 'display_name': ('django.db.models.fields.CharField', [], {'default': "''", 'max_length': '255'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'modified': ('model_utils.fields.AutoLastModifiedField', [], {'default': 'datetime.datetime.now'}), + 'usage_key': ('xmodule_django.models.LocationKeyField', [], {'unique': 'True', 'max_length': '255', 'db_index': 'True'}) + }, + 'contenttypes.contenttype': { + 'Meta': {'ordering': "('name',)", 'unique_together': "(('app_label', 'model'),)", 'object_name': 'ContentType', 'db_table': "'django_content_type'"}, + 'app_label': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}), + 'model': ('django.db.models.fields.CharField', [], {'max_length': '100'}), + 'name': ('django.db.models.fields.CharField', [], {'max_length': '100'}) + } + } + + complete_apps = ['bookmarks'] \ No newline at end of file diff --git a/lms/djangoapps/bookmarks/migrations/__init__.py b/openedx/core/djangoapps/bookmarks/migrations/__init__.py similarity index 100% rename from lms/djangoapps/bookmarks/migrations/__init__.py rename to openedx/core/djangoapps/bookmarks/migrations/__init__.py diff --git a/openedx/core/djangoapps/bookmarks/models.py b/openedx/core/djangoapps/bookmarks/models.py new file mode 100644 index 0000000000..d3013cce28 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/models.py @@ -0,0 +1,246 @@ +""" +Models for Bookmarks. +""" +import logging + +from django.contrib.auth.models import User +from django.db import models + +from jsonfield.fields import JSONField +from model_utils.models import TimeStampedModel + +from opaque_keys.edx.keys import UsageKey +from xmodule.modulestore import search +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem +from xmodule_django.models import CourseKeyField, LocationKeyField + +from . import PathItem + +log = logging.getLogger(__name__) + + +def prepare_path_for_serialization(path): + """ + Return the data from a list of PathItems ready for serialization to json. + """ + return [(unicode(path_item.usage_key), path_item.display_name) for path_item in path] + + +def parse_path_data(path_data): + """ + Return a list of PathItems constructed from parsing path_data. + """ + path = [] + for item in path_data: + usage_key = UsageKey.from_string(item[0]) + usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) + path.append(PathItem(usage_key, item[1])) + return path + + +class Bookmark(TimeStampedModel): + """ + Bookmarks model. + """ + user = models.ForeignKey(User, db_index=True) + course_key = CourseKeyField(max_length=255, db_index=True) + usage_key = LocationKeyField(max_length=255, db_index=True) + _path = JSONField(db_column='path', help_text='Path in course tree to the block') + + xblock_cache = models.ForeignKey('bookmarks.XBlockCache') + + class Meta(object): + """ + Bookmark metadata. + """ + unique_together = ('user', 'usage_key') + + @classmethod + def create(cls, data): + """ + Create a Bookmark object. + + Arguments: + data (dict): The data to create the object with. + + Returns: + A Bookmark object. + + Raises: + ItemNotFoundError: If no block exists for the usage_key. + """ + data = dict(data) + + usage_key = data.pop('usage_key') + usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) + + with modulestore().bulk_operations(usage_key.course_key): + block = modulestore().get_item(usage_key) + + xblock_cache = XBlockCache.create({ + 'usage_key': usage_key, + 'display_name': block.display_name, + }) + data['_path'] = prepare_path_for_serialization(Bookmark.updated_path(usage_key, xblock_cache)) + + data['course_key'] = usage_key.course_key + data['xblock_cache'] = xblock_cache + + user = data.pop('user') + + bookmark, created = cls.objects.get_or_create(usage_key=usage_key, user=user, defaults=data) + return bookmark, created + + @property + def resource_id(self): + """ + Return the resource id: {username,usage_id}. + """ + return "{0},{1}".format(self.user.username, self.usage_key) # pylint: disable=no-member + + @property + def display_name(self): + """ + Return the display_name from self.xblock_cache. + + Returns: + String. + """ + return self.xblock_cache.display_name # pylint: disable=no-member + + @property + def path(self): + """ + Return the path to the bookmark's block after checking self.xblock_cache. + + Returns: + List of dicts. + """ + if self.modified < self.xblock_cache.modified: # pylint: disable=no-member + path = Bookmark.updated_path(self.usage_key, self.xblock_cache) + self._path = prepare_path_for_serialization(path) + self.save() # Always save so that self.modified is updated. + return path + + return parse_path_data(self._path) + + @staticmethod + def updated_path(usage_key, xblock_cache): + """ + Return the update-to-date path. + + xblock_cache.paths is the list of all possible paths to a block + constructed by doing a DFS of the tree. However, in case of DAGS, + which section jump_to_id() takes the user to depends on the + modulestore. If xblock_cache.paths has only one item, we can + just use it. Otherwise, we use path_to_location() to get the path + jump_to_id() will take the user to. + """ + if xblock_cache.paths and len(xblock_cache.paths) == 1: + return xblock_cache.paths[0] + + return Bookmark.get_path(usage_key) + + @staticmethod + def get_path(usage_key): + """ + Returns data for the path to the block in the course graph. + + Note: In case of multiple paths to the block from the course + root, this function returns a path arbitrarily but consistently, + depending on the modulestore. In the future, we may want to + extend it to check which of the paths, the user has access to + and return its data. + + Arguments: + block (XBlock): The block whose path is required. + + Returns: + list of PathItems + """ + with modulestore().bulk_operations(usage_key.course_key): + try: + path = search.path_to_location(modulestore(), usage_key, full_path=True) + except ItemNotFoundError: + log.error(u'Block with usage_key: %s not found.', usage_key) + return [] + except NoPathToItem: + log.error(u'No path to block with usage_key: %s.', usage_key) + return [] + + path_data = [] + for ancestor_usage_key in path: + if ancestor_usage_key != usage_key and ancestor_usage_key.block_type != 'course': # pylint: disable=no-member + try: + block = modulestore().get_item(ancestor_usage_key) + except ItemNotFoundError: + return [] # No valid path can be found. + + path_data.append( + PathItem(usage_key=block.location, display_name=block.display_name) + ) + + return path_data + + +class XBlockCache(TimeStampedModel): + """ + XBlockCache model to store info about xblocks. + """ + + course_key = CourseKeyField(max_length=255, db_index=True) + usage_key = LocationKeyField(max_length=255, db_index=True, unique=True) + + display_name = models.CharField(max_length=255, default='') + _paths = JSONField( + db_column='paths', default=[], help_text='All paths in course tree to the corresponding block.' + ) + + @property + def paths(self): + """ + Return paths. + + Returns: + list of list of PathItems. + """ + return [parse_path_data(path) for path in self._paths] if self._paths else self._paths + + @paths.setter + def paths(self, value): + """ + Set paths. + + Arguments: + value (list of list of PathItems): The list of paths to cache. + """ + self._paths = [prepare_path_for_serialization(path) for path in value] if value else value + + @classmethod + def create(cls, data): + """ + Create an XBlockCache object. + + Arguments: + data (dict): The data to create the object with. + + Returns: + An XBlockCache object. + """ + data = dict(data) + + usage_key = data.pop('usage_key') + usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key)) + + data['course_key'] = usage_key.course_key + + xblock_cache, created = cls.objects.get_or_create(usage_key=usage_key, defaults=data) + + if not created: + new_display_name = data.get('display_name', xblock_cache.display_name) + if xblock_cache.display_name != new_display_name: + xblock_cache.display_name = new_display_name + xblock_cache.save() + + return xblock_cache diff --git a/lms/djangoapps/bookmarks/serializers.py b/openedx/core/djangoapps/bookmarks/serializers.py similarity index 73% rename from lms/djangoapps/bookmarks/serializers.py rename to openedx/core/djangoapps/bookmarks/serializers.py index e0d9e80b6f..d20a036efb 100644 --- a/lms/djangoapps/bookmarks/serializers.py +++ b/openedx/core/djangoapps/bookmarks/serializers.py @@ -15,7 +15,8 @@ class BookmarkSerializer(serializers.ModelSerializer): course_id = serializers.Field(source='course_key') usage_id = serializers.Field(source='usage_key') block_type = serializers.Field(source='usage_key.block_type') - path = serializers.Field(source='path') + display_name = serializers.Field(source='display_name') + path = serializers.SerializerMethodField('path_data') def __init__(self, *args, **kwargs): # Don't pass the 'fields' arg up to the superclass @@ -44,3 +45,15 @@ class BookmarkSerializer(serializers.ModelSerializer): 'path', 'created', ) + + def resource_id(self, bookmark): + """ + Return the REST resource id: {username,usage_id}. + """ + return "{0},{1}".format(bookmark.user.username, bookmark.usage_key) + + def path_data(self, bookmark): + """ + Serialize and return the path data of the bookmark. + """ + return [path_item._asdict() for path_item in bookmark.path] diff --git a/openedx/core/djangoapps/bookmarks/services.py b/openedx/core/djangoapps/bookmarks/services.py new file mode 100644 index 0000000000..f59a008621 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/services.py @@ -0,0 +1,138 @@ +""" +Bookmarks service. +""" +import logging + +from django.core.exceptions import ObjectDoesNotExist + +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + +from request_cache.middleware import RequestCache + +from . import DEFAULT_FIELDS, OPTIONAL_FIELDS, api + +log = logging.getLogger(__name__) + +CACHE_KEY_TEMPLATE = u"bookmarks.list.{}.{}" + + +class BookmarksService(object): + """ + A service that provides access to the bookmarks API. + + When bookmarks() or is_bookmarked() is called for the + first time, the service fetches and caches all the bookmarks + of the user for the relevant course. So multiple calls to + get bookmark status during a request (for, example when + rendering courseware and getting bookmarks status for search + results) will not cause repeated queries to the database. + """ + + def __init__(self, user, **kwargs): + super(BookmarksService, self).__init__(**kwargs) + self._user = user + + def _bookmarks_cache(self, course_key, fetch=False): + """ + Return the user's bookmarks cache for a particular course. + + Arguments: + course_key (CourseKey): course_key of the course whose bookmarks cache should be returned. + fetch (Bool): if the bookmarks should be fetched and cached if they already aren't. + """ + if hasattr(modulestore(), 'fill_in_run'): + course_key = modulestore().fill_in_run(course_key) + if course_key.run is None: + return [] + cache_key = CACHE_KEY_TEMPLATE.format(self._user.id, course_key) + + bookmarks_cache = RequestCache.get_request_cache().data.get(cache_key, None) + if bookmarks_cache is None and fetch is True: + bookmarks_cache = api.get_bookmarks( + self._user, course_key=course_key, fields=DEFAULT_FIELDS + OPTIONAL_FIELDS + ) + RequestCache.get_request_cache().data[cache_key] = bookmarks_cache + + return bookmarks_cache + + def bookmarks(self, course_key): + """ + Return a list of bookmarks for the course for the current user. + + Arguments: + course_key: CourseKey of the course for which to retrieve the user's bookmarks for. + + Returns: + list of dict: + """ + return self._bookmarks_cache(course_key, fetch=True) + + def is_bookmarked(self, usage_key): + """ + Return whether the block has been bookmarked by the user. + + Arguments: + usage_key: UsageKey of the block. + + Returns: + Bool + """ + usage_id = unicode(usage_key) + bookmarks_cache = self._bookmarks_cache(usage_key.course_key, fetch=True) + for bookmark in bookmarks_cache: + if bookmark['usage_id'] == usage_id: + return True + + return False + + def set_bookmarked(self, usage_key): + """ + Adds a bookmark for the block. + + Arguments: + usage_key: UsageKey of the block. + + Returns: + Bool indicating whether the bookmark was added. + """ + try: + bookmark = api.create_bookmark(user=self._user, usage_key=usage_key) + except ItemNotFoundError: + log.error(u'Block with usage_id: %s not found.', usage_key) + return False + + bookmarks_cache = self._bookmarks_cache(usage_key.course_key) + if bookmarks_cache is not None: + bookmarks_cache.append(bookmark) + + return True + + def unset_bookmarked(self, usage_key): + """ + Removes the bookmark for the block. + + Arguments: + usage_key: UsageKey of the block. + + Returns: + Bool indicating whether the bookmark was removed. + """ + try: + api.delete_bookmark(self._user, usage_key=usage_key) + except ObjectDoesNotExist: + log.error(u'Bookmark with usage_id: %s does not exist.', usage_key) + return False + + bookmarks_cache = self._bookmarks_cache(usage_key.course_key) + if bookmarks_cache is not None: + deleted_bookmark_index = None + usage_id = unicode(usage_key) + for index, bookmark in enumerate(bookmarks_cache): + if bookmark['usage_id'] == usage_id: + deleted_bookmark_index = index + break + if deleted_bookmark_index is not None: + bookmarks_cache.pop(deleted_bookmark_index) + + return True diff --git a/openedx/core/djangoapps/bookmarks/signals.py b/openedx/core/djangoapps/bookmarks/signals.py new file mode 100644 index 0000000000..0f04939d78 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/signals.py @@ -0,0 +1,20 @@ +""" +Signals for bookmarks. +""" +from importlib import import_module + +from django.dispatch.dispatcher import receiver + +from xmodule.modulestore.django import SignalHandler + + +@receiver(SignalHandler.course_published) +def trigger_update_xblocks_cache_task(sender, course_key, **kwargs): # pylint: disable=invalid-name,unused-argument + """ + Trigger update_xblocks_cache() when course_published signal is fired. + """ + tasks = import_module('openedx.core.djangoapps.bookmarks.tasks') # Importing tasks early causes issues in tests. + + # Note: The countdown=0 kwarg is set to ensure the method below does not attempt to access the course + # before the signal emitter has finished all operations. This is also necessary to ensure all tests pass. + tasks.update_xblocks_cache.apply_async([unicode(course_key)], countdown=0) diff --git a/openedx/core/djangoapps/bookmarks/startup.py b/openedx/core/djangoapps/bookmarks/startup.py new file mode 100644 index 0000000000..d3867d383c --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/startup.py @@ -0,0 +1,5 @@ +""" +Setup the signals on startup. +""" + +from . import signals # pylint: disable=unused-import diff --git a/openedx/core/djangoapps/bookmarks/tasks.py b/openedx/core/djangoapps/bookmarks/tasks.py new file mode 100644 index 0000000000..9eec9ceb9c --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tasks.py @@ -0,0 +1,160 @@ +""" +Tasks for bookmarks. +""" +import logging + +from django.db import transaction + +from celery.task import task # pylint: disable=import-error,no-name-in-module +from opaque_keys.edx.keys import CourseKey +from xmodule.modulestore.django import modulestore + +from . import PathItem + +log = logging.getLogger('edx.celery.task') + + +def _calculate_course_xblocks_data(course_key): + """ + Fetch data for all the blocks in the course. + + This data consists of the display_name and path of the block. + """ + with modulestore().bulk_operations(course_key): + + course = modulestore().get_course(course_key, depth=None) + blocks_info_dict = {} + + # Collect display_name and children usage keys. + blocks_stack = [course] + while blocks_stack: + current_block = blocks_stack.pop() + children = current_block.get_children() if current_block.has_children else [] + usage_id = unicode(current_block.scope_ids.usage_id) + block_info = { + 'usage_key': current_block.scope_ids.usage_id, + 'display_name': current_block.display_name, + 'children_ids': [unicode(child.scope_ids.usage_id) for child in children] + } + blocks_info_dict[usage_id] = block_info + + # Add this blocks children to the stack so that we can traverse them as well. + blocks_stack.extend(children) + + # Set children + for block in blocks_info_dict.values(): + block.setdefault('children', []) + for child_id in block['children_ids']: + block['children'].append(blocks_info_dict[child_id]) + block.pop('children_ids', None) + + # Calculate paths + def add_path_info(block_info, current_path): + """Do a DFS and add paths info to each block_info.""" + + block_info.setdefault('paths', []) + block_info['paths'].append(current_path) + + for child_block_info in block_info['children']: + add_path_info(child_block_info, current_path + [block_info]) + + add_path_info(blocks_info_dict[unicode(course.scope_ids.usage_id)], []) + + return blocks_info_dict + + +def _paths_from_data(paths_data): + """ + Construct a list of paths from path data. + """ + paths = [] + for path_data in paths_data: + paths.append([ + PathItem(item['usage_key'], item['display_name']) for item in path_data + if item['usage_key'].block_type != 'course' + ]) + + return [path for path in paths if path] + + +def paths_equal(paths_1, paths_2): + """ + Check if two paths are equivalent. + """ + if len(paths_1) != len(paths_2): + return False + + for path_1, path_2 in zip(paths_1, paths_2): + if len(path_1) != len(path_2): + return False + + for path_item_1, path_item_2 in zip(path_1, path_2): + if path_item_1.display_name != path_item_2.display_name: + return False + + usage_key_1 = path_item_1.usage_key.replace( + course_key=modulestore().fill_in_run(path_item_1.usage_key.course_key) + ) + usage_key_2 = path_item_1.usage_key.replace( + course_key=modulestore().fill_in_run(path_item_2.usage_key.course_key) + ) + if usage_key_1 != usage_key_2: + return False + + return True + + +def _update_xblocks_cache(course_key): + """ + Calculate the XBlock cache data for a course and update the XBlockCache table. + """ + from .models import XBlockCache + blocks_data = _calculate_course_xblocks_data(course_key) + + def update_block_cache_if_needed(block_cache, block_data): + """ Compare block_cache object with data and update if there are differences. """ + paths = _paths_from_data(block_data['paths']) + if block_cache.display_name != block_data['display_name'] or not paths_equal(block_cache.paths, paths): + log.info(u'Updating XBlockCache with usage_key: %s', unicode(block_cache.usage_key)) + block_cache.display_name = block_data['display_name'] + block_cache.paths = paths + block_cache.save() + + with transaction.commit_on_success(): + block_caches = XBlockCache.objects.filter(course_key=course_key) + for block_cache in block_caches: + block_data = blocks_data.pop(unicode(block_cache.usage_key), None) + if block_data: + update_block_cache_if_needed(block_cache, block_data) + + for block_data in blocks_data.values(): + with transaction.commit_on_success(): + paths = _paths_from_data(block_data['paths']) + log.info(u'Creating XBlockCache with usage_key: %s', unicode(block_data['usage_key'])) + block_cache, created = XBlockCache.objects.get_or_create(usage_key=block_data['usage_key'], defaults={ + 'course_key': course_key, + 'display_name': block_data['display_name'], + 'paths': paths, + }) + + if not created: + update_block_cache_if_needed(block_cache, block_data) + + +@task(name=u'openedx.core.djangoapps.bookmarks.tasks.update_xblock_cache') +def update_xblocks_cache(course_id): + """ + Update the XBlocks cache for a course. + + Arguments: + course_id (String): The course_id of a course. + """ + # Ideally we'd like to accept a CourseLocator; however, CourseLocator is not JSON-serializable (by default) so + # Celery's delayed tasks fail to start. For this reason, callers should pass the course key as a Unicode string. + if not isinstance(course_id, basestring): + raise ValueError('course_id must be a string. {} is not acceptable.'.format(type(course_id))) + + course_key = CourseKey.from_string(course_id) + log.info(u'Starting XBlockCaches update for course_key: %s', course_id) + _update_xblocks_cache(course_key) + log.info(u'Ending XBlockCaches update for course_key: %s', course_id) diff --git a/lms/djangoapps/bookmarks/tests/__init__.py b/openedx/core/djangoapps/bookmarks/tests/__init__.py similarity index 100% rename from lms/djangoapps/bookmarks/tests/__init__.py rename to openedx/core/djangoapps/bookmarks/tests/__init__.py diff --git a/openedx/core/djangoapps/bookmarks/tests/factories.py b/openedx/core/djangoapps/bookmarks/tests/factories.py new file mode 100644 index 0000000000..3030f75e41 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/factories.py @@ -0,0 +1,39 @@ +""" +Factories for Bookmark models. +""" + +import factory +from factory.django import DjangoModelFactory +from functools import partial + +from student.tests.factories import UserFactory +from opaque_keys.edx.locations import SlashSeparatedCourseKey +from ..models import Bookmark, XBlockCache + +COURSE_KEY = SlashSeparatedCourseKey(u'edX', u'test_course', u'test') +LOCATION = partial(COURSE_KEY.make_usage_key, u'problem') + + +class BookmarkFactory(DjangoModelFactory): + """ Simple factory class for generating Bookmark """ + FACTORY_FOR = Bookmark + + user = factory.SubFactory(UserFactory) + course_key = COURSE_KEY + usage_key = LOCATION('usage_id') + path = list() + xblock_cache = factory.SubFactory( + 'openedx.core.djangoapps.bookmarks.tests.factories.XBlockCacheFactory', + course_key=factory.SelfAttribute('..course_key'), + usage_key=factory.SelfAttribute('..usage_key'), + ) + + +class XBlockCacheFactory(DjangoModelFactory): + """ Simple factory class for generating XblockCache. """ + FACTORY_FOR = XBlockCache + + course_key = COURSE_KEY + usage_key = factory.Sequence(u'4x://edx/100/block/{0}'.format) + display_name = '' + paths = list() diff --git a/openedx/core/djangoapps/bookmarks/tests/test_api.py b/openedx/core/djangoapps/bookmarks/tests/test_api.py new file mode 100644 index 0000000000..2d249a0a5d --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/test_api.py @@ -0,0 +1,212 @@ +""" +Tests for bookmarks api. +""" +import ddt +from mock import patch +from unittest import skipUnless + +from django.conf import settings +from django.core.exceptions import ObjectDoesNotExist + +from opaque_keys.edx.keys import UsageKey + +from xmodule.modulestore.exceptions import ItemNotFoundError + +from .. import api +from ..models import Bookmark +from .test_models import BookmarksTestsBase + + +class BookmarkApiEventTestMixin(object): + """ Mixin for verifying that bookmark api events were emitted during a test. """ + + def assert_bookmark_event_emitted(self, mock_tracker, event_name, **kwargs): + """ Assert that an event has been emitted. """ + mock_tracker.assert_any_call( + event_name, + kwargs, + ) + + def assert_no_events_were_emitted(self, mock_tracker): + """ + Assert no events were emitted. + """ + self.assertFalse(mock_tracker.called) # pylint: disable=maybe-no-member + + +@ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): + """ + These tests cover the parts of the API methods. + """ + + def setUp(self): + super(BookmarksAPITests, self).setUp() + + def test_get_bookmark(self): + """ + Verifies that get_bookmark returns data as expected. + """ + bookmark_data = api.get_bookmark(user=self.user, usage_key=self.sequential_1.location) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmark_data) + + # With Optional fields. + with self.assertNumQueries(1): + bookmark_data = api.get_bookmark( + user=self.user, + usage_key=self.sequential_1.location, + fields=self.ALL_FIELDS + ) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmark_data, check_optional_fields=True) + + def test_get_bookmark_raises_error(self): + """ + Verifies that get_bookmark raises error as expected. + """ + with self.assertNumQueries(1): + with self.assertRaises(ObjectDoesNotExist): + api.get_bookmark(user=self.other_user, usage_key=self.vertical_1.location) + + @ddt.data( + 1, 10, 100 + ) + def test_get_bookmarks(self, count): + """ + Verifies that get_bookmarks returns data as expected. + """ + course, __, bookmarks = self.create_course_with_bookmarks_count(count) + + # Without course key. + with self.assertNumQueries(1): + bookmarks_data = api.get_bookmarks(user=self.user) + self.assertEqual(len(bookmarks_data), count + 3) + # Assert them in ordered manner. + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[-1]) + self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[-2]) + + # Without course key, with optional fields. + with self.assertNumQueries(1): + bookmarks_data = api.get_bookmarks(user=self.user, fields=self.ALL_FIELDS) + self.assertEqual(len(bookmarks_data), count + 3) + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[-1]) + + # With course key. + with self.assertNumQueries(1): + bookmarks_data = api.get_bookmarks(user=self.user, course_key=course.id) + self.assertEqual(len(bookmarks_data), count) + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) + self.assert_bookmark_data_is_valid(bookmarks[0], bookmarks_data[-1]) + + # With course key, with optional fields. + with self.assertNumQueries(1): + bookmarks_data = api.get_bookmarks(user=self.user, course_key=course.id, fields=self.ALL_FIELDS) + self.assertEqual(len(bookmarks_data), count) + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) + self.assert_bookmark_data_is_valid(bookmarks[0], bookmarks_data[-1]) + + # Without Serialized. + with self.assertNumQueries(1): + bookmarks = api.get_bookmarks(user=self.user, course_key=course.id, serialized=False) + self.assertEqual(len(bookmarks), count) + self.assertTrue(bookmarks.model is Bookmark) # pylint: disable=no-member + + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_create_bookmark(self, mock_tracker): + """ + Verifies that create_bookmark create & returns data as expected. + """ + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) + + with self.assertNumQueries(4): + bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.added', + course_id=unicode(self.course_id), + bookmark_id=bookmark_data['id'], + component_type=self.vertical_2.location.block_type, + component_usage_id=unicode(self.vertical_2.location), + ) + + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 3) + + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_create_bookmark_do_not_create_duplicates(self, mock_tracker): + """ + Verifies that create_bookmark do not create duplicate bookmarks. + """ + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) + + with self.assertNumQueries(4): + bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.added', + course_id=unicode(self.course_id), + bookmark_id=bookmark_data['id'], + component_type=self.vertical_2.location.block_type, + component_usage_id=unicode(self.vertical_2.location), + ) + + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 3) + + mock_tracker.reset_mock() + + with self.assertNumQueries(4): + bookmark_data_2 = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location) + + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 3) + self.assertEqual(bookmark_data, bookmark_data_2) + + self.assert_no_events_were_emitted(mock_tracker) + + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_create_bookmark_raises_error(self, mock_tracker): + """ + Verifies that create_bookmark raises error as expected. + """ + with self.assertNumQueries(0): + with self.assertRaises(ItemNotFoundError): + api.create_bookmark(user=self.user, usage_key=UsageKey.from_string('i4x://brb/100/html/340ef1771a0940')) + + self.assert_no_events_were_emitted(mock_tracker) + + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_delete_bookmark(self, mock_tracker): + """ + Verifies that delete_bookmark removes bookmark as expected. + """ + self.assertEqual(len(api.get_bookmarks(user=self.user)), 3) + + with self.assertNumQueries(3): + api.delete_bookmark(user=self.user, usage_key=self.sequential_1.location) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.removed', + course_id=unicode(self.course_id), + bookmark_id=self.bookmark_1.resource_id, + component_type=self.sequential_1.location.block_type, + component_usage_id=unicode(self.sequential_1.location), + ) + + bookmarks_data = api.get_bookmarks(user=self.user) + self.assertEqual(len(bookmarks_data), 2) + self.assertNotEqual(unicode(self.sequential_1.location), bookmarks_data[0]['usage_id']) + self.assertNotEqual(unicode(self.sequential_1.location), bookmarks_data[1]['usage_id']) + + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_delete_bookmark_raises_error(self, mock_tracker): + """ + Verifies that delete_bookmark raises error as expected. + """ + with self.assertNumQueries(1): + with self.assertRaises(ObjectDoesNotExist): + api.delete_bookmark(user=self.other_user, usage_key=self.vertical_1.location) + + self.assert_no_events_were_emitted(mock_tracker) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py new file mode 100644 index 0000000000..95c2ebe153 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -0,0 +1,490 @@ +""" +Tests for Bookmarks models. +""" +from contextlib import contextmanager +import datetime +import ddt +from freezegun import freeze_time +import mock +import pytz +from unittest import skipUnless + +from django.conf import settings + +from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import CourseLocator, BlockUsageLocator +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, ItemFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +from student.tests.factories import AdminFactory, UserFactory + +from .. import DEFAULT_FIELDS, OPTIONAL_FIELDS, PathItem +from ..models import Bookmark, XBlockCache, parse_path_data +from .factories import BookmarkFactory + +EXAMPLE_USAGE_KEY_1 = u'i4x://org.15/course_15/chapter/Week_1' +EXAMPLE_USAGE_KEY_2 = u'i4x://org.15/course_15/chapter/Week_2' + + +noop_contextmanager = contextmanager(lambda x: (yield)) # pylint: disable=invalid-name + + +class BookmarksTestsBase(ModuleStoreTestCase): + """ + Test the Bookmark model. + """ + ALL_FIELDS = DEFAULT_FIELDS + OPTIONAL_FIELDS + STORE_TYPE = ModuleStoreEnum.Type.mongo + TEST_PASSWORD = 'test' + + def setUp(self): + super(BookmarksTestsBase, self).setUp() + + self.admin = AdminFactory() + self.user = UserFactory.create(password=self.TEST_PASSWORD) + self.other_user = UserFactory.create(password=self.TEST_PASSWORD) + self.setup_test_data(self.STORE_TYPE) + + def setup_test_data(self, store_type=ModuleStoreEnum.Type.mongo): + """ Create courses and add some test blocks. """ + + with self.store.default_store(store_type): + + self.course = CourseFactory.create(display_name='An Introduction to API Testing') + self.course_id = unicode(self.course.id) + + if store_type == ModuleStoreEnum.Type.mongo: + bulk_operations_manager = self.store.bulk_operations + else: + bulk_operations_manager = noop_contextmanager + + with bulk_operations_manager(self.course.id): + + self.chapter_1 = ItemFactory.create( + parent_location=self.course.location, category='chapter', display_name='Week 1' + ) + self.chapter_2 = ItemFactory.create( + parent_location=self.course.location, category='chapter', display_name='Week 2' + ) + + self.sequential_1 = ItemFactory.create( + parent_location=self.chapter_1.location, category='sequential', display_name='Lesson 1' + ) + self.sequential_2 = ItemFactory.create( + parent_location=self.chapter_1.location, category='sequential', display_name='Lesson 2' + ) + + self.vertical_1 = ItemFactory.create( + parent_location=self.sequential_1.location, category='vertical', display_name='Subsection 1' + ) + self.vertical_2 = ItemFactory.create( + parent_location=self.sequential_2.location, category='vertical', display_name='Subsection 2' + ) + self.vertical_3 = ItemFactory.create( + parent_location=self.sequential_2.location, category='vertical', display_name='Subsection 3' + ) + + self.html_1 = ItemFactory.create( + parent_location=self.vertical_2.location, category='html', display_name='Details 1' + ) + + self.path = [ + PathItem(self.chapter_1.location, self.chapter_1.display_name), + PathItem(self.sequential_2.location, self.sequential_2.display_name), + ] + + self.bookmark_1 = BookmarkFactory.create( + user=self.user, + course_key=self.course_id, + usage_key=self.sequential_1.location, + xblock_cache=XBlockCache.create({ + 'display_name': self.sequential_1.display_name, + 'usage_key': self.sequential_1.location, + }), + ) + self.bookmark_2 = BookmarkFactory.create( + user=self.user, + course_key=self.course_id, + usage_key=self.sequential_2.location, + xblock_cache=XBlockCache.create({ + 'display_name': self.sequential_2.display_name, + 'usage_key': self.sequential_2.location, + }), + ) + + self.other_course = CourseFactory.create(display_name='An Introduction to API Testing 2') + + with bulk_operations_manager(self.other_course.id): + + self.other_chapter_1 = ItemFactory.create( + parent_location=self.other_course.location, category='chapter', display_name='Other Week 1' + ) + self.other_sequential_1 = ItemFactory.create( + parent_location=self.other_chapter_1.location, category='sequential', display_name='Other Lesson 1' + ) + self.other_sequential_2 = ItemFactory.create( + parent_location=self.other_chapter_1.location, category='sequential', display_name='Other Lesson 2' + ) + self.other_vertical_1 = ItemFactory.create( + parent_location=self.other_sequential_1.location, category='vertical', display_name='Other Subsection 1' + ) + self.other_vertical_2 = ItemFactory.create( + parent_location=self.other_sequential_1.location, category='vertical', display_name='Other Subsection 2' + ) + + # self.other_vertical_1 has two parents + self.other_sequential_2.children.append(self.other_vertical_1.location) + modulestore().update_item(self.other_sequential_2, self.admin.id) # pylint: disable=no-member + + self.other_bookmark_1 = BookmarkFactory.create( + user=self.user, + course_key=unicode(self.other_course.id), + usage_key=self.other_vertical_1.location, + xblock_cache=XBlockCache.create({ + 'display_name': self.other_vertical_1.display_name, + 'usage_key': self.other_vertical_1.location, + }), + ) + + def create_course_with_blocks(self, children_per_block=1, depth=1, store_type=ModuleStoreEnum.Type.mongo): + """ + Create a course and add blocks. + """ + with self.store.default_store(store_type): + + course = CourseFactory.create() + display_name = 0 + + if store_type == ModuleStoreEnum.Type.mongo: + bulk_operations_manager = self.store.bulk_operations + else: + bulk_operations_manager = noop_contextmanager + + with bulk_operations_manager(course.id): + blocks_at_next_level = [course] + + for __ in range(depth): + blocks_at_current_level = blocks_at_next_level + blocks_at_next_level = [] + + for block in blocks_at_current_level: + for __ in range(children_per_block): + blocks_at_next_level += [ItemFactory.create( + parent_location=block.scope_ids.usage_id, display_name=unicode(display_name) + )] + display_name += 1 + + return course + + def create_course_with_bookmarks_count(self, count, store_type=ModuleStoreEnum.Type.mongo): + """ + Create a course, add some content and add bookmarks. + """ + with self.store.default_store(store_type): + + course = CourseFactory.create() + + if store_type == ModuleStoreEnum.Type.mongo: + bulk_operations_manager = self.store.bulk_operations + else: + bulk_operations_manager = noop_contextmanager + + with bulk_operations_manager(course.id): + blocks = [ItemFactory.create( + parent_location=course.location, category='chapter', display_name=unicode(index) + ) for index in range(count)] + + bookmarks = [BookmarkFactory.create( + user=self.user, + course_key=course.id, + usage_key=block.location, + xblock_cache=XBlockCache.create({ + 'display_name': block.display_name, + 'usage_key': block.location, + }), + ) for block in blocks] + + return course, blocks, bookmarks + + def assert_bookmark_model_is_valid(self, bookmark, bookmark_data): + """ + Assert that the attributes of the bookmark model were set correctly. + """ + self.assertEqual(bookmark.user, bookmark_data['user']) + self.assertEqual(bookmark.course_key, bookmark_data['course_key']) + self.assertEqual(unicode(bookmark.usage_key), unicode(bookmark_data['usage_key'])) + self.assertEqual(bookmark.resource_id, u"{},{}".format(bookmark_data['user'], bookmark_data['usage_key'])) + self.assertEqual(bookmark.display_name, bookmark_data['display_name']) + self.assertEqual(bookmark.path, self.path) + self.assertIsNotNone(bookmark.created) + + self.assertEqual(bookmark.xblock_cache.course_key, bookmark_data['course_key']) + self.assertEqual(bookmark.xblock_cache.display_name, bookmark_data['display_name']) + + def assert_bookmark_data_is_valid(self, bookmark, bookmark_data, check_optional_fields=False): + """ + Assert that the bookmark data matches the data in the model. + """ + self.assertEqual(bookmark_data['id'], bookmark.resource_id) + self.assertEqual(bookmark_data['course_id'], unicode(bookmark.course_key)) + self.assertEqual(bookmark_data['usage_id'], unicode(bookmark.usage_key)) + self.assertEqual(bookmark_data['block_type'], unicode(bookmark.usage_key.block_type)) + self.assertIsNotNone(bookmark_data['created']) + + if check_optional_fields: + self.assertEqual(bookmark_data['display_name'], bookmark.display_name) + self.assertEqual(bookmark_data['path'], bookmark.path) + + +@ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +class BookmarkModelTests(BookmarksTestsBase): + """ + Test the Bookmark model. + """ + def get_bookmark_data(self, block, user=None): + """ + Returns bookmark data for testing. + """ + return { + 'user': user or self.user, + 'usage_key': block.location, + 'course_key': block.location.course_key, + 'display_name': block.display_name, + } + + @ddt.data( + (ModuleStoreEnum.Type.mongo, 'course', [], 3), + (ModuleStoreEnum.Type.mongo, 'chapter_1', [], 3), + (ModuleStoreEnum.Type.mongo, 'sequential_1', ['chapter_1'], 4), + (ModuleStoreEnum.Type.mongo, 'vertical_1', ['chapter_1', 'sequential_1'], 5), + (ModuleStoreEnum.Type.mongo, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 6), + (ModuleStoreEnum.Type.split, 'course', [], 3), + (ModuleStoreEnum.Type.split, 'chapter_1', [], 2), + (ModuleStoreEnum.Type.split, 'sequential_1', ['chapter_1'], 2), + (ModuleStoreEnum.Type.split, 'vertical_1', ['chapter_1', 'sequential_1'], 2), + (ModuleStoreEnum.Type.split, 'html_1', ['chapter_1', 'sequential_2', 'vertical_2'], 2), + ) + @ddt.unpack + def test_path_and_queries_on_create(self, store_type, block_to_bookmark, ancestors_attrs, expected_mongo_calls): + """ + In case of mongo, 1 query is used to fetch the block, and 2 by path_to_location(), and then + 1 query per parent in path is needed to fetch the parent blocks. + """ + + self.setup_test_data(store_type) + user = UserFactory.create() + + expected_path = [PathItem( + usage_key=getattr(self, ancestor_attr).location, display_name=getattr(self, ancestor_attr).display_name + ) for ancestor_attr in ancestors_attrs] + + bookmark_data = self.get_bookmark_data(getattr(self, block_to_bookmark), user=user) + + with check_mongo_calls(expected_mongo_calls): + bookmark, __ = Bookmark.create(bookmark_data) + + self.assertEqual(bookmark.path, expected_path) + self.assertIsNotNone(bookmark.xblock_cache) + self.assertEqual(bookmark.xblock_cache.paths, []) + + def test_create_bookmark_success(self): + """ + Tests creation of bookmark. + """ + bookmark_data = self.get_bookmark_data(self.vertical_2) + bookmark, __ = Bookmark.create(bookmark_data) + self.assert_bookmark_model_is_valid(bookmark, bookmark_data) + + bookmark_data_different_values = self.get_bookmark_data(self.vertical_2) + bookmark_data_different_values['display_name'] = 'Introduction Video' + bookmark2, __ = Bookmark.create(bookmark_data_different_values) + # The bookmark object already created should have been returned without modifications. + self.assertEqual(bookmark, bookmark2) + self.assertEqual(bookmark.xblock_cache, bookmark2.xblock_cache) + self.assert_bookmark_model_is_valid(bookmark2, bookmark_data) + + bookmark_data_different_user = self.get_bookmark_data(self.vertical_2) + bookmark_data_different_user['user'] = UserFactory.create() + bookmark3, __ = Bookmark.create(bookmark_data_different_user) + self.assertNotEqual(bookmark, bookmark3) + self.assert_bookmark_model_is_valid(bookmark3, bookmark_data_different_user) + + @ddt.data( + (-30, [[PathItem(EXAMPLE_USAGE_KEY_1, '1')]], 1), + (30, None, 2), + (30, [], 2), + (30, [[PathItem(EXAMPLE_USAGE_KEY_1, '1')]], 1), + (30, [[PathItem(EXAMPLE_USAGE_KEY_1, '1')], [PathItem(EXAMPLE_USAGE_KEY_2, '2')]], 2), + ) + @ddt.unpack + @mock.patch('openedx.core.djangoapps.bookmarks.models.Bookmark.get_path') + def test_path(self, seconds_delta, paths, get_path_call_count, mock_get_path): + + block_path = [PathItem(UsageKey.from_string(EXAMPLE_USAGE_KEY_1), '1')] + mock_get_path.return_value = block_path + + html = ItemFactory.create( + parent_location=self.other_chapter_1.location, category='html', display_name='Other Lesson 1' + ) + + bookmark_data = self.get_bookmark_data(html) + bookmark, __ = Bookmark.create(bookmark_data) + self.assertIsNotNone(bookmark.xblock_cache) + + modification_datetime = datetime.datetime.now(pytz.utc) + datetime.timedelta(seconds=seconds_delta) + with freeze_time(modification_datetime): + bookmark.xblock_cache.paths = paths + bookmark.xblock_cache.save() + + self.assertEqual(bookmark.path, block_path) + self.assertEqual(mock_get_path.call_count, get_path_call_count) + + @ddt.data( + (ModuleStoreEnum.Type.mongo, 2, 2, 2), + (ModuleStoreEnum.Type.mongo, 4, 2, 2), + (ModuleStoreEnum.Type.mongo, 6, 2, 2), + (ModuleStoreEnum.Type.mongo, 2, 3, 3), + (ModuleStoreEnum.Type.mongo, 4, 3, 3), + (ModuleStoreEnum.Type.mongo, 6, 3, 3), + (ModuleStoreEnum.Type.mongo, 2, 4, 4), + (ModuleStoreEnum.Type.mongo, 4, 4, 4), + (ModuleStoreEnum.Type.split, 2, 2, 2), + (ModuleStoreEnum.Type.split, 4, 2, 2), + (ModuleStoreEnum.Type.split, 2, 3, 2), + (ModuleStoreEnum.Type.split, 4, 3, 2), + (ModuleStoreEnum.Type.split, 2, 4, 2), + ) + @ddt.unpack + def test_get_path_queries(self, store_type, children_per_block, depth, expected_mongo_calls): + """ + In case of mongo, 2 queries are used by path_to_location(), and then + 1 query per parent in path is needed to fetch the parent blocks. + """ + + course = self.create_course_with_blocks(children_per_block, depth, store_type) + + # Find a leaf block. + block = modulestore().get_course(course.id, depth=None) + for __ in range(depth - 1): + children = block.get_children() + block = children[-1] + + with check_mongo_calls(expected_mongo_calls): + path = Bookmark.get_path(block.location) + self.assertEqual(len(path), depth - 2) + + def test_get_path_in_case_of_exceptions(self): + + user = UserFactory.create() + + # Block does not exist + usage_key = UsageKey.from_string('i4x://edX/apis/html/interactive') + usage_key.replace(course_key=self.course.id) + self.assertEqual(Bookmark.get_path(usage_key), []) + + # Block is an orphan + self.other_sequential_1.children = [] + modulestore().update_item(self.other_sequential_1, self.admin.id) # pylint: disable=no-member + + bookmark_data = self.get_bookmark_data(self.other_vertical_2, user=user) + bookmark, __ = Bookmark.create(bookmark_data) + + self.assertEqual(bookmark.path, []) + self.assertIsNotNone(bookmark.xblock_cache) + self.assertEqual(bookmark.xblock_cache.paths, []) + + # Parent block could not be retrieved + with mock.patch('openedx.core.djangoapps.bookmarks.models.search.path_to_location') as mock_path_to_location: + mock_path_to_location.return_value = [usage_key] + bookmark_data = self.get_bookmark_data(self.other_sequential_1, user=user) + bookmark, __ = Bookmark.create(bookmark_data) + self.assertEqual(bookmark.path, []) + + +@ddt.ddt +class XBlockCacheModelTest(ModuleStoreTestCase): + """ + Test the XBlockCache model. + """ + + COURSE_KEY = CourseLocator(org='test', course='test', run='test') + CHAPTER1_USAGE_KEY = BlockUsageLocator(COURSE_KEY, block_type='chapter', block_id='chapter1') + SECTION1_USAGE_KEY = BlockUsageLocator(COURSE_KEY, block_type='section', block_id='section1') + SECTION2_USAGE_KEY = BlockUsageLocator(COURSE_KEY, block_type='section', block_id='section1') + VERTICAL1_USAGE_KEY = BlockUsageLocator(COURSE_KEY, block_type='vertical', block_id='sequential1') + PATH1 = [ + [unicode(CHAPTER1_USAGE_KEY), 'Chapter 1'], + [unicode(SECTION1_USAGE_KEY), 'Section 1'], + ] + PATH2 = [ + [unicode(CHAPTER1_USAGE_KEY), 'Chapter 1'], + [unicode(SECTION2_USAGE_KEY), 'Section 2'], + ] + + def setUp(self): + super(XBlockCacheModelTest, self).setUp() + + def assert_xblock_cache_data(self, xblock_cache, data): + """ + Assert that the XBlockCache object values match. + """ + self.assertEqual(xblock_cache.usage_key, data['usage_key']) + self.assertEqual(xblock_cache.course_key, data['usage_key'].course_key) + self.assertEqual(xblock_cache.display_name, data['display_name']) + self.assertEqual(xblock_cache._paths, data['_paths']) # pylint: disable=protected-access + self.assertEqual(xblock_cache.paths, [parse_path_data(path) for path in data['_paths']]) + + @ddt.data( + ( + [ + {'usage_key': VERTICAL1_USAGE_KEY, }, + {'display_name': '', '_paths': [], }, + ], + [ + {'usage_key': VERTICAL1_USAGE_KEY, 'display_name': 'Vertical 5', '_paths': [PATH2]}, + {'_paths': []}, + ], + ), + ( + [ + {'usage_key': VERTICAL1_USAGE_KEY, 'display_name': 'Vertical 4', '_paths': [PATH1]}, + {}, + ], + [ + {'usage_key': VERTICAL1_USAGE_KEY, 'display_name': 'Vertical 5', '_paths': [PATH2]}, + {'_paths': [PATH1]}, + ], + ), + ) + def test_create(self, data): + """ + Test XBlockCache.create() constructs and updates objects correctly. + """ + for create_data, additional_data_to_expect in data: + xblock_cache = XBlockCache.create(create_data) + create_data.update(additional_data_to_expect) + self.assert_xblock_cache_data(xblock_cache, create_data) + + @ddt.data( + ([], [PATH1]), + ([PATH1, PATH2], [PATH1]), + ([PATH1], []), + ) + @ddt.unpack + def test_paths(self, original_paths, updated_paths): + xblock_cache = XBlockCache.create({ + 'usage_key': self.VERTICAL1_USAGE_KEY, + 'display_name': 'The end.', + '_paths': original_paths, + }) + self.assertEqual(xblock_cache.paths, [parse_path_data(path) for path in original_paths]) + + xblock_cache.paths = [parse_path_data(path) for path in updated_paths] + xblock_cache.save() + + xblock_cache = XBlockCache.objects.get(id=xblock_cache.id) + self.assertEqual(xblock_cache._paths, updated_paths) # pylint: disable=protected-access + self.assertEqual(xblock_cache.paths, [parse_path_data(path) for path in updated_paths]) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_services.py b/openedx/core/djangoapps/bookmarks/tests/test_services.py new file mode 100644 index 0000000000..a12ca9d18e --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/test_services.py @@ -0,0 +1,84 @@ +""" +Tests for bookmark services. +""" +from unittest import skipUnless + +from django.conf import settings + +from opaque_keys.edx.keys import UsageKey + +from ..services import BookmarksService +from .test_models import BookmarksTestsBase + + +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +class BookmarksServiceTests(BookmarksTestsBase): + """ + Tests the Bookmarks service. + """ + + def setUp(self): + super(BookmarksServiceTests, self).setUp() + + self.bookmark_service = BookmarksService(user=self.user) + + def test_get_bookmarks(self): + """ + Verifies get_bookmarks returns data as expected. + """ + with self.assertNumQueries(1): + bookmarks_data = self.bookmark_service.bookmarks(course_key=self.course.id) + + self.assertEqual(len(bookmarks_data), 2) + self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[0]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[1]) + + def test_is_bookmarked(self): + """ + Verifies is_bookmarked returns Bool as expected. + """ + with self.assertNumQueries(1): + self.assertTrue(self.bookmark_service.is_bookmarked(usage_key=self.sequential_1.location)) + self.assertFalse(self.bookmark_service.is_bookmarked(usage_key=self.vertical_2.location)) + self.assertTrue(self.bookmark_service.is_bookmarked(usage_key=self.sequential_2.location)) + + self.bookmark_service.set_bookmarked(usage_key=self.chapter_1.location) + with self.assertNumQueries(0): + self.assertTrue(self.bookmark_service.is_bookmarked(usage_key=self.chapter_1.location)) + self.assertFalse(self.bookmark_service.is_bookmarked(usage_key=self.vertical_2.location)) + + # Removing a bookmark should result in the cache being updated on the next request + self.bookmark_service.unset_bookmarked(usage_key=self.chapter_1.location) + with self.assertNumQueries(0): + self.assertFalse(self.bookmark_service.is_bookmarked(usage_key=self.chapter_1.location)) + self.assertFalse(self.bookmark_service.is_bookmarked(usage_key=self.vertical_2.location)) + + # Get bookmark that does not exist. + bookmark_service = BookmarksService(self.other_user) + with self.assertNumQueries(1): + self.assertFalse(bookmark_service.is_bookmarked(usage_key=self.sequential_1.location)) + + def test_set_bookmarked(self): + """ + Verifies set_bookmarked returns Bool as expected. + """ + # Assert False for item that does not exist. + with self.assertNumQueries(0): + self.assertFalse( + self.bookmark_service.set_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive")) + ) + + with self.assertNumQueries(4): + self.assertTrue(self.bookmark_service.set_bookmarked(usage_key=self.vertical_2.location)) + + def test_unset_bookmarked(self): + """ + Verifies unset_bookmarked returns Bool as expected. + """ + with self.assertNumQueries(1): + self.assertFalse( + self.bookmark_service.unset_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive")) + ) + + with self.assertNumQueries(3): + self.assertTrue(self.bookmark_service.unset_bookmarked(usage_key=self.sequential_1.location)) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py new file mode 100644 index 0000000000..1b9afb0efe --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py @@ -0,0 +1,166 @@ +""" +Tests for tasks. +""" +import ddt + +from xmodule.modulestore import ModuleStoreEnum +from xmodule.modulestore.tests.factories import check_mongo_calls + +from ..models import XBlockCache +from ..tasks import _calculate_course_xblocks_data, _update_xblocks_cache +from .test_models import BookmarksTestsBase + + +@ddt.ddt +class XBlockCacheTaskTests(BookmarksTestsBase): + """ + Test the XBlockCache model. + """ + def setUp(self): + super(XBlockCacheTaskTests, self).setUp() + + self.course_expected_cache_data = { + self.course.location: [ + [], + ], self.chapter_1.location: [ + [ + self.course.location, + ], + ], self.chapter_2.location: [ + [ + self.course.location, + ], + ], self.sequential_1.location: [ + [ + self.course.location, + self.chapter_1.location, + ], + ], self.sequential_2.location: [ + [ + self.course.location, + self.chapter_1.location, + ], + ], self.vertical_1.location: [ + [ + self.course.location, + self.chapter_1.location, + self.sequential_1.location, + ], + ], self.vertical_2.location: [ + [ + self.course.location, + self.chapter_1.location, + self.sequential_2.location, + ], + ], self.vertical_3.location: [ + [ + self.course.location, + self.chapter_1.location, + self.sequential_2.location, + ], + ], + } + + self.other_course_expected_cache_data = { # pylint: disable=invalid-name + self.other_course.location: [ + [], + ], self.other_chapter_1.location: [ + [ + self.other_course.location, + ], + ], self.other_sequential_1.location: [ + [ + self.other_course.location, + self.other_chapter_1.location, + ], + ], self.other_sequential_2.location: [ + [ + self.other_course.location, + self.other_chapter_1.location, + ], + ], self.other_vertical_1.location: [ + [ + self.other_course.location, + self.other_chapter_1.location, + self.other_sequential_1.location, + ], + [ + self.other_course.location, + self.other_chapter_1.location, + self.other_sequential_2.location, + ] + ], self.other_vertical_2.location: [ + [ + self.other_course.location, + self.other_chapter_1.location, + self.other_sequential_1.location, + ], + ], + } + + @ddt.data( + (ModuleStoreEnum.Type.mongo, 2, 2, 3), + (ModuleStoreEnum.Type.mongo, 4, 2, 3), + (ModuleStoreEnum.Type.mongo, 2, 3, 4), + (ModuleStoreEnum.Type.mongo, 4, 3, 4), + (ModuleStoreEnum.Type.mongo, 2, 4, 5), + (ModuleStoreEnum.Type.mongo, 4, 4, 6), + (ModuleStoreEnum.Type.split, 2, 2, 3), + (ModuleStoreEnum.Type.split, 4, 2, 3), + (ModuleStoreEnum.Type.split, 2, 3, 3), + (ModuleStoreEnum.Type.split, 2, 4, 3), + ) + @ddt.unpack + def test_calculate_course_xblocks_data_queries(self, store_type, children_per_block, depth, expected_mongo_calls): + + course = self.create_course_with_blocks(children_per_block, depth, store_type) + + with check_mongo_calls(expected_mongo_calls): + blocks_data = _calculate_course_xblocks_data(course.id) + self.assertGreater(len(blocks_data), children_per_block ** depth) + + @ddt.data( + ('course',), + ('other_course',) + ) + @ddt.unpack + def test_calculate_course_xblocks_data(self, course_attr): + """ + Test that the xblocks data is calculated correctly. + """ + course = getattr(self, course_attr) + blocks_data = _calculate_course_xblocks_data(course.id) + + expected_cache_data = getattr(self, course_attr + '_expected_cache_data') + for usage_key, __ in expected_cache_data.items(): + for path_index, path in enumerate(blocks_data[unicode(usage_key)]['paths']): + for path_item_index, path_item in enumerate(path): + self.assertEqual( + path_item['usage_key'], expected_cache_data[usage_key][path_index][path_item_index] + ) + + @ddt.data( + ('course', 19), + ('other_course', 13) + ) + @ddt.unpack + def test_update_xblocks_cache(self, course_attr, expected_sql_queries): + """ + Test that the xblocks data is persisted correctly. + """ + course = getattr(self, course_attr) + + with self.assertNumQueries(expected_sql_queries): + _update_xblocks_cache(course.id) + + expected_cache_data = getattr(self, course_attr + '_expected_cache_data') + for usage_key, __ in expected_cache_data.items(): + xblock_cache = XBlockCache.objects.get(usage_key=usage_key) + for path_index, path in enumerate(xblock_cache.paths): + for path_item_index, path_item in enumerate(path): + self.assertEqual( + path_item.usage_key, expected_cache_data[usage_key][path_index][path_item_index + 1] + ) + + with self.assertNumQueries(1): + _update_xblocks_cache(course.id) diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py new file mode 100644 index 0000000000..eed9280704 --- /dev/null +++ b/openedx/core/djangoapps/bookmarks/tests/test_views.py @@ -0,0 +1,540 @@ +""" +Tests for bookmark views. +""" + +import ddt +import json +from unittest import skipUnless +import urllib + +from django.conf import settings +from django.core.urlresolvers import reverse +from mock import patch +from rest_framework.test import APIClient + +from xmodule.modulestore import ModuleStoreEnum + +from .test_models import BookmarksTestsBase +from .test_api import BookmarkApiEventTestMixin + + +# pylint: disable=no-member +class BookmarksViewsTestsBase(BookmarksTestsBase, BookmarkApiEventTestMixin): + """ + Base class for bookmarks views tests. + """ + def setUp(self): + super(BookmarksViewsTestsBase, self).setUp() + + self.anonymous_client = APIClient() + self.client = self.login_client(user=self.user) + + def login_client(self, user): + """ + Helper method for getting the client and user and logging in. Returns client. + """ + client = APIClient() + client.login(username=user.username, password=self.TEST_PASSWORD) + return client + + def send_get(self, client, url, query_parameters=None, expected_status=200): + """ + Helper method for sending a GET to the server. Verifies the expected status and returns the response. + """ + url = url + '?' + query_parameters if query_parameters else url + response = client.get(url) + self.assertEqual(expected_status, response.status_code) + return response + + def send_post(self, client, url, data, content_type='application/json', expected_status=201): + """ + Helper method for sending a POST to the server. Verifies the expected status and returns the response. + """ + response = client.post(url, data=json.dumps(data), content_type=content_type) + self.assertEqual(expected_status, response.status_code) + return response + + def send_delete(self, client, url, expected_status=204): + """ + Helper method for sending a DELETE to the server. Verifies the expected status and returns the response. + """ + response = client.delete(url) + self.assertEqual(expected_status, response.status_code) + return response + + +@ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +class BookmarksListViewTests(BookmarksViewsTestsBase): + """ + This contains the tests for GET & POST methods of bookmark.views.BookmarksListView class + GET /api/bookmarks/v1/bookmarks/?course_id={course_id1} + POST /api/bookmarks/v1/bookmarks + """ + @ddt.data( + (1, False), + (10, False), + (25, False), + (1, True), + (10, True), + (25, True), + ) + @ddt.unpack + @patch('eventtracking.tracker.emit') + def test_get_bookmarks_successfully(self, bookmarks_count, check_all_fields, mock_tracker): + """ + Test that requesting bookmarks for a course returns records successfully in + expected order without optional fields. + """ + + course, __, bookmarks = self.create_course_with_bookmarks_count( + bookmarks_count, store_type=ModuleStoreEnum.Type.mongo + ) + + query_parameters = 'course_id={}&page_size={}'.format(urllib.quote(unicode(course.id)), 100) + if check_all_fields: + query_parameters += '&fields=path,display_name' + + with self.assertNumQueries(7): # 2 queries for bookmark table. + response = self.send_get( + client=self.client, + url=reverse('bookmarks'), + query_parameters=query_parameters, + ) + + bookmarks_data = response.data['results'] + + self.assertEqual(len(bookmarks_data), len(bookmarks)) + self.assertEqual(response.data['count'], len(bookmarks)) + self.assertEqual(response.data['num_pages'], 1) + + # As bookmarks are sorted by -created so we will compare in that order. + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0], check_optional_fields=check_all_fields) + self.assert_bookmark_data_is_valid(bookmarks[0], bookmarks_data[-1], check_optional_fields=check_all_fields) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.listed', + course_id=unicode(course.id), + list_type='per_course', + bookmarks_count=bookmarks_count, + page_size=100, + page_number=1 + ) + + @ddt.data( + 10, 25 + ) + @patch('eventtracking.tracker.emit') + def test_get_bookmarks_with_pagination(self, bookmarks_count, mock_tracker): + """ + Test that requesting bookmarks for a course return results with pagination 200 code. + """ + + course, __, bookmarks = self.create_course_with_bookmarks_count( + bookmarks_count, store_type=ModuleStoreEnum.Type.mongo + ) + + page_size = 5 + query_parameters = 'course_id={}&page_size={}'.format(urllib.quote(unicode(course.id)), page_size) + + with self.assertNumQueries(7): # 2 queries for bookmark table. + response = self.send_get( + client=self.client, + url=reverse('bookmarks'), + query_parameters=query_parameters + ) + + bookmarks_data = response.data['results'] + + # Pagination assertions. + self.assertEqual(response.data['count'], bookmarks_count) + self.assertIn('page=2&page_size={}'.format(page_size), response.data['next']) + self.assertEqual(response.data['num_pages'], bookmarks_count / page_size) + + self.assertEqual(len(bookmarks_data), min(bookmarks_count, page_size)) + self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.listed', + course_id=unicode(course.id), + list_type='per_course', + bookmarks_count=bookmarks_count, + page_size=page_size, + page_number=1 + ) + + @patch('eventtracking.tracker.emit') + def test_get_bookmarks_with_invalid_data(self, mock_tracker): + """ + Test that requesting bookmarks with invalid data returns 0 records. + """ + # Invalid course id. + with self.assertNumQueries(5): # No queries for bookmark table. + response = self.send_get( + client=self.client, + url=reverse('bookmarks'), + query_parameters='course_id=invalid' + ) + bookmarks_data = response.data['results'] + self.assertEqual(len(bookmarks_data), 0) + + self.assertFalse(mock_tracker.emit.called) # pylint: disable=maybe-no-member + + @patch('eventtracking.tracker.emit') + def test_get_all_bookmarks_when_course_id_not_given(self, mock_tracker): + """ + Test that requesting bookmarks returns all records for that user. + """ + # Without course id we would return all the bookmarks for that user. + + with self.assertNumQueries(7): # 2 queries for bookmark table. + response = self.send_get( + client=self.client, + url=reverse('bookmarks') + ) + bookmarks_data = response.data['results'] + self.assertEqual(len(bookmarks_data), 3) + self.assert_bookmark_data_is_valid(self.other_bookmark_1, bookmarks_data[0]) + self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[1]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[2]) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.listed', + list_type='all_courses', + bookmarks_count=3, + page_size=10, + page_number=1 + ) + + def test_anonymous_access(self): + """ + Test that an anonymous client (not logged in) cannot call GET or POST. + """ + query_parameters = 'course_id={}'.format(self.course_id) + with self.assertNumQueries(1): # No queries for bookmark table. + self.send_get( + client=self.anonymous_client, + url=reverse('bookmarks'), + query_parameters=query_parameters, + expected_status=401 + ) + + with self.assertNumQueries(1): # No queries for bookmark table. + self.send_post( + client=self.anonymous_client, + url=reverse('bookmarks'), + data={'usage_id': 'test'}, + expected_status=401 + ) + + def test_post_bookmark_successfully(self): + """ + Test that posting a bookmark successfully returns newly created data with 201 code. + """ + with self.assertNumQueries(9): + response = self.send_post( + client=self.client, + url=reverse('bookmarks'), + data={'usage_id': unicode(self.vertical_3.location)} + ) + + # Assert Newly created bookmark. + self.assertEqual(response.data['id'], '%s,%s' % (self.user.username, unicode(self.vertical_3.location))) + self.assertEqual(response.data['course_id'], self.course_id) + self.assertEqual(response.data['usage_id'], unicode(self.vertical_3.location)) + self.assertIsNotNone(response.data['created']) + self.assertEqual(len(response.data['path']), 2) + self.assertEqual(response.data['display_name'], self.vertical_3.display_name) + + def test_post_bookmark_with_invalid_data(self): + """ + Test that posting a bookmark for a block with invalid usage id returns a 400. + Scenarios: + 1) Invalid usage id. + 2) Without usage id. + 3) With empty request.DATA + """ + # Send usage_id with invalid format. + with self.assertNumQueries(5): # No queries for bookmark table. + response = self.send_post( + client=self.client, + url=reverse('bookmarks'), + data={'usage_id': 'invalid'}, + expected_status=400 + ) + self.assertEqual(response.data['user_message'], u'Invalid usage_id: invalid.') + + # Send data without usage_id. + with self.assertNumQueries(4): # No queries for bookmark table. + response = self.send_post( + client=self.client, + url=reverse('bookmarks'), + data={'course_id': 'invalid'}, + expected_status=400 + ) + self.assertEqual(response.data['user_message'], u'Parameter usage_id not provided.') + self.assertEqual(response.data['developer_message'], u'Parameter usage_id not provided.') + + # Send empty data dictionary. + with self.assertNumQueries(4): # No queries for bookmark table. + response = self.send_post( + client=self.client, + url=reverse('bookmarks'), + data={}, + expected_status=400 + ) + self.assertEqual(response.data['user_message'], u'No data provided.') + self.assertEqual(response.data['developer_message'], u'No data provided.') + + def test_post_bookmark_for_non_existing_block(self): + """ + Test that posting a bookmark for a block that does not exist returns a 400. + """ + with self.assertNumQueries(5): # No queries for bookmark table. + response = self.send_post( + client=self.client, + url=reverse('bookmarks'), + data={'usage_id': 'i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21'}, + expected_status=400 + ) + self.assertEqual( + response.data['user_message'], + u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.' + ) + self.assertEqual( + response.data['developer_message'], + u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.' + ) + + def test_unsupported_methods(self): + """ + Test that DELETE and PUT are not supported. + """ + self.client.login(username=self.user.username, password=self.TEST_PASSWORD) + self.assertEqual(405, self.client.put(reverse('bookmarks')).status_code) + self.assertEqual(405, self.client.delete(reverse('bookmarks')).status_code) + + @patch('eventtracking.tracker.emit') + @ddt.unpack + @ddt.data( + {'page_size': -1, 'expected_bookmarks_count': 2, 'expected_page_size': 10, 'expected_page_number': 1}, + {'page_size': 0, 'expected_bookmarks_count': 2, 'expected_page_size': 10, 'expected_page_number': 1}, + {'page_size': 999, 'expected_bookmarks_count': 2, 'expected_page_size': 500, 'expected_page_number': 1} + ) + def test_listed_event_for_different_page_size_values(self, mock_tracker, page_size, expected_bookmarks_count, + expected_page_size, expected_page_number): + """ Test that edx.course.bookmark.listed event values are as expected for different page size values """ + query_parameters = 'course_id={}&page_size={}'.format(urllib.quote(self.course_id), page_size) + + self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.listed', + course_id=self.course_id, + list_type='per_course', + bookmarks_count=expected_bookmarks_count, + page_size=expected_page_size, + page_number=expected_page_number + ) + + @patch('openedx.core.djangoapps.bookmarks.views.eventtracking.tracker.emit') + def test_listed_event_for_page_number(self, mock_tracker): + """ Test that edx.course.bookmark.listed event values are as expected when we request a specific page number """ + self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters='page_size=2&page=2') + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.listed', + list_type='all_courses', + bookmarks_count=3, + page_size=2, + page_number=2 + ) + + +@ddt.ddt +@skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Tests only valid in LMS') +class BookmarksDetailViewTests(BookmarksViewsTestsBase): + """ + This contains the tests for GET & DELETE methods of bookmark.views.BookmarksDetailView class + """ + @ddt.data( + ('', False), + ('fields=path,display_name', True) + ) + @ddt.unpack + def test_get_bookmark_successfully(self, query_params, check_optional_fields): + """ + Test that requesting bookmark returns data with 200 code. + """ + with self.assertNumQueries(6): # 1 query for bookmark table. + response = self.send_get( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': unicode(self.sequential_1.location)} + ), + query_parameters=query_params + ) + data = response.data + self.assertIsNotNone(data) + self.assert_bookmark_data_is_valid(self.bookmark_1, data, check_optional_fields=check_optional_fields) + + def test_get_bookmark_that_belongs_to_other_user(self): + """ + Test that requesting bookmark that belongs to other user returns 404 status code. + """ + with self.assertNumQueries(5): # No queries for bookmark table. + self.send_get( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)} + ), + expected_status=404 + ) + + def test_get_bookmark_that_does_not_exist(self): + """ + Test that requesting bookmark that does not exist returns 404 status code. + """ + with self.assertNumQueries(6): # 1 query for bookmark table. + response = self.send_get( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': 'i4x://arbi/100/html/340ef1771a0940'} + ), + expected_status=404 + ) + self.assertEqual( + response.data['user_message'], + 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' + ) + self.assertEqual( + response.data['developer_message'], + 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' + ) + + def test_get_bookmark_with_invalid_usage_id(self): + """ + Test that requesting bookmark with invalid usage id returns 400. + """ + with self.assertNumQueries(5): # No queries for bookmark table. + response = self.send_get( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': 'i4x'} + ), + expected_status=404 + ) + self.assertEqual(response.data['user_message'], u'Invalid usage_id: i4x.') + + def test_anonymous_access(self): + """ + Test that an anonymous client (not logged in) cannot call GET or DELETE. + """ + url = reverse('bookmarks_detail', kwargs={'username': self.user.username, 'usage_id': 'i4x'}) + with self.assertNumQueries(4): # No queries for bookmark table. + self.send_get( + client=self.anonymous_client, + url=url, + expected_status=401 + ) + + with self.assertNumQueries(1): + self.send_delete( + client=self.anonymous_client, + url=url, + expected_status=401 + ) + + def test_delete_bookmark_successfully(self): + """ + Test that delete bookmark returns 204 status code with success. + """ + query_parameters = 'course_id={}'.format(urllib.quote(self.course_id)) + response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) + bookmarks_data = response.data['results'] + self.assertEqual(len(bookmarks_data), 2) + + with self.assertNumQueries(7): # 2 queries for bookmark table. + self.send_delete( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': unicode(self.sequential_1.location)} + ) + ) + response = self.send_get(client=self.client, url=reverse('bookmarks'), query_parameters=query_parameters) + bookmarks_data = response.data['results'] + + self.assertEqual(len(bookmarks_data), 1) + + def test_delete_bookmark_that_belongs_to_other_user(self): + """ + Test that delete bookmark that belongs to other user returns 404. + """ + with self.assertNumQueries(5): # No queries for bookmark table. + self.send_delete( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': 'other', 'usage_id': unicode(self.vertical_1.location)} + ), + expected_status=404 + ) + + def test_delete_bookmark_that_does_not_exist(self): + """ + Test that delete bookmark that does not exist returns 404. + """ + with self.assertNumQueries(6): # 1 query for bookmark table. + response = self.send_delete( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': 'i4x://arbi/100/html/340ef1771a0940'} + ), + expected_status=404 + ) + self.assertEqual( + response.data['user_message'], + u'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' + ) + self.assertEqual( + response.data['developer_message'], + 'Bookmark with usage_id: i4x://arbi/100/html/340ef1771a0940 does not exist.' + ) + + def test_delete_bookmark_with_invalid_usage_id(self): + """ + Test that delete bookmark with invalid usage id returns 400. + """ + with self.assertNumQueries(5): # No queries for bookmark table. + response = self.send_delete( + client=self.client, + url=reverse( + 'bookmarks_detail', + kwargs={'username': self.user.username, 'usage_id': 'i4x'} + ), + expected_status=404 + ) + self.assertEqual(response.data['user_message'], u'Invalid usage_id: i4x.') + + def test_unsupported_methods(self): + """ + Test that POST and PUT are not supported. + """ + url = reverse('bookmarks_detail', kwargs={'username': self.user.username, 'usage_id': 'i4x'}) + self.client.login(username=self.user.username, password=self.TEST_PASSWORD) + with self.assertNumQueries(5): # No queries for bookmark table. + self.assertEqual(405, self.client.put(url).status_code) + + with self.assertNumQueries(4): + self.assertEqual(405, self.client.post(url).status_code) diff --git a/lms/djangoapps/bookmarks/urls.py b/openedx/core/djangoapps/bookmarks/urls.py similarity index 100% rename from lms/djangoapps/bookmarks/urls.py rename to openedx/core/djangoapps/bookmarks/urls.py diff --git a/lms/djangoapps/bookmarks/views.py b/openedx/core/djangoapps/bookmarks/views.py similarity index 97% rename from lms/djangoapps/bookmarks/views.py rename to openedx/core/djangoapps/bookmarks/views.py index 680dfa1476..b16c0deb22 100644 --- a/lms/djangoapps/bookmarks/views.py +++ b/openedx/core/djangoapps/bookmarks/views.py @@ -4,7 +4,7 @@ HTTP end-points for the Bookmarks API. For more information, see: https://openedx.atlassian.net/wiki/display/TNL/Bookmarks+API """ -from eventtracking import tracker +import eventtracking import logging from django.core.exceptions import ObjectDoesNotExist @@ -165,7 +165,10 @@ class BookmarksListView(ListCreateAPIView, BookmarksViewMixin): else: course_key = None - return api.get_bookmarks(user=self.request.user, course_key=course_key, serialized=False) + return api.get_bookmarks( + user=self.request.user, course_key=course_key, + fields=self.fields_to_return(self.request.QUERY_PARAMS), serialized=False + ) def paginate_queryset(self, queryset, page_size=None): """ Override GenericAPIView.paginate_queryset for the purpose of eventing """ @@ -188,7 +191,7 @@ class BookmarksListView(ListCreateAPIView, BookmarksViewMixin): event_data['list_type'] = 'per_course' event_data['course_id'] = course_id - tracker.emit('edx.bookmark.listed', event_data) + eventtracking.tracker.emit('edx.bookmark.listed', event_data) return page