From 683ba5ecb1beb2f5654df88dbcae3205feff820d Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Sat, 1 Jul 2023 15:48:16 -0400 Subject: [PATCH] Fix rails `rewhere` deprecation warning in directories api controller (#25625) --- .../api/v1/directories_controller.rb | 34 +++++- app/models/account.rb | 2 +- .../api/v1/directories_controller_spec.rb | 115 +++++++++++++++++- 3 files changed, 140 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/v1/directories_controller.rb b/app/controllers/api/v1/directories_controller.rb index c0585e8599..1109435507 100644 --- a/app/controllers/api/v1/directories_controller.rb +++ b/app/controllers/api/v1/directories_controller.rb @@ -21,11 +21,35 @@ class Api::V1::DirectoriesController < Api::BaseController def accounts_scope Account.discoverable.tap do |scope| - scope.merge!(Account.local) if truthy_param?(:local) - scope.merge!(Account.by_recent_status) if params[:order].blank? || params[:order] == 'active' - scope.merge!(Account.order(id: :desc)) if params[:order] == 'new' - scope.merge!(Account.not_excluded_by_account(current_account)) if current_account - scope.merge!(Account.not_domain_blocked_by_account(current_account)) if current_account && !truthy_param?(:local) + scope.merge!(account_order_scope) + scope.merge!(local_account_scope) if local_accounts? + scope.merge!(account_exclusion_scope) if current_account + scope.merge!(account_domain_block_scope) if current_account && !local_accounts? end end + + def local_accounts? + truthy_param?(:local) + end + + def account_order_scope + case params[:order] + when 'new' + Account.order(id: :desc) + when 'active', nil + Account.by_recent_status + end + end + + def local_account_scope + Account.local + end + + def account_exclusion_scope + Account.not_excluded_by_account(current_account) + end + + def account_domain_block_scope + Account.not_domain_blocked_by_account(current_account) + end end diff --git a/app/models/account.rb b/app/models/account.rb index 8a606fd2a2..aa2cb395db 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -112,7 +112,7 @@ class Account < ApplicationRecord scope :matches_username, ->(value) { where('lower((username)::text) LIKE lower(?)', "#{value}%") } scope :matches_display_name, ->(value) { where(arel_table[:display_name].matches("#{value}%")) } scope :matches_domain, ->(value) { where(arel_table[:domain].matches("%#{value}%")) } - scope :without_unapproved, -> { left_outer_joins(:user).remote.or(left_outer_joins(:user).merge(User.approved.confirmed)) } + scope :without_unapproved, -> { left_outer_joins(:user).merge(User.approved.confirmed).or(remote) } scope :searchable, -> { without_unapproved.without_suspended.where(moved_to_account_id: nil) } scope :discoverable, -> { searchable.without_silenced.where(discoverable: true).left_outer_joins(:account_stat) } scope :followable_by, ->(account) { joins(arel_table.join(Follow.arel_table, Arel::Nodes::OuterJoin).on(arel_table[:id].eq(Follow.arel_table[:target_account_id]).and(Follow.arel_table[:account_id].eq(account.id))).join_sources).where(Follow.arel_table[:id].eq(nil)).joins(arel_table.join(FollowRequest.arel_table, Arel::Nodes::OuterJoin).on(arel_table[:id].eq(FollowRequest.arel_table[:target_account_id]).and(FollowRequest.arel_table[:account_id].eq(account.id))).join_sources).where(FollowRequest.arel_table[:id].eq(nil)) } diff --git a/spec/controllers/api/v1/directories_controller_spec.rb b/spec/controllers/api/v1/directories_controller_spec.rb index b18aedc4d1..5e21802e7a 100644 --- a/spec/controllers/api/v1/directories_controller_spec.rb +++ b/spec/controllers/api/v1/directories_controller_spec.rb @@ -5,19 +5,124 @@ require 'rails_helper' describe Api::V1::DirectoriesController do render_views - let(:user) { Fabricate(:user) } + let(:user) { Fabricate(:user, confirmed_at: nil) } let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'read:follows') } - let(:account) { Fabricate(:account) } before do allow(controller).to receive(:doorkeeper_token) { token } end describe 'GET #show' do - it 'returns http success' do - get :show + context 'with no params' do + before do + _local_unconfirmed_account = Fabricate( + :account, + domain: nil, + user: Fabricate(:user, confirmed_at: nil, approved: true), + username: 'local_unconfirmed' + ) - expect(response).to have_http_status(200) + local_unapproved_account = Fabricate( + :account, + domain: nil, + user: Fabricate(:user, confirmed_at: 10.days.ago), + username: 'local_unapproved' + ) + local_unapproved_account.user.update(approved: false) + + _local_undiscoverable_account = Fabricate( + :account, + domain: nil, + user: Fabricate(:user, confirmed_at: 10.days.ago, approved: true), + discoverable: false, + username: 'local_undiscoverable' + ) + + excluded_from_timeline_account = Fabricate( + :account, + domain: 'host.example', + discoverable: true, + username: 'remote_excluded_from_timeline' + ) + Fabricate(:block, account: user.account, target_account: excluded_from_timeline_account) + + _domain_blocked_account = Fabricate( + :account, + domain: 'test.example', + discoverable: true, + username: 'remote_domain_blocked' + ) + Fabricate(:account_domain_block, account: user.account, domain: 'test.example') + end + + it 'returns only the local discoverable account' do + local_discoverable_account = Fabricate( + :account, + domain: nil, + user: Fabricate(:user, confirmed_at: 10.days.ago, approved: true), + discoverable: true, + username: 'local_discoverable' + ) + + eligible_remote_account = Fabricate( + :account, + domain: 'host.example', + discoverable: true, + username: 'eligible_remote' + ) + + get :show + + expect(response).to have_http_status(200) + expect(body_as_json.size).to eq(2) + expect(body_as_json.first[:id]).to include(eligible_remote_account.id.to_s) + expect(body_as_json.second[:id]).to include(local_discoverable_account.id.to_s) + end + end + + context 'when asking for local accounts only' do + it 'returns only the local accounts' do + user = Fabricate(:user, confirmed_at: 10.days.ago, approved: true) + local_account = Fabricate(:account, domain: nil, user: user) + remote_account = Fabricate(:account, domain: 'host.example') + + get :show, params: { local: '1' } + + expect(response).to have_http_status(200) + expect(body_as_json.size).to eq(1) + expect(body_as_json.first[:id]).to include(local_account.id.to_s) + expect(response.body).to_not include(remote_account.id.to_s) + end + end + + context 'when ordered by active' do + it 'returns accounts in order of most recent status activity' do + status_old = Fabricate(:status) + travel_to 10.seconds.from_now + status_new = Fabricate(:status) + + get :show, params: { order: 'active' } + + expect(response).to have_http_status(200) + expect(body_as_json.size).to eq(2) + expect(body_as_json.first[:id]).to include(status_new.account.id.to_s) + expect(body_as_json.second[:id]).to include(status_old.account.id.to_s) + end + end + + context 'when ordered by new' do + it 'returns accounts in order of creation' do + account_old = Fabricate(:account) + travel_to 10.seconds.from_now + account_new = Fabricate(:account) + + get :show, params: { order: 'new' } + + expect(response).to have_http_status(200) + expect(body_as_json.size).to eq(2) + expect(body_as_json.first[:id]).to include(account_new.id.to_s) + expect(body_as_json.second[:id]).to include(account_old.id.to_s) + end end end end