From 896d0e34458c5a0ee09add7d888f647875783cd2 Mon Sep 17 00:00:00 2001
From: Jan Cernik <66427541+jancernik@users.noreply.github.com>
Date: Tue, 17 Jan 2023 08:09:35 -0300
Subject: [PATCH] FIX: Broken posts when assigning a closed topic (#425)

The previous behavior was:
1. Assign a topic to someone
2. Close the topic with the setting unassign_on_close enabled
3. Assign the topic to the same user as before

This caused small broken or empty posts to be displayed.

This commit also introduces system specs
---
 .../initializers/extend-for-assigns.js        |   5 +
 lib/assigner.rb                               |   6 +-
 spec/system/assign_topic_spec.rb              | 114 ++++++++++++++++++
 spec/system/page_objects/modals/assign.rb     |  27 +++++
 spec/system/page_objects/pages/topic.rb       |  37 ++++++
 5 files changed, 188 insertions(+), 1 deletion(-)
 create mode 100644 spec/system/assign_topic_spec.rb
 create mode 100644 spec/system/page_objects/modals/assign.rb
 create mode 100644 spec/system/page_objects/pages/topic.rb

diff --git a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js
index 392f29b..90a4cc6 100644
--- a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js
+++ b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js
@@ -811,6 +811,11 @@ function initialize(api) {
             });
             this.appEvents.trigger("post-stream:refresh", { id: data.post_id });
           }
+          if (topic.closed) {
+            this.appEvents.trigger("post-stream:refresh", {
+              id: topic.postStream.posts[0].id,
+            });
+          }
         }
         this.appEvents.trigger("header:update-topic", topic);
       });
diff --git a/lib/assigner.rb b/lib/assigner.rb
index 7f6478d..d3bf379 100644
--- a/lib/assigner.rb
+++ b/lib/assigner.rb
@@ -261,7 +261,7 @@ class ::Assigner
       forbidden_reasons(assign_to: assign_to, type: assigned_to_type, note: note, status: status)
     return { success: false, reason: forbidden_reason } if forbidden_reason
 
-    if no_assignee_change?(assign_to)
+    if no_assignee_change?(assign_to) && details_change?(note, status)
       return update_details(assign_to, note, status, skip_small_action_post: skip_small_action_post)
     end
 
@@ -536,6 +536,10 @@ class ::Assigner
     @target.assignment&.assigned_to_id == assignee.id
   end
 
+  def details_change?(note, status)
+    note.present? || @target.assignment&.status != status
+  end
+
   def assignment_eq?(assignment, assign_to, type, note, status)
     return false if !assignment&.active
     return false if assignment.assigned_to_id != assign_to.id
