Fix 404 when upstream has disabled merge requests

Author: mhasbini

app/helpers/merge_requests_helper.rb

@@ -1,6 +1,6 @@
 module MergeRequestsHelper
   def new_mr_path_from_push_event(event)
-    target_project = event.project.forked_from_project || event.project
+    target_project = event.project.default_merge_request_target
     new_namespace_project_merge_request_path(
       event.project.namespace,
       event.project,
@@ -127,6 +127,10 @@ def format_mr_branch_names(merge_request)
     end
   end
 
+  def target_projects(project)
+    [project, project.default_merge_request_target].uniq
+  end
+
   def merge_request_button_visibility(merge_request, closed)
     return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?) || merge_request.closed_without_fork?
   end

app/models/merge_request.rb

@@ -100,6 +100,7 @@ class MergeRequest < ActiveRecord::Base
   validates :merge_user, presence: true, if: :merge_when_pipeline_succeeds?, unless: :importing?
   validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
   validate :validate_fork, unless: :closed_without_fork?
+  validate :validate_target_project, on: :create
 
   scope :by_source_or_target_branch, ->(branch_name) do
     where("source_branch = :branch OR target_branch = :branch", branch: branch_name)
@@ -330,6 +331,12 @@ def validate_branches
     end
   end
 
+  def validate_target_project
+    return true if target_project.merge_requests_enabled?
+
+    errors.add :base, 'Target project has disabled merge requests'
+  end
+
   def validate_fork
     return true unless target_project && source_project
     return true if target_project == source_project

app/models/project.rb

@@ -1314,6 +1314,14 @@ def parent_changed?
     namespace_id_changed?
   end
 
+  def default_merge_request_target
+    if forked_from_project&.merge_requests_enabled?
+      forked_from_project
+    else
+      self
+    end
+  end
+
   alias_method :name_with_namespace, :full_name
   alias_method :human_name, :full_name
   alias_method :path_with_namespace, :full_path

app/services/merge_requests/build_service.rb

@@ -28,7 +28,7 @@ def find_source_project
 
     def find_target_project
       return target_project if target_project.present? && can?(current_user, :read_project, target_project)
-      project.forked_from_project || project
+      project.default_merge_request_target
     end
 
     def find_target_branch

app/views/projects/merge_requests/_new_compare.html.haml

@@ -38,7 +38,7 @@
         .panel-heading
           Target branch
         .panel-body.clearfix
-          - projects =  @project.forked_from_project.nil? ? [@project] : [@project, @project.forked_from_project]
+          - projects = target_projects(@project)
           .merge-request-select.dropdown
             = f.hidden_field :target_project_id
             = dropdown_toggle f.object.target_project.path_with_namespace, { toggle: "dropdown", field_name: "#{f.object_name}[target_project_id]", disabled: @merge_request.persisted? }, { toggle_class: "js-compare-dropdown js-target-project" }

changelogs/unreleased/26488-target-disabled-mr.yml

@@ -0,0 +1,4 @@
+---
+title: Disallow merge requests from fork when source project have disabled merge requests
+merge_request:
+author: mhasbini

lib/api/merge_requests.rb

@@ -20,6 +20,8 @@ def handle_merge_request_errors!(errors)
             error!(errors[:validate_fork], 422)
           elsif errors[:validate_branches].any?
             conflict!(errors[:validate_branches])
+          elsif errors[:base].any?
+            error!(errors[:base], 422)
           end
 
           render_api_error!(errors, 400)

lib/api/v3/merge_requests.rb

@@ -23,6 +23,8 @@ def handle_merge_request_errors!(errors)
               error!(errors[:validate_fork], 422)
             elsif errors[:validate_branches].any?
               conflict!(errors[:validate_branches])
+            elsif errors[:base].any?
+              error!(errors[:base], 422)
             end
 
             render_api_error!(errors, 400)

spec/helpers/merge_requests_helper_spec.rb

@@ -64,7 +64,7 @@
 
       it do
         @project = project
-        
+
         is_expected.to eq("#1, #2, and #{other_project.namespace.path}/#{other_project.path}#3")
       end
     end
