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

@@ -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)

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

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

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

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