commit: 2cc3111a7775066c34eb407cd3b4707acc659488
parent: bf811e4d4a8626579fc5187f465cdf1e79a32e10
Author: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Wed, 31 May 2017 14:28:45 -0400
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
Diffstat:
4 files changed, 152 insertions(+), 69 deletions(-)
diff --git 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
@@ -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
@@ -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
@@ -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
