Allow service to update existing Collections (#38329)

app/services/activitypub/process_featured_collection_service.rb

@@ -14,21 +14,22 @@ class ActivityPub::ProcessFeaturedCollectionService
     return if non_matching_uri_hosts?(@account.uri, @json['id'])
 
     with_redis_lock("collection:#{@json['id']}") do
-      return if @account.collections.exists?(uri: @json['id'])
+      Collection.transaction do
+        @collection = @account.collections.find_or_initialize_by(uri: @json['id'])
 
-      @collection = @account.collections.create!(
+        @collection.update!(
-        local: false,
+          local: false,
-        uri: @json['id'],
+          name: (@json['name'] || '')[0, Collection::NAME_LENGTH_HARD_LIMIT],
-        name: (@json['name'] || '')[0, Collection::NAME_LENGTH_HARD_LIMIT],
+          description_html: truncated_summary,
-        description_html: truncated_summary,
+          language:,
-        language:,
+          sensitive: @json['sensitive'],
-        sensitive: @json['sensitive'],
+          discoverable: @json['discoverable'],
-        discoverable: @json['discoverable'],
+          original_number_of_items: @json['totalItems'] || 0,
-        original_number_of_items: @json['totalItems'] || 0,
+          tag_name: @json.dig('topic', 'name')
-        tag_name: @json.dig('topic', 'name')
+        )
-      )
 
-      process_items! if @json['totalItems'].positive?
+        process_items!
+      end
 
       @collection
     end
@@ -46,8 +47,13 @@ class ActivityPub::ProcessFeaturedCollectionService
   end
 
   def process_items!
-    @json['orderedItems'].take(ITEMS_LIMIT).each do |item_json|
+    uris = []
-      ActivityPub::ProcessFeaturedItemWorker.perform_async(@collection.id, item_json, @request_id)
+    items = @json['orderedItems'] || []
+    items.take(ITEMS_LIMIT).each_with_index do |item_json, index|
+      uris << value_or_id(item_json)
+      ActivityPub::ProcessFeaturedItemWorker.perform_async(@collection.id, item_json, index, @request_id)
     end
+    uris.compact!
+    @collection.collection_items.where.not(uri: uris).delete_all
   end
 end

app/services/activitypub/process_featured_item_service.rb

@@ -5,29 +5,24 @@ class ActivityPub::ProcessFeaturedItemService
   include Lockable
   include Redisable
 
-  def call(collection, uri_or_object, request_id: nil)
+  def call(collection, uri_or_object, position: nil, request_id: nil)
+    @collection = collection
     @request_id = request_id
-    item_json = uri_or_object.is_a?(String) ? fetch_resource(uri_or_object, true) : uri_or_object
+    @item_json = uri_or_object.is_a?(String) ? fetch_resource(uri_or_object, true) : uri_or_object
-    return if non_matching_uri_hosts?(collection.uri, item_json['id'])
+    return if non_matching_uri_hosts?(@collection.uri, @item_json['id'])
 
-    with_redis_lock("collection_item:#{item_json['id']}") do
+    with_redis_lock("collection_item:#{@item_json['id']}") do
-      return if collection.collection_items.exists?(uri: item_json['id'])
+      @collection_item = existing_item || pre_approved_item || new_item
 
-      local_account = ActivityPub::TagManager.instance.uris_to_local_accounts([item_json['featuredObject']]).first
+      @collection_item.update!(
+        uri: @item_json['id'],
+        object_uri: value_or_id(@item_json['featuredObject']),
+        position:
+      )
 
-      if local_account.present?
+      @approval_uri = @item_json['featureAuthorization']
-        # This is a local account that has authorized this item already
-        @collection_item = collection.collection_items.accepted_partial(local_account).first
-        @collection_item&.update!(uri: item_json['id'])
-      else
-        @collection_item = collection.collection_items.create!(
-          uri: item_json['id'],
-          object_uri: item_json['featuredObject']
-        )
-        @approval_uri = item_json['featureAuthorization']
 
-        verify_authorization!
+      verify_authorization! unless @collection_item&.account&.local?
-      end
 
       @collection_item
     end
@@ -35,6 +30,20 @@ class ActivityPub::ProcessFeaturedItemService
 
   private
 
+  def existing_item
+    @collection.collection_items.find_by(uri: @item_json['id'])
+  end
+
+  def pre_approved_item
+    # This is a local account that has authorized this item already
+    local_account = ActivityPub::TagManager.instance.uris_to_local_accounts([@item_json['featuredObject']]).first
+    @collection.collection_items.accepted_partial(local_account).first if local_account.present?
+  end
+
+  def new_item
+    @collection.collection_items.new
+  end
+
   def verify_authorization!
     ActivityPub::VerifyFeaturedItemService.new.call(@collection_item, @approval_uri, request_id: @request_id)
   rescue Mastodon::RecursionLimitExceededError, Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS

app/workers/activitypub/process_featured_item_worker.rb

@@ -6,10 +6,10 @@ class ActivityPub::ProcessFeaturedItemWorker
 
   sidekiq_options queue: 'pull', retry: 3
 
-  def perform(collection_id, id_or_json, request_id = nil)
+  def perform(collection_id, id_or_json, position = nil, request_id = nil)
     collection = Collection.find(collection_id)
 
-    ActivityPub::ProcessFeaturedItemService.new.call(collection, id_or_json, request_id:)
+    ActivityPub::ProcessFeaturedItemService.new.call(collection, id_or_json, position:, request_id:)
   rescue ActiveRecord::RecordNotFound
     true
   end

spec/services/activitypub/process_featured_collection_service_spec.rb

@@ -73,4 +73,36 @@ RSpec.describe ActivityPub::ProcessFeaturedCollectionService do
       expect(new_collection.description_html).to eq '<p>A list of remote actors you should follow.</p>'
     end
   end
+
+  context 'when the collection already exists' do
+    let(:collection) { Fabricate(:remote_collection, account:, uri: base_json['id'], name: 'placeholder') }
+
+    before do
+      Fabricate(:collection_item, collection:, uri: 'https://example.com/featured_items/1')
+      Fabricate(:collection_item, collection:, uri: 'https://example.com/featured_items/3')
+    end
+
+    it 'updates the existing collection, removes the item that no longer exists and queues a jobs to fetch the other items' do
+      expect { subject.call(account, featured_collection_json) }
+        .to change(collection.collection_items, :count).by(-1)
+
+      expect(collection.reload.name).to eq 'Good people from other servers'
+      expect(ActivityPub::ProcessFeaturedItemWorker).to have_enqueued_sidekiq_job.exactly(2).times
+    end
+
+    context 'when the updated collection no longer contains any items' do
+      let(:featured_collection_json) do
+        base_json.merge({
+          'summary' => summary,
+          'totalItems' => 0,
+          'orderedItems' => nil,
+        })
+      end
+
+      it 'removes all items' do
+        expect { subject.call(account, featured_collection_json) }
+          .to change(collection.collection_items, :count).by(-2)
+      end
+    end
+  end
 end

spec/services/activitypub/process_featured_item_service_spec.rb

@@ -8,6 +8,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
   subject { described_class.new }
 
   let(:collection) { Fabricate(:remote_collection, uri: 'https://other.example.com/collection/1') }
+  let(:position) { 3 }
   let(:featured_object_uri) { 'https://example.com/actor/1' }
   let(:feature_authorization_uri) { 'https://example.com/auth/1' }
   let(:featured_item_json) do
@@ -34,7 +35,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
 
       it 'does not create a collection item and returns `nil`' do
         expect do
-          expect(subject.call(collection, object)).to be_nil
+          expect(subject.call(collection, object, position:)).to be_nil
         end.to_not change(CollectionItem, :count)
       end
     end
@@ -45,14 +46,29 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
 
     it_behaves_like 'non-matching URIs'
 
-    it 'creates and verifies the item' do
+    context 'when item does not yet exist' do
-      expect { subject.call(collection, object) }.to change(collection.collection_items, :count).by(1)
+      it 'creates and verifies the item' do
+        expect { subject.call(collection, object, position:) }.to change(collection.collection_items, :count).by(1)
 
-      expect(stubbed_service).to have_received(:call)
+        expect(stubbed_service).to have_received(:call)
 
-      new_item = collection.collection_items.last
+        new_item = collection.collection_items.last
-      expect(new_item.object_uri).to eq 'https://example.com/actor/1'
+        expect(new_item.object_uri).to eq 'https://example.com/actor/1'
-      expect(new_item.approval_uri).to be_nil
+        expect(new_item.approval_uri).to be_nil
+        expect(new_item.position).to eq 3
+      end
+    end
+
+    context 'when item exists at a different position' do
+      let!(:collection_item) do
+        Fabricate(:collection_item, collection:, uri: featured_item_json['id'], position: 2)
+      end
+
+      it 'updates the position' do
+        expect { subject.call(collection, object, position:) }.to_not change(collection.collection_items, :count)
+
+        expect(collection_item.reload.position).to eq 3
+      end
     end
 
     context 'when an item exists for a local featured account' do
@@ -63,7 +79,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
       let(:feature_authorization_uri) { ap_account_feature_authorization_url(collection_item.account_id, collection_item) }
 
       it 'updates the URI of the existing record' do
-        expect { subject.call(collection, object) }.to_not change(collection.collection_items, :count)
+        expect { subject.call(collection, object, position:) }.to_not change(collection.collection_items, :count)
         expect(collection_item.reload.uri).to eq 'https://other.example.com/featured_item/1'
       end
     end
@@ -87,7 +103,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
     it_behaves_like 'non-matching URIs'
 
     it 'fetches the collection item' do
-      expect { subject.call(collection, object) }.to change(collection.collection_items, :count).by(1)
+      expect { subject.call(collection, object, position:) }.to change(collection.collection_items, :count).by(1)
 
       expect(featured_item_request).to have_been_requested
 

spec/workers/activitypub/process_featured_item_worker_spec.rb

@@ -19,7 +19,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemWorker do
     it 'calls the service to process the item' do
       subject.perform(collection.id, object)
 
-      expect(stubbed_service).to have_received(:call).with(collection, object, request_id: nil)
+      expect(stubbed_service).to have_received(:call).with(collection, object, position: nil, request_id: nil)
     end
   end
 end