FIX: When user has already hit bookmark limit, do not error for clear_reminder! or other updates (#12658)

We introduced a cap on the number of bookmarks the user can add in be145ccf2f. However this has caused unintended side effects; when the `jobs/scheduled/bookmark_reminder_notifications.rb` runs we get this error for users who already had more bookmarks than the limit:

> Job exception: Validation failed: Sorry, you have too many bookmarks, visit #{url}/my/activity/bookmarks to remove some.

This is because the `clear_reminder!` call was triggering a bookmark validation, which raised an error because the user already had to many, holding up other reminders.

This PR also adds `max_bookmarks_per_user` hidden site setting (default 2000). This replaces the BOOKMARK_LIMIT const so we can raise it for certain sites.

app/models/bookmark.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class Bookmark < ActiveRecord::Base
-  BOOKMARK_LIMIT = 2000
-
   self.ignored_columns = [
     "delete_when_reminder_sent" # TODO(2021-07-22): remove
   ]
@@ -69,8 +67,17 @@ class Bookmark < ActiveRecord::Base
   end
 
   def bookmark_limit_not_reached
-    return if user.bookmarks.count < BOOKMARK_LIMIT
-    self.errors.add(:base, I18n.t("bookmarks.errors.too_many", user_bookmarks_url: "#{Discourse.base_url}/my/activity/bookmarks"))
+    return if user.bookmarks.count < SiteSetting.max_bookmarks_per_user
+    return if !new_record?
+
+    self.errors.add(
+      :base,
+      I18n.t(
+        "bookmarks.errors.too_many",
+        user_bookmarks_url: "#{Discourse.base_url}/my/activity/bookmarks",
+        limit: SiteSetting.max_bookmarks_per_user
+      )
+    )
   end
 
   def no_reminder?

config/locales/server.en.yml

@@ -416,7 +416,7 @@ en:
   bookmarks:
     errors:
       already_bookmarked_post: "You cannot bookmark the same post twice."
-      too_many: "Sorry, you have too many bookmarks, visit %{user_bookmarks_url} to remove some."
+      too_many: "Sorry, you cannot add more than %{limit} bookmarks, visit <a href='%{user_bookmarks_url}'>%{user_bookmarks_url}</a> to remove some."
       cannot_set_past_reminder: "You cannot set a bookmark reminder in the past."
       cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future."
       time_must_be_provided: "time must be provided for all reminders"

config/site_settings.yml

@@ -659,6 +659,9 @@ users:
   gravatar_login_url:
     default: /emails
     client: true
+  max_bookmarks_per_user:
+    default: 2000
+    hidden: true
 
 groups:
   enable_group_directory:

spec/jobs/bookmark_reminder_notifications_spec.rb

@@ -48,6 +48,15 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
     expect(bookmark4.reload.reminder_at).not_to eq(nil)
   end
 
+  context "when a user is over the bookmark limit" do
+    it "clearing their reminder does not error and hold up the rest" do
+      other_bookmark = Fabricate(:bookmark, user: bookmark1.user)
+      other_bookmark.update_column(:reminder_at, five_minutes_ago)
+      SiteSetting.max_bookmarks_per_user = 2
+      expect { subject.execute }.not_to raise_error
+    end
+  end
+
   context "when the number of notifications exceed max_reminder_notifications_per_run" do
     it "does not send them in the current run, but will send them in the next" do
       begin

spec/models/bookmark_spec.rb

@@ -39,5 +39,24 @@ describe Bookmark do
       expect(Bookmark.find(bookmark.id)).to eq(bookmark)
       expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2)
     end
+
+    describe "bookmark limits" do
+      fab!(:user) { Fabricate(:user) }
+
+      it "does not get the bookmark limit error because it is not creating a new bookmark (for users already over the limit)" do
+        Fabricate(:bookmark, user: user)
+        Fabricate(:bookmark, user: user)
+        last_bookmark = Fabricate(:bookmark, user: user)
+        SiteSetting.max_bookmarks_per_user = 2
+        expect { last_bookmark.clear_reminder! }.not_to raise_error
+      end
+
+      it "gets the bookmark limit error when creating a new bookmark over the limit" do
+        Fabricate(:bookmark, user: user)
+        Fabricate(:bookmark, user: user)
+        SiteSetting.max_bookmarks_per_user = 2
+        expect { Fabricate(:bookmark, user: user) }.to raise_error(ActiveRecord::RecordInvalid)
+      end
+    end
   end
 end

spec/requests/bookmarks_controller_spec.rb

@@ -33,9 +33,7 @@ describe BookmarksController do
 
     context "if the user reached the max bookmark limit" do
       before do
-        @old_constant = Bookmark::BOOKMARK_LIMIT
-        Bookmark.send(:remove_const, "BOOKMARK_LIMIT")
-        Bookmark.const_set("BOOKMARK_LIMIT", 1)
+        SiteSetting.max_bookmarks_per_user = 1
       end
 
       it "returns failed JSON with a 400 error" do
@@ -51,14 +49,9 @@ describe BookmarksController do
         expect(response.status).to eq(400)
         user_bookmarks_url = "#{Discourse.base_url}/my/activity/bookmarks"
         expect(response.parsed_body['errors']).to include(
-          I18n.t("bookmarks.errors.too_many", user_bookmarks_url: user_bookmarks_url)
+          I18n.t("bookmarks.errors.too_many", user_bookmarks_url: user_bookmarks_url, limit: SiteSetting.max_bookmarks_per_user)
         )
       end
-
-      after do
-        Bookmark.send(:remove_const, "BOOKMARK_LIMIT")
-        Bookmark.const_set("BOOKMARK_LIMIT", @old_constant)
-      end
     end
 
     context "if the user already has bookmarked the post" do