User can edit closed MR with deleted fork

Add test for closed MR without fork

Add view test visibility of Reopen and Close buttons

Fix controller tests and validation method

Fix missing space

Remove unused variables from test

closed_without_fork? method refactoring

Add information about missing fork

When closed MR without fork can't edit target branch

Tests for closed MR edit view

Fix indentation and rebase, refactoring

CHANGELOG

@@ -223,6 +223,7 @@ v 8.10.6
   - Restore "Largest repository" sort option on Admin > Projects page. !5797
   - Fix privilege escalation via project export.
   - Require administrator privileges to perform a project import.
+  - User can edit closed MR with deleted fork (Katarzyna Kobierska Ula Budziszewska) !5496
 
 v 8.10.5
   - Add a data migration to fix some missing timestamps in the members table. !5670

app/helpers/merge_requests_helper.rb

@@ -98,6 +98,6 @@ module MergeRequestsHelper
   end
 
   def merge_request_button_visibility(merge_request, closed)
-    return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?)
+    return 'hidden' if merge_request.closed? == closed || (merge_request.merged? == closed && !merge_request.closed?) || merge_request.closed_without_fork?
   end
 end

app/models/merge_request.rb

@@ -91,13 +91,13 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  validates :source_project, presence: true, unless: [:allow_broken, :importing?]
+  validates :source_project, presence: true, unless: [:allow_broken, :importing?, :closed_without_fork?]
   validates :source_branch, presence: true
   validates :target_project, presence: true
   validates :target_branch, presence: true
   validates :merge_user, presence: true, if: :merge_when_build_succeeds?
-  validate :validate_branches, unless: [:allow_broken, :importing?]
-  validate :validate_fork
+  validate :validate_branches, unless: [:allow_broken, :importing?, :closed_without_fork?]
+  validate :validate_fork, unless: :closed_without_fork?
 
   scope :by_branch, ->(branch_name) { where("(source_branch LIKE :branch) OR (target_branch LIKE :branch)", branch: branch_name) }
   scope :cared, ->(user) { where('assignee_id = :user OR author_id = :user', user: user.id) }
@@ -305,19 +305,22 @@ class MergeRequest < ActiveRecord::Base
 
   def validate_fork
     return true unless target_project && source_project
+    return true if target_project == source_project
+    return true unless fork_missing?
 
-    if target_project == source_project
-      true
-    else
-      # If source and target projects are different
-      # we should check if source project is actually a fork of target project
-      if source_project.forked_from?(target_project)
-        true
-      else
-        errors.add :validate_fork,
-                   'Source project is not a fork of target project'
-      end
-    end
+    errors.add :validate_fork,
+               'Source project is not a fork of target project'
+  end
+
+  def closed_without_fork?
+    closed? && fork_missing?
+  end
+
+  def fork_missing?
+    return false unless for_fork?
+    return true unless source_project
+
+    !source_project.forked_from?(target_project)
   end
 
   def ensure_merge_request_diff

app/services/merge_requests/update_service.rb

@@ -11,6 +11,10 @@ module MergeRequests
       params.except!(:target_project_id)
       params.except!(:source_branch)
 
+      if merge_request.closed_without_fork?
+        params.except!(:target_branch, :force_remove_source_branch)
+      end
+
       merge_request.merge_params['force_remove_source_branch'] = params.delete(:force_remove_source_branch)
 
       update(merge_request)

app/views/projects/merge_requests/show/_mr_title.html.haml

@@ -1,3 +1,7 @@
+- if @merge_request.closed_without_fork?
+  .alert.alert-danger
+    %p Source project is not a fork of the target project
+
 .clearfix.detail-page-header
   .issuable-header
     .issuable-status-box.status-box{ class: status_box_class(@merge_request) }

app/views/shared/issuable/_form.html.haml

@@ -135,28 +135,29 @@
         = icon('question-circle')
 
 - if issuable.is_a?(MergeRequest)
-  %hr
-  - if @merge_request.new_record?
+  - unless @merge_request.closed_without_fork?
+    %hr
+    - if @merge_request.new_record?
+      .form-group
+        = f.label :source_branch, class: 'control-label'
+        .col-sm-10
+          .issuable-form-select-holder
+            = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true })
     .form-group
