FEATURE: Remove user topic timers and migrate to bookmarks with reminders (#10474)

This PR removes the user reminder topic timers, because that system has been supplanted and improved by bookmark reminders. The option is removed from the UI and all existing user reminder topic timers are migrated to bookmark reminders.

Migration does this:

* Get all topic_timers with status_type 5 (reminders)
* Gets all bookmarks where the user ID and topic ID match
* Loops through the found topic timers
  * If there is no bookmark for the OP of the topic, then we just create a bookmark with a reminder
  * If there is a bookmark for the OP of the topic and it does **not** have a reminder set, then just 
update it with the topic timer reminder
  * If there is a bookmark for the OP of the topic with a reminder then just discard the topic timer
* Cancels all outstanding user reminder topic timers
* **Trashes (not deletes) all user reminder topic timers**

Notes:

* For now I have left the user reminder topic timer job class in place; this is so the jobs can be cancelled in the migration. It and the specs will be deleted in the next PR.
* At a later date I will write a migration to delete all trashed user topic timers. They are not deleted here in case there are data issues and they need to be recovered.
* A future PR will change the UI of the topic timer modal to make it look more like the bookmark modal.

app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js

@@ -6,7 +6,6 @@ import {
   PUBLISH_TO_CATEGORY_STATUS_TYPE,
   OPEN_STATUS_TYPE,
   DELETE_STATUS_TYPE,
-  REMINDER_TYPE,
   CLOSE_STATUS_TYPE,
   BUMP_TYPE,
   DELETE_REPLIES_TYPE,
@@ -19,9 +18,8 @@ export default Component.extend({
   autoDelete: equal("selection", DELETE_STATUS_TYPE),
   autoBump: equal("selection", BUMP_TYPE),
   publishToCategory: equal("selection", PUBLISH_TO_CATEGORY_STATUS_TYPE),
-  reminder: equal("selection", REMINDER_TYPE),
   autoDeleteReplies: equal("selection", DELETE_REPLIES_TYPE),
-  showTimeOnly: or("autoOpen", "autoDelete", "reminder", "autoBump"),
+  showTimeOnly: or("autoOpen", "autoDelete", "autoBump"),
   showFutureDateInput: or(
     "showTimeOnly",
     "publishToCategory",

app/assets/javascripts/discourse/app/components/topic-timer-info.js

@@ -4,10 +4,7 @@ import { cancel, later } from "@ember/runloop";
 import Component from "@ember/component";
 import { iconHTML } from "discourse-common/lib/icon-library";
 import Category from "discourse/models/category";
-import {
+import { DELETE_REPLIES_TYPE } from "discourse/controllers/edit-topic-timer";
-  REMINDER_TYPE,
-  DELETE_REPLIES_TYPE,
-} from "discourse/controllers/edit-topic-timer";
 import { isTesting } from "discourse-common/config/environment";
 
 export default Component.extend({
@@ -20,9 +17,8 @@ export default Component.extend({
   notice: null,
   showTopicTimer: null,
 
-  @discourseComputed("statusType")
+  @discourseComputed
-  canRemoveTimer(type) {
+  canRemoveTimer() {
-    if (type === REMINDER_TYPE) return true;
     return this.currentUser && this.currentUser.get("canManageTopic");
   },
 

app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js

@@ -1,4 +1,5 @@
 import I18n from "I18n";
+import { alias } from "@ember/object/computed";
 import EmberObject, { setProperties } from "@ember/object";
 import Controller from "@ember/controller";
 import discourseComputed from "discourse-common/utils/decorators";
@@ -11,7 +12,6 @@ export const CLOSE_STATUS_TYPE = "close";
 export const OPEN_STATUS_TYPE = "open";
 export const PUBLISH_TO_CATEGORY_STATUS_TYPE = "publish_to_category";
 export const DELETE_STATUS_TYPE = "delete";
-export const REMINDER_TYPE = "reminder";
 export const BUMP_TYPE = "bump";
 export const DELETE_REPLIES_TYPE = "delete_replies";
 
@@ -58,24 +58,7 @@ export default Controller.extend(ModalFunctionality, {
     return types;
   },
 
-  @discourseComputed()
+  topicTimer: alias("model.topic_timer"),
-  privateTimerTypes() {
-    return [{ id: REMINDER_TYPE, name: I18n.t("topic.reminder.title") }];
-  },
-
-  @discourseComputed("isPublic", "publicTimerTypes", "privateTimerTypes")
-  selections(isPublic, publicTimerTypes, privateTimerTypes) {
-    return "true" === isPublic ? publicTimerTypes : privateTimerTypes;
-  },
-
-  @discourseComputed(
-    "isPublic",
-    "model.topic_timer",
-    "model.private_topic_timer"
-  )
-  topicTimer(isPublic, publicTopicTimer, privateTopicTimer) {
-    return "true" === isPublic ? publicTopicTimer : privateTopicTimer;
-  },
 
   _setTimer(time, duration, statusType, basedOnLastPost, categoryId) {
     this.set("loading", true);
@@ -100,9 +83,7 @@ export default Controller.extend(ModalFunctionality, {
 
           this.set("model.closed", result.closed);
         } else {
-          const topicTimer =
+          this.set("model.topic_timer", EmberObject.create({}));
-            this.isPublic === "true" ? "topic_timer" : "private_topic_timer";
-          this.set(`model.${topicTimer}`, EmberObject.create({}));
 
           this.setProperties({
             selection: null,

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

@@ -114,11 +114,6 @@ const TopicRoute = DiscourseRoute.extend({
         model.set("topic_timer", {});
       }
 
-      const privateTopicTimer = model.get("private_topic_timer");
-      if (!privateTopicTimer) {
-        model.set("private_topic_timer", {});
-      }
-
       showModal("edit-topic-timer", { model });
       this.controllerFor("modal").set("modalClass", "edit-topic-timer-modal");
     },

app/assets/javascripts/discourse/app/templates/modal/edit-topic-timer.hbs

@@ -1,20 +1,8 @@
 {{#d-modal-body title="topic.topic_status_update.title" autoFocus="false"}}
-  <div class="radios">
-    <label for="public-topic-timer">
-      {{radio-button id="public-topic-timer" name="topic-timer" value="true" selection=isPublic}}
-      <b>{{i18n "topic.topic_status_update.public_timer_types"}}</b>
-    </label>
-
-    <label for="private-topic-timer">
-      {{radio-button id="private-topic-timer" name="topic-timer" value="false" selection=isPublic}}
-      <b>{{i18n "topic.topic_status_update.private_timer_types"}}</b>
-    </label>
-  </div>
-
   {{edit-topic-timer-form
     topic=model
     topicTimer=topicTimer
-    timerTypes=selections
+    timerTypes=publicTimerTypes
     updateTime=updateTime
     onChangeStatusType=(action "onChangeStatusType")
     onChangeInput=(action "onChangeInput")

app/assets/javascripts/discourse/app/templates/topic.hbs

@@ -286,15 +286,6 @@
                 </div>
               {{/if}}
 
-              {{#if model.private_topic_timer.execute_at}}
-                {{topic-timer-info
-                    topicClosed=model.closed
-                    statusType=model.private_topic_timer.status_type
-                    executeAt=model.private_topic_timer.execute_at
-                    duration=model.private_topic_timer.duration
-                    removeTopicTimer=(action "removeTopicTimer" model.private_topic_timer.status_type "private_topic_timer")}}
-              {{/if}}
-
               {{topic-timer-info
                   topicClosed=model.closed
                   statusType=model.topic_timer.status_type

app/jobs/regular/topic_reminder.rb

@@ -4,6 +4,9 @@ module Jobs
   class TopicReminder < ::Jobs::Base
 
     def execute(args)
+      # noop, TODO(martin 2021-03-11): Remove this after timers migrated and outstanding jobs cancelled
+      return
+
       topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
 
       topic = topic_timer&.topic

app/models/topic.rb

@@ -554,7 +554,6 @@ class Topic < ActiveRecord::Base
   def reload(options = nil)
     @post_numbers = nil
     @public_topic_timer = nil
-    @private_topic_timer = nil
     @is_category_topic = nil
     super(options)
   end
@@ -1309,10 +1308,6 @@ class Topic < ActiveRecord::Base
     @public_topic_timer ||= topic_timers.find_by(deleted_at: nil, public_type: true)
   end
 
-  def private_topic_timer(user)
-    @private_topic_Timer ||= topic_timers.find_by(deleted_at: nil, public_type: false, user_id: user.id)
-  end
-
   def delete_topic_timer(status_type, by_user: Discourse.system_user)
     options = { status_type: status_type }
     options.merge!(user: by_user) unless TopicTimer.public_types[status_type]

app/models/topic_timer.rb

@@ -156,7 +156,7 @@ class TopicTimer < ActiveRecord::Base
   end
 
   def schedule_auto_reminder_job(time)
-    Jobs.enqueue_at(time, :topic_reminder, topic_timer_id: id)
+    # noop, TODO(martin 2021-03-11): Remove this after timers migrated and outstanding jobs cancelled
   end
 end
 

app/serializers/topic_view_serializer.rb

@@ -64,7 +64,6 @@ class TopicViewSerializer < ApplicationSerializer
     :bookmark_reminder_at,
     :message_archived,
     :topic_timer,
-    :private_topic_timer,
     :unicode_title,
     :message_bus_last_id,
     :participant_count,
@@ -202,15 +201,6 @@ class TopicViewSerializer < ApplicationSerializer
     TopicTimerSerializer.new(object.topic.public_topic_timer, root: false)
   end
 
-  def include_private_topic_timer?
-    scope.user
-  end
-
-  def private_topic_timer
-    timer = object.topic.private_topic_timer(scope.user)
-    TopicTimerSerializer.new(timer, root: false)
-  end
-
   def include_featured_link?
     SiteSetting.topic_featured_link_enabled
   end

app/serializers/web_hook_topic_view_serializer.rb

@@ -18,7 +18,6 @@ class WebHookTopicViewSerializer < TopicViewSerializer
     current_post_number
     chunk_size
     topic_timer
-    private_topic_timer
     details
     image_url
   }.each do |attr|

config/locales/client.en.yml

@@ -2312,8 +2312,6 @@ en:
         remove: "Remove Timer"
         publish_to: "Publish To:"
         when: "When:"
-        public_timer_types: Topic Timers
-        private_timer_types: User Topic Timers
         time_frame_required: Please select a time frame
       auto_update_input:
         none: "Select a timeframe"

db/migrate/20200819030609_migrate_user_topic_timers_to_bookmark_reminders.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+class MigrateUserTopicTimersToBookmarkReminders < ActiveRecord::Migration[6.0]
+  def up
+    topic_timers_to_migrate = DB.query(<<~SQL, reminder_type: 5)
+      SELECT topic_timers.id, topic_timers.topic_id, topic_timers.user_id, execute_at,
+      posts.id AS first_post_id
+      FROM topic_timers
+      INNER JOIN topics ON topics.id = topic_timers.topic_id
+      INNER JOIN posts ON posts.topic_id = topics.id AND posts.post_number = 1
+      WHERE topics.deleted_at IS NULL
+      AND topic_timers.deleted_at IS NULL
+      AND topic_timers.status_type = :reminder_type
+    SQL
+
+    return if topic_timers_to_migrate.empty?
+
+    topic_timer_tuples = topic_timers_to_migrate.map do |tt|
+      "(#{tt.user_id}, #{tt.topic_id})"
+    end.join(", ")
+
+    existing_bookmarks = DB.query(<<~SQL)
+      SELECT bookmarks.id, reminder_at, reminder_type,
+             bookmarks.topic_id, post_id, bookmarks.user_id, posts.post_number
+      FROM bookmarks
+      INNER JOIN posts ON posts.id = bookmarks.post_id
+      WHERE (bookmarks.user_id, bookmarks.topic_id) IN (#{topic_timer_tuples})
+    SQL
+
+    new_bookmarks = []
+    topic_timers_to_migrate.each do |tt|
+      bookmark = existing_bookmarks.find do |bm|
+
+        # we only care about existing topic-level bookmarks here
+        # because topic timers are (funnily enough) topic-level
+        bm.topic_id == tt.topic_id && bm.user_id == tt.user_id && bm.post_number == 1
+      end
+
+      if !bookmark
+        # create one
+        now = Time.zone.now
+        new_bookmarks << "(#{tt.user_id}, #{tt.topic_id}, #{tt.first_post_id}, '#{tt.execute_at}', 6, '#{now}', '#{now}')"
+      else
+        if !bookmark.reminder_at
+          DB.exec(
+            "UPDATE bookmarks SET reminder_at = :reminder_at, reminder_type = 6 WHERE id = :bookmark_id",
+            reminder_at: tt.execute_at,
+            bookmark_id: bookmark.id
+          )
+        end
+
+        # if there is a bookmark with reminder already do nothing,
+        # we do not need to migrate the topic timer because it would
+        # cause a conflict
+      end
+    end
+
+    DB.exec(<<~SQL)
+    INSERT INTO bookmarks(user_id, topic_id, post_id, reminder_at, reminder_type, created_at, updated_at)
+    VALUES #{new_bookmarks.join(",\n")}
+    ON CONFLICT DO NOTHING
+    SQL
+
+    # TODO(2021-01-07): delete leftover trashed records
+    # trash these so the records are kept around for any possible data issues,
+    # they can be deleted in a few months
+    topic_timers_to_migrate_ids = topic_timers_to_migrate.map(&:id)
+    DB.exec(
+      "UPDATE topic_timers SET deleted_at = :deleted_at, deleted_by_id = :deleted_by WHERE ID IN (:ids)",
+      ids: topic_timers_to_migrate_ids,
+      deleted_at: Time.zone.now,
+      deleted_by: Discourse.system_user
+    )
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6

@@ -202,7 +202,6 @@ acceptance("Poll quote", {
             chunk_size: 20,
             bookmarked: false,
             topic_timer: null,
-            private_topic_timer: null,
             message_bus_last_id: 1,
             participant_count: 1,
             queued_posts_count: 0,
@@ -627,7 +626,6 @@ acceptance("Poll quote", {
             chunk_size: 20,
             bookmarked: false,
             topic_timer: null,
-            private_topic_timer: null,
             message_bus_last_id: 2,
             participant_count: 1,
             queued_posts_count: 0,

spec/jobs/topic_reminder_spec.rb

@@ -1,59 +0,0 @@
-# frozen_string_literal: true
-
-require 'rails_helper'
-
-describe Jobs::TopicReminder do
-  fab!(:admin) { Fabricate(:admin) }
-
-  fab!(:topic) do
-    Fabricate(:topic_timer,
-      user: admin,
-      status_type: TopicTimer.types[:reminder]
-    ).topic
-  end
-
-  it "should be able to create a reminder" do
-    topic_timer = topic.topic_timers.first
-    freeze_time 1.day.from_now
-
-    expect {
-      described_class.new.execute(topic_timer_id: topic_timer.id)
-    }.to change { Notification.count }.by(1)
-    expect(admin.notifications.where(notification_type: Notification.types[:topic_reminder]).first&.topic_id).to eq(topic.id)
-    expect(TopicTimer.where(id: topic_timer.id).first).to be_nil
-  end
-
-  it "does nothing if it was trashed before the scheduled time" do
-    topic_timer = topic.topic_timers.first
-    topic_timer.trash!(Discourse.system_user)
-
-    freeze_time(1.day.from_now)
-
-    expect {
-      described_class.new.execute(topic_timer_id: topic_timer.id)
-    }.to_not change { Notification.count }
-  end
-
-  it "does nothing if job runs too early" do
-    topic_timer = topic.topic_timers.first
-    topic_timer.update_attribute(:execute_at, 8.hours.from_now)
-
-    freeze_time(6.hours.from_now)
-
-    expect {
-      described_class.new.execute(topic_timer_id: topic_timer.id)
-    }.to_not change { Notification.count }
-  end
-
-  it "does nothing if topic was deleted" do
-    topic_timer = topic.topic_timers.first
-    topic.trash!
-
-    freeze_time(1.day.from_now)
-
-    expect {
-      described_class.new.execute(topic_timer_id: topic_timer.id)
-    }.to_not change { Notification.count }
-  end
-
-end

spec/models/topic_spec.rb

@@ -1711,20 +1711,6 @@ describe Topic do
     end
   end
 
-  describe '#private_topic_timer' do
-    let(:topic_timer) do
-      Fabricate(:topic_timer,
-        public_type: false,
-        user: user,
-        status_type: TopicTimer.private_types[:reminder]
-      )
-    end
-
-    it 'should return the right record' do
-      expect(topic_timer.topic.private_topic_timer(user)).to eq(topic_timer)
-    end
-  end
-
   describe '#set_or_create_timer' do
     let(:topic) { Fabricate.build(:topic) }
 
@@ -1875,38 +1861,6 @@ describe Topic do
         expect(topic.reload.closed).to eq(true)
       end
     end
-
-    describe "private status type" do
-      fab!(:topic) { Fabricate(:topic) }
-      let(:reminder) { Fabricate(:topic_timer, user: admin, topic: topic, status_type: TopicTimer.types[:reminder]) }
-      fab!(:other_admin) { Fabricate(:admin) }
-
-      it "lets two users have their own record" do
-        reminder
-        expect {
-          topic.set_or_create_timer(TopicTimer.types[:reminder], 2, by_user: other_admin)
-        }.to change { TopicTimer.count }.by(1)
-      end
-
-      it 'should not be override when setting a public topic timer' do
-        reminder
-
-        expect do
-          topic.set_or_create_timer(TopicTimer.types[:close], 3, by_user: reminder.user)
-        end.to change { TopicTimer.count }.by(1)
-      end
-
-      it "can update a user's existing record" do
-        freeze_time now
-
-        reminder
-        expect {
-          topic.set_or_create_timer(TopicTimer.types[:reminder], 11, by_user: admin)
-        }.to_not change { TopicTimer.count }
-        reminder.reload
-        expect(reminder.execute_at).to eq_time(11.hours.from_now)
-      end
-    end
   end
 
   describe '.for_digest' do

spec/models/topic_timer_spec.rb

@@ -18,23 +18,6 @@ RSpec.describe TopicTimer, type: :model do
         expect { Fabricate(:topic_timer, topic: topic) }
           .to raise_error(ActiveRecord::RecordInvalid)
       end
-
-      it 'should ensure that only one active private topic timer exists per user' do
-        Fabricate(:topic_timer, topic: topic, user: admin, status_type: TopicTimer.types[:reminder])
-
-        expect { Fabricate(:topic_timer, topic: topic, user: admin, status_type: TopicTimer.types[:reminder]) }
-          .to raise_error(ActiveRecord::RecordInvalid)
-      end
-
-      it 'should allow users to have their own private topic timer' do
-        expect do
-          Fabricate(:topic_timer,
-            topic: topic,
-            user: Fabricate(:admin),
-            status_type: TopicTimer.types[:reminder]
-          )
-        end.to_not raise_error
-      end
     end
 
     describe '#execute_at' do
@@ -274,23 +257,5 @@ RSpec.describe TopicTimer, type: :model do
         state: false
       })).to eq(true)
     end
-
-    it "should enqueue remind me jobs that have been missed" do
-      reminder = Fabricate(:topic_timer,
-        status_type: described_class.types[:reminder],
-        execute_at: Time.zone.now - 1.hour,
-        created_at: Time.zone.now - 2.hour
-      )
-
-      # creating topic timers already enqueues jobs
-      # let's delete them to test ensure_consistency!
-      Sidekiq::Worker.clear_all
-
-      expect { described_class.ensure_consistency! }
-        .to change { Jobs::TopicReminder.jobs.count }.by(1)
-
-      job_args = Jobs::TopicReminder.jobs.first["args"].first
-      expect(job_args["topic_timer_id"]).to eq(reminder.id)
-    end
   end
 end

spec/requests/api/topics_spec.rb

@@ -309,7 +309,6 @@ describe 'posts' do
           chunk_size: { type: :integer },
           bookmarked: { type: :boolean },
           topic_timer: { type: :string, nullable: true },
-          private_topic_timer: { type: :string, nullable: true },
           message_bus_last_id: { type: :integer },
           participant_count: { type: :integer },
           show_read_indicator: { type: :boolean },

test/javascripts/acceptance/composer-actions-test.js

@@ -295,7 +295,6 @@ QUnit.test("reply_as_new_group_message", async (assert) => {
         bookmarked: false,
         message_archived: false,
         topic_timer: null,
-        private_topic_timer: null,
         message_bus_last_id: 5,
         participant_count: 1,
         pm_with_non_human_user: false,

test/javascripts/acceptance/topic-edit-timer-test.js

@@ -21,7 +21,6 @@ acceptance("Topic - Edit timer", {
 
 QUnit.test("default", async (assert) => {
   updateCurrentUser({ moderator: true });
-  const timerType = selectKit(".select-kit.timer-type");
   const futureDateInputSelector = selectKit(".future-date-input-selector");
 
   await visit("/t/internationalization-localization");
@@ -30,14 +29,6 @@ QUnit.test("default", async (assert) => {
 
   assert.equal(futureDateInputSelector.header().label(), "Select a timeframe");
   assert.equal(futureDateInputSelector.header().value(), null);
-
-  await click("#private-topic-timer");
-
-  assert.equal(timerType.header().label(), "Remind Me");
-  assert.equal(timerType.header().value(), "reminder");
-
-  assert.equal(futureDateInputSelector.header().label(), "Select a timeframe");
-  assert.equal(futureDateInputSelector.header().value(), null);
 });
 
 QUnit.test("autoclose - specific time", async (assert) => {

test/javascripts/fixtures/poll.js

@@ -691,7 +691,6 @@ export default {
     chunk_size: 20,
     bookmarked: false,
     topic_timer: null,
-    private_topic_timer: null,
     message_bus_last_id: 1,
     participant_count: 1,
     show_read_indicator: false,

test/javascripts/fixtures/topic.js

@@ -5071,7 +5071,6 @@ export default {
     bookmarked: false,
     message_archived: false,
     topic_timer: null,
-    private_topic_timer: null,
     message_bus_last_id: 7,
     participant_count: 2,
     pm_with_non_human_user: false
@@ -5352,7 +5351,6 @@ export default {
     chunk_size: 20,
     bookmarked: false,
     topic_timer: null,
-    private_topic_timer: null,
     message_bus_last_id: 4,
     participant_count: 2,
     show_read_indicator: false,