FIX: can now correctly unassign corrupt topics

if a topic is somehow assigned to an array we can correctly unassign

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_TO_ID] = assign_to.id
-    @topic.custom_fields["assigned_by_id"] = @assigned_by.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"]
+    if assigned_to_id = @topic.custom_fields[ASSIGNED_TO_ID]
-      @topic.custom_fields["assigned_to_id"] = nil
+
-      @topic.custom_fields["assigned_by_id"] = nil
+      # TODO core needs an API for this stuff badly
-      @topic.save_custom_fields
+      # 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?

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)

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])