Merge branch 'ph/213263/editButtonOnMergeRequestDiffs' into 'master'

Disable diff file edit button if source branch is deleted

Closes #213263

See merge request gitlab-org/gitlab!30707

app/assets/javascripts/diffs/components/edit_button.vue

 <script>
 import { GlTooltipDirective, GlDeprecatedButton } from '@gitlab/ui';
+import { __ } from '~/locale';
 import Icon from '~/vue_shared/components/icon.vue';
 
 export default {
@@ -13,7 +14,8 @@ export default {
   props: {
     editPath: {
       type: String,
-      required: true,
+      required: false,
+      default: '',
     },
     canCurrentUserFork: {
       type: Boolean,
@@ -25,6 +27,18 @@ export default {
       default: false,
     },
   },
+  computed: {
+    tooltipTitle() {
+      if (this.isDisabled) {
+        return __("Can't edit as source branch was deleted");
+      }
+
+      return __('Edit file');
+    },
+    isDisabled() {
+      return !this.editPath;
+    },
+  },
   methods: {
     handleEditClick(evt) {
       if (this.canCurrentUserFork && !this.canModifyBlob) {
@@ -37,13 +51,15 @@ export default {
 </script>
 
 <template>
-  <gl-deprecated-button
-    v-gl-tooltip.top
-    :href="editPath"
-    :title="__('Edit file')"
-    class="js-edit-blob"
-    @click.native="handleEditClick"
-  >
-    <icon name="pencil" />
-  </gl-deprecated-button>
+  <span v-gl-tooltip.top :title="tooltipTitle">
+    <gl-deprecated-button
+      :href="editPath"
+      :disabled="isDisabled"
+      :class="{ 'cursor-not-allowed': isDisabled }"
+      class="rounded-0 js-edit-blob"
+      @click.native="handleEditClick"
+    >
+      <icon name="pencil" />
+    </gl-deprecated-button>
+  </span>
 </template>

app/assets/stylesheets/framework/buttons.scss

@@ -507,6 +507,10 @@
   opacity: 1 !important;
   cursor: default !important;
 
+  &.cursor-not-allowed {
+    cursor: not-allowed !important;
+  }
+
   i {
     color: $gl-text-color-disabled !important;
   }

app/serializers/diff_file_base_entity.rb

@@ -22,16 +22,16 @@ class DiffFileBaseEntity < Grape::Entity
   expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
     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)
-      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
-      project_edit_blob_path(merge_request.source_project,
-        tree_join(merge_request.source_branch, diff_file.new_path),
-        options)
+      options = merge_request.persisted? && merge_request.source_branch_exists? && !merge_request.merged? ? { from_merge_request_iid: merge_request.iid } : {}
+
+      project_edit_blob_path(target_project, tree_join(target_branch, diff_file.new_path), options)
     end
   end
 
@@ -61,7 +61,7 @@ class DiffFileBaseEntity < Grape::Entity
     next unless diff_file.blob
 
     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
       false
     end
@@ -113,4 +113,12 @@ class DiffFileBaseEntity < Grape::Entity
   def current_user
     request.current_user
   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

app/serializers/diffs_entity.rb

@@ -11,6 +11,10 @@ class DiffsEntity < Grape::Entity
     merge_request&.source_branch
   end
 
+  expose :source_branch_exists do |diffs|
+    merge_request&.source_branch_exists?
+  end
+
   expose :target_branch_name do |diffs|
     merge_request&.target_branch
   end

locale/gitlab.pot

@@ -3545,6 +3545,9 @@ msgstr ""
 msgid "Can override approvers and approvals required per merge request"
 msgstr ""
 
+msgid "Can't edit as source branch was deleted"
+msgstr ""
+
 msgid "Can't find HEAD commit for this branch"
 msgstr ""
 

spec/frontend/diffs/components/edit_button_spec.js

 import { shallowMount } from '@vue/test-utils';
+import { GlDeprecatedButton } from '@gitlab/ui';
 import EditButton from '~/diffs/components/edit_button.vue';
 
 const editPath = 'test-path';
@@ -22,7 +23,7 @@ describe('EditButton', () => {
       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', () => {
@@ -30,7 +31,7 @@ describe('EditButton', () => {
       editPath,
       canCurrentUserFork: true,
     });
-    wrapper.trigger('click');
+    wrapper.find(GlDeprecatedButton).trigger('click');
 
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.emitted('showForkMessage')).toBeTruthy();
@@ -42,7 +43,7 @@ describe('EditButton', () => {
       editPath,
       canCurrentUserFork: false,
     });
-    wrapper.trigger('click');
+    wrapper.find(GlDeprecatedButton).trigger('click');
 
     return wrapper.vm.$nextTick().then(() => {
       expect(wrapper.emitted('showForkMessage')).toBeFalsy();
@@ -55,10 +56,20 @@ describe('EditButton', () => {
       canCurrentUserFork: true,
       canModifyBlob: true,
     });
-    wrapper.trigger('click');
+    wrapper.find(GlDeprecatedButton).trigger('click');
 
     return wrapper.vm.$nextTick().then(() => {
       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');
+  });
 });

spec/serializers/diff_file_base_entity_spec.rb

@@ -34,4 +34,62 @@ describe DiffFileBaseEntity do
       expect(entity[:new_size]).to eq(132)
     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

spec/serializers/diffs_metadata_entity_spec.rb

@@ -29,7 +29,7 @@ describe DiffsMetadataEntity do
         :added_lines, :removed_lines, :render_overflow_warning,
         :email_patch_path, :plain_diff_path,
         :merge_request_diffs, :context_commits,
-        :definition_path_prefix,
+        :definition_path_prefix, :source_branch_exists,
         # Attributes
         :diff_files
       )