From 2cc3111a7775066c34eb407cd3b4707acc659488 Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Wed, 31 May 2017 14:28:45 -0400 Subject: [PATCH] Expand spec coverage and refactor the `Account.find_` methods (#3485) * Move specs for account finder methods to concern spec * Move account finder methods to concern * Improve spec wording * Use more explicit comparison to ensure correct return value * Add coverage for .find_local! and .find_remote! * Add some methods to the finder * Use arel on matching_username method * Avoid ternary in matching domain method * Simplify finder methods * Use an AccountFinder class to simplify lookup --- app/models/account.rb | 22 +---- app/models/concerns/account_finder_concern.rb | 58 ++++++++++++ spec/models/account_spec.rb | 48 ---------- .../concerns/account_finder_concern_spec.rb | 93 +++++++++++++++++++ 4 files changed, 152 insertions(+), 69 deletions(-) create mode 100644 app/models/concerns/account_finder_concern.rb create mode 100644 spec/models/concerns/account_finder_concern_spec.rb diff --git a/app/models/account.rb b/app/models/account.rb index cb116dbaf6..4561fd0a48 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -42,6 +42,7 @@ class Account < ApplicationRecord MENTION_RE = /(?:^|[^\/[:word:]])@([a-z0-9_]+(?:@[a-z0-9\.\-]+[a-z0-9]+)?)/i include AccountAvatar + include AccountFinderConcern include AccountHeader include AccountInteractions include Attachmentable @@ -162,27 +163,6 @@ class Account < ApplicationRecord end class << self - def find_local!(username) - find_remote!(username, nil) - end - - def find_remote!(username, domain) - raise ActiveRecord::RecordNotFound if username.blank? - where('lower(accounts.username) = ?', username.downcase).where(domain.nil? ? { domain: nil } : 'lower(accounts.domain) = ?', domain&.downcase).take! - end - - def find_local(username) - find_local!(username) - rescue ActiveRecord::RecordNotFound - nil - end - - def find_remote(username, domain) - find_remote!(username, domain) - rescue ActiveRecord::RecordNotFound - nil - end - def triadic_closures(account, limit: 5, offset: 0) sql = <<-SQL.squish WITH first_degree AS ( diff --git a/app/models/concerns/account_finder_concern.rb b/app/models/concerns/account_finder_concern.rb new file mode 100644 index 0000000000..d3ad519b12 --- /dev/null +++ b/app/models/concerns/account_finder_concern.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +module AccountFinderConcern + extend ActiveSupport::Concern + + class_methods do + def find_local!(username) + find_local(username) || raise(ActiveRecord::RecordNotFound) + end + + def find_remote!(username, domain) + find_remote(username, domain) || raise(ActiveRecord::RecordNotFound) + end + + def find_local(username) + find_remote(username, nil) + end + + def find_remote(username, domain) + AccountFinder.new(username, domain).account + end + end + + class AccountFinder + attr_reader :username, :domain + + def initialize(username, domain) + @username = username + @domain = domain + end + + def account + scoped_accounts.take + end + + private + + def scoped_accounts + Account.unscoped.tap do |scope| + scope.merge! matching_username + scope.merge! matching_domain + end + end + + def matching_username + raise(ActiveRecord::RecordNotFound) if username.blank? + Account.where(Account.arel_table[:username].lower.eq username.downcase) + end + + def matching_domain + if domain.nil? + Account.where(domain: nil) + else + Account.where(Account.arel_table[:domain].lower.eq domain.downcase) + end + end + end +end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 08098854b1..ebec463e12 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -301,54 +301,6 @@ RSpec.describe Account, type: :model do end end - describe '.find_local' do - before do - Fabricate(:account, username: 'Alice') - end - - it 'returns Alice for alice' do - expect(Account.find_local('alice')).to_not be_nil - end - - it 'returns Alice for Alice' do - expect(Account.find_local('Alice')).to_not be_nil - end - - it 'does not return anything for a_ice' do - expect(Account.find_local('a_ice')).to be_nil - end - - it 'does not return anything for al%' do - expect(Account.find_local('al%')).to be_nil - end - end - - describe '.find_remote' do - before do - Fabricate(:account, username: 'Alice', domain: 'mastodon.social') - end - - it 'returns Alice for alice@mastodon.social' do - expect(Account.find_remote('alice', 'mastodon.social')).to_not be_nil - end - - it 'returns Alice for ALICE@MASTODON.SOCIAL' do - expect(Account.find_remote('ALICE', 'MASTODON.SOCIAL')).to_not be_nil - end - - it 'does not return anything for a_ice@mastodon.social' do - expect(Account.find_remote('a_ice', 'mastodon.social')).to be_nil - end - - it 'does not return anything for alice@m_stodon.social' do - expect(Account.find_remote('alice', 'm_stodon.social')).to be_nil - end - - it 'does not return anything for alice@m%' do - expect(Account.find_remote('alice', 'm%')).to be_nil - end - end - describe '.following_map' do it 'returns an hash' do expect(Account.following_map([], 1)).to be_a Hash diff --git a/spec/models/concerns/account_finder_concern_spec.rb b/spec/models/concerns/account_finder_concern_spec.rb new file mode 100644 index 0000000000..05f0f44f27 --- /dev/null +++ b/spec/models/concerns/account_finder_concern_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe AccountFinderConcern do + describe 'local finders' do + before do + @account = Fabricate(:account, username: 'Alice') + end + + describe '.find_local' do + it 'returns case-insensitive result' do + expect(Account.find_local('alice')).to eq(@account) + end + + it 'returns correctly cased result' do + expect(Account.find_local('Alice')).to eq(@account) + end + + it 'returns nil without a match' do + expect(Account.find_local('a_ice')).to be_nil + end + + it 'returns nil for regex style username value' do + expect(Account.find_local('al%')).to be_nil + end + end + + describe '.find_local!' do + it 'returns matching result' do + expect(Account.find_local!('alice')).to eq(@account) + end + + it 'raises on non-matching result' do + expect { Account.find_local!('missing') }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises with blank username' do + expect { Account.find_local!('') }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises with nil username' do + expect { Account.find_local!(nil) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end + + describe 'remote finders' do + before do + @account = Fabricate(:account, username: 'Alice', domain: 'mastodon.social') + end + + describe '.find_remote' do + it 'returns exact match result' do + expect(Account.find_remote('alice', 'mastodon.social')).to eq(@account) + end + + it 'returns case-insensitive result' do + expect(Account.find_remote('ALICE', 'MASTODON.SOCIAL')).to eq(@account) + end + + it 'returns nil when username does not match' do + expect(Account.find_remote('a_ice', 'mastodon.social')).to be_nil + end + + it 'returns nil when domain does not match' do + expect(Account.find_remote('alice', 'm_stodon.social')).to be_nil + end + + it 'returns nil for regex style domain value' do + expect(Account.find_remote('alice', 'm%')).to be_nil + end + end + + describe '.find_remote!' do + it 'returns matching result' do + expect(Account.find_remote!('alice', 'mastodon.social')).to eq(@account) + end + + it 'raises on non-matching result' do + expect { Account.find_remote!('missing', 'mastodon.host') }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises with blank username' do + expect { Account.find_remote!('', '') }.to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises with nil username' do + expect { Account.find_remote!(nil, nil) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + end +end