Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #25809 from raccoongang/rg/fix/bookmarks_deletion

Browse Source 
In the LMS, bookmarks associated to deleted units are not deleted
This commit is contained in:
Ned Batchelder
2020-12-09 06:19:27 -05:00
committed by GitHub
parent 09bd76b47a 22a88de1ea
commit eb82e64b51
7 changed files with 121 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
cms/djangoapps/contentstore/views/item.py
View File

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

29
openedx/core/djangoapps/bookmarks/api.py
View File

@@ -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.

59
openedx/core/djangoapps/bookmarks/tests/test_api.py
View File

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

18
openedx/core/djangoapps/bookmarks/tests/test_models.py
View File

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

8
openedx/core/djangoapps/bookmarks/tests/test_services.py
View File

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

2
openedx/core/djangoapps/bookmarks/tests/test_tasks.py
View File

@@ -147,7 +147,7 @@ class XBlockCacheTaskTests(BookmarksTestsBase):
)
@ddt.data(
('course', 47),
('course', 36),
('other_course', 34)
)
@ddt.unpack

30
openedx/core/djangoapps/bookmarks/tests/test_views.py
View File

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