From e27f792c24d1040246bc6ae890e6052f763cdb84 Mon Sep 17 00:00:00 2001 From: "Akihiko Odaki (@fn_aki@pawoo.net)" Date: Fri, 23 Jun 2017 01:34:27 +0900 Subject: [PATCH] Some minor change and spec for Account (#3813) * Introduce domains method to Account relation Account had followers_domains method, which was excessively specific. Let relation of Account have domains method instead. * Move follow_mapping in Account to AccountInteractions * Introduce shared examples for AccountAvatar inclusion * Cover Account more --- app/models/account.rb | 12 +- app/models/concerns/account_interactions.rb | 6 + .../pubsubhubbub/distribution_worker.rb | 2 +- spec/models/account_spec.rb | 417 +++++++++++++++--- .../models/concerns/account_avatar.rb | 19 + 5 files changed, 380 insertions(+), 76 deletions(-) create mode 100644 spec/support/examples/models/concerns/account_avatar.rb diff --git a/app/models/account.rb b/app/models/account.rb index 72bad51a2..2b54cee5f 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -124,10 +124,6 @@ class Account < ApplicationRecord subscription_expires_at.present? end - def followers_domains - followers.reorder(nil).pluck('distinct accounts.domain') - end - def keypair OpenSSL::PKey::RSA.new(private_key || public_key) end @@ -163,6 +159,10 @@ class Account < ApplicationRecord end class << self + def domains + reorder(nil).pluck('distinct accounts.domain') + end + def triadic_closures(account, limit: 5, offset: 0) sql = <<-SQL.squish WITH first_degree AS ( @@ -236,10 +236,6 @@ class Account < ApplicationRecord [textsearch, query] end - - def follow_mapping(query, field) - query.pluck(field).each_with_object({}) { |id, mapping| mapping[id] = true } - end end before_create :generate_keys diff --git a/app/models/concerns/account_interactions.rb b/app/models/concerns/account_interactions.rb index de5979958..9ffed2910 100644 --- a/app/models/concerns/account_interactions.rb +++ b/app/models/concerns/account_interactions.rb @@ -29,6 +29,12 @@ module AccountInteractions blocked_domains = AccountDomainBlock.where(account_id: account_id, domain: accounts_map.values).pluck(:domain) accounts_map.map { |id, domain| [id, blocked_domains.include?(domain)] }.to_h end + + private + + def follow_mapping(query, field) + query.pluck(field).each_with_object({}) { |id, mapping| mapping[id] = true } + end end included do diff --git a/app/workers/pubsubhubbub/distribution_worker.rb b/app/workers/pubsubhubbub/distribution_worker.rb index da01dcd91..b41cec90d 100644 --- a/app/workers/pubsubhubbub/distribution_worker.rb +++ b/app/workers/pubsubhubbub/distribution_worker.rb @@ -33,7 +33,7 @@ class Pubsubhubbub::DistributionWorker return if stream_entries.empty? @payload = AtomSerializer.render(AtomSerializer.new.feed(@account, stream_entries)) - @domains = @account.followers_domains + @domains = @account.followers.domains Pubsubhubbub::DeliveryWorker.push_bulk(@subscriptions.reject { |s| !allowed_to_receive?(s.callback_url) }) do |subscription| [subscription.id, @payload] diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 73ae34f8e..eeaebb779 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -1,10 +1,9 @@ require 'rails_helper' RSpec.describe Account, type: :model do - subject { Fabricate(:account, username: 'alice') } - context do let(:bob) { Fabricate(:account, username: 'bob') } + subject { Fabricate(:account) } describe '#follow!' do it 'creates a follow' do @@ -45,12 +44,13 @@ RSpec.describe Account, type: :model do describe '#local?' do it 'returns true when the account is local' do - expect(subject.local?).to be true + account = Fabricate(:account, domain: nil) + expect(account.local?).to be true end it 'returns false when the account is on a different domain' do - subject.domain = 'foreign.tld' - expect(subject.local?).to be false + account = Fabricate(:account, domain: 'foreign.tld') + expect(account.local?).to be false end end @@ -61,6 +61,8 @@ RSpec.describe Account, type: :model do Rails.configuration.x.local_domain = before end + subject { Fabricate(:account, domain: nil, username: 'alice') } + describe '#to_webfinger_s' do it 'returns a webfinger string for the account' do Rails.configuration.x.local_domain = 'example.com' @@ -80,41 +82,72 @@ RSpec.describe Account, type: :model do describe '#acct' do it 'returns username for local users' do - expect(subject.acct).to eql 'alice' + account = Fabricate(:account, domain: nil, username: 'alice') + expect(account.acct).to eql 'alice' end it 'returns username@domain for foreign users' do - subject.domain = 'foreign.tld' - expect(subject.acct).to eql 'alice@foreign.tld' + account = Fabricate(:account, domain: 'foreign.tld', username: 'alice') + expect(account.acct).to eql 'alice@foreign.tld' + end + end + + describe '#save_with_optional_media!' do + it 'sets default avatar, header, avatar_remote_url, and header_remote_url if some of them are invalid' do + stub_request(:get, 'https://remote/valid_avatar').to_return(request_fixture('avatar.txt')) + stub_request(:get, 'https://remote/invalid_avatar').to_return(request_fixture('feed.txt')) + account = Fabricate(:account, + avatar_remote_url: 'https://remote/valid_avatar', + header_remote_url: 'https://remote/valid_avatar') + + account.avatar_remote_url = 'https://remote/invalid_avatar' + account.save_with_optional_media! + + account.reload + expect(account.avatar_remote_url).to eq '' + expect(account.header_remote_url).to eq '' + expect(account.avatar_file_name).to eq nil + expect(account.header_file_name).to eq nil end end describe '#subscribed?' do it 'returns false when no subscription expiration information is present' do - expect(subject.subscribed?).to be false + account = Fabricate(:account, subscription_expires_at: nil) + expect(account.subscribed?).to be false end it 'returns true when subscription expiration has been set' do - subject.subscription_expires_at = 30.days.from_now - expect(subject.subscribed?).to be true + account = Fabricate(:account, subscription_expires_at: 30.days.from_now) + expect(account.subscribed?).to be true + end + end + + describe '#to_param' do + it 'returns username' do + account = Fabricate(:account, username: 'alice') + expect(account.to_param).to eq 'alice' end end describe '#keypair' do it 'returns an RSA key pair' do - expect(subject.keypair).to be_instance_of OpenSSL::PKey::RSA + account = Fabricate(:account) + expect(account.keypair).to be_instance_of OpenSSL::PKey::RSA end end describe '#subscription' do it 'returns an OStatus subscription' do - expect(subject.subscription('')).to be_instance_of OStatus2::Subscription + account = Fabricate(:account) + expect(account.subscription('')).to be_instance_of OStatus2::Subscription end end describe '#object_type' do it 'is always a person' do - expect(subject.object_type).to be :person + account = Fabricate(:account) + expect(account.object_type).to be :person end end @@ -124,6 +157,8 @@ RSpec.describe Account, type: :model do Fabricate(:status, account: author) end + subject { Fabricate(:account) } + context 'when the status is a reblog of another status' do let(:original_reblog) do author = Fabricate(:account, username: 'original_reblogger') @@ -160,6 +195,8 @@ RSpec.describe Account, type: :model do Fabricate(:status, account: author) end + subject { Fabricate(:account) } + context 'when the status is a reblog of another status'do let(:original_reblog) do author = Fabricate(:account, username: 'original_reblogger') @@ -205,14 +242,16 @@ RSpec.describe Account, type: :model do end end + describe '#excluded_from_timeline_domains' do + it 'returns the domains blocked by the account' do + account = Fabricate(:account) + account.block_domain!('domain') + expect(account.excluded_from_timeline_domains).to match_array ['domain'] + end + end + describe '.search_for' do before do - @match = Fabricate( - :account, - display_name: "Display Name", - username: "username", - domain: "example.com" - ) _missing = Fabricate( :account, display_name: "Missing", @@ -221,33 +260,103 @@ RSpec.describe Account, type: :model do ) end + it 'accepts ?, \, : and space as delimiter' do + match = Fabricate( + :account, + display_name: 'A & l & i & c & e', + username: 'username', + domain: 'example.com' + ) + + results = Account.search_for('A?l\i:c e') + expect(results).to eq [match] + end + it 'finds accounts with matching display_name' do + match = Fabricate( + :account, + display_name: "Display Name", + username: "username", + domain: "example.com" + ) + results = Account.search_for("display") - expect(results).to eq [@match] + expect(results).to eq [match] end it 'finds accounts with matching username' do + match = Fabricate( + :account, + display_name: "Display Name", + username: "username", + domain: "example.com" + ) + results = Account.search_for("username") - expect(results).to eq [@match] + expect(results).to eq [match] end it 'finds accounts with matching domain' do + match = Fabricate( + :account, + display_name: "Display Name", + username: "username", + domain: "example.com" + ) + results = Account.search_for("example") - expect(results).to eq [@match] + expect(results).to eq [match] + end + + it 'limits by 10 by default' do + 11.times.each { Fabricate(:account, display_name: "Display Name") } + results = Account.search_for("display") + expect(results.size).to eq 10 + end + + it 'accepts arbitrary limits' do + 2.times.each { Fabricate(:account, display_name: "Display Name") } + results = Account.search_for("display", 1) + expect(results.size).to eq 1 end it 'ranks multiple matches higher' do - account = Fabricate( - :account, - username: "username", - display_name: "username" - ) + matches = [ + { username: "username", display_name: "username" }, + { display_name: "Display Name", username: "username", domain: "example.com" }, + ].map(&method(:Fabricate).curry(2).call(:account)) + results = Account.search_for("username") - expect(results).to eq [account, @match] + expect(results).to eq matches end end describe '.advanced_search_for' do + it 'accepts ?, \, : and space as delimiter' do + account = Fabricate(:account) + match = Fabricate( + :account, + display_name: 'A & l & i & c & e', + username: 'username', + domain: 'example.com' + ) + + results = Account.advanced_search_for('A?l\i:c e', account) + expect(results).to eq [match] + end + + it 'limits by 10 by default' do + 11.times { Fabricate(:account, display_name: "Display Name") } + results = Account.search_for("display") + expect(results.size).to eq 10 + end + + it 'accepts arbitrary limits' do + 2.times { Fabricate(:account, display_name: "Display Name") } + results = Account.search_for("display", 1) + expect(results.size).to eq 1 + end + it 'ranks followed accounts higher' do account = Fabricate(:account) match = Fabricate(:account, username: "Matching") @@ -260,9 +369,14 @@ RSpec.describe Account, type: :model do end end - describe '.triadic_closures' do - subject { described_class.triadic_closures(me) } + describe '.domains' do + it 'returns domains' do + Fabricate(:account, domain: 'domain') + expect(Account.domains).to match_array(['domain']) + end + end + describe '.triadic_closures' do let!(:me) { Fabricate(:account) } let!(:friend) { Fabricate(:account) } let!(:friends_friend) { Fabricate(:account) } @@ -277,7 +391,39 @@ RSpec.describe Account, type: :model do end it 'finds accounts you dont follow which are followed by accounts you do follow' do - is_expected.to eq [friends_friend] + expect(described_class.triadic_closures(me)).to eq [friends_friend] + end + + it 'limits by 5 with offset 0 by defualt' do + first_degree = 6.times.map { Fabricate(:account) } + matches = 5.times.map { Fabricate(:account) } + first_degree.each { |account| me.follow!(account) } + matches.each do |match| + first_degree.each { |account| account.follow!(match) } + first_degree.shift + end + + expect(described_class.triadic_closures(me)).to eq matches + end + + it 'accepts arbitrary limits' do + another_friend = Fabricate(:account) + higher_friends_friend = Fabricate(:account) + me.follow!(another_friend) + friend.follow!(higher_friends_friend) + another_friend.follow!(higher_friends_friend) + + expect(described_class.triadic_closures(me, limit: 1)).to eq [higher_friends_friend] + end + + it 'acceps arbitrary offset' do + another_friend = Fabricate(:account) + higher_friends_friend = Fabricate(:account) + me.follow!(another_friend) + friend.follow!(higher_friends_friend) + another_friend.follow!(higher_friends_friend) + + expect(described_class.triadic_closures(me, offset: 1)).to eq [friends_friend] end context 'when you block account' do @@ -286,7 +432,7 @@ RSpec.describe Account, type: :model do end it 'rejects blocked accounts' do - is_expected.to be_empty + expect(described_class.triadic_closures(me)).to be_empty end end @@ -296,7 +442,7 @@ RSpec.describe Account, type: :model do end it 'rejects muted accounts' do - is_expected.to be_empty + expect(described_class.triadic_closures(me)).to be_empty end end end @@ -374,26 +520,26 @@ RSpec.describe Account, type: :model do expect(account).to model_have_error_on_field(:username) end - it 'is invalid if the username already exists' do - account_1 = Fabricate(:account, username: 'the_doctor') - account_2 = Fabricate.build(:account, username: 'the_doctor') - account_2.valid? - expect(account_2).to model_have_error_on_field(:username) - end - - it 'is invalid if the username is reserved' do - account = Fabricate.build(:account, username: 'support') - account.valid? - expect(account).to model_have_error_on_field(:username) - end - - it 'is valid when username is reserved but record has already been created' do - account = Fabricate.build(:account, username: 'support') - account.save(validate: false) - expect(account.valid?).to be true - end - context 'when is local' do + it 'is invalid if the username is not unique in case-insensitive comparsion among local accounts' do + account_1 = Fabricate(:account, username: 'the_doctor') + account_2 = Fabricate.build(:account, username: 'the_Doctor') + account_2.valid? + expect(account_2).to model_have_error_on_field(:username) + end + + it 'is invalid if the username is reserved' do + account = Fabricate.build(:account, username: 'support') + account.valid? + expect(account).to model_have_error_on_field(:username) + end + + it 'is valid when username is reserved but record has already been created' do + account = Fabricate.build(:account, username: 'support') + account.save(validate: false) + expect(account.valid?).to be true + end + it 'is invalid if the username doesn\'t only contains letters, numbers and underscores' do account = Fabricate.build(:account, username: 'the-doctor') account.valid? @@ -405,10 +551,109 @@ RSpec.describe Account, type: :model do account.valid? expect(account).to model_have_error_on_field(:username) end + + it 'is invalid if the display name is longer than 30 characters' do + account = Fabricate.build(:account, display_name: Faker::Lorem.characters(31)) + account.valid? + expect(account).to model_have_error_on_field(:display_name) + end + + it 'is invalid if the note is longer than 160 characters' do + account = Fabricate.build(:account, note: Faker::Lorem.characters(161)) + account.valid? + expect(account).to model_have_error_on_field(:note) + end + end + + context 'when is remote' do + it 'is invalid if the username is not unique in case-sensitive comparison among accounts in the same normalized domain' do + Fabricate(:account, domain: 'にゃん', username: 'username') + account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'username') + account.valid? + expect(account).to model_have_error_on_field(:username) + end + + it 'is valid even if the username is unique only in case-sensitive comparison among accounts in the same normalized domain' do + Fabricate(:account, domain: 'にゃん', username: 'username') + account = Fabricate.build(:account, domain: 'xn--r9j5b5b', username: 'Username') + account.valid? + expect(account).not_to model_have_error_on_field(:username) + end + + it 'is valid even if the username doesn\'t only contains letters, numbers and underscores' do + account = Fabricate.build(:account, domain: 'domain', username: 'the-doctor') + account.valid? + expect(account).not_to model_have_error_on_field(:username) + end + + it 'is valid even if the username is longer then 30 characters' do + account = Fabricate.build(:account, domain: 'domain', username: Faker::Lorem.characters(31)) + account.valid? + expect(account).not_to model_have_error_on_field(:username) + end + + it 'is valid even if the display name is longer than 30 characters' do + account = Fabricate.build(:account, domain: 'domain', display_name: Faker::Lorem.characters(31)) + account.valid? + expect(account).not_to model_have_error_on_field(:display_name) + end + + it 'is valid even if the note is longer than 160 characters' do + account = Fabricate.build(:account, domain: 'domain', note: Faker::Lorem.characters(161)) + account.valid? + expect(account).not_to model_have_error_on_field(:note) + end end end describe 'scopes' do + describe 'alphabetic' do + it 'sorts by alphabetic order of domain and username' do + matches = [ + { username: 'a', domain: 'a' }, + { username: 'b', domain: 'a' }, + { username: 'a', domain: 'b' }, + { username: 'b', domain: 'b' }, + ].map(&method(:Fabricate).curry(2).call(:account)) + + expect(Account.alphabetic).to eq matches + end + end + + describe 'matches_display_name' do + it 'matches display name which starts with the given string' do + match = Fabricate(:account, display_name: 'pattern and suffix') + Fabricate(:account, display_name: 'prefix and pattern') + + expect(Account.matches_display_name('pattern')).to eq [match] + end + end + + describe 'matches_username' do + it 'matches display name which starts with the given string' do + match = Fabricate(:account, username: 'pattern_and_suffix') + Fabricate(:account, username: 'prefix_and_pattern') + + expect(Account.matches_username('pattern')).to eq [match] + end + end + + describe 'expiring' do + it 'returns remote accounts with followers whose subscription expiration date is past or not given' do + local = Fabricate(:account, domain: nil) + matches = [ + { domain: 'remote', subscription_expires_at: nil }, + { domain: 'remote', subscription_expires_at: '2000-01-01T00:00:00Z' }, + ].map(&method(:Fabricate).curry(2).call(:account)) + matches.each(&local.method(:follow!)) + Fabricate(:account, domain: 'remote', subscription_expires_at: nil) + local.follow!(Fabricate(:account, domain: 'remote', subscription_expires_at: '2000-01-03T00:00:00Z')) + local.follow!(Fabricate(:account, domain: nil, subscription_expires_at: nil)) + + expect(Account.expiring('2000-01-02T00:00:00Z').recent).to eq matches.reverse + end + end + describe 'remote' do it 'returns an array of accounts who have a domain' do account_1 = Fabricate(:account, domain: nil) @@ -439,6 +684,24 @@ RSpec.describe Account, type: :model do end end + describe 'partitioned' do + it 'returns a relation of accounts partitioned by domain' do + matches = ['a', 'b', 'a', 'b'] + matches.size.times.to_a.shuffle.each do |index| + matches[index] = Fabricate(:account, domain: matches[index]) + end + + expect(Account.partitioned).to match_array(matches) + end + end + + describe 'recent' do + it 'returns a relation of accounts sorted by recent creation' do + matches = 2.times.map { Fabricate(:account) } + expect(Account.recent).to match_array(matches) + end + end + describe 'silenced' do it 'returns an array of accounts who are silenced' do account_1 = Fabricate(:account, silenced: true) @@ -454,25 +717,45 @@ RSpec.describe Account, type: :model do expect(Account.suspended).to match_array([account_1]) end end - end - describe 'static avatars' do - describe 'when GIF' do - it 'creates a png static style' do - subject.avatar = attachment_fixture('avatar.gif') - subject.save - - expect(subject.avatar_static_url).to_not eq subject.avatar_original_url + describe 'without_followers' do + it 'returns a relation of accounts without followers' do + account_1 = Fabricate(:account) + account_2 = Fabricate(:account) + Fabricate(:follow, account: account_1, target_account: account_2) + expect(Account.without_followers).to match_array([account_1]) end end - describe 'when non-GIF' do - it 'does not create extra static style' do - subject.avatar = attachment_fixture('attachment.jpg') - subject.save - - expect(subject.avatar_static_url).to eq subject.avatar_original_url + describe 'with_followers' do + it 'returns a relation of accounts with followers' do + account_1 = Fabricate(:account) + account_2 = Fabricate(:account) + Fabricate(:follow, account: account_1, target_account: account_2) + expect(Account.with_followers).to match_array([account_2]) end end end + + context 'when is local' do + it 'generates keys' do + account = Account.create!(domain: nil, username: Faker::Internet.user_name(nil, ['_'])) + expect(account.keypair.private?).to eq true + end + end + + context 'when is remote' do + it 'does not generate keys' do + key = OpenSSL::PKey::RSA.new(1024).public_key + account = Account.create!(domain: 'remote', username: Faker::Internet.user_name(nil, ['_']), public_key: key.to_pem) + expect(account.keypair.params).to eq key.params + end + + it 'normalizes domain' do + account = Account.create!(domain: 'にゃん', username: Faker::Internet.user_name(nil, ['_'])) + expect(account.domain).to eq 'xn--r9j5b5b' + end + end + + include_examples 'AccountAvatar', :account end diff --git a/spec/support/examples/models/concerns/account_avatar.rb b/spec/support/examples/models/concerns/account_avatar.rb new file mode 100644 index 000000000..f2a8a2459 --- /dev/null +++ b/spec/support/examples/models/concerns/account_avatar.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +shared_examples 'AccountAvatar' do |fabricator| + describe 'static avatars' do + describe 'when GIF' do + it 'creates a png static style' do + account = Fabricate(fabricator, avatar: attachment_fixture('avatar.gif')) + expect(account.avatar_static_url).to_not eq account.avatar_original_url + end + end + + describe 'when non-GIF' do + it 'does not create extra static style' do + account = Fabricate(fabricator, avatar: attachment_fixture('attachment.jpg')) + expect(account.avatar_static_url).to eq account.avatar_original_url + end + end + end +end