DEV: Ignore reminder_type for bookmarks (#14349)

We don't actually use the reminder_type for bookmarks anywhere;
we are just storing it. It has no bearing on the UI. It used
to be relevant with the at_desktop bookmark reminders (see
fa572d3a7a)

This commit marks the column as readonly, ignores it, and removes
the index, and it will be dropped in a later PR. Some plugins
are relying on reminder_type partially so some stubs have been
left in place to avoid errors.

app/assets/javascripts/discourse/app/lib/transform-post.js

@@ -39,7 +39,6 @@ export function transformBasicPost(post) {
     bookmarked: post.bookmarked,
     bookmarkReminderAt: post.bookmark_reminder_at,
     bookmarkName: post.bookmark_name,
-    bookmarkReminderType: post.bookmark_reminder_type,
     yours: post.yours,
     shareUrl: post.get("shareUrl"),
     staff: post.staff,

app/controllers/bookmarks_controller.rb

@@ -14,7 +14,6 @@ class BookmarksController < ApplicationController
     bookmark = bookmark_manager.create(
       post_id: params[:post_id],
       name: params[:name],
-      reminder_type: params[:reminder_type],
       reminder_at: params[:reminder_at],
       options: {
         auto_delete_preference: params[:auto_delete_preference] || 0
@@ -41,7 +40,6 @@ class BookmarksController < ApplicationController
     bookmark_manager.update(
       bookmark_id: params[:id],
       name: params[:name],
-      reminder_type: params[:reminder_type],
       reminder_at: params[:reminder_at],
       options: {
         auto_delete_preference: params[:auto_delete_preference] || 0

app/jobs/regular/export_user_archive.rb

@@ -31,7 +31,7 @@ module Jobs
       auth_tokens: ['id', 'auth_token_hash', 'prev_auth_token_hash', 'auth_token_seen', 'client_ip', 'user_agent', 'seen_at', 'rotated_at', 'created_at', 'updated_at'],
       auth_token_logs: ['id', 'action', 'user_auth_token_id', 'client_ip', 'auth_token_hash', 'created_at', 'path', 'user_agent'],
       badges: ['badge_id', 'badge_name', 'granted_at', 'post_id', 'seq', 'granted_manually', 'notification_id', 'featured_rank'],
-      bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_type', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
+      bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
       category_preferences: ['category_id', 'category_names', 'notification_level', 'dismiss_new_timestamp'],
       flags: ['id', 'post_id', 'flag_type', 'created_at', 'updated_at', 'deleted_at', 'deleted_by', 'related_post_id', 'targets_topic', 'was_take_action'],
       likes: ['id', 'post_id', 'topic_id', 'post_number', 'created_at', 'updated_at', 'deleted_at', 'deleted_by'],
@@ -248,7 +248,6 @@ module Jobs
           bkmk.name,
           bkmk.created_at,
           bkmk.updated_at,
-          Bookmark.reminder_types[bkmk.reminder_type],
           bkmk.reminder_at,
           bkmk.reminder_last_sent_at,
           bkmk.reminder_set_at,

app/models/bookmark.rb

@@ -2,7 +2,8 @@
 
 class Bookmark < ActiveRecord::Base
   self.ignored_columns = [
-    "topic_id" # TODO (martin) (2021-12-01): remove
+    "topic_id", # TODO (martin) (2021-12-01): remove
+    "reminder_type" # TODO (martin) (2021-12-01): remove
   ]
 
   belongs_to :user
@@ -11,6 +12,15 @@ class Bookmark < ActiveRecord::Base
 
   delegate :topic_id, to: :post
 
+  def self.auto_delete_preferences
+    @auto_delete_preferences ||= Enum.new(
+      never: 0,
+      when_reminder_sent: 1,
+      on_owner_reply: 2
+    )
+  end
+
+  # TODO (martin) (2021-12-01) Remove this once plugins are not using it.
   def self.reminder_types
     @reminder_types ||= Enum.new(
       later_today: 1,
@@ -24,19 +34,6 @@ class Bookmark < ActiveRecord::Base
     )
   end
 
-  def self.auto_delete_preferences
-    @auto_delete_preferences ||= Enum.new(
-      never: 0,
-      when_reminder_sent: 1,
-      on_owner_reply: 2
-    )
-  end
-
-  validates :reminder_at, presence: {
-    message: I18n.t("bookmarks.errors.time_must_be_provided"),
-    if: -> { reminder_type.present? }
-  }
-
   validate :unique_per_post_for_user,
     on: [:create, :update],
     if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? }
@@ -83,7 +80,7 @@ class Bookmark < ActiveRecord::Base
   end
 
   def no_reminder?
-    self.reminder_at.blank? && self.reminder_type.blank?
+    self.reminder_at.blank?
   end
 
   def auto_delete_when_reminder_sent?
@@ -101,7 +98,6 @@ class Bookmark < ActiveRecord::Base
   def clear_reminder!
     update!(
       reminder_at: nil,
-      reminder_type: nil,
       reminder_last_sent_at: Time.zone.now,
       reminder_set_at: nil
     )
@@ -173,7 +169,6 @@ end
 #  user_id                :bigint           not null
 #  post_id                :bigint           not null
 #  name                   :string(100)
-#  reminder_type          :integer
 #  reminder_at            :datetime
 #  created_at             :datetime         not null
 #  updated_at             :datetime         not null
@@ -188,7 +183,6 @@ end
 #  index_bookmarks_on_post_id                            (post_id)
 #  index_bookmarks_on_reminder_at                        (reminder_at)
 #  index_bookmarks_on_reminder_set_at                    (reminder_set_at)
-#  index_bookmarks_on_reminder_type                      (reminder_type)
 #  index_bookmarks_on_topic_id                           (topic_id)
 #  index_bookmarks_on_user_id                            (user_id)
 #  index_bookmarks_on_user_id_and_post_id_and_for_topic  (user_id,post_id,for_topic) UNIQUE

app/serializers/post_serializer.rb

@@ -53,7 +53,6 @@ class PostSerializer < BasicPostSerializer
              :bookmarked,
              :bookmark_reminder_at,
              :bookmark_id,
-             :bookmark_reminder_type,
              :bookmark_name,
              :bookmark_auto_delete_preference,
              :raw,
@@ -346,10 +345,6 @@ class PostSerializer < BasicPostSerializer
     bookmarked
   end
 
-  def include_bookmark_reminder_type?
-    bookmarked
-  end
-
   def include_bookmark_name?
     bookmarked
   end
@@ -374,11 +369,6 @@ class PostSerializer < BasicPostSerializer
     post_bookmark&.reminder_at
   end
 
-  def bookmark_reminder_type
-    return if post_bookmark.blank?
-    Bookmark.reminder_types[post_bookmark.reminder_type].to_s
-  end
-
   def bookmark_name
     post_bookmark&.name
   end

db/migrate/20210913032326_add_for_topic_to_bookmarks.rb

@@ -4,6 +4,8 @@ class AddForTopicToBookmarks < ActiveRecord::Migration[6.1]
   def change
     add_column :bookmarks, :for_topic, :boolean, default: false, null: false
     add_index :bookmarks, [:user_id, :post_id, :for_topic], unique: true
-    remove_index :bookmarks, [:user_id, :post_id]
+    if index_exists?(:bookmarks, [:user_id, :post_id])
+      remove_index :bookmarks, [:user_id, :post_id]
+    end
   end
 end

db/migrate/20210915215952_mark_reminder_type_as_readonly_for_bookmarks.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class MarkReminderTypeAsReadonlyForBookmarks < ActiveRecord::Migration[6.1]
+  def up
+    Migration::ColumnDropper.mark_readonly(:bookmarks, :reminder_type)
+  end
+
+  def down
+    Migration::ColumnDropper.drop_readonly(:bookmarks, :reminder_type)
+  end
+end

db/migrate/20210915222124_drop_reminder_type_index_for_bookmarks.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class DropReminderTypeIndexForBookmarks < ActiveRecord::Migration[6.1]
+  def up
+    if index_exists?(:bookmarks, [:reminder_type])
+      remove_index :bookmarks, [:reminder_type]
+    end
+  end
+
+  def down
+    if !index_exists?(:bookmarks, [:reminder_type])
+      add_index :bookmarks, :reminder_type
+    end
+  end
+end

lib/bookmark_manager.rb

@@ -7,9 +7,9 @@ class BookmarkManager
     @user = user
   end
 
-  def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil, options: {})
+  # TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it.
+  def create(post_id:, name: nil, reminder_at: nil, reminder_type: nil, options: {})
     post = Post.find_by(id: post_id)
-    reminder_type = parse_reminder_type(reminder_type)
 
     # no bookmarking deleted posts or topics
     raise Discourse::InvalidAccess if post.blank? || post.topic.blank?
@@ -23,7 +23,6 @@ class BookmarkManager
         user_id: @user.id,
         post: post,
         name: name,
-        reminder_type: reminder_type,
         reminder_at: reminder_at,
         reminder_set_at: Time.zone.now
       }.merge(options)
@@ -67,16 +66,14 @@ class BookmarkManager
     BookmarkReminderNotificationHandler.send_notification(bookmark)
   end
 
-  def update(bookmark_id:, name:, reminder_type:, reminder_at:, options: {})
+  # TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it.
+  def update(bookmark_id:, name:, reminder_at:, reminder_type: nil, options: {})
     bookmark = find_bookmark_and_check_access(bookmark_id)
 
-    reminder_type = parse_reminder_type(reminder_type)
-
     success = bookmark.update(
       {
         name: name,
         reminder_at: reminder_at,
-        reminder_type: reminder_type,
         reminder_set_at: Time.zone.now
       }.merge(options)
     )
@@ -118,9 +115,4 @@ class BookmarkManager
     TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic)
     bookmarks_remaining_in_topic
   end
-
-  def parse_reminder_type(reminder_type)
-    return if reminder_type.blank?
-    reminder_type.is_a?(Integer) ? reminder_type : Bookmark.reminder_types[reminder_type.to_sym]
-  end
 end

lib/bookmark_reminder_notification_handler.rb

@@ -23,7 +23,7 @@ class BookmarkReminderNotificationHandler
 
   def self.clear_reminder(bookmark)
     Rails.logger.debug(
-      "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder info: #{bookmark.reminder_at} | #{Bookmark.reminder_types[bookmark.reminder_type]}"
+      "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}"
     )
 
     bookmark.clear_reminder!

spec/fabricators/bookmark_fabricator.rb

@@ -4,13 +4,11 @@ Fabricator(:bookmark) do
   user
   post { Fabricate(:post) }
   name "This looked interesting"
-  reminder_type { Bookmark.reminder_types[:tomorrow] }
   reminder_at { 1.day.from_now.iso8601 }
   reminder_set_at { Time.zone.now }
 end
 
 Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do
-  reminder_type { Bookmark.reminder_types[:next_business_day] }
   reminder_at do
     date = if Time.zone.now.friday?
       Time.zone.now + 3.days

spec/jobs/export_user_archive_spec.rb

@@ -264,7 +264,6 @@ describe Jobs::ExportUserArchive do
     let(:post3) { Fabricate(:post) }
     let(:message) { Fabricate(:private_message_topic) }
     let(:post4) { Fabricate(:post, topic: message) }
-    let(:reminder_type) { Bookmark.reminder_types[:tomorrow] }
     let(:reminder_at) { 1.day.from_now }
 
     it 'properly includes bookmark records' do
@@ -274,9 +273,9 @@ describe Jobs::ExportUserArchive do
       update1_at = now + 1.hours
       bkmk1.update(name: 'great food recipe', updated_at: update1_at)
 
-      manager.create(post_id: post2.id, name: name, reminder_type: :tomorrow, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+      manager.create(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
       twelve_hr_ago = freeze_time now - 12.hours
-      pending_reminder = manager.create(post_id: post3.id, name: name, reminder_type: :later_today, reminder_at: now - 8.hours)
+      pending_reminder = manager.create(post_id: post3.id, name: name, reminder_at: now - 8.hours)
       freeze_time now
 
       tau_record = message.topic_allowed_users.create!(user_id: user.id)
@@ -296,7 +295,6 @@ describe Jobs::ExportUserArchive do
       expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s))
 
       expect(data[1]['name']).to eq(name)
-      expect(data[1]['reminder_type']).to eq('tomorrow')
       expect(DateTime.parse(data[1]['reminder_at'])).to eq(DateTime.parse(reminder_at.to_s))
       expect(data[1]['auto_delete_preference']).to eq('when_reminder_sent')
 

spec/lib/bookmark_manager_spec.rb

@@ -5,7 +5,6 @@ require 'rails_helper'
 RSpec.describe BookmarkManager do
   let(:user) { Fabricate(:user) }
 
-  let(:reminder_type) { 'tomorrow' }
   let(:reminder_at) { 1.day.from_now }
   fab!(:post) { Fabricate(:post) }
   let(:name) { 'Check this out!' }
@@ -32,7 +31,7 @@ RSpec.describe BookmarkManager do
     end
 
     it "updates the topic user bookmarked column to true if any post is bookmarked" do
-      subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+      subject.create(post_id: post.id, name: name, reminder_at: reminder_at)
       tu = TopicUser.find_by(user: user)
       expect(tu.bookmarked).to eq(true)
       tu.update(bookmarked: false)
@@ -41,14 +40,13 @@ RSpec.describe BookmarkManager do
       expect(tu.bookmarked).to eq(true)
     end
 
-    context "when a reminder time + type is provided" do
+    context "when a reminder time is provided" do
       it "saves the values correctly" do
-        subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+        subject.create(post_id: post.id, name: name, reminder_at: reminder_at)
         bookmark = Bookmark.find_by(user: user)
 
         expect(bookmark.reminder_at).to eq_time(reminder_at)
         expect(bookmark.reminder_set_at).not_to eq(nil)
-        expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:tomorrow])
       end
     end
 
