TNL-2810 backend implementation to limit the maximum bookmarks per course

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