From 8ad7304bdda28f2c962cd01645ab7ded84c2bd94 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 4 Sep 2018 15:10:17 +1000 Subject: [PATCH] FIX: can now correctly unassign corrupt topics if a topic is somehow assigned to an array we can correctly unassign --- lib/topic_assigner.rb | 44 +++++++++++++++++++++++++-------- plugin.rb | 18 +++++++------- spec/integration/assign_spec.rb | 15 ++++++++++- 3 files changed, 57 insertions(+), 20 deletions(-) diff --git a/lib/topic_assigner.rb b/lib/topic_assigner.rb index cf4919a..20c43b6 100644 --- a/lib/topic_assigner.rb +++ b/lib/topic_assigner.rb @@ -1,14 +1,19 @@ +# frozen_string_literal: true + require_dependency 'email/sender' class ::TopicAssigner + ASSIGNED_TO_ID = 'assigned_to_id' + ASSIGNED_BY_ID = 'assigned_by_id' + def self.unassign_all(user, assigned_by) - topic_ids = TopicCustomField.where(name: 'assigned_to_id', value: user.id).pluck(:topic_id) + topic_ids = TopicCustomField.where(name: ASSIGNED_TO_ID, value: user.id).pluck(:topic_id) # Fast path: by doing this we can instantly refresh for the user showing no assigned topics # while doing the "full" removal asynchronously. TopicCustomField.where( - name: ['assigned_to_id', 'assigned_by_id'], + name: [ASSIGNED_TO_ID, ASSIGNED_BY_ID], topic_id: topic_ids ).delete_all @@ -30,7 +35,7 @@ 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 topic_custom_fields tc ON tc.name = '#{ASSIGNED_TO_ID}' AND tc.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 NOT t.closed AND t.deleted_at IS NULL GROUP BY p.topic_id @@ -68,7 +73,7 @@ SQL return unless SiteSetting.assigns_by_staff_mention if post.user && post.topic && post.user.staff? - can_assign = force || post.topic.custom_fields["assigned_to_id"].nil? + can_assign = force || post.topic.custom_fields[ASSIGNED_TO_ID].nil? assign_other = assign_other_passes?(post) && mentioned_staff(post) assign_self = assign_self_passes?(post) && post.user @@ -125,8 +130,8 @@ SQL end def assign(assign_to, silent: false) - @topic.custom_fields["assigned_to_id"] = assign_to.id - @topic.custom_fields["assigned_by_id"] = @assigned_by.id + @topic.custom_fields[ASSIGNED_TO_ID] = assign_to.id + @topic.custom_fields[ASSIGNED_BY_ID] = @assigned_by.id @topic.save_custom_fields first_post = @topic.posts.find_by(post_number: 1) @@ -198,10 +203,29 @@ SQL end def unassign(silent: false) - if assigned_to_id = @topic.custom_fields["assigned_to_id"] - @topic.custom_fields["assigned_to_id"] = nil - @topic.custom_fields["assigned_by_id"] = nil - @topic.save_custom_fields + 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[ASSIGNED_TO_ID] = nil + @topic.custom_fields[ASSIGNED_BY_ID] = nil + + if !assigned_to_id + # nothing to do here + return + end post = @topic.posts.where(post_number: 1).first return unless post.present? diff --git a/plugin.rb b/plugin.rb index 65db180..2fc49f5 100644 --- a/plugin.rb +++ b/plugin.rb @@ -27,7 +27,7 @@ after_initialize do if SiteSetting.assign_locks_flags? if custom_fields = args[:post].topic.custom_fields - if assigned_to_id = custom_fields['assigned_to_id'] + if assigned_to_id = custom_fields[TopicAssigner::ASSIGNED_TO_ID] unless assigned_to_id.to_i == args[:user].id raise Discourse::InvalidAccess.new( "That flag has been assigned to another user", @@ -47,7 +47,7 @@ after_initialize do end end - TopicList.preloaded_custom_fields << "assigned_to_id" + TopicList.preloaded_custom_fields << TopicAssigner::ASSIGNED_TO_ID TopicList.on_preload do |topics, topic_list| if SiteSetting.assign_enabled? @@ -67,7 +67,7 @@ after_initialize do users.each { |u| map[u.id] = u } topics.each do |t| - if id = t.custom_fields['assigned_to_id'] + if id = t.custom_fields[TopicAssigner::ASSIGNED_TO_ID] t.preload_assigned_to_user(map[id.to_i]) end end @@ -143,7 +143,7 @@ after_initialize do add_to_class(:topic, :assigned_to_user) do @assigned_to_user || - if user_id = custom_fields["assigned_to_id"] + if user_id = custom_fields[TopicAssigner::ASSIGNED_TO_ID] @assigned_to_user = User.find_by(id: user_id) end end @@ -176,7 +176,7 @@ after_initialize do end add_to_class(:topic_view_serializer, :assigned_to_user_id) do - id = object.topic.custom_fields["assigned_to_id"] + 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 end @@ -184,7 +184,7 @@ after_initialize do add_to_serializer(:topic_view, 'include_assigned_to_user?') do if SiteSetting.assigns_public || scope.is_staff? # subtle but need to catch cases where stuff is not assigned - object.topic.custom_fields.keys.include?("assigned_to_id") + object.topic.custom_fields.keys.include?(TopicAssigner::ASSIGNED_TO_ID) end end @@ -193,7 +193,7 @@ after_initialize do end add_to_serializer(:flagged_topic, :assigned_to_user_id) do - id = object.custom_fields["assigned_to_id"] + 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 end @@ -238,7 +238,7 @@ after_initialize do on(:move_to_inbox) do |info| topic = info[:topic] - if (assigned_id = topic.custom_fields["assigned_to_id"].to_i) == info[:user]&.id + if (assigned_id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i) == info[:user]&.id TopicTrackingState.publish_assigned_private_message(topic, assigned_id) end @@ -253,7 +253,7 @@ after_initialize do on(:archive_message) do |info| topic = info[:topic] - user_id = topic.custom_fields["assigned_to_id"].to_i + user_id = topic.custom_fields[TopicAssigner::ASSIGNED_TO_ID].to_i if user_id == info[:user]&.id TopicTrackingState.publish_assigned_private_message(topic, user_id) diff --git a/spec/integration/assign_spec.rb b/spec/integration/assign_spec.rb index afe0640..dc5c900 100644 --- a/spec/integration/assign_spec.rb +++ b/spec/integration/assign_spec.rb @@ -13,6 +13,19 @@ 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 } @@ -25,7 +38,7 @@ describe 'integration tests' do yield end - message = messages.find { |message| message.channel == channel } + message = messages.find { |m| m.channel == channel } expect(message.data[:topic_id]).to eq(topic.id) expect(message.user_ids).to eq([user.id])