From c9c2503c5186a38302ed606f793b52ffa394f52c Mon Sep 17 00:00:00 2001 From: Katarzyna Kobierska Date: Tue, 26 Jul 2016 13:57:43 +0200 Subject: [PATCH] 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 | 1 + app/helpers/merge_requests_helper.rb | 2 +- app/models/merge_request.rb | 33 +++++----- app/services/merge_requests/update_service.rb | 4 ++ .../merge_requests/show/_mr_title.html.haml | 4 ++ app/views/shared/issuable/_form.html.haml | 41 ++++++------ .../merge_requests_controller_spec.rb | 29 +++++++++ spec/models/merge_request_spec.rb | 64 +++++++++++++++++++ .../merge_requests/edit.html.haml_spec.rb | 41 ++++++++++++ .../merge_requests/show.html.haml_spec.rb | 40 ++++++++++++ 10 files changed, 223 insertions(+), 36 deletions(-) create mode 100644 spec/views/projects/merge_requests/edit.html.haml_spec.rb create mode 100644 spec/views/projects/merge_requests/show.html.haml_spec.rb diff --git a/CHANGELOG b/CHANGELOG index a6cd8f4c7e1..cc98863dac8 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index db6e731c744..a9e175c3f5c 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/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 diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1d05e4a85d1..b41a1f0c547 100644 --- a/app/models/merge_request.rb +++ b/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 diff --git a/app/services/merge_requests/update_service.rb b/app/services/merge_requests/update_service.rb index 30c5f24988c..398ec47f0ea 100644 --- a/app/services/merge_requests/update_service.rb +++ b/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) diff --git a/app/views/projects/merge_requests/show/_mr_title.html.haml b/app/views/projects/merge_requests/show/_mr_title.html.haml index 098ce19da21..48016645019 100644 --- a/app/views/projects/merge_requests/show/_mr_title.html.haml +++ b/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) } diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 22594b46443..75753a6b0af 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/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' diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index c64c2b075c5..f95c3fc771b 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/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 diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index d67f71bbb9c..5fea6adf329 100644 --- a/spec/models/merge_request_spec.rb +++ b/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 diff --git a/spec/views/projects/merge_requests/edit.html.haml_spec.rb b/spec/views/projects/merge_requests/edit.html.haml_spec.rb new file mode 100644 index 00000000000..d7a1a2447ea --- /dev/null +++ b/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 diff --git a/spec/views/projects/merge_requests/show.html.haml_spec.rb b/spec/views/projects/merge_requests/show.html.haml_spec.rb new file mode 100644 index 00000000000..ed12b730eeb --- /dev/null +++ b/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