Refactor the merge-request tracking branches deletion to be in an aft…

…er_destroy hook

Pass along all the data needed for the processor in the message body when the
after_destroy hook is called. That way users don't get confused and delete it
multiple times (since there's a small delay in the msg processing

app/controllers/merge_requests_controller.rb

@@ -207,7 +207,7 @@ def update
   end
 
   def destroy
-    @merge_request.soft_delete
+    @merge_request.destroy
     flash[:success] = I18n.t "merge_requests_controller.destroy_success"
     redirect_to [@owner, @repository]
   end

app/models/merge_request.rb

@@ -31,7 +31,7 @@ class MergeRequest < ActiveRecord::Base
   has_many :versions, :class_name => 'MergeRequestVersion', :order => 'version'
 
   before_destroy :nullify_messages
-
+  after_destroy  :delete_tracking_branches
 
   is_indexed :fields => ["proposal", {:field => "status_tag", :as => "status"}],
     :include => [{
@@ -497,18 +497,25 @@ def push_new_branch_to_tracking_repo
       user, "new version #{current_version_number}")
   end
 
-  def delete_target_repository_ref
-    source_repository.git.git.push({},target_repository.full_repository_path, ":#{merge_branch_name}")
-  end
-
-  # Since we'll be deleting the ref in the backend, this will be handled in the message queue
-  def soft_delete
-    msg = {:merge_request_id => to_param, :action => "delete"}
+  # Since we'll be deleting the ref in the backend, this will be
+  # handled in the message queue
+  def delete_tracking_branches
+    msg = {
+      :merge_request_id => to_param,
+      :action => "delete",
+      :target_path => target_repository.full_repository_path,
+      :target_name => target_repository.url_path,
+      :merge_branch_name => merge_branch_name,
+      :source_repository_id => source_repository.id,
+      :target_repository_id => target_repository.id,
+    }
     publish :merge_request_backend_updates, msg.to_json
   end
 
   def tracking_repository
-    target_repository.create_tracking_repository unless target_repository.has_tracking_repository?
+    unless target_repository.has_tracking_repository?
+      target_repository.create_tracking_repository
+    end
     target_repository.tracking_repository
   end
 

app/processors/merge_request_git_backend_processor.rb

@@ -28,18 +28,28 @@ def action
     @body["action"].to_sym
   end
 
-  def merge_request
-    @merge_request ||= MergeRequest.find(@body["merge_request_id"])
+  def source_repository
+    @source_repository ||= Repository.find(@body["source_repository_id"])
+  end
+
+  def target_repository
+    @target_repository ||= Repository.find(@body["target_repository_id"])
+  end
+
+  def delete_target_repository_ref
+    source_repository.git.git.push({:timeout => false},
+      target_repository.full_repository_path,
+      ":#{@body['merge_branch_name']}")
   end
 
   private
   def do_delete
-    logger.info("Deleting tracking branch #{merge_request.merge_branch_name} for merge request in target repository #{merge_request.target_repository.id}")
+    logger.info("Deleting tracking branch #{@body['merge_branch_name']} for merge request " +
+      "in target repository #{@body['target_name']}")
     begin
-      merge_request.delete_target_repository_ref
+      delete_target_repository_ref
     rescue Grit::NoSuchPathError => e
       logger.error "Could not find Git path. Message is #{e.message}"
     end
-    merge_request.destroy
   end
 end

test/functional/merge_requests_controller_test.rb

@@ -643,9 +643,9 @@ def do_delete
 
 		should "soft-delete the record" do
 			login_as :johan
-      MergeRequest.stubs(:find).returns(@merge_request)
-      @merge_request.expects(:soft_delete)
-			do_delete
+      assert_difference("@target_repository.merge_requests.open.count", -1) do
+        do_delete
+      end
 			assert_redirected_to(project_repository_path(@project, @target_repository))
 			assert_match(/merge request was retracted/i, flash[:success])
 		end

test/unit/merge_request_test.rb

@@ -645,29 +645,24 @@ def setup
     end
   end
 
-  context "Deletion of merge requests in the target git repository" do
-    setup do
-      @merge_request = merge_requests(:moes_to_johans)
-      @source_git_repo = mock
-      @source_git = mock
-      @source_git_repo.stubs(:git).returns(@source_git)
-      @merge_request.source_repository.stubs(:git).returns(@source_git_repo)
-    end
-
-    should "push a deletion to the target repository" do
-      @source_git.expects(:push).with({}, @merge_request.target_repository.full_repository_path, ":#{@merge_request.merge_branch_name}")
-      @merge_request.delete_target_repository_ref
-    end
-  end
-
   context "Soft deletion of merge requests" do
     setup do
       @merge_request = merge_requests(:moes_to_johans_open)
     end
-    should "send a message when being soft deleted" do
-      p = proc {@merge_request.soft_delete}
-      message = find_message_with_queue_and_regexp('/queue/GitoriousMergeRequestBackend', /.*/) {p.call}
-      assert_equal({'merge_request_id' => @merge_request.id.to_s, "action" => "delete"}, message)
+
+    should "send a message when being destroyed" do
+      deletion_proc = proc{ @merge_request.destroy }
+      msg = find_message_with_queue_and_regexp('/queue/GitoriousMergeRequestBackend', /.*/) {
+        deletion_proc.call
+      }
+      assert_nil MergeRequest.find_by_id(@merge_request.id)
+      assert_equal @merge_request.id.to_s, msg["merge_request_id"]
+      assert_equal "delete", msg["action"]
+      assert_equal @merge_request.target_repository.full_repository_path, msg["target_path"]
+      assert_equal @merge_request.target_repository.url_path, msg["target_name"]
+      assert_equal @merge_request.merge_branch_name, msg["merge_branch_name"]
+      assert_equal @merge_request.target_repository.id, msg["target_repository_id"]
+      assert_equal @merge_request.source_repository.id, msg["source_repository_id"]
     end
   end
 end

test/unit/processors/merge_request_git_backend_processor_test.rb

@@ -33,18 +33,30 @@ def teardown
 
   context "Deleting the merge request and its tracking branch" do
     setup do
-      @msg = {:merge_request_id => @merge_request.to_param, :action => "delete"}
-      @processor.instance_variable_set("@merge_request", @merge_request)
+      @source_git_repo = mock
+      @source_git = mock
+      @source_git_repo.stubs(:git).returns(@source_git)
+      @merge_request.source_repository.stubs(:git).returns(@source_git_repo)
+      @msg = {
+        :merge_request_id => @merge_request.to_param,
+        :action => "delete",
+        :target_path => @merge_request.target_repository.full_repository_path,
+        :target_name => @merge_request.target_repository.url_path,
+        :merge_branch_name => @merge_request.merge_branch_name,
+        :source_repository_id => @merge_request.source_repository.id,
+        :target_repository_id => @merge_request.target_repository.id,
+      }
     end
 
-    should "delete the tracking repo and the merge request itself" do
-      @merge_request.expects(:delete_target_repository_ref).once
-      @merge_request.expects(:destroy).once
+    should "push to delete the tracking branch" do
+      @processor.stubs(:source_repository).returns(@merge_request.source_repository)
+      @source_git.expects(:push).with({:timeout => false},
+        @merge_request.target_repository.full_repository_path,
+        ":#{@merge_request.merge_branch_name}")
       @processor.on_message(@msg.to_json)
     end
 
     should "handle non-existing target gits" do
-      @merge_request.expects(:destroy).once
       assert_nothing_raised do
         @processor.on_message(@msg.to_json)
       end
@@ -57,7 +69,6 @@ def teardown
       @processor.expects(:do_delete).once
       @processor.on_message(msg.to_json)
       assert_equal :delete, @processor.action
-      assert_equal @merge_request, @processor.merge_request
     end
 
     should "understand the close command" do