From 533477e77cf5255125a565e8404d4249e8f8b86e Mon Sep 17 00:00:00 2001
From: Claire <claire.github-309c@sitedethib.com>
Date: Wed, 15 Jan 2025 11:59:28 +0100
Subject: [PATCH] Fix processing of mentions for post edits with an existing
corresponding silent mention (#33227)
---
.../process_status_update_service.rb | 20 +++++--------------
app/services/process_mentions_service.rb | 8 +++++---
app/workers/mention_resolve_worker.rb | 2 +-
.../process_status_update_service_spec.rb | 16 +++++++++++++--
spec/services/update_status_service_spec.rb | 8 ++++++++
5 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/app/services/activitypub/process_status_update_service.rb b/app/services/activitypub/process_status_update_service.rb
index 32f563013..e68abe972 100644
--- a/app/services/activitypub/process_status_update_service.rb
+++ b/app/services/activitypub/process_status_update_service.rb
@@ -188,40 +188,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?
-
- mention = previous_mentions.find { |x| x.account_id == account.id }
- mention ||= account.mentions.new(status: @status)
-
- current_mentions << mention
+ account&.id
rescue Mastodon::UnexpectedResponseError, HTTP::TimeoutError, HTTP::ConnectionError, OpenSSL::SSL::SSLError
# 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|
- mention.save if mention.new_record?
- end
+ @status.mentions.upsert_all(currently_mentioned_account_ids.map { |id| { account_id: id, silent: false } }, unique_by: %w(status_id account_id))
# 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
-
- Mention.where(id: removed_mentions.map(&:id)).update_all(silent: true) unless removed_mentions.empty?
+ @status.mentions.where.not(account_id: currently_mentioned_account_ids).update_all(silent: true)
# Queue unresolved mentions for later
unresolved_mentions.uniq.each do |uri|
diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb
index 1c4c7805f..06777bee9 100644
--- a/app/services/process_mentions_service.rb
+++ b/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)
diff --git a/app/workers/mention_resolve_worker.rb b/app/workers/mention_resolve_worker.rb
index 72dcd9633..8c5938aea 100644
--- a/app/workers/mention_resolve_worker.rb
+++ b/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
diff --git a/spec/services/activitypub/process_status_update_service_spec.rb b/spec/services/activitypub/process_status_update_service_spec.rb
index 498b3c8aa..ed6e8e316 100644
--- a/spec/services/activitypub/process_status_update_service_spec.rb
+++ b/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)
diff --git a/spec/services/update_status_service_spec.rb b/spec/services/update_status_service_spec.rb
index 7c92adeff..463605dd2 100644
--- a/spec/services/update_status_service_spec.rb
+++ b/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