From 581f8d7226e79734f594d13452cd5861aecb5311 Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Mon, 16 May 2016 19:57:30 +0200
Subject: [PATCH] don't force remove contact on block

this creates inconsistent states, if you remove the block in the future
---
 app/controllers/blocks_controller.rb       |  4 +---
 app/models/user/connecting.rb              | 21 ++++++++++-----------
 spec/controllers/blocks_controller_spec.rb |  2 +-
 spec/integration/receiving_spec.rb         |  8 +++++---
 4 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/app/controllers/blocks_controller.rb b/app/controllers/blocks_controller.rb
index bcea0e561..dd5cb4ae9 100644
--- a/app/controllers/blocks_controller.rb
+++ b/app/controllers/blocks_controller.rb
@@ -35,9 +35,7 @@ class BlocksController < ApplicationController
   private
 
   def disconnect_if_contact(person)
-    if contact = current_user.contact_for(person)
-      current_user.disconnect(contact, :force => true)
-    end
+    current_user.contact_for(person).try {|contact| current_user.disconnect(contact) }
   end
 
   def block_params
diff --git a/app/models/user/connecting.rb b/app/models/user/connecting.rb
index 238b06199..c163054e1 100644
--- a/app/models/user/connecting.rb
+++ b/app/models/user/connecting.rb
@@ -27,29 +27,28 @@ class User
       contact
     end
 
-    def disconnect(contact, opts={force: false})
+    def disconnect(contact)
       logger.info "event=disconnect user=#{diaspora_handle} target=#{contact.person.diaspora_handle}"
 
       # TODO: send retraction
 
       contact.aspect_memberships.delete_all
 
-      if !contact.sharing || opts[:force]
-        contact.destroy
-      else
-        contact.update_attributes(receiving: false)
-      end
+      disconnect_contact(contact, direction: :receiving, destroy: !contact.sharing)
     end
 
     def disconnected_by(person)
       logger.info "event=disconnected_by user=#{diaspora_handle} target=#{person.diaspora_handle}"
-      contact = contact_for(person)
-      return unless contact
+      contact_for(person).try {|contact| disconnect_contact(contact, direction: :sharing, destroy: !contact.receiving) }
+    end
 
-      if contact.receiving
-        contact.update_attributes(sharing: false)
-      else
+    private
+
+    def disconnect_contact(contact, direction:, destroy:)
+      if destroy
         contact.destroy
+      else
+        contact.update_attributes(direction => false)
       end
     end
   end
diff --git a/spec/controllers/blocks_controller_spec.rb b/spec/controllers/blocks_controller_spec.rb
index fe733cf8d..1937c1e2d 100644
--- a/spec/controllers/blocks_controller_spec.rb
+++ b/spec/controllers/blocks_controller_spec.rb
@@ -55,7 +55,7 @@ describe BlocksController, :type => :controller do
     it "calls disconnect with the force option if there is a contact for a given user" do
       contact = alice.contact_for(bob.person)
       allow(alice).to receive(:contact_for).and_return(contact)
-      expect(alice).to receive(:disconnect).with(contact, hash_including(:force => true))
+      expect(alice).to receive(:disconnect).with(contact)
       @controller.send(:disconnect_if_contact, bob.person)
     end
 
diff --git a/spec/integration/receiving_spec.rb b/spec/integration/receiving_spec.rb
index f927c35cf..c07a6d509 100644
--- a/spec/integration/receiving_spec.rb
+++ b/spec/integration/receiving_spec.rb
@@ -45,9 +45,11 @@ describe 'a user receives a post', :type => :request do
     end
 
     it "does not remove visibility on disconnect" do
-      alice.disconnect(alice.contact_for(bob.person), force: true)
-      alice.reload
-      expect(ShareVisibility.find_by(user_id: alice.id, shareable_id: @status_message.id)).not_to be_nil
+      contact = alice.contact_for(bob.person)
+      alice.disconnect(contact)
+      contact.destroy
+
+      expect(ShareVisibility.exists?(user_id: alice.id, shareable_id: @status_message.id)).to be_truthy
     end
   end
 end