Merge branch '21654-ide-button-in-mr-diff-files' into 'master'

Add Web IDE as dropdown item to diff file edit

See merge request gitlab-org/gitlab!42275

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 { uniqueId } from 'lodash';
+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 +23,11 @@ export default {
       required: false,
       default: '',
     },
+    ideEditPath: {
+      type: String,
+      required: false,
+      default: '',
+    },
     canCurrentUserFork: {
       type: Boolean,
       required: true,
@@ -26,39 +38,62 @@ export default {
       default: false,
     },
   },
+  data() {
+    return { tooltipId: uniqueId('edit_button_tooltip_') };
+  },
   computed: {
     tooltipTitle() {
       if (this.isDisabled) {
         return __("Can't edit as source branch was deleted");
       }
 
-      return __('Edit file');
+      return __('Edit file in...');
     },
     isDisabled() {
       return !this.editPath;
     },
   },
   methods: {
-    handleEditClick(evt) {
+    handleShow(evt) {
+      // We must hide the tooltip because it is redundant and doesn't close itself
+      // when dropdown opens because we are still "focused".
+      this.$root.$emit('bv::hide::tooltip', this.tooltipId);
+
       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="{ title: tooltipTitle, id: tooltipId }" 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 in single-file editor')
+      }}</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/gitlab_routing_helper.rb

@@ -343,6 +343,18 @@ module GitlabRoutingHelper
     Gitlab::UrlBuilder.wiki_page_url(wiki, page, only_path: true, **options)
   end
 
+  def gitlab_ide_merge_request_path(merge_request)
+    target_project = merge_request.target_project
+    source_project = merge_request.source_project
+    params = {}
+
+    if target_project != source_project
+      params = { target_project: target_project.full_path }
+    end
+
+    ide_merge_request_path(source_project.namespace, source_project, merge_request, params)
+  end
+
   private
 
   def snippet_query_params(snippet, *args)

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)
+
+    gitlab_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

config/routes.rb

@@ -122,6 +122,7 @@ Rails.application.routes.draw do
 
     get 'ide' => 'ide#index'
     get 'ide/*vueroute' => 'ide#index', format: false
+    get 'ide/project/:namespace/:project/merge_requests/:id' => 'ide#index', format: false, as: :ide_merge_request
 
     draw :operations
     draw :jira_connect

locale/gitlab.pot

@@ -9228,7 +9228,7 @@ msgstr ""
 msgid "Edit environment"
 msgstr ""
 
-msgid "Edit file"
+msgid "Edit file in..."
 msgstr ""
 
 msgid "Edit files in the editor and commit changes here"
@@ -9243,6 +9243,12 @@ msgstr ""
 msgid "Edit identity for %{user_name}"
 msgstr ""
 
+msgid "Edit in Web IDE"
+msgstr ""
+
+msgid "Edit in single-file editor"
+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 in single-file editor'
+    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 in single-file editor'
+
+      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';
+jest.mock('lodash/uniqueId', () => (str = '') => `${str}fake`);
+
+const TOOLTIP_ID = 'edit_button_tooltip_fake';
+const EDIT_ITEM = {
+  href: 'test-path',
+  text: 'Edit in single-file editor',
+};
+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 +36,105 @@ 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;
+  };
+
+  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(wrapper.find(GlDeprecatedButton).attributes('href')).toBe(editPath);
+    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()).toEqual({ id: TOOLTIP_ID, title: 'Edit file in...' });
     });
-  });
 
-  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();
+    describe.each`
+      event     | expectedEmit | expectedRootEmit
+      ${'show'} | ${'open'}    | ${[['bv::hide::tooltip', TOOLTIP_ID]]}
+      ${'hide'} | ${'close'}   | ${[]}
+    `('when dropdown emits $event', ({ event, expectedEmit, expectedRootEmit }) => {
+      let rootEmitSpy;
+
+      beforeEach(() => {
+        rootEmitSpy = jest.spyOn(wrapper.vm.$root, '$emit');
+
+        findDropdown().vm.$emit(event);
+      });
+
+      it(`emits ${expectedEmit}`, () => {
+        expect(wrapper.emitted(expectedEmit)).toEqual([[]]);
+      });
+
+      it(`emits root = ${JSON.stringify(expectedRootEmit)}`, () => {
+        expect(rootEmitSpy.mock.calls).toEqual(expectedRootEmit);
+      });
     });
   });
 
-  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: '',
+      });
+    });
+
+    it('should disable dropdown', () => {
+      expect(findDropdown().attributes('disabled')).toBe('true');
     });
 
-    expect(wrapper.find(GlDeprecatedButton).attributes('disabled')).toBe('true');
+    it('should have tooltip', () => {
+      expect(getTooltip()).toEqual({
+        id: TOOLTIP_ID,
+        title: "Can't edit as source branch was deleted",
+      });
+    });
   });
 });

spec/serializers/diff_file_base_entity_spec.rb

@@ -3,10 +3,24 @@
 require 'spec_helper'
 
 RSpec.describe DiffFileBaseEntity do
-  let(:project) { create(:project, :repository) }
+  include ProjectForksHelper
+
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+
   let(:repository) { project.repository }
   let(:entity) { described_class.new(diff_file, options).as_json }
 
+  shared_examples 'nil if removed source branch' do |key|
+    before do
+      allow(merge_request).to receive(:source_branch_exists?).and_return(false)
+    end
+
+    specify do
+      expect(entity[key]).to eq(nil)
+    end
+  end
+
   context 'submodule information for a' do
     let(:commit_sha) { "" }
     let(:commit) { project.commit(commit_sha) }
@@ -67,7 +81,7 @@ RSpec.describe DiffFileBaseEntity do
 
   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(:options) { { request: EntityRequest.new(current_user: user), merge_request: merge_request } }
     let(:params) { {} }
 
     shared_examples 'a diff file edit path to the source branch' do
@@ -81,16 +95,7 @@ RSpec.describe DiffFileBaseEntity do
       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
+      it_behaves_like 'nil if removed source branch', :edit_path
     end
 
     context 'closed' do
@@ -118,4 +123,30 @@ RSpec.describe DiffFileBaseEntity do
       end
     end
   end
+
+  context 'ide_edit_path' do
+    let(:source_project) { project }
+    let(:merge_request) { create(:merge_request, iid: 123, target_project: target_project, source_project: source_project) }
+    let(:diff_file) { merge_request.diffs.diff_files.to_a.last }
+    let(:options) { { request: EntityRequest.new(current_user: user), merge_request: merge_request } }
+    let(:expected_merge_request_path) { "/-/ide/project/#{source_project.full_path}/merge_requests/#{merge_request.iid}" }
+
+    context 'when source_project and target_project are the same' do
+      let(:target_project) { source_project }
+
+      it_behaves_like 'nil if removed source branch', :ide_edit_path
+
+      it 'returns the merge_request ide route' do
+        expect(entity[:ide_edit_path]).to eq expected_merge_request_path
+      end
+    end
+
+    context 'when source_project and target_project are different' do
+      let(:target_project) { fork_project(source_project, source_project.owner, repository: true) }
+
+      it 'returns the merge_request ide route with the target_project as param' do
+        expect(entity[:ide_edit_path]).to eq("#{expected_merge_request_path}?target_project=#{ERB::Util.url_encode(target_project.full_path)}")
+      end
+    end
+  end
 end