From 09454b702a960923ca4f7f21f41e64404542d90a Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Wed, 19 Jun 2019 12:41:21 -0800
Subject: Support creating an MR on a fork from an issue

---
 app/controllers/projects/issues_controller.rb      |   1 +
 .../merge_requests/create_from_issue_service.rb    |  38 ++-
 .../create_from_issue_service_spec.rb              | 276 +++++++++++++++------
 3 files changed, 222 insertions(+), 93 deletions(-)

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index f221f0363d3..e275b417784 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -172,6 +172,7 @@ class Projects::IssuesController < Projects::ApplicationController
 
   def create_merge_request
     create_params = params.slice(:branch_name, :ref).merge(issue_iid: issue.iid)
+    create_params[:target_project_id] = params[:target_project_id] if Feature.enabled?(:create_confidential_merge_request, @project)
     result = ::MergeRequests::CreateFromIssueService.new(project, current_user, create_params).execute
 
     if result[:status] == :success
diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb
index e69791872cc..074a45e328b 100644
--- a/app/services/merge_requests/create_from_issue_service.rb
+++ b/app/services/merge_requests/create_from_issue_service.rb
@@ -6,27 +6,30 @@ module MergeRequests
       # branch - the name of new branch
       # ref    - the source of new branch.
 
-      @branch_name = params[:branch_name]
-      @issue_iid   = params[:issue_iid]
-      @ref         = params[:ref]
+      @branch_name       = params[:branch_name]
+      @issue_iid         = params[:issue_iid]
+      @ref               = params[:ref]
+      @target_project_id = params[:target_project_id]
 
       super(project, user)
     end
 
     def execute
+      return error('Project not found') if target_project.blank?
+      return error('Not allowed to create merge request') unless can_create_merge_request?
       return error('Invalid issue iid') unless @issue_iid.present? && issue.present?
 
-      result = CreateBranchService.new(project, current_user).execute(branch_name, ref)
+      result = CreateBranchService.new(target_project, current_user).execute(branch_name, ref)
       return result if result[:status] == :error
 
       new_merge_request = create(merge_request)
 
       if new_merge_request.valid?
-        SystemNoteService.new_merge_request(issue, project, current_user, new_merge_request)
+        SystemNoteService.new_merge_request(issue, target_project, current_user, new_merge_request)
 
         success(new_merge_request)
       else
-        SystemNoteService.new_issue_branch(issue, project, current_user, branch_name)
+        SystemNoteService.new_issue_branch(issue, target_project, current_user, branch_name)
 
         error(new_merge_request.errors)
       end
@@ -34,6 +37,10 @@ module MergeRequests
 
     private
 
+    def can_create_merge_request?
+      can?(current_user, :create_merge_request_from, target_project)
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
     def issue
       @issue ||= IssuesFinder.new(current_user, project_id: project.id).find_by(iid: @issue_iid)
@@ -45,21 +52,21 @@ module MergeRequests
     end
 
     def ref
-      return @ref if project.repository.branch_exists?(@ref)
+      return @ref if target_project.repository.branch_exists?(@ref)
 
-      project.default_branch || 'master'
+      target_project.default_branch || 'master'
     end
 
     def merge_request
-      MergeRequests::BuildService.new(project, current_user, merge_request_params).execute
+      MergeRequests::BuildService.new(target_project, current_user, merge_request_params).execute
     end
 
     def merge_request_params
       {
         issue_iid: @issue_iid,
-        source_project_id: project.id,
+        source_project_id: target_project.id,
         source_branch: branch_name,
-        target_project_id: project.id,
+        target_project_id: target_project.id,
         target_branch: ref
       }
     end
@@ -67,5 +74,14 @@ module MergeRequests
     def success(merge_request)
       super().merge(merge_request: merge_request)
     end
+
+    def target_project
+      @target_project ||=
+        if @target_project_id.present?
+          project.forks.find_by_id(@target_project_id)
+        else
+          project
+        end
+    end
   end
 end
diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb
index a0ac7dba89d..6b17b14d4b4 100644
--- a/spec/services/merge_requests/create_from_issue_service_spec.rb
+++ b/spec/services/merge_requests/create_from_issue_service_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe MergeRequests::CreateFromIssueService do
+  include ProjectForksHelper
+
   let(:project) { create(:project, :repository) }
   let(:user) { create(:user) }
   let(:label_ids) { create_pair(:label, project: project).map(&:id) }
@@ -10,139 +12,249 @@ describe MergeRequests::CreateFromIssueService do
   let(:issue) { create(:issue, project: project, milestone_id: milestone_id) }
   let(:custom_source_branch) { 'custom-source-branch' }
 
-  subject(:service) { described_class.new(project, user, issue_iid: issue.iid) }
-  subject(:service_with_custom_source_branch) { described_class.new(project, user, issue_iid: issue.iid, branch_name: custom_source_branch) }
+  subject(:service) { described_class.new(project, user, service_params) }
+  subject(:service_with_custom_source_branch) { described_class.new(project, user, service_params.merge(branch_name: custom_source_branch)) }
 
   before do
     project.add_developer(user)
   end
 
   describe '#execute' do
-    it 'returns an error with invalid issue iid' do
-      result = described_class.new(project, user, issue_iid: -1).execute
-
-      expect(result[:status]).to eq(:error)
-      expect(result[:message]).to eq('Invalid issue iid')
-    end
+    context 'no target_project_id specified' do
+      let(:service_params) { { issue_iid: issue.iid } }
 
-    it 'delegates issue search to IssuesFinder' do
-      expect_any_instance_of(IssuesFinder).to receive(:find_by).once.and_call_original
+      it 'returns an error with invalid issue iid' do
+        result = described_class.new(project, user, issue_iid: -1).execute
 
-      described_class.new(project, user, issue_iid: -1).execute
-    end
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Invalid issue iid')
+      end
 
-    it "inherits labels" do
-      issue.assign_attributes(label_ids: label_ids)
+      it "inherits labels" do
+        issue.assign_attributes(label_ids: label_ids)
 
-      result = service.execute
+        result = service.execute
 
-      expect(result[:merge_request].label_ids).to eq(label_ids)
-    end
+        expect(result[:merge_request].label_ids).to eq(label_ids)
+      end
 
-    it "inherits milestones" do
-      result = service.execute
+      it "inherits milestones" do
+        result = service.execute
 
-      expect(result[:merge_request].milestone_id).to eq(milestone_id)
-    end
+        expect(result[:merge_request].milestone_id).to eq(milestone_id)
+      end
 
-    it 'delegates the branch creation to CreateBranchService' do
-      expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original
+      it 'delegates the branch creation to CreateBranchService' do
+        expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original
 
-      service.execute
-    end
+        service.execute
+      end
 
-    it 'creates a branch based on issue title' do
-      service.execute
+      it 'creates a branch based on issue title' do
+        service.execute
 
-      expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
-    end
+        expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
+      end
 
-    it 'creates a branch using passed name' do
-      service_with_custom_source_branch.execute
+      it 'creates a branch using passed name' do
+        service_with_custom_source_branch.execute
 
-      expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy
-    end
+        expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy
+      end
 
-    it 'creates the new_merge_request system note' do
-      expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest))
+      it 'creates the new_merge_request system note' do
+        expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest))
 
-      service.execute
-    end
+        service.execute
+      end
 
-    it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
-      project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
+      it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
+        expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false)
 
-      expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name)
+        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name)
 
-      service.execute
-    end
+        service.execute
+      end
 
-    it 'creates a merge request' do
-      expect { service.execute }.to change(project.merge_requests, :count).by(1)
-    end
+      it 'creates a merge request' do
+        expect { service.execute }.to change(project.merge_requests, :count).by(1)
+      end
 
-    it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do
-      result = service.execute
+      it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do
+        result = service.execute
 
-      expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"")
-    end
+        expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"")
+      end
 
-    it 'sets the merge request author to current user' do
-      result = service.execute
+      it 'sets the merge request author to current user' do
+        result = service.execute
 
-      expect(result[:merge_request].author).to eq(user)
-    end
+        expect(result[:merge_request].author).to eq(user)
+      end
 
-    it 'sets the merge request source branch to the new issue branch' do
-      result = service.execute
+      it 'sets the merge request source branch to the new issue branch' do
+        result = service.execute
 
-      expect(result[:merge_request].source_branch).to eq(issue.to_branch_name)
-    end
+        expect(result[:merge_request].source_branch).to eq(issue.to_branch_name)
+      end
 
-    it 'sets the merge request source branch to the passed branch name' do
-      result = service_with_custom_source_branch.execute
+      it 'sets the merge request source branch to the passed branch name' do
+        result = service_with_custom_source_branch.execute
 
-      expect(result[:merge_request].source_branch).to eq(custom_source_branch)
-    end
+        expect(result[:merge_request].source_branch).to eq(custom_source_branch)
+      end
 
-    it 'sets the merge request target branch to the project default branch' do
-      result = service.execute
+      it 'sets the merge request target branch to the project default branch' do
+        result = service.execute
 
-      expect(result[:merge_request].target_branch).to eq(project.default_branch)
-    end
+        expect(result[:merge_request].target_branch).to eq(project.default_branch)
+      end
 
-    it 'executes quick actions if the build service sets them in the description' do
-      allow(service).to receive(:merge_request).and_wrap_original do |m, *args|
-        m.call(*args).tap do |merge_request|
-          merge_request.description = "/assign #{user.to_reference}"
+      it 'executes quick actions if the build service sets them in the description' do
+        allow(service).to receive(:merge_request).and_wrap_original do |m, *args|
+          m.call(*args).tap do |merge_request|
+            merge_request.description = "/assign #{user.to_reference}"
+          end
         end
+
+        result = service.execute
+
+        expect(result[:merge_request].assignees).to eq([user])
       end
 
-      result = service.execute
+      context 'when ref branch is set' do
+        subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute }
 
-      expect(result[:merge_request].assignees).to eq([user])
-    end
+        it 'sets the merge request source branch to the new issue branch' do
+          expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name)
+        end
 
-    context 'when ref branch is set' do
-      subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'feature').execute }
+        it 'sets the merge request target branch to the ref branch' do
+          expect(subject[:merge_request].target_branch).to eq('feature')
+        end
 
-      it 'sets the merge request source branch to the new issue branch' do
-        expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name)
+        context 'when ref branch does not exist' do
+          subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute }
+
+          it 'creates a merge request' do
+            expect { subject }.to change(project.merge_requests, :count).by(1)
+          end
+
+          it 'sets the merge request target branch to the project default branch' do
+            expect(subject[:merge_request].target_branch).to eq(project.default_branch)
+          end
+        end
       end
+    end
+
+    context 'target_project_id is specified' do
+      let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } }
+
+      context 'target project is not a fork of the project' do
+        let(:target_project) { create(:project, :repository) }
 
-      it 'sets the merge request target branch to the ref branch' do
-        expect(subject[:merge_request].target_branch).to eq('feature')
+        it 'does not create merge request' do
+          expect { service.execute }.to change(target_project.merge_requests, :count).by(0)
+        end
       end
 
-      context 'when ref branch does not exist' do
-        subject { described_class.new(project, user, issue_iid: issue.iid, ref: 'no-such-branch').execute }
+      context 'target project is a fork of project project' do
+        let(:target_project) { fork_project(project, user, repository: true) }
+
+        it 'creates a branch based on issue title' do
+          service.execute
+
+          expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
+        end
+
+        it 'creates a branch using passed name' do
+          service_with_custom_source_branch.execute
+
+          expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy
+        end
+
+        it 'creates the new_merge_request system note' do
+          expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest))
+
+          service.execute
+        end
+
+        it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
+          expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false)
+
+          expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name)
+
+          service.execute
+        end
 
         it 'creates a merge request' do
-          expect { subject }.to change(project.merge_requests, :count).by(1)
+          expect { service.execute }.to change(target_project.merge_requests, :count).by(1)
+        end
+
+        it 'sets the merge request title to: "WIP: $issue-branch-name' do
+          result = service.execute
+
+          expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}")
+        end
+
+        it 'sets the merge request author to current user' do
+          result = service.execute
+
+          expect(result[:merge_request].author).to eq(user)
+        end
+
+        it 'sets the merge request source branch to the new issue branch' do
+          result = service.execute
+
+          expect(result[:merge_request].source_branch).to eq(issue.to_branch_name)
+        end
+
+        it 'sets the merge request source branch to the passed branch name' do
+          result = service_with_custom_source_branch.execute
+
+          expect(result[:merge_request].source_branch).to eq(custom_source_branch)
         end
 
         it 'sets the merge request target branch to the project default branch' do
-          expect(subject[:merge_request].target_branch).to eq(project.default_branch)
+          result = service.execute
+
+          expect(result[:merge_request].target_branch).to eq(project.default_branch)
+        end
+
+        it 'executes quick actions if the build service sets them in the description' do
+          allow(service).to receive(:merge_request).and_wrap_original do |m, *args|
+            m.call(*args).tap do |merge_request|
+              merge_request.description = "/assign #{user.to_reference}"
+            end
+          end
+
+          result = service.execute
+
+          expect(result[:merge_request].assignees).to eq([user])
+        end
+
+        context 'when ref branch is set' do
+          subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute }
+
+          it 'sets the merge request source branch to the new issue branch' do
+            expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name)
+          end
+
+          it 'sets the merge request target branch to the ref branch' do
+            expect(subject[:merge_request].target_branch).to eq('feature')
+          end
+
+          context 'when ref branch does not exist' do
+            subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute }
+
+            it 'creates a merge request' do
+              expect { subject }.to change(target_project.merge_requests, :count).by(1)
+            end
+
+            it 'sets the merge request target branch to the project default branch' do
+              expect(subject[:merge_request].target_branch).to eq(target_project.default_branch)
+            end
+          end
         end
       end
     end
-- 
cgit v1.2.1


From ac3de494bde32e4ce13bd665d1de3132b84c002d Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Wed, 19 Jun 2019 16:56:17 +0800
Subject: Support branch creation from confidential issue

Accept a `confidential_issue_project_id` param which will
be used for the system note target.

This also includes some refactoring on the spec to use
shared examples.
---
 app/controllers/projects/branches_controller.rb    |  13 +-
 .../projects/branches_controller_spec.rb           |  69 ++++++++
 .../controllers/projects/issues_controller_spec.rb |  44 ++++-
 .../create_from_issue_service_spec.rb              | 177 ++++++---------------
 4 files changed, 170 insertions(+), 133 deletions(-)

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index fc708400657..df4002d075b 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -64,7 +64,7 @@ class Projects::BranchesController < Projects::ApplicationController
     success = (result[:status] == :success)
 
     if params[:issue_iid] && success
-      issue = IssuesFinder.new(current_user, project_id: @project.id).find_by(iid: params[:issue_iid])
+      issue = IssuesFinder.new(current_user, project_id: (confidential_issue_project || @project).id).find_by(iid: params[:issue_iid])
       SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue
     end
 
@@ -162,4 +162,15 @@ class Projects::BranchesController < Projects::ApplicationController
       @branches = Kaminari.paginate_array(@branches).page(params[:page])
     end
   end
+
+  def confidential_issue_project
+    return unless Feature.enabled?(:create_confidential_merge_request, @project)
+    return if params[:confidential_issue_project_id].blank?
+
+    confidential_issue_project = Project.find(params[:confidential_issue_project_id])
+
+    return unless can?(current_user, :push_code, confidential_issue_project)
+
+    confidential_issue_project
+  end
 end
diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb
index cf201c9f735..8c349b1e988 100644
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -103,6 +103,75 @@ describe Projects::BranchesController do
              }
       end
 
+      context 'confidential_issue_project_id is present' do
+        let(:confidential_issue_project) { create(:project) }
+
+        def create_branch_with_confidential_issue_project
+          post(
+            :create,
+            params: {
+              namespace_id: project.namespace,
+              project_id: project,
+              branch_name: branch,
+              confidential_issue_project_id: confidential_issue_project.id,
+              issue_iid: issue.iid
+            }
+          )
+        end
+
+        context 'create_confidential_merge_request feature is enabled' do
+          before do
+            stub_feature_flags(create_confidential_merge_request: true)
+          end
+
+          context 'user cannot push code to issue project' do
+            let(:issue) { create(:issue, project: confidential_issue_project) }
+
+            it 'does not post a system note' do
+              expect(SystemNoteService).not_to receive(:new_issue_branch)
+
+              create_branch_with_confidential_issue_project
+            end
+          end
+
+          context 'user can push code to issue project' do
+            before do
+              confidential_issue_project.add_developer(user)
+            end
+
+            context 'issue is under the specified project' do
+              let(:issue) { create(:issue, project: confidential_issue_project) }
+
+              it 'posts a system note' do
+                expect(SystemNoteService).to receive(:new_issue_branch).with(issue, confidential_issue_project, user, "1-feature-branch", branch_project: project)
+
+                create_branch_with_confidential_issue_project
+              end
+            end
+
+            context 'issue is not under the specified project' do
+              it 'does not post a system note' do
+                expect(SystemNoteService).not_to receive(:new_issue_branch)
+
+                create_branch_with_confidential_issue_project
+              end
+            end
+          end
+        end
+
+        context 'create_confidential_merge_request feature is disabled' do
+          before do
+            stub_feature_flags(create_confidential_merge_request: false)
+          end
+
+          it 'posts a system note on project' do
+            expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch", branch_project: project)
+
+            create_branch_with_confidential_issue_project
+          end
+        end
+      end
+
       context 'repository-less project' do
         let(:project) { create :project }
 
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index f82e3c8c7dc..bc5e0b4671e 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 describe Projects::IssuesController do
+  include ProjectForksHelper
+
   let(:project) { create(:project) }
   let(:user)    { create(:user) }
   let(:issue)   { create(:issue, project: project) }
@@ -1130,6 +1132,7 @@ describe Projects::IssuesController do
   end
 
   describe 'POST create_merge_request' do
+    let(:target_project_id) { nil }
     let(:project) { create(:project, :repository, :public) }
 
     before do
@@ -1163,13 +1166,42 @@ describe Projects::IssuesController do
       expect(response).to have_gitlab_http_status(404)
     end
 
+    context 'target_project_id is set' do
+      let(:target_project) { fork_project(project, user, repository: true) }
+      let(:target_project_id) { target_project.id }
+
+      context 'create_confidential_merge_request feature is enabled' do
+        before do
+          stub_feature_flags(create_confidential_merge_request: true)
+        end
+
+        it 'creates a new merge request' do
+          expect { create_merge_request }.to change(target_project.merge_requests, :count).by(1)
+        end
+      end
+
+      context 'create_confidential_merge_request feature is disabled' do
+        before do
+          stub_feature_flags(create_confidential_merge_request: false)
+        end
+
+        it 'creates a new merge request' do
+          expect { create_merge_request }.to change(project.merge_requests, :count).by(1)
+        end
+      end
+    end
+
     def create_merge_request
