diff options
author | Jarka Kadlecova <jarka@gitlab.com> | 2016-12-30 20:49:59 +0100 |
---|---|---|
committer | Jarka Kadlecova <jarka@gitlab.com> | 2017-01-13 09:16:35 -0500 |
commit | 557a0bf14c79c02c65196ff8f7a2251ecd77073c (patch) | |
tree | 36cef1d85fbc5541ec15949d5ea42feb8444c6df | |
parent | aa934c7469372cac7b8cd10b49761d90d8e367fa (diff) | |
download | gitlab-ce-557a0bf14c79c02c65196ff8f7a2251ecd77073c.tar.gz |
Address MR comments
-rw-r--r-- | app/assets/javascripts/notes.js | 2 | ||||
-rw-r--r-- | app/controllers/projects/merge_requests_controller.rb | 2 | ||||
-rw-r--r-- | app/helpers/merge_requests_helper.rb | 8 | ||||
-rw-r--r-- | app/services/slash_commands/interpret_service.rb | 6 | ||||
-rw-r--r-- | app/views/projects/merge_requests/_show.html.haml | 3 | ||||
-rw-r--r-- | spec/controllers/projects/merge_requests_controller_spec.rb | 22 | ||||
-rw-r--r-- | spec/features/merge_requests/user_uses_slash_commands_spec.rb | 18 | ||||
-rw-r--r-- | spec/helpers/merge_requests_helper_spec.rb | 15 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 6 | ||||
-rw-r--r-- | spec/services/merge_requests/update_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/services/slash_commands/interpret_service_spec.rb | 2 |
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 } |