FEATURE: Highlight expired bookmark reminders (#15317)

The user can select what happens with a bookamrk after it expires. New
option allow bookmark's reminder to be kept even after it has expired.
After a bookmark's reminder notification is created, the reminder date
will be highlighted in red until the user resets the reminder date.
User can do that using the new Clear Reminder button from the dropdown.

app/assets/javascripts/discourse/app/components/bookmark-actions-dropdown.js

@@ -5,6 +5,7 @@ import I18n from "I18n";
 
 const ACTION_REMOVE = "remove";
 const ACTION_EDIT = "edit";
+const ACTION_CLEAR_REMINDER = "clear_reminder";
 const ACTION_PIN = "pin";
 
 export default DropdownSelectBoxComponent.extend({
@@ -18,32 +19,45 @@ export default DropdownSelectBoxComponent.extend({
 
   @discourseComputed("bookmark")
   content(bookmark) {
-    return [
+    const actions = [];
-      {
+
-        id: ACTION_REMOVE,
+    actions.push({
-        icon: "trash-alt",
+      id: ACTION_REMOVE,
-        name: I18n.t("post.bookmarks.actions.delete_bookmark.name"),
+      icon: "trash-alt",
+      name: I18n.t("post.bookmarks.actions.delete_bookmark.name"),
+      description: I18n.t("post.bookmarks.actions.delete_bookmark.description"),
+    });
+
+    actions.push({
+      id: ACTION_EDIT,
+      icon: "pencil-alt",
+      name: I18n.t("post.bookmarks.actions.edit_bookmark.name"),
+      description: I18n.t("post.bookmarks.actions.edit_bookmark.description"),
+    });
+
+    if (bookmark.reminder_at) {
+      actions.push({
+        id: ACTION_CLEAR_REMINDER,
+        icon: "history",
+        name: I18n.t("post.bookmarks.actions.clear_bookmark_reminder.name"),
         description: I18n.t(
-          "post.bookmarks.actions.delete_bookmark.description"
+          "post.bookmarks.actions.clear_bookmark_reminder.description"
         ),
-      },
+      });
-      {
+    }
-        id: ACTION_EDIT,
+
-        icon: "pencil-alt",
+    actions.push({
-        name: I18n.t("post.bookmarks.actions.edit_bookmark.name"),
+      id: ACTION_PIN,
-        description: I18n.t("post.bookmarks.actions.edit_bookmark.description"),
+      icon: "thumbtack",
-      },
+      name: I18n.t(
-      {
+        `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.name`
-        id: ACTION_PIN,
+      ),
-        icon: "thumbtack",
+      description: I18n.t(
-        name: I18n.t(
+        `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.description`
-          `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.name`
+      ),
-        ),
+    });
-        description: I18n.t(
+
-          `post.bookmarks.actions.${bookmark.pinAction()}_bookmark.description`
+    return actions;
-        ),
-      },
-    ];
   },
 
   @action
@@ -52,6 +66,8 @@ export default DropdownSelectBoxComponent.extend({
       this.removeBookmark(this.bookmark);
     } else if (selectedAction === ACTION_EDIT) {
       this.editBookmark(this.bookmark);
+    } else if (selectedAction === ACTION_CLEAR_REMINDER) {
+      this.clearBookmarkReminder(this.bookmark);
     } else if (selectedAction === ACTION_PIN) {
       this.togglePinBookmark(this.bookmark);
     }

app/assets/javascripts/discourse/app/components/bookmark-list.js

@@ -4,6 +4,7 @@ import { schedule } from "@ember/runloop";
 import bootbox from "bootbox";
 import discourseDebounce from "discourse-common/lib/debounce";
 import { openBookmarkModal } from "discourse/controllers/bookmark";
+import { ajax } from "discourse/lib/ajax";
 import {
   openLinkInNewTab,
   shouldOpenInNewTab,
@@ -107,6 +108,16 @@ export default Component.extend(Scrolling, {
     });
   },
 
+  @action
+  clearBookmarkReminder(bookmark) {
+    return ajax(`/bookmarks/${bookmark.id}`, {
+      type: "PUT",
+      data: { reminder_at: null },
+    }).then(() => {
+      bookmark.set("reminder_at", null);
+    });
+  },
+
   @action
   togglePinBookmark(bookmark) {
     bookmark.togglePin().then(this.reload);

app/assets/javascripts/discourse/app/controllers/topic.js

@@ -767,6 +767,8 @@ export default Controller.extend(bufferedProperty("model"), {
               post_id: post.id,
               topic_id: post.topic_id,
               for_topic: false,
+              auto_delete_preference: this.currentUser
+                .bookmark_auto_delete_preference,
             }),
           post
         );
@@ -1320,6 +1322,8 @@ export default Controller.extend(bufferedProperty("model"), {
           post_id: firstPost.id,
           topic_id: this.model.id,
           for_topic: true,
+          auto_delete_preference: this.currentUser
+            .bookmark_auto_delete_preference,
         })
       );
     }

app/assets/javascripts/discourse/app/models/bookmark.js

@@ -14,6 +14,7 @@ import { none } from "@ember/object/computed";
 
 export const AUTO_DELETE_PREFERENCES = {
   NEVER: 0,
+  CLEAR_REMINDER: 3,
   WHEN_REMINDER_SENT: 1,
   ON_OWNER_REPLY: 2,
 };
@@ -129,6 +130,11 @@ const Bookmark = RestModel.extend({
     ).capitalize();
   },
 
+  @discourseComputed("reminder_at")
+  reminderAtExpired(bookmarkReminderAt) {
+    return moment(bookmarkReminderAt) < moment();
+  },
+
   @discourseComputed()
   topicForList() {
     // for topic level bookmarks we want to jump to the last unread post URL,

app/assets/javascripts/discourse/app/models/user.js

@@ -98,6 +98,7 @@ let userOptionFields = [
   "timezone",
   "skip_new_user_tips",
   "default_calendar",
+  "bookmark_auto_delete_preference",
 ];
 
 export function addSaveableUserOptionField(fieldName) {

app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs

@@ -17,7 +17,7 @@
               <span class="link-top-line">
                 <div class="bookmark-metadata">
                   {{#if bookmark.reminder_at}}
-                    <span class="bookmark-metadata-item">
+                    <span class="bookmark-metadata-item bookmark-reminder {{if bookmark.reminderAtExpired "bookmark-expired-reminder"}}">
                       {{d-icon "far-clock"}}{{bookmark.formattedReminder}}
                     </span>
                   {{/if}}
@@ -63,6 +63,7 @@
               bookmark=bookmark
               removeBookmark=(action "removeBookmark")
               editBookmark=(action "editBookmark")
+              clearBookmarkReminder=(action "clearBookmarkReminder")
               togglePinBookmark=(action "togglePinBookmark")
               }}
             </td>

app/assets/javascripts/discourse/tests/acceptance/user-bookmarks-test.js

@@ -34,6 +34,10 @@ acceptance("User's bookmarks - reminder", function (needs) {
       json.user_bookmark_list.bookmarks[0].reminder_at = "2028-01-01T08:00";
       return helper.response(json);
     });
+
+    server.put("/bookmarks/:id", () => {
+      return helper.response({});
+    });
   });
 
   test("removing a bookmark with a reminder shows a confirmation", async function (assert) {
@@ -48,6 +52,18 @@ acceptance("User's bookmarks - reminder", function (needs) {
     await click(".bootbox.modal a.btn-primary");
     assert.notOk(exists(".bootbox.modal"));
   });
+
+  test("bookmarks with reminders have a clear reminder option", async function (assert) {
+    await visit("/u/eviltrout/activity/bookmarks");
+
+    assert.ok(exists(".bookmark-reminder"));
+
+    const dropdown = selectKit(".bookmark-actions-dropdown");
+    await dropdown.expand();
+    await dropdown.selectRowByValue("clear_reminder");
+
+    assert.not(exists(".bookmark-reminder"));
+  });
 });
 
 acceptance("User's bookmarks - no bookmarks", function (needs) {

app/assets/stylesheets/common/components/bookmark-list.scss

@@ -38,6 +38,10 @@ $mobile-breakpoint: 700px;
       span {
         word-break: break-word;
       }
+
+      &.bookmark-expired-reminder {
+        color: var(--danger);
+      }
     }
 
     .d-icon {

app/jobs/scheduled/bookmark_reminder_notifications.rb

@@ -18,7 +18,7 @@ module Jobs
     end
 
     def execute(args = nil)
-      bookmarks = Bookmark.pending_reminders.includes(:user).order('reminder_at ASC')
+      bookmarks = Bookmark.pending_reminders.includes(:user).where(reminder_last_sent_at: nil).order('reminder_at ASC')
       bookmarks.limit(BookmarkReminderNotifications.max_reminder_notifications_per_run).each do |bookmark|
         BookmarkReminderNotificationHandler.send_notification(bookmark)
       end

app/models/bookmark.rb

@@ -21,7 +21,8 @@ class Bookmark < ActiveRecord::Base
     @auto_delete_preferences ||= Enum.new(
       never: 0,
       when_reminder_sent: 1,
-      on_owner_reply: 2
+      on_owner_reply: 2,
+      clear_reminder: 3,
     )
   end
 
@@ -33,7 +34,7 @@ class Bookmark < ActiveRecord::Base
     on: [:create, :update],
     if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_for_topic? }
 
-  validate :ensure_sane_reminder_at_time
+  validate :ensure_sane_reminder_at_time, if: :will_save_change_to_reminder_at?
   validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
 
@@ -83,10 +84,6 @@ class Bookmark < ActiveRecord::Base
     @is_for_first_post ||= new_record? ? Post.exists?(id: post_id, post_number: 1) : post.post_number == 1
   end
 
-  def no_reminder?
-    self.reminder_at.blank?
-  end
-
   def auto_delete_when_reminder_sent?
     self.auto_delete_preference == Bookmark.auto_delete_preferences[:when_reminder_sent]
   end
@@ -95,15 +92,18 @@ class Bookmark < ActiveRecord::Base
     self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply]
   end
 
+  def auto_clear_reminder_when_reminder_sent?
+    self.auto_delete_preference == Bookmark.auto_delete_preferences[:clear_reminder]
+  end
+
   def reminder_at_ics(offset: 0)
     (reminder_at + offset).strftime(I18n.t("datetime_formats.formats.calendar_ics"))
   end
 
   def clear_reminder!
     update!(
-      reminder_at: nil,
       reminder_last_sent_at: Time.zone.now,
-      reminder_set_at: nil
+      reminder_set_at: nil,
     )
   end
 

app/models/user_option.rb

@@ -260,6 +260,7 @@ end
 #  color_scheme_id                  :integer
 #  default_calendar                 :integer          default("none_selected"), not null
 #  oldest_search_log_date           :datetime
+#  bookmark_auto_delete_preference  :integer          default(3), not null
 #
 # Indexes
 #

app/serializers/current_user_serializer.rb

@@ -68,6 +68,7 @@ class CurrentUserSerializer < BasicUserSerializer
              :can_review,
              :draft_count,
              :default_calendar,
+             :bookmark_auto_delete_preference,
              :pending_posts_count
 
   delegate :user_stat, to: :object, private: true
@@ -142,6 +143,10 @@ class CurrentUserSerializer < BasicUserSerializer
     object.user_option.default_calendar
   end
 
+  def bookmark_auto_delete_preference
+    object.user_option.bookmark_auto_delete_preference
+  end
+
   def can_send_private_email_messages
     scope.can_send_private_messages_to_email?
   end

config/locales/client.en.yml

@@ -330,10 +330,11 @@ en:
       list_permission_denied: "You do not have permission to view this user's bookmarks."
       no_user_bookmarks: "You have no bookmarked posts; bookmarks allow you to quickly refer to specific posts."
       auto_delete_preference:
-        label: "Automatically delete"
+        label: "After you are notified"
-        never: "Never"
+        never: "Keep bookmark"
-        when_reminder_sent: "Once the reminder is sent"
+        when_reminder_sent: "Delete bookmark"
-        on_owner_reply: "After I reply to this topic"
+        on_owner_reply: "Delete bookmark, once I reply"
+        clear_reminder: "Keep bookmark and clear reminder"
       search_placeholder: "Search bookmarks by name, topic title, or post content"
       search: "Search"
       reminders:
@@ -3268,6 +3269,9 @@ en:
           edit_bookmark:
             name: "Edit bookmark"
             description: "Edit the bookmark name or change the reminder date and time"
+          clear_bookmark_reminder:
+            name: "Clear reminder"
+            description: "Clear the reminder date and time"
           pin_bookmark:
             name: "Pin bookmark"
             description: "Pin the bookmark. This will make it appear at the top of your bookmarks list."

db/migrate/20220209210449_add_bookmark_auto_delete_preference_to_user_option.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddBookmarkAutoDeletePreferenceToUserOption < ActiveRecord::Migration[6.1]
+  def change
+    add_column :user_options, :bookmark_auto_delete_preference, :integer, default: 3, null: false
+  end
+end

lib/bookmark_manager.rb

@@ -65,6 +65,7 @@ class BookmarkManager
     end
 
     update_topic_user_bookmarked(post.topic)
+    update_user_option(bookmark)
 
     bookmark
   end
@@ -101,11 +102,15 @@ class BookmarkManager
   def update(bookmark_id:, name:, reminder_at:, options: {})
     bookmark = find_bookmark_and_check_access(bookmark_id)
 
+    if bookmark.reminder_at != reminder_at
+      bookmark.reminder_at = reminder_at
+      bookmark.reminder_last_sent_at = nil
+    end
+
     success = bookmark.update(
       {
         name: name,
-        reminder_at: reminder_at,
+        reminder_set_at: Time.zone.now,
-        reminder_set_at: Time.zone.now
       }.merge(options)
     )
 
@@ -113,6 +118,8 @@ class BookmarkManager
       return add_errors_from(bookmark)
     end
 
+    update_user_option(bookmark)
+
     success
   end
 
@@ -146,4 +153,8 @@ class BookmarkManager
     TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic)
     bookmarks_remaining_in_topic
   end
+
+  def update_user_option(bookmark)
+    @user.user_option.update!(bookmark_auto_delete_preference: bookmark.auto_delete_preference)
+  end
 end

lib/bookmark_reminder_notification_handler.rb

@@ -26,6 +26,10 @@ class BookmarkReminderNotificationHandler
       "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}"
     )
 
+    if bookmark.auto_clear_reminder_when_reminder_sent?
+      bookmark.reminder_at = nil
+    end
+
     bookmark.clear_reminder!
   end
 

spec/jobs/bookmark_reminder_notifications_spec.rb

@@ -23,16 +23,13 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
     Discourse.redis.flushdb
   end
 
-  it "sends every reminder and marks the reminder_at to nil for all bookmarks, as well as last sent date" do
+  it "sends every reminder and sets the reminder_last_sent_at" do
     subject.execute
     bookmark1.reload
     bookmark2.reload
     bookmark3.reload
-    expect(bookmark1.reminder_at).to eq(nil)
     expect(bookmark1.reminder_last_sent_at).not_to eq(nil)
-    expect(bookmark2.reminder_at).to eq(nil)
     expect(bookmark2.reminder_last_sent_at).not_to eq(nil)
-    expect(bookmark3.reminder_at).to eq(nil)
     expect(bookmark3.reminder_last_sent_at).not_to eq(nil)
   end
 
@@ -60,9 +57,9 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
       begin
         Jobs::BookmarkReminderNotifications.max_reminder_notifications_per_run = 2
         subject.execute
-        expect(bookmark1.reload.reminder_at).to eq(nil)
+        expect(bookmark1.reload.reminder_last_sent_at).not_to eq(nil)
-        expect(bookmark2.reload.reminder_at).to eq(nil)
+        expect(bookmark2.reload.reminder_last_sent_at).not_to eq(nil)
-        expect(bookmark3.reload.reminder_at).not_to eq(nil)
+        expect(bookmark3.reload.reminder_last_sent_at).to eq(nil)
       end
     end
   end

spec/lib/bookmark_manager_spec.rb

@@ -133,6 +133,15 @@ RSpec.describe BookmarkManager do
         expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess)
       end
     end
+
+    it "saves user's preference" do
+      subject.create(post_id: post.id, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent])
+
+      bookmark = Bookmark.find_by(user: user)
+      subject.update(bookmark_id: bookmark, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] })
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+    end
   end
 
   describe ".destroy" do
