In the LMS, bookmarks associated to deleted units are not deleted

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)
 
 

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.

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):
         """

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')
 

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.assertEqual(len(bookmarks_data), 4)
-        self.assert_bookmark_data_is_valid(self.bookmark_2, bookmarks_data[0])
+        self.assert_bookmark_data_is_valid(self.bookmark_4, bookmarks_data[0])
-        self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[1])
+        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):
         """

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

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_4, bookmarks_data[1])
-        self.assert_bookmark_data_is_valid(self.bookmark_1, bookmarks_data[2])
+        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': -1, 'expected_bookmarks_count': 4, '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': 0, 'expected_bookmarks_count': 4, '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': 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):
         """