From a058b4a0f2857d16dd732008e764f0d0ae81e9fe Mon Sep 17 00:00:00 2001 From: KMY Date: Mon, 20 Nov 2023 13:28:36 +0900 Subject: [PATCH] =?UTF-8?q?Revert=20"Add:=20=E3=83=AD=E3=83=BC=E3=82=AB?= =?UTF-8?q?=E3=83=AB=E6=8A=95=E7=A8=BF=E6=99=82=E3=81=AB=E3=83=A1=E3=83=B3?= =?UTF-8?q?=E3=82=B7=E3=83=A7=E3=83=B3=E8=BF=BD=E5=8A=A0=E3=83=BB=E4=BB=96?= =?UTF-8?q?=E3=82=B5=E3=83=BC=E3=83=90=E3=83=BC=E3=81=B8=E3=81=AE=E8=BB=A2?= =?UTF-8?q?=E9=80=81"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit d3182a74e220036ad54048d3748155491bd0e43c. --- app/lib/activitypub/activity/create.rb | 11 --- app/models/status.rb | 2 +- app/services/post_status_service.rb | 5 +- app/services/process_conversion_service.rb | 33 -------- .../activitypub/distribution_worker.rb | 15 ---- .../activitypub/forward_conversion_worker.rb | 37 --------- spec/services/post_status_service_spec.rb | 82 +------------------ .../activitypub/distribution_worker_spec.rb | 16 ---- 8 files changed, 4 insertions(+), 197 deletions(-) delete mode 100644 app/services/process_conversion_service.rb delete mode 100644 app/workers/activitypub/forward_conversion_worker.rb diff --git a/app/lib/activitypub/activity/create.rb b/app/lib/activitypub/activity/create.rb index cd9fee8b158a10..4925cbc19d08b6 100644 --- a/app/lib/activitypub/activity/create.rb +++ b/app/lib/activitypub/activity/create.rb @@ -96,7 +96,6 @@ def process_status process_references! distribute forward_for_reply - forward_for_conversation if @status.limited_visibility? join_group! end @@ -512,16 +511,6 @@ def forward_for_reply ActivityPub::RawDistributionWorker.perform_async(Oj.dump(@json), replied_to_status.account_id, [@account.preferred_inbox_url]) end - def forward_for_conversation - return unless @status.conversation.present? && @status.conversation.local? - - ProcessConversionService.new.call(@status) - - return if @json['signature'].blank? - - ActivityPub::ForwardConversationWorker.perform_async(Oj.dump(@json), @status.id) - end - def increment_voters_count! poll = replied_to_status.preloadable_poll diff --git a/app/models/status.rb b/app/models/status.rb index 631ffc4aaaf394..938f196948cdcd 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -657,7 +657,7 @@ def set_conversation self.reply = !(in_reply_to_id.nil? && thread.nil?) unless reply - if reply? && !thread.nil? && (!limited_visibility? || none_limited? || reply_limited?) + if reply? && !thread.nil? 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? diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 0ae6d0a9a75fdc..cba204eaf548e3 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -78,7 +78,7 @@ def preprocess_attributes! @visibility = :limited if %w(mutual circle reply).include?(@options[:visibility]) @visibility = :unlisted if (@visibility == :public || @visibility == :public_unlisted || @visibility == :login) && @account.silenced? @visibility = :public_unlisted if @visibility == :public && !@options[:force_visibility] && !@options[:application]&.superapp && @account.user&.setting_public_post_to_unlisted && Setting.enable_public_unlisted_visibility - @limited_scope = @options[:visibility]&.to_sym if @visibility == :limited && @options[:visibility] != 'limited' + @limited_scope = @options[:visibility]&.to_sym if @visibility == :limited @searchability = searchability @searchability = :private if @account.silenced? && %i(public public_unlisted).include?(@searchability&.to_sym) @markdown = @options[:markdown] || false @@ -87,7 +87,7 @@ def preprocess_attributes! @reference_ids = (@options[:status_reference_ids] || []).map(&:to_i).filter(&:positive?) raise ArgumentError if !Setting.enable_public_unlisted_visibility && @visibility == :public_unlisted - if @in_reply_to.present? && ((@options[:visibility] == 'limited' && @options[:circle_id].nil?) || @limited_scope == :reply) + if @in_reply_to.present? && ((@visibility == :limited && @options[:circle_id].nil?) || @limited_scope == :reply) @visibility = :limited @limited_scope = :reply end @@ -201,7 +201,6 @@ def postprocess_status! process_hashtags_service.call(@status) Trends.tags.register(@status) - ProcessConversionService.new.call(@status) if @status.limited_visibility? && @status.reply_limited? ProcessReferencesService.call_service(@status, @reference_ids, []) LinkCrawlWorker.perform_async(@status.id) DistributionWorker.perform_async(@status.id) diff --git a/app/services/process_conversion_service.rb b/app/services/process_conversion_service.rb deleted file mode 100644 index ccae2881936629..00000000000000 --- a/app/services/process_conversion_service.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -class ProcessConversionService < BaseService - def call(status) - @status = status - - return if !@status.limited_visibility? || @status.conversation.nil? - - duplicate_reply! - end - - private - - def thread - @thread ||= @status.thread || @status.conversation.ancestor_status - end - - def duplicate_reply! - return unless @status.conversation.local? - return if !@status.reply? || thread.nil? - return if thread.conversation_id != @status.conversation_id - - mentioned_account_ids = @status.mentions.pluck(:account_id) - - thread.mentioned_accounts.find_each do |account| - @status.mentions << @status.mentions.new(silent: true, account: account) unless mentioned_account_ids.include?(account.id) - mentioned_account_ids << account.id - end - @status.mentions << @status.mentions.new(silent: true, account: thread.account) unless mentioned_account_ids.include?(thread.account.id) - - @status.save! - end -end diff --git a/app/workers/activitypub/distribution_worker.rb b/app/workers/activitypub/distribution_worker.rb index 4ea25a58e14938..f8f80864a4afc4 100644 --- a/app/workers/activitypub/distribution_worker.rb +++ b/app/workers/activitypub/distribution_worker.rb @@ -19,26 +19,11 @@ def perform(status_id) protected def distribute_limited! - if @status.reply? && @status.conversation.present? && !@status.conversation.local? - distribute_conversation! - else - distribute_limited_mentions! - end - end - - def distribute_limited_mentions! ActivityPub::DeliveryWorker.push_bulk(inboxes_for_limited, limit: 1_000) do |inbox_url| [payload, @account.id, inbox_url, options] end end - def distribute_conversation! - inbox_url = @status.conversation.inbox_url - return if inbox_url.blank? - - ActivityPub::DeliveryWorker.perform_async(payload, @account.id, inbox_url, options) - end - def inboxes @inboxes ||= status_reach_finder.inboxes end diff --git a/app/workers/activitypub/forward_conversion_worker.rb b/app/workers/activitypub/forward_conversion_worker.rb deleted file mode 100644 index 3bc846b4df2196..00000000000000 --- a/app/workers/activitypub/forward_conversion_worker.rb +++ /dev/null @@ -1,37 +0,0 @@ -# frozen_string_literal: true - -class ActivityPub::ForwardConversationWorker - def perform(payload, status_id) - @status = Status.find(status_id) - @payload = payload - - return unless @status.conversation.present? && @status.conversation.local? && @status.conversation.ancestor_status.present? - return unless @status.limited_visibility? - - @account = @status.conversation.ancestor_status.account - - distribute_limited_mentions! - rescue ActiveRecord::RecordNotFound - true - end - - protected - - def distribute_limited_mentions! - ActivityPub::DeliveryWorker.push_bulk(inboxes_for_limited, limit: 1_000) do |inbox_url| - [payload, @account.id, inbox_url, options] - end - end - - def inboxes_for_limited - DeliveryFailureTracker.without_unavailable( - Account.remote.merge(@status.mentioned_accounts).pluck(:inbox_url).compact_blank.uniq - ) - end - - def options - { 'synchronize_followers' => @status.private_visibility? } - end - - attr_reader :payload -end diff --git a/spec/services/post_status_service_spec.rb b/spec/services/post_status_service_spec.rb index 455dac4e7e9c00..2956bd44c60cfc 100644 --- a/spec/services/post_status_service_spec.rb +++ b/spec/services/post_status_service_spec.rb @@ -314,7 +314,7 @@ account = Fabricate(:account) text = 'test status update' - status = subject.call(account, text: text, thread: in_reply_to_status, visibility: 'limited') + status = subject.call(account, text: text, thread: in_reply_to_status, visibility: :limited) expect(status).to be_persisted expect(status.thread).to eq in_reply_to_status @@ -365,86 +365,6 @@ expect(status.thread).to eq in_reply_to_status expect(status.visibility).to eq 'direct' end - - it 'duplicate replies' do - in_reply_to_status = Fabricate(:status, visibility: :limited) - in_reply_to_status.mentions.create!(account: Fabricate(:account)) - - status = subject.call(Fabricate(:user).account, text: 'Ohagi is good', thread: in_reply_to_status, visibility: 'reply') - - thread_account_ids = [in_reply_to_status.account, in_reply_to_status.mentions.first.account].map(&:id) - - expect(status).to be_persisted - expect(status.conversation_id).to eq in_reply_to_status.conversation_id - expect(status.conversation.ancestor_status_id).to eq in_reply_to_status.id - expect(status.mentions.pluck(:account_id)).to match_array thread_account_ids - end - - it 'duplicate reply-to-reply' do - ancestor_account = Fabricate(:account, username: 'ancestor', domain: nil) - reply_account = Fabricate(:account) - - first_status = Fabricate(:status, account: ancestor_account, visibility: :limited) - in_reply_to_status = subject.call(reply_account, text: 'Ohagi is good, @ancestor', thread: first_status, visibility: 'reply') - status = subject.call(ancestor_account, text: 'Ohagi is good', thread: in_reply_to_status, visibility: 'reply') - - thread_account_ids = [ancestor_account, reply_account].map(&:id) - - expect(status).to be_persisted - expect(status.conversation_id).to eq in_reply_to_status.conversation_id - expect(status.conversation_id).to eq first_status.conversation_id - expect(status.conversation.ancestor_status_id).to eq first_status.id - expect(status.mentions.pluck(:account_id)).to match_array thread_account_ids - end - - it 'duplicate reply-to-third_reply' do - first_status = Fabricate(:status, visibility: :limited) - first_status.mentions.create!(account: Fabricate(:account)) - - mentioned_account = Fabricate(:account, username: 'ohagi', domain: nil) - mentioned_account2 = Fabricate(:account, username: 'bob', domain: nil) - in_reply_to_status = subject.call(Fabricate(:user).account, text: 'Ohagi is good, @ohagi', thread: first_status, visibility: 'reply') - status = subject.call(Fabricate(:user).account, text: 'Ohagi is good, @bob', thread: in_reply_to_status, visibility: 'reply') - - thread_account_ids = [first_status.account, first_status.mentions.first.account, mentioned_account, mentioned_account2, in_reply_to_status.account].map(&:id) - - expect(status).to be_persisted - expect(status.conversation_id).to eq in_reply_to_status.conversation_id - expect(status.conversation_id).to eq first_status.conversation_id - expect(status.conversation.ancestor_status_id).to eq first_status.id - expect(status.mentions.pluck(:account_id)).to match_array thread_account_ids - end - - it 'do not duplicate replies when limited post' do - in_reply_to_status = Fabricate(:status, visibility: :limited) - in_reply_to_status.mentions.create!(account: Fabricate(:account)) - - status = subject.call(Fabricate(:user).account, text: 'Ohagi is good', thread: in_reply_to_status, visibility: 'mutual') - - [in_reply_to_status.account, in_reply_to_status.mentions.first.account].map(&:id) - - expect(status).to be_persisted - expect(status.limited_scope).to eq 'personal' - - mentions = status.mentions.pluck(:account_id) - expect(mentions).to_not include in_reply_to_status.account_id - expect(mentions).to_not include in_reply_to_status.mentions.first.account_id - end - - it 'do not duplicate replies when not limited post' do - in_reply_to_status = Fabricate(:status, visibility: :limited) - in_reply_to_status.mentions.create!(account: Fabricate(:account)) - - status = subject.call(Fabricate(:user).account, text: 'Ohagi is good', thread: in_reply_to_status, visibility: 'public') - - [in_reply_to_status.account, in_reply_to_status.mentions.first.account].map(&:id) - - expect(status).to be_persisted - - mentions = status.mentions.pluck(:account_id) - expect(mentions).to_not include in_reply_to_status.account_id - expect(mentions).to_not include in_reply_to_status.mentions.first.account_id - end end it 'safeguards mentions' do diff --git a/spec/workers/activitypub/distribution_worker_spec.rb b/spec/workers/activitypub/distribution_worker_spec.rb index 635eac5042b359..3b11207d8eb817 100644 --- a/spec/workers/activitypub/distribution_worker_spec.rb +++ b/spec/workers/activitypub/distribution_worker_spec.rb @@ -63,22 +63,6 @@ end end - context 'with limited response status' do - before do - allow(ActivityPub::DeliveryWorker).to receive(:perform_async).with(kind_of(String), status.account.id, 'http://example.com/conversation/inbox', anything) - status.update(visibility: :limited, thread: Fabricate(:status)) - status.conversation.update(uri: 'https://example.com/conversation', inbox_url: 'http://example.com/conversation/inbox') - status.capability_tokens.create! - status.mentions.create!(account: follower, silent: true) - stub_request(:post, 'http://example.com/conversation/inbox') - end - - it 'delivers to followers' do - subject.perform(status.id) - expect(ActivityPub::DeliveryWorker).to have_received(:perform_async) - end - end - context 'with direct status' do let(:mentioned_account) { Fabricate(:account, protocol: :activitypub, inbox_url: 'https://foo.bar/inbox', domain: 'foo.bar') }