From a97647158c79e1c9bca80f52bad57a657d79723b Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 25 Apr 2025 17:12:05 +0200 Subject: [PATCH] Add REST API for featuring and unfeaturing a hashtag (#34489) Co-authored-by: Matt Jankowski Co-authored-by: Claire --- .../api/v1/featured_tags_controller.rb | 2 +- app/controllers/api/v1/tags_controller.rb | 13 +- .../settings/featured_tags_controller.rb | 2 +- app/models/featured_tag.rb | 24 ++-- app/presenters/tag_relationships_presenter.rb | 14 ++- app/serializers/rest/tag_serializer.rb | 9 ++ app/services/create_featured_tag_service.rb | 24 ++-- app/services/remove_featured_tag_service.rb | 19 ++- config/routes/api.rb | 2 + lib/mastodon/version.rb | 2 +- spec/models/featured_tag_spec.rb | 2 +- spec/requests/api/v1/featured_tags_spec.rb | 4 +- spec/requests/api/v1/tags_spec.rb | 112 ++++++++++++++++++ .../create_featured_tag_service_spec.rb | 6 +- .../remove_featured_tag_service_spec.rb | 10 +- 15 files changed, 195 insertions(+), 50 deletions(-) diff --git a/app/controllers/api/v1/featured_tags_controller.rb b/app/controllers/api/v1/featured_tags_controller.rb index 516046f009..15c5de67a2 100644 --- a/app/controllers/api/v1/featured_tags_controller.rb +++ b/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 diff --git a/app/controllers/api/v1/tags_controller.rb b/app/controllers/api/v1/tags_controller.rb index 672535a018..67a4d8ef49 100644 --- a/app/controllers/api/v1/tags_controller.rb +++ b/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 diff --git a/app/controllers/settings/featured_tags_controller.rb b/app/controllers/settings/featured_tags_controller.rb index 0f352e1913..d2fbd1f0f3 100644 --- a/app/controllers/settings/featured_tags_controller.rb +++ b/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 diff --git a/app/models/featured_tag.rb b/app/models/featured_tag.rb index dfc700649c..74fc72ba5c 100644 --- a/app/models/featured_tag.rb +++ b/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_create :reset_data + before_validation :set_tag - 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 - self.tag = Tag.find_or_create_by_names(name)&.first + 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 diff --git a/app/presenters/tag_relationships_presenter.rb b/app/presenters/tag_relationships_presenter.rb index 52e24314be..922eb7a39b 100644 --- a/app/presenters/tag_relationships_presenter.rb +++ b/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? - {} - 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] || {}) - end + if current_account_id.nil? + @following_map = {} + @featuring_map = {} + else + @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 diff --git a/app/serializers/rest/tag_serializer.rb b/app/serializers/rest/tag_serializer.rb index a2bcb5fd1f..f41a1513db 100644 --- a/app/serializers/rest/tag_serializer.rb +++ b/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 diff --git a/app/services/create_featured_tag_service.rb b/app/services/create_featured_tag_service.rb index 3cc59156db..13d6ac0201 100644 --- a/app/services/create_featured_tag_service.rb +++ b/app/services/create_featured_tag_service.rb @@ -3,18 +3,24 @@ 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| - ActivityPub::AccountRawDistributionWorker.perform_async(build_json(featured_tag), account.id) if @account.local? - end - 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) + @featured_tag = begin + if name_or_tag.is_a?(Tag) + account.featured_tags.find_or_initialize_by(tag: name_or_tag) + else + 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 diff --git a/app/services/remove_featured_tag_service.rb b/app/services/remove_featured_tag_service.rb index 2aa70e8fc6..af8c5a64ee 100644 --- a/app/services/remove_featured_tag_service.rb +++ b/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! - ActivityPub::AccountRawDistributionWorker.perform_async(build_json(featured_tag), account.id) if @account.local? + @featured_tag = begin + 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 diff --git a/config/routes/api.rb b/config/routes/api.rb index 8fb8f5d0af..9c94467f01 100644 --- a/config/routes/api.rb +++ b/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 diff --git a/lib/mastodon/version.rb b/lib/mastodon/version.rb index 6a77163f7c..88e82a2b58 100644 --- a/lib/mastodon/version.rb +++ b/lib/mastodon/version.rb @@ -45,7 +45,7 @@ module Mastodon def api_versions { - mastodon: 5, + mastodon: 6, } end diff --git a/spec/models/featured_tag_spec.rb b/spec/models/featured_tag_spec.rb index 20059cfba4..1197776b02 100644 --- a/spec/models/featured_tag_spec.rb +++ b/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 } diff --git a/spec/requests/api/v1/featured_tags_spec.rb b/spec/requests/api/v1/featured_tags_spec.rb index b9c78cc11b..7a5f92cdfd 100644 --- a/spec/requests/api/v1/featured_tags_spec.rb +++ b/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 diff --git a/spec/requests/api/v1/tags_spec.rb b/spec/requests/api/v1/tags_spec.rb index f6ff7c614f..5beda68db0 100644 --- a/spec/requests/api/v1/tags_spec.rb +++ b/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 diff --git a/spec/services/create_featured_tag_service_spec.rb b/spec/services/create_featured_tag_service_spec.rb index f057bc8538..ce8f8a4c38 100644 --- a/spec/services/create_featured_tag_service_spec.rb +++ b/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) - expect(ActivityPub::AccountRawDistributionWorker) - .to_not have_enqueued_sidekiq_job(any_args) + .to raise_error ArgumentError end end end diff --git a/spec/services/remove_featured_tag_service_spec.rb b/spec/services/remove_featured_tag_service_spec.rb index 2f0694bc65..18aa1a9762 100644 --- a/spec/services/remove_featured_tag_service_spec.rb +++ b/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 - subject.call(account, featured_tag) - - expect { featured_tag.reload } - .to raise_error(ActiveRecord::RecordNotFound) - expect(ActivityPub::AccountRawDistributionWorker) - .to_not have_enqueued_sidekiq_job(any_args) + it 'raises argument error' do + expect { subject.call(account, featured_tag) } + .to raise_error(ArgumentError) end end end