First draft of Collection update API (#37110)

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
+  def collection_creation_params
-    params.permit(:name, :description, :sensitive, :discoverable, :tag, account_ids: [])
+    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

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

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

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?
 

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

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

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

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

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') }