From f2fff6be669d6fcf66a8bd5f46f9db3e3492bc37 Mon Sep 17 00:00:00 2001 From: Claire Date: Fri, 6 Oct 2023 12:58:16 +0200 Subject: [PATCH] =?UTF-8?q?Fix=20crash=20when=20filtering=20for=20?= =?UTF-8?q?=E2=80=9Cdormant=E2=80=9D=20relationships=20(#27306)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/models/account.rb | 4 +- app/models/relationship_filter.rb | 2 +- spec/fabricators/account_stat_fabricator.rb | 10 +-- spec/models/relationship_filter_spec.rb | 68 +++++++++++++++------ 4 files changed, 57 insertions(+), 27 deletions(-) diff --git a/app/models/account.rb b/app/models/account.rb index 28bd828b06..2fbed78012 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -114,8 +114,8 @@ class Account < ApplicationRecord 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)) } - scope :by_recent_status, -> { order(Arel.sql('(case when account_stats.last_status_at is null then 1 else 0 end) asc, account_stats.last_status_at desc, accounts.id desc')) } - scope :by_recent_sign_in, -> { order(Arel.sql('(case when users.current_sign_in_at is null then 1 else 0 end) asc, users.current_sign_in_at desc, accounts.id desc')) } + scope :by_recent_status, -> { includes(:account_stat).merge(AccountStat.order('last_status_at DESC NULLS LAST')).references(:account_stat) } + scope :by_recent_sign_in, -> { order(Arel.sql('users.current_sign_in_at DESC NULLS LAST')) } scope :popular, -> { order('account_stats.followers_count desc') } scope :by_domain_and_subdomains, ->(domain) { where(domain: domain).or(where(arel_table[:domain].matches("%.#{domain}"))) } scope :not_excluded_by_account, ->(account) { where.not(id: account.excluded_from_timeline_account_ids) } diff --git a/app/models/relationship_filter.rb b/app/models/relationship_filter.rb index dae4e87b7d..8e069c80a7 100644 --- a/app/models/relationship_filter.rb +++ b/app/models/relationship_filter.rb @@ -112,7 +112,7 @@ class RelationshipFilter def activity_scope(value) case value when 'dormant' - AccountStat.where(last_status_at: nil).or(AccountStat.where(AccountStat.arel_table[:last_status_at].lt(1.month.ago))) + Account.joins(:account_stat).where(account_stat: { last_status_at: [nil, ...1.month.ago] }) else raise Mastodon::InvalidParameterError, "Unknown activity: #{value}" end diff --git a/spec/fabricators/account_stat_fabricator.rb b/spec/fabricators/account_stat_fabricator.rb index 2b06b47909..20272fb22f 100644 --- a/spec/fabricators/account_stat_fabricator.rb +++ b/spec/fabricators/account_stat_fabricator.rb @@ -1,6 +1,8 @@ +# frozen_string_literal: true + Fabricator(:account_stat) do - account nil - statuses_count "" - following_count "" - followers_count "" + account { Fabricate.build(:account) } + statuses_count '123' + following_count '456' + followers_count '789' end diff --git a/spec/models/relationship_filter_spec.rb b/spec/models/relationship_filter_spec.rb index 7c0f37a06f..fccd42aaad 100644 --- a/spec/models/relationship_filter_spec.rb +++ b/spec/models/relationship_filter_spec.rb @@ -6,32 +6,60 @@ describe RelationshipFilter do let(:account) { Fabricate(:account) } describe '#results' do - context 'when default params are used' do - let(:subject) do - RelationshipFilter.new(account, 'order' => 'active').results + let(:account_of_7_months) { Fabricate(:account_stat, statuses_count: 1, last_status_at: 7.months.ago).account } + let(:account_of_1_day) { Fabricate(:account_stat, statuses_count: 1, last_status_at: 1.day.ago).account } + let(:account_of_3_days) { Fabricate(:account_stat, statuses_count: 1, last_status_at: 3.days.ago).account } + let(:silent_account) { Fabricate(:account_stat, statuses_count: 0, last_status_at: nil).account } + + before do + account.follow!(account_of_7_months) + account.follow!(account_of_1_day) + account.follow!(account_of_3_days) + account.follow!(silent_account) + end + + context 'when ordering by last activity' do + context 'when not filtering' do + subject do + described_class.new(account, 'order' => 'active').results + end + + it 'returns followings ordered by last activity' do + expect(subject).to eq [account_of_1_day, account_of_3_days, account_of_7_months, silent_account] + end end - before do - add_following_account_with(last_status_at: 7.days.ago) - add_following_account_with(last_status_at: 1.day.ago) - add_following_account_with(last_status_at: 3.days.ago) + context 'when filtering for dormant accounts' do + subject do + described_class.new(account, 'order' => 'active', 'activity' => 'dormant').results + end + + it 'returns dormant followings ordered by last activity' do + expect(subject).to eq [account_of_7_months, silent_account] + end + end + end + + context 'when ordering by account creation' do + context 'when not filtering' do + subject do + described_class.new(account, 'order' => 'recent').results + end + + it 'returns followings ordered by last account creation' do + expect(subject).to eq [silent_account, account_of_3_days, account_of_1_day, account_of_7_months] + end end - it 'returns followings ordered by last activity' do - expected_result = account.following.eager_load(:account_stat).reorder(nil).by_recent_status + context 'when filtering for dormant accounts' do + subject do + described_class.new(account, 'order' => 'recent', 'activity' => 'dormant').results + end - expect(subject).to eq expected_result + it 'returns dormant followings ordered by last activity' do + expect(subject).to eq [silent_account, account_of_7_months] + end end end end - - def add_following_account_with(last_status_at:) - following_account = Fabricate(:account) - Fabricate(:account_stat, account: following_account, - last_status_at: last_status_at, - statuses_count: 1, - following_count: 0, - followers_count: 0) - Fabricate(:follow, account: account, target_account: following_account).account - end end