Address MR comments

11 files changed, 60 insertions, 26 deletions

diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js
index 0016070b648..fac21f8cd32 100644
--- a/app/assets/javascripts/notes.js
+++ b/app/assets/javascripts/notes.js
@@ -250,7 +250,7 @@
         return;
       }
 
-      if (note.commands_changes && note.commands_changes.includes('merge')) {
+      if (note.commands_changes && note.commands_changes.indexOf('merge') !== -1) {
         $.get(mrRefreshWidgetUrl);
       }
     };
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index 72dcf020c9f..6d6115413a5 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/app/controllers/projects/merge_requests_controller.rb
@@ -348,7 +348,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def merge_widget_refresh
-    if merge_request.in_progress_merge_commit_sha
+    if merge_request.in_progress_merge_commit_sha || merge_request.state == 'merged'
       @status = :success
     elsif merge_request.merge_when_build_succeeds
       @status = :merge_when_build_succeeds
diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb
index 20218775659..8c2c4e8833b 100644
--- a/app/helpers/merge_requests_helper.rb
+++ b/app/helpers/merge_requests_helper.rb
@@ -19,6 +19,14 @@ module MergeRequestsHelper
     }
   end
 
+  def mr_widget_refresh_url(mr)
+    if mr && mr.source_project
+      merge_widget_refresh_namespace_project_merge_request_url(mr.source_project.namespace, mr.source_project, mr)
+    else
+      ''
+    end
+  end
+
   def mr_css_classes(mr)
     classes = "merge-request"
     classes << " closed" if mr.closed?
diff --git a/app/services/slash_commands/interpret_service.rb b/app/services/slash_commands/interpret_service.rb
index 14fad3ba120..d18844ac0e3 100644
--- a/app/services/slash_commands/interpret_service.rb
+++ b/app/services/slash_commands/interpret_service.rb
@@ -61,10 +61,10 @@ module SlashCommands
 
     desc 'Merge (when build succeeds)'
     condition do
-      last_diff_sha = params.to_h[:merge_request_diff_head_sha]
+      last_diff_sha = params && params[:merge_request_diff_head_sha]
       issuable.is_a?(MergeRequest) &&
-        issuable.mergeable_with_slash_command?(current_user, autocomplete_precheck: !last_diff_sha, last_diff_sha: last_diff_sha) &&
-        issuable.persisted?
+        issuable.persisted? &&
+        issuable.mergeable_with_slash_command?(current_user, autocomplete_precheck: !last_diff_sha, last_diff_sha: last_diff_sha)
     end
     command :merge do
       @updates[:merge] = params[:merge_request_diff_head_sha]
diff --git a/app/views/projects/merge_requests/_show.html.haml b/app/views/projects/merge_requests/_show.html.haml
index 2e7cd52df1e..d95017286ba 100644
--- a/app/views/projects/merge_requests/_show.html.haml
+++ b/app/views/projects/merge_requests/_show.html.haml
@@ -113,4 +113,5 @@
     action: "#{controller.action_name}"
   });
 
-  var mrRefreshWidgetUrl = "#{@merge_request && @merge_request.source_project ? merge_widget_refresh_namespace_project_merge_request_url(@merge_request.source_project.namespace, @merge_request.source_project, @merge_request) : ''}";
+  var mrRefreshWidgetUrl = "#{mr_widget_refresh_url(@merge_request)}";
+
diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index c6a38863cf5..7ea3ea4f376 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/spec/controllers/projects/merge_requests_controller_spec.rb
@@ -1071,8 +1071,22 @@ describe Projects::MergeRequestsController do
         expect(response).to have_http_status(:ok)
       end
 
-      it 'returns :success' do
+      it 'sets status to :success' do
+        expect(assigns(:status)).to eq(:success)
+        expect(response).to render_template('merge')
+      end
+    end
+
+    context 'when merge request was merged already' do
+      let(:merge_request) { create(:merge_request, source_project: project, state: :merged) }
+
+      it 'returns an OK response' do
+        expect(response).to have_http_status(:ok)
+      end
+
+      it 'sets status to :success' do
         expect(assigns(:status)).to eq(:success)
+        expect(response).to render_template('merge')
       end
     end
 
@@ -1083,8 +1097,9 @@ describe Projects::MergeRequestsController do
         expect(response).to have_http_status(:ok)
       end
 
-      it 'returns :merge_when_build_succeeds' do
+      it 'sets status to :merge_when_build_succeeds' do
         expect(assigns(:status)).to eq(:merge_when_build_succeeds)
