diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 8850c9e6e2..d8bcf7bca6 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -39,6 +39,7 @@ from common.djangoapps.edxmako.shortcuts import render_to_string from openedx.core.djangoapps.schedules.config import COURSE_UPDATE_WAFFLE_FLAG from openedx.core.lib.gating import api as gating_api from openedx.core.lib.xblock_utils import hash_resource, request_token, wrap_xblock, wrap_xblock_aside +from openedx.core.djangoapps.bookmarks import api as bookmarks_api from common.djangoapps.static_replace import replace_static_urls from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from openedx.core.toggles import ENTRANCE_EXAMS @@ -987,6 +988,8 @@ def _delete_item(usage_key, user): course.tabs = [tab for tab in existing_tabs if tab.get('url_slug') != usage_key.block_id] store.update_item(course, user.id) + # Delete user bookmarks + bookmarks_api.delete_bookmarks(usage_key) store.delete_item(usage_key, user.id) diff --git a/openedx/core/djangoapps/bookmarks/api.py b/openedx/core/djangoapps/bookmarks/api.py index 67b1c701de..f58a5f38fe 100644 --- a/openedx/core/djangoapps/bookmarks/api.py +++ b/openedx/core/djangoapps/bookmarks/api.py @@ -159,6 +159,35 @@ def delete_bookmark(user, usage_key): _track_event('edx.bookmark.removed', bookmark) +def delete_bookmarks(usage_key): + """ + Delete all bookmarks for usage_key. + + Arguments: + usage_key (UsageKey): The usage_key of the bookmarks. + """ + units_keys = [] + + if usage_key.block_type == 'vertical': + units_keys.append(usage_key) + else: + # NOTE: Get all children for deleted block + descriptor = modulestore().get_item(usage_key) + for child in descriptor.get_children(): + if usage_key.block_type == 'chapter': + units_keys += [unit.location for unit in child.get_children()] + else: + units_keys.append(child.location) + + bookmarks = Bookmark.objects.filter(usage_key__in=units_keys) + + # Emit removed bookmard event + for bookmark in bookmarks: + _track_event('edx.bookmark.removed', bookmark) + + bookmarks.delete() + + def _track_event(event_name, bookmark): """ Emit events for a bookmark. diff --git a/openedx/core/djangoapps/bookmarks/tests/test_api.py b/openedx/core/djangoapps/bookmarks/tests/test_api.py index 3ddff6ba0b..dbd2737027 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_api.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_api.py @@ -7,7 +7,7 @@ import ddt import six from django.conf import settings from django.core.exceptions import ObjectDoesNotExist -from mock import patch +from mock import Mock, patch from opaque_keys.edx.keys import UsageKey from openedx.core.djangoapps.bookmarks.api import BookmarksLimitReachedError @@ -79,7 +79,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): # Without course key. with self.assertNumQueries(1): bookmarks_data = api.get_bookmarks(user=self.user) - self.assertEqual(len(bookmarks_data), count + 3) + self.assertEqual(len(bookmarks_data), count + 5) # 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]) @@ -88,7 +88,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): # 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.assertEqual(len(bookmarks_data), count + 5) self.assert_bookmark_data_is_valid(bookmarks[-1], bookmarks_data[0]) self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[-1]) @@ -117,7 +117,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): """ Verifies that create_bookmark create & returns data as expected. """ - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2) + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 4) with self.assertNumQueries(9): bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location) @@ -131,14 +131,14 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): component_usage_id=six.text_type(self.vertical_2.location), ) - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 3) + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 5) @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) + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 4) with self.assertNumQueries(9): bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location) @@ -152,14 +152,14 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): component_usage_id=six.text_type(self.vertical_2.location), ) - self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 3) + self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 5) mock_tracker.reset_mock() with self.assertNumQueries(5): 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(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 5) self.assertEqual(bookmark_data, bookmark_data_2) self.assert_no_events_were_emitted(mock_tracker) @@ -206,7 +206,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): """ Verifies that delete_bookmark removes bookmark as expected. """ - self.assertEqual(len(api.get_bookmarks(user=self.user)), 3) + self.assertEqual(len(api.get_bookmarks(user=self.user)), 5) with self.assertNumQueries(3): api.delete_bookmark(user=self.user, usage_key=self.sequential_1.location) @@ -221,10 +221,49 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase): ) bookmarks_data = api.get_bookmarks(user=self.user) - self.assertEqual(len(bookmarks_data), 2) + self.assertEqual(len(bookmarks_data), 4) self.assertNotEqual(six.text_type(self.sequential_1.location), bookmarks_data[0]['usage_id']) self.assertNotEqual(six.text_type(self.sequential_1.location), bookmarks_data[1]['usage_id']) + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_delete_bookmarks_with_vertical_block_type(self, mock_tracker): + self.assertEqual(len(api.get_bookmarks(user=self.user)), 5) + + api.delete_bookmarks(usage_key=self.vertical_3.location) + + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.removed', + course_id=six.text_type(self.course_id), + bookmark_id=self.bookmark_3.resource_id, + component_type=self.vertical_1.location.block_type, + component_usage_id=six.text_type(self.vertical_3.location), + ) + + self.assertEqual(len(api.get_bookmarks(self.user)), 4) + + @patch('openedx.core.djangoapps.bookmarks.api.modulestore') + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') + def test_delete_bookmarks_with_chapter_block_type(self, mock_tracker, mocked_modulestore): + mocked_modulestore().get_item().get_children = Mock( + return_value=[Mock(get_children=Mock(return_value=[Mock( + location=self.chapter_2.location)]))]) + + api.delete_bookmarks(usage_key=self.chapter_2.location) + + self.assertEqual(mocked_modulestore.call_count, 2) + self.assertEqual(mocked_modulestore().get_item.call_count, 2) + mocked_modulestore().get_item().get_children.assert_called() + self.assert_bookmark_event_emitted( + mock_tracker, + event_name='edx.bookmark.removed', + course_id=six.text_type(self.course_id), + bookmark_id=self.bookmark_4.resource_id, + component_type=self.chapter_2.location.block_type, + component_usage_id=six.text_type(self.chapter_2.location), + ) + self.assertEqual(len(api.get_bookmarks(self.user)), 4) + @patch('openedx.core.djangoapps.bookmarks.api.tracker.emit') def test_delete_bookmark_raises_error(self, mock_tracker): """ diff --git a/openedx/core/djangoapps/bookmarks/tests/test_models.py b/openedx/core/djangoapps/bookmarks/tests/test_models.py index e60e11014b..a17dd617ef 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_models.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_models.py @@ -112,6 +112,24 @@ class BookmarksTestsBase(ModuleStoreTestCase): 'usage_key': self.sequential_2.location, }), ) + self.bookmark_3 = BookmarkFactory.create( + user=self.user, + course_key=self.course_id, + usage_key=self.vertical_3.location, + xblock_cache=XBlockCache.create({ + 'display_name': self.vertical_3.display_name, + 'usage_key': self.vertical_3.location, + }), + ) + self.bookmark_4 = BookmarkFactory.create( + user=self.user, + course_key=self.course_id, + usage_key=self.chapter_2.location, + xblock_cache=XBlockCache.create({ + 'display_name': self.chapter_2.display_name, + 'usage_key': self.chapter_2.location, + }), + ) self.other_course = CourseFactory.create(display_name='An Introduction to API Testing 2') diff --git a/openedx/core/djangoapps/bookmarks/tests/test_services.py b/openedx/core/djangoapps/bookmarks/tests/test_services.py index 73a0578f9b..f74066af9e 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_services.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_services.py @@ -29,9 +29,11 @@ class BookmarksServiceTests(BookmarksTestsBase): 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]) + self.assertEqual(len(bookmarks_data), 4) + self.assert_bookmark_data_is_valid(self.bookmark_4, bookmarks_data[0]) + self.assert_bookmark_data_is_valid(self.bookmark_3, bookmarks_data[1]) + self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[2]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[3]) def test_is_bookmarked(self): """ diff --git a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py index 840fbd8b54..bc907cf0c0 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_tasks.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_tasks.py @@ -147,7 +147,7 @@ class XBlockCacheTaskTests(BookmarksTestsBase): ) @ddt.data( - ('course', 47), + ('course', 36), ('other_course', 34) ) @ddt.unpack diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py index c60725ba38..332646fc6a 100644 --- a/openedx/core/djangoapps/bookmarks/tests/test_views.py +++ b/openedx/core/djangoapps/bookmarks/tests/test_views.py @@ -191,16 +191,18 @@ class BookmarksListViewTests(BookmarksViewsTestsBase): url=reverse('bookmarks') ) bookmarks_data = response.data['results'] - self.assertEqual(len(bookmarks_data), 3) + self.assertEqual(len(bookmarks_data), 5) 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_data_is_valid(self.bookmark_4, bookmarks_data[1]) + self.assert_bookmark_data_is_valid(self.bookmark_3, bookmarks_data[2]) + self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[3]) + self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[4]) self.assert_bookmark_event_emitted( mock_tracker, event_name='edx.bookmark.listed', list_type='all_courses', - bookmarks_count=3, + bookmarks_count=5, page_size=10, page_number=1 ) @@ -230,16 +232,16 @@ class BookmarksListViewTests(BookmarksViewsTestsBase): response = self.send_post( client=self.client, url=reverse('bookmarks'), - data={'usage_id': six.text_type(self.vertical_3.location)} + data={'usage_id': six.text_type(self.vertical_2.location)} ) # Assert Newly created bookmark. - self.assertEqual(response.data['id'], '%s,%s' % (self.user.username, six.text_type(self.vertical_3.location))) + self.assertEqual(response.data['id'], '%s,%s' % (self.user.username, six.text_type(self.vertical_2.location))) self.assertEqual(response.data['course_id'], self.course_id) - self.assertEqual(response.data['usage_id'], six.text_type(self.vertical_3.location)) + self.assertEqual(response.data['usage_id'], six.text_type(self.vertical_2.location)) self.assertIsNotNone(response.data['created']) self.assertEqual(len(response.data['path']), 2) - self.assertEqual(response.data['display_name'], self.vertical_3.display_name) + self.assertEqual(response.data['display_name'], self.vertical_2.display_name) def test_post_bookmark_with_invalid_data(self): """ @@ -334,9 +336,9 @@ class BookmarksListViewTests(BookmarksViewsTestsBase): @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': 100, 'expected_page_number': 1} + {'page_size': -1, 'expected_bookmarks_count': 4, 'expected_page_size': 10, 'expected_page_number': 1}, + {'page_size': 0, 'expected_bookmarks_count': 4, 'expected_page_size': 10, 'expected_page_number': 1}, + {'page_size': 999, 'expected_bookmarks_count': 4, 'expected_page_size': 100, '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): @@ -364,7 +366,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase): mock_tracker, event_name='edx.bookmark.listed', list_type='all_courses', - bookmarks_count=3, + bookmarks_count=5, page_size=2, page_number=2 ) @@ -482,7 +484,7 @@ class BookmarksDetailViewTests(BookmarksViewsTestsBase): query_parameters = 'course_id={}'.format(six.moves.urllib.parse.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) + self.assertEqual(len(bookmarks_data), 4) self.send_delete( client=self.client, @@ -494,7 +496,7 @@ class BookmarksDetailViewTests(BookmarksViewsTestsBase): 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) + self.assertEqual(len(bookmarks_data), 3) def test_delete_bookmark_that_belongs_to_other_user(self): """