From 2654a6685cea512ef8e3f8aea29079dc617775fd Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 24 Jun 2021 11:35:36 +0200 Subject: [PATCH] DEV: adds support for bannered until (#13417) ATM it only implements server side of it, as my need is for automation purposes. However it should probably be added in the UI too as it's unexpected to have pinned_until and no bannered_until. --- app/controllers/topics_controller.rb | 2 + app/jobs/regular/remove_banner.rb | 18 +++++++ app/jobs/regular/unpin_topic.rb | 2 +- app/models/topic.rb | 35 ++++++++++--- .../20210621103509_add_bannered_until.rb | 9 ++++ ...24080131_add_partial_index_pinned_until.rb | 11 ++++ spec/jobs/remove_banner_spec.rb | 51 +++++++++++++++++++ spec/models/topic_spec.rb | 18 +++++++ 8 files changed, 137 insertions(+), 9 deletions(-) create mode 100644 app/jobs/regular/remove_banner.rb create mode 100644 db/migrate/20210621103509_add_bannered_until.rb create mode 100644 db/migrate/20210624080131_add_partial_index_pinned_until.rb create mode 100644 spec/jobs/remove_banner_spec.rb diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 7f5e36a4570..7089f89ee01 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -435,6 +435,8 @@ class TopicsController < ApplicationController guardian.ensure_can_moderate!(@topic) end + params[:until] === '' ? params[:until] = nil : params[:until] + @topic.update_status(status, enabled, current_user, until: params[:until]) render json: success_json.merge!( diff --git a/app/jobs/regular/remove_banner.rb b/app/jobs/regular/remove_banner.rb new file mode 100644 index 00000000000..880c8696c94 --- /dev/null +++ b/app/jobs/regular/remove_banner.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module Jobs + + class RemoveBanner < ::Jobs::Base + + def execute(args) + topic_id = args[:topic_id] + + return unless topic_id.present? + + topic = Topic.find_by(id: topic_id) + topic.remove_banner!(Discourse.system_user) if topic.present? + end + + end + +end diff --git a/app/jobs/regular/unpin_topic.rb b/app/jobs/regular/unpin_topic.rb index b99e505904b..9d4f6b93eed 100644 --- a/app/jobs/regular/unpin_topic.rb +++ b/app/jobs/regular/unpin_topic.rb @@ -7,7 +7,7 @@ module Jobs def execute(args) topic_id = args[:topic_id] - raise Discourse::InvalidParameters.new(:topic_id) unless topic_id.present? + return unless topic_id.present? topic = Topic.find_by(id: topic_id) topic.update_pinned(false) if topic.present? diff --git a/app/models/topic.rb b/app/models/topic.rb index 32c004ed0ad..00a3eacaa80 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1093,7 +1093,15 @@ class Topic < ActiveRecord::Base @participants_summary ||= TopicParticipantsSummary.new(self, options).summary end - def make_banner!(user) + def make_banner!(user, bannered_until = nil) + if bannered_until + bannered_until = begin + Time.parse(bannered_until) + rescue ArgumentError + raise Discourse::InvalidParameters.new(:bannered_until) + end + end + # only one banner at the same time previous_banner = Topic.where(archetype: Archetype.banner).first previous_banner.remove_banner!(user) if previous_banner.present? @@ -1102,18 +1110,25 @@ class Topic < ActiveRecord::Base .update_all(dismissed_banner_key: nil) self.archetype = Archetype.banner + self.bannered_until = bannered_until self.add_small_action(user, "banner.enabled") self.save MessageBus.publish('/site/banner', banner) + + Jobs.cancel_scheduled_job(:remove_banner, topic_id: self.id) + Jobs.enqueue_at(bannered_until, :remove_banner, topic_id: self.id) if bannered_until end def remove_banner!(user) self.archetype = Archetype.default + self.bannered_until = nil self.add_small_action(user, "banner.disabled") self.save MessageBus.publish('/site/banner', nil) + + Jobs.cancel_scheduled_job(:remove_banner, topic_id: self.id) end def banner @@ -1199,12 +1214,13 @@ class Topic < ActiveRecord::Base TopicUser.change(user.id, id, cleared_pinned_at: nil) end - def update_pinned(status, global = false, pinned_until = "") - pinned_until ||= '' - - pinned_until = begin - Time.parse(pinned_until) - rescue ArgumentError + def update_pinned(status, global = false, pinned_until = nil) + if pinned_until + pinned_until = begin + Time.parse(pinned_until) + rescue ArgumentError + raise Discourse::InvalidParameters.new(:pinned_until) + end end update_columns( @@ -1233,7 +1249,10 @@ class Topic < ActiveRecord::Base def self.ensure_consistency! # unpin topics that might have been missed - Topic.where("pinned_until < now()").update_all(pinned_at: nil, pinned_globally: false, pinned_until: nil) + Topic.where('pinned_until < ?', Time.now).update_all(pinned_at: nil, pinned_globally: false, pinned_until: nil) + Topic.where('bannered_until < ?', Time.now).find_each do |topic| + topic.remove_banner!(Discourse.system_user) + end end def inherit_auto_close_from_category(timer_type: :close) diff --git a/db/migrate/20210621103509_add_bannered_until.rb b/db/migrate/20210621103509_add_bannered_until.rb new file mode 100644 index 00000000000..6a48cf416e1 --- /dev/null +++ b/db/migrate/20210621103509_add_bannered_until.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddBanneredUntil < ActiveRecord::Migration[6.1] + def change + add_column :topics, :bannered_until, :datetime, null: true + + add_index :topics, :bannered_until, where: 'bannered_until IS NOT NULL' + end +end diff --git a/db/migrate/20210624080131_add_partial_index_pinned_until.rb b/db/migrate/20210624080131_add_partial_index_pinned_until.rb new file mode 100644 index 00000000000..c91cdb45236 --- /dev/null +++ b/db/migrate/20210624080131_add_partial_index_pinned_until.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +class AddPartialIndexPinnedUntil < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def change + add_index :topics, :pinned_until, + where: 'pinned_until IS NOT NULL', + algorithm: :concurrently + end +end diff --git a/spec/jobs/remove_banner_spec.rb b/spec/jobs/remove_banner_spec.rb new file mode 100644 index 00000000000..13c60b87e93 --- /dev/null +++ b/spec/jobs/remove_banner_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Jobs::RemoveBanner do + fab!(:topic) { Fabricate(:topic) } + fab!(:user) { topic.user } + + context 'topic is not bannered until' do + it 'doesn’t enqueue a future job to remove it' do + expect do + topic.make_banner!(user) + end.to change { Jobs::RemoveBanner.jobs.size }.by(0) + end + end + + context 'topic is bannered until' do + context 'bannered_until is a valid date' do + it 'enqueues a future job to remove it' do + bannered_until = 5.days.from_now + + expect(topic.archetype).to eq(Archetype.default) + + expect do + topic.make_banner!(user, bannered_until.to_s) + end.to change { Jobs::RemoveBanner.jobs.size }.by(1) + + topic.reload + expect(topic.archetype).to eq(Archetype.banner) + + job = Jobs::RemoveBanner.jobs[0] + expect(Time.at(job['at'])).to be_within_one_minute_of(bannered_until) + expect(job['args'][0]['topic_id']).to eq(topic.id) + + job['class'].constantize.new.perform(*job['args']) + topic.reload + expect(topic.archetype).to eq(Archetype.default) + end + end + + context 'bannered_until is an invalid date' do + it 'doesn’t enqueue a future job to remove it' do + expect do + expect do + topic.make_banner!(user, 'xxx') + end.to raise_error(Discourse::InvalidParameters) + end.to change { Jobs::RemoveBanner.jobs.size }.by(0) + end + end + end +end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index b797e1cb07b..efd5a6026ca 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1380,6 +1380,24 @@ describe Topic do end + context "bannered_until date" do + + it 'sets bannered_until to be caught by ensure_consistency' do + bannered_until = 5.days.from_now + topic.make_banner!(user, bannered_until.to_s) + + freeze_time 6.days.from_now do + expect(topic.archetype).to eq(Archetype.banner) + + Topic.ensure_consistency! + topic.reload + + expect(topic.archetype).to eq(Archetype.default) + end + end + + end + end context 'last_poster info' do