Skip to content

E
edx-platform
Commit a93ef10f by Usman Khalid
Options

Merge pull request #11016 from edx/ekafeel/bookmarks-limit 

Limit the number of bookmarks a learner can add in a course
parents 7338f4bf 3385d1e1
Showing with 177 additions and 36 deletions
lms/envs/aws.py
......@@ -731,3 +731,6 @@ JWT_EXPIRATION = ENV_TOKENS.get('JWT_EXPIRATION', JWT_EXPIRATION)
PROCTORING_BACKEND_PROVIDER = AUTH_TOKENS.get("PROCTORING_BACKEND_PROVIDER", PROCTORING_BACKEND_PROVIDER)
PROCTORING_SETTINGS = ENV_TOKENS.get("PROCTORING_SETTINGS", PROCTORING_SETTINGS)
# Course Content Bookmarks Settings
MAX_BOOKMARKS_PER_COURSE = ENV_TOKENS.get('MAX_BOOKMARKS_PER_COURSE', MAX_BOOKMARKS_PER_COURSE)
lms/envs/common.py
......@@ -2655,3 +2655,6 @@ CCX_MAX_STUDENTS_ALLOWED = 200
# financial assistance form
FINANCIAL_ASSISTANCE_MIN_LENGTH = 800
FINANCIAL_ASSISTANCE_MAX_LENGTH = 2500
# Course Content Bookmarks Settings
MAX_BOOKMARKS_PER_COURSE = 100
lms/static/js/bookmarks/views/bookmark_button.js
......@@ -4,8 +4,6 @@
function (gettext, $, _, Backbone, MessageBannerView) {
return Backbone.View.extend({
errorIcon: '<i class="fa fa-fw fa-exclamation-triangle message-error" aria-hidden="true"></i>',
errorMessage: gettext('An error has occurred. Please try again.'),
srAddBookmarkText: gettext('Click to add'),
......@@ -15,6 +13,8 @@
'click': 'toggleBookmark'
},
showBannerInterval: 5000, // time in ms
initialize: function (options) {
this.apiUrl = options.apiUrl;
this.bookmarkId = options.bookmarkId;
......@@ -36,7 +36,7 @@
addBookmark: function() {
var view = this;
$.ajax({
data: {usage_id: view.usageId},
data: {usage_id: view.usageId},
type: "POST",
url: view.apiUrl,
dataType: 'json',
......@@ -44,8 +44,10 @@
view.$el.trigger('bookmark:add');
view.setBookmarkState(true);
},
error: function() {
view.showError();
error: function (jqXHR) {
var response = jqXHR.responseText ? JSON.parse(jqXHR.responseText) : '';
var userMessage = response ? response.user_message : '';
view.showError(userMessage);
}
});
},
......@@ -79,13 +81,21 @@
}
},
showError: function() {
showError: function (errorText) {
var errorMsg = errorText || this.errorMessage;
if (!this.messageView) {
this.messageView = new MessageBannerView({
el: $('.message-banner')
el: $('.message-banner'),
type: 'error'
});
}
this.messageView.showMessage(this.errorMessage, this.errorIcon);
this.messageView.showMessage(errorMsg);
// Hide message automatically after some interval
setTimeout(_.bind(function () {
this.messageView.hideMessage();
}, this), this.showBannerInterval);
}
});
});
......
lms/static/js/bookmarks/views/bookmarks_list.js
......@@ -55,14 +55,11 @@
this.hideErrorMessage();
this.showBookmarksContainer();
this.showLoadingMessage();
this.collection.goTo(this.defaultPage).done(function () {
view.hideLoadingMessage();
view.render();
view.focusBookmarksElement();
}).fail(function () {
view.hideLoadingMessage();
view.showErrorMessage();
});
},
......@@ -100,7 +97,7 @@
hideBookmarks: function () {
this.$el.hide();
$(this.coursewareResultsWrapperEl).hide();
$(this.coursewareContentEl).css('display', 'table-cell');
$(this.coursewareContentEl).css( 'display', 'table-cell');
},
showBookmarksContainer: function () {
......
lms/static/js/spec/bookmarks/bookmark_button_view_spec.js
......@@ -5,6 +5,7 @@ define(['backbone', 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers
'use strict';
describe("bookmarks.button", function () {
var timerCallback;
var API_URL = 'bookmarks/api/v1/bookmarks/';
......@@ -15,6 +16,9 @@ define(['backbone', 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers
'templates/fields/message_banner'
]
);
timerCallback = jasmine.createSpy('timerCallback');
jasmine.Clock.useMock();
});
var createBookmarkButtonView = function(isBookmarked) {
......@@ -135,5 +139,18 @@ define(['backbone', 'jquery', 'underscore', 'common/js/spec_helpers/ajax_helpers
expect($messageBanner.text().trim()).toBe(bookmarkButtonView.errorMessage);
});
it('removes error message after 5 seconds', function () {
var requests = AjaxHelpers.requests(this),
$messageBanner = $('.message-banner'),
bookmarkButtonView = createBookmarkButtonView(false);
bookmarkButtonView.$el.click();
AjaxHelpers.respondWithError(requests);
expect($messageBanner.text().trim()).toBe(bookmarkButtonView.errorMessage);
jasmine.Clock.tick(5001);
expect($messageBanner.text().trim()).toBe('');
});
});
});
lms/static/js/spec/bookmarks/bookmarks_list_view_spec.js
......@@ -162,8 +162,6 @@ define(['backbone',
var expectedData = createBookmarksData({numBookmarksToCreate: 3});
bookmarksButtonView.$('.bookmarks-list-button').click();
expect($('#loading-message').text().trim()).
toBe(bookmarksButtonView.bookmarksListView.loadingMessage);
verifyRequestParams(
requests,
......
openedx/core/djangoapps/bookmarks/api.py
......@@ -3,10 +3,20 @@ Bookmarks Python API.
"""
from eventtracking import tracker
from . import DEFAULT_FIELDS, OPTIONAL_FIELDS
from xmodule.modulestore.django import modulestore
from django.conf import settings
from xmodule.modulestore.exceptions import ItemNotFoundError
from .models import Bookmark
from .serializers import BookmarkSerializer
class BookmarksLimitReachedError(Exception):
"""
if try to create new bookmark when max limit of bookmarks already reached
"""
pass
def get_bookmark(user, usage_key, fields=None):
"""
Return data for a bookmark.
......@@ -66,6 +76,28 @@ def get_bookmarks(user, course_key=None, fields=None, serialized=True):
return bookmarks_queryset
def can_create_more(data):
"""
Determine if a new Bookmark can be created for the course
based on limit defined in django.conf.settings.MAX_BOOKMARKS_PER_COURSE
Arguments:
data (dict): The data to create the object with.
Returns:
Boolean
"""
data = dict(data)
user = data['user']
course_key = data['usage_key'].course_key
# User can create up to max_bookmarks_per_course bookmarks
if Bookmark.objects.filter(user=user, course_key=course_key).count() >= settings.MAX_BOOKMARKS_PER_COURSE:
return False
return True
def create_bookmark(user, usage_key):
"""
Create a bookmark.
......@@ -79,11 +111,22 @@ def create_bookmark(user, usage_key):
Raises:
ItemNotFoundError: If no block exists for the usage_key.
BookmarksLimitReachedError: if try to create new bookmark when max limit of bookmarks already reached
"""
bookmark, created = Bookmark.create({
usage_key = usage_key.replace(course_key=modulestore().fill_in_run(usage_key.course_key))
data = {
'user': user,
'usage_key': usage_key
})
}
if usage_key.course_key.run is None:
raise ItemNotFoundError
if not can_create_more(data):
raise BookmarksLimitReachedError
bookmark, created = Bookmark.create(data)
if created:
_track_event('edx.bookmark.added', bookmark)
return BookmarkSerializer(bookmark, context={'fields': DEFAULT_FIELDS + OPTIONAL_FIELDS}).data
......
openedx/core/djangoapps/bookmarks/models.py
......@@ -74,9 +74,7 @@ class Bookmark(TimeStampedModel):
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)
......
openedx/core/djangoapps/bookmarks/tests/test_api.py
......@@ -14,6 +14,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError
from .. import api
from ..models import Bookmark
from openedx.core.djangoapps.bookmarks.api import BookmarksLimitReachedError
from .test_models import BookmarksTestsBase
......@@ -120,7 +121,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
"""
self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2)
with self.assertNumQueries(8):
with self.assertNumQueries(9):
bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location)
self.assert_bookmark_event_emitted(
......@@ -141,7 +142,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
"""
self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.course.id)), 2)
with self.assertNumQueries(8):
with self.assertNumQueries(9):
bookmark_data = api.create_bookmark(user=self.user, usage_key=self.vertical_2.location)
self.assert_bookmark_event_emitted(
......@@ -157,7 +158,7 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
mock_tracker.reset_mock()
with self.assertNumQueries(4):
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)
......@@ -177,6 +178,32 @@ class BookmarksAPITests(BookmarkApiEventTestMixin, BookmarksTestsBase):
self.assert_no_events_were_emitted(mock_tracker)
@patch('openedx.core.djangoapps.bookmarks.api.tracker.emit')
@patch('django.conf.settings.MAX_BOOKMARKS_PER_COURSE', 5)
def bookmark_more_than_limit_raise_error(self, mock_tracker):
"""
Verifies that create_bookmark raises error when maximum number of units
allowed to bookmark per course are already bookmarked.
"""
max_bookmarks = settings.MAX_BOOKMARKS_PER_COURSE
__, blocks, __ = self.create_course_with_bookmarks_count(max_bookmarks)
with self.assertNumQueries(1):
with self.assertRaises(BookmarksLimitReachedError):
api.create_bookmark(user=self.user, usage_key=blocks[-1].location)
self.assert_no_events_were_emitted(mock_tracker)
# if user tries to create bookmark in another course it should succeed
self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.other_course.id)), 1)
api.create_bookmark(user=self.user, usage_key=self.other_chapter_1.location)
self.assertEqual(len(api.get_bookmarks(user=self.user, course_key=self.other_course.id)), 2)
# if another user tries to create bookmark it should succeed
self.assertEqual(len(api.get_bookmarks(user=self.other_user, course_key=blocks[-1].location.course_key)), 0)
api.create_bookmark(user=self.other_user, usage_key=blocks[-1].location)
self.assertEqual(len(api.get_bookmarks(user=self.other_user, course_key=blocks[-1].location.course_key)), 1)
@patch('openedx.core.djangoapps.bookmarks.api.tracker.emit')
def test_delete_bookmark(self, mock_tracker):
"""
Verifies that delete_bookmark removes bookmark as expected.
......
openedx/core/djangoapps/bookmarks/tests/test_services.py
......@@ -68,7 +68,7 @@ class BookmarksServiceTests(BookmarksTestsBase):
self.bookmark_service.set_bookmarked(usage_key=UsageKey.from_string("i4x://ed/ed/ed/interactive"))
)
with self.assertNumQueries(8):
with self.assertNumQueries(9):
self.assertTrue(self.bookmark_service.set_bookmarked(usage_key=self.vertical_2.location))
def test_unset_bookmarked(self):
......
openedx/core/djangoapps/bookmarks/tests/test_views.py
......@@ -234,7 +234,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
"""
Test that posting a bookmark successfully returns newly created data with 201 code.
"""
with self.assertNumQueries(15):
with self.assertNumQueries(16):
response = self.send_post(
client=self.client,
url=reverse('bookmarks'),
......@@ -265,7 +265,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
data={'usage_id': 'invalid'},
expected_status=400
)
self.assertEqual(response.data['user_message'], u'Invalid usage_id: invalid.')
self.assertEqual(response.data['user_message'], u'An error has occurred. Please try again.')
# Send data without usage_id.
with self.assertNumQueries(7): # No queries for bookmark table.
......@@ -275,7 +275,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
data={'course_id': 'invalid'},
expected_status=400
)
self.assertEqual(response.data['user_message'], u'Parameter usage_id not provided.')
self.assertEqual(response.data['user_message'], u'An error has occurred. Please try again.')
self.assertEqual(response.data['developer_message'], u'Parameter usage_id not provided.')
# Send empty data dictionary.
......@@ -286,7 +286,7 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
data={},
expected_status=400
)
self.assertEqual(response.data['user_message'], u'No data provided.')
self.assertEqual(response.data['user_message'], u'An error has occurred. Please try again.')
self.assertEqual(response.data['developer_message'], u'No data provided.')
def test_post_bookmark_for_non_existing_block(self):
......@@ -302,13 +302,39 @@ class BookmarksListViewTests(BookmarksViewsTestsBase):
)
self.assertEqual(
response.data['user_message'],
u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.'
u'An error has occurred. Please try again.'
)
self.assertEqual(
response.data['developer_message'],
u'Block with usage_id: i4x://arbi/100/html/340ef1771a094090ad260ec940d04a21 not found.'
)
@patch('django.conf.settings.MAX_BOOKMARKS_PER_COURSE', 5)
def test_post_bookmark_when_max_bookmarks_already_exist(self):
"""
Test that posting a bookmark for a block that does not exist returns a 400.
"""
max_bookmarks = settings.MAX_BOOKMARKS_PER_COURSE
__, blocks, __ = self.create_course_with_bookmarks_count(max_bookmarks)
with self.assertNumQueries(8): # No queries for bookmark table.
response = self.send_post(
client=self.client,
url=reverse('bookmarks'),
data={'usage_id': unicode(blocks[-1].location)},
expected_status=400
)
self.assertEqual(
response.data['user_message'],
u'You can create up to {0} bookmarks.'
u' You must remove some bookmarks before you can add new ones.'.format(max_bookmarks)
)
self.assertEqual(
response.data['developer_message'],
u'You can create up to {0} bookmarks.'
u' You must remove some bookmarks before you can add new ones.'.format(max_bookmarks)
)
def test_unsupported_methods(self):
"""
Test that DELETE and PUT are not supported.
......
openedx/core/djangoapps/bookmarks/views.py
......@@ -20,6 +20,8 @@ from rest_framework_oauth.authentication import OAuth2Authentication
from opaque_keys import InvalidKeyError
from opaque_keys.edx.keys import CourseKey, UsageKey
from django.conf import settings
from openedx.core.djangoapps.bookmarks.api import BookmarksLimitReachedError
from openedx.core.lib.api.permissions import IsUserInUrl
......@@ -33,6 +35,9 @@ from .serializers import BookmarkSerializer
log = logging.getLogger(__name__)
# Default error message for user
DEFAULT_USER_MESSAGE = ugettext_noop(u'An error has occurred. Please try again.')
class BookmarksPagination(DefaultPagination):
"""
......@@ -71,7 +76,7 @@ class BookmarksViewMixin(object):
optional_fields = params.get('fields', '').split(',')
return DEFAULT_FIELDS + [field for field in optional_fields if field in OPTIONAL_FIELDS]
def error_response(self, message, error_status=status.HTTP_400_BAD_REQUEST):
def error_response(self, developer_message, user_message=None, error_status=status.HTTP_400_BAD_REQUEST):
"""
Create and return a Response.
......@@ -80,10 +85,13 @@ class BookmarksViewMixin(object):
and user_message fields.
status: The status of the response. Default is HTTP_400_BAD_REQUEST.
"""
if not user_message:
user_message = developer_message
return Response(
{
"developer_message": message,
"user_message": _(message) # pylint: disable=translation-of-non-string
"developer_message": developer_message,
"user_message": _(user_message) # pylint: disable=translation-of-non-string
},
status=error_status
)
......@@ -219,25 +227,36 @@ class BookmarksListView(ListCreateAPIView, BookmarksViewMixin):
POST /api/bookmarks/v1/bookmarks/
Request data: {"usage_id": "<usage-id>"}
"""
if not request.data:
return self.error_response(ugettext_noop(u'No data provided.'))
return self.error_response(ugettext_noop(u'No data provided.'), DEFAULT_USER_MESSAGE)
usage_id = request.data.get('usage_id', None)
if not usage_id:
return self.error_response(ugettext_noop(u'Parameter usage_id not provided.'))
return self.error_response(ugettext_noop(u'Parameter usage_id not provided.'), DEFAULT_USER_MESSAGE)
try:
usage_key = UsageKey.from_string(unquote_slashes(usage_id))
except InvalidKeyError:
error_message = ugettext_noop(u'Invalid usage_id: {usage_id}.').format(usage_id=usage_id)
log.error(error_message)
return self.error_response(error_message)
return self.error_response(error_message, DEFAULT_USER_MESSAGE)
try:
bookmark = api.create_bookmark(user=self.request.user, usage_key=usage_key)
except ItemNotFoundError:
error_message = ugettext_noop(u'Block with usage_id: {usage_id} not found.').format(usage_id=usage_id)
log.error(error_message)
return self.error_response(error_message, DEFAULT_USER_MESSAGE)
except BookmarksLimitReachedError:
error_message = ugettext_noop(
u'You can create up to {max_num_bookmarks_per_course} bookmarks.'
u' You must remove some bookmarks before you can add new ones.'
).format(max_num_bookmarks_per_course=settings.MAX_BOOKMARKS_PER_COURSE)
log.info(
u'Attempted to create more than %s bookmarks',
settings.MAX_BOOKMARKS_PER_COURSE
)
return self.error_response(error_message)
return Response(bookmark, status=status.HTTP_201_CREATED)
......@@ -301,7 +320,7 @@ class BookmarksDetailView(APIView, BookmarksViewMixin):
except InvalidKeyError:
error_message = ugettext_noop(u'Invalid usage_id: {usage_id}.').format(usage_id=usage_id)
log.error(error_message)
return self.error_response(error_message, status.HTTP_404_NOT_FOUND)
return self.error_response(error_message, error_status=status.HTTP_404_NOT_FOUND)
def get(self, request, username=None, usage_id=None): # pylint: disable=unused-argument
"""
......@@ -323,7 +342,7 @@ class BookmarksDetailView(APIView, BookmarksViewMixin):
u'Bookmark with usage_id: {usage_id} does not exist.'
).format(usage_id=usage_id)
log.error(error_message)
return self.error_response(error_message, status.HTTP_404_NOT_FOUND)
return self.error_response(error_message, error_status=status.HTTP_404_NOT_FOUND)
return Response(bookmark_data)
......@@ -343,6 +362,6 @@ class BookmarksDetailView(APIView, BookmarksViewMixin):
u'Bookmark with usage_id: {usage_id} does not exist.'
).format(usage_id=usage_id)
log.error(error_message)
return self.error_response(error_message, status.HTTP_404_NOT_FOUND)
return self.error_response(error_message, error_status=status.HTTP_404_NOT_FOUND)
return Response(status=status.HTTP_204_NO_CONTENT)
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment