Commit b9b1910c authored by Douwe Maan's avatar Douwe Maan

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
parents cbafc9ef 6109daf4
...@@ -104,6 +104,7 @@ v 8.11.0 (unreleased) ...@@ -104,6 +104,7 @@ v 8.11.0 (unreleased)
- Fix importing GitLab projects with an invalid MR source project - Fix importing GitLab projects with an invalid MR source project
- Sort folders with submodules in Files view !5521 - Sort folders with submodules in Files view !5521
- Each `File::exists?` replaced to `File::exist?` because of deprecate since ruby version 2.2.0 - 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 v 8.10.5
- Add a data migration to fix some missing timestamps in the members table. !5670 - Add a data migration to fix some missing timestamps in the members table. !5670
......
...@@ -104,6 +104,7 @@ class MergeRequest < ActiveRecord::Base ...@@ -104,6 +104,7 @@ class MergeRequest < ActiveRecord::Base
scope :from_project, ->(project) { where(source_project_id: project.id) } scope :from_project, ->(project) { where(source_project_id: project.id) }
scope :merged, -> { with_state(:merged) } scope :merged, -> { with_state(:merged) }
scope :closed_and_merged, -> { with_states(:closed, :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 :join_project, -> { joins(:target_project) }
scope :references_project, -> { references(:target_project) } scope :references_project, -> { references(:target_project) }
......
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
...@@ -74,6 +74,10 @@ module API ...@@ -74,6 +74,10 @@ module API
response response
end end
get "/merge_request_urls" do
::MergeRequests::GetUrlsService.new(project).execute(params[:changes])
end
# #
# Discover user by ssh key # Discover user by ssh key
# #
......
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
...@@ -4,8 +4,8 @@ module Gitlab ...@@ -4,8 +4,8 @@ module Gitlab
attr_reader :user_access, :project attr_reader :user_access, :project
def initialize(change, user_access:, project:) def initialize(change, user_access:, project:)
@oldrev, @newrev, @ref = change.split(' ') @oldrev, @newrev, @ref = change.values_at(:oldrev, :newrev, :ref)
@branch_name = branch_name(@ref) @branch_name = Gitlab::Git.branch_name(@ref)
@user_access = user_access @user_access = user_access
@project = project @project = project
end end
...@@ -47,7 +47,7 @@ module Gitlab ...@@ -47,7 +47,7 @@ module Gitlab
end end
def tag_checks 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) 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." "You are not allowed to change existing tags on this project."
...@@ -73,24 +73,6 @@ module Gitlab ...@@ -73,24 +73,6 @@ module Gitlab
def matching_merge_request? def matching_merge_request?
Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match? Checks::MatchingMergeRequest.new(@newrev, @branch_name, @project).match?
end 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 end
end end
...@@ -9,6 +9,24 @@ module Gitlab ...@@ -9,6 +9,24 @@ module Gitlab
ref.gsub(/\Arefs\/(tags|heads)\//, '') ref.gsub(/\Arefs\/(tags|heads)\//, '')
end 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) def tag_ref?(ref)
ref.start_with?(TAG_REF_PREFIX) ref.start_with?(TAG_REF_PREFIX)
end end
......
...@@ -76,10 +76,10 @@ module Gitlab ...@@ -76,10 +76,10 @@ module Gitlab
return build_status_object(false, "A repository for this project does not exist yet.") return build_status_object(false, "A repository for this project does not exist yet.")
end 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 # 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) status = change_access_check(change)
unless status.allowed? unless status.allowed?
# If user does not have access to make at least one change - cancel all push # If user does not have access to make at least one change - cancel all push
......
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
...@@ -275,6 +275,24 @@ describe API::API, api: true do ...@@ -275,6 +275,24 @@ describe API::API, api: true do
end end
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') def pull(key, project, protocol = 'ssh')
post( post(
api("/internal/allowed"), api("/internal/allowed"),
......
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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment