Add REST API for featuring and unfeaturing a hashtag (#34489)

Co-authored-by: Matt Jankowski <matt@jankowski.online>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>

app/controllers/api/v1/featured_tags_controller.rb

@@ -18,7 +18,7 @@ class Api::V1::FeaturedTagsController < Api::BaseController
   end
 
   def destroy
-    RemoveFeaturedTagWorker.perform_async(current_account.id, @featured_tag.id)
+    RemoveFeaturedTagService.new.call(current_account, @featured_tag)
     render_empty
   end
 

app/controllers/api/v1/tags_controller.rb

@@ -1,7 +1,8 @@
 # frozen_string_literal: true
 
 class Api::V1::TagsController < Api::BaseController
-  before_action -> { doorkeeper_authorize! :follow, :write, :'write:follows' }, except: :show
+  before_action -> { doorkeeper_authorize! :follow, :write, :'write:follows' }, only: [:follow, :unfollow]
+  before_action -> { doorkeeper_authorize! :write, :'write:accounts' }, only: [:feature, :unfeature]
   before_action :require_user!, except: :show
   before_action :set_or_create_tag
 
@@ -23,6 +24,16 @@ class Api::V1::TagsController < Api::BaseController
     render json: @tag, serializer: REST::TagSerializer
   end
 
+  def feature
+    CreateFeaturedTagService.new.call(current_account, @tag)
+    render json: @tag, serializer: REST::TagSerializer
+  end
+
+  def unfeature
+    RemoveFeaturedTagService.new.call(current_account, @tag)
+    render json: @tag, serializer: REST::TagSerializer
+  end
+
   private
 
   def set_or_create_tag

app/controllers/settings/featured_tags_controller.rb

@@ -12,7 +12,7 @@ class Settings::FeaturedTagsController < Settings::BaseController
   end
 
   def create
-    @featured_tag = CreateFeaturedTagService.new.call(current_account, featured_tag_params[:name], force: false)
+    @featured_tag = CreateFeaturedTagService.new.call(current_account, featured_tag_params[:name], raise_error: false)
 
     if @featured_tag.valid?
       redirect_to settings_featured_tags_path

app/models/featured_tag.rb

@@ -18,17 +18,17 @@ class FeaturedTag < ApplicationRecord
   belongs_to :account, inverse_of: :featured_tags
   belongs_to :tag, inverse_of: :featured_tags, optional: true # Set after validation
 
-  validates :name, presence: true, format: { with: Tag::HASHTAG_NAME_RE }, on: :create
+  validates :name, presence: true, on: :create, if: -> { tag_id.nil? }
+  validates :name, format: { with: Tag::HASHTAG_NAME_RE }, on: :create, allow_blank: true
+  validates :tag_id, uniqueness: { scope: :account_id }
 
-  validate :validate_tag_uniqueness, on: :create
   validate :validate_featured_tags_limit, on: :create
 
   normalizes :name, with: ->(name) { name.strip.delete_prefix('#') }
 
-  before_create :set_tag
+  before_validation :set_tag
-  before_create :reset_data
 
-  scope :by_name, ->(name) { joins(:tag).where(tag: { name: HashtagNormalizer.new.normalize(name) }) }
+  before_create :reset_data
 
   LIMIT = 10
 
@@ -59,7 +59,11 @@ class FeaturedTag < ApplicationRecord
   private
 
   def set_tag
+    if tag.nil?
       self.tag = Tag.find_or_create_by_names(name)&.first
+    elsif tag&.new_record?
+      tag.save
+    end
   end
 
   def reset_data
@@ -73,14 +77,6 @@ class FeaturedTag < ApplicationRecord
     errors.add(:base, I18n.t('featured_tags.errors.limit')) if account.featured_tags.count >= LIMIT
   end
 
-  def validate_tag_uniqueness
-    errors.add(:name, :taken) if tag_already_featured_for_account?
-  end
-
-  def tag_already_featured_for_account?
-    FeaturedTag.by_name(name).exists?(account_id: account_id)
-  end
-
   def visible_tagged_account_statuses
     account.statuses.distributable_visibility.tagged_with(tag)
   end

app/presenters/tag_relationships_presenter.rb

@@ -1,13 +1,15 @@
 # frozen_string_literal: true
 
 class TagRelationshipsPresenter
-  attr_reader :following_map
+  attr_reader :following_map, :featuring_map
 
   def initialize(tags, current_account_id = nil, **options)
-    @following_map = if current_account_id.nil?
+    if current_account_id.nil?
-                       {}
+      @following_map = {}
+      @featuring_map = {}
     else
-                       TagFollow.select(:tag_id).where(tag_id: tags.map(&:id), account_id: current_account_id).each_with_object({}) { |f, h| h[f.tag_id] = true }.merge(options[:following_map] || {})
+      @following_map = TagFollow.select(:tag_id).where(tag_id: tags.map(&:id), account_id: current_account_id).each_with_object({}) { |f, h| h[f.tag_id] = true }.merge(options[:following_map] || {})
+      @featuring_map = FeaturedTag.select(:tag_id).where(tag_id: tags.map(&:id), account_id: current_account_id).each_with_object({}) { |f, h| h[f.tag_id] = true }.merge(options[:featuring_map] || {})
     end
   end
 end

app/serializers/rest/tag_serializer.rb

@@ -6,6 +6,7 @@ class REST::TagSerializer < ActiveModel::Serializer
   attributes :id, :name, :url, :history
 
   attribute :following, if: :current_user?
+  attribute :featuring, if: :current_user?
 
   def id
     object.id.to_s
@@ -27,6 +28,14 @@ class REST::TagSerializer < ActiveModel::Serializer
     end
   end
 
+  def featuring
+    if instance_options && instance_options[:relationships]
+      instance_options[:relationships].featuring_map[object.id] || false
+    else
+      FeaturedTag.exists?(tag_id: object.id, account_id: current_user.account_id)
+    end
+  end
+
   def current_user?
     !current_user.nil?
   end

app/services/create_featured_tag_service.rb

@@ -3,20 +3,26 @@
 class CreateFeaturedTagService < BaseService
   include Payloadable
 
-  def call(account, name, force: true)
+  def call(account, name_or_tag, raise_error: true)
+    raise ArgumentError unless account.local?
+
     @account = account
 
-    FeaturedTag.create!(account: account, name: name).tap do |featured_tag|
+    @featured_tag = begin
-      ActivityPub::AccountRawDistributionWorker.perform_async(build_json(featured_tag), account.id) if @account.local?
+      if name_or_tag.is_a?(Tag)
-    end
+        account.featured_tags.find_or_initialize_by(tag: name_or_tag)
-  rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e
-    if force && e.is_a(ActiveRecord::RecordNotUnique)
-      FeaturedTag.by_name(name).find_by!(account: account)
       else
-      account.featured_tags.new(name: name)
+        account.featured_tags.find_or_initialize_by(name: name_or_tag)
       end
     end
 
+    create_method = raise_error ? :save! : :save
+
+    ActivityPub::AccountRawDistributionWorker.perform_async(build_json(@featured_tag), @account.id) if @featured_tag.new_record? && @featured_tag.public_send(create_method)
+
+    @featured_tag
+  end
+
   private
 
   def build_json(featured_tag)

app/services/remove_featured_tag_service.rb

@@ -3,11 +3,24 @@
 class RemoveFeaturedTagService < BaseService
   include Payloadable
 
-  def call(account, featured_tag)
+  def call(account, featured_tag_or_tag)
+    raise ArgumentError unless account.local?
+
     @account = account
 
-    featured_tag.destroy!
+    @featured_tag = begin
-    ActivityPub::AccountRawDistributionWorker.perform_async(build_json(featured_tag), account.id) if @account.local?
+      if featured_tag_or_tag.is_a?(FeaturedTag)
+        featured_tag_or_tag
+      elsif featured_tag_or_tag.is_a?(Tag)
+        FeaturedTag.find_by(account: account, tag: featured_tag_or_tag)
+      end
+    end
+
+    return if @featured_tag.nil?
+
+    @featured_tag.destroy!
+
+    ActivityPub::AccountRawDistributionWorker.perform_async(build_json(@featured_tag), account.id) if @account.local?
   end
 
   private

config/routes/api.rb

@@ -216,6 +216,8 @@ namespace :api, format: false do
       member do
         post :follow
         post :unfollow
+        post :feature
+        post :unfeature
       end
     end
 

lib/mastodon/version.rb

@@ -45,7 +45,7 @@ module Mastodon
 
     def api_versions
       {
-        mastodon: 5,
+        mastodon: 6,
       }
     end
 

spec/models/featured_tag_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe FeaturedTag do
 
       let(:account) { Fabricate :account }
 
-      it { is_expected.to_not allow_value('Test').for(:name) }
+      it { is_expected.to_not allow_value('Test').for(:name).against(:tag_id) }
 
       context 'when account has hit limit' do
         before { stub_const 'FeaturedTag::LIMIT', 1 }

spec/requests/api/v1/featured_tags_spec.rb

@@ -127,10 +127,10 @@ RSpec.describe 'FeaturedTags' do
         FeaturedTag.create(name: params[:name], account: user.account)
       end
 
-      it 'returns http unprocessable entity' do
+      it 'returns http success' do
         post '/api/v1/featured_tags', headers: headers, params: params
 
-        expect(response).to have_http_status(422)
+        expect(response).to have_http_status(200)
         expect(response.content_type)
           .to start_with('application/json')
       end

spec/requests/api/v1/tags_spec.rb

@@ -161,4 +161,116 @@ RSpec.describe 'Tags' do
       end
     end
   end
+
+  describe 'POST /api/v1/tags/:id/feature' do
+    subject do
+      post "/api/v1/tags/#{name}/feature", headers: headers
+    end
+
+    let!(:tag) { Fabricate(:tag) }
+    let(:name) { tag.name }
+    let(:scopes) { 'write:accounts' }
+
+    it_behaves_like 'forbidden for wrong scope', 'read read:follows'
+
+    context 'when the tag exists' do
+      it 'creates featured tag', :aggregate_failures do
+        subject
+
+        expect(response).to have_http_status(:success)
+        expect(response.content_type)
+          .to start_with('application/json')
+        expect(FeaturedTag.where(tag: tag, account: user.account)).to exist
+      end
+    end
+
+    context 'when the tag does not exist' do
+      let(:name) { 'hoge' }
+
+      it 'creates a new tag with the specified name', :aggregate_failures do
+        subject
+
+        expect(response).to have_http_status(200)
+        expect(response.content_type)
+          .to start_with('application/json')
+        expect(Tag.where(name: name)).to exist
+        expect(FeaturedTag.where(tag: Tag.find_by(name: name), account: user.account)).to exist
+      end
+    end
+
+    context 'when the tag name is invalid' do
+      let(:name) { 'tag-name' }
+
+      it 'returns http not found' do
+        subject
+
+        expect(response).to have_http_status(404)
+        expect(response.content_type)
+          .to start_with('application/json')
+      end
+    end
+
+    context 'when the Authorization header is missing' do
+      let(:headers) { {} }
+      let(:name)    { 'unauthorized' }
+
+      it 'returns http unauthorized' do
+        subject
+
+        expect(response).to have_http_status(401)
+        expect(response.content_type)
+          .to start_with('application/json')
+      end
+    end
+  end
+
+  describe 'POST #unfeature' do
+    subject do
+      post "/api/v1/tags/#{name}/unfeature", headers: headers
+    end
+
+    let(:name) { tag.name }
+    let!(:tag) { Fabricate(:tag, name: 'foo') }
+    let(:scopes) { 'write:accounts' }
+
+    before do
+      Fabricate(:featured_tag, account: user.account, tag: tag)
+    end
+
+    it_behaves_like 'forbidden for wrong scope', 'read read:follows'
+
+    it 'removes the featured tag', :aggregate_failures do
+      subject
+
+      expect(response).to have_http_status(200)
+      expect(response.content_type)
+        .to start_with('application/json')
+      expect(FeaturedTag.where(tag: tag, account: user.account)).to_not exist
+    end
+
+    context 'when the tag name is invalid' do
+      let(:name) { 'tag-name' }
+
+      it 'returns http not found' do
+        subject
+
+        expect(response).to have_http_status(404)
+        expect(response.content_type)
+          .to start_with('application/json')
+      end
+    end
+
+    context 'when the Authorization header is missing' do
+      let(:headers) { {} }
+      let(:name)    { 'unauthorized' }
+
+      it 'returns http unauthorized' do
+        subject
+
+        expect(response).to have_http_status(401)
+        expect(response.content_type)
+          .to start_with('application/json')
+      end
+    end
+  end
 end

spec/services/create_featured_tag_service_spec.rb

@@ -20,11 +20,9 @@ RSpec.describe CreateFeaturedTagService do
     context 'with a remote account' do
       let(:account) { Fabricate(:account, domain: 'host.example') }
 
-      it 'creates a new featured tag and does not distributes' do
+      it 'raises argument error' do
         expect { subject.call(account, tag) }
-          .to change(FeaturedTag, :count).by(1)
+          .to raise_error ArgumentError
-        expect(ActivityPub::AccountRawDistributionWorker)
-          .to_not have_enqueued_sidekiq_job(any_args)
       end
     end
   end

spec/services/remove_featured_tag_service_spec.rb

@@ -23,13 +23,9 @@ RSpec.describe RemoveFeaturedTagService do
       context 'when called by a non local account' do
         let(:account) { Fabricate(:account, domain: 'host.example') }
 
-        it 'destroys the featured tag and does not send a distribution' do
+        it 'raises argument error' do
-          subject.call(account, featured_tag)
+          expect { subject.call(account, featured_tag) }
-
+            .to raise_error(ArgumentError)
-          expect { featured_tag.reload }
-            .to raise_error(ActiveRecord::RecordNotFound)
-          expect(ActivityPub::AccountRawDistributionWorker)
-            .to_not have_enqueued_sidekiq_job(any_args)
         end
       end
     end