diff --git a/spec/system/assign_topic_spec.rb b/spec/system/assign_topic_spec.rb
new file mode 100644
index 0000000..455af25
--- /dev/null
+++ b/spec/system/assign_topic_spec.rb
@@ -0,0 +1,114 @@
+# frozen_string_literal: true
+
+describe "Assign | Assigning topics", type: :system, js: true do
+  let(:topic_page) { PageObjects::Pages::Topic.new }
+  let(:assign_modal) { PageObjects::Modals::Assign.new }
+  fab!(:staff_user) { Fabricate(:user, groups: [Group[:staff]]) }
+  fab!(:admin) { Fabricate(:admin) }
+  fab!(:topic) { Fabricate(:topic) }
+  fab!(:post) { Fabricate(:post, topic: topic) }
+
+  before do
+    SiteSetting.assign_enabled = true
+    sign_in(admin)
+  end
+
+  describe "with open topic" do
+    it "can assign and unassign" do
+      visit "/t/#{topic.id}"
+
+      topic_page.click_assign_topic
+      assign_modal.assignee = staff_user
+      assign_modal.confirm
+
+      expect(topic_page).to have_assigned(user: staff_user, at_post: 2)
+      expect(find("#topic .assigned-to")).to have_content(staff_user.username)
+
+      topic_page.click_unassign_topic
+
+      expect(topic_page).to have_unassigned(user: staff_user, at_post: 3)
+      expect(page).to have_no_css("#topic .assigned-to")
+    end
+
+    context "when unassign_on_close is set to true" do
+      before { SiteSetting.unassign_on_close = true }
+
+      it "unassigns the topic on close" do
+        visit "/t/#{topic.id}"
+
+        topic_page.click_assign_topic
+        assign_modal.assignee = staff_user
+        assign_modal.confirm
+
+        expect(topic_page).to have_assigned(user: staff_user, at_post: 2)
+
+        find(".topic-footer-main-buttons .toggle-admin-menu").click
+        find(".topic-admin-close").click
+
+        expect(find("#post_3")).to have_content(
+          I18n.t("js.action_codes.closed.enabled", when: "just now"),
+        )
+        expect(page).to have_no_css("#post_4")
+        expect(page).to have_no_css("#topic .assigned-to")
+      end
+
+      it "can assign the previous assignee" do
+        visit "/t/#{topic.id}"
+
+        topic_page.click_assign_topic
+        assign_modal.assignee = staff_user
+        assign_modal.confirm
+
+        expect(topic_page).to have_assigned(user: staff_user, at_post: 2)
+
+        find(".topic-footer-main-buttons .toggle-admin-menu").click
+        find(".topic-admin-close").click
+
+        expect(find("#post_3")).to have_content(
+          I18n.t("js.action_codes.closed.enabled", when: "just now"),
+        )
+        expect(page).to have_no_css("#post_4")
+        expect(page).to have_no_css("#topic .assigned-to")
+
+        topic_page.click_assign_topic
+        assign_modal.assignee = staff_user
+        assign_modal.confirm
+
+        expect(page).to have_no_css("#post_4")
+        expect(find("#topic .assigned-to")).to have_content(staff_user.username)
+      end
+
+      context "when reassign_on_open is set to true" do
+        before { SiteSetting.reassign_on_open = true }
+
+        it "reassigns the topic on open" do
+          visit "/t/#{topic.id}"
+
+          topic_page.click_assign_topic
+          assign_modal.assignee = staff_user
+          assign_modal.confirm
+
+          expect(topic_page).to have_assigned(user: staff_user, at_post: 2)
+
+          find(".topic-footer-main-buttons .toggle-admin-menu").click
+          find(".topic-admin-close").click
+
+          expect(find("#post_3")).to have_content(
+            I18n.t("js.action_codes.closed.enabled", when: "just now"),
+          )
+          expect(page).to have_no_css("#post_4")
+          expect(page).to have_no_css("#topic .assigned-to")
+
+          find(".topic-footer-main-buttons .toggle-admin-menu").click
+          find(".topic-admin-open").click
+
+          expect(find("#post_4")).to have_content(
+            I18n.t("js.action_codes.closed.disabled", when: "just now"),
+          )
+          expect(page).to have_no_css("#post_5")
+          expect(find("#topic .assigned-to")).to have_content(staff_user.username)
+        end
+      end
+    end
+  end
+end
diff --git a/spec/system/page_objects/modals/assign.rb b/spec/system/page_objects/modals/assign.rb
new file mode 100644
index 0000000..a03573f
--- /dev/null
+++ b/spec/system/page_objects/modals/assign.rb
@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Modals
+    class Assign < PageObjects::Modals::Base
+      def assignee=(assignee)
+        assignee = assignee.is_a?(Group) ? assignee.name : assignee.username
+        find(".control-group .multi-select").click
+        find(".control-group input").fill_in(with: assignee)
+        find("li[data-value='#{assignee}']").click
+      end
+
+      def status=(status)
+        find("#assign-status").click
+        find("[data-value='#{status}']").click
+      end
+
+      def note=(note)
+        find("#assign-modal-note").fill_in(with: note)
+      end
+
+      def confirm
+        find(".modal-footer .btn-primary").click
+      end
+    end
+  end
+end
diff --git a/spec/system/page_objects/pages/topic.rb b/spec/system/page_objects/pages/topic.rb
new file mode 100644
index 0000000..ab85b61
--- /dev/null
+++ b/spec/system/page_objects/pages/topic.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module PageObjects
+  module Pages
+    class Topic < PageObjects::Pages::Base
+      def click_assign_topic
+        find("#topic-footer-button-assign").click
+      end
+
+      def click_unassign_topic
+        find("#topic-footer-dropdown-reassign").click
+        find("[data-value='unassign']").click
+      end
+
+      def click_edit_topic_assignment
+        find("#topic-footer-dropdown-reassign").click
+        find("[data-value='reassign']").click
+      end
+
+      def has_assigned?(args)
+        has_assignment_action?(action: "assigned", **args)
+      end
+
+      def has_unassigned?(args)
+        has_assignment_action?(action: "unassigned", **args)
+      end
+
+      def has_assignment_action?(args)
+        assignee = args[:group]&.name || args[:user]&.username
+        container = args[:at_post] ? find("#post_#{args[:at_post]}") : page
+        container.has_content?(
+          I18n.t("js.action_codes.#{args[:action]}", who: "@#{assignee}", when: "just now"),
+        )
+      end
+    end
+  end
+end