-      = f.label :source_branch, class: 'control-label'
+      = f.label :target_branch, class: 'control-label'
       .col-sm-10
         .issuable-form-select-holder
-          = f.select(:source_branch, [@merge_request.source_branch], { }, { class: 'source_branch select2 span2', disabled: true })
-  .form-group
-    = f.label :target_branch, class: 'control-label'
-    .col-sm-10
-      .issuable-form-select-holder
-        = f.select(:target_branch, @merge_request.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: @merge_request.new_record?, data: {placeholder: "Select branch"} })
-      - if @merge_request.new_record?
-         
-        = link_to 'Change branches', mr_change_branches_path(@merge_request)
-  - if @merge_request.can_remove_source_branch?(current_user)
-    .form-group
-      .col-sm-10.col-sm-offset-2
-        .checkbox
-          = label_tag 'merge_request[force_remove_source_branch]' do
-            = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
-            Remove source branch when merge request is accepted.
+          = f.select(:target_branch, @merge_request.target_branches, { include_blank: true }, { class: 'target_branch select2 span2', disabled: @merge_request.new_record?, data: {placeholder: "Select branch"} })
+        - if @merge_request.new_record?
+           
+          = link_to 'Change branches', mr_change_branches_path(@merge_request)
+    - if @merge_request.can_remove_source_branch?(current_user)
+      .form-group
+        .col-sm-10.col-sm-offset-2
+          .checkbox
+            = label_tag 'merge_request[force_remove_source_branch]' do
+              = check_box_tag 'merge_request[force_remove_source_branch]', '1', @merge_request.force_remove_source_branch?
+              Remove source branch when merge request is accepted.
 
 - is_footer = !(issuable.is_a?(MergeRequest) && issuable.new_record?)
 .row-content-block{class: (is_footer ? "footer-block" : "middle-block")}
@@ -175,7 +176,7 @@
     = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable.class]), class: 'btn btn-cancel'
   - else
     .pull-right
-      - if current_user.can?(:"destroy_#{issuable.to_ability_name}", @project)
+      - if can?(current_user, :"destroy_#{issuable.to_ability_name}", @project)
         = link_to 'Delete', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), data: { confirm: "#{issuable.class.name.titleize} will be removed! Are you sure?" },
                                                                                                   method: :delete, class: 'btn btn-danger btn-grouped'
       = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -170,6 +170,35 @@ describe Projects::MergeRequestsController do
         expect(response).to redirect_to([merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request])
         expect(merge_request.reload.closed?).to be_truthy
       end
+
+      it 'allow to edit closed MR' do
+        merge_request.close!
+
+        put :update,
+            namespace_id: project.namespace.path,
+            project_id: project.path,
+            id: merge_request.iid,
+            merge_request: {
+              title: 'New title'
+            }
+
+        expect(response).to redirect_to([merge_request.target_project.namespace.becomes(Namespace), merge_request.target_project, merge_request])
+        expect(merge_request.reload.title).to eq 'New title'
+      end
+
+      it 'does not allow to update target branch closed MR' do
+        merge_request.close!
+
+        put :update,
+            namespace_id: project.namespace.path,
+            project_id: project.path,
+            id: merge_request.iid,
+            merge_request: {
+              target_branch: 'new_branch'
+            }
+
+        expect { merge_request.reload.target_branch }.not_to change { merge_request.target_branch }
+      end
     end
   end
 

spec/models/merge_request_spec.rb

@@ -962,4 +962,68 @@ describe MergeRequest, models: true do
       expect(merge_request.conflicts_can_be_resolved_in_ui?).to be_truthy
     end
   end
