Fix creation of duplicate conversations (#37108)

app/models/conversation.rb

@@ -17,7 +17,7 @@ class Conversation < ApplicationRecord
 
   has_many :statuses, dependent: nil
 
-  belongs_to :parent_status, class_name: 'Status', optional: true, inverse_of: :conversation
+  belongs_to :parent_status, class_name: 'Status', optional: true, inverse_of: :owned_conversation
   belongs_to :parent_account, class_name: 'Account', optional: true
 
   scope :local, -> { where(uri: nil) }

app/models/status.rb

@@ -154,6 +154,7 @@ class Status < ApplicationRecord
   around_create Mastodon::Snowflake::Callbacks
 
   after_create :set_poll_id
+  after_create :update_conversation
 
   # The `prepend: true` option below ensures this runs before
   # the `dependent: destroy` callbacks remove relevant records
@@ -448,11 +449,16 @@ class Status < ApplicationRecord
       self.in_reply_to_account_id = carried_over_reply_to_account_id
       self.conversation_id        = thread.conversation_id if conversation_id.nil?
     elsif conversation_id.nil?
-      conversation = build_owned_conversation
+      build_conversation
-      self.conversation = conversation
     end
   end
 
+  def update_conversation
+    return if reply?
+
+    conversation.update!(parent_status: self, parent_account: account) if conversation && conversation.parent_status.nil?
+  end
+
   def carried_over_reply_to_account_id
     if thread.account_id == account_id && thread.reply?
       thread.in_reply_to_account_id

spec/models/status_spec.rb

@@ -512,34 +512,6 @@ RSpec.describe Status do
     end
   end
 
-  describe 'before_validation' do
-    it 'sets account being replied to correctly over intermediary nodes' do
-      first_status = Fabricate(:status, account: bob)
-      intermediary = Fabricate(:status, thread: first_status, account: alice)
-      final        = Fabricate(:status, thread: intermediary, account: alice)
-
-      expect(final.in_reply_to_account_id).to eq bob.id
-    end
-
-    it 'creates new conversation for stand-alone status' do
-      expect(described_class.create(account: alice, text: 'First').conversation_id).to_not be_nil
-    end
-
-    it 'keeps conversation of parent node' do
-      parent = Fabricate(:status, text: 'First')
-      expect(described_class.create(account: alice, thread: parent, text: 'Response').conversation_id).to eq parent.conversation_id
-    end
-
-    it 'sets `local` to true for status by local account' do
-      expect(described_class.create(account: alice, text: 'foo').local).to be true
-    end
-
-    it 'sets `local` to false for status by remote account' do
-      alice.update(domain: 'example.com')
-      expect(described_class.create(account: alice, text: 'foo').local).to be false
-    end
-  end
-
   describe 'Validations' do
     context 'with a remote account' do
       subject { Fabricate.build :status, account: remote_account }
@@ -588,13 +560,49 @@ RSpec.describe Status do
         end
       end
     end
-  end
 
-  describe 'after_create' do
+    describe 'Wiring up replies and conversations' do
-    it 'saves ActivityPub uri as uri for local status' do
+      it 'sets account being replied to correctly over intermediary nodes' do
-      status = described_class.create(account: alice, text: 'foo')
+        first_status = Fabricate(:status, account: bob)
-      status.reload
+        intermediary = Fabricate(:status, thread: first_status, account: alice)
-      expect(status.uri).to start_with('https://')
+        final        = Fabricate(:status, thread: intermediary, account: alice)
+
+        expect(final.in_reply_to_account_id).to eq bob.id
+      end
+
+      it 'creates new conversation for stand-alone status' do
+        new_status = nil
+        expect do
+          new_status = described_class.create(account: alice, text: 'First')
+        end.to change(Conversation, :count).by(1)
+
+        expect(new_status.conversation_id).to_not be_nil
+        expect(new_status.conversation.parent_status_id).to eq new_status.id
+      end
+
+      it 'keeps conversation of parent node' do
+        parent = Fabricate(:status, text: 'First')
+        expect(described_class.create(account: alice, thread: parent, text: 'Response').conversation_id).to eq parent.conversation_id
+      end
+    end
+
+    describe 'Setting the `local` flag correctly' do
+      it 'sets `local` to true for status by local account' do
+        expect(described_class.create(account: alice, text: 'foo').local).to be true
+      end
+
+      it 'sets `local` to false for status by remote account' do
+        alice.update(domain: 'example.com')
+        expect(described_class.create(account: alice, text: 'foo').local).to be false
+      end
+    end
+
+    describe 'after_create' do
+      it 'saves ActivityPub uri as uri for local status' do
+        status = described_class.create(account: alice, text: 'foo')
+        status.reload
+        expect(status.uri).to start_with('https://')
+      end
     end
   end
 end