From fb83757edb381416cf9a19b81a9695875062a538 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 17 Feb 2021 07:51:39 +1000 Subject: [PATCH] FIX: Auto close topic from category settings based on topic created_at (#12082) Previously when inheriting category auto-close settings for a topic, those settings were disrupted if another topic timer was assigned or if a topic was closed then manually re-opened. This PR makes it so that when a topic is manually re-opened the topic auto-close settings are inherited from the category. However, they will now be based on the topic created_at date. As an example, for a topic with a category auto close hours setting of 72 (3 days): * Topic was created on 2021-02-15 08:00 * Topic was closed on 2021-02-16 10:00 * Topic was opened again on 2021-02-17 06:00 Now, the topic will inherit the auto close timer again and will close automatically at **2021-02-18 08:00**, which is based on the creation date. If the current date and time is greater than the original auto-close time (e.g. we were at 2021-02-20 13:45) then no auto-close timer is created. Note, this will not happen if the topic category auto-close setting is "based on last post". --- app/models/topic.rb | 60 ++++++++++----- app/services/topic_status_updater.rb | 1 + spec/integration/topic_auto_close_spec.rb | 8 +- spec/jobs/publish_topic_to_category_spec.rb | 2 +- spec/models/topic_spec.rb | 4 +- spec/services/topic_status_updater_spec.rb | 84 +++++++++++++++++++++ 6 files changed, 132 insertions(+), 27 deletions(-) diff --git a/app/models/topic.rb b/app/models/topic.rb index b4d1080eebf..e2497b298e0 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -370,26 +370,6 @@ class Topic < ActiveRecord::Base self.last_post_user_id ||= user_id end - def inherit_auto_close_from_category(timer_type: :close) - if !self.closed && - !@ignore_category_auto_close && - self.category && - self.category.auto_close_hours && - !public_topic_timer&.execute_at - - based_on_last_post = self.category.auto_close_based_on_last_post - duration_minutes = based_on_last_post ? self.category.auto_close_hours * 60 : nil - - self.set_or_create_timer( - TopicTimer.types[timer_type], - self.category.auto_close_hours, - by_user: Discourse.system_user, - based_on_last_post: based_on_last_post, - duration_minutes: duration_minutes - ) - end - end - def advance_draft_sequence if self.private_message? DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE) @@ -1287,6 +1267,45 @@ class Topic < ActiveRecord::Base Topic.where("pinned_until < now()").update_all(pinned_at: nil, pinned_globally: false, pinned_until: nil) end + def inherit_auto_close_from_category(timer_type: :close) + auto_close_hours = self.category&.auto_close_hours + + if self.open? && + !@ignore_category_auto_close && + auto_close_hours.present? && + public_topic_timer&.execute_at.blank? + + based_on_last_post = self.category.auto_close_based_on_last_post + duration_minutes = based_on_last_post ? auto_close_hours * 60 : nil + + # the timer time can be a timestamp or an integer based + # on the number of hours + auto_close_time = auto_close_hours + + if !based_on_last_post + # set auto close to the original time it should have been + # when the topic was first created. + start_time = self.created_at || Time.zone.now + auto_close_time = start_time + auto_close_hours.hours + + # if we have already passed the original close time then + # we should not recreate the auto-close timer for the topic + return if auto_close_time < Time.zone.now + + # timestamp must be a string for set_or_create_timer + auto_close_time = auto_close_time.to_s + end + + self.set_or_create_timer( + TopicTimer.types[timer_type], + auto_close_time, + by_user: Discourse.system_user, + based_on_last_post: based_on_last_post, + duration_minutes: duration_minutes + ) + end + end + def public_topic_timer @public_topic_timer ||= topic_timers.find_by(deleted_at: nil, public_type: true) end @@ -1295,6 +1314,7 @@ class Topic < ActiveRecord::Base options = { status_type: status_type } options.merge!(user: by_user) unless TopicTimer.public_types[status_type] self.topic_timers.find_by(options)&.trash!(by_user) + @public_topic_timer = nil nil end diff --git a/app/services/topic_status_updater.rb b/app/services/topic_status_updater.rb index 1013c2d0dc9..fe0f912ec0d 100644 --- a/app/services/topic_status_updater.rb +++ b/app/services/topic_status_updater.rb @@ -56,6 +56,7 @@ TopicStatusUpdater = Struct.new(:topic, :user) do topic.delete_topic_timer(TopicTimer.types[:silent_close]) elsif status.manually_opening_topic? || status.opening_topic? topic.delete_topic_timer(TopicTimer.types[:open]) + topic.inherit_auto_close_from_category end end diff --git a/spec/integration/topic_auto_close_spec.rb b/spec/integration/topic_auto_close_spec.rb index 170a3afc11c..0771e7c7045 100644 --- a/spec/integration/topic_auto_close_spec.rb +++ b/spec/integration/topic_auto_close_spec.rb @@ -41,7 +41,7 @@ describe Topic do topic_status_update = TopicTimer.last expect(topic_status_update.topic).to eq(topic) - expect(topic.public_topic_timer.execute_at).to eq_time(2.hours.from_now) + expect(topic.public_topic_timer.execute_at).to be_within_one_second_of(2.hours.from_now) end context 'topic was created by staff user' do @@ -54,7 +54,7 @@ describe Topic do topic_status_update = TopicTimer.last expect(topic_status_update.topic).to eq(staff_topic) - expect(topic_status_update.execute_at).to eq_time(2.hours.from_now) + expect(topic_status_update.execute_at).to be_within_one_second_of(2.hours.from_now) expect(topic_status_update.user).to eq(Discourse.system_user) end @@ -65,7 +65,7 @@ describe Topic do staff_topic.update_status('closed', true, admin) expect(TopicTimer.with_deleted.find(topic_timer_id).deleted_at) - .to eq_time(Time.zone.now) + .to be_within_one_second_of(Time.zone.now) end end end @@ -80,7 +80,7 @@ describe Topic do topic_status_update = TopicTimer.last expect(topic_status_update.topic).to eq(regular_user_topic) - expect(topic_status_update.execute_at).to eq_time(2.hours.from_now) + expect(topic_status_update.execute_at).to be_within_one_second_of(2.hours.from_now) expect(topic_status_update.user).to eq(Discourse.system_user) end end diff --git a/spec/jobs/publish_topic_to_category_spec.rb b/spec/jobs/publish_topic_to_category_spec.rb index 585766f8b1a..7d895de0cac 100644 --- a/spec/jobs/publish_topic_to_category_spec.rb +++ b/spec/jobs/publish_topic_to_category_spec.rb @@ -133,7 +133,7 @@ RSpec.describe Jobs::PublishTopicToCategory do topic_timer = topic.public_topic_timer expect(topic.category).to eq(another_category) expect(topic_timer.status_type).to eq(TopicTimer.types[:close]) - expect(topic_timer.execute_at).to eq_time(5.hours.from_now) + expect(topic_timer.execute_at).to be_within_one_second_of(5.hours.from_now) end end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 822e7b8da6d..6a63d1578f9 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1628,7 +1628,7 @@ describe Topic do expect(topic_timer.user).to eq(Discourse.system_user) expect(topic_timer.topic).to eq(topic) - expect(topic_timer.execute_at).to eq_time(5.hours.from_now) + expect(topic_timer.execute_at).to be_within_one_second_of(5.hours.from_now) end describe 'when topic is already closed' do @@ -1885,7 +1885,7 @@ describe Topic do freeze_time Jobs.run_immediately! - expect(topic.topic_timers.first.execute_at).to eq_time(topic.created_at + 4.hours) + expect(topic.topic_timers.first.execute_at).to be_within_one_second_of(topic.created_at + 4.hours) topic.set_or_create_timer(TopicTimer.types[:close], 2, by_user: admin) diff --git a/spec/services/topic_status_updater_spec.rb b/spec/services/topic_status_updater_spec.rb index b83fe5e5901..5b8fd0c7b2c 100644 --- a/spec/services/topic_status_updater_spec.rb +++ b/spec/services/topic_status_updater_spec.rb @@ -70,6 +70,90 @@ describe TopicStatusUpdater do expect(last_post.raw).to eq(I18n.t("topic_statuses.autoclosed_enabled_lastpost_hours", count: 10)) end + describe "opening the topic" do + it "opens the topic and deletes the timer" do + topic = create_topic + + topic.set_or_create_timer( + TopicTimer.types[:open], 10.hours.from_now + ) + + TopicStatusUpdater.new(topic, admin).update!("closed", false) + timer = TopicTimer.find_by(topic: topic) + expect(timer).to eq(nil) + end + + context "when the category has auto close settings" do + let(:topic) { create_topic } + let(:based_on_last_post) { false } + + before do + # auto close after 3 days, topic was created a day ago + topic.update( + category: Fabricate(:category, auto_close_hours: 72, auto_close_based_on_last_post: based_on_last_post), + created_at: 1.day.ago + ) + end + + it "inherits auto close from the topic category, based on the created_at date of the topic" do + + # close the topic manually, and set a timer to automatically open + TopicStatusUpdater.new(topic, admin).update!("closed", true) + topic.set_or_create_timer( + TopicTimer.types[:open], 10.hours.from_now + ) + + # manually open the topic. it has been 1 days since creation so the + # topic should auto-close 2 days from now, the original auto close time + TopicStatusUpdater.new(topic, admin).update!("closed", false) + + timer = TopicTimer.find_by(topic: topic) + expect(timer).not_to eq(nil) + expect(timer.execute_at).to be_within_one_second_of(topic.created_at + 72.hours) + end + + it "does not inherit auto close from the topic category if it has already been X hours since topic creation" do + + topic.category.update(auto_close_hours: 1) + + # close the topic manually, and set a timer to automatically open + TopicStatusUpdater.new(topic, admin).update!("closed", true) + topic.set_or_create_timer( + TopicTimer.types[:open], 10.hours.from_now + ) + + # manually open the topic. it has been over a day since creation and + # the auto close hours was 1 so a new timer should not be made + TopicStatusUpdater.new(topic, admin).update!("closed", false) + + timer = TopicTimer.find_by(topic: topic) + expect(timer).to eq(nil) + end + + context "when category setting is based_on_last_post" do + let(:based_on_last_post) { true } + + it "inherits auto close from the topic category, using the duration because the close is based_on_last_post" do + + # close the topic manually, and set a timer to automatically open + TopicStatusUpdater.new(topic, admin).update!("closed", true) + topic.set_or_create_timer( + TopicTimer.types[:open], 10.hours.from_now + ) + + # manually open the topic. it should re open 3 days from now, NOT + # 3 days from creation + TopicStatusUpdater.new(topic, admin).update!("closed", false) + + timer = TopicTimer.find_by(topic: topic) + expect(timer).not_to eq(nil) + expect(timer.duration_minutes).to eq(72 * 60) + expect(timer.execute_at).to be_within_one_second_of(Time.zone.now + 72.hours) + end + end + end + end + describe "repeat actions" do shared_examples "an action that doesn't repeat" do