+
+  describe "#fork_missing?" do
+    let(:project)      { create(:project) }
+    let(:fork_project) { create(:project, forked_from_project: project) }
+    let(:user)         { create(:user) }
+    let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
+
+    context "when fork exists" do
+      let(:merge_request) do
+        create(:merge_request,
+          source_project: fork_project,
+          target_project: project)
+      end
+
+      it { expect(merge_request.fork_missing?).to be_falsey }
+    end
+
+    context "when source project is the target project" do
+      let(:merge_request) { create(:merge_request, source_project: project) }
+
+      it { expect(merge_request.fork_missing?).to be_falsey }
+    end
+
+    context "when fork does not exist" do
+      let(:merge_request) do
+        create(:merge_request,
+          source_project: fork_project,
+          target_project: project)
+      end
+
+      it do
+        unlink_project.execute
+        merge_request.reload
+
+        expect(merge_request.fork_missing?).to be_truthy
+      end
+    end
+  end
+
+  describe "#closed_without_fork?" do
+    let(:project)      { create(:project) }
+    let(:fork_project) { create(:project, forked_from_project: project) }
+    let(:user)         { create(:user) }
+    let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
+
+    context "closed MR" do
+      let(:closed_merge_request) do
+        create(:closed_merge_request,
+          source_project: fork_project,
+          target_project: project)
+      end
+
+      it "has a fork" do
+        expect(closed_merge_request.closed_without_fork?).to be_falsey
+      end
+
+      it "does not have a fork" do
+        unlink_project.execute
+        closed_merge_request.reload
+
+        expect(closed_merge_request.closed_without_fork?).to be_truthy
+      end
+    end
+  end
 end

spec/views/projects/merge_requests/edit.html.haml_spec.rb

@@ -0,0 +1,41 @@
+require 'spec_helper'
+
+describe 'projects/merge_requests/edit.html.haml' do
+  include Devise::TestHelpers
+
+  let(:user) { create(:user) }
+  let(:project) { create(:project) }
+  let(:fork_project) { create(:project, forked_from_project: project) }
+  let(:closed_merge_request) do
+    create(:closed_merge_request,
+      source_project: fork_project,
+      target_project: project,
+      author: user)
+  end
+  let(:unlink_project) { Projects::UnlinkForkService.new(fork_project, user) }
+
+  before do
+    assign(:project, project)
+    assign(:merge_request, closed_merge_request)
+
+    allow(view).to receive(:can?).and_return(true)
+    allow(view).to receive(:current_user).and_return(User.find(closed_merge_request.author_id))
+  end
+
+  context 'when closed MR without fork' do
+    it "shows editable fields" do
+      unlink_project.execute
+      closed_merge_request.reload
+      render
+
+      expect(rendered).to have_field('merge_request[title]')
+      expect(rendered).to have_css('label', text: "Title")
+      expect(rendered).to have_field('merge_request[description]')
+      expect(rendered).to have_css('label', text: "Description")
+      expect(rendered).to have_css('label', text: "Assignee")
+      expect(rendered).to have_css('label', text: "Milestone")
+      expect(rendered).to have_css('label', text: "Labels")
+      expect(rendered).not_to have_css('label', text: "Target branch")
+    end
+  end
+end

spec/views/projects/merge_requests/show.html.haml_spec.rb

@@ -0,0 +1,40 @@
+require 'spec_helper'
+
+describe 'projects/merge_requests/show.html.haml' do
+  include Devise::TestHelpers
+
+  let(:project) { create(:project) }
+  let(:fork_project) { create(:project, forked_from_project: project) }
+  let(:merge_request) do
+    create(:merge_request,
+      source_project: fork_project,
+      source_branch: 'add-submodule-version-bump',
+      target_branch: 'master', target_project: project)
+  end
+
+  before do
+    assign(:project, project)
+    assign(:merge_request, merge_request)
+    assign(:commits_count, 0)
+
+    merge_request.close!
+    allow(view).to receive(:can?).and_return(true)
+  end
+
+  context 'closed MR' do
+    it 'shows Reopen button' do
+      render
+
+      expect(rendered).to have_css('a', visible: true, text: 'Reopen')
+      expect(rendered).to have_css('a', visible: false, text: 'Close')
+    end
+
+    it 'does not show Reopen button without fork' do
+      fork_project.destroy
+      render
+
+      expect(rendered).to have_css('a', visible: false, text: 'Reopen')
+      expect(rendered).to have_css('a', visible: false, text: 'Close')
+    end
+  end
+end