@@ -191,7 +200,14 @@ RSpec.describe BookmarkManager do
       update_bookmark
       bookmark.reload
       expect(bookmark.name).to eq(new_name)
-      expect(bookmark.reminder_at).to eq_time(new_reminder_at)
+      expect(bookmark.reminder_last_sent_at).to eq(nil)
+    end
+
+    it "does not reminder_last_sent_at if reminder did not change" do
+      bookmark.update(reminder_last_sent_at: 1.day.ago)
+      subject.update(bookmark_id: bookmark.id, name: new_name, reminder_at: bookmark.reminder_at)
+      bookmark.reload
+      expect(bookmark.reminder_last_sent_at).not_to eq(nil)
     end
 
     context "when options are provided" do
@@ -248,11 +264,10 @@ RSpec.describe BookmarkManager do
 
   describe ".send_reminder_notification" do
     let(:bookmark) { Fabricate(:bookmark, user: user) }
-    it "clears the reminder_at and sets the reminder_last_sent_at" do
+    it "sets the reminder_last_sent_at" do
       expect(bookmark.reminder_last_sent_at).to eq(nil)
       described_class.send_reminder_notification(bookmark.id)
       bookmark.reload
-      expect(bookmark.reminder_at).to eq(nil)
       expect(bookmark.reminder_last_sent_at).not_to eq(nil)
     end
 
