Add collection reach finder (#39346)

app/lib/activitypub/activity/accept.rb

@@ -53,7 +53,7 @@ class ActivityPub::Activity::Accept < ActivityPub::Activity
     collection_item.update!(approval_uri:, state: :accepted)
 
     activity_json = ActiveModelSerializers::SerializableResource.new(collection_item, serializer: ActivityPub::AddFeaturedItemSerializer, adapter: ActivityPub::Adapter).to_json
-    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, collection_item.collection.account_id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(activity_json, collection_item.collection_id)
   end
 
   def accept_quote!(quote)

app/lib/collection_reach_finder.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+class CollectionReachFinder < AccountReachFinder
+  def initialize(collection)
+    @collection = collection
+    super(@collection.account)
+  end
+
+  def inboxes
+    (super + collection_member_inboxes).uniq
+  end
+
+  private
+
+  def collection_member_inboxes
+    @collection.accounts.inboxes
+  end
+end

app/models/collection.rb

@@ -32,6 +32,7 @@ class Collection < ApplicationRecord
   has_many :collection_items, dependent: :delete_all
   has_many :accepted_collection_items, -> { accepted }, class_name: 'CollectionItem', inverse_of: :collection # rubocop:disable Rails/HasManyOrHasOneDependent
   has_many :collection_reports, dependent: :delete_all
+  has_many :accounts, -> { merge(CollectionItem.pending_or_accepted) }, through: :collection_items
 
   validates :name, presence: true
   validates :name, length: { maximum: 40 }, if: :local?

app/services/add_account_to_collection_service.rb

@@ -30,7 +30,7 @@ class AddAccountToCollectionService
   end
 
   def distribute_add_activity
-    ActivityPub::AccountRawDistributionWorker.perform_async(add_activity_json, @collection.account_id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(add_activity_json, @collection.id)
   end
 
   def distribute_feature_request_activity

app/services/create_collection_service.rb

@@ -19,7 +19,7 @@ class CreateCollectionService
   private
 
   def distribute_add_activity
-    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, @account.id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(activity_json, @collection.id)
   end
 
   def distribute_feature_request_activities

app/services/delete_collection_item_service.rb

@@ -16,7 +16,7 @@ class DeleteCollectionItemService
   private
 
   def distribute_remove_activity
-    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, @collection.account.id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(activity_json, @collection.id)
   end
 
   def activity_json

app/services/delete_collection_service.rb

@@ -3,6 +3,7 @@
 class DeleteCollectionService
   def call(collection)
     @collection = collection
+    @account_ids = @collection.account_ids
     @collection.destroy!
 
     distribute_remove_activity
@@ -11,10 +12,13 @@ class DeleteCollectionService
   private
 
   def distribute_remove_activity
-    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, @collection.account.id)
+    @account_ids.each do |account_id|
+      ActivityPub::DeliveryWorker.perform_async(activity_json, account_id, @collection.account.inbox_url)
+    end
+    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, @collection.account_id)
   end
 
   def activity_json
-    ActiveModelSerializers::SerializableResource.new(@collection, serializer: ActivityPub::RemoveFeaturedCollectionSerializer, adapter: ActivityPub::Adapter).to_json
+    @activity_json ||= ActiveModelSerializers::SerializableResource.new(@collection, serializer: ActivityPub::RemoveFeaturedCollectionSerializer, adapter: ActivityPub::Adapter).to_json
   end
 end

app/services/revoke_collection_item_service.rb

@@ -17,7 +17,7 @@ class RevokeCollectionItemService < BaseService
 
   def distribute_stamp_deletion!
     ActivityPub::DeliveryWorker.perform_async(signed_activity_json, @account.id, @collection.account.inbox_url)
-    ActivityPub::AccountRawDistributionWorker.perform_async(signed_activity_json, @collection.account_id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(signed_activity_json, @collection.id)
   end
 
   def signed_activity_json

app/services/update_collection_service.rb

@@ -16,7 +16,7 @@ class UpdateCollectionService
   def distribute_update_activity
     return unless relevant_attributes_changed?
 
-    ActivityPub::AccountRawDistributionWorker.perform_async(activity_json, @collection.account.id)
+    ActivityPub::CollectionRawDistributionWorker.perform_async(activity_json, @collection.id)
   end
 
   def notify_about_update

app/workers/activitypub/collection_raw_distribution_worker.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class ActivityPub::CollectionRawDistributionWorker < ActivityPub::RawDistributionWorker
+  def perform(json, collection_id, exclude_inboxes = [])
+    @collection = Collection.find(collection_id)
+
+    super(json, @collection.account_id, exclude_inboxes)
+  end
+
+  private
+
+  def inboxes
+    @inboxes ||= CollectionReachFinder.new(@collection).inboxes
+  end
+end

spec/lib/activitypub/activity/accept_spec.rb

@@ -194,7 +194,7 @@ RSpec.describe ActivityPub::Activity::Accept do
 
           expect(collection_item.reload).to be_accepted
           expect(collection_item.approval_uri).to eq 'https://example.com/stamps/1'
-          expect(ActivityPub::AccountRawDistributionWorker)
+          expect(ActivityPub::CollectionRawDistributionWorker)
             .to have_enqueued_sidekiq_job
         end
       end
@@ -206,7 +206,7 @@ RSpec.describe ActivityPub::Activity::Accept do
 
             expect(collection_item.reload).to_not be_accepted
             expect(collection_item.approval_uri).to be_nil
-            expect(ActivityPub::AccountRawDistributionWorker)
+            expect(ActivityPub::CollectionRawDistributionWorker)
               .to_not have_enqueued_sidekiq_job
           end
         end

spec/lib/activitypub/activity/delete_spec.rb

@@ -139,7 +139,7 @@ RSpec.describe ActivityPub::Activity::Delete do
         subject.perform
 
         expect(collection_item.reload).to be_revoked
-        expect(ActivityPub::AccountRawDistributionWorker).to have_enqueued_sidekiq_job
+        expect(ActivityPub::CollectionRawDistributionWorker).to have_enqueued_sidekiq_job
       end
     end
   end

spec/lib/collection_reach_finder_spec.rb

@@ -0,0 +1,35 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe CollectionReachFinder do
+  let(:account) { Fabricate(:account) }
+  let(:collection) { Fabricate(:collection, account:) }
+
+  let(:follower_example_com) { Fabricate(:account, protocol: :activitypub, inbox_url: 'https://example.com/inbox-1', domain: 'example.com') }
+  let(:follower_with_shared) { Fabricate(:account, protocol: :activitypub, inbox_url: 'https://foo.bar/users/a/inbox', domain: 'foo.bar', shared_inbox_url: 'https://foo.bar/inbox') }
+
+  let(:collection_member_with_shared) { Fabricate(:account, protocol: :activitypub, inbox_url: 'https://foo.bar/users/b/inbox', domain: 'foo.bar', shared_inbox_url: 'https://foo.bar/inbox') }
+  let(:collection_member_example_org) { Fabricate(:account, protocol: :activitypub, inbox_url: 'https://example.org/inbox-2', domain: 'example.org') }
+
+  before do
+    follower_example_com.follow!(account)
+    follower_with_shared.follow!(account)
+
+    [follower_example_com, collection_member_with_shared, collection_member_example_org].each do |collection_member|
+      Fabricate(:collection_item, collection:, account: collection_member, activity_uri: "https://#{collection_member.domain}/activity", approval_uri: "https://#{collection_member.domain}/approval")
+    end
+  end
+
+  describe '#inboxes' do
+    subject { described_class.new(collection).inboxes }
+
+    it 'includes unique inbox URIs of followers and collection members respecting shared inbox URIs where present' do
+      expect(subject).to contain_exactly(
+        'https://example.com/inbox-1',
+        'https://foo.bar/inbox',
+        'https://example.org/inbox-2'
+      )
+    end
+  end
+end

spec/services/add_account_to_collection_service_spec.rb

@@ -25,9 +25,9 @@ RSpec.describe AddAccountToCollectionService do
         it 'federates an `Add` activity and schedules a notification' do
           subject.call(collection, account)
 
-          expect(ActivityPub::AccountRawDistributionWorker)
+          expect(ActivityPub::CollectionRawDistributionWorker)
             .to have_enqueued_sidekiq_job
-            .with(anything, collection.account_id)
+            .with(anything, collection.id)
           expect(LocalNotificationWorker)
             .to have_enqueued_sidekiq_job
             .with(account.id, anything, 'CollectionItem', 'added_to_collection')

spec/services/create_collection_service_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe CreateCollectionService do
       it 'federates an `Add` activity' do
         subject.call(base_params, author)
 
-        expect(ActivityPub::AccountRawDistributionWorker).to have_enqueued_sidekiq_job
+        expect(ActivityPub::CollectionRawDistributionWorker).to have_enqueued_sidekiq_job
       end
 
       context 'when given account ids' do

spec/services/delete_collection_item_service_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe DeleteCollectionItemService do
       it 'federates a `Remove` activity' do
         subject.call(collection_item)
 
-        expect(ActivityPub::AccountRawDistributionWorker).to have_enqueued_sidekiq_job
+        expect(ActivityPub::CollectionRawDistributionWorker).to have_enqueued_sidekiq_job
       end
 
       context 'when `revoke` is set to true' do
@@ -36,7 +36,7 @@ RSpec.describe DeleteCollectionItemService do
       it 'destroys the collection withouth federating anything' do
         expect { subject.call(collection_item, revoke: true) }.to change(collection.collection_items, :count).by(-1)
 
-        expect(ActivityPub::AccountRawDistributionWorker).to_not have_enqueued_sidekiq_job
+        expect(ActivityPub::CollectionRawDistributionWorker).to_not have_enqueued_sidekiq_job
       end
     end
   end

spec/services/delete_collection_service_spec.rb

@@ -7,15 +7,20 @@ RSpec.describe DeleteCollectionService do
 
   let!(:collection) { Fabricate(:collection) }
 
+  before do
+    Fabricate.times(2, :collection_item, collection:)
+  end
+
   describe '#call' do
     it 'destroys the collection' do
       expect { subject.call(collection) }.to change(Collection, :count).by(-1)
     end
 
-    it 'federates a `Remove` activity' do
+    it "federates a `Remove` activity to the account's reach plus each collection member" do
       subject.call(collection)
 
       expect(ActivityPub::AccountRawDistributionWorker).to have_enqueued_sidekiq_job
+      expect(ActivityPub::DeliveryWorker).to have_enqueued_sidekiq_job.exactly(2).times
     end
   end
 end

spec/services/revoke_collection_item_service_spec.rb

@@ -21,7 +21,7 @@ RSpec.describe RevokeCollectionItemService do
       subject.call(collection_item)
 
       expect(ActivityPub::DeliveryWorker).to have_enqueued_sidekiq_job.with(instance_of(String), collection_item.account_id, 'https://example.com/actor/1/inbox')
-      expect(ActivityPub::AccountRawDistributionWorker).to have_enqueued_sidekiq_job
+      expect(ActivityPub::CollectionRawDistributionWorker).to have_enqueued_sidekiq_job
     end
   end
 end

spec/services/update_collection_service_spec.rb

@@ -14,14 +14,14 @@ RSpec.describe UpdateCollectionService do
         expect { subject.call(collection, { name: 'Newly updated name' }) }
           .to change(collection, :name).to('Newly updated name')
           .and enqueue_sidekiq_job(LocalNotificationWorker).with(collection_item.account_id, collection.id, collection.class.name, 'collection_update')
-          .and enqueue_sidekiq_job(ActivityPub::AccountRawDistributionWorker)
+          .and enqueue_sidekiq_job(ActivityPub::CollectionRawDistributionWorker)
       end
 
       context 'when nothing changed' do
         it 'does not federate an activity' do
           subject.call(collection, { name: collection.name })
 
-          expect(ActivityPub::AccountRawDistributionWorker).to_not have_enqueued_sidekiq_job
+          expect(ActivityPub::CollectionRawDistributionWorker).to_not have_enqueued_sidekiq_job
           expect(LocalNotificationWorker).to_not have_enqueued_sidekiq_job
         end
       end