-      post :create_merge_request, params: {
-                                    namespace_id: project.namespace.to_param,
-                                    project_id: project.to_param,
-                                    id: issue.to_param
-                                  },
-                                  format: :json
+      post(
+        :create_merge_request,
+        params: {
+          namespace_id: project.namespace.to_param,
+          project_id: project.to_param,
+          id: issue.to_param,
+          target_project_id: target_project_id
+        },
+        format: :json
+      )
     end
   end
 
diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb
index 6b17b14d4b4..2c83b2851a1 100644
--- a/spec/services/merge_requests/create_from_issue_service_spec.rb
+++ b/spec/services/merge_requests/create_from_issue_service_spec.rb
@@ -13,15 +13,20 @@ describe MergeRequests::CreateFromIssueService do
   let(:custom_source_branch) { 'custom-source-branch' }
 
   subject(:service) { described_class.new(project, user, service_params) }
-  subject(:service_with_custom_source_branch) { described_class.new(project, user, service_params.merge(branch_name: custom_source_branch)) }
+  subject(:service_with_custom_source_branch) { described_class.new(project, user, branch_name: custom_source_branch, **service_params) }
 
   before do
     project.add_developer(user)
   end
 
   describe '#execute' do
-    context 'no target_project_id specified' do
-      let(:service_params) { { issue_iid: issue.iid } }
+    shared_examples_for 'a service that creates a merge request from an issue' do
+      it 'returns an error when user can not create merge request on target project' do
+        result = described_class.new(project, create(:user), service_params).execute
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Not allowed to create merge request')
+      end
 
       it 'returns an error with invalid issue iid' do
         result = described_class.new(project, user, issue_iid: -1).execute
@@ -30,40 +35,20 @@ describe MergeRequests::CreateFromIssueService do
         expect(result[:message]).to eq('Invalid issue iid')
       end
 
-      it "inherits labels" do
-        issue.assign_attributes(label_ids: label_ids)
-
-        result = service.execute
-
-        expect(result[:merge_request].label_ids).to eq(label_ids)
-      end
-
-      it "inherits milestones" do
-        result = service.execute
-
-        expect(result[:merge_request].milestone_id).to eq(milestone_id)
-      end
-
-      it 'delegates the branch creation to CreateBranchService' do
-        expect_any_instance_of(CreateBranchService).to receive(:execute).once.and_call_original
-
-        service.execute
-      end
-
       it 'creates a branch based on issue title' do
         service.execute
 
-        expect(project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
+        expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
       end
 
       it 'creates a branch using passed name' do
         service_with_custom_source_branch.execute
 
-        expect(project.repository.branch_exists?(custom_source_branch)).to be_truthy
+        expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy
       end
 
       it 'creates the new_merge_request system note' do
-        expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest))
+        expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest))
 
         service.execute
       end
@@ -71,19 +56,13 @@ describe MergeRequests::CreateFromIssueService do
       it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
         expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false)
 
-        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name)
+        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name)
 
         service.execute
       end
 
       it 'creates a merge request' do
-        expect { service.execute }.to change(project.merge_requests, :count).by(1)
-      end
-
-      it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do
-        result = service.execute
-
-        expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"")
+        expect { service.execute }.to change(target_project.merge_requests, :count).by(1)
       end
 
       it 'sets the merge request author to current user' do
@@ -107,7 +86,7 @@ describe MergeRequests::CreateFromIssueService do
       it 'sets the merge request target branch to the project default branch' do
         result = service.execute
 
-        expect(result[:merge_request].target_branch).to eq(project.default_branch)
+        expect(result[:merge_request].target_branch).to eq(target_project.default_branch)
       end
 
       it 'executes quick actions if the build service sets them in the description' do
@@ -123,7 +102,7 @@ describe MergeRequests::CreateFromIssueService do
       end
 
       context 'when ref branch is set' do
-        subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute }
+        subject { described_class.new(project, user, ref: 'feature', **service_params).execute }
 
         it 'sets the merge request source branch to the new issue branch' do
           expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name)
@@ -134,127 +113,73 @@ describe MergeRequests::CreateFromIssueService do
         end
 
         context 'when ref branch does not exist' do
-          subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute }
+          subject { described_class.new(project, user, ref: 'no-such-branch', **service_params).execute }
 
           it 'creates a merge request' do
