diff --git a/lms/envs/aws.py b/lms/envs/aws.py
index 868fbd9251..80c5373273 100644
--- a/lms/envs/aws.py
+++ b/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)
diff --git a/lms/envs/common.py b/lms/envs/common.py
index e622fe1199..a90f10f2cb 100644
--- a/lms/envs/common.py
+++ b/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
diff --git a/lms/static/js/bookmarks/views/bookmark_button.js b/lms/static/js/bookmarks/views/bookmark_button.js
index cd31d8e275..869169ce02 100644
--- a/lms/static/js/bookmarks/views/bookmark_button.js
+++ b/lms/static/js/bookmarks/views/bookmark_button.js
@@ -4,8 +4,6 @@
function (gettext, $, _, Backbone, MessageBannerView) {
return Backbone.View.extend({
-
- errorIcon: '',
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);
}
});
});
diff --git a/lms/static/js/bookmarks/views/bookmarks_list.js b/lms/static/js/bookmarks/views/bookmarks_list.js
index 2f61978c9f..3b9fc1b01b 100644
--- a/lms/static/js/bookmarks/views/bookmarks_list.js
+++ b/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 () {
diff --git a/lms/static/js/spec/bookmarks/bookmark_button_view_spec.js b/lms/static/js/spec/bookmarks/bookmark_button_view_spec.js
index ad04b44646..4b83ecf3e0 100644
--- a/lms/static/js/spec/bookmarks/bookmark_button_view_spec.js
+++ b/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('');
+ });
});
});
diff --git a/lms/static/js/spec/bookmarks/bookmarks_list_view_spec.js b/lms/static/js/spec/bookmarks/bookmarks_list_view_spec.js
index e2ae7ab9d8..ab356d348a 100644
--- a/lms/static/js/spec/bookmarks/bookmarks_list_view_spec.js
+++ b/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,
diff --git a/openedx/core/djangoapps/bookmarks/api.py b/openedx/core/djangoapps/bookmarks/api.py
index 93bbe434bb..3aa81dd306 100644
--- a/openedx/core/djangoapps/bookmarks/api.py
+++ b/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
diff --git a/openedx/core/djangoapps/bookmarks/models.py b/openedx/core/djangoapps/bookmarks/models.py
index 788710a1a4..662afc42e1 100644
--- a/openedx/core/djangoapps/bookmarks/models.py
+++ b/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)
diff --git a/openedx/core/djangoapps/bookmarks/tests/test_api.py b/openedx/core/djangoapps/bookmarks/tests/test_api.py
index 8104c264ff..f9e6f36dfe 100644
--- a/openedx/core/djangoapps/bookmarks/tests/test_api.py
+++ b/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)
@@ -176,6 +177,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):
"""
diff --git a/openedx/core/djangoapps/bookmarks/tests/test_services.py b/openedx/core/djangoapps/bookmarks/tests/test_services.py
index 78e58f2ce8..d55ab4d37b 100644
--- a/openedx/core/djangoapps/bookmarks/tests/test_services.py
+++ b/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):
diff --git a/openedx/core/djangoapps/bookmarks/tests/test_views.py b/openedx/core/djangoapps/bookmarks/tests/test_views.py
index ac82de893d..3de5bfe670 100644
--- a/openedx/core/djangoapps/bookmarks/tests/test_views.py
+++ b/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.
diff --git a/openedx/core/djangoapps/bookmarks/views.py b/openedx/core/djangoapps/bookmarks/views.py
index 69b03afb1e..31a6578ff6 100644
--- a/openedx/core/djangoapps/bookmarks/views.py
+++ b/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": ""}
"""
+
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)