Add dropdown for diff edit button

- Adds a "Edit in Web IDE" action for the dropdown.
- Adds a "ide_edit_path" for the diff file entity.

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

@@ -69,6 +69,11 @@ export default {
       default: false,
     },
   },
+  data() {
+    return {
+      hasDropdownOpen: false,
+    };
+  },
   computed: {
     ...mapGetters('diffs', ['diffHasExpandedDiscussions', 'diffHasDiscussions']),
     diffContentIDSelector() {
@@ -179,6 +184,9 @@ export default {
         }
       }
     },
+    setDropdownOpen(val) {
+      this.hasDropdownOpen = val;
+    },
   },
 };
 </script>
@@ -187,6 +195,7 @@ export default {
   <div
     ref="header"
     class="js-file-title file-title file-title-flex-parent"
+    :class="{ 'gl-z-dropdown-menu!': hasDropdownOpen }"
     @click.self="handleToggleFile"
   >
     <div class="file-header-content">
@@ -273,11 +282,14 @@ export default {
             v-if="!diffFile.deleted_file"
             :can-current-user-fork="canCurrentUserFork"
             :edit-path="diffFile.edit_path"
+            :ide-edit-path="diffFile.ide_edit_path"
             :can-modify-blob="diffFile.can_modify_blob"
             data-track-event="click_toggle_edit_button"
             data-track-label="diff_toggle_edit_button"
             data-track-property="diff_toggle_edit"
             @showForkMessage="showForkMessage"
+            @open="setDropdownOpen(true)"
+            @close="setDropdownOpen(false)"
           />
         </template>
 

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

 <script>
-import { GlTooltipDirective, GlDeprecatedButton, GlIcon } from '@gitlab/ui';
+import {
+  GlTooltipDirective,
+  GlIcon,
+  GlDeprecatedDropdown as GlDropdown,
+  GlDeprecatedDropdownItem as GlDropdownItem,
+} from '@gitlab/ui';
 import { __ } from '~/locale';
 
 export default {
   components: {
-    GlDeprecatedButton,
+    GlDropdown,
+    GlDropdownItem,
     GlIcon,
   },
   directives: {
@@ -16,6 +22,11 @@ export default {
       required: false,
       default: '',
     },
+    ideEditPath: {
+      type: String,
+      required: false,
+      default: '',
+    },
     canCurrentUserFork: {
       type: Boolean,
       required: true,
@@ -32,33 +43,47 @@ export default {
         return __("Can't edit as source branch was deleted");
       }
 
-      return __('Edit file');
+      return '';
     },
     isDisabled() {
       return !this.editPath;
     },
   },
   methods: {
-    handleEditClick(evt) {
+    handleShow(evt) {
       if (this.canCurrentUserFork && !this.canModifyBlob) {
         evt.preventDefault();
         this.$emit('showForkMessage');
+      } else {
+        this.$emit('open');
       }
     },
+    handleHide() {
+      this.$emit('close');
+    },
   },
 };
 </script>
 
 <template>
-  <span v-gl-tooltip.top :title="tooltipTitle">
-    <gl-deprecated-button
-      :href="editPath"
+  <div v-gl-tooltip.top="tooltipTitle" class="gl-display-flex">
+    <gl-dropdown
+      toggle-class="rounded-0"
       :disabled="isDisabled"
       :class="{ 'cursor-not-allowed': isDisabled }"
-      class="rounded-0 js-edit-blob"
-      @click.native="handleEditClick"
+      right
+      data-testid="edit_file"
+      @show="handleShow"
+      @hide="handleHide"
     >
-      <gl-icon name="pencil" />
-    </gl-deprecated-button>
-  </span>
+      <template #button-content>
+        <span class="gl-dropdown-toggle-text"><gl-icon name="pencil"/></span>
+        <gl-icon class="gl-dropdown-caret" name="chevron-down" aria-hidden="true" />
+      </template>
+      <gl-dropdown-item v-if="editPath" :href="editPath">{{ __('Edit file') }}</gl-dropdown-item>
+      <gl-dropdown-item v-if="ideEditPath" :href="ideEditPath">{{
+        __('Edit in Web IDE')
+      }}</gl-dropdown-item>
+    </gl-dropdown>
+  </div>
 </template>

app/assets/stylesheets/utilities.scss

@@ -156,3 +156,8 @@
     display: none;
   }
 }
+
+// This utility is used to force the z-index to match that of dropdown menu's
+.gl-z-dropdown-menu\! {
+  z-index: 300 !important;
+}

app/helpers/blob_helper.rb

@@ -32,6 +32,19 @@ module BlobHelper
     File.join(segments)
   end
 
+  def ide_merge_request_path(merge_request)
+    target_project = merge_request.target_project
+    source_project = merge_request.source_project
+
+    web_ide_path_base = File.join([ide_path, 'project', source_project.full_path, 'merge_requests', merge_request.iid.to_s])
+
+    if target_project != source_project
+      "#{web_ide_path_base}?#{{ target_project: target_project.full_path }.to_query}"
+    else
+      web_ide_path_base
+    end
+  end
+
   def ide_fork_and_edit_path(project = @project, ref = @ref, path = @path, options = {})
     if current_user
       project_forks_path(project,

app/serializers/diff_file_base_entity.rb

@@ -34,7 +34,7 @@ class DiffFileBaseEntity < Grape::Entity
   expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
     merge_request = options[:merge_request]
 
-    next unless merge_request.merged? || merge_request.source_branch_exists?
+    next unless has_edit_path?(merge_request)
 
     target_project, target_branch = edit_project_branch_options(merge_request)
 
@@ -43,6 +43,14 @@ class DiffFileBaseEntity < Grape::Entity
     project_edit_blob_path(target_project, tree_join(target_branch, diff_file.new_path), options)
   end
 
+  expose :ide_edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
+    merge_request = options[:merge_request]
+
+    next unless has_edit_path?(merge_request)
+
+    ide_merge_request_path(merge_request)
+  end
+
   expose :old_path_html do |diff_file|
     old_path, _ = mark_inline_diffs(diff_file.old_path, diff_file.new_path)
     old_path
@@ -125,4 +133,8 @@ class DiffFileBaseEntity < Grape::Entity
       [merge_request.target_project, merge_request.target_branch]
     end
   end
+
+  def has_edit_path?(merge_request)
+    merge_request.merged? || merge_request.source_branch_exists?
+  end
 end

changelogs/unreleased/21654-ide-button-in-mr-diff-files.yml

+---
+title: Add Web IDE as dropdown item to diff file edit
+merge_request: 42275
+author:
+type: changed

locale/gitlab.pot

@@ -9255,6 +9255,9 @@ msgstr ""
 msgid "Edit identity for %{user_name}"
 msgstr ""
 
+msgid "Edit in Web IDE"
+msgstr ""
+
 msgid "Edit issues"
 msgstr ""
 

spec/features/merge_request/maintainer_edits_fork_spec.rb

@@ -26,7 +26,10 @@ RSpec.describe 'a maintainer edits files on a source-branch of an MR from a fork
     visit project_merge_request_path(target_project, merge_request)
     click_link 'Changes'
     wait_for_requests
-    first('.js-file-title').find('.js-edit-blob').click
+    within first('.js-file-title') do
+      find('[data-testid="edit_file"]').click
+      click_link 'Edit file'
+    end
     wait_for_requests
   end
 

spec/features/merge_request/user_sees_diff_spec.rb

@@ -63,7 +63,7 @@ RSpec.describe 'Merge request > User sees diff', :js do
         visit diffs_project_merge_request_path(project, merge_request)
 
         # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax
-        expect(page).to have_selector("[id=\"#{changelog_id}\"] a.js-edit-blob")
+        expect(page).to have_selector("[id=\"#{changelog_id}\"] [data-testid='edit_file']")
       end
     end
 
@@ -73,7 +73,7 @@ RSpec.describe 'Merge request > User sees diff', :js do
         visit diffs_project_merge_request_path(project, merge_request)
 
         # Throws `Capybara::Poltergeist::InvalidSelector` if we try to use `#hash` syntax
-        find("[id=\"#{changelog_id}\"] .js-edit-blob").click
+        find("[id=\"#{changelog_id}\"] [data-testid=\"edit_file\"").click
 
         expect(page).to have_selector('.js-fork-suggestion-button', count: 1)
         expect(page).to have_selector('.js-cancel-fork-suggestion-button', count: 1)

spec/features/projects/blobs/edit_spec.rb

@@ -23,6 +23,19 @@ RSpec.describe 'Editing file blob', :js do
     def edit_and_commit(commit_changes: true)
       wait_for_requests
       find('.js-edit-blob').click
+
+      fill_and_commit(commit_changes)
+    end
+
+    def mr_edit_and_commit(commit_changes: true)
+      wait_for_requests
+      find('[data-testid="edit_file"]').click
+      click_link 'Edit file'
+
+      fill_and_commit(commit_changes)
+    end
+
+    def fill_and_commit(commit_changes)
       fill_editor(content: 'class NextFeature\\nend\\n')
 
       if commit_changes
@@ -38,7 +51,7 @@ RSpec.describe 'Editing file blob', :js do
     context 'from MR diff' do
       before do
         visit diffs_project_merge_request_path(project, merge_request)
-        edit_and_commit
+        mr_edit_and_commit
       end
 
       it 'returns me to the mr' do

spec/frontend/diffs/components/diff_file_header_spec.js

@@ -76,6 +76,7 @@ describe('DiffFileHeader component', () => {
   const findReplacedFileButton = () => wrapper.find({ ref: 'replacedFileButton' });
   const findViewFileButton = () => wrapper.find({ ref: 'viewButton' });
   const findCollapseIcon = () => wrapper.find({ ref: 'collapseIcon' });
+  const hasZDropdownMenuClass = () => wrapper.classes('gl-z-dropdown-menu!');
 
   const findIconByName = iconName => {
     const icons = wrapper.findAll(GlIcon).filter(w => w.props('name') === iconName);
@@ -151,6 +152,10 @@ describe('DiffFileHeader component', () => {
     expect(wrapper.find(ClipboardButton).exists()).toBe(true);
   });
 
+  it('should not have z dropdown menu class', () => {
+    expect(hasZDropdownMenuClass()).toBe(false);
+  });
+
   describe('for submodule', () => {
     const submoduleDiffFile = {
       ...diffFile,
@@ -303,6 +308,27 @@ describe('DiffFileHeader component', () => {
         expect(wrapper.find(EditButton).exists()).toBe(true);
       });
 
+      describe('when edit button opens', () => {
+        beforeEach(async () => {
+          createComponent({ addMergeRequestButtons: true });
+          wrapper.find(EditButton).vm.$emit('open');
+
+          await wrapper.vm.$nextTick();
+        });
+
+        it('should add z dropdown menu class when edit button opens', async () => {
+          expect(hasZDropdownMenuClass()).toBe(true);
+        });
+
+        it('when closes again, should remove class', async () => {
+          wrapper.find(EditButton).vm.$emit('close');
+
+          await wrapper.vm.$nextTick();
+
+          expect(hasZDropdownMenuClass()).toBe(false);
+        });
+      });
+
       describe('view on environment button', () => {
         it('is displayed when external url is provided', () => {
           const externalUrl = 'link://to/external';

spec/frontend/diffs/components/edit_button_spec.js

-import { shallowMount } from '@vue/test-utils';
-import { GlDeprecatedButton } from '@gitlab/ui';
+import { shallowMount, mount } from '@vue/test-utils';
+import { GlDeprecatedDropdown, GlDeprecatedDropdownItem, GlIcon } from '@gitlab/ui';
+import { createMockDirective, getBinding } from 'helpers/vue_mock_directive';
 import EditButton from '~/diffs/components/edit_button.vue';
 
-const editPath = 'test-path';
+const EDIT_ITEM = {
+  href: 'test-path',
+  text: 'Edit file',
+};
+const IDE_EDIT_ITEM = {
+  href: 'ide-test-path',
+  text: 'Edit in Web IDE',
+};
 
 describe('EditButton', () => {
   let wrapper;
 
-  const createComponent = (props = {}) => {
-    wrapper = shallowMount(EditButton, {
-      propsData: { ...props },
+  const createComponent = (props = {}, mountFn = shallowMount) => {
+    wrapper = mountFn(EditButton, {
+      propsData: {
+        editPath: EDIT_ITEM.href,
+        ideEditPath: IDE_EDIT_ITEM.href,
+        canCurrentUserFork: false,
+        ...props,
+      },
+      directives: {
+        GlTooltip: createMockDirective(),
+      },
     });
   };
 
@@ -17,59 +33,92 @@ describe('EditButton', () => {
     wrapper.destroy();
   });
 
-  it('has correct href attribute', () => {
-    createComponent({
-      editPath,
-      canCurrentUserFork: false,
-    });
+  const getTooltip = () => getBinding(wrapper.element, 'gl-tooltip').value;
+  const findDropdown = () => wrapper.find(GlDeprecatedDropdown);
+  const parseDropdownItems = () =>
+    wrapper.findAll(GlDeprecatedDropdownItem).wrappers.map(x => ({
+      text: x.text(),
+      href: x.attributes('href'),
+    }));
+  const triggerShow = () => {
+    const event = new Event('');
+    jest.spyOn(event, 'preventDefault');
+
+    findDropdown().vm.$emit('show', event);
+
+    return event;
+  };
 
-    expect(wrapper.find(GlDeprecatedButton).attributes('href')).toBe(editPath);
+  it.each`
+    props                  | expectedItems
+    ${{}}                  | ${[EDIT_ITEM, IDE_EDIT_ITEM]}
+    ${{ editPath: '' }}    | ${[IDE_EDIT_ITEM]}
+    ${{ ideEditPath: '' }} | ${[EDIT_ITEM]}
+  `('should render items with=$props', ({ props, expectedItems }) => {
+    createComponent(props);
+
+    expect(parseDropdownItems()).toEqual(expectedItems);
   });
 
-  it('emits a show fork message event if current user can fork', () => {
-    createComponent({
-      editPath,
-      canCurrentUserFork: true,
+  describe('with default', () => {
+    beforeEach(() => {
+      createComponent({}, mount);
     });
-    wrapper.find(GlDeprecatedButton).trigger('click');
 
-    return wrapper.vm.$nextTick().then(() => {
-      expect(wrapper.emitted('showForkMessage')).toBeTruthy();
+    it('does not have tooltip', () => {
+      expect(getTooltip()).toBe('');
     });
-  });
 
-  it('doesnt emit a show fork message event if current user cannot fork', () => {
-    createComponent({
-      editPath,
-      canCurrentUserFork: false,
+    it('shows pencil dropdown', () => {
+      expect(wrapper.find(GlIcon).props('name')).toBe('pencil');
+      expect(wrapper.find('.gl-dropdown-caret').exists()).toBe(true);
     });
-    wrapper.find(GlDeprecatedButton).trigger('click');
 
-    return wrapper.vm.$nextTick().then(() => {
-      expect(wrapper.emitted('showForkMessage')).toBeFalsy();
+    it.each`
+      event     | expectedEmit
+      ${'show'} | ${'open'}
+      ${'hide'} | ${'close'}
+    `('when dropdown emits $event, emits $expectedEmit', ({ event, expectedEmit }) => {
+      expect(wrapper.emitted(expectedEmit)).toBeUndefined();
+
+      findDropdown().vm.$emit(event);
+
+      expect(wrapper.emitted(expectedEmit)).toEqual([[]]);
     });
   });
 
-  it('doesnt emit a show fork message event if current user can modify blob', () => {
-    createComponent({
-      editPath,
-      canCurrentUserFork: true,
-      canModifyBlob: true,
+  describe('with cant modify blob and can fork', () => {
+    beforeEach(() => {
+      createComponent({
+        canModifyBlob: false,
+        canCurrentUserFork: true,
+      });
     });
-    wrapper.find(GlDeprecatedButton).trigger('click');
 
-    return wrapper.vm.$nextTick().then(() => {
-      expect(wrapper.emitted('showForkMessage')).toBeFalsy();
+    it('when try to open, emits showForkMessage', () => {
+      expect(wrapper.emitted('showForkMessage')).toBeUndefined();
+
+      const event = triggerShow();
+
+      expect(wrapper.emitted('showForkMessage')).toEqual([[]]);
+      expect(event.preventDefault).toHaveBeenCalled();
+      expect(wrapper.emitted('open')).toBeUndefined();
     });
   });
 
-  it('disables button if editPath is empty', () => {
-    createComponent({
-      editPath: '',
-      canCurrentUserFork: true,
-      canModifyBlob: true,
+  describe('with editPath is falsey', () => {
+    beforeEach(() => {
+      createComponent({
+        editPath: '',
+      });
     });
 
-    expect(wrapper.find(GlDeprecatedButton).attributes('disabled')).toBe('true');
+    it('should disable dropdown', () => {
+      expect(findDropdown().attributes('disabled')).toBe('true');
+    });
+
+    it('should have tooltip', () => {
+      expect(getTooltip()).toBe("Can't edit as source branch was deleted");
+    });
   });
 });