-            expect { subject }.to change(project.merge_requests, :count).by(1)
+            expect { subject }.to change(target_project.merge_requests, :count).by(1)
           end
 
           it 'sets the merge request target branch to the project default branch' do
-            expect(subject[:merge_request].target_branch).to eq(project.default_branch)
+            expect(subject[:merge_request].target_branch).to eq(target_project.default_branch)
           end
         end
       end
     end
 
-    context 'target_project_id is specified' do
-      let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } }
-
-      context 'target project is not a fork of the project' do
-        let(:target_project) { create(:project, :repository) }
-
-        it 'does not create merge request' do
-          expect { service.execute }.to change(target_project.merge_requests, :count).by(0)
-        end
-      end
-
-      context 'target project is a fork of project project' do
-        let(:target_project) { fork_project(project, user, repository: true) }
-
-        it 'creates a branch based on issue title' do
-          service.execute
-
-          expect(target_project.repository.branch_exists?(issue.to_branch_name)).to be_truthy
-        end
-
-        it 'creates a branch using passed name' do
-          service_with_custom_source_branch.execute
-
-          expect(target_project.repository.branch_exists?(custom_source_branch)).to be_truthy
-        end
+    context 'no target_project_id specified' do
+      let(:service_params) { { issue_iid: issue.iid } }
+      let(:target_project) { project }
 
-        it 'creates the new_merge_request system note' do
-          expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest))
+      it_behaves_like 'a service that creates a merge request from an issue'
 
-          service.execute
-        end
+      it "inherits labels" do
+        issue.assign_attributes(label_ids: label_ids)
 
-        it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
-          expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false)
+        result = service.execute
 
-          expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name)
+        expect(result[:merge_request].label_ids).to eq(label_ids)
+      end
 
-          service.execute
-        end
+      it "inherits milestones" do
+        result = service.execute
 
-        it 'creates a merge request' do
-          expect { service.execute }.to change(target_project.merge_requests, :count).by(1)
-        end
+        expect(result[:merge_request].milestone_id).to eq(milestone_id)
+      end
 
-        it 'sets the merge request title to: "WIP: $issue-branch-name' do
-          result = service.execute
+      it 'sets the merge request title to: "WIP: Resolves "$issue-title"' do
+        result = service.execute
 
-          expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}")
-        end
+        expect(result[:merge_request].title).to eq("WIP: Resolve \"#{issue.title}\"")
+      end
+    end
 
-        it 'sets the merge request author to current user' do
-          result = service.execute
+    context 'target_project_id is specified' do
+      let(:service_params) { { issue_iid: issue.iid, target_project_id: target_project.id } }
 
-          expect(result[:merge_request].author).to eq(user)
-        end
+      context 'target project is not a fork of the project' do
+        let(:target_project) { create(:project, :repository) }
 
-        it 'sets the merge request source branch to the new issue branch' do
+        it 'returns an error about not finding the project' do
           result = service.execute
 
-          expect(result[:merge_request].source_branch).to eq(issue.to_branch_name)
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to eq('Project not found')
         end
 
-        it 'sets the merge request source branch to the passed branch name' do
-          result = service_with_custom_source_branch.execute
-
-          expect(result[:merge_request].source_branch).to eq(custom_source_branch)
+        it 'does not create merge request' do
+          expect { service.execute }.to change(target_project.merge_requests, :count).by(0)
         end
+      end
 
-        it 'sets the merge request target branch to the project default branch' do
-          result = service.execute
-
-          expect(result[:merge_request].target_branch).to eq(project.default_branch)
-        end
+      context 'target project is a fork of project project' do
+        let(:target_project) { fork_project(project, user, repository: true) }
 
-        it 'executes quick actions if the build service sets them in the description' do
-          allow(service).to receive(:merge_request).and_wrap_original do |m, *args|
-            m.call(*args).tap do |merge_request|
-              merge_request.description = "/assign #{user.to_reference}"
-            end
-          end
+        it_behaves_like 'a service that creates a merge request from an issue'
 
+        it 'sets the merge request title to: "WIP: $issue-branch-name' do
           result = service.execute
 
-          expect(result[:merge_request].assignees).to eq([user])
-        end
-
-        context 'when ref branch is set' do
-          subject { described_class.new(project, user, service_params.merge(ref: 'feature')).execute }
-
-          it 'sets the merge request source branch to the new issue branch' do
-            expect(subject[:merge_request].source_branch).to eq(issue.to_branch_name)
-          end
-
-          it 'sets the merge request target branch to the ref branch' do
-            expect(subject[:merge_request].target_branch).to eq('feature')
-          end
-
-          context 'when ref branch does not exist' do
-            subject { described_class.new(project, user, service_params.merge(ref: 'no-such-branch')).execute }
-
-            it 'creates a merge request' do
-              expect { subject }.to change(target_project.merge_requests, :count).by(1)
-            end
-
-            it 'sets the merge request target branch to the project default branch' do
-              expect(subject[:merge_request].target_branch).to eq(target_project.default_branch)
-            end
-          end
+          expect(result[:merge_request].title).to eq("WIP: #{issue.to_branch_name.titleize.humanize}")
         end
       end
     end