@@ -81,21 +79,11 @@ RSpec.describe BookmarkManager do
       end
     end
 
-    context "when the reminder time is not provided when it needs to be" do
-      let(:reminder_at) { nil }
-      it "adds an error to the manager" do
-        subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
-        expect(subject.errors.full_messages).to include(
-          "Reminder at " + I18n.t("bookmarks.errors.time_must_be_provided")
-        )
-      end
-    end
-
     context "when the reminder time is in the past" do
       let(:reminder_at) { 10.days.ago }
 
       it "adds an error to the manager" do
-        subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+        subject.create(post_id: post.id, name: name, reminder_at: reminder_at)
         expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder"))
       end
     end
@@ -104,7 +92,7 @@ RSpec.describe BookmarkManager do
       let(:reminder_at) { 11.years.from_now }
 
       it "adds an error to the manager" do
-        subject.create(post_id: post.id, name: name, reminder_type: reminder_type, reminder_at: reminder_at)
+        subject.create(post_id: post.id, name: name, reminder_at: reminder_at)
         expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future"))
       end
     end
@@ -169,25 +157,22 @@ RSpec.describe BookmarkManager do
     let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, post: post, name: "Old name") }
     let(:new_name) { "Some new name" }
     let(:new_reminder_at) { 10.days.from_now }
