discourse
/
discourse-assign
mirror of https://github.com/discourse/discourse-assign.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DEV: Move assignments from custom fields to a table (#169)

This commit is contained in:
Jarek Radosz 2021-07-14 10:48:19 +02:00 committed by GitHub
parent f559fc9557
commit 470dd939aa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
15 changed files with 247 additions and 242 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
app/controllers/discourse_assign/assign_controller.rb
View File

@ -11,10 +11,9 @@ module DiscourseAssign
.where('users.id <> ?', current_user.id) .where('users.id <> ?', current_user.id)
.joins(<<~SQL .joins(<<~SQL
JOIN( JOIN(
SELECT value::integer user_id, MAX(created_at) last_assigned SELECT assigned_to_id user_id, MAX(created_at) last_assigned
FROM topic_custom_fields FROM assignments
WHERE name = 'assigned_to_id' GROUP BY assigned_to_id
GROUP BY value::integer
HAVING COUNT(*) < #{SiteSetting.max_assigned_topics} HAVING COUNT(*) < #{SiteSetting.max_assigned_topics}
) as X ON X.user_id = users.id ) as X ON X.user_id = users.id
SQL SQL
@ -33,18 +32,18 @@ module DiscourseAssign
topic_id = params.require(:topic_id).to_i topic_id = params.require(:topic_id).to_i
topic = Topic.find(topic_id) topic = Topic.find(topic_id)
assigned = TopicCustomField.where( assigned_id = Assignment
"topic_id = :topic_id AND name = 'assigned_to_id' AND value IS NOT NULL", .where(topic_id: topic_id)
topic_id: topic_id .where.not(assigned_to_id: nil)
).pluck(:value) .pluck_first(:assigned_to_id)
if assigned && user_id = assigned[0] if assigned_id
extras = nil if user = User.where(id: assigned_id).first
if user = User.where(id: user_id).first
extras = { extras = {
assigned_to: serialize_data(user, BasicUserSerializer, root: false) assigned_to: serialize_data(user, BasicUserSerializer, root: false)
} }
end end
return render_json_error(I18n.t('discourse_assign.already_claimed'), extras: extras) return render_json_error(I18n.t('discourse_assign.already_claimed'), extras: extras)
end end
@ -91,26 +90,28 @@ module DiscourseAssign
topics = Topic topics = Topic
.includes(:tags) .includes(:tags)
.includes(:user) .includes(:user)
.joins("JOIN topic_custom_fields tcf ON topics.id = tcf.topic_id AND tcf.name = 'assigned_to_id' AND tcf.value IS NOT NULL") .joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL")
.order('tcf.value::integer, topics.bumped_at desc') .order("a.assigned_to_id, topics.bumped_at desc")
.offset(offset) .offset(offset)
.limit(limit) .limit(limit)
Topic.preload_custom_fields(topics, TopicList.preloaded_custom_fields) Topic.preload_custom_fields(topics, TopicList.preloaded_custom_fields)
assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h
users = User users = User
.where("users.id IN (SELECT value::int FROM topic_custom_fields WHERE name = 'assigned_to_id' AND topic_id IN (?))", topics.map(&:id)) .where("users.id IN (?)", assignments.values.uniq)
.joins('join user_emails on user_emails.user_id = users.id AND user_emails.primary') .joins("join user_emails on user_emails.user_id = users.id AND user_emails.primary")
.select(UserLookup.lookup_columns) .select(UserLookup.lookup_columns)
.to_a .to_a
User.preload_custom_fields(users, User.allowed_user_custom_fields(guardian)) User.preload_custom_fields(users, User.allowed_user_custom_fields(guardian))
users = users.to_h { |u| [u.id, u] } users_map = users.index_by(&:id)
topics.each do |t|
if id = t.custom_fields[TopicAssigner::ASSIGNED_TO_ID] topics.each do |topic|
t.preload_assigned_to_user(users[id.to_i]) user_id = assignments[topic.id]
end topic.preload_assigned_to_user(users_map[user_id]) if user_id
end end
render json: { topics: serialize_data(topics, AssignedTopicSerializer) } render json: { topics: serialize_data(topics, AssignedTopicSerializer) }
@ -129,26 +130,30 @@ module DiscourseAssign
guardian.ensure_can_see_group_members!(group) guardian.ensure_can_see_group_members!(group)
members = User members = User
.joins("LEFT OUTER JOIN group_users g on users.id=g.user_id") .joins("LEFT OUTER JOIN group_users g ON g.user_id = users.id")
.joins("LEFT OUTER JOIN topic_custom_fields tcf ON tcf.value::int = users.id") .joins("LEFT OUTER JOIN assignments a ON a.assigned_to_id = users.id")
.joins("LEFT OUTER JOIN topics t ON t.id = tcf.topic_id") .joins("LEFT OUTER JOIN topics t ON t.id = a.topic_id")
.where("tcf.name = 'assigned_to_id' AND g.group_id=? AND (users.id > 0) AND t.deleted_at IS NULL", group.id) .where("g.group_id = ? AND users.id > 0 AND t.deleted_at IS NULL", group.id)
.where("a.assigned_to_id IS NOT NULL")
.order('COUNT(users.id) DESC') .order('COUNT(users.id) DESC')
.group('users.id') .group('users.id')
.select('users.*, COUNT(users.id) as "assignments_count"') .select('users.*, COUNT(users.id) as "assignments_count"')
members = members.where("users.name ILIKE :pattern OR users.username_lower ILIKE :pattern", pattern: "%#{params[:filter]}%") if params[:filter]
members = members
.limit(limit) .limit(limit)
.offset(offset) .offset(offset)
assignment_count = Topic.joins("JOIN topic_custom_fields tcf ON topics.id = tcf.topic_id AND tcf.name = 'assigned_to_id' AND tcf.value IS NOT NULL") if params[:filter]
.where("tcf.value IN (SELECT group_users.user_id::varchar(255) FROM group_users WHERE (group_id IN (SELECT id FROM groups WHERE name = ?)))", group.name) members = members.where(<<~SQL, pattern: "%#{params[:filter]}%")
users.name ILIKE :pattern OR users.username_lower ILIKE :pattern
SQL
end
assignment_count = Topic
.joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL")
.where("a.assigned_to_id IN (SELECT group_users.user_id FROM group_users WHERE (group_id IN (SELECT id FROM groups WHERE name = ?)))", group.name)
.where("topics.deleted_at IS NULL") .where("topics.deleted_at IS NULL")
.count .count
render json: { members: serialize_data(members, GroupUserAssignedSerializer), "assignment_count" => assignment_count } render json: { members: serialize_data(members, GroupUserAssignedSerializer), assignment_count: assignment_count }
end end
private private

5
app/models/assignment.rb Normal file
View File

@ -0,0 +1,5 @@
# frozen_string_literal: true
class Assignment < ActiveRecord::Base
belongs_to :topic
end

15
db/migrate/20210627100932_create_assignments.rb Normal file
View File

@ -0,0 +1,15 @@
# frozen_string_literal: true
class CreateAssignments < ActiveRecord::Migration[6.1]
def change
create_table :assignments do |t|
t.integer :topic_id, null: false
t.integer :assigned_to_id, null: false
t.integer :assigned_by_user_id, null: false
t.timestamps
end
add_index :assignments, :topic_id, unique: true
end
end

22
db/post_migrate/20210709101534_move_assignments_from_custom_fields_to_a_table.rb Normal file
View File

@ -0,0 +1,22 @@
# frozen_string_literal: true
class MoveAssignmentsFromCustomFieldsToATable < ActiveRecord::Migration[6.1]
def up
execute <<~SQL
INSERT INTO assignments (topic_id, assigned_by_user_id, assigned_to_id, created_at, updated_at)
SELECT (
SELECT value::integer assigned_to_id
FROM topic_custom_fields tcf1
WHERE tcf1.name = 'assigned_to_id' AND tcf1.topic_id = tcf2.topic_id
), value::integer assgined_by_id, topic_id, created_at, updated_at
FROM topic_custom_fields tcf2
WHERE name = 'assigned_by_id'
ORDER BY created_at DESC
ON CONFLICT DO NOTHING
SQL
end
def down
raise ActiveRecord::IrreversibleMigration
end
end

19
jobs/scheduled/enqueue_reminders.rb
View File

@ -24,19 +24,19 @@ module Jobs
frequency = ActiveRecord::Base.sanitize_sql("COALESCE(user_frequency.value, '#{global_frequency}')::INT") frequency = ActiveRecord::Base.sanitize_sql("COALESCE(user_frequency.value, '#{global_frequency}')::INT")
DB.query_single(<<~SQL DB.query_single(<<~SQL
SELECT topic_custom_fields.value SELECT assignments.assigned_to_id
FROM topic_custom_fields FROM assignments
LEFT OUTER JOIN user_custom_fields AS last_reminder LEFT OUTER JOIN user_custom_fields AS last_reminder
ON topic_custom_fields.value::INT = last_reminder.user_id ON assignments.assigned_to_id = last_reminder.user_id
AND last_reminder.name = '#{PendingAssignsReminder::REMINDED_AT}' AND last_reminder.name = '#{PendingAssignsReminder::REMINDED_AT}'
LEFT OUTER JOIN user_custom_fields AS user_frequency LEFT OUTER JOIN user_custom_fields AS user_frequency
ON topic_custom_fields.value::INT = user_frequency.user_id ON assignments.assigned_to_id = user_frequency.user_id
AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}' AND user_frequency.name = '#{PendingAssignsReminder::REMINDERS_FREQUENCY}'
INNER JOIN group_users ON topic_custom_fields.value::INT = group_users.user_id INNER JOIN group_users ON assignments.assigned_to_id = group_users.user_id
INNER JOIN topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL) INNER JOIN topics ON topics.id = assignments.topic_id AND topics.deleted_at IS NULL
WHERE group_users.group_id IN (#{allowed_group_ids}) WHERE group_users.group_id IN (#{allowed_group_ids})
AND #{frequency} > 0 AND #{frequency} > 0
@ -44,11 +44,10 @@ module Jobs
last_reminder.value IS NULL OR last_reminder.value IS NULL OR
last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency})
) )
AND topic_custom_fields.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) AND assignments.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency})
AND topic_custom_fields.name = '#{TopicAssigner::ASSIGNED_TO_ID}'
GROUP BY topic_custom_fields.value GROUP BY assignments.assigned_to_id
HAVING COUNT(topic_custom_fields.value) > 1 HAVING COUNT(assignments.assigned_to_id) > 1
SQL SQL
) )
end end

