DEV: Split toggle topic close job (#11679)

Splits the `ToggleTopicClosed` job into two distinct `OpenTopic` and `CloseTopic` jobs to make the code clearer. The old job cannot be deleted yet because of outstanding sidekiq schedules, so a todo has been added to do so later this year.

Also replaced mentions of `topic_status_update` with `topic_timer` in some files, because the `topic_status_update` model is obsolete and replaced by topic timer.

Added some shortcut methods for checking if a topic is open/whether a user can change an open topic.

app/jobs/regular/bump_topic.rb

@@ -4,7 +4,7 @@ module Jobs
   class BumpTopic < ::Jobs::Base
 
     def execute(args)
-      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
 
       topic = topic_timer&.topic
 

app/jobs/regular/clear_slow_mode.rb

@@ -4,7 +4,7 @@ module Jobs
   class ClearSlowMode < ::Jobs::Base
 
     def execute(args)
-      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
 
       if topic_timer.nil? || topic_timer.execute_at > Time.zone.now
         return

app/jobs/regular/close_topic.rb

@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+module Jobs
+  class CloseTopic < ::Jobs::Base
+    def execute(args)
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
+      return if !topic_timer&.runnable?
+
+      topic = topic_timer.topic
+      user = topic_timer.user
+      silent = args[:silent]
+
+      if topic.blank? || topic.closed?
+        topic_timer.destroy!
+        return
+      end
+
+      if !Guardian.new(user).can_close_topic?(topic)
+        topic_timer.destroy!
+        topic.reload
+
+        if topic_timer.based_on_last_post
+          topic.inherit_auto_close_from_category(timer_type: silent ? :silent_close : :close)
+        end
+
+        return
+      end
+
+      # this handles deleting the topic timer as wel, see TopicStatusUpdater
+      topic.update_status('autoclosed', true, user, { silent: silent })
+    end
+  end
+end

app/jobs/regular/delete_replies.rb

@@ -4,7 +4,7 @@ module Jobs
   class DeleteReplies < ::Jobs::Base
 
     def execute(args)
-      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
 
       topic = topic_timer&.topic
 

app/jobs/regular/delete_topic.rb

@@ -4,7 +4,7 @@ module Jobs
   class DeleteTopic < ::Jobs::Base
 
     def execute(args)
-      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
 
       topic = topic_timer&.topic
 

app/jobs/regular/open_topic.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module Jobs
+  class OpenTopic < ::Jobs::Base
+    def execute(args)
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
+      return if !topic_timer&.runnable?
+
+      topic = topic_timer.topic
+      user = topic_timer.user
+
+      if topic.blank?
+        topic_timer.destroy!
+        return
+      end
+
+      if !Guardian.new(user).can_open_topic?(topic) || topic.open?
+        topic_timer.destroy!
+        topic.reload
+
+        topic.inherit_auto_close_from_category(timer_type: :close)
+
+        return
+      end
+
+      # guards against reopening a topic too early if the topic has
+      # been auto closed because of reviewables/reports, this will
+      # just update the existing topic timer and push it down the line
+      if topic.auto_close_threshold_reached?
+        topic.set_or_create_timer(
+          TopicTimer.types[:open],
+          SiteSetting.num_hours_to_close_topic,
+          by_user: Discourse.system_user
+        )
+      else
+
+        # autoclosed, false is just another way of saying open.
+        # this handles deleting the topic timer as wel, see TopicStatusUpdater
+        topic.update_status('autoclosed', false, user)
+      end
+
+      topic.inherit_auto_close_from_category(timer_type: :close)
+    end
+  end
+end

app/jobs/regular/publish_topic_to_category.rb

@@ -3,7 +3,7 @@
 module Jobs
   class PublishTopicToCategory < ::Jobs::Base
     def execute(args)
-      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+      topic_timer = TopicTimer.find_by(id: args[:topic_timer_id])
       return if topic_timer.blank?
 
       topic = topic_timer.topic

app/jobs/regular/toggle_topic_closed.rb

@@ -1,9 +1,14 @@
 # frozen_string_literal: true
 
 module Jobs
+  # TODO: DEPRECATED - Use OpenTopic and CloseTopic instead.
+  # (martin - 2021-05-01) - Delete once topic timer revamp is completed.
   class ToggleTopicClosed < ::Jobs::Base
     def execute(args)
       topic_timer = TopicTimer.find_by(id: args[:topic_timer_id] || args[:topic_status_update_id])
+
+      # state false is Open Topic
+      # state true is Close Topic
       state = !!args[:state]
       timer_type = args[:silent] ? :silent_close : :close
 

app/models/topic.rb

@@ -576,13 +576,17 @@ class Topic < ActiveRecord::Base
   end
 
   def private_message?
-    archetype == Archetype.private_message
+    self.archetype == Archetype.private_message
   end
 
   def regular?
     self.archetype == Archetype.default
   end
 
+  def open?
+    !self.closed?
+  end
+
   MAX_SIMILAR_BODY_LENGTH ||= 200
 
   def self.similar_to(title, raw, user = nil)

app/models/topic_timer.rb

@@ -15,7 +15,7 @@ class TopicTimer < ActiveRecord::Base
   validates :status_type, uniqueness: { scope: [:topic_id, :deleted_at, :user_id] }, if: :private_type?
   validates :category_id, presence: true, if: :publishing_to_category?
 
-  validate :ensure_update_will_happen
+  validate :executed_at_in_future?
 
   scope :scheduled_bump_topics, -> { where(status_type: TopicTimer.types[:bump], deleted_at: nil).pluck(:topic_id) }
 
@@ -84,23 +84,38 @@ class TopicTimer < ActiveRecord::Base
     !!self.class.private_types[self.status_type]
   end
 
+  def runnable?
+    return false if deleted_at.present?
+    return false if execute_at > Time.zone.now
+    true
+  end
+
   private
 
-  def ensure_update_will_happen
+  def executed_at_in_future?
-    if created_at && (execute_at < created_at)
+    return if created_at.blank? || (execute_at > created_at)
-      errors.add(:execute_at, I18n.t(
+
-        'activerecord.errors.models.topic_timer.attributes.execute_at.in_the_past'
+    errors.add(:execute_at, I18n.t(
-      ))
+      'activerecord.errors.models.topic_timer.attributes.execute_at.in_the_past'
-    end
+    ))
   end
 
+  # TODO(martin - 2021-05-01) - Remove cancels for toggle_topic_closed once topic timer revamp completed.
   def cancel_auto_close_job
     Jobs.cancel_scheduled_job(:toggle_topic_closed, topic_timer_id: id)
+    Jobs.cancel_scheduled_job(:close_topic, topic_timer_id: id)
   end
-  alias_method :cancel_auto_open_job, :cancel_auto_close_job
 
+  # TODO(martin - 2021-05-01) - Remove cancels for toggle_topic_closed once topic timer revamp completed.
+  def cancel_auto_open_job
+    Jobs.cancel_scheduled_job(:toggle_topic_closed, topic_timer_id: id)
+    Jobs.cancel_scheduled_job(:open_topic, topic_timer_id: id)
+  end
+
+  # TODO(martin - 2021-05-01) - Remove cancels for toggle_topic_closed once topic timer revamp completed.
   def cancel_auto_silent_close_job
     Jobs.cancel_scheduled_job(:toggle_topic_closed, topic_timer_id: id)
+    Jobs.cancel_scheduled_job(:close_topic, topic_timer_id: id)
   end
 
   def cancel_auto_publish_to_category_job
@@ -138,29 +153,19 @@ class TopicTimer < ActiveRecord::Base
   def schedule_auto_open_job(time)
     topic.update_status('closed', true, user) if topic && !topic.closed
 
-    Jobs.enqueue_at(time, :toggle_topic_closed,
+    Jobs.enqueue_at(time, :open_topic, topic_timer_id: id)
-      topic_timer_id: id,
-      state: false
-    )
   end
 
   def schedule_auto_close_job(time)
     topic.update_status('closed', false, user) if topic&.closed
 
-    Jobs.enqueue_at(time, :toggle_topic_closed,
+    Jobs.enqueue_at(time, :close_topic, topic_timer_id: id)
-      topic_timer_id: id,
-      state: true
-    )
   end
 
   def schedule_auto_silent_close_job(time)
     topic.update_status('closed', false, user) if topic&.closed
 
-    Jobs.enqueue_at(time, :toggle_topic_closed,
+    Jobs.enqueue_at(time, :close_topic, topic_timer_id: id, silent: true)
-      topic_timer_id: id,
-      silent: true,
-      state: true
-    )
   end
 
   def schedule_auto_publish_to_category_job(time)

app/services/topic_status_updater.rb

@@ -4,7 +4,7 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
   def update!(status, enabled, opts = {})
     status = Status.new(status, enabled)
 
-    @topic_status_update = topic.public_topic_timer
+    @topic_timer = topic.public_topic_timer
 
     updated = nil
     Topic.transaction do
@@ -46,7 +46,7 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
       UserProfile.remove_featured_topic_from_all_profiles(topic)
     end
 
-    if @topic_status_update
+    if @topic_timer
       if status.manually_closing_topic? || status.closing_topic?
         topic.delete_topic_timer(TopicTimer.types[:close])
         topic.delete_topic_timer(TopicTimer.types[:silent_close])
@@ -83,17 +83,17 @@ TopicStatusUpdater = Struct.new(:topic, :user) do
   def message_for(status)
     if status.autoclosed?
       locale_key = status.locale_key.dup
-      locale_key << "_lastpost" if @topic_status_update&.based_on_last_post
+      locale_key << "_lastpost" if @topic_timer&.based_on_last_post
       message_for_autoclosed(locale_key)
     end
   end
 
   def message_for_autoclosed(locale_key)
     num_minutes =
-      if @topic_status_update&.based_on_last_post
+      if @topic_timer&.based_on_last_post
-        (@topic_status_update.duration || 0).hours
+        (@topic_timer.duration || 0).hours
-      elsif @topic_status_update&.created_at
+      elsif @topic_timer&.created_at
-        Time.zone.now - @topic_status_update.created_at
+        Time.zone.now - @topic_timer.created_at
       else
         Time.zone.now - topic.created_at
       end

lib/guardian/topic_guardian.rb

@@ -234,6 +234,7 @@ module TopicGuardian
   end
   alias :can_archive_topic? :can_perform_action_available_to_group_moderators?
   alias :can_close_topic? :can_perform_action_available_to_group_moderators?
+  alias :can_open_topic? :can_perform_action_available_to_group_moderators?
   alias :can_split_merge_topic? :can_perform_action_available_to_group_moderators?
   alias :can_edit_staff_notes? :can_perform_action_available_to_group_moderators?
 

spec/integration/topic_auto_close_spec.rb

@@ -4,7 +4,7 @@
 require 'rails_helper'
 
 describe Topic do
-  let(:job_klass) { Jobs::ToggleTopicClosed }
+  let(:job_klass) { Jobs::CloseTopic }
 
   context 'creating a topic without auto-close' do
     let(:topic) { Fabricate(:topic, category: category) }
@@ -46,7 +46,6 @@ describe Topic do
           args = job_klass.jobs.last['args'].first
 
           expect(args["topic_timer_id"]).to eq(topic.public_topic_timer.id)
-          expect(args["state"]).to eq(true)
         end
 
         context 'topic was created by staff user' do
@@ -65,7 +64,6 @@ describe Topic do
             args = job_klass.jobs.last['args'].first
 
             expect(args["topic_timer_id"]).to eq(topic_status_update.id)
-            expect(args["state"]).to eq(true)
           end
 
           context 'topic is closed manually' do
@@ -96,7 +94,6 @@ describe Topic do
             args = job_klass.jobs.last['args'].first
 
             expect(args["topic_timer_id"]).to eq(topic_status_update.id)
-            expect(args["state"]).to eq(true)
           end
         end
       end

spec/jobs/close_topic_spec.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Jobs::CloseTopic do
+  fab!(:admin) { Fabricate(:admin) }
+
+  fab!(:topic) do
+    Fabricate(:topic_timer, user: admin).topic
+  end
+
+  it 'should be able to close a topic' do
+    freeze_time(61.minutes.from_now) do
+      described_class.new.execute(
+        topic_timer_id: topic.public_topic_timer.id,
+        state: true
+      )
+
+      expect(topic.reload.closed).to eq(true)
+
+      expect(Post.last.raw).to eq(I18n.t(
+        'topic_statuses.autoclosed_enabled_minutes', count: 61
+      ))
+    end
+  end
+
+  describe 'when trying to close a topic that has already been closed' do
+    it 'should delete the topic timer' do
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      topic.update!(closed: true)
+
+      expect do
+        described_class.new.execute(
+          topic_timer_id: topic.public_topic_timer.id,
+          state: true
+        )
+      end.to change { TopicTimer.exists?(topic_id: topic.id) }.from(true).to(false)
+    end
+  end
+
+  describe 'when trying to close a topic that has been deleted' do
+    it 'should delete the topic timer' do
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      topic.trash!
+
+      expect do
+        described_class.new.execute(
+          topic_timer_id: topic.public_topic_timer.id,
+          state: true
+        )
+      end.to change { TopicTimer.exists?(topic_id: topic.id) }.from(true).to(false)
+    end
+  end
+
+  describe 'when user is no longer authorized to close topics' do
+    fab!(:user) { Fabricate(:user) }
+
+    fab!(:topic) do
+      Fabricate(:topic_timer, user: user).topic
+    end
+
+    it 'should destroy the topic timer' do
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      expect do
+        described_class.new.execute(
+          topic_timer_id: topic.public_topic_timer.id,
+          state: true
+        )
+      end.to change { TopicTimer.exists?(topic_id: topic.id) }.from(true).to(false)
+
+      expect(topic.reload.closed).to eq(false)
+    end
+
+    it "should reconfigure topic timer if category's topics are set to autoclose" do
+      category = Fabricate(:category,
+        auto_close_based_on_last_post: true,
+        auto_close_hours: 5
+      )
+
+      topic = Fabricate(:topic, category: category)
+      topic.public_topic_timer.update!(user: user)
+
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      expect do
+        described_class.new.execute(
+          topic_timer_id: topic.public_topic_timer.id,
+          state: true
+        )
+      end.to change { topic.reload.public_topic_timer.user }.from(user).to(Discourse.system_user)
+        .and change { topic.public_topic_timer.id }
+
+      expect(topic.reload.closed).to eq(false)
+    end
+  end
+end

spec/jobs/open_topic_spec.rb

@@ -0,0 +1,99 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Jobs::OpenTopic do
+  fab!(:admin) { Fabricate(:admin) }
+
+  fab!(:topic) do
+    Fabricate(:topic_timer, user: admin).topic
+  end
+
+  before do
+    topic.update!(closed: true)
+  end
+
+  it 'should work' do
+    freeze_time(61.minutes.from_now) do
+      described_class.new.execute(topic_timer_id: topic.public_topic_timer.id)
+
+      expect(topic.reload.open?).to eq(true)
+
+      expect(Post.last.raw).to eq(I18n.t(
+        'topic_statuses.autoclosed_disabled_minutes', count: 61
+      ))
+    end
+  end
+
+  describe 'when category has auto close configured' do
+    fab!(:category) do
+      Fabricate(:category,
+                auto_close_based_on_last_post: true,
+                auto_close_hours: 5
+               )
+    end
+
+    fab!(:topic) { Fabricate(:topic, category: category, closed: true) }
+
+    it "should restore the category's auto close timer" do
+      Fabricate(:topic_timer,
+                status_type: TopicTimer.types[:open],
+                topic: topic,
+                user: admin
+               )
+
+      freeze_time(61.minutes.from_now) do
+        described_class.new.execute(topic_timer_id: topic.public_topic_timer.id)
+
+        expect(topic.reload.open?).to eq(true)
+
+        topic_timer = topic.public_topic_timer
+
+        expect(topic_timer.status_type).to eq(TopicTimer.types[:close])
+        expect(topic_timer.execute_at).to eq_time(5.hours.from_now)
+      end
+    end
+  end
+
+  describe 'when user is no longer authorized to open topics' do
+    fab!(:user) { Fabricate(:user) }
+
+    fab!(:topic) do
+      Fabricate(:topic_timer, user: user).topic
+    end
+
+    it 'should destroy the topic timer' do
+      topic.update!(closed: true)
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      expect do
+        described_class.new.execute(topic_timer_id: topic.public_topic_timer.id)
+      end.to change { TopicTimer.exists?(topic_id: topic.id) }.from(true).to(false)
+
+      expect(topic.reload.open?).to eq(false)
+    end
+
+    it "should reconfigure topic timer if category's topics are set to autoclose" do
+      category = Fabricate(:category,
+        auto_close_based_on_last_post: true,
+        auto_close_hours: 5
+      )
+
+      topic = Fabricate(:topic, category: category)
+      topic.public_topic_timer.update!(user: user)
+      topic.reload
+
+      freeze_time(topic.public_topic_timer.execute_at + 1.minute)
+
+      expect do
+        described_class.new.execute(
+          topic_timer_id: topic.public_topic_timer.id,
+          state: true
+        )
+      end.to change { topic.reload.public_topic_timer.user }.from(user).to(Discourse.system_user)
+        .and change { topic.public_topic_timer.id }
+
+      expect(topic.reload.closed).to eq(false)
+    end
+  end
+end

spec/models/post_action_spec.rb

@@ -764,8 +764,8 @@ describe PostAction do
 
         freeze_time timer.execute_at
 
-        expect_enqueued_with(job: :toggle_topic_closed, args: { topic_timer_id: timer.id, state: false }, at: Time.zone.now + 1.hour) do
+        expect_enqueued_with(job: :open_topic, args: { topic_timer_id: timer.id }, at: Time.zone.now + 1.hour) do
-          Jobs::ToggleTopicClosed.new.execute(topic_timer_id: timer.id, state: false)
+          Jobs::OpenTopic.new.execute(topic_timer_id: timer.id)
         end
 
         expect(topic.reload.closed).to eq(true)

spec/models/topic_timer_spec.rb

@@ -91,15 +91,18 @@ RSpec.describe TopicTimer, type: :model do
         Jobs.expects(:cancel_scheduled_job).with(
           :toggle_topic_closed, topic_timer_id: topic_timer.id
         )
+        Jobs.expects(:cancel_scheduled_job).with(
+          :close_topic, topic_timer_id: topic_timer.id
+        )
 
-        expect_enqueued_with(job: :toggle_topic_closed, args: { topic_timer_id: topic_timer.id, state: true }, at: 3.days.from_now) do
+        expect_enqueued_with(job: :close_topic, args: { topic_timer_id: topic_timer.id }, at: 3.days.from_now) do
           topic_timer.update!(execute_at: 3.days.from_now, created_at: Time.zone.now)
         end
       end
 
       describe 'when execute_at is smaller than the current time' do
         it 'should enqueue the job immediately' do
-          expect_enqueued_with(job: :toggle_topic_closed, args: { topic_timer_id: topic_timer.id, state: true }, at: Time.zone.now) do
+          expect_enqueued_with(job: :close_topic, args: { topic_timer_id: topic_timer.id }, at: Time.zone.now) do
             topic_timer.update!(
               execute_at: Time.zone.now - 1.hour,
               created_at: Time.zone.now - 2.hour
@@ -114,8 +117,11 @@ RSpec.describe TopicTimer, type: :model do
         Jobs.expects(:cancel_scheduled_job).with(
           :toggle_topic_closed, topic_timer_id: topic_timer.id
         )
+        Jobs.expects(:cancel_scheduled_job).with(
+          :close_topic, topic_timer_id: topic_timer.id
+        )
 
-        expect_enqueued_with(job: :toggle_topic_closed, args: { topic_timer_id: topic_timer.id, state: true }, at: topic_timer.execute_at) do
+        expect_enqueued_with(job: :close_topic, args: { topic_timer_id: topic_timer.id }, at: topic_timer.execute_at) do
           topic_timer.update!(user: admin)
         end
       end
@@ -235,27 +241,58 @@ RSpec.describe TopicTimer, type: :model do
       Sidekiq::Worker.clear_all
 
       expect { described_class.ensure_consistency! }
-        .to change { Jobs::ToggleTopicClosed.jobs.count }.by(4)
+        .to change { Jobs::CloseTopic.jobs.count }.by(2).and change { Jobs::OpenTopic.jobs.count }.by(2)
 
-      expect(job_enqueued?(job: :toggle_topic_closed, args: {
+      expect(job_enqueued?(job: :close_topic, args: {
-        topic_timer_id: close_topic_timer.id,
+        topic_timer_id: close_topic_timer.id
-        state: true
       })).to eq(true)
 
-      expect(job_enqueued?(job: :toggle_topic_closed, args: {
+      expect(job_enqueued?(job: :open_topic, args: {
-        topic_timer_id: open_topic_timer.id,
+        topic_timer_id: open_topic_timer.id
-        state: false
       })).to eq(true)
 
-      expect(job_enqueued?(job: :toggle_topic_closed, args: {
+      expect(job_enqueued?(job: :close_topic, args: {
-        topic_timer_id: trashed_close_topic_timer.id,
+        topic_timer_id: trashed_close_topic_timer.id
-        state: true
       })).to eq(true)
 
-      expect(job_enqueued?(job: :toggle_topic_closed, args: {
+      expect(job_enqueued?(job: :open_topic, args: {
-        topic_timer_id: trashed_open_topic_timer.id,
+        topic_timer_id: trashed_open_topic_timer.id
-        state: false
       })).to eq(true)
     end
   end
+
+  describe "runnable?" do
+    it "returns false if execute_at > now" do
+      topic_timer = Fabricate.build(:topic_timer,
+                                    execute_at: Time.zone.now + 1.hour,
+                                    user: Fabricate(:user),
+                                    topic: Fabricate(:topic)
+                                   )
+
+      expect(topic_timer.runnable?).to eq(false)
+    end
+
+    it "returns false if timer is deleted" do
+      topic_timer = Fabricate.create(:topic_timer,
+                                    execute_at: Time.zone.now - 1.hour,
+                                    created_at: Time.zone.now - 2.hour,
+                                    user: Fabricate(:user),
+                                    topic: Fabricate(:topic)
+                                   )
+      topic_timer.trash!
+
+      expect(topic_timer.runnable?).to eq(false)
+    end
+
+    it "returns true if execute_at < now" do
+      topic_timer = Fabricate.build(:topic_timer,
+                                    execute_at: Time.zone.now - 1.hour,
+                                    created_at: Time.zone.now - 2.hour,
+                                    user: Fabricate(:user),
+                                    topic: Fabricate(:topic)
+                                   )
+
+      expect(topic_timer.runnable?).to eq(true)
+    end
+  end
 end