-    let(:new_reminder_type) { Bookmark.reminder_types[:custom] }
     let(:options) { {} }
 
     def update_bookmark
       subject.update(
         bookmark_id: bookmark.id,
         name: new_name,
-        reminder_type: new_reminder_type,
         reminder_at: new_reminder_at,
         options: options
       )
     end
 
-    it "saves the time and new reminder type and new name successfully" do
+    it "saves the time and new name successfully" 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_type).to eq(new_reminder_type)
     end
 
     context "when options are provided" do
@@ -200,15 +185,6 @@ RSpec.describe BookmarkManager do
       end
     end
 
-    context "if the new reminder type is a string" do
-      let(:new_reminder_type) { "custom" }
-      it "is parsed" do
-        update_bookmark
-        bookmark.reload
-        expect(bookmark.reminder_type).to eq(Bookmark.reminder_types[:custom])
-      end
-    end
-
     context "if the bookmark is belonging to some other user" do
       let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
       it "raises an invalid access error" do

spec/lib/bookmark_query_spec.rb

@@ -168,11 +168,11 @@ RSpec.describe BookmarkQuery do
   end
 
   describe "#list_all ordering" do
-    let!(:bookmark1) { Fabricate(:bookmark, user: user, updated_at: 1.day.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark1) { Fabricate(:bookmark, user: user, updated_at: 1.day.ago, reminder_at: nil) }
-    let!(:bookmark2) { Fabricate(:bookmark, user: user, updated_at: 2.days.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark2) { Fabricate(:bookmark, user: user, updated_at: 2.days.ago, reminder_at: nil) }
-    let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark3) { Fabricate(:bookmark, user: user, updated_at: 6.days.ago, reminder_at: nil) }
-    let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark4) { Fabricate(:bookmark, user: user, updated_at: 4.days.ago, reminder_at: nil) }
-    let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_type: nil, reminder_at: nil) }
+    let!(:bookmark5) { Fabricate(:bookmark, user: user, updated_at: 3.days.ago, reminder_at: nil) }
 
     it "order defaults to updated_at DESC" do
       expect(bookmark_query.list_all.map(&:id)).to eq([
@@ -185,9 +185,7 @@ RSpec.describe BookmarkQuery do
     end
 
     it "orders by reminder_at, then updated_at" do
-      bookmark4.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow])
       bookmark4.update_column(:reminder_at, 1.day.from_now)
