From 285f63c02ed34c5faa99c08713fa77fbf8ed4318 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Mon, 8 Apr 2024 09:53:49 -0400 Subject: [PATCH] Use composable query in `User.active` scope (#29775) --- app/lib/feed_manager.rb | 2 +- app/lib/vacuum/feeds_vacuum.rb | 2 +- app/models/concerns/account/interactions.rb | 4 +- app/models/user.rb | 6 ++- spec/models/user_spec.rb | 42 ++++++++++++++++++--- 5 files changed, 45 insertions(+), 11 deletions(-) diff --git a/app/lib/feed_manager.rb b/app/lib/feed_manager.rb index bf6e6db59..9ddc54c16 100644 --- a/app/lib/feed_manager.rb +++ b/app/lib/feed_manager.rb @@ -18,7 +18,7 @@ class FeedManager # @yield [Account] # @return [void] def with_active_accounts(&block) - Account.joins(:user).where('users.current_sign_in_at > ?', User::ACTIVE_DURATION.ago).find_each(&block) + Account.joins(:user).merge(User.signed_in_recently).find_each(&block) end # Redis key of a feed diff --git a/app/lib/vacuum/feeds_vacuum.rb b/app/lib/vacuum/feeds_vacuum.rb index fb0b8a847..429215760 100644 --- a/app/lib/vacuum/feeds_vacuum.rb +++ b/app/lib/vacuum/feeds_vacuum.rb @@ -21,7 +21,7 @@ class Vacuum::FeedsVacuum end def inactive_users - User.confirmed.inactive + User.confirmed.not_signed_in_recently end def inactive_users_lists diff --git a/app/models/concerns/account/interactions.rb b/app/models/concerns/account/interactions.rb index a32697b66..536afba17 100644 --- a/app/models/concerns/account/interactions.rb +++ b/app/models/concerns/account/interactions.rb @@ -255,13 +255,13 @@ module Account::Interactions def followers_for_local_distribution followers.local .joins(:user) - .where('users.current_sign_in_at > ?', User::ACTIVE_DURATION.ago) + .merge(User.signed_in_recently) end def lists_for_local_distribution scope = lists.joins(account: :user) scope.where.not(list_accounts: { follow_id: nil }).or(scope.where(account_id: id)) - .where('users.current_sign_in_at > ?', User::ACTIVE_DURATION.ago) + .merge(User.signed_in_recently) end def remote_followers_hash(url) diff --git a/app/models/user.rb b/app/models/user.rb index c62a6d0de..584120cf2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -109,14 +109,16 @@ class User < ApplicationRecord validates :confirm_password, absence: true, on: :create validate :validate_role_elevation + scope :account_not_suspended, -> { joins(:account).merge(Account.without_suspended) } scope :recent, -> { order(id: :desc) } scope :pending, -> { where(approved: false) } scope :approved, -> { where(approved: true) } scope :confirmed, -> { where.not(confirmed_at: nil) } scope :enabled, -> { where(disabled: false) } scope :disabled, -> { where(disabled: true) } - scope :inactive, -> { where(arel_table[:current_sign_in_at].lt(ACTIVE_DURATION.ago)) } - scope :active, -> { confirmed.where(arel_table[:current_sign_in_at].gteq(ACTIVE_DURATION.ago)).joins(:account).where(accounts: { suspended_at: nil }) } + scope :active, -> { confirmed.signed_in_recently.account_not_suspended } + scope :signed_in_recently, -> { where(current_sign_in_at: ACTIVE_DURATION.ago..) } + scope :not_signed_in_recently, -> { where(current_sign_in_at: ...ACTIVE_DURATION.ago) } scope :matches_email, ->(value) { where(arel_table[:email].matches("#{value}%")) } scope :matches_ip, ->(value) { left_joins(:ips).where('user_ips.ip <<= ?', value).group('users.id') } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2a0726306..714d595dc 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -107,12 +107,36 @@ RSpec.describe User do end end - describe 'inactive' do - it 'returns a relation of inactive users' do - specified = Fabricate(:user, current_sign_in_at: 15.days.ago) - Fabricate(:user, current_sign_in_at: 6.days.ago) + describe 'signed_in_recently' do + it 'returns a relation of users who have signed in during the recent period' do + recent_sign_in_user = Fabricate(:user, current_sign_in_at: within_duration_window_days.ago) + Fabricate(:user, current_sign_in_at: exceed_duration_window_days.ago) - expect(described_class.inactive).to contain_exactly(specified) + expect(described_class.signed_in_recently) + .to contain_exactly(recent_sign_in_user) + end + end + + describe 'not_signed_in_recently' do + it 'returns a relation of users who have not signed in during the recent period' do + no_recent_sign_in_user = Fabricate(:user, current_sign_in_at: exceed_duration_window_days.ago) + Fabricate(:user, current_sign_in_at: within_duration_window_days.ago) + + expect(described_class.not_signed_in_recently) + .to contain_exactly(no_recent_sign_in_user) + end + end + + describe 'account_not_suspended' do + it 'returns with linked accounts that are not suspended' do + suspended_account = Fabricate(:account, suspended_at: 10.days.ago) + non_suspended_account = Fabricate(:account, suspended_at: nil) + suspended_user = Fabricate(:user, account: suspended_account) + non_suspended_user = Fabricate(:user, account: non_suspended_account) + + expect(described_class.account_not_suspended) + .to include(non_suspended_user) + .and not_include(suspended_user) end end @@ -137,6 +161,14 @@ RSpec.describe User do expect(described_class.matches_ip('2160:2160::/32')).to contain_exactly(user1) end end + + def exceed_duration_window_days + described_class::ACTIVE_DURATION + 2.days + end + + def within_duration_window_days + described_class::ACTIVE_DURATION - 2.days + end end describe 'blacklist' do