From b846f88e16b6cd13706042fb041f7ca5d953e6b7 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Fri, 17 Apr 2026 15:28:39 +0200 Subject: [PATCH] Improve collection item behavior in REST API (#38732) --- app/models/collection.rb | 1 + app/models/collection_item.rb | 1 + .../rest/collection_item_serializer.rb | 8 +++--- app/serializers/rest/collection_serializer.rb | 6 ++++- spec/models/collection_spec.rb | 25 ++++++++++++++---- .../rest/collection_item_serializer_spec.rb | 24 ++++++++++++----- .../rest/collection_serializer_spec.rb | 26 +++++++++++++++++++ 7 files changed, 75 insertions(+), 16 deletions(-) diff --git a/app/models/collection.rb b/app/models/collection.rb index 5956b7c087..8d372eb8f4 100644 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -65,6 +65,7 @@ class Collection < ApplicationRecord def items_for(account = nil) result = collection_items.with_accounts + result = account == self.account ? result.pending_or_accepted : result.accepted result = result.not_blocked_by(account) unless account.nil? result end diff --git a/app/models/collection_item.rb b/app/models/collection_item.rb index c2ba2e9b68..189cc29e69 100644 --- a/app/models/collection_item.rb +++ b/app/models/collection_item.rb @@ -43,6 +43,7 @@ class CollectionItem < ApplicationRecord scope :not_blocked_by, ->(account) { where.not(accounts: { id: account.blocking }) } scope :local, -> { joins(:collection).merge(Collection.local) } scope :accepted_partial, ->(account) { joins(:account).merge(Account.local).accepted.where(uri: nil, account_id: account.id) } + scope :pending_or_accepted, -> { where(state: [:pending, :accepted]) } def revoke! update!(state: :revoked) diff --git a/app/serializers/rest/collection_item_serializer.rb b/app/serializers/rest/collection_item_serializer.rb index 49cd8c7a0e..4d75910a7a 100644 --- a/app/serializers/rest/collection_item_serializer.rb +++ b/app/serializers/rest/collection_item_serializer.rb @@ -1,11 +1,9 @@ # frozen_string_literal: true class REST::CollectionItemSerializer < ActiveModel::Serializer - delegate :accepted?, to: :object - attributes :id, :state, :created_at - attribute :account_id, if: :accepted? + attribute :account_id, if: :accepted_or_pending? def id object.id.to_s @@ -14,4 +12,8 @@ class REST::CollectionItemSerializer < ActiveModel::Serializer def account_id object.account_id.to_s end + + def accepted_or_pending? + object.pending? || object.accepted? + end end diff --git a/app/serializers/rest/collection_serializer.rb b/app/serializers/rest/collection_serializer.rb index c9696b9aef..c1c2c70d32 100644 --- a/app/serializers/rest/collection_serializer.rb +++ b/app/serializers/rest/collection_serializer.rb @@ -29,7 +29,11 @@ class REST::CollectionSerializer < ActiveModel::Serializer end def items - object.items_for(current_user&.account) + @items ||= object.items_for(current_user&.account) + end + + def item_count + items.size end def account_id diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index 64e93b6797..619359a19d 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -64,11 +64,18 @@ RSpec.describe Collection do describe '#item_for' do subject { Fabricate(:collection) } - let!(:items) { Fabricate.times(2, :collection_item, collection: subject) } + let!(:accepted_items) { Fabricate.times(2, :collection_item, collection: subject, state: :accepted) } + let!(:pending_item) { Fabricate(:collection_item, collection: subject, state: :pending) } + + before do + %i(rejected revoked).each do |state| + Fabricate(:collection_item, collection: subject, state:) + end + end context 'when given no account' do - it 'returns all items' do - expect(subject.items_for).to match_array(items) + it 'returns all accepted items' do + expect(subject.items_for).to match_array(accepted_items) end end @@ -76,11 +83,19 @@ RSpec.describe Collection do let(:account) { Fabricate(:account) } before do - account.block!(items.first.account) + account.block!(accepted_items.first.account) end it 'does not return items blocked by this account' do - expect(subject.items_for(account)).to contain_exactly(items.last) + expect(subject.items_for(account)).to contain_exactly(accepted_items.last) + end + end + + context 'when given the owner of the collection' do + let(:account) { subject.account } + + it 'returns accepted and pending items' do + expect(subject.items_for(account)).to match_array(accepted_items + [pending_item]) end end end diff --git a/spec/serializers/rest/collection_item_serializer_spec.rb b/spec/serializers/rest/collection_item_serializer_spec.rb index a16af9bce0..b57daf1157 100644 --- a/spec/serializers/rest/collection_item_serializer_spec.rb +++ b/spec/serializers/rest/collection_item_serializer_spec.rb @@ -11,23 +11,33 @@ RSpec.describe REST::CollectionItemSerializer do state:) end - context 'when state is `accepted`' do - let(:state) { :accepted } - + shared_examples 'full result' do it 'includes the relevant attributes including the account' do expect(subject) .to include( 'id' => '2342', 'account_id' => collection_item.account_id.to_s, - 'state' => 'accepted', + 'state' => state.to_s, 'created_at' => match_api_datetime_format ) end end - %i(pending rejected revoked).each do |unaccepted_state| - context "when state is `#{unaccepted_state}`" do - let(:state) { unaccepted_state } + context 'when state is `accepted`' do + let(:state) { :accepted } + + it_behaves_like 'full result' + end + + context 'when state is `pending`' do + let(:state) { :pending } + + it_behaves_like 'full result' + end + + %i(rejected revoked).each do |rejected_state| + context "when state is `#{rejected_state}`" do + let(:state) { rejected_state } it 'does not include an account' do expect(subject.keys).to_not include('account_id') diff --git a/spec/serializers/rest/collection_serializer_spec.rb b/spec/serializers/rest/collection_serializer_spec.rb index dbb9803753..8055c2afbe 100644 --- a/spec/serializers/rest/collection_serializer_spec.rb +++ b/spec/serializers/rest/collection_serializer_spec.rb @@ -67,4 +67,30 @@ RSpec.describe REST::CollectionSerializer do end end end + + context 'when the collection has items in different states' do + before do + %i(accepted pending rejected revoked).each do |state| + Fabricate(:collection_item, collection:, state:) + end + end + + context 'when `current_user` is the owner of the collection' do + let(:current_user) { collection.account.user } + + it 'includes accepted and pending items' do + expect(subject['item_count']).to eq 2 + expect(subject['items'].size).to eq 2 + expect(subject['items'].pluck('state')).to contain_exactly('accepted', 'pending') + end + end + + context 'when `current_user` is not the owner of the collection' do + it 'only includes the accepted item' do + expect(subject['item_count']).to eq 1 + expect(subject['items'].size).to eq 1 + expect(subject['items'].first['state']).to eq 'accepted' + end + end + end end