@@ -149,6 +149,50 @@
     end
   end
 
+  describe '#target_projects' do
+    let(:project) { create(:empty_project) }
+    let(:fork_project) { create(:empty_project, forked_from_project: project) }
+
+    context 'when target project has enabled merge requests' do
+      it 'returns the forked_from project' do
+        expect(target_projects(fork_project)).to contain_exactly(project, fork_project)
+      end
+    end
+
+    context 'when target project has disabled merge requests' do
+      it 'returns the forked project' do
+        project.project_feature.update(merge_requests_access_level: 0)
+
+        expect(target_projects(fork_project)).to contain_exactly(fork_project)
+      end
+    end
+  end
+
+  describe '#new_mr_path_from_push_event' do
+    subject(:url_params) { URI.decode_www_form(new_mr_path_from_push_event(event)).to_h }
+    let(:user) { create(:user) }
+    let(:project) { create(:empty_project, creator: user) }
+    let(:fork_project) { create(:project, forked_from_project: project, creator: user) }
+    let(:event) do
+      push_data = Gitlab::DataBuilder::Push.build_sample(fork_project, user)
+      create(:event, :pushed, project: fork_project, target: fork_project, author: user, data: push_data)
+    end
+
+    context 'when target project has enabled merge requests' do
+      it 'returns link to create merge request on source project' do
+        expect(url_params['merge_request[target_project_id]'].to_i).to eq(project.id)
+      end
+    end
+
+    context 'when target project has disabled merge requests' do
+      it 'returns link to create merge request on forked project' do
+        project.project_feature.update(merge_requests_access_level: 0)
+
+        expect(url_params['merge_request[target_project_id]'].to_i).to eq(fork_project.id)
+      end
+    end
+  end
+
   describe '#mr_issues_mentioned_but_not_closing' do
     let(:user_1) { create(:user) }
     let(:user_2) { create(:user) }

spec/requests/api/merge_requests_spec.rb

@@ -434,6 +434,19 @@
         expect(json_response['title']).to eq('Test merge_request')
       end
 
+      it 'returns 422 when target project has disabled merge requests' do
+        project.project_feature.update(merge_requests_access_level: 0)
+
+        post api("/projects/#{fork_project.id}/merge_requests", user2),
+             title: 'Test',
+             target_branch: 'master',
+             source_branch: 'markdown',
+             author: user2,
+             target_project_id: project.id
+
+        expect(response).to have_http_status(422)
+      end
+
       it "returns 400 when source_branch is missing" do
         post api("/projects/#{fork_project.id}/merge_requests", user2),
         title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id

spec/requests/api/v3/merge_requests_spec.rb

@@ -338,6 +338,19 @@
         expect(json_response['title']).to eq('Test merge_request')
       end
 
+      it "returns 422 when target project has disabled merge requests" do
+        project.project_feature.update(merge_requests_access_level: 0)
+
+        post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
+             title: 'Test',
+             target_branch: "master",
+             source_branch: 'markdown',
+             author: user2,
+             target_project_id: project.id
+
+        expect(response).to have_http_status(422)
+      end
+
       it "returns 400 when source_branch is missing" do
         post v3_api("/projects/#{fork_project.id}/merge_requests", user2),
         title: 'Test merge_request', target_branch: "master", author: user2, target_project_id: project.id

spec/services/merge_requests/build_service_spec.rb

@@ -261,6 +261,16 @@
       end
     end
 
+    context 'upstream project has disabled merge requests' do
+      let(:upstream_project) { create(:empty_project, :merge_requests_disabled) }
+      let(:project) { create(:empty_project, forked_from_project: upstream_project) }
+      let(:commits) { Commit.decorate([commit_1], project) }
+
+      it 'sets target project correctly' do
+        expect(merge_request.target_project).to eq(project)
+      end
+    end
+
     context 'target_project is set and accessible by current_user' do
       let(:target_project) { create(:project, :public, :repository)}
       let(:commits) { Commit.decorate([commit_1], project) }