Fix adding items without a position (#38368)

app/models/collection_item.rb

@@ -62,7 +62,7 @@ class CollectionItem < ApplicationRecord
   private
 
   def set_position
-    return if position_changed?
+    return if position.present? && position_changed?
 
     self.position = self.class.where(collection_id:).maximum(:position).to_i + 1
   end

spec/models/collection_item_spec.rb

@@ -62,6 +62,12 @@ RSpec.describe CollectionItem do
       expect(custom_item.position).to eq 7
     end
 
+    it 'automatically sets the position if excplicitly set to `nil`' do
+      item = collection.collection_items.create!(account:, position: nil)
+
+      expect(item.position).to eq 1
+    end
+
     it 'automatically sets `activity_uri` when account is remote' do
       item = collection.collection_items.create(account: Fabricate(:remote_account))
 

spec/services/activitypub/process_featured_item_service_spec.rb

@@ -47,15 +47,26 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
     it_behaves_like 'non-matching URIs'
 
     context 'when item does not yet exist' do
-      it 'creates and verifies the item' do
+      context 'when a position is given' do
-        expect { subject.call(collection, object, position:) }.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
+          expect(new_item.position).to eq 3
+        end
+      end
+
+      context 'when no position is given' do
+        it 'creates the item' do
+          expect { subject.call(collection, object) }.to change(collection.collection_items, :count).by(1)
+          new_item = collection.collection_items.last
+
+          expect(new_item.position).to eq 1
+        end
       end
     end