diff --git a/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js b/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js index 9513308561b..ecacbdf95ea 100644 --- a/app/assets/javascripts/discourse/app/components/edit-topic-timer-form.js +++ b/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", diff --git a/app/assets/javascripts/discourse/app/components/topic-timer-info.js b/app/assets/javascripts/discourse/app/components/topic-timer-info.js index c5e5da4d929..177b5d9d6e5 100644 --- a/app/assets/javascripts/discourse/app/components/topic-timer-info.js +++ b/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 { - REMINDER_TYPE, - DELETE_REPLIES_TYPE, -} from "discourse/controllers/edit-topic-timer"; +import { 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") - canRemoveTimer(type) { - if (type === REMINDER_TYPE) return true; + @discourseComputed + canRemoveTimer() { return this.currentUser && this.currentUser.get("canManageTopic"); }, diff --git a/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js b/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js index f3d2b972fb6..784a6d07054 100644 --- a/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js +++ b/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() - 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; - }, + topicTimer: alias("model.topic_timer"), _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.isPublic === "true" ? "topic_timer" : "private_topic_timer"; - this.set(`model.${topicTimer}`, EmberObject.create({})); + this.set("model.topic_timer", EmberObject.create({})); this.setProperties({ selection: null, diff --git a/app/assets/javascripts/discourse/app/routes/topic.js b/app/assets/javascripts/discourse/app/routes/topic.js index fa3e3168907..28d61612353 100644 --- a/app/assets/javascripts/discourse/app/routes/topic.js +++ b/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"); }, diff --git a/app/assets/javascripts/discourse/app/templates/modal/edit-topic-timer.hbs b/app/assets/javascripts/discourse/app/templates/modal/edit-topic-timer.hbs index 7f6634297bc..cc3fffea505 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/edit-topic-timer.hbs +++ b/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"}} -
- - - -
- {{edit-topic-timer-form topic=model topicTimer=topicTimer - timerTypes=selections + timerTypes=publicTimerTypes updateTime=updateTime onChangeStatusType=(action "onChangeStatusType") onChangeInput=(action "onChangeInput") diff --git a/app/assets/javascripts/discourse/app/templates/topic.hbs b/app/assets/javascripts/discourse/app/templates/topic.hbs index f0715b2d3f3..af022006355 100644 --- a/app/assets/javascripts/discourse/app/templates/topic.hbs +++ b/app/assets/javascripts/discourse/app/templates/topic.hbs @@ -286,15 +286,6 @@ {{/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 diff --git a/app/jobs/regular/topic_reminder.rb b/app/jobs/regular/topic_reminder.rb index a9eed2b36e6..e4b103d66d9 100644 --- a/app/jobs/regular/topic_reminder.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index 598d434ea57..3e8c333c36c 100644 --- a/app/models/topic.rb +++ b/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] diff --git a/app/models/topic_timer.rb b/app/models/topic_timer.rb index 55f54d04cb7..89da22a24fa 100644 --- a/app/models/topic_timer.rb +++ b/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 diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 27cc97523ed..7c6bd0bbef9 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/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 diff --git a/app/serializers/web_hook_topic_view_serializer.rb b/app/serializers/web_hook_topic_view_serializer.rb index 76dc3818834..849250bc1ca 100644 --- a/app/serializers/web_hook_topic_view_serializer.rb +++ b/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| diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index e0c002e456b..e630d466b79 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/db/migrate/20200819030609_migrate_user_topic_timers_to_bookmark_reminders.rb b/db/migrate/20200819030609_migrate_user_topic_timers_to_bookmark_reminders.rb new file mode 100644 index 00000000000..2c9ddbc6b65 --- /dev/null +++ b/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 diff --git a/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 b/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 index e30cde370eb..9105e5e9766 100644 --- a/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 +++ b/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, diff --git a/spec/jobs/topic_reminder_spec.rb b/spec/jobs/topic_reminder_spec.rb deleted file mode 100644 index a0c331a889a..00000000000 --- a/spec/jobs/topic_reminder_spec.rb +++ /dev/null @@ -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 diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 36b6c1ec611..17346ce41e8 100644 --- a/spec/models/topic_spec.rb +++ b/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 diff --git a/spec/models/topic_timer_spec.rb b/spec/models/topic_timer_spec.rb index 4d1e09c819a..dbb540950a7 100644 --- a/spec/models/topic_timer_spec.rb +++ b/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 diff --git a/spec/requests/api/topics_spec.rb b/spec/requests/api/topics_spec.rb index 6dac194b42a..bd3d5f280a6 100644 --- a/spec/requests/api/topics_spec.rb +++ b/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 }, diff --git a/test/javascripts/acceptance/composer-actions-test.js b/test/javascripts/acceptance/composer-actions-test.js index 317acde5e89..821665371a2 100644 --- a/test/javascripts/acceptance/composer-actions-test.js +++ b/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, diff --git a/test/javascripts/acceptance/topic-edit-timer-test.js b/test/javascripts/acceptance/topic-edit-timer-test.js index f20caf47c58..6a2c0912272 100644 --- a/test/javascripts/acceptance/topic-edit-timer-test.js +++ b/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) => { diff --git a/test/javascripts/fixtures/poll.js b/test/javascripts/fixtures/poll.js index 71eacf3729a..674c711a4b5 100644 --- a/test/javascripts/fixtures/poll.js +++ b/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, diff --git a/test/javascripts/fixtures/topic.js b/test/javascripts/fixtures/topic.js index c4ba2e604f0..beddf91d948 100644 --- a/test/javascripts/fixtures/topic.js +++ b/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,