diff --git a/app/controllers/discourse_assign/assign_controller.rb b/app/controllers/discourse_assign/assign_controller.rb index 612a3b2..98b1406 100644 --- a/app/controllers/discourse_assign/assign_controller.rb +++ b/app/controllers/discourse_assign/assign_controller.rb @@ -11,12 +11,11 @@ module DiscourseAssign .where('users.id <> ?', current_user.id) .joins(<<~SQL JOIN( - SELECT value::integer user_id, MAX(created_at) last_assigned - FROM topic_custom_fields - WHERE name = 'assigned_to_id' - GROUP BY value::integer - HAVING COUNT(*) < #{SiteSetting.max_assigned_topics} - ) as X ON X.user_id = users.id + SELECT assigned_to_id user_id, MAX(created_at) last_assigned + FROM assignments + GROUP BY assigned_to_id + HAVING COUNT(*) < #{SiteSetting.max_assigned_topics} + ) as X ON X.user_id = users.id SQL ) .assign_allowed @@ -33,18 +32,18 @@ module DiscourseAssign topic_id = params.require(:topic_id).to_i topic = Topic.find(topic_id) - assigned = TopicCustomField.where( - "topic_id = :topic_id AND name = 'assigned_to_id' AND value IS NOT NULL", - topic_id: topic_id - ).pluck(:value) + assigned_id = Assignment + .where(topic_id: topic_id) + .where.not(assigned_to_id: nil) + .pluck_first(:assigned_to_id) - if assigned && user_id = assigned[0] - extras = nil - if user = User.where(id: user_id).first + if assigned_id + if user = User.where(id: assigned_id).first extras = { assigned_to: serialize_data(user, BasicUserSerializer, root: false) } end + return render_json_error(I18n.t('discourse_assign.already_claimed'), extras: extras) end @@ -91,26 +90,28 @@ module DiscourseAssign topics = Topic .includes(:tags) .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") - .order('tcf.value::integer, topics.bumped_at desc') + .joins("JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NOT NULL") + .order("a.assigned_to_id, topics.bumped_at desc") .offset(offset) .limit(limit) Topic.preload_custom_fields(topics, TopicList.preloaded_custom_fields) + assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h + 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)) - .joins('join user_emails on user_emails.user_id = users.id AND user_emails.primary') + .where("users.id IN (?)", assignments.values.uniq) + .joins("join user_emails on user_emails.user_id = users.id AND user_emails.primary") .select(UserLookup.lookup_columns) .to_a User.preload_custom_fields(users, User.allowed_user_custom_fields(guardian)) - users = users.to_h { |u| [u.id, u] } - topics.each do |t| - if id = t.custom_fields[TopicAssigner::ASSIGNED_TO_ID] - t.preload_assigned_to_user(users[id.to_i]) - end + users_map = users.index_by(&:id) + + topics.each do |topic| + user_id = assignments[topic.id] + topic.preload_assigned_to_user(users_map[user_id]) if user_id end render json: { topics: serialize_data(topics, AssignedTopicSerializer) } @@ -129,26 +130,30 @@ module DiscourseAssign guardian.ensure_can_see_group_members!(group) members = User - .joins("LEFT OUTER JOIN group_users g on users.id=g.user_id") - .joins("LEFT OUTER JOIN topic_custom_fields tcf ON tcf.value::int = users.id") - .joins("LEFT OUTER JOIN topics t ON t.id = tcf.topic_id") - .where("tcf.name = 'assigned_to_id' AND g.group_id=? AND (users.id > 0) AND t.deleted_at IS NULL", group.id) + .joins("LEFT OUTER JOIN group_users g ON g.user_id = users.id") + .joins("LEFT OUTER JOIN assignments a ON a.assigned_to_id = users.id") + .joins("LEFT OUTER JOIN topics t ON t.id = a.topic_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') .group('users.id') .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) .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") - .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) + if params[:filter] + 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") .count - render json: { members: serialize_data(members, GroupUserAssignedSerializer), "assignment_count" => assignment_count } + render json: { members: serialize_data(members, GroupUserAssignedSerializer), assignment_count: assignment_count } end private diff --git a/app/models/assignment.rb b/app/models/assignment.rb new file mode 100644 index 0000000..936c1cc --- /dev/null +++ b/app/models/assignment.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +class Assignment < ActiveRecord::Base + belongs_to :topic +end diff --git a/db/migrate/20210627100932_create_assignments.rb b/db/migrate/20210627100932_create_assignments.rb new file mode 100644 index 0000000..0eb7144 --- /dev/null +++ b/db/migrate/20210627100932_create_assignments.rb @@ -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 diff --git a/db/post_migrate/20210709101534_move_assignments_from_custom_fields_to_a_table.rb b/db/post_migrate/20210709101534_move_assignments_from_custom_fields_to_a_table.rb new file mode 100644 index 0000000..d2db7ae --- /dev/null +++ b/db/post_migrate/20210709101534_move_assignments_from_custom_fields_to_a_table.rb @@ -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 diff --git a/jobs/scheduled/enqueue_reminders.rb b/jobs/scheduled/enqueue_reminders.rb index 041a9a1..687afe4 100644 --- a/jobs/scheduled/enqueue_reminders.rb +++ b/jobs/scheduled/enqueue_reminders.rb @@ -24,19 +24,19 @@ module Jobs frequency = ActiveRecord::Base.sanitize_sql("COALESCE(user_frequency.value, '#{global_frequency}')::INT") DB.query_single(<<~SQL - SELECT topic_custom_fields.value - FROM topic_custom_fields + SELECT assignments.assigned_to_id + FROM assignments 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}' 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}' - INNER JOIN group_users ON topic_custom_fields.value::INT = group_users.user_id - INNER JOIN topics ON topics.id = topic_custom_fields.topic_id AND (topics.deleted_at IS NULL) + INNER JOIN group_users ON assignments.assigned_to_id = group_users.user_id + INNER JOIN topics ON topics.id = assignments.topic_id AND topics.deleted_at IS NULL WHERE group_users.group_id IN (#{allowed_group_ids}) AND #{frequency} > 0 @@ -44,11 +44,10 @@ module Jobs last_reminder.value IS NULL OR last_reminder.value::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) ) - AND topic_custom_fields.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) - AND topic_custom_fields.name = '#{TopicAssigner::ASSIGNED_TO_ID}' + AND assignments.updated_at::TIMESTAMP <= CURRENT_TIMESTAMP - ('1 MINUTE'::INTERVAL * #{frequency}) - GROUP BY topic_custom_fields.value - HAVING COUNT(topic_custom_fields.value) > 1 + GROUP BY assignments.assigned_to_id + HAVING COUNT(assignments.assigned_to_id) > 1 SQL ) end diff --git a/lib/discourse_assign/helpers.rb b/lib/discourse_assign/helpers.rb index d325737..bbba57b 100644 --- a/lib/discourse_assign/helpers.rb +++ b/lib/discourse_assign/helpers.rb @@ -4,10 +4,7 @@ module DiscourseAssign module Helpers 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) - assigned_at = TopicCustomField.where( - topic_id: topic.id, - name: "assigned_to_id" - ).pluck(:created_at).first + assigned_at = Assignment.where(topic_id: topic.id).pluck_first(:created_at) { username: user.username, diff --git a/lib/pending_assigns_reminder.rb b/lib/pending_assigns_reminder.rb index d4fcb43..6eef72c 100644 --- a/lib/pending_assigns_reminder.rb +++ b/lib/pending_assigns_reminder.rb @@ -28,19 +28,18 @@ class PendingAssignsReminder private def assigned_count_for(user) - TopicCustomField - .joins(:topic) - .where(name: TopicAssigner::ASSIGNED_TO_ID, value: user.id) - .count + Assignment.joins(:topic).where(assigned_to_id: user.id).count end def assigned_topics(user, order:) 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') - .where('topic_custom_fields.name = ? AND topic_custom_fields.value = ?', TopicAssigner::ASSIGNED_TO_ID, user.id.to_s) + Topic + .joins(:assignment) + .select(:slug, :id, :title, :fancy_title, 'assignments.created_at AS assigned_at') + .where('assignments.assigned_to_id = ?', user.id) .merge(secure) - .order("topic_custom_fields.created_at #{order}") + .order("assignments.created_at #{order}") .limit(3) end diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index 13d59bb..f824d1f 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -4,10 +4,6 @@ require 'email/sender' require 'nokogiri' class ::TopicAssigner - - ASSIGNED_TO_ID ||= 'assigned_to_id' - ASSIGNED_BY_ID ||= 'assigned_by_id' - def self.backfill_auto_assign staff_mention = User .assign_allowed @@ -19,10 +15,10 @@ class ::TopicAssigner SELECT p.topic_id, MAX(post_number) post_number FROM posts p 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) AND (#{staff_mention}) - AND tc.value IS NULL + AND a.assigned_to_id IS NULL AND NOT t.closed AND t.deleted_at IS NULL GROUP BY p.topic_id @@ -57,7 +53,7 @@ class ::TopicAssigner return unless SiteSetting.assigns_by_staff_mention 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 doc = Nokogiri::HTML5.fragment(post.cooked) @@ -124,11 +120,10 @@ class ::TopicAssigner def can_assign_to?(user) return true if @assigned_by.id == user.id - assigned_total = TopicCustomField + assigned_total = Assignment .joins(:topic) .where(topics: { deleted_at: nil }) - .where(name: ASSIGNED_TO_ID) - .where(value: user.id) + .where(assigned_to_id: user.id) .count assigned_total < SiteSetting.max_assigned_topics @@ -148,12 +143,11 @@ class ::TopicAssigner return { success: false, reason: reason } end 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) - @topic.custom_fields[ASSIGNED_TO_ID] = assign_to.id - @topic.custom_fields[ASSIGNED_BY_ID] = @assigned_by.id - @topic.save_custom_fields + @topic.assignment&.destroy! + @topic.create_assignment!(assigned_to_id: assign_to.id, assigned_by_user_id: @assigned_by.id) first_post = @topic.posts.find_by(post_number: 1) first_post.publish_change_to_clients!(:revised, reload_topic: true) @@ -260,45 +254,26 @@ class ::TopicAssigner end { success: true } - end def unassign(silent: false) - if assigned_to_id = @topic.custom_fields[ASSIGNED_TO_ID] - - # 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 + if assignment = @topic.assignment + assignment.destroy! 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) if TopicUser.exists?( - user_id: assigned_to_id, + user_id: assignment.assigned_to_id, topic: @topic, notification_level: TopicUser.notification_levels[:watching], notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] ) TopicUser.change( - assigned_to_id, + assignment.assigned_to_id, @topic.id, notification_level: TopicUser.notification_levels[:tracking], notifications_reason_id: TopicUser.notification_reasons[:plugin_changed] @@ -314,7 +289,7 @@ class ::TopicAssigner 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) UserAction.where( diff --git a/plugin.rb b/plugin.rb index fb82444..60dcbf3 100644 --- a/plugin.rb +++ b/plugin.rb @@ -32,6 +32,10 @@ after_initialize do require 'topic_assigner' require 'pending_assigns_reminder' + class ::Topic + has_one :assignment, dependent: :destroy + end + frequency_field = PendingAssignsReminder::REMINDERS_FREQUENCY register_editable_user_custom_field frequency_field User.register_custom_field_type frequency_field, :integer @@ -41,8 +45,18 @@ after_initialize do end 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") - .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) + Topic + .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") .count end @@ -118,7 +132,7 @@ after_initialize do end 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) end end @@ -127,20 +141,18 @@ after_initialize do TopicAssigner.new(topic, unassigning_user).unassign end - TopicList.preloaded_custom_fields << TopicAssigner::ASSIGNED_TO_ID 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| if SiteSetting.assign_enabled? - assigned_user_ids = bookmarks.map(&:topic).map { |topic| topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] }.compact.uniq - assigned_users = User.where(id: assigned_user_ids).index_by(&:id) + topics = bookmarks.map(&:topic) + 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| - bookmark.topic.preload_assigned_to_user( - assigned_users[bookmark.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID]] - ) + topics.each do |topic| + user_id = assignments[topic.id] + user = users_map[user_id] if user_id + topic.preload_assigned_to_user(user) end end end @@ -151,22 +163,17 @@ after_initialize do allowed_access = SiteSetting.assigns_public || can_assign if allowed_access && topics.length > 0 - users = User - .where(<<~SQL, topic_ids: topics.map(&:id)) - users.id in ( - SELECT value::int - FROM topic_custom_fields - WHERE name = 'assigned_to_id' AND topic_id IN (:topic_ids) - ) - SQL + assignments = Assignment.where(topic: topics).pluck(:topic_id, :assigned_to_id).to_h + + users_map = User + .where(id: assignments.values.uniq) .select(UserLookup.lookup_columns) + .index_by(&:id) - users_map = users.index_by(&:id) - - topics.each do |t| - if id = t.custom_fields[TopicAssigner::ASSIGNED_TO_ID] - t.preload_assigned_to_user(users_map[id.to_i]) - end + topics.each do |topic| + user_id = assignments[topic.id] + user = users_map[user_id] if user_id + topic.preload_assigned_to_user(user) end end end @@ -178,12 +185,14 @@ after_initialize do allowed_access = SiteSetting.assigns_public || can_assign 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 - assigned_users = User.where(id: assigned_user_ids).index_by(&:id) + topics = results.posts.map(&:topic) + 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| - post.topic.preload_assigned_to_user( - assigned_users[post.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID]] - ) + user_id = assignments[post.topic.id] + user = users_map[user_id] if user_id + post.topic.preload_assigned_to_user(user) end end end @@ -202,22 +211,15 @@ after_initialize do if user_id || special if username == "nobody" - results = results.joins("LEFT JOIN topic_custom_fields tc_assign ON - topics.id = tc_assign.topic_id AND - tc_assign.name = 'assigned_to_id'") - .where("tc_assign.name IS NULL") + results = results.joins("LEFT JOIN assignments a ON a.topic_id = topics.id AND a.assigned_to_id IS NULL") else if username == "*" - filter = "AND tc_assign.value IS NOT NULL" + filter = "a.assigned_to_id IS NOT NULL" else - filter = "AND tc_assign.value = '#{user_id.to_i.to_s}'" + filter = "a.assigned_to_id = #{user_id}" end - results = results.joins("JOIN topic_custom_fields tc_assign ON - topics.id = tc_assign.topic_id AND - tc_assign.name = 'assigned_to_id' - #{filter} - ") + results = results.joins("JOIN assignments a ON a.topic_id = topics.id AND #{filter}") end end end @@ -240,10 +242,9 @@ after_initialize do list = list.where(" topics.id IN ( - SELECT topic_id FROM topic_custom_fields - WHERE name = 'assigned_to_id' - AND value = ?) - ", user.id.to_s) + SELECT topic_id FROM assignments WHERE assigned_to_id = ? + ) + ", user.id) create_list(:assigned, { unordered: true }, list) end @@ -265,12 +266,12 @@ after_initialize do add_to_class(:topic_query, :list_group_topics_assigned) do |group| list = default_results(include_pms: true) - list = list.where(" + list = list.where(<<~SQL, group.id.to_s) topics.id IN ( - SELECT topic_id FROM topic_custom_fields - WHERE name = 'assigned_to_id' - AND value IN (SELECT user_id::varchar(255) from group_users where group_id = ?)) - ", group.id.to_s) + SELECT topic_id FROM assignments + WHERE assigned_to_id IN (SELECT user_id from group_users where group_id = ?) + ) + SQL create_list(:assigned, { unordered: true }, list) end @@ -302,17 +303,16 @@ after_initialize do list = list.where(" topics.id IN ( - SELECT topic_id FROM topic_custom_fields - WHERE name = 'assigned_to_id' - AND value = ?) - ", user.id.to_s) + SELECT topic_id FROM assignments WHERE assigned_to_id = ? + ) + ", user.id) end add_to_class(:topic, :assigned_to_user) do - @assigned_to_user || - if user_id = custom_fields[TopicAssigner::ASSIGNED_TO_ID] - @assigned_to_user = User.find_by(id: user_id) - end + return @assigned_to_user if defined?(@assigned_to_user) + + user_id = assignment&.assigned_to_id + @assigned_to_user = user_id ? User.find_by(id: user_id) : nil end add_to_class(:topic, :preload_assigned_to_user) do |assigned_to_user| @@ -343,10 +343,7 @@ after_initialize do end add_to_serializer(:topic_view, 'include_assigned_to_user?') do - if SiteSetting.assigns_public || scope.can_assign? - # subtle but need to catch cases where stuff is not assigned - object.topic.custom_fields.keys.include?(TopicAssigner::ASSIGNED_TO_ID) - end + (SiteSetting.assigns_public || scope.can_assign?) && object.topic.assigned_to_user end 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 add_to_class(:user_bookmark_serializer, :assigned_to_user_id) do - id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] - # a bit messy but race conditions can give us an array here, avoid - id && id.to_i rescue nil + topic.assignment&.assigned_to_id end add_to_serializer(:user_bookmark, :assigned_to_user, false) do @@ -397,9 +392,7 @@ after_initialize do end add_to_class(:topic_view_serializer, :assigned_to_user_id) do - id = object.topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID] - # a bit messy but race conditions can give us an array here, avoid - id && id.to_i rescue nil + object.topic.assignment&.assigned_to_id end add_to_serializer(:flagged_topic, :assigned_to_user) do @@ -407,9 +400,7 @@ after_initialize do end add_to_serializer(:flagged_topic, :assigned_to_user_id) do - id = object.custom_fields[TopicAssigner::ASSIGNED_TO_ID] - # a bit messy but race conditions can give us an array here, avoid - id && id.to_i rescue nil + object.topic.assignment&.assigned_to_id end add_custom_reviewable_filter( @@ -419,12 +410,11 @@ after_initialize do results.joins(<<~SQL INNER JOIN posts p ON p.id = target_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 users u ON u.id = tcf.value::integer + INNER JOIN assignments a ON a.topic_id = t.id + INNER JOIN users u ON u.id = a.assigned_to_id SQL ) .where(target_type: Post.name) - .where('tcf.name = ?', TopicAssigner::ASSIGNED_TO_ID) .where('u.username = ?', value) end ] @@ -458,7 +448,7 @@ after_initialize do on(:move_to_inbox) do |info| 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) end @@ -473,7 +463,7 @@ after_initialize do on(:archive_message) do |info| 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 TopicTrackingState.publish_assigned_private_message(topic, user_id) @@ -501,36 +491,32 @@ after_initialize do register_search_advanced_filter(/in:assigned/) do |posts| if @guardian.can_assign? - posts.where("topics.id IN ( - SELECT tc.topic_id - FROM topic_custom_fields tc - WHERE tc.name = 'assigned_to_id' AND - tc.value IS NOT NULL - )") + posts.where(<<~SQL) + topics.id IN ( + SELECT a.topic_id FROM assignments a + ) + SQL end end register_search_advanced_filter(/in:unassigned/) do |posts| if @guardian.can_assign? - posts.where("topics.id NOT IN ( - SELECT tc.topic_id - FROM topic_custom_fields tc - WHERE tc.name = 'assigned_to_id' AND - tc.value IS NOT NULL - )") + posts.where(<<~SQL) + topics.id NOT IN ( + SELECT a.topic_id FROM assignments a + ) + SQL end end register_search_advanced_filter(/assigned:(.+)$/) do |posts, match| if @guardian.can_assign? if user_id = User.find_by_username(match)&.id - posts.where("topics.id IN ( - SELECT tc.topic_id - FROM topic_custom_fields tc - WHERE tc.name = 'assigned_to_id' AND - tc.value IS NOT NULL AND - tc.value::int = #{user_id} - )") + posts.where(<<~SQL, user_id) + topics.id IN ( + SELECT a.topic_id FROM assignments a WHERE a.assigned_to_id = ? + ) + SQL end end end @@ -542,11 +528,7 @@ after_initialize do groups = GroupUser.where(user: user).pluck(:group_id) if (groups & assign_allowed_groups).empty? - topics = Topic.joins(:_custom_fields) - .where( - 'topic_custom_fields.name = ? AND topic_custom_fields.value = ?', - TopicAssigner::ASSIGNED_TO_ID, user.id.to_s - ) + topics = Topic.joins(:assignment).where('assignments.assigned_to_id = ?', user.id) topics.each do |topic| TopicAssigner.new(topic, Discourse.system_user).unassign diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 06eeb50..9ffe2fc 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -4,7 +4,6 @@ require 'rails_helper' require_relative '../support/assign_allowed_group' describe Search do - fab!(:user) { Fabricate(:active_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("assigned:#{user.username}", guardian: Guardian.new(user)).posts.length).to eq(1) end - end end diff --git a/spec/integration/assign_spec.rb b/spec/integration/assign_spec.rb index 0b03e61..0a6d417 100644 --- a/spec/integration/assign_spec.rb +++ b/spec/integration/assign_spec.rb @@ -17,19 +17,6 @@ describe 'integration tests' do # should not explode for now 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 let(:post) { Fabricate(:private_message_post) } let(:pm) { post.topic } @@ -85,13 +72,13 @@ describe 'integration tests' do it "assigns topic" do 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) - 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) - 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 it "triggers a webhook for assigned and unassigned" do diff --git a/spec/lib/pending_assigns_reminder_spec.rb b/spec/lib/pending_assigns_reminder_spec.rb index a67cf05..99b493a 100644 --- a/spec/lib/pending_assigns_reminder_spec.rb +++ b/spec/lib/pending_assigns_reminder_spec.rb @@ -3,6 +3,10 @@ require 'rails_helper' 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 before { SiteSetting.assign_enabled = true } @@ -19,10 +23,6 @@ RSpec.describe PendingAssignsReminder do assert_reminder_not_created 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 let(:system) { Discourse.system_user } diff --git a/spec/lib/topic_assigner_spec.rb b/spec/lib/topic_assigner_spec.rb index 638f59e..0ea2fde 100644 --- a/spec/lib/topic_assigner_spec.rb +++ b/spec/lib/topic_assigner_spec.rb @@ -120,16 +120,14 @@ RSpec.describe TopicAssigner do it "doesn't assign system user" do TopicAssigner.auto_assign(post) - expect(topic.custom_fields["assigned_to_id"]) - .to eq(nil) + expect(topic.assignment).to eq(nil) end 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?") TopicAssigner.auto_assign(post) - expect(topic.custom_fields["assigned_to_id"].to_i) - .to eq(moderator.id) + expect(topic.assignment.assigned_to_id).to eq(moderator.id) end end @@ -234,7 +232,8 @@ RSpec.describe TopicAssigner do it "automatically assigns to myself" do 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 it "does not automatically assign to myself" do @@ -273,7 +272,8 @@ RSpec.describe TopicAssigner do it "automatically assigns to other" do 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 diff --git a/spec/plugin_spec.rb b/spec/plugin_spec.rb index 11fb716..8466e97 100644 --- a/spec/plugin_spec.rb +++ b/spec/plugin_spec.rb @@ -20,9 +20,7 @@ describe 'plugin' do TopicAssigner.new(@topic, Discourse.system_user).assign(@user) @group_a.remove(@user) - custom_fields = @topic.reload.custom_fields - expect(custom_fields[TopicAssigner::ASSIGNED_TO_ID]).to be_nil - expect(custom_fields[TopicAssigner::ASSIGNED_BY_ID]).to be_nil + expect(Assignment.count).to eq(0) end 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) @group_a.remove(@user) - custom_fields = @topic.reload.custom_fields - expect(custom_fields[TopicAssigner::ASSIGNED_TO_ID]).to eq(@user.id.to_s) - expect(custom_fields[TopicAssigner::ASSIGNED_BY_ID]).to eq(Discourse::SYSTEM_USER_ID.to_s) + assignment = Assignment.first + expect(assignment.assigned_to_id).to eq(@user.id) + expect(assignment.assigned_by_user_id).to eq(Discourse::SYSTEM_USER_ID) end end end diff --git a/spec/requests/assign_controller_spec.rb b/spec/requests/assign_controller_spec.rb index 62bf77b..2672a91 100644 --- a/spec/requests/assign_controller_spec.rb +++ b/spec/requests/assign_controller_spec.rb @@ -4,18 +4,17 @@ require 'rails_helper' require_relative '../support/assign_allowed_group' RSpec.describe DiscourseAssign::AssignController do - before { SiteSetting.assign_enabled = true } let(:default_allowed_group) { Group.find_by(name: 'staff') } let(:user) { Fabricate(:admin, groups: [default_allowed_group], name: 'Robin Ward', username: 'eviltrout') } - let(:post) { Fabricate(:post) } - let(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david') } + fab!(:post) { Fabricate(:post) } + fab!(:user2) { Fabricate(:active_user, name: 'David Tylor', username: 'david') } let(:nonadmin) { Fabricate(:user, groups: [default_allowed_group]) } - let(:normal_user) { Fabricate(:user) } - let(:normal_admin) { Fabricate(:admin) } + fab!(:normal_user) { Fabricate(:user) } + 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) } 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) end - context '#suggestions' do + describe '#suggestions' do before { sign_in(user) } it 'includes users in allowed groups' do @@ -75,7 +74,7 @@ RSpec.describe DiscourseAssign::AssignController do end end - context "#suggestions" do + describe "#suggestions" do before do SiteSetting.max_assigned_topics = 1 sign_in(user) @@ -92,8 +91,7 @@ RSpec.describe DiscourseAssign::AssignController do end end - context '#assign' do - + describe '#assign' do include_context 'A group that is allowed to assign' before do @@ -107,7 +105,7 @@ RSpec.describe DiscourseAssign::AssignController do } 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 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(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: { topic_id: post.topic_id, username: user2.username @@ -171,7 +169,7 @@ RSpec.describe DiscourseAssign::AssignController do end end - context '#assigned' do + describe '#assigned' do include_context 'A group that is allowed to assign' fab!(:post1) { Fabricate(:post) } @@ -207,6 +205,7 @@ RSpec.describe DiscourseAssign::AssignController do context "with custom allowed groups" do let(:custom_allowed_group) { Fabricate(:group, name: 'mygroup') } let(:other_user) { Fabricate(:user, groups: [custom_allowed_group]) } + before do SiteSetting.assign_allowed_on_groups += "|#{custom_allowed_group.id}" end @@ -224,7 +223,7 @@ RSpec.describe DiscourseAssign::AssignController do end end - context '#group_members' do + describe '#group_members' do include_context 'A group that is allowed to assign' fab!(:post1) { Fabricate(:post) } @@ -287,4 +286,28 @@ RSpec.describe DiscourseAssign::AssignController do expect(response.status).to eq(200) 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