From c696e4471401d116752a573a7382160c57ff8f41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Wed, 13 Mar 2024 17:59:09 +0100 Subject: [PATCH] FIX: Recreate notifications on topic reopening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, when reopening a topic that has assignments, the notifications in the user menu aren’t recreated. This patch fixes that issue. It also addresses the same type of issue with posts being destroyed and recovered. --- app/models/assignment.rb | 49 ++++++++++++++---- plugin.rb | 42 ++++------------ spec/models/assignment_spec.rb | 78 +++++++++++++++++++++++++++++ spec/plugin_spec.rb | 91 ++++++++++++++++++++++++++++++++++ 4 files changed, 216 insertions(+), 44 deletions(-) diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 58479e5..bf28baf 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -23,20 +23,30 @@ class Assignment < ActiveRecord::Base validate :validate_status, if: -> { SiteSetting.enable_assign_status } - def self.valid_type?(type) - VALID_TYPES.include?(type.downcase) - end + class << self + def valid_type?(type) + VALID_TYPES.include?(type.downcase) + end - def self.statuses - SiteSetting.assign_statuses.split("|") - end + def statuses + SiteSetting.assign_statuses.split("|") + end - def self.default_status - Assignment.statuses.first - end + def default_status + Assignment.statuses.first + end - def self.status_enabled? - SiteSetting.enable_assign_status + def status_enabled? + SiteSetting.enable_assign_status + end + + def deactivate!(topic:) + active.where(topic: topic).find_each(&:deactivate!) + end + + def reactivate!(topic:) + inactive.where(topic: topic).find_each(&:reactivate!) + end end def assigned_to_user? @@ -67,6 +77,23 @@ class Assignment < ActiveRecord::Base end end + def reactivate! + return unless target + update!(active: true) + Jobs.enqueue(:assign_notification, assignment_id: id) + end + + def deactivate! + update!(active: false) + Jobs.enqueue( + :unassign_notification, + topic_id: topic_id, + assigned_to_id: assigned_to_id, + assigned_to_type: assigned_to_type, + assignment_id: id, + ) + end + private def default_status diff --git a/plugin.rb b/plugin.rb index b95ed8f..bbee0e8 100644 --- a/plugin.rb +++ b/plugin.rb @@ -784,7 +784,7 @@ after_initialize do on(:topic_status_updated) do |topic, status, enabled| if SiteSetting.unassign_on_close && (status == "closed" || status == "autoclosed") && enabled && - Assignment.exists?(topic_id: topic.id, active: true) + Assignment.active.exists?(topic: topic) assigner = ::Assigner.new(topic, Discourse.system_user) assigner.unassign(silent: true, deactivate: true) @@ -799,19 +799,15 @@ after_initialize do end if SiteSetting.reassign_on_open && (status == "closed" || status == "autoclosed") && !enabled && - Assignment.exists?(topic_id: topic.id, active: false) - Assignment.where(topic_id: topic.id, target_type: "Topic").update_all(active: true) - Assignment - .where(topic_id: topic.id, target_type: "Post") - .joins("INNER JOIN posts ON posts.id = target_id AND posts.deleted_at IS NULL") - .update_all(active: true) + Assignment.inactive.exists?(topic: topic) + Assignment.reactivate!(topic: topic) MessageBus.publish("/topic/#{topic.id}", reload_topic: true, refresh_stream: true) end end on(:post_destroyed) do |post| - if Assignment.exists?(target_type: "Post", target_id: post.id, active: true) - Assignment.where(target_type: "Post", target_id: post.id).update_all(active: false) + if Assignment.active.exists?(target: post) + post.assignment.deactivate! MessageBus.publish("/topic/#{post.topic_id}", reload_topic: true, refresh_stream: true) end @@ -830,9 +826,8 @@ after_initialize do end on(:post_recovered) do |post| - if SiteSetting.reassign_on_open && - Assignment.where(target_type: "Post", target_id: post.id, active: false) - Assignment.where(target_type: "Post", target_id: post.id).update_all(active: true) + if SiteSetting.reassign_on_open && Assignment.inactive.exists?(target: post) + post.assignment.reactivate! MessageBus.publish("/topic/#{post.topic_id}", reload_topic: true, refresh_stream: true) end end @@ -847,14 +842,7 @@ after_initialize do next if !SiteSetting.unassign_on_group_archive next if !info[:group] - Assignment - .inactive - .where(topic: topic) - .find_each do |assignment| - next unless assignment.target - assignment.update!(active: true) - Jobs.enqueue(:assign_notification, assignment_id: assignment.id) - end + Assignment.reactivate!(topic: topic) end on(:archive_message) do |info| @@ -866,19 +854,7 @@ after_initialize do next if !SiteSetting.unassign_on_group_archive next if !info[:group] - Assignment - .active - .where(topic: topic) - .find_each do |assignment| - assignment.update!(active: false) - Jobs.enqueue( - :unassign_notification, - topic_id: topic.id, - assigned_to_id: assignment.assigned_to_id, - assigned_to_type: assignment.assigned_to_type, - assignment_id: assignment.id, - ) - end + Assignment.deactivate!(topic: topic) end on(:user_added_to_group) do |user, group, automatic:| diff --git a/spec/models/assignment_spec.rb b/spec/models/assignment_spec.rb index 1b29305..f3c4188 100644 --- a/spec/models/assignment_spec.rb +++ b/spec/models/assignment_spec.rb @@ -28,6 +28,36 @@ RSpec.describe Assignment do end end + describe ".deactivate!" do + subject(:deactivate!) { described_class.deactivate!(topic: topic) } + + let!(:assignment1) { Fabricate(:topic_assignment) } + let!(:assignment2) { Fabricate(:post_assignment, topic: topic) } + let!(:assignment3) { Fabricate(:post_assignment) } + let(:topic) { assignment1.topic } + + it "deactivates each assignment of the provided topic" do + deactivate! + expect([assignment1, assignment2].map(&:reload).map(&:active?)).to all eq false + expect(assignment3.reload).to be_active + end + end + + describe ".reactivate!" do + subject(:reactivate!) { described_class.reactivate!(topic: topic) } + + let!(:assignment1) { Fabricate(:topic_assignment, active: false) } + let!(:assignment2) { Fabricate(:post_assignment, topic: topic, active: false) } + let!(:assignment3) { Fabricate(:post_assignment, active: false) } + let(:topic) { assignment1.topic } + + it "reactivates each assignment of the provided topic" do + reactivate! + expect([assignment1, assignment2].map(&:reload)).to all be_active + expect(assignment3.reload).not_to be_active + end + end + describe "#assigned_users" do subject(:assigned_users) { assignment.assigned_users } @@ -170,4 +200,52 @@ RSpec.describe Assignment do end end end + + describe "#reactivate!" do + subject(:reactivate!) { assignment.reactivate! } + + fab!(:assignment) { Fabricate.create(:topic_assignment, active: false) } + + context "when target does not exist" do + before { assignment.target.delete } + + it "does nothing" do + expect { reactivate! }.not_to change { assignment.reload.active } + end + end + + context "when target exists" do + it "sets the assignment as active" do + expect { reactivate! }.to change { assignment.reload.active? }.to true + end + + it "enqueues a job to create notifications" do + reactivate! + expect_job_enqueued(job: Jobs::AssignNotification, args: { assignment_id: assignment.id }) + end + end + end + + describe "#deactivate!" do + subject(:deactivate!) { assignment.deactivate! } + + fab!(:assignment) { Fabricate.create(:topic_assignment) } + + it "sets the assignment as inactive" do + expect { deactivate! }.to change { assignment.reload.active? }.to false + end + + it "enqueues a job to delete notifications" do + deactivate! + expect_job_enqueued( + job: Jobs::UnassignNotification, + args: { + topic_id: assignment.topic_id, + assigned_to_id: assignment.assigned_to_id, + assigned_to_type: assignment.assigned_to_type, + assignment_id: assignment.id, + }, + ) + end + end end diff --git a/spec/plugin_spec.rb b/spec/plugin_spec.rb index 1537b1f..fd69deb 100644 --- a/spec/plugin_spec.rb +++ b/spec/plugin_spec.rb @@ -55,5 +55,96 @@ RSpec.describe DiscourseAssign do ).to eq(false) end end + + describe "on 'topic_status_updated'" do + context "when closing a topic" do + let!(:first_assignment) { Fabricate(:topic_assignment) } + let!(:second_assignment) { Fabricate(:post_assignment, topic: topic) } + let(:topic) { first_assignment.topic } + + before do + SiteSetting.unassign_on_close = true + topic.update_status("closed", true, Discourse.system_user) + end + + it "deactivates existing assignments" do + [first_assignment, second_assignment].each do |assignment| + assignment.reload + expect(assignment).not_to be_active + expect_job_enqueued( + job: Jobs::UnassignNotification, + args: { + topic_id: assignment.topic_id, + assignment_id: assignment.id, + assigned_to_id: assignment.assigned_to_id, + assigned_to_type: assignment.assigned_to_type, + }, + ) + end + end + end + + context "when reopening a topic" do + let!(:topic) { Fabricate(:closed_topic) } + let!(:first_assignment) { Fabricate(:topic_assignment, topic: topic, active: false) } + let!(:second_assignment) { Fabricate(:post_assignment, topic: topic, active: false) } + + before do + SiteSetting.reassign_on_open = true + topic.update_status("closed", false, Discourse.system_user) + end + + it "reactivates existing assignments" do + [first_assignment, second_assignment].each do |assignment| + assignment.reload + expect(assignment).to be_active + expect_job_enqueued( + job: Jobs::AssignNotification, + args: { + assignment_id: assignment.id, + }, + ) + end + end + end + end + + describe "on 'post_destroyed'" do + let!(:assignment) { Fabricate(:post_assignment) } + let(:post) { assignment.target } + + before { PostDestroyer.new(Discourse.system_user, post).destroy } + + it "deactivates the existing assignment" do + assignment.reload + expect(assignment).not_to be_active + expect_job_enqueued( + job: Jobs::UnassignNotification, + args: { + topic_id: assignment.topic_id, + assignment_id: assignment.id, + assigned_to_id: assignment.assigned_to_id, + assigned_to_type: assignment.assigned_to_type, + }, + ) + end + end + + describe "on 'post_recovered'" do + let!(:assignment) { Fabricate(:post_assignment, active: false) } + let(:post) { assignment.target } + + before do + SiteSetting.reassign_on_open = true + post.trash! + PostDestroyer.new(Discourse.system_user, post).recover + end + + it "reactivates the existing assignment" do + assignment.reload + expect(assignment).to be_active + expect_job_enqueued(job: Jobs::AssignNotification, args: { assignment_id: assignment.id }) + end + end end end