-- 
cgit v1.2.1


From 1ca5520bd6f3447ada3a1120d2a3bd445ab6746a Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Fri, 28 Jun 2019 14:08:26 -0800
Subject: Fix issues when creating system notes

When `confidential_issue_project_id` is set and the issue is
under that project, create the a note about branch creation
in that project. If not, do nothing.

When creating `new_merge_request` system note, set the project
where the MR will be referenced from so it'll be linked to when
the MR is created in another project.
---
 app/controllers/projects/branches_controller.rb    |  5 ++--
 .../merge_requests/create_from_issue_service.rb    |  4 +--
 app/services/system_note_service.rb                |  7 +++--
 .../projects/branches_controller_spec.rb           |  2 +-
 .../create_from_issue_service_spec.rb              |  4 +--
 spec/services/system_note_service_spec.rb          | 32 ++++++++++++++++------
 6 files changed, 36 insertions(+), 18 deletions(-)

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index df4002d075b..b3dfafb7b87 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -64,8 +64,9 @@ class Projects::BranchesController < Projects::ApplicationController
     success = (result[:status] == :success)
 
     if params[:issue_iid] && success
-      issue = IssuesFinder.new(current_user, project_id: (confidential_issue_project || @project).id).find_by(iid: params[:issue_iid])
-      SystemNoteService.new_issue_branch(issue, @project, current_user, branch_name) if issue
+      target_project = confidential_issue_project || @project
+      issue = IssuesFinder.new(current_user, project_id: target_project.id).find_by(iid: params[:issue_iid])
+      SystemNoteService.new_issue_branch(issue, target_project, current_user, branch_name, branch_project: @project) if issue
     end
 
     respond_to do |format|
diff --git a/app/services/merge_requests/create_from_issue_service.rb b/app/services/merge_requests/create_from_issue_service.rb
index 074a45e328b..2a217a6f689 100644
--- a/app/services/merge_requests/create_from_issue_service.rb
+++ b/app/services/merge_requests/create_from_issue_service.rb
@@ -25,11 +25,11 @@ module MergeRequests
       new_merge_request = create(merge_request)
 
       if new_merge_request.valid?
-        SystemNoteService.new_merge_request(issue, target_project, current_user, new_merge_request)
+        SystemNoteService.new_merge_request(issue, project, current_user, new_merge_request)
 
         success(new_merge_request)
       else
-        SystemNoteService.new_issue_branch(issue, target_project, current_user, branch_name)
+        SystemNoteService.new_issue_branch(issue, project, current_user, branch_name, branch_project: target_project)
 
         error(new_merge_request.errors)
       end
diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb
index 1390f7cdf46..8f7cfe582ca 100644
--- a/app/services/system_note_service.rb
+++ b/app/services/system_note_service.rb
@@ -404,8 +404,9 @@ module SystemNoteService
   # Example note text:
   #
   #   "created branch `201-issue-branch-button`"
-  def new_issue_branch(issue, project, author, branch)
-    link = url_helpers.project_compare_path(project, from: project.default_branch, to: branch)
+  def new_issue_branch(issue, project, author, branch, branch_project: nil)
+    branch_project ||= project
+    link = url_helpers.project_compare_path(branch_project, from: branch_project.default_branch, to: branch)
 
     body = "created branch [`#{branch}`](#{link}) to address this issue"
 
@@ -413,7 +414,7 @@ module SystemNoteService
   end
 
   def new_merge_request(issue, project, author, merge_request)
-    body = "created merge request #{merge_request.to_reference} to address this issue"
+    body = "created merge request #{merge_request.to_reference(project)} to address this issue"
 
     create_note(NoteSummary.new(issue, project, author, body, action: 'merge'))
   end
diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb
index 8c349b1e988..712c3fa0ffe 100644
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -92,7 +92,7 @@ describe Projects::BranchesController do
       end
 
       it 'posts a system note' do
