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 https://github.com/discourse/discourse/commit/be145ccf2fe117d97b36aa1e2fc500e44319d58c. 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.
diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb
index 5ac37b1..c4ccd2f 100644
--- a/app/models/bookmark.rb
+++ b/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?
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index d26ca60..03737f1 100644
--- a/config/locales/server.en.yml
+++ b/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"
diff --git a/config/site_settings.yml b/config/site_settings.yml
index 327b1be..ec707e7 100644
--- a/config/site_settings.yml
+++ b/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:
diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb
index 852aaba..b7dd164 100644
--- a/spec/jobs/bookmark_reminder_notifications_spec.rb
+++ b/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
diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb
index 3d4b5a6..177c003 100644
--- a/spec/models/bookmark_spec.rb
+++ b/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
diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb
index b7e7ea5..1011bb5 100644
--- a/spec/requests/bookmarks_controller_spec.rb
+++ b/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
GitHub sha: 1ba5ccd8