DEV: Small refactors for future work (#332)

jobs/regular/assign_notification.rb

@@ -8,7 +8,7 @@ module Jobs
       raise Discourse::InvalidParameters.new(:assigned_to_id) if args[:assigned_to_id].nil?
       raise Discourse::InvalidParameters.new(:assigned_to_type) if args[:assigned_to_type].nil?
       raise Discourse::InvalidParameters.new(:assigned_by_id) if args[:assigned_by_id].nil?
-      raise Discourse::InvalidParameters.new(:silent) if args[:silent].nil?
+      raise Discourse::InvalidParameters.new(:skip_small_action_post) if args[:skip_small_action_post].nil?
 
       topic = Topic.find(args[:topic_id])
       post = Post.find(args[:post_id])
@@ -37,7 +37,7 @@ module Jobs
           )
         )
 
-        next if args[:silent]
+        next if args[:skip_small_action_post]
         Notification.create!(
           notification_type: Notification.types[:assigned] || Notification.types[:custom],
           user_id: user.id,

lib/assigner.rb

@@ -69,9 +69,9 @@ class ::Assigner
       if is_last_staff_post?(post)
         assigner = new(post.topic, post.user)
         if assign_other
-          assigner.assign(assign_other, silent: true)
+          assigner.assign(assign_other, skip_small_action_post: true)
         elsif assign_self
-          assigner.assign(assign_self, silent: true)
+          assigner.assign(assign_self, skip_small_action_post: true)
         end
       end
     end
@@ -199,47 +199,33 @@ class ::Assigner
     end
   end
 
-  def assign(assign_to, note: nil, silent: false)
+  def assign(assign_to, note: nil, skip_small_action_post: false)
-    type = assign_to.is_a?(User) ? "User" : "Group"
+    assigned_to_type = assign_to.is_a?(User) ? "User" : "Group"
 
-    forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type, note: note)
+    forbidden_reason = forbidden_reasons(assign_to: assign_to, type: assigned_to_type, note: note)
     return { success: false, reason: forbidden_reason } if forbidden_reason
 
     action_code = {}
     action_code[:user] = topic.assignment.present? ? "reassigned" : "assigned"
     action_code[:group] = topic.assignment.present? ? "reassigned_group" : "assigned_group"
 
-    silent = silent || no_assignee_change?(assign_to)
+    skip_small_action_post = skip_small_action_post || no_assignee_change?(assign_to)
 
     @target.assignment&.destroy!
 
-    assignment = @target.create_assignment!(assigned_to_id: assign_to.id, assigned_to_type: type, assigned_by_user_id: @assigned_by.id, topic_id: topic.id, note: note)
+    assignment = @target.create_assignment!(assigned_to_id: assign_to.id, assigned_to_type: assigned_to_type, assigned_by_user_id: @assigned_by.id, topic_id: topic.id, note: note)
 
     first_post.publish_change_to_clients!(:revised, reload_topic: true)
 
-    serializer = assignment.assigned_to_user? ? BasicUserSerializer : BasicGroupSerializer
-
     Jobs.enqueue(:assign_notification,
                  topic_id: topic.id,
                  post_id: topic_target? ? first_post.id : @target.id,
                  assigned_to_id: assign_to.id,
-                 assigned_to_type: type,
+                 assigned_to_type: assigned_to_type,
                  assigned_by_id: @assigned_by.id,
-                 silent: silent)
+                 skip_small_action_post: skip_small_action_post)
 
-    MessageBus.publish(
+    publish_assignment(assignment, assign_to, note)
-      "/staff/topic-assignment",
-      {
-        type: "assigned",
-        topic_id: topic.id,
-        post_id: post_target? && @target.id,
-        post_number: post_target? && @target.post_number,
-        assigned_type: type,
-        assigned_to: serializer.new(assign_to, scope: Guardian.new, root: false).as_json,
-        assignment_note: note,
-      },
-      user_ids: allowed_user_ids
-    )
 
     if assignment.assigned_to_user?
       if !TopicUser.exists?(
@@ -262,7 +248,8 @@ class ::Assigner
         end
       end
     end
-    if !silent
+
+    unless skip_small_action_post
       custom_fields = { "action_code_who" => assign_to.is_a?(User) ? assign_to.username : assign_to.name }
 
       if post_target?
@@ -281,9 +268,9 @@ class ::Assigner
 
     # Create a webhook event
     if WebHook.active_web_hooks(:assign).exists?
-      type = :assigned
+      assigned_to_type = :assigned
       payload = {
-        type: type,
+        type: assigned_to_type,
         topic_id: topic.id,
         topic_title: topic.title,
         assigned_by_id: @assigned_by.id,
@@ -300,7 +287,7 @@ class ::Assigner
           assigned_to_group_name: assign_to.name,
         })
       end
