Merge branch 'report-delete-branch-error-codes' into 'master'

Return 40x error codes if branch could not be deleted in UI

### What does this MR do?

This MR makes `BranchesController` return the proper status code in case of a failure to delete a branch.

### Why was this MR needed?

Deleting a branch would always return status 200, even if the branch could not be found or deleted for some reason. This was confusing because while trying to debug an issue with encoded slashes, it looked like the deletion was successful when it had failed.

### What are the relevant issue numbers?

This issue was uncovered in #1804

See merge request !823

3 files changed, 29 insertions, 2 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 49b93b977bb..6f607e2da06 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -20,6 +20,7 @@ v 7.13.0 (unreleased)
   - Update maintenance documentation to explain no need to recompile asssets for omnibus installations (Stan Hu)
   - Support commenting on diffs in side-by-side mode (Stan Hu)
   - Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu)
+  - Return 40x error codes if branch could not be deleted in UI (Stan Hu)
   - Remove project visibility icons from dashboard projects list
   - Rename "Design" profile settings page to "Preferences".
   - Allow users to customize their default Dashboard page.
diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index 696011b94b9..117ae3aaa3d 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -32,7 +32,7 @@ class Projects::BranchesController < Projects::ApplicationController
   end
 
   def destroy
-    DeleteBranchService.new(project, current_user).execute(params[:id])
+    status = DeleteBranchService.new(project, current_user).execute(params[:id])
     @branch_name = params[:id]
 
     respond_to do |format|
@@ -40,7 +40,7 @@ class Projects::BranchesController < Projects::ApplicationController
         redirect_to namespace_project_branches_path(@project.namespace,
                                                     @project)
       end
-      format.js
+      format.js { render status: status[:return_code] }
     end
   end
 end
diff --git a/spec/controllers/branches_controller_spec.rb b/spec/controllers/branches_controller_spec.rb
index db3b64babcd..bd4c946b64b 100644
--- a/spec/controllers/branches_controller_spec.rb
+++ b/spec/controllers/branches_controller_spec.rb
@@ -55,4 +55,30 @@ describe Projects::BranchesController do
       it { is_expected.to render_template('new') }
     end
   end
+
+  describe "POST destroy" do
+    render_views
+
+    before do
+      post :destroy,
+           format: :js,
+           id: branch,
+           namespace_id: project.namespace.to_param,
+           project_id: project.to_param
+    end
+
+    context "valid branch name, valid source" do
+      let(:branch) { "feature" }
+
+      it { expect(response.status).to eq(200) }
+      it { expect(subject).to render_template('destroy') }
+    end
+
+    context "invalid branch name, valid ref" do
+      let(:branch) { "no-branch" }
+
+      it { expect(response.status).to eq(404) }
+      it { expect(subject).to render_template('destroy') }
+    end
+  end
 end