From 1ba5ccd8af6cb2fee214063980508422159b7251 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 9 Apr 2021 13:06:35 +1000 Subject: [PATCH] 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. --- app/models/bookmark.rb | 15 +++++++++++---- config/locales/server.en.yml | 2 +- config/site_settings.yml | 3 +++ .../bookmark_reminder_notifications_spec.rb | 9 +++++++++ spec/models/bookmark_spec.rb | 19 +++++++++++++++++++ spec/requests/bookmarks_controller_spec.rb | 11 ++--------- 6 files changed, 45 insertions(+), 14 deletions(-) diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 5ac37b106c7..c4ccd2f24fc 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 d26ca60134d..03737f1da65 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 %{user_bookmarks_url} 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 327b1bee977..ec707e762b5 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 852aaba13e1..b7dd16454bf 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 3d4b5a6acf1..177c0037325 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 b7e7ea52b7f..1011bb517b3 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