diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-08-11 18:23:06 +0000 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-08-11 18:23:06 +0000 |
commit | b9b1910c3b93b1440710681ddeb5b03528b10266 (patch) | |
tree | c8cc67f15a3536df40f9bafde78b8bf274a172c2 | |
parent | cbafc9ef301dbdc3c3205541cd0f8e31e28ece7a (diff) | |
parent | 6109daf480327581b6e2dcdfffe90464be6c7796 (diff) | |
download | gitlab-ce-b9b1910c3b93b1440710681ddeb5b03528b10266.tar.gz |
Merge branch 'feature/merge-request-link-after-push' into 'master'
Merge request link after pushing
## What does this MR do?
Add API to generate new merge request urls if it is a new branch. The API will be called by gitlab-shell
## What are the relevant issue numbers?
#18266
## Does this MR meet the acceptance criteria?
- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] API support added
- Tests
- [x] Added for this feature/bug
- [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)
See merge request !5542
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/models/merge_request.rb | 1 | ||||
-rw-r--r-- | app/services/merge_requests/get_urls_service.rb | 52 | ||||
-rw-r--r-- | lib/api/internal.rb | 4 | ||||
-rw-r--r-- | lib/gitlab/changes_list.rb | 25 | ||||
-rw-r--r-- | lib/gitlab/checks/change_access.rb | 24 | ||||
-rw-r--r-- | lib/gitlab/git.rb | 18 | ||||
-rw-r--r-- | lib/gitlab/git_access.rb | 4 | ||||
-rw-r--r-- | spec/lib/gitlab/changes_list_spec.rb | 30 | ||||
-rw-r--r-- | spec/requests/api/internal_spec.rb | 18 | ||||
-rw-r--r-- | spec/services/merge_requests/get_urls_service_spec.rb | 100 |
11 files changed, 254 insertions, 23 deletions
diff --git a/CHANGELOG b/CHANGELOG index 8cefaf5d70a..599ff678f82 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -104,6 +104,7 @@ v 8.11.0 (unreleased) - Fix importing GitLab projects with an invalid MR source project - Sort folders with submodules in Files view !5521 - Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0 + - Print urls to create (or view) merge requests after git push !5542 (Scott Le) v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b1fb3ce5d69..f6d0d0c98f5 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -104,6 +104,7 @@ class MergeRequest < ActiveRecord::Base scope :from_project, ->(project) { where(source_project_id: project.id) } scope :merged, -> { with_state(:merged) } scope :closed_and_merged, -> { with_states(:closed, :merged) } + scope :from_source_branches, ->(branches) { where(source_branch: branches) } scope :join_project, -> { joins(:target_project) } scope :references_project, -> { references(:target_project) } diff --git a/app/services/merge_requests/get_urls_service.rb b/app/services/merge_requests/get_urls_service.rb new file mode 100644 index 00000000000..501fd135e16 --- /dev/null +++ b/app/services/merge_requests/get_urls_service.rb @@ -0,0 +1,52 @@ +module MergeRequests + class GetUrlsService < BaseService + attr_reader :project + + def initialize(project) + @project = project + end + + def execute(changes) + branches = get_branches(changes) + merge_requests_map = opened_merge_requests_from_source_branches(branches) + branches.map do |branch| + existing_merge_request = merge_requests_map[branch] + if existing_merge_request + url_for_existing_merge_request(existing_merge_request) + else + url_for_new_merge_request(branch) + end + end + end + + private + + def opened_merge_requests_from_source_branches(branches) + merge_requests = MergeRequest.from_project(project).opened.from_source_branches(branches) + merge_requests.inject({}) do |hash, mr| + hash[mr.source_branch] = mr + hash + end + end + + def get_branches(changes) + changes_list = Gitlab::ChangesList.new(changes) + changes_list.map do |change| + next unless Gitlab::Git.branch_ref?(change[:ref]) + Gitlab::Git.branch_name(change[:ref]) + end.compact + end + + def url_for_new_merge_request(branch_name) + merge_request_params = { source_branch: branch_name } + url = Gitlab::Routing.url_helpers.new_namespace_project_merge_request_url(project.namespace, project, merge_request: merge_request_params) + { branch_name: branch_name, url: url, new_merge_request: true } + end + + def url_for_existing_merge_request(merge_request) + target_project = merge_request.target_project + url = Gitlab::Routing.url_helpers.namespace_project_merge_request_url(target_project.namespace, target_project, merge_request) + { branch_name: merge_request.source_branch, url: url, new_merge_request: false } + end + end +end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 959b700de78..d8e9ac406c4 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -74,6 +74,10 @@ module API response end + get "/merge_request_urls" do + ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) + end + # # Discover user by ssh key # diff --git a/lib/gitlab/changes_list.rb b/lib/gitlab/changes_list.rb new file mode 100644 index 00000000000..95308aca95f --- /dev/null +++ b/lib/gitlab/changes_list.rb @@ -0,0 +1,25 @@ +module Gitlab + class ChangesList + include Enumerable + + attr_reader :raw_changes + + def initialize(changes) + @raw_changes = changes.kind_of?(String) ? changes.lines : changes + end + + def each(&block) + changes.each(&block) + end + + def changes + @changes ||= begin + @raw_changes.map do |change| + next if change.blank? + oldrev, newrev, ref = change.strip.split(' ') + { oldrev: oldrev, newrev: newrev, ref: ref } + end.compact + end + end + end +end diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 5551fac4b8b..52f117e963b 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -4,8 +4,8 @@ module Gitlab attr_reader :user_access, :project def initialize(change, user_access:, project:) - @oldrev, @newrev, @ref = change.split(' ') - @branch_name = branch_name(@ref) + @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref) + @branch_name = Gitlab::Git.branch_name(@ref) @user_access = user_access @project = project end @@ -47,7 +47,7 @@ module Gitlab end def tag_checks - tag_ref = tag_name(@ref) + tag_ref = Gitlab::Git.tag_name(@ref) if tag_ref && protected_tag?(tag_ref) && user_access.cannot_do_action?(:admin_project) "You are not allowed to change existing tags on this project." @@ -73,24 +73,6 @@ module Gitlab def matching_merge_request? Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? end - - def branch_name(ref) - ref = @ref.to_s - if Gitlab::Git.branch_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end - - def tag_name(ref) - ref = @ref.to_s - if Gitlab::Git.tag_ref?(ref) - Gitlab::Git.ref_name(ref) - else - nil - end - end end end end diff --git a/lib/gitlab/git.rb b/lib/gitlab/git.rb index 191bea86ac3..7584efe4fa8 100644 --- a/lib/gitlab/git.rb +++ b/lib/gitlab/git.rb @@ -9,6 +9,24 @@ module Gitlab ref.gsub(/\Arefs\/(tags|heads)\//, '') end + def branch_name(ref) + ref = ref.to_s + if self.branch_ref?(ref) + self.ref_name(ref) + else + nil + end + end + + def tag_name(ref) + ref = ref.to_s + if self.tag_ref?(ref) + self.ref_name(ref) + else + nil + end + end + def tag_ref?(ref) ref.start_with?(TAG_REF_PREFIX) end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 5bc70f530d2..1882eb8d050 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -76,10 +76,10 @@ module Gitlab return build_status_object(false, "A repository for this project does not exist yet.") end - changes = changes.lines if changes.kind_of?(String) + changes_list = Gitlab::ChangesList.new(changes) # Iterate over all changes to find if user allowed all of them to be applied - changes.map(&:strip).reject(&:blank?).each do |change| + changes_list.each do |change| status = change_access_check(change) unless status.allowed? # If user does not have access to make at least one change - cancel all push diff --git a/spec/lib/gitlab/changes_list_spec.rb b/spec/lib/gitlab/changes_list_spec.rb new file mode 100644 index 00000000000..69d86144e32 --- /dev/null +++ b/spec/lib/gitlab/changes_list_spec.rb @@ -0,0 +1,30 @@ +require "spec_helper" + +describe Gitlab::ChangesList do + let(:valid_changes_string) { "\n000000 570e7b2 refs/heads/my_branch\nd14d6c 6fd24d refs/heads/master" } + let(:invalid_changes) { 1 } + + context 'when changes is a valid string' do + let(:changes_list) { Gitlab::ChangesList.new(valid_changes_string) } + + it 'splits elements by newline character' do + expect(changes_list).to contain_exactly({ + oldrev: "000000", + newrev: "570e7b2", + ref: "refs/heads/my_branch" + }, { + oldrev: "d14d6c", + newrev: "6fd24d", + ref: "refs/heads/master" + }) + end + + it 'behaves like a list' do + expect(changes_list.first).to eq({ + oldrev: "000000", + newrev: "570e7b2", + ref: "refs/heads/my_branch" + }) + end + end +end diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb index f6f85d6e95e..be52f88831f 100644 --- a/spec/requests/api/internal_spec.rb +++ b/spec/requests/api/internal_spec.rb @@ -275,6 +275,24 @@ describe API::API, api: true do end end + describe 'GET /internal/merge_request_urls' do + let(:repo_name) { "#{project.namespace.name}/#{project.path}" } + let(:changes) { URI.escape("#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch") } + + 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 + expect(json_response).to match [{ + "branch_name" => "new_branch", + "url" => "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch", + "new_merge_request" => true + }] + end + end + def pull(key, project, protocol = 'ssh') post( api("/internal/allowed"), diff --git a/spec/services/merge_requests/get_urls_service_spec.rb b/spec/services/merge_requests/get_urls_service_spec.rb new file mode 100644 index 00000000000..ec26770c3eb --- /dev/null +++ b/spec/services/merge_requests/get_urls_service_spec.rb @@ -0,0 +1,100 @@ +require "spec_helper" + +describe MergeRequests::GetUrlsService do + let(:project) { create(:project, :public) } + let(:service) { MergeRequests::GetUrlsService.new(project) } + let(:source_branch) { "my_branch" } + let(:new_merge_request_url) { "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=#{source_branch}" } + let(:show_merge_request_url) { "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/#{merge_request.iid}" } + let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{source_branch}" } + + describe "#execute" do + shared_examples 'new_merge_request_link' do + it 'returns url to create new merge request' do + result = service.execute(changes) + expect(result).to match([{ + branch_name: source_branch, + url: new_merge_request_url, + new_merge_request: true + }]) + end + end + + shared_examples 'show_merge_request_url' do + it 'returns url to view merge request' do + result = service.execute(changes) + expect(result).to match([{ + branch_name: source_branch, + url: show_merge_request_url, + new_merge_request: false + }]) + end + end + + context 'pushing one completely new branch' do + let(:changes) { new_branch_changes } + it_behaves_like 'new_merge_request_link' + end + + context 'pushing to existing branch but no merge request' do + let(:changes) { existing_branch_changes } + it_behaves_like 'new_merge_request_link' + end + + context 'pushing to existing branch and merge request opened' do + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: source_branch) } + let(:changes) { existing_branch_changes } + it_behaves_like 'show_merge_request_url' + end + + context 'pushing to existing branch and merge request is reopened' do + let!(:merge_request) { create(:merge_request, :reopened, source_project: project, source_branch: source_branch) } + let(:changes) { existing_branch_changes } + it_behaves_like 'show_merge_request_url' + end + + context 'pushing to existing branch from forked project' do + let(:user) { create(:user) } + let!(:forked_project) { Projects::ForkService.new(project, user).execute } + let!(:merge_request) { create(:merge_request, source_project: forked_project, target_project: project, source_branch: source_branch) } + let(:changes) { existing_branch_changes } + # Source project is now the forked one + let(:service) { MergeRequests::GetUrlsService.new(forked_project) } + it_behaves_like 'show_merge_request_url' + end + + context 'pushing to existing branch and merge request is closed' do + let!(:merge_request) { create(:merge_request, :closed, source_project: project, source_branch: source_branch) } + let(:changes) { existing_branch_changes } + it_behaves_like 'new_merge_request_link' + end + + context 'pushing to existing branch and merge request is merged' do + let!(:merge_request) { create(:merge_request, :merged, source_project: project, source_branch: source_branch) } + let(:changes) { existing_branch_changes } + it_behaves_like 'new_merge_request_link' + end + + context 'pushing new branch and existing branch (with merge request created) at once' do + let!(:merge_request) { create(:merge_request, source_project: project, source_branch: "existing_branch") } + let(:new_branch_changes) { "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/new_branch" } + let(:existing_branch_changes) { "d14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/existing_branch" } + let(:changes) { "#{new_branch_changes}\n#{existing_branch_changes}" } + let(:new_merge_request_url) { "http://localhost/#{project.namespace.name}/#{project.path}/merge_requests/new?merge_request%5Bsource_branch%5D=new_branch" } + + it 'returns 2 urls for both creating new and showing merge request' do + result = service.execute(changes) + expect(result).to match([{ + branch_name: "new_branch", + url: new_merge_request_url, + new_merge_request: true + }, { + branch_name: "existing_branch", + url: show_merge_request_url, + new_merge_request: false + }]) + end + end + end +end |