@@ -276,10 +291,9 @@ RSpec.describe BookmarkManager do
       before do
         bookmark.post.trash!
       end
-      it "does not error, and does not create a notification, and clears the reminder" do
+      it "does not error and does not create a notification" do
         described_class.send_reminder_notification(bookmark.id)
         bookmark.reload
-        expect(bookmark.reminder_at).to eq(nil)
         expect(notifications_for_user.any?).to eq(false)
       end
     end

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -26,20 +26,15 @@ RSpec.describe BookmarkReminderNotificationHandler do
       expect(data["bookmark_name"]).to eq(bookmark.name)
     end
 
-    it "clears the reminder" do
-      subject.send_notification(bookmark)
-      expect(bookmark.reload.no_reminder?).to eq(true)
-    end
-
     context "when the topic is deleted" do
       before do
         bookmark.topic.trash!
         bookmark.reload
       end
 
-      it "does not send a notification and clears the reminder" do
+      it "does not send a notification and updates last notification attempt time" do
         expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
-        expect(bookmark.reload.no_reminder?).to eq(true)
+        expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end
 
@@ -49,9 +44,9 @@ RSpec.describe BookmarkReminderNotificationHandler do
         bookmark.reload
       end
 
-      it "does not send a notification and clears the reminder" do
+      it "does not send a notification and updates last notification attempt time" do
         expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
-        expect(bookmark.reload.no_reminder?).to eq(true)
+        expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end
 
@@ -83,12 +78,24 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
     end
 
+    context "when the auto_delete_preference is clear_reminder" do
+      before do
+        TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
+        bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:clear_reminder])
+      end
+
+      it "resets reminder_at after the reminder gets sent" do
+        subject.send_notification(bookmark)
+        expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil)
+      end
+    end
+
     context "when the post has been deleted" do
-      it "clears the reminder and does not send a notification" do
+      it "does not send a notification" do
         bookmark.post.trash!
         bookmark.reload
-        subject.send_notification(bookmark)
+        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
-        expect(bookmark.reload.no_reminder?).to eq(true)
+        expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end
   end