From 143cdceed977c4b6fc0bd042a5e14015db638b24 Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Tue, 28 Feb 2017 08:35:18 -0400 Subject: [PATCH 1/5] Add ability to disable Merge Request URL on push The field is added to the project settings page. There is also a new internal API call for `project_settings` which supplies the value of this field. This is used by GitLab-Shell to determine whether to print the URL. --- app/controllers/projects_controller.rb | 1 + .../_merge_request_merge_settings.html.haml | 4 ++++ .../21451-allow-disable-mr-link.yml | 4 ++++ ...g_merge_request_link_enabled_to_project.rb | 18 +++++++++++++++ db/schema.rb | 1 + lib/api/internal.rb | 6 +++++ .../settings/merge_requests_settings_spec.rb | 23 +++++++++++++++++++ spec/requests/api/internal_spec.rb | 14 +++++++++++ 8 files changed, 71 insertions(+) create mode 100644 changelogs/unreleased/21451-allow-disable-mr-link.yml create mode 100644 db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 3e2015b7d5e6..2ee99eddc6d8 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -315,6 +315,7 @@ def project_params_ce :namespace_id, :only_allow_merge_if_all_discussions_are_resolved, :only_allow_merge_if_pipeline_succeeds, + :printing_merge_request_link_enabled, :path, :public_builds, :request_access_enabled, diff --git a/app/views/projects/_merge_request_merge_settings.html.haml b/app/views/projects/_merge_request_merge_settings.html.haml index 188198c47d53..7670dad2eda1 100644 --- a/app/views/projects/_merge_request_merge_settings.html.haml +++ b/app/views/projects/_merge_request_merge_settings.html.haml @@ -13,3 +13,7 @@ = form.label :only_allow_merge_if_all_discussions_are_resolved do = form.check_box :only_allow_merge_if_all_discussions_are_resolved %strong Only allow merge requests to be merged if all discussions are resolved + .checkbox + = form.label :printing_merge_request_link_enabled do + = form.check_box :printing_merge_request_link_enabled + %strong Enable link to create/view merge request on push diff --git a/changelogs/unreleased/21451-allow-disable-mr-link.yml b/changelogs/unreleased/21451-allow-disable-mr-link.yml new file mode 100644 index 000000000000..ef99970a7a2e --- /dev/null +++ b/changelogs/unreleased/21451-allow-disable-mr-link.yml @@ -0,0 +1,4 @@ +--- +title: Add ability to disable Merge Request URL on push +merge_request: 9663 +author: Alex Sanford diff --git a/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb b/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb new file mode 100644 index 000000000000..f54608ecceb5 --- /dev/null +++ b/db/migrate/20170301125302_add_printing_merge_request_link_enabled_to_project.rb @@ -0,0 +1,18 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddPrintingMergeRequestLinkEnabledToProject < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + disable_ddl_transaction! + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + def up + add_column_with_default(:projects, :printing_merge_request_link_enabled, :boolean, default: true) + end + + def down + remove_column(:projects, :printing_merge_request_link_enabled) + end +end diff --git a/db/schema.rb b/db/schema.rb index 3ec5461f6005..b0d2b8ff4b3b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -1003,6 +1003,7 @@ t.boolean "lfs_enabled" t.text "description_html" t.boolean "only_allow_merge_if_all_discussions_are_resolved" + t.boolean "printing_merge_request_link_enabled", default: true, null: false end add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 7eed93aba006..36907d1d5bc1 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -78,6 +78,12 @@ class Internal < Grape::API ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end + get "/project_settings" do + { + printing_merge_request_link_enabled: project.printing_merge_request_link_enabled + } + end + # # Discover user by ssh key # diff --git a/spec/features/projects/settings/merge_requests_settings_spec.rb b/spec/features/projects/settings/merge_requests_settings_spec.rb index 6815039d5eda..321af416c914 100644 --- a/spec/features/projects/settings/merge_requests_settings_spec.rb +++ b/spec/features/projects/settings/merge_requests_settings_spec.rb @@ -62,4 +62,27 @@ expect(page).to have_content('Only allow merge requests to be merged if all discussions are resolved') end end + + describe 'Checkbox to enable merge request link' do + before do + visit edit_project_path(project) + end + + scenario 'is initially checked' do + checkbox = find_field('project_printing_merge_request_link_enabled') + expect(checkbox).to be_checked + end + + scenario 'when unchecked sets :printing_merge_request_link_enabled to false' do + uncheck('project_printing_merge_request_link_enabled') + click_on('Save') + + # Wait for save to complete and page to reload + checkbox = find_field('project_printing_merge_request_link_enabled') + expect(checkbox).not_to be_checked + + project.reload + expect(project.printing_merge_request_link_enabled).to be(false) + end + end end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index f18b8e987078..89fd14877efa 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -437,6 +437,20 @@ end end + describe 'GET /internal/project_settings' do + it 'returns flag to enable printing merge request URLs' do + project.printing_merge_request_link_enabled = false + project.save! + repo_name = "#{project.namespace.name}/#{project.path}" + + get api("/internal/project_settings?project=#{repo_name}"), secret_token: secret_token + + expect(json_response).to match({ + 'printing_merge_request_link_enabled' => false + }) + end + end + def project_with_repo_path(path) double().tap do |fake_project| allow(fake_project).to receive_message_chain('repository.path_to_repo' => path) -- GitLab From 1dcddf51a62cd1181d53f95765ffdc10068a143e Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Fri, 10 Mar 2017 08:01:42 -0400 Subject: [PATCH 2/5] Change wording for option --- app/views/projects/_merge_request_merge_settings.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/_merge_request_merge_settings.html.haml b/app/views/projects/_merge_request_merge_settings.html.haml index 7670dad2eda1..5d732e1cc4b2 100644 --- a/app/views/projects/_merge_request_merge_settings.html.haml +++ b/app/views/projects/_merge_request_merge_settings.html.haml @@ -16,4 +16,4 @@ .checkbox = form.label :printing_merge_request_link_enabled do = form.check_box :printing_merge_request_link_enabled - %strong Enable link to create/view merge request on push + %strong Show link to create/view merge request on push -- GitLab From bb003248b324db82ba258101a47ce675a5ba7fe7 Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Thu, 16 Mar 2017 08:38:37 -0300 Subject: [PATCH 3/5] Remove project_settings endpoint and use merge_request_urls --- .../merge_requests/get_urls_service.rb | 2 ++ lib/api/internal.rb | 6 ----- spec/requests/api/internal_spec.rb | 25 ++++++++----------- .../merge_requests/get_urls_service_spec.rb | 10 ++++++++ 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/app/services/merge_requests/get_urls_service.rb b/app/services/merge_requests/get_urls_service.rb index 1262ecbc29aa..424549e62f12 100644 --- a/app/services/merge_requests/get_urls_service.rb +++ b/app/services/merge_requests/get_urls_service.rb @@ -7,6 +7,8 @@ def initialize(project) end def execute(changes) + return [] unless project.printing_merge_request_link_enabled + branches = get_branches(changes) merge_requests_map = opened_merge_requests_from_source_branches(branches) branches.map do |branch| diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 36907d1d5bc1..7eed93aba006 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -78,12 +78,6 @@ class Internal < Grape::API ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end - get "/project_settings" do - { - printing_merge_request_link_enabled: project.printing_merge_request_link_enabled - } - end - # # Discover user by ssh key # diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index 89fd14877efa..b30c8f44e497 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -397,16 +397,25 @@ before do project.team << [user, :developer] - get api("/internal/merge_request_urls?project=#{repo_name}&changes=#{changes}"), secret_token: secret_token end it 'returns link to create new merge request' do + get api("/internal/merge_request_urls?project=#{repo_name}&changes=#{changes}"), secret_token: secret_token + expect(json_response).to match [{ "branch_name" => "new_branch", "url" => "http://#{Gitlab.config.gitlab.host}/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch", "new_merge_request" => true }] end + + it 'returns empty array if printing_merge_request_link_enabled is false' do + project.update!(printing_merge_request_link_enabled: false) + + get api("/internal/merge_request_urls?project=#{repo_name}&changes=#{changes}"), secret_token: secret_token + + expect(json_response).to match([]) + end end describe 'POST /notify_post_receive' do @@ -437,20 +446,6 @@ end end - describe 'GET /internal/project_settings' do - it 'returns flag to enable printing merge request URLs' do - project.printing_merge_request_link_enabled = false - project.save! - repo_name = "#{project.namespace.name}/#{project.path}" - - get api("/internal/project_settings?project=#{repo_name}"), secret_token: secret_token - - expect(json_response).to match({ - 'printing_merge_request_link_enabled' => false - }) - end - end - def project_with_repo_path(path) double().tap do |fake_project| allow(fake_project).to receive_message_chain('repository.path_to_repo' => path) diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb index 08829e4be705..b7a059072089 100644 --- a/spec/services/merge_requests/get_urls_service_spec.rb +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -130,5 +130,15 @@ }]) end end + + context 'when printing_merge_request_link_enabled is false' do + it 'returns empty array' do + project.update!(printing_merge_request_link_enabled: false) + + result = service.execute(existing_branch_changes) + + expect(result).to eq([]) + end + end end end -- GitLab From 454092ee56ce3a2af9d5ad06fbc06003deeeb8dd Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Thu, 16 Mar 2017 08:46:32 -0300 Subject: [PATCH 4/5] Update text for checkbox --- app/views/projects/_merge_request_merge_settings.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/_merge_request_merge_settings.html.haml b/app/views/projects/_merge_request_merge_settings.html.haml index 5d732e1cc4b2..61420fd0fb6c 100644 --- a/app/views/projects/_merge_request_merge_settings.html.haml +++ b/app/views/projects/_merge_request_merge_settings.html.haml @@ -16,4 +16,4 @@ .checkbox = form.label :printing_merge_request_link_enabled do = form.check_box :printing_merge_request_link_enabled - %strong Show link to create/view merge request on push + %strong Show link to create/view merge request when pushing from the command line -- GitLab From b1630de054faedddd950604a8da7462fddcf5d0c Mon Sep 17 00:00:00 2001 From: Alex Sanford Date: Thu, 16 Mar 2017 11:40:34 -0300 Subject: [PATCH 5/5] Fix spec --- spec/requests/api/internal_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index b30c8f44e497..63ec00cdf049 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -414,7 +414,7 @@ get api("/internal/merge_request_urls?project=#{repo_name}&changes=#{changes}"), secret_token: secret_token - expect(json_response).to match([]) + expect(json_response).to eq([]) end end -- GitLab