FIX: Recreate notifications on topic reopening

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

@@ -23,20 +23,30 @@ class Assignment < ActiveRecord::Base
 
   validate :validate_status, if: -> { SiteSetting.enable_assign_status }
 
-  def self.valid_type?(type)
+  class << self
-    VALID_TYPES.include?(type.downcase)
+    def valid_type?(type)
-  end
+      VALID_TYPES.include?(type.downcase)
+    end
 
-  def self.statuses
+    def statuses
-    SiteSetting.assign_statuses.split("|")
+      SiteSetting.assign_statuses.split("|")
-  end
+    end
 
-  def self.default_status
+    def default_status
-    Assignment.statuses.first
+      Assignment.statuses.first
-  end
+    end
 
-  def self.status_enabled?
+    def status_enabled?
-    SiteSetting.enable_assign_status
+      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

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.inactive.exists?(topic: topic)
-      Assignment.where(topic_id: topic.id, target_type: "Topic").update_all(active: true)
+      Assignment.reactivate!(topic: topic)
-      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)
       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)
+    if Assignment.active.exists?(target: post)
-      Assignment.where(target_type: "Post", target_id: post.id).update_all(active: false)
+      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 &&
+    if SiteSetting.reassign_on_open && Assignment.inactive.exists?(target: post)
-         Assignment.where(target_type: "Post", target_id: post.id, active: false)
+      post.assignment.reactivate!
-      Assignment.where(target_type: "Post", target_id: post.id).update_all(active: true)
       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
+    Assignment.reactivate!(topic: topic)
-      .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
   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
+    Assignment.deactivate!(topic: topic)
-      .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
   end
 
   on(:user_added_to_group) do |user, group, automatic:|

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

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