Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRAFT: Add priorities to assignment #317

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/controllers/discourse_assign/assign_controller.rb
Original file line number Diff line number Diff line change
@@ -48,6 +48,7 @@ def assign
target_type = params.require(:target_type)
username = params.permit(:username)['username']
group_name = params.permit(:group_name)['group_name']
priority = (params.permit(:priority)['priority'])&.to_i

assign_to = username.present? ? User.find_by(username_lower: username.downcase) : Group.where("LOWER(name) = ?", group_name.downcase).first

@@ -56,7 +57,7 @@ def assign
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, priority: priority)

if assign[:success]
render json: success_json
8 changes: 8 additions & 0 deletions app/models/assignment.rb
Original file line number Diff line number Diff line change
@@ -8,6 +8,13 @@ class Assignment < ActiveRecord::Base
belongs_to :assigned_by_user, class_name: "User"
belongs_to :target, polymorphic: true

enum priority: {
low: 4,
medium: 3,
high: 2,
urgent: 1,
}, _prefix: true

scope :joins_with_topics, -> { joins("INNER JOIN topics ON topics.id = assignments.target_id AND assignments.target_type = 'Topic' AND topics.deleted_at IS NULL") }

def self.valid_type?(type)
@@ -37,6 +44,7 @@ def assigned_to_group?
# target_id :integer not null
# target_type :string not null
# active :boolean default(TRUE)
# priority :integer
#
# Indexes
#
26 changes: 18 additions & 8 deletions assets/javascripts/discourse-assign/controllers/assign-user.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
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";
import I18n from "I18n";

const PRIORITIES = [
{ name: I18n.t("discourse_assign.priorities.low"), value: 4 },
{ name: I18n.t("discourse_assign.priorities.medium"), value: 3 },
{ name: I18n.t("discourse_assign.priorities.high"), value: 2 },
{ name: I18n.t("discourse_assign.priorities.urgent"), value: 1 },
];

