Fix processing of mentions for post edits with an existing corresponding silent mention (#33227)

app/services/activitypub/process_status_update_service.rb

@@ -190,40 +190,30 @@ class ActivityPub::ProcessStatusUpdateService < BaseService
   end
 
   def update_mentions!
-    previous_mentions = @status.active_mentions.includes(:account).to_a
-    current_mentions  = []
     unresolved_mentions = []
 
-    @raw_mentions.each do |href|
+    currently_mentioned_account_ids = @raw_mentions.filter_map do |href|
       next if href.blank?
 
       account   = ActivityPub::TagManager.instance.uri_to_resource(href, Account)
       account ||= ActivityPub::FetchRemoteAccountService.new.call(href, request_id: @request_id)
 
-      next if account.nil?
+      account&.id
-
-      mention   = previous_mentions.find { |x| x.account_id == account.id }
-      mention ||= account.mentions.new(status: @status)
-
-      current_mentions << mention
     rescue Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS
       # Since previous mentions are about already-known accounts,
       # they don't try to resolve again and won't fall into this case.
       # In other words, this failure case is only for new mentions and won't
       # affect `removed_mentions` so they can safely be retried asynchronously
       unresolved_mentions << href
+      nil
     end
 
-    current_mentions.each do |mention|
+    @status.mentions.upsert_all(currently_mentioned_account_ids.map { |id| { account_id: id, silent: false } }, unique_by: %w(status_id account_id))
-      mention.save if mention.new_record?
-    end
 
     # If previous mentions are no longer contained in the text, convert them
     # to silent mentions, since withdrawing access from someone who already
     # received a notification might be more confusing
-    removed_mentions = previous_mentions - current_mentions
+    @status.mentions.where.not(account_id: currently_mentioned_account_ids).update_all(silent: true)
-
-    Mention.where(id: removed_mentions.map(&:id)).update_all(silent: true) unless removed_mentions.empty?
 
     # Queue unresolved mentions for later
     unresolved_mentions.uniq.each do |uri|

app/services/process_mentions_service.rb

@@ -13,7 +13,7 @@ class ProcessMentionsService < BaseService
 
     return unless @status.local?
 
-    @previous_mentions = @status.active_mentions.includes(:account).to_a
+    @previous_mentions = @status.mentions.includes(:account).to_a
     @current_mentions  = []
 
     Status.transaction do
@@ -57,6 +57,8 @@ class ProcessMentionsService < BaseService
       mention ||= @current_mentions.find  { |x| x.account_id == mentioned_account.id }
       mention ||= @status.mentions.new(account: mentioned_account)
 
+      mention.silent = false
+
       @current_mentions << mention
 
       "@#{mentioned_account.acct}"
@@ -78,7 +80,7 @@ class ProcessMentionsService < BaseService
     end
 
     @current_mentions.each do |mention|
-      mention.save if mention.new_record? && @save_records
+      mention.save if (mention.new_record? || mention.silent_changed?) && @save_records
     end
 
     # If previous mentions are no longer contained in the text, convert them
@@ -86,7 +88,7 @@ class ProcessMentionsService < BaseService
     # received a notification might be more confusing
     removed_mentions = @previous_mentions - @current_mentions
 
-    Mention.where(id: removed_mentions.map(&:id)).update_all(silent: true) unless removed_mentions.empty?
+    Mention.where(id: removed_mentions.map(&:id), silent: false).update_all(silent: true) unless removed_mentions.empty?
   end
 
   def mention_undeliverable?(mentioned_account)

app/workers/mention_resolve_worker.rb

@@ -16,7 +16,7 @@ class MentionResolveWorker
 
     return if account.nil?
 
-    status.mentions.create!(account: account, silent: false)
+    status.mentions.upsert({ account_id: account.id, silent: false }, unique_by: %w(status_id account_id))
   rescue ActiveRecord::RecordNotFound
     # Do nothing
   rescue Mastodon::UnexpectedResponseError => e

spec/services/activitypub/process_status_update_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService do
   let(:media_attachments) { [] }
 
   before do
-    mentions.each { |a| Fabricate(:mention, status: status, account: a) }
+    mentions.each { |(account, silent)| Fabricate(:mention, status: status, account: account, silent: silent) }
     tags.each { |t| status.tags << t }
     media_attachments.each { |m| status.media_attachments << m }
     stub_request(:get, bogus_mention).to_raise(HTTP::ConnectionError)
@@ -280,7 +280,19 @@ RSpec.describe ActivityPub::ProcessStatusUpdateService do
     end
 
     context 'when originally with mentions' do
-      let(:mentions) { [alice, bob] }
+      let(:mentions) { [[alice, false], [bob, false]] }
+
+      before do
+        subject.call(status, json, json)
+      end
+
+      it 'updates mentions' do
+        expect(status.active_mentions.reload.map(&:account_id)).to eq [alice.id]
+      end
+    end
+
+    context 'when originally with silent mentions' do
+      let(:mentions) { [[alice, true], [bob, true]] }
 
       before do
         subject.call(status, json, json)

spec/services/update_status_service_spec.rb

@@ -150,6 +150,14 @@ RSpec.describe UpdateStatusService do
         .to eq [bob.id]
       expect(status.mentions.pluck(:account_id))
         .to contain_exactly(alice.id, bob.id)
+
+      # Going back when a mention was switched to silence should still be possible
+      subject.call(status, status.account_id, text: 'Hello @alice')
+
+      expect(status.active_mentions.pluck(:account_id))
+        .to eq [alice.id]
+      expect(status.mentions.pluck(:account_id))
+        .to contain_exactly(alice.id, bob.id)
     end
   end