Commit 6d7d12d1 authored by Phil Hughes's avatar Phil Hughes

Disable diff file edit button if source branch is deleted

If the merge request is merged, point the edit button gets pointed to
the target branch
if the merge request is opened or closed and the source branch
gets deleted then we disable the button with a tooltip.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/213263
parent 61506593
<script> <script>
import { GlTooltipDirective, GlDeprecatedButton } from '@gitlab/ui'; import { GlTooltipDirective, GlDeprecatedButton } from '@gitlab/ui';
import { __ } from '~/locale';
import Icon from '~/vue_shared/components/icon.vue'; import Icon from '~/vue_shared/components/icon.vue';
export default { export default {
...@@ -13,7 +14,8 @@ export default { ...@@ -13,7 +14,8 @@ export default {
props: { props: {
editPath: { editPath: {
type: String, type: String,
required: true, required: false,
default: '',
}, },
canCurrentUserFork: { canCurrentUserFork: {
type: Boolean, type: Boolean,
...@@ -25,6 +27,18 @@ export default { ...@@ -25,6 +27,18 @@ export default {
default: false, default: false,
}, },
}, },
computed: {
tooltipTitle() {
if (this.isDisabled) {
return __("Can't edit as source branch was deleted");
}
return __('Edit file');
},
isDisabled() {
return !this.editPath;
},
},
methods: { methods: {
handleEditClick(evt) { handleEditClick(evt) {
if (this.canCurrentUserFork && !this.canModifyBlob) { if (this.canCurrentUserFork && !this.canModifyBlob) {
...@@ -37,13 +51,15 @@ export default { ...@@ -37,13 +51,15 @@ export default {
</script> </script>
<template> <template>
<span v-gl-tooltip.top :title="tooltipTitle">
<gl-deprecated-button <gl-deprecated-button
v-gl-tooltip.top
:href="editPath" :href="editPath"
:title="__('Edit file')" :disabled="isDisabled"
class="js-edit-blob" :class="{ 'cursor-not-allowed': isDisabled }"
class="rounded-0 js-edit-blob"
@click.native="handleEditClick" @click.native="handleEditClick"
> >
<icon name="pencil" /> <icon name="pencil" />
</gl-deprecated-button> </gl-deprecated-button>
</span>
</template> </template>
...@@ -507,6 +507,10 @@ ...@@ -507,6 +507,10 @@
opacity: 1 !important; opacity: 1 !important;
cursor: default !important; cursor: default !important;
&.cursor-not-allowed {
cursor: not-allowed !important;
}
i { i {
color: $gl-text-color-disabled !important; color: $gl-text-color-disabled !important;
} }
......
...@@ -22,16 +22,16 @@ class DiffFileBaseEntity < Grape::Entity ...@@ -22,16 +22,16 @@ class DiffFileBaseEntity < Grape::Entity
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file| expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
merge_request = options[:merge_request] merge_request = options[:merge_request]
options = merge_request.persisted? ? { from_merge_request_iid: merge_request.iid } : {} next unless merge_request.merged? || merge_request.source_branch_exists?
next unless merge_request.source_project target_project, target_branch = edit_project_branch_options(merge_request)
if Feature.enabled?(:web_ide_default) if Feature.enabled?(:web_ide_default)
ide_edit_path(merge_request.source_project, merge_request.source_branch, diff_file.new_path) ide_edit_path(target_project, target_branch, diff_file.new_path)
else else
project_edit_blob_path(merge_request.source_project, options = merge_request.persisted? && merge_request.source_branch_exists? && !merge_request.merged? ? { from_merge_request_iid: merge_request.iid } : {}
tree_join(merge_request.source_branch, diff_file.new_path),
options) project_edit_blob_path(target_project, tree_join(target_branch, diff_file.new_path), options)
end end
end end
...@@ -61,7 +61,7 @@ class DiffFileBaseEntity < Grape::Entity ...@@ -61,7 +61,7 @@ class DiffFileBaseEntity < Grape::Entity
next unless diff_file.blob next unless diff_file.blob
if merge_request&.source_project && current_user if merge_request&.source_project && current_user
can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch) can_modify_blob?(diff_file.blob, merge_request.source_project, merge_request.source_branch_exists? ? merge_request.source_branch : merge_request.target_branch)
else else
false false
end end
...@@ -113,4 +113,12 @@ class DiffFileBaseEntity < Grape::Entity ...@@ -113,4 +113,12 @@ class DiffFileBaseEntity < Grape::Entity
def current_user def current_user
request.current_user request.current_user
end end
def edit_project_branch_options(merge_request)
if merge_request.source_branch_exists? && !merge_request.merged?
[merge_request.source_project, merge_request.source_branch]
else
[merge_request.target_project, merge_request.target_branch]
end
end
end end
...@@ -11,6 +11,10 @@ class DiffsEntity < Grape::Entity ...@@ -11,6 +11,10 @@ class DiffsEntity < Grape::Entity
merge_request&.source_branch merge_request&.source_branch
end end
expose :source_branch_exists do |diffs|
merge_request&.source_branch_exists?
end
expose :target_branch_name do |diffs| expose :target_branch_name do |diffs|
merge_request&.target_branch merge_request&.target_branch
end end
......
...@@ -3548,6 +3548,9 @@ msgstr "" ...@@ -3548,6 +3548,9 @@ msgstr ""
msgid "Can override approvers and approvals required per merge request" msgid "Can override approvers and approvals required per merge request"
msgstr "" msgstr ""
msgid "Can't edit as source branch was deleted"
msgstr ""
msgid "Can't find HEAD commit for this branch" msgid "Can't find HEAD commit for this branch"
msgstr "" msgstr ""
......
import { shallowMount } from '@vue/test-utils'; import { shallowMount } from '@vue/test-utils';
import { GlDeprecatedButton } from '@gitlab/ui';
import EditButton from '~/diffs/components/edit_button.vue'; import EditButton from '~/diffs/components/edit_button.vue';
const editPath = 'test-path'; const editPath = 'test-path';
...@@ -22,7 +23,7 @@ describe('EditButton', () => { ...@@ -22,7 +23,7 @@ describe('EditButton', () => {
canCurrentUserFork: false, canCurrentUserFork: false,
}); });
expect(wrapper.attributes('href')).toBe(editPath); expect(wrapper.find(GlDeprecatedButton).attributes('href')).toBe(editPath);
}); });
it('emits a show fork message event if current user can fork', () => { it('emits a show fork message event if current user can fork', () => {
...@@ -30,7 +31,7 @@ describe('EditButton', () => { ...@@ -30,7 +31,7 @@ describe('EditButton', () => {
editPath, editPath,
canCurrentUserFork: true, canCurrentUserFork: true,
}); });
wrapper.trigger('click'); wrapper.find(GlDeprecatedButton).trigger('click');
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
expect(wrapper.emitted('showForkMessage')).toBeTruthy(); expect(wrapper.emitted('showForkMessage')).toBeTruthy();
...@@ -42,7 +43,7 @@ describe('EditButton', () => { ...@@ -42,7 +43,7 @@ describe('EditButton', () => {
editPath, editPath,
canCurrentUserFork: false, canCurrentUserFork: false,
}); });
wrapper.trigger('click'); wrapper.find(GlDeprecatedButton).trigger('click');
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
expect(wrapper.emitted('showForkMessage')).toBeFalsy(); expect(wrapper.emitted('showForkMessage')).toBeFalsy();
...@@ -55,10 +56,20 @@ describe('EditButton', () => { ...@@ -55,10 +56,20 @@ describe('EditButton', () => {
canCurrentUserFork: true, canCurrentUserFork: true,
canModifyBlob: true, canModifyBlob: true,
}); });
wrapper.trigger('click'); wrapper.find(GlDeprecatedButton).trigger('click');
return wrapper.vm.$nextTick().then(() => { return wrapper.vm.$nextTick().then(() => {
expect(wrapper.emitted('showForkMessage')).toBeFalsy(); expect(wrapper.emitted('showForkMessage')).toBeFalsy();
}); });
}); });
it('disables button if editPath is empty', () => {
createComponent({
editPath: '',
canCurrentUserFork: true,
canModifyBlob: true,
});
expect(wrapper.find(GlDeprecatedButton).attributes('disabled')).toBe('true');
});
}); });
...@@ -34,4 +34,62 @@ describe DiffFileBaseEntity do ...@@ -34,4 +34,62 @@ describe DiffFileBaseEntity do
expect(entity[:new_size]).to eq(132) expect(entity[:new_size]).to eq(132)
end end
end end
context 'edit_path' do
let(:diff_file) { merge_request.diffs.diff_files.to_a.last }
let(:options) { { request: EntityRequest.new(current_user: create(:user)), merge_request: merge_request } }
let(:params) { {} }
before do
stub_feature_flags(web_ide_default: false)
end
shared_examples 'a diff file edit path to the source branch' do
it do
expect(entity[:edit_path]).to eq(Gitlab::Routing.url_helpers.project_edit_blob_path(project, File.join(merge_request.source_branch, diff_file.new_path), params))
end
end
context 'open' do
let(:merge_request) { create(:merge_request, source_project: project, target_branch: 'master', source_branch: 'feature') }
let(:params) { { from_merge_request_iid: merge_request.iid } }
it_behaves_like 'a diff file edit path to the source branch'
context 'removed source branch' do
before do
allow(merge_request).to receive(:source_branch_exists?).and_return(false)
end
it do
expect(entity[:edit_path]).to eq(nil)
end
end
end
context 'closed' do
let(:merge_request) { create(:merge_request, source_project: project, state: :closed, target_branch: 'master', source_branch: 'feature') }
let(:params) { { from_merge_request_iid: merge_request.iid } }
it_behaves_like 'a diff file edit path to the source branch'
context 'removed source branch' do
before do
allow(merge_request).to receive(:source_branch_exists?).and_return(false)
end
it do
expect(entity[:edit_path]).to eq(nil)
end
end
end
context 'merged' do
let(:merge_request) { create(:merge_request, source_project: project, state: :merged) }
it do
expect(entity[:edit_path]).to eq(Gitlab::Routing.url_helpers.project_edit_blob_path(project, File.join(merge_request.target_branch, diff_file.new_path), {}))
end
end
end
end end
...@@ -29,7 +29,7 @@ describe DiffsMetadataEntity do ...@@ -29,7 +29,7 @@ describe DiffsMetadataEntity do
:added_lines, :removed_lines, :render_overflow_warning, :added_lines, :removed_lines, :render_overflow_warning,
:email_patch_path, :plain_diff_path, :email_patch_path, :plain_diff_path,
:merge_request_diffs, :context_commits, :merge_request_diffs, :context_commits,
:definition_path_prefix, :definition_path_prefix, :source_branch_exists,
# Attributes # Attributes
:diff_files :diff_files
) )
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment