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