+        expect(response).to render_template('merge')
       end
     end
 
@@ -1095,8 +1110,9 @@ describe Projects::MergeRequestsController do
         expect(response).to have_http_status(:ok)
       end
 
-      it 'returns nil' do
+      it 'sets status to nil' do
         expect(assigns(:status)).to be_nil
+        expect(response).to render_template('merge')
       end
     end
   end
diff --git a/spec/features/merge_requests/user_uses_slash_commands_spec.rb b/spec/features/merge_requests/user_uses_slash_commands_spec.rb
index d7e8723b63a..b13674b4db9 100644
--- a/spec/features/merge_requests/user_uses_slash_commands_spec.rb
+++ b/spec/features/merge_requests/user_uses_slash_commands_spec.rb
@@ -73,7 +73,7 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
         it 'merges the MR' do
           write_note("/merge")
 
-          expect(page).to have_content 'Your commands have been executed!'
+          expect(page).to have_content 'Commands applied'
 
           expect(merge_request.reload).to be_merged
         end
@@ -81,20 +81,8 @@ feature 'Merge Requests > User uses slash commands', feature: true, js: true do
 
       context 'when the head diff changes in the meanwhile' do
         before do
-          path = File.expand_path("#{project.repository_storage_path}/#{project.namespace.path}/#{project.path}/new_file.txt")
-
-          params = {
-            source_project: merge_request.project,
-            target_project: merge_request.project,
-            target_branch: merge_request.source_branch,
-            source_branch: merge_request.source_branch,
-            file_path: path,
-            file_content: 'some content',
-            commit_message: 'additional commit',
-          }
-
-          Files::UpdateService.new(project, user, params).execute
-          merge_request.reload_diff
+          merge_request.source_branch = 'another_branch'
+          merge_request.save
         end
 
         it 'does not merge the MR' do
diff --git a/spec/helpers/merge_requests_helper_spec.rb b/spec/helpers/merge_requests_helper_spec.rb
index 903224589dd..1f221487393 100644
--- a/spec/helpers/merge_requests_helper_spec.rb
+++ b/spec/helpers/merge_requests_helper_spec.rb
@@ -62,4 +62,19 @@ describe MergeRequestsHelper do
       it { is_expected.to eq([source_title, target_title]) }
     end
   end
+
+  describe 'mr_widget_refresh_url' do
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    let(:project)       { create(:project) }
+
+    it 'returns correct url for MR' do
+      expected_url = "#{project.path_with_namespace}/merge_requests/#{merge_request.iid}/merge_widget_refresh"
+
+      expect(mr_widget_refresh_url(merge_request)).to end_with(expected_url)
+    end
+
+    it 'returns empty string for nil' do
+      expect(mr_widget_refresh_url(nil)).to end_with('')
+    end
+  end
 end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 722987a423c..861426acbc3 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1578,6 +1578,12 @@ describe MergeRequest, models: true do
         end
       end
 
+      context 'sha is not provided'  do
+        it 'is not mergeable' do
+          expect(merge_request.mergeable_with_slash_command?(developer)).to be_falsey
+        end
+      end
+
       context 'with pipeline ok'  do
         before do
           create_pipeline(:success)
diff --git a/spec/services/merge_requests/update_service_spec.rb b/spec/services/merge_requests/update_service_spec.rb
index c2f6ae29d62..7d73c0ea5d0 100644
--- a/spec/services/merge_requests/update_service_spec.rb
+++ b/spec/services/merge_requests/update_service_spec.rb
@@ -179,7 +179,7 @@ describe MergeRequests::UpdateService, services: true do
         it { service.execute(merge_request) }
       end
 
-      context 'MR can not be merged by non authorised user' do
+      context 'with a non-authorised user' do
         let(:visitor) { create(:user) }
         let(:service) { MergeRequests::UpdateService.new(project, visitor, opts) }
 
diff --git a/spec/services/slash_commands/interpret_service_spec.rb b/spec/services/slash_commands/interpret_service_spec.rb
index dfcffcc6131..ffcf02d2c56 100644
--- a/spec/services/slash_commands/interpret_service_spec.rb
+++ b/spec/services/slash_commands/interpret_service_spec.rb
@@ -283,7 +283,7 @@ describe SlashCommands::InterpretService, services: true do
         end
       end
 
-      context 'non merge request object cant be merged' do
+      context 'issue can not be merged' do
         it_behaves_like 'empty command' do
           let(:content) { "/merge" }
           let(:issuable) { issue }