export default Controller.extend({
topicBulkActions: controller(),
@@ -13,6 +21,7 @@ export default Controller.extend({
taskActions: service(),
autofocus: not("capabilities.touch"),
assigneeName: or("model.username", "model.group_name"),
priorities: PRIORITIES,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should set this in init, and have it as priorities: null here, this is mostly due to controllers being singletons in Ember.


init() {
this._super(...arguments);
@@ -63,14 +72,14 @@ export default Controller.extend({
}

this.send("closeModal");

return ajax(path, {
type: "PUT",
data: {
username: this.get("model.username"),
group_name: this.get("model.group_name"),
target_id: this.get("model.target.id"),
target_type: this.get("model.targetType"),
priority: this.get("model.priority"),
},
})
.then(() => {
@@ -99,14 +108,15 @@ export default Controller.extend({
"model.allowedGroups": this.taskActions.allowedGroups,
});
}

if (name) {
return this.assign();
}
},

@action
assignUsername(selected) {
this.assignUser(selected.firstObject);
},

@action
assignPriority(priority) {
this.set("model.priority", priority);
},
});
Original file line number Diff line number Diff line change
@@ -23,6 +23,7 @@ const DEPENDENT_KEYS = [
"topic.assigned_to_group",
"currentUser.can_assign",
"topic.assigned_to_user.username",
"topic.assignment_priority",
];

function titleForState(name) {
1 change: 1 addition & 0 deletions assets/javascripts/discourse/services/task-actions.js
Original file line number Diff line number Diff line change
@@ -38,6 +38,7 @@ export default Service.extend({
group_name: target.assigned_to_group?.name,
target,
targetType: options.targetType,
priority: target.assignment_priority,
},
});
},
11 changes: 11 additions & 0 deletions assets/javascripts/discourse/templates/modal/assign-user.hbs
Original file line number Diff line number Diff line change
@@ -25,6 +25,17 @@
</a>
{{/each}}
</div>
<label>{{i18n "discourse_assign.assign_modal.priority_label"}}</label>
{{combo-box
name="assign-priority"
content=priorities
value=model.priority
valueProperty="value"
onChange=(action "assignPriority")
options=(hash
none="discourse_assign.priorities.none"
)
}}
</div>
{{/d-modal-body}}

2 changes: 2 additions & 0 deletions assets/stylesheets/assigns.scss
Original file line number Diff line number Diff line change
@@ -78,6 +78,8 @@

.assign-suggestions {
margin-top: 15px;
margin-bottom: 15px;

img {
margin-right: 5px;
cursor: pointer;
9 changes: 8 additions & 1 deletion config/locales/client.en.yml
Original file line number Diff line number Diff line change
@@ -47,12 +47,13 @@ en:
help: "Reassign Topic to a different user"
reassign_modal:
title: "Reassign Topic"
description: "Enter the name of the user you'd like to Reassign this topic"
description: "Enter the name of the user you'd like to reassign this topic"
assign_modal:
title: "Assign Topic"
reassign_title: "Reassign Topic"
description: "Enter the name of the user you'd like to assign this topic"
assign: "Assign"
priority_label: Set a priority (Optional)
assign_post_modal:
title: "Assign Post"
description: "Enter the name of the user you'd like to assign this post"
@@ -79,6 +80,12 @@ en:
assign: "Assign"
assignable_levels:
title: "Who can assign this group"
priorities:
none: None
low: Low
medium: Medium
high: High
urgent: Urgent
user:
messages:
assigned_title: "Assigned (%{count})"
7 changes: 7 additions & 0 deletions db/migrate/20220419093001_add_priority_to_assignments.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# frozen_string_literal: true

class AddPriorityToAssignments < ActiveRecord::Migration[6.1]
def change
add_column :assignments, :priority, :integer, null: true
end
end
4 changes: 2 additions & 2 deletions lib/assigner.rb
Original file line number Diff line number Diff line change
@@ -199,7 +199,7 @@ def forbidden_reasons(assign_to:, type:)
end
end

def assign(assign_to, silent: false)
def assign(assign_to, priority: nil, silent: false)
type = assign_to.is_a?(User) ? "User" : "Group"

forbidden_reason = forbidden_reasons(assign_to: assign_to, type: type)
@@ -211,7 +211,7 @@ def assign(assign_to, silent: false)

@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, priority: priority)

first_post.publish_change_to_clients!(:revised, reload_topic: true)

16 changes: 16 additions & 0 deletions plugin.rb
Original file line number Diff line number Diff line change
@@ -484,6 +484,14 @@ class ::ListController
(SiteSetting.assigns_public || scope.can_assign?) && object.topic.indirectly_assigned_to.present?
end

add_to_serializer(:topic_view, :assignment_priority, false) do
Assignment.priorities[object.topic.assignment.priority]
end

add_to_serializer(:topic_view, :include_assignment_priority?, 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 @@ class ::ListController
(SiteSetting.assigns_public || scope.can_assign?) && object.assignment&.assigned_to&.is_a?(Group) && object.assignment.active
end

add_to_serializer(:post, :assignment_priority, false) do
Assignment.priorities[object.assignment.priority]
end

add_to_serializer(:post, :include_assignment_priority?, 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?
6 changes: 6 additions & 0 deletions spec/lib/assigner_spec.rb
Original file line number Diff line number Diff line change
@@ -43,6 +43,12 @@
.to eq(TopicUser.notification_levels[:tracking])
end

it "can assign with priority" do
assigner.assign(moderator, priority: 2)

expect(topic.assignment.priority_high?).to eq true
end

it 'does not update notification level if already watching' do
TopicUser.change(moderator.id, topic.id,
notification_level: TopicUser.notification_levels[:watching]
10 changes: 9 additions & 1 deletion spec/requests/assign_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# frozen_string_literal: true

require 'rails_helper'
require_relative '../support/assign_allowed_group'

RSpec.describe DiscourseAssign::AssignController do
@@ -116,6 +115,15 @@
expect(post.topic.reload.assignment.assigned_to_id).to eq(user2.id)
end

it 'assigns topic with priority to a user' do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test case which passes in an invalid priority like a random string or an invalid number? My hunch is that this is not handled right now and might result in us writing invalid values into the DB.

Copy link
Contributor Author

@nattsw nattsw Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The activerecord actually is smart about it because of the enums declared. Let me see if I can get you an error...

Copy link
Contributor Author

@nattsw nattsw Apr 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigner.new(t, u).assign(s, priority: 5)
...
ArgumentError: '5' is not a valid priority

🪄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we have any special rescues for ArgumentError at the controller level so this might result in a 500 response. I think we might want to ensure that the priority passed in by the client is valid and return a nicer response.

Copy link
Contributor Author

@nattsw nattsw Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your concern that we're writing invalid values to the DB is addressed...

But for this one where it results in 500, I'm not sure if we have to care: the only consumer for this endpoint is our frontend, and the frontend only ever passes 1-4. I'd like to avoid testing the framework if possible since this is a built-in enum feature.

put '/assign/assign.json', params: {
target_id: post.topic_id, target_type: 'Topic', username: user2.username, priority: 4
}

topicPriority = post.topic.reload.assignment.priority
expect(Assignment.priorities[topicPriority]).to eq(4)
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
6 changes: 6 additions & 0 deletions spec/serializers/post_serializer_spec.rb
Original file line number Diff line number Diff line change
@@ -29,4 +29,10 @@
expect(serializer.as_json[:post][:assigned_to_group].id).to eq(assign_allowed_group.id)
expect(serializer.as_json[:post][:assigned_to_user]).to be nil
end

it "includes priority in serializer" do
Assigner.new(post, user).assign(user, priority: 1)
serializer = PostSerializer.new(post, scope: guardian)
expect(serializer.as_json[:post][:assignment_priority]).to eq(1)
end
end
37 changes: 37 additions & 0 deletions spec/serializers/topic_view_serializer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# frozen_string_literal: true

require_relative '../support/assign_allowed_group'

RSpec.describe TopicViewSerializer do
fab!(:user) { Fabricate(:user) }
fab!(:topic) { Fabricate(:topic) }
fab!(:post) { Fabricate(:post, topic: topic) }
let(:guardian) { Guardian.new(user) }

include_context 'A group that is allowed to assign'

before do
SiteSetting.assign_enabled = true
add_to_assign_allowed_group(user)
end

it "includes assigned user in serializer" do
Assigner.new(topic, user).assign(user)
serializer = TopicViewSerializer.new(TopicView.new(topic), scope: guardian)
expect(serializer.as_json[:topic_view][:assigned_to_user][:username]).to eq(user.username)
expect(serializer.as_json[:topic_view][:assigned_to_group]).to be nil
end

it "includes assigned group in serializer" do
Assigner.new(topic, user).assign(assign_allowed_group)
serializer = TopicViewSerializer.new(TopicView.new(topic), scope: guardian)
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 priority in serializer" do
Assigner.new(topic, user).assign(user, priority: 1)
serializer = TopicViewSerializer.new(TopicView.new(topic), scope: guardian)
expect(serializer.as_json[:topic_view][:assignment_priority]).to eq(1)
end
end