5
lib/discourse_assign/helpers.rb
View File

@ -4,10 +4,7 @@ module DiscourseAssign
module Helpers module Helpers
def self.build_assigned_to_user(assigned_to_user_id, topic) def self.build_assigned_to_user(assigned_to_user_id, topic)
if assigned_to_user_id && user = User.find_by(id: assigned_to_user_id) if assigned_to_user_id && user = User.find_by(id: assigned_to_user_id)
assigned_at = TopicCustomField.where( assigned_at = Assignment.where(topic_id: topic.id).pluck_first(:created_at)
topic_id: topic.id,
name: "assigned_to_id"
).pluck(:created_at).first
{ {
username: user.username, username: user.username,

13
lib/pending_assigns_reminder.rb
View File

@ -28,19 +28,18 @@ class PendingAssignsReminder
private private
def assigned_count_for(user) def assigned_count_for(user)
TopicCustomField Assignment.joins(:topic).where(assigned_to_id: user.id).count
.joins(:topic)
.where(name: TopicAssigner::ASSIGNED_TO_ID, value: user.id)
.count
end end
def assigned_topics(user, order:) def assigned_topics(user, order:)
secure = Topic.listable_topics.secured(Guardian.new(user)).or(Topic.private_messages_for_user(user)) secure = Topic.listable_topics.secured(Guardian.new(user)).or(Topic.private_messages_for_user(user))
Topic.joins(:_custom_fields).select(:slug, :id, :title, :fancy_title, 'topic_custom_fields.created_at AS assigned_at') Topic
.where('topic_custom_fields.name = ? AND topic_custom_fields.value = ?', TopicAssigner::ASSIGNED_TO_ID, user.id.to_s) .joins(:assignment)
.select(:slug, :id, :title, :fancy_title, 'assignments.created_at AS assigned_at')
.where('assignments.assigned_to_id = ?', user.id)
.merge(secure) .merge(secure)
.order("topic_custom_fields.created_at #{order}") .order("assignments.created_at #{order}")
.limit(3) .limit(3)
end end

53
lib/topic_assigner.rb
View File

@ -4,10 +4,6 @@ require 'email/sender'
require 'nokogiri' require 'nokogiri'
class ::TopicAssigner class ::TopicAssigner
ASSIGNED_TO_ID ||= 'assigned_to_id'
ASSIGNED_BY_ID ||= 'assigned_by_id'
def self.backfill_auto_assign def self.backfill_auto_assign
staff_mention = User staff_mention = User
.assign_allowed .assign_allowed
@ -19,10 +15,10 @@ class ::TopicAssigner
SELECT p.topic_id, MAX(post_number) post_number SELECT p.topic_id, MAX(post_number) post_number
FROM posts p FROM posts p
JOIN topics t ON t.id = p.topic_id JOIN topics t ON t.id = p.topic_id
LEFT JOIN topic_custom_fields tc ON tc.name = '#{ASSIGNED_TO_ID}' AND tc.topic_id = p.topic_id LEFT JOIN assignments a ON a.topic_id = p.topic_id
WHERE p.user_id IN (SELECT id FROM users WHERE moderator OR admin) WHERE p.user_id IN (SELECT id FROM users WHERE moderator OR admin)
AND (#{staff_mention}) AND (#{staff_mention})
AND tc.value IS NULL AND a.assigned_to_id IS NULL
AND NOT t.closed AND NOT t.closed
AND t.deleted_at IS NULL AND t.deleted_at IS NULL
GROUP BY p.topic_id GROUP BY p.topic_id
@ -57,7 +53,7 @@ class ::TopicAssigner
return unless SiteSetting.assigns_by_staff_mention return unless SiteSetting.assigns_by_staff_mention
if post.user && post.topic && post.user.can_assign? if post.user && post.topic && post.user.can_assign?
return unless force || post.topic.custom_fields[ASSIGNED_TO_ID].nil? return if post.topic.assignment.present? && !force
# remove quotes, oneboxes and code blocks # remove quotes, oneboxes and code blocks
doc = Nokogiri::HTML5.fragment(post.cooked) doc = Nokogiri::HTML5.fragment(post.cooked)
@ -124,11 +120,10 @@ class ::TopicAssigner
def can_assign_to?(user) def can_assign_to?(user)
return true if @assigned_by.id == user.id return true if @assigned_by.id == user.id
assigned_total = TopicCustomField assigned_total = Assignment
.joins(:topic) .joins(:topic)
.where(topics: { deleted_at: nil }) .where(topics: { deleted_at: nil })
.where(name: ASSIGNED_TO_ID) .where(assigned_to_id: user.id)
.where(value: user.id)
.count .count
assigned_total < SiteSetting.max_assigned_topics assigned_total < SiteSetting.max_assigned_topics
@ -148,12 +143,11 @@ class ::TopicAssigner
return { success: false, reason: reason } return { success: false, reason: reason }
end end
return { success: false, reason: :forbidden_assign_to } unless can_be_assigned?(assign_to) return { success: false, reason: :forbidden_assign_to } unless can_be_assigned?(assign_to)
return { success: false, reason: :already_assigned } if @topic.custom_fields[ASSIGNED_TO_ID] == assign_to.id.to_s return { success: false, reason: :already_assigned } if @topic.assignment&.assigned_to_id == assign_to.id
return { success: false, reason: :too_many_assigns } unless can_assign_to?(assign_to) return { success: false, reason: :too_many_assigns } unless can_assign_to?(assign_to)
@topic.custom_fields[ASSIGNED_TO_ID] = assign_to.id @topic.assignment&.destroy!
@topic.custom_fields[ASSIGNED_BY_ID] = @assigned_by.id @topic.create_assignment!(assigned_to_id: assign_to.id, assigned_by_user_id: @assigned_by.id)
@topic.save_custom_fields
first_post = @topic.posts.find_by(post_number: 1) first_post = @topic.posts.find_by(post_number: 1)
first_post.publish_change_to_clients!(:revised, reload_topic: true) first_post.publish_change_to_clients!(:revised, reload_topic: true)
@ -260,45 +254,26 @@ class ::TopicAssigner
end end
{ success: true } { success: true }
end end
def unassign(silent: false) def unassign(silent: false)
if assigned_to_id = @topic.custom_fields[ASSIGNED_TO_ID] if assignment = @topic.assignment
assignment.destroy!
# TODO core needs an API for this stuff badly
# currently there is no 100% surefire way of deleting a custom field
TopicCustomField
.where(topic_id: @topic.id)
.where('name in (?)', [ASSIGNED_BY_ID, ASSIGNED_TO_ID])
.destroy_all
if Array === assigned_to_id
# more custom field mess, try to recover
assigned_to_id = assigned_to_id.first
end
# clean up in memory object
@topic.custom_fields.delete(ASSIGNED_TO_ID)
@topic.custom_fields.delete(ASSIGNED_BY_ID)
# nothing to do here
return if !assigned_to_id
post = @topic.posts.where(post_number: 1).first post = @topic.posts.where(post_number: 1).first
return unless post.present? return if post.blank?
post.publish_change_to_clients!(:revised, reload_topic: true) post.publish_change_to_clients!(:revised, reload_topic: true)
if TopicUser.exists?( if TopicUser.exists?(
user_id: assigned_to_id, user_id: assignment.assigned_to_id,
topic: @topic, topic: @topic,
notification_level: TopicUser.notification_levels[:watching], notification_level: TopicUser.notification_levels[:watching],
notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] notifications_reason_id: TopicUser.notification_reasons[:plugin_changed]
) )
TopicUser.change( TopicUser.change(
assigned_to_id, assignment.assigned_to_id,
@topic.id, @topic.id,
notification_level: TopicUser.notification_levels[:tracking], notification_level: TopicUser.notification_levels[:tracking],
notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] notifications_reason_id: TopicUser.notification_reasons[:plugin_changed]
@ -314,7 +289,7 @@ class ::TopicAssigner
user_ids: allowed_user_ids user_ids: allowed_user_ids
) )
assigned_user = User.find_by(id: assigned_to_id) assigned_user = User.find_by(id: assignment.assigned_to_id)
publish_topic_tracking_state(@topic, assigned_user.id) publish_topic_tracking_state(@topic, assigned_user.id)
UserAction.where( UserAction.where(

184
plugin.rb
View File

@ -32,6 +32,10 @@ after_initialize do
require 'topic_assigner' require 'topic_assigner'
require 'pending_assigns_reminder' require 'pending_assigns_reminder'
class ::Topic
has_one :assignment, dependent: :destroy
end
frequency_field = PendingAssignsReminder::REMINDERS_FREQUENCY frequency_field = PendingAssignsReminder::REMINDERS_FREQUENCY
register_editable_user_custom_field frequency_field register_editable_user_custom_field frequency_field
User.register_custom_field_type frequency_field, :integer User.register_custom_field_type frequency_field, :integer
@ -41,8 +45,18 @@ after_initialize do
end end
add_to_serializer(:group_show, :assignment_count) do add_to_serializer(:group_show, :assignment_count) do
Topic.joins("JOIN topic_custom_fields tcf ON topics.id = tcf.topic_id AND tcf.name = 'assigned_to_id' AND tcf.value IS NOT NULL") Topic
.where("tcf.value IN (SELECT group_users.user_id::varchar(255) FROM group_users WHERE (group_id IN (SELECT id FROM groups WHERE name = ?)))", object.name) .joins(<<~SQL)
JOIN assignments
ON topics.id = assignments.topic_id AND assignments.assigned_to_id IS NOT NULL
SQL
.where(<<~SQL, object.name)
assignments.assigned_to_id IN (
SELECT group_users.user_id
FROM group_users
WHERE group_id IN (SELECT id FROM groups WHERE name = ?)
)
SQL
.where("topics.deleted_at IS NULL") .where("topics.deleted_at IS NULL")
.count .count
end end
@ -118,7 +132,7 @@ after_initialize do
end end
DiscourseEvent.on(:assign_topic) do |topic, user, assigning_user, force| DiscourseEvent.on(:assign_topic) do |topic, user, assigning_user, force|
if force || !topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] if force || !Assignment.exists?(topic: topic)
TopicAssigner.new(topic, assigning_user).assign(user) TopicAssigner.new(topic, assigning_user).assign(user)
end end
end end
@ -127,20 +141,18 @@ after_initialize do
TopicAssigner.new(topic, unassigning_user).unassign TopicAssigner.new(topic, unassigning_user).unassign
end end
TopicList.preloaded_custom_fields << TopicAssigner::ASSIGNED_TO_ID
Site.preloaded_category_custom_fields << "enable_unassigned_filter" Site.preloaded_category_custom_fields << "enable_unassigned_filter"
Search.preloaded_topic_custom_fields << TopicAssigner::ASSIGNED_TO_ID
BookmarkQuery.preloaded_custom_fields << TopicAssigner::ASSIGNED_TO_ID
BookmarkQuery.on_preload do |bookmarks, bookmark_query| BookmarkQuery.on_preload do |bookmarks, bookmark_query|
if SiteSetting.assign_enabled? if SiteSetting.assign_enabled?
assigned_user_ids = bookmarks.map(&:topic).map { |topic| topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] }.compact.uniq topics = bookmarks.map(&:topic)
assigned_users = User.where(id: assigned_user_ids).index_by(&:id) assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h
users_map = User.where(id: assignments.values.uniq).index_by(&:id)
bookmarks.each do |bookmark| topics.each do |topic|
bookmark.topic.preload_assigned_to_user( user_id = assignments[topic.id]
assigned_users[bookmark.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID]] user = users_map[user_id] if user_id
) topic.preload_assigned_to_user(user)
end end
end end
end end
@ -151,22 +163,17 @@ after_initialize do
allowed_access = SiteSetting.assigns_public || can_assign allowed_access = SiteSetting.assigns_public || can_assign
if allowed_access && topics.length > 0 if allowed_access && topics.length > 0
users = User assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h
.where(<<~SQL, topic_ids: topics.map(&:id))
users.id in ( users_map = User
SELECT value::int .where(id: assignments.values.uniq)
FROM topic_custom_fields
WHERE name = 'assigned_to_id' AND topic_id IN (:topic_ids)
)
SQL
.select(UserLookup.lookup_columns) .select(UserLookup.lookup_columns)
.index_by(&:id)
users_map = users.index_by(&:id) topics.each do |topic|
user_id = assignments[topic.id]
topics.each do |t| user = users_map[user_id] if user_id
if id = t.custom_fields[TopicAssigner::ASSIGNED_TO_ID] topic.preload_assigned_to_user(user)
t.preload_assigned_to_user(users_map[id.to_i])
end
end end
end end
end end
@ -178,12 +185,14 @@ after_initialize do
allowed_access = SiteSetting.assigns_public || can_assign allowed_access = SiteSetting.assigns_public || can_assign
if allowed_access && results.posts.length > 0 if allowed_access && results.posts.length > 0
assigned_user_ids = results.posts.map(&:topic).map { |topic| topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] }.compact.uniq topics = results.posts.map(&:topic)
assigned_users = User.where(id: assigned_user_ids).index_by(&:id) assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h
users_map = User.where(id: assignments.values.uniq).index_by(&:id)
results.posts.each do |post| results.posts.each do |post|
post.topic.preload_assigned_to_user( user_id = assignments[post.topic.id]
assigned_users[post.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID]] user = users_map[user_id] if user_id
) post.topic.preload_assigned_to_user(user)
end end
end end
end end
@ -202,22 +211,15 @@ after_initialize do
if user_id || special if user_id || special
if username == "nobody" if username == "nobody"
results = results.joins("LEFT JOIN topic_custom_fields tc_assign ON results = results.joins("LEFT JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NULL")
topics.id = tc_assign.topic_id AND
tc_assign.name = 'assigned_to_id'")
.where("tc_assign.name IS NULL")
else else
if username == "*" if username == "*"
filter = "AND tc_assign.value IS NOT NULL" filter = "a.assigned_to_id IS NOT NULL"
else else
filter = "AND tc_assign.value = '#{user_id.to_i.to_s}'" filter = "a.assigned_to_id = #{user_id}"
end end
results = results.joins("JOIN topic_custom_fields tc_assign ON results = results.joins("JOIN assignments a ON a.topic_id = topics.id AND #{filter}")
topics.id = tc_assign.topic_id AND
tc_assign.name = 'assigned_to_id'
#{filter}
")
end end
end end
end end
@ -240,10 +242,9 @@ after_initialize do
list = list.where(" list = list.where("
topics.id IN ( topics.id IN (
SELECT topic_id FROM topic_custom_fields SELECT topic_id FROM assignments WHERE assigned_to_id = ?
WHERE name = 'assigned_to_id' )
AND value = ?) ", user.id)
", user.id.to_s)
create_list(:assigned, { unordered: true }, list) create_list(:assigned, { unordered: true }, list)
end end
@ -265,12 +266,12 @@ after_initialize do
add_to_class(:topic_query, :list_group_topics_assigned) do |group| add_to_class(:topic_query, :list_group_topics_assigned) do |group|
list = default_results(include_pms: true) list = default_results(include_pms: true)
list = list.where(" list = list.where(<<~SQL, group.id.to_s)
topics.id IN ( topics.id IN (
SELECT topic_id FROM topic_custom_fields SELECT topic_id FROM assignments
WHERE name = 'assigned_to_id' WHERE assigned_to_id IN (SELECT user_id from group_users where group_id = ?)
AND value IN (SELECT user_id::varchar(255) from group_users where group_id = ?)) )
", group.id.to_s) SQL
create_list(:assigned, { unordered: true }, list) create_list(:assigned, { unordered: true }, list)
end end
@ -302,17 +303,16 @@ after_initialize do
list = list.where(" list = list.where("
topics.id IN ( topics.id IN (
SELECT topic_id FROM topic_custom_fields SELECT topic_id FROM assignments WHERE assigned_to_id = ?
WHERE name = 'assigned_to_id' )
AND value = ?) ", user.id)
", user.id.to_s)
end end
add_to_class(:topic, :assigned_to_user) do add_to_class(:topic, :assigned_to_user) do
@assigned_to_user || return @assigned_to_user if defined?(@assigned_to_user)
if user_id = custom_fields[TopicAssigner::ASSIGNED_TO_ID]
@assigned_to_user = User.find_by(id: user_id) user_id = assignment&.assigned_to_id
end @assigned_to_user = user_id ? User.find_by(id: user_id) : nil
end end
add_to_class(:topic, :preload_assigned_to_user) do |assigned_to_user| add_to_class(:topic, :preload_assigned_to_user) do |assigned_to_user|
@ -343,10 +343,7 @@ after_initialize do
end end
add_to_serializer(:topic_view, 'include_assigned_to_user?') do add_to_serializer(:topic_view, 'include_assigned_to_user?') do
if SiteSetting.assigns_public || scope.can_assign? (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assigned_to_user
# subtle but need to catch cases where stuff is not assigned
object.topic.custom_fields.keys.include?(TopicAssigner::ASSIGNED_TO_ID)
end
end end
add_to_serializer(:search_topic_list_item, :assigned_to_user, false) do add_to_serializer(:search_topic_list_item, :assigned_to_user, false) do
@ -379,9 +376,7 @@ after_initialize do
register_permitted_bulk_action_parameter :username register_permitted_bulk_action_parameter :username
add_to_class(:user_bookmark_serializer, :assigned_to_user_id) do add_to_class(:user_bookmark_serializer, :assigned_to_user_id) do
id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] topic.assignment&.assigned_to_id
# a bit messy but race conditions can give us an array here, avoid
id && id.to_i rescue nil
end end
add_to_serializer(:user_bookmark, :assigned_to_user, false) do add_to_serializer(:user_bookmark, :assigned_to_user, false) do
@ -397,9 +392,7 @@ after_initialize do
end end
add_to_class(:topic_view_serializer, :assigned_to_user_id) do add_to_class(:topic_view_serializer, :assigned_to_user_id) do
id = object.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] object.topic.assignment&.assigned_to_id
# a bit messy but race conditions can give us an array here, avoid
id && id.to_i rescue nil
end end
add_to_serializer(:flagged_topic, :assigned_to_user) do add_to_serializer(:flagged_topic, :assigned_to_user) do
@ -407,9 +400,7 @@ after_initialize do
end end
add_to_serializer(:flagged_topic, :assigned_to_user_id) do add_to_serializer(:flagged_topic, :assigned_to_user_id) do
id = object.custom_fields[TopicAssigner::ASSIGNED_TO_ID] object.topic.assignment&.assigned_to_id
# a bit messy but race conditions can give us an array here, avoid
id && id.to_i rescue nil
end end
add_custom_reviewable_filter( add_custom_reviewable_filter(
@ -419,12 +410,11 @@ after_initialize do
results.joins(<<~SQL results.joins(<<~SQL
INNER JOIN posts p ON p.id = target_id INNER JOIN posts p ON p.id = target_id
INNER JOIN topics t ON t.id = p.topic_id INNER JOIN topics t ON t.id = p.topic_id
INNER JOIN topic_custom_fields tcf ON tcf.topic_id = t.id INNER JOIN assignments a ON a.topic_id = t.id
INNER JOIN users u ON u.id = tcf.value::integer INNER JOIN users u ON u.id = a.assigned_to_id
SQL SQL
) )
.where(target_type: Post.name) .where(target_type: Post.name)
.where('tcf.name = ?', TopicAssigner::ASSIGNED_TO_ID)
.where('u.username = ?', value) .where('u.username = ?', value)
end end
] ]
@ -458,7 +448,7 @@ after_initialize do
on(:move_to_inbox) do |info| on(:move_to_inbox) do |info|
topic = info[:topic] topic = info[:topic]
if (assigned_id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i) == info[:user]&.id if (assigned_id = topic.assignment&.assigned_to_id) == info[:user]&.id
TopicTrackingState.publish_assigned_private_message(topic, assigned_id) TopicTrackingState.publish_assigned_private_message(topic, assigned_id)
end end
@ -473,7 +463,7 @@ after_initialize do
on(:archive_message) do |info| on(:archive_message) do |info|
topic = info[:topic] topic = info[:topic]
user_id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i user_id = topic.assignment&.assigned_to_id
if user_id == info[:user]&.id if user_id == info[:user]&.id
TopicTrackingState.publish_assigned_private_message(topic, user_id) TopicTrackingState.publish_assigned_private_message(topic, user_id)
@ -501,36 +491,32 @@ after_initialize do
register_search_advanced_filter(/in:assigned/) do |posts| register_search_advanced_filter(/in:assigned/) do |posts|
if @guardian.can_assign? if @guardian.can_assign?
posts.where("topics.id IN ( posts.where(<<~SQL)
SELECT tc.topic_id topics.id IN (
FROM topic_custom_fields tc SELECT a.topic_id FROM assignments a
WHERE tc.name = 'assigned_to_id' AND )
tc.value IS NOT NULL SQL
)")
end end
end end
register_search_advanced_filter(/in:unassigned/) do |posts| register_search_advanced_filter(/in:unassigned/) do |posts|
if @guardian.can_assign? if @guardian.can_assign?
posts.where("topics.id NOT IN ( posts.where(<<~SQL)
SELECT tc.topic_id topics.id NOT IN (
FROM topic_custom_fields tc SELECT a.topic_id FROM assignments a
WHERE tc.name = 'assigned_to_id' AND )
tc.value IS NOT NULL SQL
)")
end end
end end
register_search_advanced_filter(/assigned:(.+)$/) do |posts, match| register_search_advanced_filter(/assigned:(.+)$/) do |posts, match|
if @guardian.can_assign? if @guardian.can_assign?
if user_id = User.find_by_username(match)&.id if user_id = User.find_by_username(match)&.id
posts.where("topics.id IN ( posts.where(<<~SQL, user_id)
SELECT tc.topic_id topics.id IN (
FROM topic_custom_fields tc SELECT a.topic_id FROM assignments a WHERE a.assigned_to_id = ?
WHERE tc.name = 'assigned_to_id' AND )
tc.value IS NOT NULL AND SQL
tc.value::int = #{user_id}
)")
end end
end end
end end
@ -542,11 +528,7 @@ after_initialize do
groups = GroupUser.where(user: user).pluck(:group_id) groups = GroupUser.where(user: user).pluck(:group_id)
if (groups & assign_allowed_groups).empty? if (groups & assign_allowed_groups).empty?
topics = Topic.joins(:_custom_fields) topics = Topic.joins(:assignment).where('assignments.assigned_to_id = ?', user.id)
.where(
'topic_custom_fields.name = ? AND topic_custom_fields.value = ?',
TopicAssigner::ASSIGNED_TO_ID, user.id.to_s
)
topics.each do |topic| topics.each do |topic|
TopicAssigner.new(topic, Discourse.system_user).unassign TopicAssigner.new(topic, Discourse.system_user).unassign

2
spec/components/search_spec.rb
View File

@ -4,7 +4,6 @@ require 'rails_helper'
require_relative '../support/assign_allowed_group' require_relative '../support/assign_allowed_group'
describe Search do describe Search do
fab!(:user) { Fabricate(:active_user) } fab!(:user) { Fabricate(:active_user) }
fab!(:user2) { Fabricate(:user) } fab!(:user2) { Fabricate(:user) }
@ -37,6 +36,5 @@ describe Search do
expect(Search.execute('in:unassigned', guardian: Guardian.new(user)).posts.length).to eq(1) expect(Search.execute('in:unassigned', guardian: Guardian.new(user)).posts.length).to eq(1)
expect(Search.execute("assigned:#{user.username}", guardian: Guardian.new(user)).posts.length).to eq(1) expect(Search.execute("assigned:#{user.username}", guardian: Guardian.new(user)).posts.length).to eq(1)
end end
end end
end end

19
spec/integration/assign_spec.rb
View File

@ -17,19 +17,6 @@ describe 'integration tests' do
# should not explode for now # should not explode for now
end end
describe 'data consistency' do
it 'can deal with problem custom fields' do
post = Fabricate(:post)
post.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] = [nil, nil]
post.topic.save_custom_fields
TopicAssigner.new(Topic.find(post.topic_id), Discourse.system_user).unassign
post.topic.reload
expect(post.topic.custom_fields).to eq({})
end
end
describe 'for a private message' do describe 'for a private message' do
let(:post) { Fabricate(:private_message_post) } let(:post) { Fabricate(:private_message_post) }
let(:pm) { post.topic } let(:pm) { post.topic }
@ -85,13 +72,13 @@ describe 'integration tests' do
it "assigns topic" do it "assigns topic" do
DiscourseEvent.trigger(:assign_topic, topic, user1, admin) DiscourseEvent.trigger(:assign_topic, topic, user1, admin)
expect(topic.reload.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i).to eq(user1.id) expect(topic.assignment.assigned_to_id).to eq(user1.id)
DiscourseEvent.trigger(:assign_topic, topic, user2, admin) DiscourseEvent.trigger(:assign_topic, topic, user2, admin)
expect(topic.reload.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i).to eq(user1.id) expect(topic.assignment.assigned_to_id).to eq(user1.id)
DiscourseEvent.trigger(:assign_topic, topic, user2, admin, true) DiscourseEvent.trigger(:assign_topic, topic, user2, admin, true)
expect(topic.reload.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i).to eq(user2.id) expect(topic.assignment.assigned_to_id).to eq(user2.id)
end end
it "triggers a webhook for assigned and unassigned" do it "triggers a webhook for assigned and unassigned" do

8
spec/lib/pending_assigns_reminder_spec.rb
View File

@ -3,6 +3,10 @@
require 'rails_helper' require 'rails_helper'
require_relative '../support/assign_allowed_group' require_relative '../support/assign_allowed_group'
def assert_reminder_not_created
expect { subject.remind(user) }.to change { Post.count }.by(0)
end
RSpec.describe PendingAssignsReminder do RSpec.describe PendingAssignsReminder do
before { SiteSetting.assign_enabled = true } before { SiteSetting.assign_enabled = true }
@ -19,10 +23,6 @@ RSpec.describe PendingAssignsReminder do
assert_reminder_not_created assert_reminder_not_created
end end
def assert_reminder_not_created
expect { subject.remind(user) }.to change { Post.count }.by(0)
end
describe 'when the user has multiple tasks' do describe 'when the user has multiple tasks' do
let(:system) { Discourse.system_user } let(:system) { Discourse.system_user }

12
spec/lib/topic_assigner_spec.rb
View File

@ -120,16 +120,14 @@ RSpec.describe TopicAssigner do
it "doesn't assign system user" do it "doesn't assign system user" do
TopicAssigner.auto_assign(post) TopicAssigner.auto_assign(post)
expect(topic.custom_fields["assigned_to_id"]) expect(topic.assignment).to eq(nil)
.to eq(nil)
end end
it "assigns first mentioned staff user after system user" do it "assigns first mentioned staff user after system user" do
post.update(raw: "Don't assign @system. @modi, can you add this to your list?") post.update(raw: "Don't assign @system. @modi, can you add this to your list?")
TopicAssigner.auto_assign(post) TopicAssigner.auto_assign(post)
expect(topic.custom_fields["assigned_to_id"].to_i) expect(topic.assignment.assigned_to_id).to eq(moderator.id)
.to eq(moderator.id)
end end
end end
@ -234,7 +232,8 @@ RSpec.describe TopicAssigner do
it "automatically assigns to myself" do it "automatically assigns to myself" do
expect(TopicAssigner.auto_assign(reply)).to eq(success: true) expect(TopicAssigner.auto_assign(reply)).to eq(success: true)
expect(op.topic.custom_fields).to eq("assigned_to_id" => me.id.to_s, "assigned_by_id" => me.id.to_s) expect(op.topic.assignment.assigned_to_id).to eq(me.id)
expect(op.topic.assignment.assigned_by_user_id).to eq(me.id)
end end
it "does not automatically assign to myself" do it "does not automatically assign to myself" do
@ -273,7 +272,8 @@ RSpec.describe TopicAssigner do
it "automatically assigns to other" do it "automatically assigns to other" do
expect(TopicAssigner.auto_assign(reply)).to eq(success: true) expect(TopicAssigner.auto_assign(reply)).to eq(success: true)
expect(op.topic.custom_fields).to eq("assigned_to_id" => other.id.to_s, "assigned_by_id" => me.id.to_s) expect(op.topic.assignment.assigned_to_id).to eq(other.id)
expect(op.topic.assignment.assigned_by_user_id).to eq(me.id)
end end
end end

10
spec/plugin_spec.rb
View File

@ -20,9 +20,7 @@ describe 'plugin' do
TopicAssigner.new(@topic, Discourse.system_user).assign(@user) TopicAssigner.new(@topic, Discourse.system_user).assign(@user)
@group_a.remove(@user) @group_a.remove(@user)
custom_fields = @topic.reload.custom_fields expect(Assignment.count).to eq(0)
expect(custom_fields[TopicAssigner::ASSIGNED_TO_ID]).to be_nil
expect(custom_fields[TopicAssigner::ASSIGNED_BY_ID]).to be_nil
end end
it "doesn't unassign the user if it still has access through another group" do it "doesn't unassign the user if it still has access through another group" do
@ -33,9 +31,9 @@ describe 'plugin' do
TopicAssigner.new(@topic, Discourse.system_user).assign(@user) TopicAssigner.new(@topic, Discourse.system_user).assign(@user)
@group_a.remove(@user) @group_a.remove(@user)
custom_fields = @topic.reload.custom_fields assignment = Assignment.first
expect(custom_fields[TopicAssigner::ASSIGNED_TO_ID]).to eq(@user.id.to_s) expect(assignment.assigned_to_id).to eq(@user.id)
expect(custom_fields[TopicAssigner::ASSIGNED_BY_ID]).to eq(Discourse::SYSTEM_USER_ID.to_s) expect(assignment.assigned_by_user_id).to eq(Discourse::SYSTEM_USER_ID)
end end
end end
end end

51
spec/requests/assign_controller_spec.rb
View File

@ -4,18 +4,17 @@ require 'rails_helper'
require_relative '../support/assign_allowed_group' require_relative '../support/assign_allowed_group'
RSpec.describe DiscourseAssign::AssignController do RSpec.describe DiscourseAssign::AssignController do
before { SiteSetting.assign_enabled = true } before { SiteSetting.assign_enabled = true }
let(:default_allowed_group) { Group.find_by(name: 'staff') } let(:default_allowed_group) { Group.find_by(name: 'staff') }
let(:user) { Fabricate(:admin, groups: [default_allowed_group], name: 'Robin Ward', username: 'eviltrout') } let(:user) { Fabricate(:admin, groups: [default_allowed_group], name: 'Robin Ward', username: 'eviltrout') }
let(:post) { Fabricate(:post) } fab!(:post) { Fabricate(:post) }
let(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david') } fab!(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david') }
let(:nonadmin) { Fabricate(:user, groups: [default_allowed_group]) } let(:nonadmin) { Fabricate(:user, groups: [default_allowed_group]) }
let(:normal_user) { Fabricate(:user) } fab!(:normal_user) { Fabricate(:user) }
let(:normal_admin) { Fabricate(:admin) } fab!(:normal_admin) { Fabricate(:admin) }
describe 'only allow users from allowed groups' do context 'only allow users from allowed groups' do
before { sign_in(user2) } before { sign_in(user2) }
it 'filters requests where current_user is not member of an allowed group' do it 'filters requests where current_user is not member of an allowed group' do
@ -28,7 +27,7 @@ RSpec.describe DiscourseAssign::AssignController do
expect(response.status).to eq(403) expect(response.status).to eq(403)
end end
context '#suggestions' do describe '#suggestions' do
before { sign_in(user) } before { sign_in(user) }
it 'includes users in allowed groups' do it 'includes users in allowed groups' do
@ -75,7 +74,7 @@ RSpec.describe DiscourseAssign::AssignController do
end end
end end
context "#suggestions" do describe "#suggestions" do
before do before do
SiteSetting.max_assigned_topics = 1 SiteSetting.max_assigned_topics = 1
sign_in(user) sign_in(user)
@ -92,8 +91,7 @@ RSpec.describe DiscourseAssign::AssignController do
end end
end end
context '#assign' do describe '#assign' do
include_context 'A group that is allowed to assign' include_context 'A group that is allowed to assign'
before do before do
@ -107,7 +105,7 @@ RSpec.describe DiscourseAssign::AssignController do
} }
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(post.topic.reload.custom_fields['assigned_to_id']).to eq(user2.id.to_s) expect(post.topic.reload.assignment.assigned_to_id).to eq(user2.id)
end end
it 'fails to assign topic to the user if its already assigned to the same user' do it 'fails to assign topic to the user if its already assigned to the same user' do
@ -116,7 +114,7 @@ RSpec.describe DiscourseAssign::AssignController do
} }
expect(response.status).to eq(200) expect(response.status).to eq(200)
expect(post.topic.reload.custom_fields['assigned_to_id']).to eq(user2.id.to_s) expect(post.topic.reload.assignment.assigned_to_id).to eq(user2.id)
put '/assign/assign.json', params: { put '/assign/assign.json', params: {
topic_id: post.topic_id, username: user2.username topic_id: post.topic_id, username: user2.username
@ -171,7 +169,7 @@ RSpec.describe DiscourseAssign::AssignController do
end end
end end
context '#assigned' do describe '#assigned' do
include_context 'A group that is allowed to assign' include_context 'A group that is allowed to assign'
fab!(:post1) { Fabricate(:post) } fab!(:post1) { Fabricate(:post) }
@ -207,6 +205,7 @@ RSpec.describe DiscourseAssign::AssignController do
context "with custom allowed groups" do context "with custom allowed groups" do
let(:custom_allowed_group) { Fabricate(:group, name: 'mygroup') } let(:custom_allowed_group) { Fabricate(:group, name: 'mygroup') }
let(:other_user) { Fabricate(:user, groups: [custom_allowed_group]) } let(:other_user) { Fabricate(:user, groups: [custom_allowed_group]) }
before do before do
SiteSetting.assign_allowed_on_groups += "|#{custom_allowed_group.id}" SiteSetting.assign_allowed_on_groups += "|#{custom_allowed_group.id}"
end end
@ -224,7 +223,7 @@ RSpec.describe DiscourseAssign::AssignController do
end end
end end
context '#group_members' do describe '#group_members' do
include_context 'A group that is allowed to assign' include_context 'A group that is allowed to assign'
fab!(:post1) { Fabricate(:post) } fab!(:post1) { Fabricate(:post) }
@ -287,4 +286,28 @@ RSpec.describe DiscourseAssign::AssignController do
expect(response.status).to eq(200) expect(response.status).to eq(200)
end end
end end
describe "#claim" do
it "assigns the topic to the current user" do
sign_in(user)
put "/assign/claim/#{post.topic_id}.json"
expect(response.status).to eq(200)
assignment = Assignment.first
expect(assignment.assigned_to_id).to eq(user.id)
expect(assignment.topic_id).to eq(post.topic_id)
end
it "returns an error if already claimed" do
TopicAssigner.new(post.topic, user).assign(user)
sign_in(user)
put "/assign/claim/#{post.topic_id}.json"
expect(response.status).to eq(422)
expect(response.parsed_body["errors"].first).to eq(I18n.t('discourse_assign.already_claimed'))
expect(response.parsed_body["extras"]["assigned_to"]["username"]).to eq(user.username)
end
end
end end