From 7cfd5f87ff16d3010274851b7442aa92c2d494d6 Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Tue, 25 Aug 2020 19:17:12 -0600
Subject: [PATCH] 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.
---
 app/controllers/topics_controller.rb    |  9 ++++++
 app/models/topic_timer.rb               |  2 --
 spec/models/topic_timer_spec.rb         | 43 +++----------------------
 spec/requests/topics_controller_spec.rb | 23 +++++++++++++
 4 files changed, 36 insertions(+), 41 deletions(-)

diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb
index 583d7ebeedb..72d3808620d 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 20e3bb67f97..0577e065deb 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 6c19bd6ca79..e38c1ccf5c1 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 17a896bac93..7fbcf8bbf9a 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