From 33ad850c982bbe03214e2e2870751920721c23af Mon Sep 17 00:00:00 2001 From: Takeshi Umeda Date: Wed, 2 Sep 2020 09:13:10 +0900 Subject: Added account featured tags API (#11817) --- .../api/v1/accounts/featured_tags_controller.rb | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 app/controllers/api/v1/accounts/featured_tags_controller.rb (limited to 'app/controllers/api') diff --git a/app/controllers/api/v1/accounts/featured_tags_controller.rb b/app/controllers/api/v1/accounts/featured_tags_controller.rb new file mode 100644 index 000000000..d6277261d --- /dev/null +++ b/app/controllers/api/v1/accounts/featured_tags_controller.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class Api::V1::Accounts::FeaturedTagsController < Api::BaseController + before_action :set_account + before_action :set_featured_tags + + respond_to :json + + def index + render json: @featured_tags, each_serializer: REST::AccountFeaturedTagSerializer + end + + private + + def set_account + @account = Account.find(params[:account_id]) + end + + def set_featured_tags + @featured_tags = @account.featured_tags + end +end -- cgit From e8bc187845b78e4a94894c69ecf930a524ad2056 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Mon, 7 Sep 2020 11:02:04 +0200 Subject: Refactor how public and tag timelines are queried (#14728) --- .../api/v1/timelines/public_controller.rb | 29 ++- app/controllers/api/v1/timelines/tag_controller.rb | 34 +-- app/controllers/tags_controller.rb | 31 +-- app/models/public_feed.rb | 90 ++++++++ app/models/status.rb | 67 +----- app/models/tag_feed.rb | 57 +++++ app/services/hashtag_query_service.rb | 22 -- spec/models/public_feed_spec.rb | 212 +++++++++++++++++++ spec/models/status_spec.rb | 235 --------------------- spec/models/tag_feed_spec.rb | 68 ++++++ spec/services/fan_out_on_write_service_spec.rb | 4 +- spec/services/hashtag_query_service_spec.rb | 60 ------ 12 files changed, 480 insertions(+), 429 deletions(-) create mode 100644 app/models/public_feed.rb create mode 100644 app/models/tag_feed.rb delete mode 100644 app/services/hashtag_query_service.rb create mode 100644 spec/models/public_feed_spec.rb create mode 100644 spec/models/tag_feed_spec.rb delete mode 100644 spec/services/hashtag_query_service_spec.rb (limited to 'app/controllers/api') diff --git a/app/controllers/api/v1/timelines/public_controller.rb b/app/controllers/api/v1/timelines/public_controller.rb index 26d877b00..d253b744f 100644 --- a/app/controllers/api/v1/timelines/public_controller.rb +++ b/app/controllers/api/v1/timelines/public_controller.rb @@ -20,26 +20,25 @@ class Api::V1::Timelines::PublicController < Api::BaseController end def cached_public_statuses_page - cache_collection_paginated_by_id( - public_statuses, - Status, - limit_param(DEFAULT_STATUSES_LIMIT), - params_slice(:max_id, :since_id, :min_id) - ) + cache_collection(public_statuses, Status) end def public_statuses - statuses = public_timeline_statuses - - if truthy_param?(:only_media) - statuses.joins(:media_attachments).group(:id) - else - statuses - end + public_feed.get( + limit_param(DEFAULT_STATUSES_LIMIT), + params[:max_id], + params[:since_id], + params[:min_id] + ) end - def public_timeline_statuses - Status.as_public_timeline(current_account, truthy_param?(:remote) ? :remote : truthy_param?(:local)) + def public_feed + PublicFeed.new( + current_account, + local: truthy_param?(:local), + remote: truthy_param?(:remote), + only_media: truthy_param?(:only_media) + ) end def insert_pagination_headers diff --git a/app/controllers/api/v1/timelines/tag_controller.rb b/app/controllers/api/v1/timelines/tag_controller.rb index 76f7d3590..64a1db58d 100644 --- a/app/controllers/api/v1/timelines/tag_controller.rb +++ b/app/controllers/api/v1/timelines/tag_controller.rb @@ -20,23 +20,29 @@ class Api::V1::Timelines::TagController < Api::BaseController end def cached_tagged_statuses - if @tag.nil? - [] - else - statuses = tag_timeline_statuses - statuses = statuses.joins(:media_attachments) if truthy_param?(:only_media) - - cache_collection_paginated_by_id( - statuses, - Status, - limit_param(DEFAULT_STATUSES_LIMIT), - params_slice(:max_id, :since_id, :min_id) - ) - end + @tag.nil? ? [] : cache_collection(tag_timeline_statuses, Status) end def tag_timeline_statuses - HashtagQueryService.new.call(@tag, params.slice(:any, :all, :none), current_account, truthy_param?(:local)) + tag_feed.get( + limit_param(DEFAULT_STATUSES_LIMIT), + params[:max_id], + params[:since_id], + params[:min_id] + ) + end + + def tag_feed + TagFeed.new( + @tag, + current_account, + any: params[:any], + all: params[:all], + none: params[:none], + local: truthy_param?(:local), + remote: truthy_param?(:remote), + only_media: truthy_param?(:only_media) + ) end def insert_pagination_headers diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 6426a7d69..6616ba107 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -10,8 +10,9 @@ class TagsController < ApplicationController before_action :require_signature!, if: -> { request.format == :json && authorized_fetch_mode? } before_action :authenticate_user!, if: :whitelist_mode? - before_action :set_tag before_action :set_local + before_action :set_tag + before_action :set_statuses before_action :set_body_classes before_action :set_instance_presenter @@ -25,20 +26,11 @@ class TagsController < ApplicationController format.rss do expires_in 0, public: true - - limit = params[:limit].present? ? [params[:limit].to_i, PAGE_SIZE_MAX].min : PAGE_SIZE - @statuses = HashtagQueryService.new.call(@tag, filter_params, nil, @local).limit(limit) - @statuses = cache_collection(@statuses, Status) - render xml: RSS::TagSerializer.render(@tag, @statuses) end format.json do expires_in 3.minutes, public: public_fetch_mode? - - @statuses = HashtagQueryService.new.call(@tag, filter_params, current_account, @local).paginate_by_max_id(PAGE_SIZE, params[:max_id]) - @statuses = cache_collection(@statuses, Status) - render json: collection_presenter, serializer: ActivityPub::CollectionSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json' end end @@ -54,6 +46,15 @@ class TagsController < ApplicationController @local = truthy_param?(:local) end + def set_statuses + case request.format&.to_sym + when :json + @statuses = cache_collection(TagFeed.new(@tag, current_account, local: @local).get(PAGE_SIZE, params[:max_id], params[:since_id], params[:min_id]), Status) + when :rss + @statuses = cache_collection(TagFeed.new(@tag, nil, local: @local).get(limit_param), Status) + end + end + def set_body_classes @body_classes = 'with-modals' end @@ -62,16 +63,16 @@ class TagsController < ApplicationController @instance_presenter = InstancePresenter.new end + def limit_param + params[:limit].present? ? [params[:limit].to_i, PAGE_SIZE_MAX].min : PAGE_SIZE + end + def collection_presenter ActivityPub::CollectionPresenter.new( - id: tag_url(@tag, filter_params), + id: tag_url(@tag), type: :ordered, size: @tag.statuses.count, items: @statuses.map { |s| ActivityPub::TagManager.instance.uri_for(s) } ) end - - def filter_params - params.slice(:any, :all, :none).permit(:any, :all, :none) - end end diff --git a/app/models/public_feed.rb b/app/models/public_feed.rb new file mode 100644 index 000000000..c8ce1a140 --- /dev/null +++ b/app/models/public_feed.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +class PublicFeed < Feed + # @param [Account] account + # @param [Hash] options + # @option [Boolean] :with_replies + # @option [Boolean] :with_reblogs + # @option [Boolean] :local + # @option [Boolean] :remote + # @option [Boolean] :only_media + def initialize(account, options = {}) + @account = account + @options = options + end + + # @param [Integer] limit + # @param [Integer] max_id + # @param [Integer] since_id + # @param [Integer] min_id + # @return [Array] + def get(limit, max_id = nil, since_id = nil, min_id = nil) + scope = public_scope + + scope.merge!(without_replies_scope) unless with_replies? + scope.merge!(without_reblogs_scope) unless with_reblogs? + scope.merge!(local_only_scope) if local_only? + scope.merge!(remote_only_scope) if remote_only? + scope.merge!(account_filters_scope) if account? + scope.merge!(media_only_scope) if media_only? + + scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) + end + + private + + def with_reblogs? + @options[:with_reblogs] + end + + def with_replies? + @options[:with_replies] + end + + def local_only? + @options[:local] + end + + def remote_only? + @options[:remote] + end + + def account? + @account.present? + end + + def media_only? + @options[:only_media] + end + + def public_scope + Status.with_public_visibility.joins(:account).merge(Account.without_suspended.without_silenced) + end + + def local_only_scope + Status.local + end + + def remote_only_scope + Status.remote + end + + def without_replies_scope + Status.without_replies + end + + def without_reblogs_scope + Status.without_reblogs + end + + def media_only_scope + Status.joins(:media_attachments).group(:id) + end + + def account_filters_scope + Status.not_excluded_by_account(@account).tap do |scope| + scope.merge!(Status.not_domain_blocked_by_account(@account)) unless local_only? + scope.merge!(Status.in_chosen_languages(@account)) if @account.chosen_languages.present? + end + end +end diff --git a/app/models/status.rb b/app/models/status.rb index 71596ec2f..c6e16ff75 100644 --- a/app/models/status.rb +++ b/app/models/status.rb @@ -85,12 +85,12 @@ class Status < ApplicationRecord scope :recent, -> { reorder(id: :desc) } scope :remote, -> { where(local: false).where.not(uri: nil) } scope :local, -> { where(local: true).or(where(uri: nil)) } - scope :with_accounts, ->(ids) { where(id: ids).includes(:account) } scope :without_replies, -> { where('statuses.reply = FALSE OR statuses.in_reply_to_account_id = statuses.account_id') } scope :without_reblogs, -> { where('statuses.reblog_of_id IS NULL') } scope :with_public_visibility, -> { where(visibility: :public) } scope :tagged_with, ->(tag) { joins(:statuses_tags).where(statuses_tags: { tag_id: tag }) } + scope :in_chosen_languages, ->(account) { where(language: nil).or where(language: account.chosen_languages) } scope :excluding_silenced_accounts, -> { left_outer_joins(:account).where(accounts: { silenced_at: nil }) } scope :including_silenced_accounts, -> { left_outer_joins(:account).where.not(accounts: { silenced_at: nil }) } scope :not_excluded_by_account, ->(account) { where.not(account_id: account.excluded_from_timeline_account_ids) } @@ -277,26 +277,6 @@ class Status < ApplicationRecord visibilities.keys - %w(direct limited) end - def in_chosen_languages(account) - where(language: nil).or where(language: account.chosen_languages) - end - - def as_public_timeline(account = nil, local_only = false) - query = timeline_scope(local_only).without_replies - - apply_timeline_filters(query, account, [:local, true].include?(local_only)) - end - - def as_tag_timeline(tag, account = nil, local_only = false) - query = timeline_scope(local_only).tagged_with(tag) - - apply_timeline_filters(query, account, local_only) - end - - def as_outbox_timeline(account) - where(account: account, visibility: :public) - end - def favourites_map(status_ids, account_id) Favourite.select('status_id').where(status_id: status_ids).where(account_id: account_id).each_with_object({}) { |f, h| h[f.status_id] = true } end @@ -373,51 +353,6 @@ class Status < ApplicationRecord status&.distributable? ? status : nil end.compact end - - private - - def timeline_scope(scope = false) - starting_scope = case scope - when :local, true - Status.local - when :remote - Status.remote - else - Status - end - - starting_scope - .with_public_visibility - .without_reblogs - end - - def apply_timeline_filters(query, account, local_only) - if account.nil? - filter_timeline_default(query) - else - filter_timeline_for_account(query, account, local_only) - end - end - - def filter_timeline_for_account(query, account, local_only) - query = query.not_excluded_by_account(account) - query = query.not_domain_blocked_by_account(account) unless local_only - query = query.in_chosen_languages(account) if account.chosen_languages.present? - query.merge(account_silencing_filter(account)) - end - - def filter_timeline_default(query) - query.excluding_silenced_accounts - end - - def account_silencing_filter(account) - if account.silenced? - including_myself = left_outer_joins(:account).where(account_id: account.id).references(:accounts) - excluding_silenced_accounts.or(including_myself) - else - excluding_silenced_accounts - end - end end def status_stat diff --git a/app/models/tag_feed.rb b/app/models/tag_feed.rb new file mode 100644 index 000000000..50634fe83 --- /dev/null +++ b/app/models/tag_feed.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class TagFeed < PublicFeed + LIMIT_PER_MODE = 4 + + # @param [Tag] tag + # @param [Account] account + # @param [Hash] options + # @option [Enumerable] :any + # @option [Enumerable] :all + # @option [Enumerable] :none + # @option [Boolean] :local + # @option [Boolean] :remote + # @option [Boolean] :only_media + def initialize(tag, account, options = {}) + @tag = tag + @account = account + @options = options + end + + # @param [Integer] limit + # @param [Integer] max_id + # @param [Integer] since_id + # @param [Integer] min_id + # @return [Array] + def get(limit, max_id = nil, since_id = nil, min_id = nil) + scope = public_scope + + scope.merge!(tagged_with_any_scope) + scope.merge!(tagged_with_all_scope) + scope.merge!(tagged_with_none_scope) + scope.merge!(local_only_scope) if local_only? + scope.merge!(remote_only_scope) if remote_only? + scope.merge!(account_filters_scope) if account? + scope.merge!(media_only_scope) if media_only? + + scope.cache_ids.to_a_paginated_by_id(limit, max_id: max_id, since_id: since_id, min_id: min_id) + end + + private + + def tagged_with_any_scope + Status.group(:id).tagged_with(tags_for(Array(@tag.name) | Array(@options[:any]))) + end + + def tagged_with_all_scope + Status.group(:id).tagged_with_all(tags_for(@options[:all])) + end + + def tagged_with_none_scope + Status.group(:id).tagged_with_none(tags_for(@options[:none])) + end + + def tags_for(names) + Tag.matching_name(Array(names).take(LIMIT_PER_MODE)) if names.present? + end +end diff --git a/app/services/hashtag_query_service.rb b/app/services/hashtag_query_service.rb deleted file mode 100644 index 0bdf60221..000000000 --- a/app/services/hashtag_query_service.rb +++ /dev/null @@ -1,22 +0,0 @@ -# frozen_string_literal: true - -class HashtagQueryService < BaseService - LIMIT_PER_MODE = 4 - - def call(tag, params, account = nil, local = false) - tags = tags_for(Array(tag.name) | Array(params[:any])).pluck(:id) - all = tags_for(params[:all]) - none = tags_for(params[:none]) - - Status.group(:id) - .as_tag_timeline(tags, account, local) - .tagged_with_all(all) - .tagged_with_none(none) - end - - private - - def tags_for(names) - Tag.matching_name(Array(names).take(LIMIT_PER_MODE)) if names.present? - end -end diff --git a/spec/models/public_feed_spec.rb b/spec/models/public_feed_spec.rb new file mode 100644 index 000000000..0392a582c --- /dev/null +++ b/spec/models/public_feed_spec.rb @@ -0,0 +1,212 @@ +require 'rails_helper' + +RSpec.describe PublicFeed, type: :model do + let(:account) { Fabricate(:account) } + + describe '#get' do + subject { described_class.new(nil).get(20).map(&:id) } + + it 'only includes statuses with public visibility' do + public_status = Fabricate(:status, visibility: :public) + private_status = Fabricate(:status, visibility: :private) + + expect(subject).to include(public_status.id) + expect(subject).not_to include(private_status.id) + end + + it 'does not include replies' do + status = Fabricate(:status) + reply = Fabricate(:status, in_reply_to_id: status.id) + + expect(subject).to include(status.id) + expect(subject).not_to include(reply.id) + end + + it 'does not include boosts' do + status = Fabricate(:status) + boost = Fabricate(:status, reblog_of_id: status.id) + + expect(subject).to include(status.id) + expect(subject).not_to include(boost.id) + end + + it 'filters out silenced accounts' do + account = Fabricate(:account) + silenced_account = Fabricate(:account, silenced: true) + status = Fabricate(:status, account: account) + silenced_status = Fabricate(:status, account: silenced_account) + + expect(subject).to include(status.id) + expect(subject).not_to include(silenced_status.id) + end + + context 'without local_only option' do + let(:viewer) { nil } + + let!(:local_account) { Fabricate(:account, domain: nil) } + let!(:remote_account) { Fabricate(:account, domain: 'test.com') } + let!(:local_status) { Fabricate(:status, account: local_account) } + let!(:remote_status) { Fabricate(:status, account: remote_account) } + + subject { described_class.new(viewer).get(20).map(&:id) } + + context 'without a viewer' do + let(:viewer) { nil } + + it 'includes remote instances statuses' do + expect(subject).to include(remote_status.id) + end + + it 'includes local statuses' do + expect(subject).to include(local_status.id) + end + end + + context 'with a viewer' do + let(:viewer) { Fabricate(:account, username: 'viewer') } + + it 'includes remote instances statuses' do + expect(subject).to include(remote_status.id) + end + + it 'includes local statuses' do + expect(subject).to include(local_status.id) + end + end + end + + context 'with a local_only option set' do + let!(:local_account) { Fabricate(:account, domain: nil) } + let!(:remote_account) { Fabricate(:account, domain: 'test.com') } + let!(:local_status) { Fabricate(:status, account: local_account) } + let!(:remote_status) { Fabricate(:status, account: remote_account) } + + subject { described_class.new(viewer, local: true).get(20).map(&:id) } + + context 'without a viewer' do + let(:viewer) { nil } + + it 'does not include remote instances statuses' do + expect(subject).to include(local_status.id) + expect(subject).not_to include(remote_status.id) + end + end + + context 'with a viewer' do + let(:viewer) { Fabricate(:account, username: 'viewer') } + + it 'does not include remote instances statuses' do + expect(subject).to include(local_status.id) + expect(subject).not_to include(remote_status.id) + end + + it 'is not affected by personal domain blocks' do + viewer.block_domain!('test.com') + expect(subject).to include(local_status.id) + expect(subject).not_to include(remote_status.id) + end + end + end + + context 'with a remote_only option set' do + let!(:local_account) { Fabricate(:account, domain: nil) } + let!(:remote_account) { Fabricate(:account, domain: 'test.com') } + let!(:local_status) { Fabricate(:status, account: local_account) } + let!(:remote_status) { Fabricate(:status, account: remote_account) } + + subject { described_class.new(viewer, remote: true).get(20).map(&:id) } + + context 'without a viewer' do + let(:viewer) { nil } + + it 'does not include local instances statuses' do + expect(subject).not_to include(local_status.id) + expect(subject).to include(remote_status.id) + end + end + + context 'with a viewer' do + let(:viewer) { Fabricate(:account, username: 'viewer') } + + it 'does not include local instances statuses' do + expect(subject).not_to include(local_status.id) + expect(subject).to include(remote_status.id) + end + end + end + + describe 'with an account passed in' do + before do + @account = Fabricate(:account) + end + + subject { described_class.new(@account).get(20).map(&:id) } + + it 'excludes statuses from accounts blocked by the account' do + blocked = Fabricate(:account) + @account.block!(blocked) + blocked_status = Fabricate(:status, account: blocked) + + expect(subject).not_to include(blocked_status.id) + end + + it 'excludes statuses from accounts who have blocked the account' do + blocker = Fabricate(:account) + blocker.block!(@account) + blocked_status = Fabricate(:status, account: blocker) + + expect(subject).not_to include(blocked_status.id) + end + + it 'excludes statuses from accounts muted by the account' do + muted = Fabricate(:account) + @account.mute!(muted) + muted_status = Fabricate(:status, account: muted) + + expect(subject).not_to include(muted_status.id) + end + + it 'excludes statuses from accounts from personally blocked domains' do + blocked = Fabricate(:account, domain: 'example.com') + @account.block_domain!(blocked.domain) + blocked_status = Fabricate(:status, account: blocked) + + expect(subject).not_to include(blocked_status.id) + end + + context 'with language preferences' do + it 'excludes statuses in languages not allowed by the account user' do + user = Fabricate(:user, chosen_languages: [:en, :es]) + @account.update(user: user) + en_status = Fabricate(:status, language: 'en') + es_status = Fabricate(:status, language: 'es') + fr_status = Fabricate(:status, language: 'fr') + + expect(subject).to include(en_status.id) + expect(subject).to include(es_status.id) + expect(subject).not_to include(fr_status.id) + end + + it 'includes all languages when user does not have a setting' do + user = Fabricate(:user, chosen_languages: nil) + @account.update(user: user) + + en_status = Fabricate(:status, language: 'en') + es_status = Fabricate(:status, language: 'es') + + expect(subject).to include(en_status.id) + expect(subject).to include(es_status.id) + end + + it 'includes all languages when account does not have a user' do + expect(@account.user).to be_nil + en_status = Fabricate(:status, language: 'en') + es_status = Fabricate(:status, language: 'es') + + expect(subject).to include(en_status.id) + expect(subject).to include(es_status.id) + end + end + end + end +end diff --git a/spec/models/status_spec.rb b/spec/models/status_spec.rb index 4aee14cbd..20fb894e7 100644 --- a/spec/models/status_spec.rb +++ b/spec/models/status_spec.rb @@ -267,241 +267,6 @@ RSpec.describe Status, type: :model do end end - describe '.as_public_timeline' do - it 'only includes statuses with public visibility' do - public_status = Fabricate(:status, visibility: :public) - private_status = Fabricate(:status, visibility: :private) - - results = Status.as_public_timeline - expect(results).to include(public_status) - expect(results).not_to include(private_status) - end - - it 'does not include replies' do - status = Fabricate(:status) - reply = Fabricate(:status, in_reply_to_id: status.id) - - results = Status.as_public_timeline - expect(results).to include(status) - expect(results).not_to include(reply) - end - - it 'does not include boosts' do - status = Fabricate(:status) - boost = Fabricate(:status, reblog_of_id: status.id) - - results = Status.as_public_timeline - expect(results).to include(status) - expect(results).not_to include(boost) - end - - it 'filters out silenced accounts' do - account = Fabricate(:account) - silenced_account = Fabricate(:account, silenced: true) - status = Fabricate(:status, account: account) - silenced_status = Fabricate(:status, account: silenced_account) - - results = Status.as_public_timeline - expect(results).to include(status) - expect(results).not_to include(silenced_status) - end - - context 'without local_only option' do - let(:viewer) { nil } - - let!(:local_account) { Fabricate(:account, domain: nil) } - let!(:remote_account) { Fabricate(:account, domain: 'test.com') } - let!(:local_status) { Fabricate(:status, account: local_account) } - let!(:remote_status) { Fabricate(:status, account: remote_account) } - - subject { Status.as_public_timeline(viewer, false) } - - context 'without a viewer' do - let(:viewer) { nil } - - it 'includes remote instances statuses' do - expect(subject).to include(remote_status) - end - - it 'includes local statuses' do - expect(subject).to include(local_status) - end - end - - context 'with a viewer' do - let(:viewer) { Fabricate(:account, username: 'viewer') } - - it 'includes remote instances statuses' do - expect(subject).to include(remote_status) - end - - it 'includes local statuses' do - expect(subject).to include(local_status) - end - end - end - - context 'with a local_only option set' do - let!(:local_account) { Fabricate(:account, domain: nil) } - let!(:remote_account) { Fabricate(:account, domain: 'test.com') } - let!(:local_status) { Fabricate(:status, account: local_account) } - let!(:remote_status) { Fabricate(:status, account: remote_account) } - - subject { Status.as_public_timeline(viewer, true) } - - context 'without a viewer' do - let(:viewer) { nil } - - it 'does not include remote instances statuses' do - expect(subject).to include(local_status) - expect(subject).not_to include(remote_status) - end - end - - context 'with a viewer' do - let(:viewer) { Fabricate(:account, username: 'viewer') } - - it 'does not include remote instances statuses' do - expect(subject).to include(local_status) - expect(subject).not_to include(remote_status) - end - - it 'is not affected by personal domain blocks' do - viewer.block_domain!('test.com') - expect(subject).to include(local_status) - expect(subject).not_to include(remote_status) - end - end - end - - context 'with a remote_only option set' do - let!(:local_account) { Fabricate(:account, domain: nil) } - let!(:remote_account) { Fabricate(:account, domain: 'test.com') } - let!(:local_status) { Fabricate(:status, account: local_account) } - let!(:remote_status) { Fabricate(:status, account: remote_account) } - - subject { Status.as_public_timeline(viewer, :remote) } - - context 'without a viewer' do - let(:viewer) { nil } - - it 'does not include local instances statuses' do - expect(subject).not_to include(local_status) - expect(subject).to include(remote_status) - end - end - - context 'with a viewer' do - let(:viewer) { Fabricate(:account, username: 'viewer') } - - it 'does not include local instances statuses' do - expect(subject).not_to include(local_status) - expect(subject).to include(remote_status) - end - end - end - - describe 'with an account passed in' do - before do - @account = Fabricate(:account) - end - - it 'excludes statuses from accounts blocked by the account' do - blocked = Fabricate(:account) - Fabricate(:block, account: @account, target_account: blocked) - blocked_status = Fabricate(:status, account: blocked) - - results = Status.as_public_timeline(@account) - expect(results).not_to include(blocked_status) - end - - it 'excludes statuses from accounts who have blocked the account' do - blocked = Fabricate(:account) - Fabricate(:block, account: blocked, target_account: @account) - blocked_status = Fabricate(:status, account: blocked) - - results = Status.as_public_timeline(@account) - expect(results).not_to include(blocked_status) - end - - it 'excludes statuses from accounts muted by the account' do - muted = Fabricate(:account) - Fabricate(:mute, account: @account, target_account: muted) - muted_status = Fabricate(:status, account: muted) - - results = Status.as_public_timeline(@account) - expect(results).not_to include(muted_status) - end - - it 'excludes statuses from accounts from personally blocked domains' do - blocked = Fabricate(:account, domain: 'example.com') - @account.block_domain!(blocked.domain) - blocked_status = Fabricate(:status, account: blocked) - - results = Status.as_public_timeline(@account) - expect(results).not_to include(blocked_status) - end - - context 'with language preferences' do - it 'excludes statuses in languages not allowed by the account user' do - user = Fabricate(:user, chosen_languages: [:en, :es]) - @account.update(user: user) - en_status = Fabricate(:status, language: 'en') - es_status = Fabricate(:status, language: 'es') - fr_status = Fabricate(:status, language: 'fr') - - results = Status.as_public_timeline(@account) - expect(results).to include(en_status) - expect(results).to include(es_status) - expect(results).not_to include(fr_status) - end - - it 'includes all languages when user does not have a setting' do - user = Fabricate(:user, chosen_languages: nil) - @account.update(user: user) - - en_status = Fabricate(:status, language: 'en') - es_status = Fabricate(:status, language: 'es') - - results = Status.as_public_timeline(@account) - expect(results).to include(en_status) - expect(results).to include(es_status) - end - - it 'includes all languages when account does not have a user' do - expect(@account.user).to be_nil - en_status = Fabricate(:status, language: 'en') - es_status = Fabricate(:status, language: 'es') - - results = Status.as_public_timeline(@account) - expect(results).to include(en_status) - expect(results).to include(es_status) - end - end - end - end - - describe '.as_tag_timeline' do - it 'includes statuses with a tag' do - tag = Fabricate(:tag) - status = Fabricate(:status, tags: [tag]) - other = Fabricate(:status) - - results = Status.as_tag_timeline(tag) - expect(results).to include(status) - expect(results).not_to include(other) - end - - it 'allows replies to be included' do - original = Fabricate(:status) - tag = Fabricate(:tag) - status = Fabricate(:status, tags: [tag], in_reply_to_id: original.id) - - results = Status.as_tag_timeline(tag) - expect(results).to include(status) - end - end - describe '.permitted_for' do subject { described_class.permitted_for(target_account, account).pluck(:visibility) } diff --git a/spec/models/tag_feed_spec.rb b/spec/models/tag_feed_spec.rb new file mode 100644 index 000000000..17d88eb99 --- /dev/null +++ b/spec/models/tag_feed_spec.rb @@ -0,0 +1,68 @@ +require 'rails_helper' + +describe TagFeed, type: :service do + describe '#get' do + let(:account) { Fabricate(:account) } + let(:tag1) { Fabricate(:tag) } + let(:tag2) { Fabricate(:tag) } + let!(:status1) { Fabricate(:status, tags: [tag1]) } + let!(:status2) { Fabricate(:status, tags: [tag2]) } + let!(:both) { Fabricate(:status, tags: [tag1, tag2]) } + + it 'can add tags in "any" mode' do + results = described_class.new(tag1, nil, any: [tag2.name]).get(20) + expect(results).to include status1 + expect(results).to include status2 + expect(results).to include both + end + + it 'can remove tags in "all" mode' do + results = described_class.new(tag1, nil, all: [tag2.name]).get(20) + expect(results).to_not include status1 + expect(results).to_not include status2 + expect(results).to include both + end + + it 'can remove tags in "none" mode' do + results = described_class.new(tag1, nil, none: [tag2.name]).get(20) + expect(results).to include status1 + expect(results).to_not include status2 + expect(results).to_not include both + end + + it 'ignores an invalid mode' do + results = described_class.new(tag1, nil, wark: [tag2.name]).get(20) + expect(results).to include status1 + expect(results).to_not include status2 + expect(results).to include both + end + + it 'handles being passed non existant tag names' do + results = described_class.new(tag1, nil, any: ['wark']).get(20) + expect(results).to include status1 + expect(results).to_not include status2 + expect(results).to include both + end + + it 'can restrict to an account' do + BlockService.new.call(account, status1.account) + results = described_class.new(tag1, account, none: [tag2.name]).get(20) + expect(results).to_not include status1 + end + + it 'can restrict to local' do + status1.account.update(domain: 'example.com') + status1.update(local: false, uri: 'example.com/toot') + results = described_class.new(tag1, nil, any: [tag2.name], local: true).get(20) + expect(results).to_not include status1 + end + + it 'allows replies to be included' do + original = Fabricate(:status) + status = Fabricate(:status, tags: [tag1], in_reply_to_id: original.id) + + results = described_class.new(tag1, nil).get(20) + expect(results).to include(status) + end + end +end diff --git a/spec/services/fan_out_on_write_service_spec.rb b/spec/services/fan_out_on_write_service_spec.rb index b7fc7f7ed..538dc2592 100644 --- a/spec/services/fan_out_on_write_service_spec.rb +++ b/spec/services/fan_out_on_write_service_spec.rb @@ -28,10 +28,10 @@ RSpec.describe FanOutOnWriteService, type: :service do end it 'delivers status to hashtag' do - expect(Tag.find_by!(name: 'test').statuses.pluck(:id)).to include status.id + expect(TagFeed.new(Tag.find_by(name: 'test'), alice).get(20).map(&:id)).to include status.id end it 'delivers status to public timeline' do - expect(Status.as_public_timeline(alice).map(&:id)).to include status.id + expect(PublicFeed.new(alice).get(20).map(&:id)).to include status.id end end diff --git a/spec/services/hashtag_query_service_spec.rb b/spec/services/hashtag_query_service_spec.rb deleted file mode 100644 index 24282d2f0..000000000 --- a/spec/services/hashtag_query_service_spec.rb +++ /dev/null @@ -1,60 +0,0 @@ -require 'rails_helper' - -describe HashtagQueryService, type: :service do - describe '.call' do - let(:account) { Fabricate(:account) } - let(:tag1) { Fabricate(:tag) } - let(:tag2) { Fabricate(:tag) } - let!(:status1) { Fabricate(:status, tags: [tag1]) } - let!(:status2) { Fabricate(:status, tags: [tag2]) } - let!(:both) { Fabricate(:status, tags: [tag1, tag2]) } - - it 'can add tags in "any" mode' do - results = subject.call(tag1, { any: [tag2.name] }) - expect(results).to include status1 - expect(results).to include status2 - expect(results).to include both - end - - it 'can remove tags in "all" mode' do - results = subject.call(tag1, { all: [tag2.name] }) - expect(results).to_not include status1 - expect(results).to_not include status2 - expect(results).to include both - end - - it 'can remove tags in "none" mode' do - results = subject.call(tag1, { none: [tag2.name] }) - expect(results).to include status1 - expect(results).to_not include status2 - expect(results).to_not include both - end - - it 'ignores an invalid mode' do - results = subject.call(tag1, { wark: [tag2.name] }) - expect(results).to include status1 - expect(results).to_not include status2 - expect(results).to include both - end - - it 'handles being passed non existant tag names' do - results = subject.call(tag1, { any: ['wark'] }) - expect(results).to include status1 - expect(results).to_not include status2 - expect(results).to include both - end - - it 'can restrict to an account' do - BlockService.new.call(account, status1.account) - results = subject.call(tag1, { none: [tag2.name] }, account) - expect(results).to_not include status1 - end - - it 'can restrict to local' do - status1.account.update(domain: 'example.com') - status1.update(local: false, uri: 'example.com/toot') - results = subject.call(tag1, { any: [tag2.name] }, nil, true) - expect(results).to_not include status1 - end - end -end -- cgit From e79d719e92e120ba3dd6ec2d8521f7aaa9482634 Mon Sep 17 00:00:00 2001 From: abcang Date: Tue, 8 Sep 2020 00:47:41 +0900 Subject: Changed tag most_used to recently_used (#14760) --- app/controllers/api/v1/featured_tags/suggestions_controller.rb | 8 ++++---- app/controllers/settings/featured_tags_controller.rb | 8 ++++---- app/models/tag.rb | 2 +- app/views/settings/featured_tags/index.html.haml | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) (limited to 'app/controllers/api') diff --git a/app/controllers/api/v1/featured_tags/suggestions_controller.rb b/app/controllers/api/v1/featured_tags/suggestions_controller.rb index 8c1b81a0f..75545d3c7 100644 --- a/app/controllers/api/v1/featured_tags/suggestions_controller.rb +++ b/app/controllers/api/v1/featured_tags/suggestions_controller.rb @@ -3,15 +3,15 @@ class Api::V1::FeaturedTags::SuggestionsController < Api::BaseController before_action -> { doorkeeper_authorize! :read, :'read:accounts' }, only: :index before_action :require_user! - before_action :set_most_used_tags, only: :index + before_action :set_recently_used_tags, only: :index def index - render json: @most_used_tags, each_serializer: REST::TagSerializer + render json: @recently_used_tags, each_serializer: REST::TagSerializer end private - def set_most_used_tags - @most_used_tags = Tag.most_used(current_account).where.not(id: current_account.featured_tags).limit(10) + def set_recently_used_tags + @recently_used_tags = Tag.recently_used(current_account).where.not(id: current_account.featured_tags).limit(10) end end diff --git a/app/controllers/settings/featured_tags_controller.rb b/app/controllers/settings/featured_tags_controller.rb index 3a3241425..e9861da56 100644 --- a/app/controllers/settings/featured_tags_controller.rb +++ b/app/controllers/settings/featured_tags_controller.rb @@ -6,7 +6,7 @@ class Settings::FeaturedTagsController < Settings::BaseController before_action :authenticate_user! before_action :set_featured_tags, only: :index before_action :set_featured_tag, except: [:index, :create] - before_action :set_most_used_tags, only: :index + before_action :set_recently_used_tags, only: :index def index @featured_tag = FeaturedTag.new @@ -20,7 +20,7 @@ class Settings::FeaturedTagsController < Settings::BaseController redirect_to settings_featured_tags_path else set_featured_tags - set_most_used_tags + set_recently_used_tags render :index end @@ -41,8 +41,8 @@ class Settings::FeaturedTagsController < Settings::BaseController @featured_tags = current_account.featured_tags.order(statuses_count: :desc).reject(&:new_record?) end - def set_most_used_tags - @most_used_tags = Tag.most_used(current_account).where.not(id: @featured_tags.map(&:id)).limit(10) + def set_recently_used_tags + @recently_used_tags = Tag.recently_used(current_account).where.not(id: @featured_tags.map(&:id)).limit(10) end def featured_tag_params diff --git a/app/models/tag.rb b/app/models/tag.rb index bce76fc16..df2f86d95 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -39,7 +39,7 @@ class Tag < ApplicationRecord scope :listable, -> { where(listable: [true, nil]) } scope :trendable, -> { Setting.trendable_by_default ? where(trendable: [true, nil]) : where(trendable: true) } scope :discoverable, -> { listable.joins(:account_tag_stat).where(AccountTagStat.arel_table[:accounts_count].gt(0)).order(Arel.sql('account_tag_stats.accounts_count desc')) } - scope :most_used, ->(account) { joins(:statuses).where(statuses: { account: account }).group(:id).order(Arel.sql('count(*) desc')) } + scope :recently_used, ->(account) { joins(:statuses).where(statuses: { id: account.statuses.select(:id).limit(1000) }).group(:id).order(Arel.sql('count(*) desc')) } scope :matches_name, ->(value) { where(arel_table[:name].matches("#{value}%")) } delegate :accounts_count, diff --git a/app/views/settings/featured_tags/index.html.haml b/app/views/settings/featured_tags/index.html.haml index 6734d027c..297379893 100644 --- a/app/views/settings/featured_tags/index.html.haml +++ b/app/views/settings/featured_tags/index.html.haml @@ -9,7 +9,7 @@ = render 'shared/error_messages', object: @featured_tag .fields-group - = f.input :name, wrapper: :with_block_label, hint: safe_join([t('simple_form.hints.featured_tag.name'), safe_join(@most_used_tags.map { |tag| link_to("##{tag.name}", settings_featured_tags_path(featured_tag: { name: tag.name }), method: :post) }, ', ')], ' ') + = f.input :name, wrapper: :with_block_label, hint: safe_join([t('simple_form.hints.featured_tag.name'), safe_join(@recently_used_tags.map { |tag| link_to("##{tag.name}", settings_featured_tags_path(featured_tag: { name: tag.name }), method: :post) }, ', ')], ' ') .actions = f.button :button, t('featured_tags.add_new'), type: :submit -- cgit