-        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch")
+        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, "1-feature-branch", branch_project: project)
 
         post :create,
              params: {
diff --git a/spec/services/merge_requests/create_from_issue_service_spec.rb b/spec/services/merge_requests/create_from_issue_service_spec.rb
index 2c83b2851a1..0e0da6a13ab 100644
--- a/spec/services/merge_requests/create_from_issue_service_spec.rb
+++ b/spec/services/merge_requests/create_from_issue_service_spec.rb
@@ -48,7 +48,7 @@ describe MergeRequests::CreateFromIssueService do
       end
 
       it 'creates the new_merge_request system note' do
-        expect(SystemNoteService).to receive(:new_merge_request).with(issue, target_project, user, instance_of(MergeRequest))
+        expect(SystemNoteService).to receive(:new_merge_request).with(issue, project, user, instance_of(MergeRequest))
 
         service.execute
       end
@@ -56,7 +56,7 @@ describe MergeRequests::CreateFromIssueService do
       it 'creates the new_issue_branch system note when the branch could be created but the merge_request cannot be created' do
         expect_any_instance_of(MergeRequest).to receive(:valid?).at_least(:once).and_return(false)
 
-        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, target_project, user, issue.to_branch_name)
+        expect(SystemNoteService).to receive(:new_issue_branch).with(issue, project, user, issue.to_branch_name, branch_project: target_project)
 
         service.execute
       end
diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb
index 93fe3290d8b..97377c2f560 100644
--- a/spec/services/system_note_service_spec.rb
+++ b/spec/services/system_note_service_spec.rb
@@ -454,16 +454,32 @@ describe SystemNoteService do
   end
 
   describe '.new_issue_branch' do
-    subject { described_class.new_issue_branch(noteable, project, author, "1-mepmep") }
+    let(:branch) { '1-mepmep' }
 
-    it_behaves_like 'a system note' do
-      let(:action) { 'branch' }
-    end
+    subject { described_class.new_issue_branch(noteable, project, author, branch, branch_project: branch_project) }
 
-    context 'when a branch is created from the new branch button' do
-      it 'sets the note text' do
-        expect(subject.note).to start_with("created branch [`1-mepmep`]")
+    shared_examples_for 'a system note for new issue branch' do
+      it_behaves_like 'a system note' do
+        let(:action) { 'branch' }
       end
+
+      context 'when a branch is created from the new branch button' do
+        it 'sets the note text' do
+          expect(subject.note).to start_with("created branch [`#{branch}`]")
+        end
+      end
+    end
+
+    context 'branch_project is set' do
+      let(:branch_project) { create(:project, :repository) }
+
+      it_behaves_like 'a system note for new issue branch'
+    end
+
+    context 'branch_project is not set' do
+      let(:branch_project) { nil }
+
+      it_behaves_like 'a system note for new issue branch'
     end
   end
 
@@ -477,7 +493,7 @@ describe SystemNoteService do
     end
 
     it 'sets the new merge request note text' do
-      expect(subject.note).to eq("created merge request #{merge_request.to_reference} to address this issue")
+      expect(subject.note).to eq("created merge request #{merge_request.to_reference(project)} to address this issue")
     end
   end
 
-- 
cgit v1.2.1


From 6b68acbfe9db1d3c855d7505817ebca62e3a61c1 Mon Sep 17 00:00:00 2001
From: Patrick Bajao <ebajao@gitlab.com>
Date: Mon, 1 Jul 2019 16:23:23 +0800
Subject: Check if user can `update_issue` on project

If user can update an issue under the specified confidential
issue project, should be able to find the project.
---
 app/controllers/projects/branches_controller.rb       | 2 +-
 spec/controllers/projects/branches_controller_spec.rb | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/controllers/projects/branches_controller.rb b/app/controllers/projects/branches_controller.rb
index b3dfafb7b87..5e50801eb23 100644
--- a/app/controllers/projects/branches_controller.rb
+++ b/app/controllers/projects/branches_controller.rb
@@ -170,7 +170,7 @@ class Projects::BranchesController < Projects::ApplicationController
 
     confidential_issue_project = Project.find(params[:confidential_issue_project_id])
 
-    return unless can?(current_user, :push_code, confidential_issue_project)
+    return unless can?(current_user, :update_issue, confidential_issue_project)
 
     confidential_issue_project
   end
diff --git a/spec/controllers/projects/branches_controller_spec.rb b/spec/controllers/projects/branches_controller_spec.rb
index 712c3fa0ffe..dbc8681eb49 100644
--- a/spec/controllers/projects/branches_controller_spec.rb
+++ b/spec/controllers/projects/branches_controller_spec.rb
@@ -124,7 +124,7 @@ describe Projects::BranchesController do
             stub_feature_flags(create_confidential_merge_request: true)
           end
 
-          context 'user cannot push code to issue project' do
+          context 'user cannot update issue' do
             let(:issue) { create(:issue, project: confidential_issue_project) }
 
             it 'does not post a system note' do
@@ -134,9 +134,9 @@ describe Projects::BranchesController do
             end
           end
 
-          context 'user can push code to issue project' do
+          context 'user can update issue' do
             before do
-              confidential_issue_project.add_developer(user)
+              confidential_issue_project.add_reporter(user)
             end
 
             context 'issue is under the specified project' do
-- 
cgit v1.2.1