From 6109daf480327581b6e2dcdfffe90464be6c7796 Mon Sep 17 00:00:00 2001
From: Scott Le <scott.lee318@gmail.com>
Date: Thu, 28 Jul 2016 11:04:57 +0700
Subject: [PATCH] api for generating new merge request
DRY code + fix rubocop
Add more test cases
Append to changelog
DRY changes list
find_url service for merge_requests
use GET for getting merge request links
remove files
rename to get_url_service
reduce loop
add test case for cross project
refactor tiny thing
update changelog
---
CHANGELOG | 1 +
app/models/merge_request.rb | 1 +
.../merge_requests/get_urls_service.rb | 52 +++++++++
lib/api/internal.rb | 4 +
lib/gitlab/changes_list.rb | 25 +++++
lib/gitlab/checks/change_access.rb | 24 +----
lib/gitlab/git.rb | 18 ++++
lib/gitlab/git_access.rb | 4 +-
spec/lib/gitlab/changes_list_spec.rb | 30 ++++++
spec/requests/api/internal_spec.rb | 18 ++++
.../merge_requests/get_urls_service_spec.rb | 100 ++++++++++++++++++
11 files changed, 254 insertions(+), 23 deletions(-)
create mode 100644 app/services/merge_requests/get_urls_service.rb
create mode 100644 lib/gitlab/changes_list.rb
create mode 100644 spec/lib/gitlab/changes_list_spec.rb
create mode 100644 spec/services/merge_requests/get_urls_service_spec.rb
diff --git a/CHANGELOG b/CHANGELOG
index 282e4b9b449..2f527bde05f 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -102,6 +102,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
--
GitLab