From 5a7a4fff11be16bb41b61b59340132c92718d9ac Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Thu, 4 Dec 2025 11:00:21 +0100 Subject: [PATCH] First draft of Collection update API (#37110) --- .../api/v1_alpha/collections_controller.rb | 26 ++++++- app/models/collection.rb | 10 ++- app/policies/collection_policy.rb | 29 ++++++++ app/services/create_collection_service.rb | 9 +-- config/routes/api.rb | 2 +- spec/models/collection_spec.rb | 54 +++++++++++++++ spec/policies/collection_policy_spec.rb | 43 ++++++++++++ .../requests/api/v1_alpha/collections_spec.rb | 69 +++++++++++++++++++ .../create_collection_service_spec.rb | 2 +- 9 files changed, 230 insertions(+), 14 deletions(-) create mode 100644 app/policies/collection_policy.rb create mode 100644 spec/policies/collection_policy_spec.rb diff --git a/app/controllers/api/v1_alpha/collections_controller.rb b/app/controllers/api/v1_alpha/collections_controller.rb index 7c33d3bfa2..b37b9c25c9 100644 --- a/app/controllers/api/v1_alpha/collections_controller.rb +++ b/app/controllers/api/v1_alpha/collections_controller.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Api::V1Alpha::CollectionsController < Api::BaseController + include Authorization + rescue_from ActiveRecord::RecordInvalid, Mastodon::ValidationError do |e| render json: { error: ValidationErrorFormatter.new(e).as_json }, status: 422 end @@ -11,23 +13,41 @@ class Api::V1Alpha::CollectionsController < Api::BaseController before_action :require_user!, only: [:create] + after_action :verify_authorized + def show cache_if_unauthenticated! @collection = Collection.find(params[:id]) + authorize @collection, :show? render json: @collection, serializer: REST::CollectionSerializer end def create - @collection = CreateCollectionService.new.call(collection_params, current_user.account) + authorize Collection, :create? + + @collection = CreateCollectionService.new.call(collection_creation_params, current_user.account) + + render json: @collection, serializer: REST::CollectionSerializer + end + + def update + @collection = Collection.find(params[:id]) + authorize @collection, :update? + + @collection.update!(collection_update_params) # TODO: Create a service for this to federate changes render json: @collection, serializer: REST::CollectionSerializer end private - def collection_params - params.permit(:name, :description, :sensitive, :discoverable, :tag, account_ids: []) + def collection_creation_params + params.permit(:name, :description, :sensitive, :discoverable, :tag_name, account_ids: []) + end + + def collection_update_params + params.permit(:name, :description, :sensitive, :discoverable, :tag_name) end def check_feature_enabled diff --git a/app/models/collection.rb b/app/models/collection.rb index 308e517e26..79af2967c1 100644 --- a/app/models/collection.rb +++ b/app/models/collection.rb @@ -50,12 +50,20 @@ class Collection < ApplicationRecord result end + def tag_name + tag&.formatted_name + end + + def tag_name=(new_name) + self.tag = Tag.find_or_create_by_names(new_name).first + end + private def tag_is_usable return if tag.blank? - errors.add(:tag, :unusable) unless tag.usable? + errors.add(:tag_name, :unusable) unless tag.usable? end def items_do_not_exceed_limit diff --git a/app/policies/collection_policy.rb b/app/policies/collection_policy.rb new file mode 100644 index 0000000000..12adfbcad1 --- /dev/null +++ b/app/policies/collection_policy.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +class CollectionPolicy < ApplicationPolicy + def index? + true + end + + def show? + true + end + + def create? + user_signed_in? + end + + def update? + owner? + end + + def destroy? + owner? + end + + private + + def owner? + current_account == record.account + end +end diff --git a/app/services/create_collection_service.rb b/app/services/create_collection_service.rb index 5cf7249fee..92c26879d1 100644 --- a/app/services/create_collection_service.rb +++ b/app/services/create_collection_service.rb @@ -2,9 +2,8 @@ class CreateCollectionService def call(params, account) - tag = params.delete(:tag) account_ids = params.delete(:account_ids) - @collection = Collection.new(params.merge({ account:, local: true, tag: find_or_create_tag(tag) })) + @collection = Collection.new(params.merge({ account:, local: true })) build_items(account_ids) @collection.save! @@ -13,12 +12,6 @@ class CreateCollectionService private - def find_or_create_tag(name) - return nil if name.blank? - - Tag.find_or_create_by_names(name).first - end - def build_items(account_ids) return if account_ids.blank? diff --git a/config/routes/api.rb b/config/routes/api.rb index 48e960af44..b7ffca3d82 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -8,7 +8,7 @@ namespace :api, format: false do namespace :v1_alpha do resources :async_refreshes, only: :show - resources :collections, only: [:show, :create] + resources :collections, only: [:show, :create, :update] end # JSON / REST API diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index 3d367b9d90..659e017869 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -72,4 +72,58 @@ RSpec.describe Collection do end end end + + describe '#tag_name=' do + context 'when the collection is new and has no tag' do + subject { Fabricate.build(:collection) } + + context 'when the tag exists' do + let!(:tag) { Fabricate.create(:tag, name: 'people') } + + it 'correctly assigns the existing tag' do + subject.tag_name = '#people' + + expect(subject.tag).to eq tag + end + end + + context 'when the tag does not exist' do + it 'creates and assigns a new tag' do + expect do + subject.tag_name = '#people' + end.to change(Tag, :count).by(1) + + expect(subject.tag).to be_present + expect(subject.tag.name).to eq 'people' + end + end + end + + context 'when the collection is persisted and has a tag' do + subject { Fabricate(:collection, tag:) } + + let!(:tag) { Fabricate(:tag, name: 'people') } + + context 'when the new tag is the same' do + it 'does not change the object' do + subject.tag_name = '#People' + + expect(subject.tag).to eq tag + expect(subject).to_not be_changed + end + end + + context 'when the new tag is different' do + it 'creates and assigns a new tag' do + expect do + subject.tag_name = '#bots' + end.to change(Tag, :count).by(1) + + expect(subject.tag).to be_present + expect(subject.tag.name).to eq 'bots' + expect(subject).to be_changed + end + end + end + end end diff --git a/spec/policies/collection_policy_spec.rb b/spec/policies/collection_policy_spec.rb new file mode 100644 index 0000000000..156e1a7657 --- /dev/null +++ b/spec/policies/collection_policy_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe CollectionPolicy do + let(:policy) { described_class } + let(:collection) { Fabricate(:collection) } + let(:owner) { collection.account } + let(:other_user) { Fabricate(:account) } + + permissions :index? do + it 'permits everyone to index' do + expect(policy).to permit(nil, Collection) + expect(policy).to permit(owner, Collection) + end + end + + permissions :show? do + it 'permits everyone to show' do + expect(policy).to permit(nil, collection) + expect(policy).to permit(owner, collection) + expect(policy).to permit(other_user, collection) + end + end + + permissions :create? do + it 'permits any user' do + expect(policy).to_not permit(nil, Collection) + + expect(policy).to permit(owner, Collection) + expect(policy).to permit(other_user, Collection) + end + end + + permissions :update?, :destroy? do + it 'only permits the owner' do + expect(policy).to_not permit(nil, collection) + expect(policy).to_not permit(other_user, collection) + + expect(policy).to permit(owner, collection) + end + end +end diff --git a/spec/requests/api/v1_alpha/collections_spec.rb b/spec/requests/api/v1_alpha/collections_spec.rb index d576fbf98b..134ff6b32b 100644 --- a/spec/requests/api/v1_alpha/collections_spec.rb +++ b/spec/requests/api/v1_alpha/collections_spec.rb @@ -95,4 +95,73 @@ RSpec.describe 'Api::V1Alpha::Collections', feature: :collections do end end end + + describe 'PATCH /api/v1_alpha/collections/:id' do + subject do + patch "/api/v1_alpha/collections/#{collection.id}", headers: headers, params: params + end + + let(:collection) { Fabricate(:collection) } + let(:params) { {} } + + it_behaves_like 'forbidden for wrong scope', 'read:collections' + + context 'when user is not owner' do + it 'returns http forbidden' do + subject + + expect(response).to have_http_status(403) + end + end + + context 'when user is the owner' do + let(:collection) do + Fabricate(:collection, + account: user.account, + name: 'Pople to follow', + description: 'Cool pople', + sensitive: true, + discoverable: false) + end + + context 'with valid params' do + let(:params) do + { + name: 'People to follow', + description: 'Cool people', + sensitive: '0', + discoverable: '1', + } + end + + it 'updates the collection and returns http success' do + subject + collection.reload + + expect(response).to have_http_status(200) + expect(collection.name).to eq 'People to follow' + expect(collection.description).to eq 'Cool people' + expect(collection.sensitive).to be false + expect(collection.discoverable).to be true + end + end + + context 'with invalid params' do + let(:params) { { name: '' } } + + it 'returns http unprocessable content and detailed errors' do + subject + + expect(response).to have_http_status(422) + expect(response.parsed_body).to include({ + 'error' => a_hash_including({ + 'details' => a_hash_including({ + 'name' => [{ 'error' => 'ERR_BLANK', 'description' => "can't be blank" }], + }), + }), + }) + end + end + end + end end diff --git a/spec/services/create_collection_service_spec.rb b/spec/services/create_collection_service_spec.rb index 43842c8cc6..bf59e299b1 100644 --- a/spec/services/create_collection_service_spec.rb +++ b/spec/services/create_collection_service_spec.rb @@ -45,7 +45,7 @@ RSpec.describe CreateCollectionService do end context 'when given a tag' do - let(:params) { base_params.merge(tag: '#people') } + let(:params) { base_params.merge(tag_name: '#people') } context 'when the tag exists' do let!(:tag) { Fabricate.create(:tag, name: 'people') }