From e07b57852e951efebae7e2380a3691236420072d Mon Sep 17 00:00:00 2001 From: "Akihiko Odaki (@fn_aki@pawoo.net)" Date: Sun, 4 Jun 2017 23:14:25 +0900 Subject: [PATCH] Remove some code in TagManager and spec (#3547) * Do not fall back to StreamEntry if object_type is unavailable in TagManager Since 6d6a429af8fe4bd92ed497f401676353fdc603e0, when Status, the only model with stream_entry, and StreamEntry got its own logic in uri_for and url_for, the purpose of the fallbacks to activity_type of StreamEntry became unclear. This commit removes the fallbacks. When adding another model with stream_entry in future, consider to update uri_for and url_for. * Cover TagManager more --- app/lib/tag_manager.rb | 4 - spec/lib/tag_manager_spec.rb | 221 ++++++++++++++++++++++++++++++----- 2 files changed, 193 insertions(+), 32 deletions(-) diff --git a/app/lib/tag_manager.rb b/app/lib/tag_manager.rb index 55aed92e3ac..f1a2234dc44 100644 --- a/app/lib/tag_manager.rb +++ b/app/lib/tag_manager.rb @@ -93,8 +93,6 @@ class TagManager account_url(target) when :note, :comment, :activity unique_tag(target.created_at, target.id, 'Status') - else - unique_tag(target.stream_entry.created_at, target.stream_entry.activity_id, target.stream_entry.activity_type) end end @@ -106,8 +104,6 @@ class TagManager short_account_url(target) when :note, :comment, :activity short_account_status_url(target.account, target) - else - account_stream_entry_url(target.account, target.stream_entry) end end end diff --git a/spec/lib/tag_manager_spec.rb b/spec/lib/tag_manager_spec.rb index cbb427a8c17..1fae6bec458 100644 --- a/spec/lib/tag_manager_spec.rb +++ b/spec/lib/tag_manager_spec.rb @@ -1,23 +1,152 @@ require 'rails_helper' RSpec.describe TagManager do - let(:local_domain) { Rails.configuration.x.local_domain } + describe '#local_domain?' do + # The following comparisons MUST be case-insensitive. + + around do |example| + original_local_domain = Rails.configuration.x.local_domain + Rails.configuration.x.local_domain = 'domain' + + example.run + + Rails.configuration.x.local_domain = original_local_domain + end + + it 'returns true for nil' do + expect(TagManager.instance.local_domain?(nil)).to eq true + end + + it 'returns true if the slash-stripped string equals to local domain' do + expect(TagManager.instance.local_domain?('DoMaIn/')).to eq true + end + + it 'returns false for irrelevant string' do + expect(TagManager.instance.local_domain?('DoMaIn!')).to eq false + end + end + + describe '#web_domain?' do + # The following comparisons MUST be case-insensitive. + + around do |example| + original_web_domain = Rails.configuration.x.web_domain + Rails.configuration.x.web_domain = 'domain' + + example.run + + Rails.configuration.x.web_domain = original_web_domain + end + + it 'returns true for nil' do + expect(TagManager.instance.web_domain?(nil)).to eq true + end + + it 'returns true if the slash-stripped string equals to web domain' do + expect(TagManager.instance.web_domain?('DoMaIn/')).to eq true + end + + it 'returns false for string with irrelevant characters' do + expect(TagManager.instance.web_domain?('DoMaIn!')).to eq false + end + end + + describe '#normalize_domain' do + it 'returns nil if the given parameter is nil' do + expect(TagManager.instance.normalize_domain(nil)).to eq nil + end + + it 'returns normalized domain' do + expect(TagManager.instance.normalize_domain('DoMaIn/')).to eq 'domain' + end + end + + describe '#local_url?' do + around do |example| + original_local_domain = Rails.configuration.x.local_domain + example.run + Rails.configuration.x.local_domain = original_local_domain + end + + it 'returns true if the normalized string with port is local URL' do + Rails.configuration.x.local_domain = 'domain:42' + expect(TagManager.instance.local_url?('https://DoMaIn:42/')).to eq true + end + + it 'returns true if the normalized string without port is local URL' do + Rails.configuration.x.local_domain = 'domain' + expect(TagManager.instance.local_url?('https://DoMaIn/')).to eq true + end + + it 'returns false for string with irrelevant characters' do + Rails.configuration.x.local_domain = 'domain' + expect(TagManager.instance.local_url?('https://domainn/')).to eq false + end + end + + describe '#same_acct?' do + # The following comparisons MUST be case-insensitive. + + it 'returns true if the needle has a correct username and domain for remote user' do + expect(TagManager.instance.same_acct?('username@domain', 'UsErNaMe@DoMaIn')).to eq true + end + + it 'returns false if the needle is missing a domain for remote user' do + expect(TagManager.instance.same_acct?('username@domain', 'UsErNaMe')).to eq false + end + + it 'returns false if the needle has an incorrect domain for remote user' do + expect(TagManager.instance.same_acct?('username@domain', 'UsErNaMe@incorrect')).to eq false + end + + it 'returns false if the needle has an incorrect username for remote user' do + expect(TagManager.instance.same_acct?('username@domain', 'incorrect@DoMaIn')).to eq false + end + + it 'returns true if the needle has a correct username and domain for local user' do + expect(TagManager.instance.same_acct?('username', 'UsErNaMe@Cb6E6126.nGrOk.Io')).to eq true + end + + it 'returns true if the needle is missing a domain for local user' do + expect(TagManager.instance.same_acct?('username', 'UsErNaMe')).to eq true + end + + it 'returns false if the needle has an incorrect username for local user' do + expect(TagManager.instance.same_acct?('username', 'UsErNaM@Cb6E6126.nGrOk.Io')).to eq false + end + + it 'returns false if the needle has an incorrect domain for local user' do + expect(TagManager.instance.same_acct?('username', 'incorrect@Cb6E6126.nGrOk.Io')).to eq false + end + end describe '#unique_tag' do - it 'returns a string' do - expect(TagManager.instance.unique_tag(Time.now, 12, 'Status')).to be_a String + it 'returns a unique tag' do + expect(TagManager.instance.unique_tag(Time.utc(2000), 12, 'Status')).to eq 'tag:cb6e6126.ngrok.io,2000-01-01:objectId=12:objectType=Status' end end describe '#unique_tag_to_local_id' do it 'returns the ID part' do - expect(TagManager.instance.unique_tag_to_local_id("tag:#{local_domain};objectId=12:objectType=Status", 'Status')).to eql '12' + expect(TagManager.instance.unique_tag_to_local_id('tag:cb6e6126.ngrok.io,2000-01-01:objectId=12:objectType=Status', 'Status')).to eql '12' + end + + it 'returns nil if it is not local id' do + expect(TagManager.instance.unique_tag_to_local_id('tag:remote,2000-01-01:objectId=12:objectType=Status', 'Status')).to eq nil + end + + it 'returns nil if it is not expected type' do + expect(TagManager.instance.unique_tag_to_local_id('tag:cb6e6126.ngrok.io,2000-01-01:objectId=12:objectType=Block', 'Status')).to eq nil + end + + it 'returns nil if it does not have object ID' do + expect(TagManager.instance.unique_tag_to_local_id('tag:cb6e6126.ngrok.io,2000-01-01:objectType=Status', 'Status')).to eq nil end end describe '#local_id?' do it 'returns true for a local ID' do - expect(TagManager.instance.local_id?("tag:#{local_domain};objectId=12:objectType=Status")).to be true + expect(TagManager.instance.local_id?('tag:cb6e6126.ngrok.io;objectId=12:objectType=Status')).to be true end it 'returns false for a foreign ID' do @@ -26,49 +155,85 @@ RSpec.describe TagManager do end describe '#uri_for' do - let(:alice) { Fabricate(:account, username: 'alice') } - let(:bob) { Fabricate(:account, username: 'bob') } - let(:status) { Fabricate(:status, text: 'Hello world', account: alice) } - subject { TagManager.instance.uri_for(target) } - context 'Account' do - let(:target) { alice } + context 'activity object' do + let(:target) { Fabricate(:status, reblog: Fabricate(:status)).stream_entry } - it 'returns a string' do - expect(subject).to be_a String + before { target.update!(created_at: '2000-01-01T00:00:00Z') } + + it 'returns the unique tag for status' do + expect(target.object_type).to eq :activity + is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" end end - context 'Status' do - let(:target) { status } + context 'comment object' do + let(:target) { Fabricate(:status, created_at: '2000-01-01T00:00:00Z', reply: true) } - it 'returns a string' do - expect(subject).to be_a String + it 'returns the unique tag for status' do + expect(target.object_type).to eq :comment + is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" + end + end + + context 'note object' do + let(:target) { Fabricate(:status, created_at: '2000-01-01T00:00:00Z', reply: false, thread: nil) } + + it 'returns the unique tag for status' do + expect(target.object_type).to eq :note + is_expected.to eq "tag:cb6e6126.ngrok.io,2000-01-01:objectId=#{target.id}:objectType=Status" + end + end + + context 'person object' do + let(:target) { Fabricate(:account, username: 'alice') } + + it 'returns the URL for account' do + expect(target.object_type).to eq :person + is_expected.to eq 'https://cb6e6126.ngrok.io/users/alice' end end end describe '#url_for' do - let(:alice) { Fabricate(:account, username: 'alice') } - let(:bob) { Fabricate(:account, username: 'bob') } - let(:status) { Fabricate(:status, text: 'Hello world', account: alice) } + let(:alice) { Fabricate(:account, username: 'alice') } subject { TagManager.instance.url_for(target) } - context 'Account' do - let(:target) { alice } + context 'activity object' do + let(:target) { Fabricate(:status, account: alice, reblog: Fabricate(:status)).stream_entry } - it 'returns a URL' do - expect(subject).to be_a String + it 'returns the unique tag for status' do + expect(target.object_type).to eq :activity + is_expected.to eq "https://cb6e6126.ngrok.io/@alice/#{target.id}" end end - context 'Status' do - let(:target) { status } + context 'comment object' do + let(:target) { Fabricate(:status, account: alice, reply: true) } - it 'returns a URL' do - expect(subject).to be_a String + it 'returns the unique tag for status' do + expect(target.object_type).to eq :comment + is_expected.to eq "https://cb6e6126.ngrok.io/@alice/#{target.id}" + end + end + + context 'note object' do + let(:target) { Fabricate(:status, account: alice, reply: false, thread: nil) } + + it 'returns the unique tag for status' do + expect(target.object_type).to eq :note + is_expected.to eq "https://cb6e6126.ngrok.io/@alice/#{target.id}" + end + end + + context 'person object' do + let(:target) { alice } + + it 'returns the URL for account' do + expect(target.object_type).to eq :person + is_expected.to eq 'https://cb6e6126.ngrok.io/@alice' end end end