From 11ff92c9d7b27c2c9ed86f649aef8d956cc8b989 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 28 Feb 2016 21:22:56 +0100 Subject: [PATCH] Adding a test for ReblogService, fixing mentions for remote statuses --- Gemfile | 1 + Gemfile.lock | 9 ++++++ app/helpers/atom_helper.rb | 6 ++-- app/models/account.rb | 3 +- app/models/mention.rb | 2 +- app/services/process_feed_service.rb | 36 +++++++++++++++++----- app/services/process_mentions_service.rb | 14 +++++---- app/views/profile/_status_footer.html.haml | 8 ----- spec/rails_helper.rb | 2 ++ spec/services/reblog_service_spec.rb | 27 +++++++++++++++- 10 files changed, 82 insertions(+), 26 deletions(-) diff --git a/Gemfile b/Gemfile index ee817a0c4..b3a5b7e6d 100644 --- a/Gemfile +++ b/Gemfile @@ -40,6 +40,7 @@ end group :test do gem 'simplecov', require: false + gem 'webmock' end group :development do diff --git a/Gemfile.lock b/Gemfile.lock index c83a5abb1..ae68368da 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -65,6 +65,8 @@ GEM execjs coffee-script-source (1.10.0) concurrent-ruby (1.0.0) + crack (0.4.3) + safe_yaml (~> 1.0.0) debug_inspector (0.0.2) descendants_tracker (0.0.4) thread_safe (~> 0.3, >= 0.3.1) @@ -116,6 +118,7 @@ GEM haml (>= 4.0.6, < 5.0) html2haml (>= 1.0.1) railties (>= 4.0.1) + hashdiff (0.3.0) hashie (3.4.3) hashie-forbidden_attributes (0.1.1) hashie (>= 3.0) @@ -255,6 +258,7 @@ GEM ruby-progressbar (1.7.5) ruby_parser (3.8.1) sexp_processor (~> 4.1) + safe_yaml (1.0.4) sass (3.4.21) sass-rails (5.0.4) railties (>= 4.0.0, < 5.0) @@ -306,6 +310,10 @@ GEM binding_of_caller (>= 0.7.2) railties (>= 4.0) sprockets-rails (>= 2.0, < 4.0) + webmock (1.24.1) + addressable (>= 2.3.6) + crack (>= 0.3.2) + hashdiff PLATFORMS ruby @@ -349,6 +357,7 @@ DEPENDENCIES therubyracer uglifier (>= 1.3.0) web-console (~> 2.0) + webmock BUNDLED WITH 1.11.2 diff --git a/app/helpers/atom_helper.rb b/app/helpers/atom_helper.rb index 0cba8e964..bd7aee9a0 100644 --- a/app/helpers/atom_helper.rb +++ b/app/helpers/atom_helper.rb @@ -1,6 +1,6 @@ module AtomHelper def stream_updated_at - @account.stream_entries.last ? @account.stream_entries.last.created_at : @account.updated_at + @account.stream_entries.last ? (@account.updated_at > @account.stream_entries.last.created_at ? @account.updated_at : @account.stream_entries.last.created_at) : @account.updated_at end def entry(xml, is_root, &block) @@ -126,7 +126,9 @@ module AtomHelper end def link_avatar(xml, account) - xml.link(rel: 'avatar', type: account.avatar_content_type, href: asset_url(account.avatar.url(:large))) + xml.link('rel' => 'avatar', 'type' => account.avatar_content_type, 'media:width' => '300', 'media:height' =>'300', 'href' => asset_url(account.avatar.url(:large))) + xml.link('rel' => 'avatar', 'type' => account.avatar_content_type, 'media:width' => '96', 'media:height' =>'96', 'href' => asset_url(account.avatar.url(:medium))) + xml.link('rel' => 'avatar', 'type' => account.avatar_content_type, 'media:width' => '48', 'media:height' =>'48', 'href' => asset_url(account.avatar.url(:small))) end def logo(xml, url) diff --git a/app/models/account.rb b/app/models/account.rb index e9fc03a10..6cb638b3e 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -11,6 +11,7 @@ class Account < ActiveRecord::Base has_many :stream_entries, inverse_of: :account has_many :statuses, inverse_of: :account has_many :favourites, inverse_of: :account + has_many :mentions, inverse_of: :account # Follow relations has_many :active_relationships, class_name: 'Follow', foreign_key: 'account_id', dependent: :destroy @@ -77,7 +78,7 @@ class Account < ActiveRecord::Base before_create do if local? - keypair = OpenSSL::PKey::RSA.new(Rails.env.test? ? 48 : 2048) + keypair = OpenSSL::PKey::RSA.new(Rails.env.test? ? 1024 : 2048) self.private_key = keypair.to_pem self.public_key = keypair.public_key.to_pem end diff --git a/app/models/mention.rb b/app/models/mention.rb index 52f40e4b2..9fefa657a 100644 --- a/app/models/mention.rb +++ b/app/models/mention.rb @@ -1,6 +1,6 @@ class Mention < ActiveRecord::Base belongs_to :account, inverse_of: :mentions - belongs_to :status, inverse_of: :mentions + belongs_to :status validates :account, :status, presence: true validates :account, uniqueness: { scope: :status } diff --git a/app/services/process_feed_service.rb b/app/services/process_feed_service.rb index 146869552..f6584bbda 100644 --- a/app/services/process_feed_service.rb +++ b/app/services/process_feed_service.rb @@ -5,10 +5,12 @@ class ProcessFeedService < BaseService def call(body, account) xml = Nokogiri::XML(body) + # If we got a full feed, make sure the account's profile is up to date unless xml.at_xpath('/xmlns:feed').nil? update_remote_profile_service.(xml.at_xpath('/xmlns:feed/xmlns:author'), account) end + # Process entries xml.xpath('//xmlns:entry').each do |entry| next unless [:note, :comment, :activity].include? object_type(entry) @@ -16,7 +18,7 @@ class ProcessFeedService < BaseService next unless status.nil? - status = Status.new(uri: activity_id(entry), account: account, text: content(entry), created_at: published(entry), updated_at: updated(entry)) + status = Status.new(uri: activity_id(entry), url: activity_link(entry), account: account, text: content(entry), created_at: published(entry), updated_at: updated(entry)) if object_type(entry) == :comment add_reply!(entry, status) @@ -26,7 +28,25 @@ class ProcessFeedService < BaseService add_post!(entry, status) end - process_mentions_service.(status) unless status.new_record? + # If we added a status, go through accounts it mentions and create respective relations + unless status.new_record? + entry.xpath('./xmlns:link[@rel="mentioned"]').each do |mention_link| + # Here we have to do a reverse lookup of local accounts by their URL! + # It's not pretty at all! I really wish all these protocols sticked to + # using acct:username@domain only! It would make things so much easier + # and tidier + + href = Addressable::URI.parse(mention_link.attribute('href').value) + + if href.host == LOCAL_DOMAIN + mentioned_account = Account.find_by(username: href.path.gsub('/users/', ''), domain: nil) + + unless mentioned_account.nil? + mentioned_account.mentions.first_or_create(status: status) + end + end + end + end end end @@ -103,12 +123,18 @@ class ProcessFeedService < BaseService xml.at_xpath('./xmlns:id').content end + def activity_link(xml) + xml.at_xpath('./xmlns:link[@rel="alternate"]').attribute('href').value + rescue + '' + end + def target_content(xml) xml.at_xpath('.//activity:object/xmlns:content').content end def target_url(xml) - xml.at_xpath('.//activity:object/xmlns:link[@rel=alternate]').attribute('href').value + xml.at_xpath('.//activity:object/xmlns:link[@rel="alternate"]').attribute('href').value end def object_type(xml) @@ -127,10 +153,6 @@ class ProcessFeedService < BaseService @follow_remote_account_service ||= FollowRemoteAccountService.new end - def process_mentions_service - @process_mentions_service ||= ProcessMentionsService.new - end - def update_remote_profile_service @update_remote_profile_service ||= UpdateRemoteProfileService.new end diff --git a/app/services/process_mentions_service.rb b/app/services/process_mentions_service.rb index 93866666a..e1fc2f038 100644 --- a/app/services/process_mentions_service.rb +++ b/app/services/process_mentions_service.rb @@ -4,15 +4,17 @@ class ProcessMentionsService < BaseService # remote users # @param [Status] status def call(status) - status.text.scan(Account::MENTION_RE).each do |match| - username, domain = match.first.split('@') - local_account = Account.find_by(username: username, domain: domain) + return unless status.local? - if local_account.nil? - local_account = follow_remote_account_service.("acct:#{match.first}") + status.text.scan(Account::MENTION_RE).each do |match| + username, domain = match.first.split('@') + mentioned_account = Account.find_by(username: username, domain: domain) + + if mentioned_account.nil? + mentioned_account = follow_remote_account_service.("acct:#{match.first}") end - local_account.mentions.first_or_create(status: status) + mentioned_account.mentions.first_or_create(status: status) end status.mentions.each do |mentioned_account| diff --git a/app/views/profile/_status_footer.html.haml b/app/views/profile/_status_footer.html.haml index 59197f26a..a2333df15 100644 --- a/app/views/profile/_status_footer.html.haml +++ b/app/views/profile/_status_footer.html.haml @@ -1,10 +1,2 @@ -.counter.counter-reblogs - %i.fa.fa-retweet - %span.num= status.reblogs.count - -.counter.counter-favourites - %i.fa.fa-star - %span.num= status.favourites.count - - if status.reply? = link_to "In response to #{status.thread.account.acct}", status_url(status.thread), class: 'conversation-link' diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 0a04d560c..cb8088061 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -5,8 +5,10 @@ abort("The Rails environment is running in production mode!") if Rails.env.produ require 'spec_helper' require 'rspec/rails' +require 'webmock/rspec' ActiveRecord::Migration.maintain_test_schema! +WebMock.disable_net_connect! RSpec.configure do |config| config.fixture_path = "#{::Rails.root}/spec/fixtures" diff --git a/spec/services/reblog_service_spec.rb b/spec/services/reblog_service_spec.rb index 4e94d5a70..f12fdb837 100644 --- a/spec/services/reblog_service_spec.rb +++ b/spec/services/reblog_service_spec.rb @@ -1,5 +1,30 @@ require 'rails_helper' RSpec.describe ReblogService do - pending + let(:alice) { Fabricate(:account, username: 'alice') } + let(:bob) { Fabricate(:account, username: 'bob', domain: 'example.com', salmon_url: 'http://salmon.example.com') } + let(:status) { Fabricate(:status, account: bob, uri: 'tag:example.com;something:something') } + + subject { ReblogService.new } + + before do + stub_const('HUB_URL', 'http://hub.example.com') + + stub_request(:post, 'http://hub.example.com') + stub_request(:post, 'http://salmon.example.com') + + subject.(alice, status) + end + + it 'creates a reblog' do + expect(status.reblogs.count).to eq 1 + end + + it 'pings PubSubHubbub hubs' do + expect(a_request(:post, 'http://hub.example.com')).to have_been_made + end + + it 'sends a Salmon slap for a remote reblog' do + expect(a_request(:post, 'http://salmon.example.com')).to have_been_made + end end