Merge branch '14615-compare-across-forks' into 'master'

Allow comparisons of branches across projects [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!51985

app/controllers/projects/compare_controller.rb

@@ -6,6 +6,7 @@ class Projects::CompareController < Projects::ApplicationController
   include DiffForPath
   include DiffHelper
   include RendersCommits
+  include CompareHelper
 
   # Authorize
   before_action :require_non_empty_project
@@ -37,16 +38,18 @@ class Projects::CompareController < Projects::ApplicationController
   end
 
   def create
-    if params[:from].blank? || params[:to].blank?
+    from_to_vars = {
+      from: params[:from].presence,
+      to: params[:to].presence,
+      from_project_id: params[:from_project_id].presence
+    }
+
+    if from_to_vars[:from].blank? || from_to_vars[:to].blank?
       flash[:alert] = "You must select a Source and a Target revision"
-      from_to_vars = {
-        from: params[:from].presence,
-        to: params[:to].presence
-      }
-      redirect_to project_compare_index_path(@project, from_to_vars)
+
+      redirect_to project_compare_index_path(source_project, from_to_vars)
     else
-      redirect_to project_compare_path(@project,
-                                               params[:from], params[:to])
+      redirect_to project_compare_path(source_project, from_to_vars)
     end
   end
 
@@ -73,13 +76,34 @@ class Projects::CompareController < Projects::ApplicationController
     return if valid.all?
 
     flash[:alert] = "Invalid branch name"
-    redirect_to project_compare_index_path(@project)
+    redirect_to project_compare_index_path(source_project)
+  end
+
+  # target == start_ref == from
+  def target_project
+    strong_memoize(:target_project) do
+      next source_project unless params.key?(:from_project_id)
+      next source_project unless Feature.enabled?(:compare_repo_dropdown, source_project, default_enabled: :yaml)
+      next source_project if params[:from_project_id].to_i == source_project.id
+
+      target_project = target_projects(source_project).find_by_id(params[:from_project_id])
+
+      # Just ignore the field if it points at a non-existent or hidden project
+      next source_project unless target_project && can?(current_user, :download_code, target_project)
+
+      target_project
+    end
+  end
+
+  # source == head_ref == to
+  def source_project
+    project
   end
 
   def compare
     return @compare if defined?(@compare)
 
-    @compare = CompareService.new(@project, head_ref).execute(@project, start_ref)
+    @compare = CompareService.new(source_project, head_ref).execute(target_project, start_ref)
   end
 
   def start_ref
@@ -102,9 +126,9 @@ class Projects::CompareController < Projects::ApplicationController
 
   def define_environment
     if compare
-      environment_params = @repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit }
+      environment_params = source_project.repository.branch_exists?(head_ref) ? { ref: head_ref } : { commit: compare.commit }
       environment_params[:find_latest] = true
-      @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last
+      @environment = EnvironmentsFinder.new(source_project, current_user, environment_params).execute.last
     end
   end
 
@@ -114,8 +138,8 @@ class Projects::CompareController < Projects::ApplicationController
 
   # rubocop: disable CodeReuse/ActiveRecord
   def merge_request
-    @merge_request ||= MergeRequestsFinder.new(current_user, project_id: @project.id).execute.opened
-      .find_by(source_project: @project, source_branch: head_ref, target_branch: start_ref)
+    @merge_request ||= MergeRequestsFinder.new(current_user, project_id: target_project.id).execute.opened
+      .find_by(source_project: source_project, source_branch: head_ref, target_branch: start_ref)
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

app/finders/merge_request_target_project_finder.rb

@@ -5,29 +5,30 @@ class MergeRequestTargetProjectFinder
 
   attr_reader :current_user, :source_project
 
-  def initialize(current_user: nil, source_project:)
+  def initialize(current_user: nil, source_project:, project_feature: :merge_requests)
     @current_user = current_user
     @source_project = source_project
+    @project_feature = project_feature
   end
 
-  # rubocop: disable CodeReuse/ActiveRecord
   def execute(include_routes: false)
     if source_project.fork_network
       include_routes ? projects.inc_routes : projects
     else
-      Project.where(id: source_project)
+      Project.id_in(source_project.id)
     end
   end
-  # rubocop: enable CodeReuse/ActiveRecord
 
   private
 