-      bookmark5.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow])
       bookmark5.update_column(:reminder_at, 26.hours.from_now)
 
       expect(bookmark_query.list_all.map(&:id)).to eq([
@@ -202,17 +200,14 @@ RSpec.describe BookmarkQuery do
 
     it "shows pinned bookmarks first ordered by reminder_at ASC then updated_at DESC" do
       bookmark3.update_column(:pinned, true)
-      bookmark3.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow])
       bookmark3.update_column(:reminder_at, 1.day.from_now)
 
       bookmark4.update_column(:pinned, true)
-      bookmark4.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow])
       bookmark4.update_column(:reminder_at, 28.hours.from_now)
 
       bookmark1.update_column(:pinned, true)
       bookmark2.update_column(:pinned, true)
 
-      bookmark5.update_column(:reminder_type, Bookmark.reminder_types[:tomorrow])
       bookmark5.update_column(:reminder_at, 1.day.from_now)
 
       expect(bookmark_query.list_all.map(&:id)).to eq([

spec/requests/bookmarks_controller_spec.rb

@@ -19,7 +19,6 @@ describe BookmarksController do
 
       post "/bookmarks.json", params: {
         post_id: bookmark_post.id,
-        reminder_type: "tomorrow",
         reminder_at: (Time.zone.now + 1.day).iso8601
       }
 
@@ -39,7 +38,6 @@ describe BookmarksController do
       it "returns failed JSON with a 400 error" do
         post "/bookmarks.json", params: {
           post_id: bookmark_post.id,
-          reminder_type: "tomorrow",
           reminder_at: (Time.zone.now + 1.day).iso8601
         }
         post "/bookmarks.json", params: {
@@ -62,7 +60,6 @@ describe BookmarksController do
       it "returns failed JSON with a 400 error" do
         post "/bookmarks.json", params: {
           post_id: bookmark_post.id,
-          reminder_type: "tomorrow",
           reminder_at: (Time.zone.now + 1.day).iso8601
         }
 
@@ -72,20 +69,6 @@ describe BookmarksController do
         )
       end
     end
-
-    context "if the user provides a reminder type that needs a reminder_at that is missing" do
-      it "returns failed JSON with a 400 error" do
-        post "/bookmarks.json", params: {
-          post_id: bookmark_post.id,
-          reminder_type: "tomorrow"
-        }
-
-        expect(response.status).to eq(400)
-        expect(response.parsed_body['errors'].first).to include(
-          I18n.t("bookmarks.errors.time_must_be_provided")
-        )
-      end
-    end
   end
 
   describe "#destroy" do