From 1ac2dfbae2a88f020926fb2ad0d2ab308be888e8 Mon Sep 17 00:00:00 2001 From: Natalie Tay Date: Thu, 5 May 2022 13:21:14 +0800 Subject: [PATCH] FEATURE: Add assign note (#326) --- .../discourse_assign/assign_controller.rb | 3 +- app/models/assignment.rb | 1 + .../controllers/assign-user.js | 11 +- .../initializers/extend-for-assigns.js | 2 + .../discourse/services/task-actions.js | 2 + .../discourse/templates/modal/assign-user.hbs | 2 + assets/stylesheets/assigns.scss | 2 + config/locales/client.en.yml | 1 + .../20220429110203_add_note_to_assignments.rb | 7 + lib/assigner.rb | 28 ++- plugin.rb | 16 ++ spec/lib/assigner_spec.rb | 169 ++++++++++-------- spec/requests/assign_controller_spec.rb | 8 + spec/serializers/post_serializer_spec.rb | 6 + .../serializers/topic_view_serializer_spec.rb | 6 + 15 files changed, 179 insertions(+), 85 deletions(-) create mode 100644 db/migrate/20220429110203_add_note_to_assignments.rb diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index d49260a..436943e 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -48,6 +48,7 @@ module DiscourseAssign target_type = params.require(:target_type) username = params.permit(:username)['username'] group_name = params.permit(:group_name)['group_name'] + note = params.permit(:note)['note'].presence assign_to = username.present? ? User.find_by(username_lower: username.downcase) : Group.where("LOWER(name) = ?", group_name.downcase).first @@ -56,7 +57,7 @@ module DiscourseAssign target = target_type.constantize.where(id: target_id).first raise Discourse::NotFound unless target - assign = Assigner.new(target, current_user).assign(assign_to) + assign = Assigner.new(target, current_user).assign(assign_to, note: note) if assign[:success] render json: success_json diff --git a/app/models/assignment.rb b/app/models/assignment.rb index 1563ab6..48d2c18 100644 --- a/app/models/assignment.rb +++ b/app/models/assignment.rb @@ -37,6 +37,7 @@ end # target_id :integer not null # target_type :string not null # active :boolean default(TRUE) +# note :string # # Indexes # diff --git a/assets/javascripts/discourse-assign/controllers/assign-user.js b/assets/javascripts/discourse-assign/controllers/assign-user.js index 7d6b826..5968732 100644 --- a/assets/javascripts/discourse-assign/controllers/assign-user.js +++ b/assets/javascripts/discourse-assign/controllers/assign-user.js @@ -1,10 +1,10 @@ import Controller, { inject as controller } from "@ember/controller"; +import { action } from "@ember/object"; +import { not, or } from "@ember/object/computed"; import { inject as service } from "@ember/service"; +import { isEmpty } from "@ember/utils"; import { ajax } from "discourse/lib/ajax"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { not, or } from "@ember/object/computed"; -import { isEmpty } from "@ember/utils"; -import { action } from "@ember/object"; export default Controller.extend({ topicBulkActions: controller(), @@ -71,6 +71,7 @@ export default Controller.extend({ group_name: this.get("model.group_name"), target_id: this.get("model.target.id"), target_type: this.get("model.targetType"), + note: this.get("model.note"), }, }) .then(() => { @@ -99,10 +100,6 @@ export default Controller.extend({ "model.allowedGroups": this.taskActions.allowedGroups, }); } - - if (name) { - return this.assign(); - } }, @action diff --git a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js index 2763ecc..99fc9c8 100644 --- a/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js +++ b/assets/javascripts/discourse-assign/initializers/extend-for-assigns.js @@ -768,6 +768,8 @@ function initialize(api) { post = topic.postStream.posts.find((p) => p.id === data.post_id); } const target = post || topic; + + target.set("assignment_note", null); if (data.assigned_type === "User") { target.set( "assigned_to_user_id", diff --git a/assets/javascripts/discourse/services/task-actions.js b/assets/javascripts/discourse/services/task-actions.js index c7cae11..ad9eca0 100644 --- a/assets/javascripts/discourse/services/task-actions.js +++ b/assets/javascripts/discourse/services/task-actions.js @@ -38,6 +38,7 @@ export default Service.extend({ group_name: target.assigned_to_group?.name, target, targetType: options.targetType, + note: target.assignment_note, }, }); }, @@ -49,6 +50,7 @@ export default Service.extend({ username: user.username, target_id: target.id, target_type: targetType, + note: target.assignment_note, }, }); }, diff --git a/assets/javascripts/discourse/templates/modal/assign-user.hbs b/assets/javascripts/discourse/templates/modal/assign-user.hbs index 1043095..0e806b2 100644 --- a/assets/javascripts/discourse/templates/modal/assign-user.hbs +++ b/assets/javascripts/discourse/templates/modal/assign-user.hbs @@ -25,6 +25,8 @@ {{/each}} + + {{textarea value=model.note}} {{/d-modal-body}} diff --git a/assets/stylesheets/assigns.scss b/assets/stylesheets/assigns.scss index b9d8b86..70d120c 100644 --- a/assets/stylesheets/assigns.scss +++ b/assets/stylesheets/assigns.scss @@ -78,6 +78,8 @@ .assign-suggestions { margin-top: 15px; + margin-bottom: 15px; + img { margin-right: 5px; cursor: pointer; diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 629b81f..9c9a213 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -53,6 +53,7 @@ en: reassign_title: "Reassign Topic" description: "Enter the name of the user you'd like to assign this topic" assign: "Assign" + note_label: Note assign_post_modal: title: "Assign Post" description: "Enter the name of the user you'd like to assign this post" diff --git a/db/migrate/20220429110203_add_note_to_assignments.rb b/db/migrate/20220429110203_add_note_to_assignments.rb new file mode 100644 index 0000000..52c2267 --- /dev/null +++ b/db/migrate/20220429110203_add_note_to_assignments.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddNoteToAssignments < ActiveRecord::Migration[6.1] + def change + add_column :assignments, :note, :string, null: true + end +end diff --git a/lib/assigner.rb b/lib/assigner.rb index 73b9838..f54e3e6 100644 --- a/lib/assigner.rb +++ b/lib/assigner.rb @@ -180,7 +180,7 @@ class ::Assigner topic.posts.where(post_number: 1).first end - def forbidden_reasons(assign_to:, type:) + def forbidden_reasons(assign_to:, type:, note:) case when assign_to.is_a?(User) && !can_assignee_see_target?(assign_to) topic.private_message? ? :forbidden_assignee_not_pm_participant : :forbidden_assignee_cant_see_topic @@ -188,9 +188,9 @@ class ::Assigner topic.private_message? ? :forbidden_group_assignee_not_pm_participant : :forbidden_group_assignee_cant_see_topic when !can_be_assigned?(assign_to) assign_to.is_a?(User) ? :forbidden_assign_to : :forbidden_group_assign_to - when topic.assignment&.assigned_to_id == assign_to.id && topic.assignment&.assigned_to_type == type && topic.assignment.active == true + when topic_same_assignee_and_note(assign_to, type, note) assign_to.is_a?(User) ? :already_assigned : :group_already_assigned - when @target.is_a?(Topic) && Assignment.where(topic_id: topic.id, target_type: "Post", active: true).any? { |assignment| assignment.assigned_to_id == assign_to.id && assignment.assigned_to_type == type } + when post_same_assignee_and_note(assign_to, type, note) assign_to.is_a?(User) ? :already_assigned : :group_already_assigned when Assignment.where(topic: topic).count >= ASSIGNMENTS_PER_TOPIC_LIMIT :too_many_assigns_for_topic @@ -199,10 +199,10 @@ class ::Assigner end end - def assign(assign_to, silent: false) + def assign(assign_to, note: nil, silent: false) type = assign_to.is_a?(User) ? "User" : "Group" - forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type) + forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type, note: note) return { success: false, reason: forbidden_reason } if forbidden_reason action_code = {} @@ -211,7 +211,7 @@ class ::Assigner @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) + 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) first_post.publish_change_to_clients!(:revised, reload_topic: true) @@ -413,4 +413,20 @@ class ::Assigner return "unassigned#{suffix}" if assignment.assigned_to_user? return "unassigned_group#{suffix}" if assignment.assigned_to_group? end + + def topic_same_assignee_and_note(assign_to, type, note) + topic.assignment&.assigned_to_id == assign_to.id && + topic.assignment&.assigned_to_type == type && + topic.assignment.active == true && + topic.assignment&.note == note + end + + def post_same_assignee_and_note(assign_to, type, note) + @target.is_a?(Topic) && + Assignment.where(topic_id: topic.id, target_type: "Post", active: true).any? do |assignment| + assignment.assigned_to_id == assign_to.id && + assignment.assigned_to_type == type && + assignment&.note == note + end + end end diff --git a/plugin.rb b/plugin.rb index 0e1c18a..1535f5a 100644 --- a/plugin.rb +++ b/plugin.rb @@ -484,6 +484,14 @@ after_initialize do (SiteSetting.assigns_public || scope.can_assign?) && object.topic.indirectly_assigned_to.present? end + add_to_serializer(:topic_view, :assignment_note, false) do + object.topic.assignment.note + end + + add_to_serializer(:topic_view, :include_assignment_note?, false) do + (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assignment.present? + end + # SuggestedTopic serializer add_to_serializer(:suggested_topic, :assigned_to_user, false) do DiscourseAssign::Helpers.build_assigned_to_user(object.assigned_to, object) @@ -631,6 +639,14 @@ after_initialize do (SiteSetting.assigns_public || scope.can_assign?) && object.assignment&.assigned_to&.is_a?(Group) && object.assignment.active end + add_to_serializer(:post, :assignment_note, false) do + object.assignment.note + end + + add_to_serializer(:post, :include_assignment_note?, false) do + (SiteSetting.assigns_public || scope.can_assign?) && object.assignment.present? + end + # CurrentUser serializer add_to_serializer(:current_user, :can_assign) do object.can_assign? diff --git a/spec/lib/assigner_spec.rb b/spec/lib/assigner_spec.rb index c1a11cd..7bb6f80 100644 --- a/spec/lib/assigner_spec.rb +++ b/spec/lib/assigner_spec.rb @@ -43,6 +43,12 @@ RSpec.describe Assigner do .to eq(TopicUser.notification_levels[:tracking]) end + it "can assign with note" do + assigner.assign(moderator, note: "tomtom best mom") + + expect(topic.assignment.note).to eq "tomtom best mom" + end + it "publishes topic assignment after assign and unassign" do messages = MessageBus.track_publish('/staff/topic-assignment') do assigner = described_class.new(topic, moderator_2) @@ -139,86 +145,107 @@ RSpec.describe Assigner do assigner.assign(assignee).fetch(:success) end - it "doesn't assign if the user has too many assigned topics" do - SiteSetting.max_assigned_topics = 1 - another_post = Fabricate.build(:post) - assigner.assign(moderator) - - second_assign = described_class.new(another_post.topic, moderator_2).assign(moderator) - - expect(second_assign[:success]).to eq(false) - expect(second_assign[:reason]).to eq(:too_many_assigns) - end - - it "doesn't enforce the limit when self-assigning" do - SiteSetting.max_assigned_topics = 1 - another_post = Fabricate(:post) - assigner.assign(moderator) - - second_assign = described_class.new(another_post.topic, moderator).assign(moderator) - - expect(second_assign[:success]).to eq(true) - end - - it "doesn't count self-assigns when enforcing the limit" do - SiteSetting.max_assigned_topics = 1 - another_post = Fabricate(:post) - - first_assign = assigner.assign(moderator) - - # reached limit so stop - second_assign = described_class.new(Fabricate(:topic), moderator_2).assign(moderator) - - # self assign has a bypass - third_assign = described_class.new(another_post.topic, moderator).assign(moderator) - - expect(first_assign[:success]).to eq(true) - expect(second_assign[:success]).to eq(false) - expect(third_assign[:success]).to eq(true) - end - - it "doesn't count inactive assigns when enforcing the limit" do - SiteSetting.max_assigned_topics = 1 - SiteSetting.unassign_on_close = true - another_post = Fabricate(:post) - - first_assign = assigner.assign(moderator) - topic.update_status("closed", true, Discourse.system_user) - - second_assign = described_class.new(another_post.topic, moderator_2).assign(moderator) - - expect(first_assign[:success]).to eq(true) - expect(second_assign[:success]).to eq(true) - end - fab!(:admin) { Fabricate(:admin) } - it 'fails to assign when the assigned user cannot view the pm' do - assign = described_class.new(pm, admin).assign(moderator) + context "forbidden reasons" do + it "doesn't assign if the user has too many assigned topics" do + SiteSetting.max_assigned_topics = 1 + another_post = Fabricate.build(:post) + assigner.assign(moderator) - expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_assignee_not_pm_participant) - end + second_assign = described_class.new(another_post.topic, moderator_2).assign(moderator) - it 'fails to assign when not all group members has access to pm' do - assign = described_class.new(pm, admin).assign(moderator.groups.first) + expect(second_assign[:success]).to eq(false) + expect(second_assign[:reason]).to eq(:too_many_assigns) + end - expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_group_assignee_not_pm_participant) - end + it "doesn't enforce the limit when self-assigning" do + SiteSetting.max_assigned_topics = 1 + another_post = Fabricate(:post) + assigner.assign(moderator) - it 'fails to assign when the assigned user cannot view the topic' do - assign = described_class.new(secure_topic, admin).assign(moderator) + second_assign = described_class.new(another_post.topic, moderator).assign(moderator) - expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_assignee_cant_see_topic) - end + expect(second_assign[:success]).to eq(true) + 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) + it "doesn't count self-assigns when enforcing the limit" do + SiteSetting.max_assigned_topics = 1 + another_post = Fabricate(:post) - expect(assign[:success]).to eq(false) - expect(assign[:reason]).to eq(:forbidden_group_assignee_cant_see_topic) + first_assign = assigner.assign(moderator) + + # reached limit so stop + second_assign = described_class.new(Fabricate(:topic), moderator_2).assign(moderator) + + # self assign has a bypass + third_assign = described_class.new(another_post.topic, moderator).assign(moderator) + + expect(first_assign[:success]).to eq(true) + expect(second_assign[:success]).to eq(false) + expect(third_assign[:success]).to eq(true) + end + + it "doesn't count inactive assigns when enforcing the limit" do + SiteSetting.max_assigned_topics = 1 + SiteSetting.unassign_on_close = true + another_post = Fabricate(:post) + + first_assign = assigner.assign(moderator) + topic.update_status("closed", true, Discourse.system_user) + + second_assign = described_class.new(another_post.topic, moderator_2).assign(moderator) + + expect(first_assign[:success]).to eq(true) + expect(second_assign[:success]).to eq(true) + end + + it 'fails to assign when the assigned user and note is the same' do + assigner = described_class.new(topic, admin) + assigner.assign(moderator, note: "note me down") + + assign = assigner.assign(moderator, note: "note me down") + + expect(assign[:success]).to eq(false) + expect(assign[:reason]).to eq(:already_assigned) + end + + it 'allows assign when the assigned user is same but note is different' do + assigner = described_class.new(topic, admin) + assigner.assign(moderator, note: "note me down") + + assign = assigner.assign(moderator, note: "note me down again") + + expect(assign[:success]).to eq(true) + end + + it 'fails to assign when the assigned user cannot view the pm' do + assign = described_class.new(pm, admin).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) + + 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) + + 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) + + expect(assign[:success]).to eq(false) + expect(assign[:reason]).to eq(:forbidden_group_assignee_cant_see_topic) + end end it "assigns the PM to the moderator when it's included in the list of allowed users" do diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index e563adb..432b88b 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -116,6 +116,14 @@ RSpec.describe DiscourseAssign::AssignController do expect(post.topic.reload.assignment.assigned_to_id).to eq(user2.id) end + it 'assigns topic with note to a user' do + put '/assign/assign.json', params: { + target_id: post.topic_id, target_type: 'Topic', username: user2.username, note: "do dis pls" + } + + expect(post.topic.reload.assignment.note).to eq("do dis pls") + end + it 'assigns topic to a group' do put '/assign/assign.json', params: { target_id: post.topic_id, target_type: 'Topic', group_name: assign_allowed_group.name diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 4fa6e19..a68875f 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -33,4 +33,10 @@ describe PostSerializer do expect(post[:assigned_to_group][:assign_icon]).to eq("group-plus") expect(post[:assigned_to_user]).to be(nil) end + + it "includes note in serializer" do + Assigner.new(post, user).assign(user, note: "tomtom best") + serializer = PostSerializer.new(post, scope: guardian) + expect(serializer.as_json[:post][:assignment_note]).to eq("tomtom best") + end end diff --git a/spec/serializers/topic_view_serializer_spec.rb b/spec/serializers/topic_view_serializer_spec.rb index 50cbbc7..f948299 100644 --- a/spec/serializers/topic_view_serializer_spec.rb +++ b/spec/serializers/topic_view_serializer_spec.rb @@ -29,4 +29,10 @@ RSpec.describe TopicViewSerializer do expect(serializer.as_json[:topic_view][:assigned_to_group][:name]).to eq(assign_allowed_group.name) expect(serializer.as_json[:topic_view][:assigned_to_user]).to be nil end + + it "includes note in serializer" do + Assigner.new(topic, user).assign(user, note: "note me down") + serializer = TopicViewSerializer.new(TopicView.new(topic), scope: guardian) + expect(serializer.as_json[:topic_view][:assignment_note]).to eq("note me down") + end end