+  attr_reader :project_feature
+
   def projects
     source_project
       .fork_network
       .projects
       .public_or_visible_to_user(current_user)
       .non_archived
-      .with_feature_available_for_user(:merge_requests, current_user)
+      .with_feature_available_for_user(project_feature, current_user)
   end
 end

app/helpers/compare_helper.rb

 # frozen_string_literal: true
 
 module CompareHelper
-  def create_mr_button?(from = params[:from], to = params[:to], project = @project)
+  def create_mr_button?(from: params[:from], to: params[:to], source_project: @project, target_project: @target_project)
     from.present? &&
       to.present? &&
       from != to &&
-      can?(current_user, :create_merge_request_from, project) &&
-      project.repository.branch_exists?(from) &&
-      project.repository.branch_exists?(to)
+      can?(current_user, :create_merge_request_from, source_project) &&
+      can?(current_user, :create_merge_request_in, target_project) &&
+      target_project.repository.branch_exists?(from) &&
+      source_project.repository.branch_exists?(to)
   end
 
-  def create_mr_path(from = params[:from], to = params[:to], project = @project)
+  def create_mr_path(from: params[:from], to: params[:to], source_project: @project, target_project: @target_project)
     project_new_merge_request_path(
-      project,
+      target_project,
       merge_request: {
+        source_project_id: source_project.id,
         source_branch: to,
+        target_project_id: target_project.id,
         target_branch: from
       }
     )
   end
+
+  def target_projects(source_project)
+    MergeRequestTargetProjectFinder
+      .new(current_user: current_user, source_project: source_project, project_feature: :repository)
+      .execute(include_routes: true)
+  end
 end

app/views/events/event/_push.html.haml

@@ -21,7 +21,8 @@
     %ul.content-list.event_commits
       = render "events/commit", project: project, event: event
 
-      - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user)
+      - create_mr = event.new_ref? && create_mr_button?(from: project.default_branch, to: event.ref_name, source_project: project, target_project: project) && event.authored_by?(current_user)
+      - create_mr_path = create_mr_path(from: project.default_branch, to: event.ref_name, source_project: project, target_project: project) if create_mr
       - if event.commits_count > 1
         %li.commits-stat
           %span ... and #{pluralize(event.commits_count - 1, 'more commit')}.
@@ -40,9 +41,9 @@
           - if create_mr
             %span
               or
-              = link_to create_mr_path(project.default_branch, event.ref_name, project) do
+              = link_to create_mr_path do
                 create a merge request
       - elsif create_mr
         %li.commits-stat
-          = link_to create_mr_path(project.default_branch, event.ref_name, project) do
+          = link_to create_mr_path do
             Create Merge Request

app/views/projects/branches/_branch.html.haml

@@ -35,8 +35,8 @@
       .gl-display-inline-flex.gl-vertical-align-middle.gl-mr-5
         %svg.s24
 
-    - if merge_project && create_mr_button?(@repository.root_ref, branch.name)
-      = link_to create_mr_path(@repository.root_ref, branch.name), class: 'gl-button btn btn-default' do
+    - if merge_project && create_mr_button?(from: @repository.root_ref, to: branch.name, source_project: @project, target_project: @project)
+      = link_to create_mr_path(from: @repository.root_ref, to: branch.name, source_project: @project, target_project: @project), class: 'gl-button btn btn-default' do
         = _('Merge request')
 
     - if branch.name != @repository.root_ref

app/views/projects/commits/show.html.haml

@@ -18,9 +18,9 @@
         - if @merge_request.present?
           .control.d-none.d-md-block
             = link_to _("View open merge request"), project_merge_request_path(@project, @merge_request), class: 'btn gl-button'
-        - elsif create_mr_button?(@repository.root_ref, @ref)
+        - elsif create_mr_button?(from: @repository.root_ref, to: @ref, source_project: @project, target_project: @project)
           .control.d-none.d-md-block
-            = link_to _("Create merge request"), create_mr_path(@repository.root_ref, @ref), class: 'btn gl-button btn-success'
+            = link_to _("Create merge request"), create_mr_path(from: @repository.root_ref, to: @ref, source_project: @project, target_project: @project), class: 'btn gl-button btn-success'
 
         .control
           = form_tag(project_commits_path(@project, @id), method: :get, class: 'commits-search-form js-signature-container', data: { 'signatures-path' => namespace_project_signatures_path }) do

