diff options
author | Benjamin Neff <benjamin@coding4coffee.ch> | 2016-06-13 03:20:57 +0300 |
---|---|---|
committer | Benjamin Neff <benjamin@coding4coffee.ch> | 2016-06-26 07:21:01 +0300 |
commit | 2476b74dbec83da02cfc88349461b0406e8405a7 (patch) | |
tree | 8d1c17ec4c801c08f8e9f58d287adf5c783f10d5 | |
parent | dee8bc6f0e955d76055bf0ab4b7a07c0fccd8b37 (diff) |
refactoring delete from services
-rw-r--r-- | app/models/service.rb | 4 | ||||
-rw-r--r-- | app/models/services/facebook.rb | 12 | ||||
-rw-r--r-- | app/models/services/tumblr.rb | 16 | ||||
-rw-r--r-- | app/models/services/twitter.rb | 12 | ||||
-rw-r--r-- | app/workers/deferred_retraction.rb | 5 | ||||
-rw-r--r-- | app/workers/delete_post_from_service.rb | 6 | ||||
-rw-r--r-- | lib/diaspora/federated/retraction.rb | 18 | ||||
-rw-r--r-- | lib/diaspora/federation/dispatcher.rb | 21 | ||||
-rw-r--r-- | spec/lib/diaspora/federated/retraction_spec.rb | 49 | ||||
-rw-r--r-- | spec/models/services/facebook_spec.rb | 23 | ||||
-rw-r--r-- | spec/models/services/tumblr_spec.rb | 23 | ||||
-rw-r--r-- | spec/models/services/twitter_spec.rb | 11 | ||||
-rw-r--r-- | spec/shared_behaviors/dispatcher.rb | 16 | ||||
-rw-r--r-- | spec/workers/delete_post_from_service_spec.rb | 19 |
14 files changed, 150 insertions, 85 deletions
diff --git a/app/models/service.rb b/app/models/service.rb index 4e443cf50..57322ce8c 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -12,8 +12,8 @@ class Service < ActiveRecord::Base nil end - def delete_post(post) - #don't do anything (should be overriden by service extensions) + def post_opts(post) + # don't do anything (should be overridden by service extensions) end class << self diff --git a/app/models/services/facebook.rb b/app/models/services/facebook.rb index e15b9ecba..cb6c83aa6 100644 --- a/app/models/services/facebook.rb +++ b/app/models/services/facebook.rb @@ -37,11 +37,13 @@ class Services::Facebook < Service "https://graph.facebook.com/#{self.uid}/picture?type=large&access_token=#{URI.escape(self.access_token)}" end - def delete_post(post) - if post.present? && post.facebook_id.present? - logger.debug "event=delete_from_service type=facebook sender_id=#{user_id} post=#{post.guid}" - delete_from_facebook("https://graph.facebook.com/#{post.facebook_id}/", {:access_token => self.access_token}) - end + def post_opts(post) + {facebook_id: post.facebook_id} if post.facebook_id.present? + end + + def delete_from_service(opts) + logger.debug "event=delete_from_service type=facebook sender_id=#{user_id} facebook_id=#{opts[:facebook_id]}" + delete_from_facebook("https://graph.facebook.com/#{opts[:facebook_id]}/", access_token: access_token) end def delete_from_facebook(url, body) diff --git a/app/models/services/tumblr.rb b/app/models/services/tumblr.rb index 52c7f8d6a..675fe5799 100644 --- a/app/models/services/tumblr.rb +++ b/app/models/services/tumblr.rb @@ -43,13 +43,15 @@ class Services::Tumblr < Service html << "\n\n[original post](#{url})" end - def delete_post(post) - if post.present? && post.tumblr_ids.present? - logger.debug "event=delete_from_service type=tumblr sender_id=#{user_id} post=#{post.guid}" - tumblr_posts = JSON.parse(post.tumblr_ids) - tumblr_posts.each do |blog_name,post_id| - delete_from_tumblr(blog_name, post_id) - end + def post_opts(post) + {tumblr_ids: post.tumblr_ids} if post.tumblr_ids.present? + end + + def delete_from_service(opts) + logger.debug "event=delete_from_service type=tumblr sender_id=#{user_id} tumblr_ids=#{opts[:tumblr_ids]}" + tumblr_posts = JSON.parse(opts[:tumblr_ids]) + tumblr_posts.each do |blog_name, post_id| + delete_from_tumblr(blog_name, post_id) end end diff --git a/app/models/services/twitter.rb b/app/models/services/twitter.rb index 17e51a619..d973a7268 100644 --- a/app/models/services/twitter.rb +++ b/app/models/services/twitter.rb @@ -20,11 +20,13 @@ class Services::Twitter < Service client.user(nickname).profile_image_url_https "original" end - def delete_post post - if post.present? && post.tweet_id.present? - logger.debug "event=delete_from_service type=twitter sender_id=#{user_id} post=#{post.guid}" - delete_from_twitter post.tweet_id - end + def post_opts(post) + {tweet_id: post.tweet_id} if post.tweet_id.present? + end + + def delete_from_service(opts) + logger.debug "event=delete_from_service type=twitter sender_id=#{user_id} tweet_id=#{opts[:tweet_id]}" + delete_from_twitter(opts[:tweet_id]) end private diff --git a/app/workers/deferred_retraction.rb b/app/workers/deferred_retraction.rb index 7c1425f4f..43675f5e8 100644 --- a/app/workers/deferred_retraction.rb +++ b/app/workers/deferred_retraction.rb @@ -6,12 +6,13 @@ module Workers class DeferredRetraction < Base sidekiq_options queue: :dispatch - def perform(user_id, retraction_data, recipient_ids) + def perform(user_id, retraction_data, recipient_ids, opts) user = User.find(user_id) subscribers = Person.where(id: recipient_ids) object = Retraction.new(retraction_data.deep_symbolize_keys, subscribers) + opts = HashWithIndifferentAccess.new(opts) - Diaspora::Federation::Dispatcher.build(user, object).dispatch + Diaspora::Federation::Dispatcher.build(user, object, opts).dispatch end end end diff --git a/app/workers/delete_post_from_service.rb b/app/workers/delete_post_from_service.rb index 9efd3bb18..6ea36c4fc 100644 --- a/app/workers/delete_post_from_service.rb +++ b/app/workers/delete_post_from_service.rb @@ -6,10 +6,10 @@ module Workers class DeletePostFromService < Base sidekiq_options queue: :http_service - def perform(service_id, post_id) + def perform(service_id, opts) service = Service.find_by_id(service_id) - post = Post.find_by_id(post_id) - service.delete_post(post) + opts = HashWithIndifferentAccess.new(opts) + service.delete_from_service(opts) end end end diff --git a/lib/diaspora/federated/retraction.rb b/lib/diaspora/federated/retraction.rb index ac814d3dd..55fcc8668 100644 --- a/lib/diaspora/federated/retraction.rb +++ b/lib/diaspora/federated/retraction.rb @@ -28,7 +28,7 @@ class Retraction end def defer_dispatch(user) - Workers::DeferredRetraction.perform_async(user.id, data, subscribers.map(&:id)) unless subscribers.empty? + Workers::DeferredRetraction.perform_async(user.id, data, subscribers.map(&:id), service_opts(user)) end def perform @@ -41,11 +41,19 @@ class Retraction data[:target][:public] end - def target_type - data[:target_type] - end - private attr_reader :target + + def service_opts(user) + return {} unless target.is_a?(StatusMessage) + + user.services.each_with_object(service_types: []) do |service, opts| + service_opts = service.post_opts(target) + if service_opts + opts.merge!(service_opts) + opts[:service_types] << service.class.to_s + end + end + end end diff --git a/lib/diaspora/federation/dispatcher.rb b/lib/diaspora/federation/dispatcher.rb index 9e2116154..21ec82c3e 100644 --- a/lib/diaspora/federation/dispatcher.rb +++ b/lib/diaspora/federation/dispatcher.rb @@ -31,7 +31,7 @@ module Diaspora attr_reader :sender, :object, :opts def deliver_to_services - deliver_to_user_services + deliver_to_user_services if opts[:service_types] end def deliver_to_subscribers @@ -52,21 +52,16 @@ module Diaspora end def deliver_to_user_services - if object.is_a?(StatusMessage) && opts[:service_types] - post_to_services - elsif object.is_a?(Retraction) && object.target_type == "Post" - delete_from_services + case object + when StatusMessage + each_service {|service| Workers::PostToService.perform_async(service.id, object.id, opts[:url]) } + when Retraction + each_service {|service| Workers::DeletePostFromService.perform_async(service.id, opts) } end end - def post_to_services - sender.services.where(type: opts[:service_types]).each do |service| - Workers::PostToService.perform_async(service.id, object.id, opts[:url]) - end - end - - def delete_from_services - sender.services.each {|service| Workers::DeletePostFromService.perform_async(service.id, object.target.id) } + def each_service + sender.services.where(type: opts[:service_types]).each {|service| yield(service) } end end end diff --git a/spec/lib/diaspora/federated/retraction_spec.rb b/spec/lib/diaspora/federated/retraction_spec.rb index 0ebd48501..b303cdd4d 100644 --- a/spec/lib/diaspora/federated/retraction_spec.rb +++ b/spec/lib/diaspora/federated/retraction_spec.rb @@ -53,21 +53,51 @@ describe Retraction do describe ".defer_dispatch" do it "queues a job to send the retraction later" do post = local_luke.post(:status_message, text: "destroy!", public: true) + federation_retraction = Diaspora::Federation::Entities.signed_retraction(post, local_luke) + + expect(Workers::DeferredRetraction).to receive(:perform_async).with( + local_luke.id, federation_retraction.to_h, [remote_raphael.id], service_types: [] + ) + + Retraction.for(post, local_luke).defer_dispatch(local_luke) + end + + it "adds service metadata to queued job for deletion" do + post.tweet_id = "123" + twitter = Services::Twitter.new(access_token: "twitter") + facebook = Services::Facebook.new(access_token: "facebook") + alice.services << twitter << facebook + federation_retraction = Diaspora::Federation::Entities.signed_retraction(post, alice) - expect(Workers::DeferredRetraction) - .to receive(:perform_async).with(alice.id, federation_retraction.to_h, [remote_raphael.id]) + expect(Workers::DeferredRetraction).to receive(:perform_async).with( + alice.id, federation_retraction.to_h, [], service_types: ["Services::Twitter"], tweet_id: "123" + ) Retraction.for(post, alice).defer_dispatch(alice) end - it "does not queue a job if subscribers is empty" do + it "queues also a job if subscribers is empty" do federation_retraction = Diaspora::Federation::Entities.signed_retraction(post, alice) - expect(Workers::DeferredRetraction).not_to receive(:perform_async).with(alice.id, federation_retraction.to_h, []) + expect(Workers::DeferredRetraction).to receive(:perform_async).with( + alice.id, federation_retraction.to_h, [], service_types: [] + ) Retraction.for(post, alice).defer_dispatch(alice) end + + it "queues a job with empty opts for non-StatusMessage" do + post = local_luke.post(:status_message, text: "hello", public: true) + comment = local_luke.comment!(post, "destroy!") + federation_retraction = Diaspora::Federation::Entities.relayable_retraction(comment, local_luke) + + expect(Workers::DeferredRetraction).to receive(:perform_async).with( + local_luke.id, federation_retraction.to_h, [remote_raphael.id], {} + ) + + Retraction.for(comment, local_luke).defer_dispatch(local_luke) + end end describe "#perform" do @@ -87,15 +117,4 @@ describe Retraction do expect(Retraction.for(private_post, alice).public?).to be_falsey end end - - describe "#target_type" do - it "returns the type of the target as String" do - expect(Retraction.for(post, alice).target_type).to eq("Post") - end - - it "is Person for a Contact-retraction" do - contact = FactoryGirl.create(:contact) - expect(Retraction.for(contact, contact.user).target_type).to eq("Person") - end - end end diff --git a/spec/models/services/facebook_spec.rb b/spec/models/services/facebook_spec.rb index 022a42e24..d82ffeec2 100644 --- a/spec/models/services/facebook_spec.rb +++ b/spec/models/services/facebook_spec.rb @@ -78,14 +78,25 @@ describe Services::Facebook, :type => :model do end end - describe '#delete_post' do - it 'removes a post from facebook' do + describe "#post_opts" do + it "returns the facebook_id of the post" do @post.facebook_id = "2345" - url="https://graph.facebook.com/#{@post.facebook_id}/" - stub_request(:delete, "#{url}?access_token=#{@service.access_token}").to_return(:status => 200) - expect(@service).to receive(:delete_from_facebook).with(url, {access_token: @service.access_token}) + expect(@service.post_opts(@post)).to eq(facebook_id: "2345") + end + + it "returns nil when the post has no facebook_id" do + expect(@service.post_opts(@post)).to be_nil + end + end + + describe "#delete_from_service" do + it "removes a post from facebook" do + facebook_id = "2345" + url = "https://graph.facebook.com/#{facebook_id}/" + stub_request(:delete, "#{url}?access_token=#{@service.access_token}").to_return(status: 200) + expect(@service).to receive(:delete_from_facebook).with(url, access_token: @service.access_token) - @service.delete_post(@post) + @service.delete_from_service(facebook_id: facebook_id) end end end diff --git a/spec/models/services/tumblr_spec.rb b/spec/models/services/tumblr_spec.rb index ea0c7e756..2a8f0d71b 100644 --- a/spec/models/services/tumblr_spec.rb +++ b/spec/models/services/tumblr_spec.rb @@ -25,7 +25,7 @@ describe Services::Tumblr, type: :model do it "posts a status message to the primary blog and stores the id" do stub = stub_request(:post, "http://api.tumblr.com/v2/blog/bar.tumblr.com/post") - .with(post_request).to_return(post_response) + .with(post_request).to_return(post_response) expect(post).to receive(:tumblr_ids=).with({"bar.tumblr.com" => post_id}.to_json) @@ -40,7 +40,7 @@ describe Services::Tumblr, type: :model do it "posts a status message to the returned blog" do stub = stub_request(:post, "http://api.tumblr.com/v2/blog/foo.tumblr.com/post") - .with(post_request).to_return(post_response) + .with(post_request).to_return(post_response) service.post(post) @@ -49,13 +49,24 @@ describe Services::Tumblr, type: :model do end end - describe "#delete_post" do - it "removes posts from tumblr" do + describe "#post_opts" do + it "returns the tumblr_ids of the post" do post.tumblr_ids = {"foodbar.tumblr.com" => post_id}.to_json + expect(service.post_opts(post)).to eq(tumblr_ids: post.tumblr_ids) + end + + it "returns nil when the post has no tumblr_ids" do + expect(service.post_opts(post)).to be_nil + end + end + + describe "#delete_from_service" do + it "removes posts from tumblr" do + tumblr_ids = {"foodbar.tumblr.com" => post_id}.to_json stub = stub_request(:post, "http://api.tumblr.com/v2/blog/foodbar.tumblr.com/post/delete") - .with(body: {"id" => post_id}).to_return(status: 200) + .with(body: {"id" => post_id}).to_return(status: 200) - service.delete_post(post) + service.delete_from_service(tumblr_ids: tumblr_ids) expect(stub).to have_been_requested end diff --git a/spec/models/services/twitter_spec.rb b/spec/models/services/twitter_spec.rb index 982b42afa..720c1ccee 100644 --- a/spec/models/services/twitter_spec.rb +++ b/spec/models/services/twitter_spec.rb @@ -147,4 +147,15 @@ describe Services::Twitter, :type => :model do expect(@service.profile_photo_url).to eq("http://a2.twimg.com/profile_images/uid/avatar.png") end end + + describe "#post_opts" do + it "returns the tweet_id of the post" do + @post.tweet_id = "2345" + expect(@service.post_opts(@post)).to eq(tweet_id: "2345") + end + + it "returns nil when the post has no tweet_id" do + expect(@service.post_opts(@post)).to be_nil + end + end end diff --git a/spec/shared_behaviors/dispatcher.rb b/spec/shared_behaviors/dispatcher.rb index 26c693fff..44f71ba33 100644 --- a/spec/shared_behaviors/dispatcher.rb +++ b/spec/shared_behaviors/dispatcher.rb @@ -2,10 +2,9 @@ shared_examples "a dispatcher" do describe "#dispatch" do context "deliver to user services" do let(:twitter) { Services::Twitter.new(access_token: "twitter") } - let(:facebook) { Services::Facebook.new(access_token: "facebook") } before do - alice.services << twitter << facebook + alice.services << twitter end it "delivers a StatusMessage to specified services" do @@ -14,11 +13,18 @@ shared_examples "a dispatcher" do Diaspora::Federation::Dispatcher.build(alice, post, opts).dispatch end - it "delivers a Retraction of a Post to all user services" do - skip # TODO + it "delivers a Retraction of a Post to specified services" do + opts = {service_types: "Services::Twitter", tweet_id: "123"} + expect(Workers::DeletePostFromService).to receive(:perform_async).with(twitter.id, opts) retraction = Retraction.for(post, alice) - Diaspora::Federation::Dispatcher.build(alice, retraction).dispatch + Diaspora::Federation::Dispatcher.build(alice, retraction, opts).dispatch + end + + it "does not queue service jobs when no services specified" do + opts = {url: "https://example.org/p/123"} + expect(Workers::PostToService).not_to receive(:perform_async) + Diaspora::Federation::Dispatcher.build(alice, post, opts).dispatch end it "does not deliver a Comment to services" do diff --git a/spec/workers/delete_post_from_service_spec.rb b/spec/workers/delete_post_from_service_spec.rb index 75e902034..2830ca7cd 100644 --- a/spec/workers/delete_post_from_service_spec.rb +++ b/spec/workers/delete_post_from_service_spec.rb @@ -1,16 +1,13 @@ -require 'spec_helper' +require "spec_helper" describe Workers::DeletePostFromService do - before do - @user = alice - @post = @user.post(:status_message, :text => "hello", :to =>@user.aspects.first.id, :public =>true, :facebook_id => "23456" ) - end + it "calls service#delete_from_service with given opts" do + service = double + opts = {facebook_id: "23456"} + + expect(service).to receive(:delete_from_service).with(opts) + allow(Service).to receive(:find_by_id).with("123").and_return(service) - it 'calls service#delete_post with given service' do - m = double() - url = "foobar" - expect(m).to receive(:delete_post) - allow(Service).to receive(:find_by_id).and_return(m) - Workers::DeletePostFromService.new.perform("123", @post.id.to_s) + Workers::DeletePostFromService.new.perform("123", opts) end end |