From 2476b74dbec83da02cfc88349461b0406e8405a7 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Mon, 13 Jun 2016 02:20:57 +0200
Subject: [PATCH] refactoring delete from services

---
 app/models/service.rb                         |  4 +-
 app/models/services/facebook.rb               | 12 +++--
 app/models/services/tumblr.rb                 | 16 +++---
 app/models/services/twitter.rb                | 12 +++--
 app/workers/deferred_retraction.rb            |  5 +-
 app/workers/delete_post_from_service.rb       |  6 +--
 lib/diaspora/federated/retraction.rb          | 18 +++++--
 lib/diaspora/federation/dispatcher.rb         | 21 +++-----
 .../lib/diaspora/federated/retraction_spec.rb | 49 +++++++++++++------
 spec/models/services/facebook_spec.rb         | 23 ++++++---
 spec/models/services/tumblr_spec.rb           | 25 +++++++---
 spec/models/services/twitter_spec.rb          | 11 +++++
 spec/shared_behaviors/dispatcher.rb           | 16 ++++--
 spec/workers/delete_post_from_service_spec.rb | 19 +++----
 14 files changed, 151 insertions(+), 86 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
 
-      @service.delete_post(@post)
+    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_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
-      stub = stub_request(:post, "http://api.tumblr.com/v2/blog/foodbar.tumblr.com/post/delete")
-        .with(body: {"id" => post_id}).to_return(status: 200)
+      expect(service.post_opts(post)).to eq(tumblr_ids: post.tumblr_ids)
+    end
 
-      service.delete_post(post)
+    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)
+
+      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"}
 
-  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)
+    expect(service).to receive(:delete_from_service).with(opts)
+    allow(Service).to receive(:find_by_id).with("123").and_return(service)
+
+    Workers::DeletePostFromService.new.perform("123", opts)
   end
 end