config/feature_flags/development/compare_repo_dropdown.yml

+---
+name: compare_repo_dropdown
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/issues/14615
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/322141
+milestone: '13.9'
+type: development
+group: group::source code
+default_enabled: false

spec/features/projects/merge_request_button_spec.rb

@@ -3,11 +3,13 @@
 require 'spec_helper'
 
 RSpec.describe 'Merge Request button' do
-  shared_examples 'Merge request button only shown when allowed' do
-    let(:user) { create(:user) }
-    let(:project) { create(:project, :public, :repository) }
-    let(:forked_project) { create(:project, :public, :repository, forked_from_project: project) }
+  include ProjectForksHelper
+
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let(:forked_project) { fork_project(project, user, repository: true) }
 
+  shared_examples 'Merge request button only shown when allowed' do
     context 'not logged in' do
       it 'does not show Create merge request button' do
         visit url
@@ -23,9 +25,15 @@ RSpec.describe 'Merge Request button' do
       end
 
       it 'shows Create merge request button' do
-        href = project_new_merge_request_path(project,
-                                              merge_request: { source_branch: 'feature',
-                                                               target_branch: 'master' })
+        href = project_new_merge_request_path(
+          project,
+          merge_request: {
+            source_project_id: project.id,
+            source_branch: 'feature',
+            target_project_id: project.id,
+            target_branch: 'master'
+          }
+        )
 
         visit url
 
@@ -75,12 +83,16 @@ RSpec.describe 'Merge Request button' do
       end
 
       context 'on own fork of project' do
-        let(:user) { forked_project.owner }
-
         it 'shows Create merge request button' do
-          href = project_new_merge_request_path(forked_project,
-                                                merge_request: { source_branch: 'feature',
-                                                                 target_branch: 'master' })
+          href = project_new_merge_request_path(
+            forked_project,
+            merge_request: {
+              source_project_id: forked_project.id,
+              source_branch: 'feature',
+              target_project_id: forked_project.id,
+              target_branch: 'master'
+            }
+          )
 
           visit fork_url
 
@@ -101,11 +113,33 @@ RSpec.describe 'Merge Request button' do
   end
 
   context 'on compare page' do
+    let(:label) { 'Create merge request' }
+
     it_behaves_like 'Merge request button only shown when allowed' do
-      let(:label) { 'Create merge request' }
       let(:url) { project_compare_path(project, from: 'master', to: 'feature') }
       let(:fork_url) { project_compare_path(forked_project, from: 'master', to: 'feature') }
     end
+
+    it 'shows the correct merge request button when viewing across forks' do
+      sign_in(user)
+      project.add_developer(user)
+
+      href = project_new_merge_request_path(
+        project,
+        merge_request: {
+          source_project_id: forked_project.id,
+          source_branch: 'feature',
+          target_project_id: project.id,
+          target_branch: 'master'
+        }
+      )
+
+      visit project_compare_path(forked_project, from: 'master', to: 'feature', from_project_id: project.id)
+
+      within("#content-body") do
+        expect(page).to have_link(label, href: href)
+      end
+    end
   end
 
   context 'on commits page' do

spec/finders/merge_request_target_project_finder_spec.rb

@@ -16,13 +16,22 @@ RSpec.describe MergeRequestTargetProjectFinder do
       expect(finder.execute).to contain_exactly(base_project, other_fork, forked_project)
     end
 
-    it 'does not include projects that have merge requests turned off' do
+    it 'does not include projects that have merge requests turned off by default' do
       other_fork.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
       base_project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
 
       expect(finder.execute).to contain_exactly(forked_project)
     end
 
+    it 'includes projects that have merge requests turned off by default with a more-permissive project feature' do
+      finder = described_class.new(current_user: user, source_project: forked_project, project_feature: :repository)
+
+      other_fork.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
+      base_project.project_feature.update!(merge_requests_access_level: ProjectFeature::DISABLED)
+
+      expect(finder.execute).to contain_exactly(base_project, other_fork, forked_project)
+    end
+
     it 'does not contain archived projects' do
       base_project.update!(archived: true)