FIX: Topic Timer auto opening closed topics (#10524)

FIX: Topic Timer auto opening closed topics (#10524)

This commit is addressing an issue where it is possible that there could be multiple topic timer jobs running to close a topic or a weird race condition state causing a topic that was just closed to be re-opened.

By removing the logic from the Topic Timer model into the Topic Timer controller endpoint we isolate the code that is used for setting an auto-open or an auto-close timer to just that functionality making the topic timer background jobs safer if multiple are running.

Possibly in the future if we would like this logic back in the model a refactor will be needed where we actually pass in the auto-close and auto-open action instead of mixing it with the close and open action that is currently being passed to the controller.

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 583d7eb..72d3808 100644
--- a/app/controllers/topics_controller.rb
+++ b/app/controllers/topics_controller.rb
@@ -463,6 +463,15 @@ class TopicsController < ApplicationController
     options.merge!(category_id: params[:category_id]) if !params[:category_id].blank?
     options.merge!(duration: params[:duration].to_i) if params[:duration].present?
 
+    # Be sure to close/open the topic when setting an auto-open/auto-close timer
+    if status_type == TopicTimer.types[:open]
+      topic.update_status('closed', true, current_user) if !topic.closed
+    end
+
+    if status_type == TopicTimer.types[:close]
+      topic.update_status('closed', false, current_user) if topic.closed
+    end
+
     topic_status_update = topic.set_or_create_timer(
       status_type,
       params[:time],
diff --git a/app/models/topic_timer.rb b/app/models/topic_timer.rb
index 20e3bb6..0577e06 100644
--- a/app/models/topic_timer.rb
+++ b/app/models/topic_timer.rb
@@ -127,7 +127,6 @@ class TopicTimer < ActiveRecord::Base
 
   def schedule_auto_open_job(time)
     return unless topic
-    topic.update_status('closed', true, user) if !topic.closed
 
     Jobs.enqueue_at(time, :toggle_topic_closed,
       topic_timer_id: id,
@@ -137,7 +136,6 @@ class TopicTimer < ActiveRecord::Base
 
   def schedule_auto_close_job(time)
     return unless topic
-    topic.update_status('closed', false, user) if topic.closed
 
     Jobs.enqueue_at(time, :toggle_topic_closed,
       topic_timer_id: id,
diff --git a/spec/models/topic_timer_spec.rb b/spec/models/topic_timer_spec.rb
index 6c19bd6..e38c1cc 100644
--- a/spec/models/topic_timer_spec.rb
+++ b/spec/models/topic_timer_spec.rb
@@ -138,7 +138,7 @@ RSpec.describe TopicTimer, type: :model do
       end
     end
 
-    describe 'when a open topic status update is created for an open topic' do
+    describe 'when a topic has been deleted' do
       fab!(:topic) { Fabricate(:topic, closed: false) }
       fab!(:topic_timer) do
         Fabricate(:topic_timer,
@@ -151,46 +151,11 @@ RSpec.describe TopicTimer, type: :model do
         Jobs.run_immediately!
       end
 
-      it 'should close the topic' do
+      it 'should not queue the job' do
+        topic.trash!
         topic_timer
-        expect(topic.reload.closed).to eq(true)
-      end
-
-      describe 'when topic has been deleted' do
-        it 'should not queue the job' do
-          topic.trash!
-          topic_timer
-
-          expect(Jobs::ToggleTopicClosed.jobs).to eq([])
-        end
-      end
-    end
 
-    describe 'when a close topic status update is created for a closed topic' do
-      fab!(:topic) { Fabricate(:topic, closed: true) }
-      fab!(:topic_timer) do
-        Fabricate(:topic_timer,
-          status_type: described_class.types[:close],
-          topic: topic
-        )
-      end
-
-      before do
-        Jobs.run_immediately!
-      end
-
-      it 'should open the topic' do
-        topic_timer
-        expect(topic.reload.closed).to eq(false)
-      end
-
-      describe 'when topic has been deleted' do
-        it 'should not queue the job' do
-          topic.trash!
-          topic_timer
-
-          expect(Jobs::ToggleTopicClosed.jobs).to eq([])
-        end
+        expect(Jobs::ToggleTopicClosed.jobs).to eq([])
       end
     end
 
diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb
index 17a896b..7fbcf8b 100644
--- a/spec/requests/topics_controller_spec.rb
+++ b/spec/requests/topics_controller_spec.rb
@@ -2925,6 +2925,29 @@ RSpec.describe TopicsController do
           expect(response.body).to include('status_type')
         end
       end
+
+      it 'should close the topic when setting an auto-open timer' do
+        post "/t/#{topic.id}/timer.json", params: {
+          time: 24,
+          status_type: "open"
+        }
+        expect(response.status).to eq(200)
+        topic_status_update = TopicTimer.last
+        topic = topic_status_update.topic
+        expect(topic.closed).to eq(true)
+      end
+
+      it 'should open the topic when setting an auto-close timer' do
+        topic = Fabricate(:topic, user: user, closed: true)
+        post "/t/#{topic.id}/timer.json", params: {
+          time: 24,
+          status_type: "close"
+        }
+        expect(response.status).to eq(200)
+        topic_status_update = TopicTimer.last
+        topic = topic_status_update.topic
+        expect(topic.closed).to eq(false)
+      end
     end
   end
 

GitHub sha: 7cfd5f87

This commit appears in #10524 which was approved by martin. It was merged by martin.