From e58e7a49f54a5042ae0d487ad8ab4c36037e715f Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 26 Aug 2024 09:17:39 +1000 Subject: [PATCH] FIX: Bookmark reminder was clearing incorrectly (#28506) Followup 76c56c82845b4fce63c6f1cdf4ac3935bf1747e7 The change introduced above made it so the expired bookmark reminders were cleared when using the bulk action menu for bookmarks. However this also affected clearing reminders for bookmarks when sending notifications. When clearing bookmark reminders after sending notifications, we take into account the auto delete preference: * never - The bookmark `reminder_at` date should not be cleared, and the bookmark is kept. * clear_reminder - The bookmark `reminder_at` date is cleared and the bookmark is kept The `never` option made it so "expired" bookmark reminder show on the user's bookmark list. This commit fixes the change from the other commit and only forces clearing of `reminder_at` if using the bookmark bulk action service. --- app/models/bookmark.rb | 10 +++++-- lib/bookmark_reminder_notification_handler.rb | 14 ++-------- lib/bookmarks_bulk_action.rb | 26 ++++++++++++------- ...mark_reminder_notification_handler_spec.rb | 12 +++++++++ spec/lib/bookmarks_bulk_action_spec.rb | 18 ++++++------- 5 files changed, 47 insertions(+), 33 deletions(-) diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 2c98fe44dcb..41e8e7e75e4 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -119,8 +119,14 @@ class Bookmark < ActiveRecord::Base (reminder_at + offset).strftime(I18n.t("datetime_formats.formats.calendar_ics")) end - def clear_reminder! - update!(reminder_last_sent_at: Time.zone.now, reminder_set_at: nil, reminder_at: nil) + def clear_reminder!(force_clear_reminder_at: false) + reminder_update_attrs = { reminder_last_sent_at: Time.zone.now, reminder_set_at: nil } + + if self.auto_clear_reminder_when_reminder_sent? || force_clear_reminder_at + reminder_update_attrs[:reminder_at] = nil + end + + update!(reminder_update_attrs) end def reminder_at_in_zone(timezone) diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 9e041c28e25..c4b741a118e 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -11,7 +11,7 @@ class BookmarkReminderNotificationHandler return if bookmark.blank? Bookmark.transaction do if !bookmark.registered_bookmarkable.can_send_reminder?(bookmark) - clear_reminder + bookmark.clear_reminder! else bookmark.registered_bookmarkable.send_reminder_notification(bookmark) @@ -19,18 +19,8 @@ class BookmarkReminderNotificationHandler BookmarkManager.new(bookmark.user).destroy(bookmark.id) end - clear_reminder + bookmark.clear_reminder! end end end - - def clear_reminder - Rails.logger.debug( - "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}", - ) - - bookmark.reminder_at = nil if bookmark.auto_clear_reminder_when_reminder_sent? - - bookmark.clear_reminder! - end end diff --git a/lib/bookmarks_bulk_action.rb b/lib/bookmarks_bulk_action.rb index 3050ab1162d..442294af2b5 100644 --- a/lib/bookmarks_bulk_action.rb +++ b/lib/bookmarks_bulk_action.rb @@ -17,27 +17,33 @@ class BookmarksBulkAction if BookmarksBulkAction.operations.exclude?(@operation[:type]) raise Discourse::InvalidParameters.new(:operation) end - # careful these are private methods, we need send - send(@operation[:type]) + + case @operation[:type] + when "clear_reminder" + clear_reminder + when "delete" + delete + end + @changed_ids.sort end private def delete - @bookmark_ids.each do |b_id| - if guardian.can_delete?(b_id) - BookmarkManager.new(@user).destroy(b_id) - @changed_ids << b_id + @bookmark_ids.each do |bookmark_id| + if guardian.can_delete?(bookmark_id) + BookmarkManager.new(@user).destroy(bookmark_id) + @changed_ids << bookmark_id end end end def clear_reminder - bookmarks.each do |b| - if guardian.can_edit?(b) - BookmarkReminderNotificationHandler.new(b).clear_reminder - @changed_ids << b.id + bookmarks.each do |bookmark| + if guardian.can_edit?(bookmark) + bookmark.clear_reminder!(force_clear_reminder_at: true) + @changed_ids << bookmark.id else raise Discourse::InvalidAccess.new end diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index 7057839e591..103ada423f1 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -87,5 +87,17 @@ RSpec.describe BookmarkReminderNotificationHandler do expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil) end end + + context "when the auto_delete_preference is never" do + before do + TopicUser.create!(topic: bookmark.bookmarkable.topic, user: user, bookmarked: true) + bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:never]) + end + + it "does not reset reminder_at after the reminder gets sent" do + send_notification + expect(Bookmark.find_by(id: bookmark.id).reminder_at).not_to eq(nil) + end + end end end diff --git a/spec/lib/bookmarks_bulk_action_spec.rb b/spec/lib/bookmarks_bulk_action_spec.rb index 4f2ec49684f..55ab4fa9956 100644 --- a/spec/lib/bookmarks_bulk_action_spec.rb +++ b/spec/lib/bookmarks_bulk_action_spec.rb @@ -12,8 +12,8 @@ RSpec.describe BookmarksBulkAction do bba = BookmarksBulkAction.new(user_2, [bookmark_1.id, bookmark_2.id], type: "delete") expect { bba.perform! }.to raise_error Discourse::InvalidAccess - expect(Bookmark.where(id: bookmark_1.id)).to_not be_empty - expect(Bookmark.where(id: bookmark_2.id)).to_not be_empty + expect(Bookmark.exists?(bookmark_1.id)).to eq(true) + expect(Bookmark.exists?(bookmark_2.id)).to eq(true) end end @@ -22,8 +22,8 @@ RSpec.describe BookmarksBulkAction do bba = BookmarksBulkAction.new(user, [bookmark_1.id, bookmark_2.id], type: "delete") bba.perform! - expect(Bookmark.where(id: bookmark_1.id)).to be_empty - expect(Bookmark.where(id: bookmark_2.id)).to be_empty + expect(Bookmark.exists?(bookmark_1.id)).to eq(false) + expect(Bookmark.exists?(bookmark_2.id)).to eq(false) end end end @@ -32,7 +32,7 @@ RSpec.describe BookmarksBulkAction do fab!(:bookmark_with_reminder) { Fabricate(:bookmark_next_business_day_reminder, user: user) } describe "when user is not the bookmark owner" do - it "does NOT clear the reminder" do + it "does not clear the reminder" do bba = BookmarksBulkAction.new(user_2, [bookmark_with_reminder], type: "clear_reminder") expect { bba.perform! }.to raise_error Discourse::InvalidAccess expect(Bookmark.find_by_id(bookmark_with_reminder).reminder_set_at).to_not be_nil @@ -40,13 +40,13 @@ RSpec.describe BookmarksBulkAction do end describe "when user is the bookmark owner" do - it "deletes the bookmarks" do + it "clears the bookmark reminders, including expired reminders" do expect do bba = BookmarksBulkAction.new(user, [bookmark_with_reminder.id], type: "clear_reminder") bba.perform! - end.to change { Bookmark.find_by_id(bookmark_with_reminder.id).reminder_set_at }.to( - nil, - ).and change { Bookmark.find_by_id(bookmark_with_reminder.id).reminder_at }.to(nil) + end.to change { bookmark_with_reminder.reload.reminder_set_at }.to(nil).and change { + bookmark_with_reminder.reload.reminder_at + }.to(nil) end end end