FIX: Refactor assign / reassign to combine logic (#250)

* Refactor and combine assign / reassign logic

app/controllers/discourse_assign/assign_controller.rb

@@ -44,11 +44,25 @@ module DiscourseAssign
     end
 
     def assign
-      reassign_or_assign_target(action: "assign")
-    end
+      target_id = params.require(:target_id)
+      target_type = params.require(:target_type)
+      username = params.permit(:username)['username']
+      group_name = params.permit(:group_name)['group_name']
 
-    def reassign
-      reassign_or_assign_target(action: "reassign")
+      assign_to = username.present? ? User.find_by(username_lower: username.downcase) : Group.where("LOWER(name) = ?", group_name.downcase).first
+
+      raise Discourse::NotFound unless assign_to
+      raise Discourse::NotFound if !Assignment.valid_type?(target_type)
+      target = target_type.constantize.where(id: target_id).first
+      raise Discourse::NotFound unless target
+
+      assign = Assigner.new(target, current_user).assign(assign_to)
+
+      if assign[:success]
+        render json: success_json
+      else
+        render json: translate_failure(assign[:reason], assign_to), status: 400
+      end
     end
 
     def assigned
@@ -169,29 +183,5 @@ module DiscourseAssign
     def ensure_assign_allowed
       raise Discourse::InvalidAccess.new unless current_user.can_assign?
     end
-
-    def reassign_or_assign_target(action:)
-      target_id = params.require(:target_id)
-      target_type = params.require(:target_type)
-      username = params.permit(:username)['username']
-      group_name = params.permit(:group_name)['group_name']
-
-      assign_to = username.present? ? User.find_by(username_lower: username.downcase) : Group.where("LOWER(name) = ?", group_name.downcase).first
-
-      raise Discourse::NotFound unless assign_to
-      raise Discourse::NotFound if !Assignment.valid_type?(target_type)
-      target = target_type.constantize.where(id: target_id).first
-      raise Discourse::NotFound unless target
-
-      # perhaps?
-      #Scheduler::Defer.later "assign topic" do
-      assign = Assigner.new(target, current_user).send(action, assign_to)
-
-      if assign[:success]
-        render json: success_json
-      else
-        render json: translate_failure(assign[:reason], assign_to), status: 400
-      end
-    end
   end
 end

assets/javascripts/discourse-assign/controllers/assign-user.js.es6

@@ -38,12 +38,14 @@ export default Controller.extend({
     });
   },
 