-      WebHook.enqueue_assign_hooks(type, payload.to_json)
+      WebHook.enqueue_assign_hooks(assigned_to_type, payload.to_json)
     end
 
     { success: true }
@@ -398,6 +385,23 @@ class ::Assigner
 
   private
 
+  def publish_assignment(assignment, assign_to, note)
+    serializer = assignment.assigned_to_user? ? BasicUserSerializer : BasicGroupSerializer
+    MessageBus.publish(
+      "/staff/topic-assignment",
+      {
+        type: "assigned",
+        topic_id: topic.id,
+        post_id: post_target? && @target.id,
+        post_number: post_target? && @target.post_number,
+        assigned_type: assignment.assigned_to_type,
+        assigned_to: serializer.new(assign_to, scope: Guardian.new, root: false).as_json,
+        assignment_note: note,
+      },
+      user_ids: allowed_user_ids
+    )
+  end
+
   def moderator_post_assign_action_code(assignment, action_code)
     if assignment.target.is_a?(Post)
       # posts do not have to handle conditions of 'assign' or 'reassign'

plugin.rb

@@ -816,7 +816,7 @@ after_initialize do
                    assigned_to_id: assignment.assigned_to_id,
                    assigned_to_type: assignment.assigned_to_type,
                    assigned_by_id: assignment.assigned_by_user_id,
-                   silent: true)
+                   skip_small_action_post: true)
     end
   end
 

spec/jobs/regular/assign_notification_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe Jobs::AssignNotification do
 
       it 'sends notification alert' do
         messages = MessageBus.track_publish("/notification-alert/#{user2.id}") do
-          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false })
+          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, skip_small_action_post: false })
         end
 
         expect(messages.length).to eq(1)
@@ -41,7 +41,7 @@ RSpec.describe Jobs::AssignNotification do
         assign_allowed_group.add(user)
 
         assert_publish_topic_state(pm, user) do
-          described_class.new.execute({ topic_id: pm.id, post_id: pm_post.id, assigned_to_id: pm.allowed_users.first.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false })
+          described_class.new.execute({ topic_id: pm.id, post_id: pm_post.id, assigned_to_id: pm.allowed_users.first.id, assigned_to_type: 'User', assigned_by_id: user1.id, skip_small_action_post: false })
         end
       end
 
@@ -58,7 +58,7 @@ RSpec.describe Jobs::AssignNotification do
             topic_title: topic.title
           }.to_json
         )
-        described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false })
+        described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, skip_small_action_post: false })
       end
     end
 
@@ -76,19 +76,19 @@ RSpec.describe Jobs::AssignNotification do
 
       it 'sends notification alert to all group members' do
         messages = MessageBus.track_publish("/notification-alert/#{user2.id}") do
-          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false })
+          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, skip_small_action_post: false })
         end
         expect(messages.length).to eq(1)
         expect(messages.first.data[:excerpt]).to eq("assigned to Developers the topic 'Basic topic title'")
 
         messages = MessageBus.track_publish("/notification-alert/#{user3.id}") do
-          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false })
+          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, skip_small_action_post: false })
         end
         expect(messages.length).to eq(1)
         expect(messages.first.data[:excerpt]).to eq("assigned to Developers the topic 'Basic topic title'")
 
         messages = MessageBus.track_publish("/notification-alert/#{user4.id}") do
-          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false })
+          described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, skip_small_action_post: false })
         end
         expect(messages.length).to eq(0)
       end
@@ -109,7 +109,7 @@ RSpec.describe Jobs::AssignNotification do
           )
         end
 
-        described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false })
+        described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, skip_small_action_post: false })
       end
     end
   end

spec/jobs/regular/unassign_notification_spec.rb

@@ -27,7 +27,7 @@ RSpec.describe Jobs::UnassignNotification do
 
     context 'User' do
       it 'deletes notifications' do
-        Jobs::AssignNotification.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, silent: false })
+        Jobs::AssignNotification.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User', assigned_by_id: user1.id, skip_small_action_post: false })
 
         expect {
           described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: user2.id, assigned_to_type: 'User' })
@@ -55,7 +55,7 @@ RSpec.describe Jobs::UnassignNotification do
       end
 
       it 'deletes notifications' do
-        Jobs::AssignNotification.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, silent: false })
+        Jobs::AssignNotification.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group', assigned_by_id: user1.id, skip_small_action_post: false })
 
         expect {
           described_class.new.execute({ topic_id: topic.id, post_id: post.id, assigned_to_id: group.id, assigned_to_type: 'Group' })

spec/lib/assigner_spec.rb

@@ -147,8 +147,6 @@ RSpec.describe Assigner do
       assigner.assign(assignee).fetch(:success)
     end
 
-    fab!(:admin) { Fabricate(:admin) }
-
     context "forbidden reasons" do
       it "doesn't assign if the user has too many assigned topics" do
         SiteSetting.max_assigned_topics = 1
@@ -203,7 +201,7 @@ RSpec.describe Assigner do
       end
 
       it 'fails to assign when the assigned user and note is the same' do
-        assigner = described_class.new(topic, admin)
+        assigner = described_class.new(topic, moderator_2)
         assigner.assign(moderator, note: "note me down")
 
         assign = assigner.assign(moderator, note: "note me down")
@@ -213,7 +211,7 @@ RSpec.describe Assigner do
       end
 
       it 'allows assign when the assigned user is same but note is different' do
-        assigner = described_class.new(topic, admin)
+        assigner = described_class.new(topic, moderator_2)
         assigner.assign(moderator, note: "note me down")
 
         assign = assigner.assign(moderator, note: "note me down again")
@@ -222,28 +220,28 @@ RSpec.describe Assigner do
       end
 
       it 'fails to assign when the assigned user cannot view the pm' do
-        assign = described_class.new(pm, admin).assign(moderator)
+        assign = described_class.new(pm, moderator_2).assign(moderator)
 
         expect(assign[:success]).to eq(false)
         expect(assign[:reason]).to eq(:forbidden_assignee_not_pm_participant)
       end
 
       it 'fails to assign when not all group members has access to pm' do
-        assign = described_class.new(pm, admin).assign(moderator.groups.first)
+        assign = described_class.new(pm, moderator_2).assign(moderator.groups.first)
 
         expect(assign[:success]).to eq(false)
         expect(assign[:reason]).to eq(:forbidden_group_assignee_not_pm_participant)
       end
 
       it 'fails to assign when the assigned user cannot view the topic' do
-        assign = described_class.new(secure_topic, admin).assign(moderator)
+        assign = described_class.new(secure_topic, moderator_2).assign(moderator)
 
         expect(assign[:success]).to eq(false)
         expect(assign[:reason]).to eq(:forbidden_assignee_cant_see_topic)
       end
 
       it 'fails to assign when the not all group members can view the topic' do
-        assign = described_class.new(secure_topic, admin).assign(moderator.groups.first)
+        assign = described_class.new(secure_topic, moderator_2).assign(moderator.groups.first)
 
         expect(assign[:success]).to eq(false)
         expect(assign[:reason]).to eq(:forbidden_group_assignee_cant_see_topic)
@@ -253,7 +251,7 @@ RSpec.describe Assigner do
     it "assigns the PM to the moderator when it's included in the list of allowed users" do
       pm.allowed_users << moderator
 
-      assign = described_class.new(pm, admin).assign(moderator)
+      assign = described_class.new(pm, moderator_2).assign(moderator)
 
       expect(assign[:success]).to eq(true)
     end
@@ -261,7 +259,7 @@ RSpec.describe Assigner do
     it "assigns the PM to the moderator when it's a member of an allowed group" do
       pm.allowed_groups << assign_allowed_group
 
-      assign = described_class.new(pm, admin).assign(moderator)
+      assign = described_class.new(pm, moderator_2).assign(moderator)
 
       expect(assign[:success]).to eq(true)
     end