-  reassignOrAssignTarget(assign_action) {
+  @action
+  assign() {
     if (this.isBulkAction) {
       this.bulkAction(this.model.username);
       return;
     }
-    let path = "/assign/" + assign_action;
+
+    let path = "/assign/assign";
 
     if (isEmpty(this.get("model.username"))) {
       this.model.target.set("assigned_to_user", null);
@@ -103,49 +105,8 @@ export default Controller.extend({
     }
   },
 
-  @action
-  assign() {
-    this.reassignOrAssignTarget("assign");
-  },
-
-  @action
-  reassign() {
-    this.reassignOrAssignTarget("reassign");
-  },
-
-  @action
-  reassignUser(name) {
-    if (this.isBulkAction) {
-      this.bulkAction(name);
-      return;
-    }
-
-    if (this.allowedGroupsForAssignment.includes(name)) {
-      this.setProperties({
-        "model.username": null,
-        "model.group_name": name,
-        "model.allowedGroups": this.taskActions.allowedGroups,
-      });
-    } else {
-      this.setProperties({
-        "model.username": name,
-        "model.group_name": null,
-        "model.allowedGroups": this.taskActions.allowedGroups,
-      });
-    }
-
-    if (name) {
-      return this.reassign();
-    }
-  },
-
   @action
   assignUsername(selected) {
     this.assignUser(selected.firstObject);
   },
-
-  @action
-  reassignUsername(selected) {
-    this.reassignUser(selected.firstObject);
-  },
 });

assets/javascripts/discourse-assign/initializers/extend-for-assigns.js.es6

@@ -86,11 +86,16 @@ function registerTopicFooterButtons(api) {
           break;
         }
         case "reassign": {
-          taskActions.reassign(this.topic).set("model.onSuccess", () => {
-            this.appEvents.trigger("post-stream:refresh", {
-              id: this.topic.postStream.firstPostId,
+          taskActions
+            .assign(this.topic, {
+              targetType: "Topic",
+              isAssigned: this.topic.isAssigned(),
+            })
+            .set("model.onSuccess", () => {
+              this.appEvents.trigger("post-stream:refresh", {
+                id: this.topic.postStream.firstPostId,
+              });
             });
-          });
           break;
         }
       }
@@ -387,11 +392,16 @@ function registerTopicFooterButtons(api) {
 
       const taskActions = getOwner(this).lookup("service:task-actions");
 
-      taskActions.reassign(this.topic).set("model.onSuccess", () => {
-        this.appEvents.trigger("post-stream:refresh", {
-          id: this.topic.postStream.firstPostId,
+      taskActions
+        .assign(this.topic, {
+          targetType: "Topic",
+          isAssigned: this.topic.isAssigned(),
+        })
+        .set("model.onSuccess", () => {
+          this.appEvents.trigger("post-stream:refresh", {
+            id: this.topic.postStream.firstPostId,
+          });
         });
-      });
     },
     dropdown() {
       return this.currentUser?.can_assign && this.topic.isAssigned();
@@ -460,7 +470,10 @@ function initialize(api) {
       });
       api.attachWidgetAction("post", "assignPost", function () {
         const taskActions = getOwner(this).lookup("service:task-actions");
-        taskActions.assign(this.model, "Post");
+        taskActions.assign(this.model, {
+          isAssigned: false,
+          targetType: "Post",
+        });
       });
       api.attachWidgetAction("post", "unassignPost", function () {
         const taskActions = getOwner(this).lookup("service:task-actions");

assets/javascripts/discourse/services/task-actions.js.es6

@@ -22,24 +22,28 @@ export default Service.extend({
     });
   },
 
-  assign(target, targetType = "Topic") {
+  assign(target, options = { isAssigned: false, targetType: "Topic" }) {
     return showModal("assign-user", {
-      title: "discourse_assign.assign" + this.i18nSuffix(targetType) + ".title",
+      title:
+        "discourse_assign.assign" +
+        this.i18nSuffix(options.targetType) +
+        `.${options.isAssigned ? "reassign_title" : "title"}`,
       model: {
         description:
-          "discourse_assign.assign" +
-          this.i18nSuffix(targetType) +
+          `discourse_assign.${options.isAssigned ? "reassign" : "assign"}` +
+          this.i18nSuffix(options.targetType) +
           ".description",
+        reassign: options.isAssigned,
         username: target.assigned_to_user?.username,
         group_name: target.assigned_to_group?.name,
         target,
-        targetType,
+        targetType: options.targetType,
       },
     });
   },
 
   reassignUserToTopic(user, target, targetType = "Topic") {
-    return ajax("/assign/reassign", {
+    return ajax("/assign/assign", {
       type: "PUT",
       data: {
         username: user.username,
@@ -48,24 +52,4 @@ export default Service.extend({
       },
     });
   },
-
-  reassign(target, targetType = "Topic") {
-    return showModal("assign-user", {
-      title:
-        "discourse_assign.assign" +
-        this.i18nSuffix(targetType) +
-        ".reassign_title",
-      model: {
-        description:
-          "discourse_assign.reassign" +
-          this.i18nSuffix(targetType) +
-          ".description",
-        reassign: true,
-        username: target.assigned_to_user?.username,
-        group_name: target.assigned_to_group?.name,
-        target,
-        targetType,
-      },
-    });
-  },
 });

assets/javascripts/discourse/templates/modal/assign-user.hbs

@@ -4,7 +4,7 @@
     {{email-group-user-chooser
       autocomplete="off"
       value=assigneeName
-      onChange=(action (if model.reassign "reassignUsername" "assignUsername"))
+      onChange=(action "assignUsername")
       autofocus="autofocus"
       options=(hash
         placementStrategy="absolute"
@@ -19,7 +19,7 @@
     }}
     <div class="assign-suggestions">
       {{#each assignSuggestions as |user|}}
-        <a href {{action (if model.reassign "reassignUser" "assignUser") user.username }}>
+        <a href {{action "assignUser" user.username }}>
           {{avatar user imageSize="small"}}
         </a>
       {{/each}}
@@ -31,7 +31,7 @@
   {{d-button
     label= (if model.reassign "discourse_assign.reassign.title" "discourse_assign.assign_modal.assign")
     icon=inviteIcon
-    action=(action (if model.reassign "reassign" "assign"))
+    action=(action "assign")
     class="btn-primary"
     disabled=disabled
   }}

config/routes.rb

@@ -3,7 +3,6 @@
 DiscourseAssign::Engine.routes.draw do
   put "/claim/:topic_id" => "assign#claim"
   put "/assign" => "assign#assign"
-  put "/reassign" => "assign#reassign"
   put "/unassign" => "assign#unassign"
   get "/suggestions" => "assign#suggestions"
   get "/assigned" => "assign#assigned"

lib/assigner.rb

@@ -198,7 +198,16 @@ class ::Assigner
       end
   end
 
-  def assign_or_reassign_target(assign_to:, type:, silent:, action_code:)
+  def assign(assign_to, silent: false)
+    type = assign_to.is_a?(User) ? "User" : "Group"
+
+    forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type)
+    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"
+
     @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)
@@ -295,26 +304,6 @@ class ::Assigner
     { success: true }
   end
 
-  def assign(assign_to, silent: false)
-    type = assign_to.is_a?(User) ? "User" : "Group"
-
-    forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type)
-    return { success: false, reason: forbidden_reason } if forbidden_reason
-
-    action_code = { user: "assigned", group: "assigned_group" }
-    assign_or_reassign_target(assign_to: assign_to, type: type, silent: silent, action_code: action_code)
-  end
-
-  def reassign(assign_to, silent: false)
-    type = assign_to.is_a?(User) ? "User" : "Group"
-
-    forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type)
-    return { success: false, reason: forbidden_reason } if forbidden_reason
-
-    action_code = { user: "reassigned", group: "reassigned_group" }
-    assign_or_reassign_target(assign_to: assign_to, type: type, silent: silent, action_code: action_code)
-  end
-
   def unassign(silent: false)
     if assignment = @target.assignment
       assignment.destroy!
@@ -406,14 +395,12 @@ class ::Assigner
   private
 
   def moderator_post_assign_action_code(assignment, action_code)
-    suffix =
-      if assignment.target.is_a?(Post)
-        "_to_post"
-      elsif assignment.target.is_a?(Topic)
-        ""
-      end
-    return "#{action_code[:user]}#{suffix}" if assignment.assigned_to_user?
-    return "#{action_code[:group]}#{suffix}" if assignment.assigned_to_group?
+    if assignment.target.is_a?(Post)
+      # posts do not have to handle conditions of 'assign' or 'reassign'
+      assignment.assigned_to_user? ? "assigned_to_post" : "assigned_group_to_post"
+    elsif assignment.target.is_a?(Topic)
+      assignment.assigned_to_user? ? "#{action_code[:user]}" : "#{action_code[:group]}"
+    end
   end
 
   def moderator_post_unassign_action_code(assignment)

lib/random_assign_utils.rb

@@ -4,7 +4,7 @@ class RandomAssignUtils
   def self.recently_assigned_users_ids(topic_id, from)
     posts = Post
       .joins(:user)
-      .where(topic_id: topic_id, action_code: 'assigned')
+      .where(topic_id: topic_id, action_code: ['assigned', 'reassigned'])
       .where('posts.created_at > ?', from)
       .order(created_at: :desc)
     usernames = Post.custom_fields_for_ids(posts, [:action_code_who]).map { |_, v| v['action_code_who'] }.uniq