Merge branch...

Merge branch '18140-add-to-next-and-previous-commit-buttons-when-viewing-commits-in-merge-request-2' into 'master'

Add "Previous" and "Next" buttons for commit-by-commit navigation

See merge request gitlab-org/gitlab!28596

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

@@ -224,6 +224,7 @@ export default {
   methods: {
     ...mapActions(['startTaskList']),
     ...mapActions('diffs', [
+      'moveToNeighboringCommit',
       'setBaseConfig',
       'fetchDiffFiles',
       'fetchDiffFilesMeta',
@@ -344,9 +345,16 @@ export default {
             break;
         }
       });
+
+      if (this.commit && this.glFeatures.mrCommitNeighborNav) {
+        Mousetrap.bind('c', () => this.moveToNeighboringCommit({ direction: 'next' }));
+        Mousetrap.bind('x', () => this.moveToNeighboringCommit({ direction: 'previous' }));
+      }
     },
     removeEventListeners() {
       Mousetrap.unbind(['[', 'k', ']', 'j']);
+      Mousetrap.unbind('c');
+      Mousetrap.unbind('x');
     },
     jumpToFile(step) {
       const targetIndex = this.currentDiffIndex + step;

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

 <script>
+import { mapActions } from 'vuex';
+import { GlButtonGroup, GlButton, GlIcon, GlTooltipDirective } from '@gitlab/ui';
+
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+
 import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue';
 import Icon from '~/vue_shared/components/icon.vue';
 import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
 import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
+
 import CommitPipelineStatus from '~/projects/tree/components/commit_pipeline_status_component.vue';
+
 import initUserPopovers from '../../user_popovers';
+import { setUrlParams } from '../../lib/utils/url_utility';
 
 /**
  * CommitItem
@@ -18,7 +26,16 @@ import initUserPopovers from '../../user_popovers';
  * coexist, but there is an issue to remove the duplication.
  * https://gitlab.com/gitlab-org/gitlab-foss/issues/51613
  *
+ * EXCEPTION WARNING
+ * 1. The commit navigation buttons (next neighbor, previous neighbor)
+ *    are not duplicated because:
+ *     - We don't have the same data available on the Rails side (yet,
+ *        without backend work)
+ *     - This Vue component should always be what's used when in the
+ *        context of an MR diff, so the HAML should never have any idea
+ *        about navigating among commits.
  */
+
 export default {
   components: {
     UserAvatarLink,
@@ -26,7 +43,14 @@ export default {
     ClipboardButton,
     TimeAgoTooltip,
     CommitPipelineStatus,
+    GlButtonGroup,
+    GlButton,
+    GlIcon,
+  },
+  directives: {
+    GlTooltip: GlTooltipDirective,
   },
+  mixins: [glFeatureFlagsMixin()],
   props: {
     commit: {
       type: Object,
@@ -54,12 +78,28 @@ export default {
     authorAvatar() {
       return this.author.avatar_url || this.commit.author_gravatar_url;
     },
+    nextCommitUrl() {
+      return this.commit.next_commit_id
+        ? setUrlParams({ commit_id: this.commit.next_commit_id })
+        : '';
+    },
+    previousCommitUrl() {
+      return this.commit.prev_commit_id
+        ? setUrlParams({ commit_id: this.commit.prev_commit_id })
+        : '';
+    },
+    hasNeighborCommits() {
+      return this.commit.next_commit_id || this.commit.prev_commit_id;
+    },
   },
   created() {
     this.$nextTick(() => {
       initUserPopovers(this.$el.querySelectorAll('.js-user-link'));
     });
   },
+  methods: {
+    ...mapActions('diffs', ['moveToNeighboringCommit']),
+  },
 };
 </script>
 
@@ -123,6 +163,41 @@ export default {
             class="btn btn-default"
           />
         </div>
+        <div
+          v-if="hasNeighborCommits && glFeatures.mrCommitNeighborNav"
+          class="commit-nav-buttons ml-3"
+        >
+          <gl-button-group>
+            <gl-button
+              :href="previousCommitUrl"
+              :disabled="!commit.prev_commit_id"
+              @click.prevent="moveToNeighboringCommit({ direction: 'previous' })"
+            >
+              <span
+                v-if="!commit.prev_commit_id"
+                v-gl-tooltip
+                class="h-100 w-100 position-absolute"
+                :title="__('You\'re at the first commit')"
+              ></span>
+              <gl-icon name="chevron-left" />
+              {{ __('Prev') }}
+            </gl-button>
+            <gl-button
+              :href="nextCommitUrl"
+              :disabled="!commit.next_commit_id"
+              @click.prevent="moveToNeighboringCommit({ direction: 'next' })"
+            >
+              <span
+                v-if="!commit.next_commit_id"
+                v-gl-tooltip
+                class="h-100 w-100 position-absolute"
+                :title="__('You\'re at the last commit')"
+              ></span>
+              {{ __('Next') }}
+              <gl-icon name="chevron-right" />
+            </gl-button>
+          </gl-button-group>
+        </div>
       </div>
     </div>
   </li>

app/assets/stylesheets/pages/commits.scss

@@ -208,6 +208,18 @@
   }
 }
 
+.commit-nav-buttons {
+  a.btn,
+  button {
+    // See: https://gitlab.com/gitlab-org/gitlab-ui/-/issues/730
+    &:last-child > svg {
+      margin-left: 0.25rem;
+      margin-right: 0;
+    }
+  }
+}
+
+
 .clipboard-group,
 .commit-sha-group {
   display: inline-flex;

app/controllers/projects/merge_requests_controller.rb

@@ -33,6 +33,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:widget_visibility_polling, @project, default_enabled: true)
     push_frontend_feature_flag(:merge_ref_head_comments, @project)
     push_frontend_feature_flag(:accessibility_merge_request_widget, @project)
+    push_frontend_feature_flag(:mr_commit_neighbor_nav, @project, default_enabled: true)
   end
 
   before_action do

app/views/help/_shortcuts.html.haml

@@ -313,6 +313,18 @@
                   %td.shortcut
                     %kbd p
                   %td= _('Previous unresolved discussion')
+              %tbody
+                %tr
+                  %th
+                  %th= _('Merge Request Commits')
+                %tr
+                  %td.shortcut
+                    %kbd c
+                  %td= _('Next commit')
+                %tr
+                  %td.shortcut
+                    %kbd x
+                  %td= _('Previous commit')
               %tbody
                 %tr
                   %th

app/views/projects/commits/_commit.html.haml

 -#-----------------------------------------------------------------
   WARNING: Please keep changes up-to-date with the following files:
   - `assets/javascripts/diffs/components/commit_item.vue`
+
+  EXCEPTION WARNING - see above `.vue` file for de-sync drift
 -#-----------------------------------------------------------------
 - view_details  = local_assigns.fetch(:view_details, false)
 - merge_request = local_assigns.fetch(:merge_request, nil)

changelogs/unreleased/feature-next-prev-commit-buttons.yml

+---
+title: Add Previous and Next buttons for commit-by-commit navigation
+merge_request: 28596
+author:
+type: added

locale/gitlab.pot

@@ -13079,6 +13079,9 @@ msgstr ""
 msgid "Merge Request Approvals"
 msgstr ""
 
+msgid "Merge Request Commits"
+msgstr ""
+
 msgid "Merge Requests"
 msgstr ""
 
@@ -13937,6 +13940,9 @@ msgstr ""
 msgid "Next"
 msgstr ""
 
+msgid "Next commit"
+msgstr ""
+
 msgid "Next file in diff"
 msgstr ""
 
@@ -15610,6 +15616,9 @@ msgstr ""
 msgid "Press %{key}-C to copy"
 msgstr ""
 
+msgid "Prev"
+msgstr ""
+
 msgid "Prevent adding new members to project membership within this group"
 msgstr ""
 
@@ -15646,6 +15655,9 @@ msgstr ""
 msgid "Previous Artifacts"
 msgstr ""
 
+msgid "Previous commit"
+msgstr ""
+
 msgid "Previous file in diff"
 msgstr ""
 
@@ -24621,6 +24633,12 @@ msgstr ""
 msgid "You're about to reduce the visibility of the project %{strong_start}%{project_name}%{strong_end}."
 msgstr ""
 
+msgid "You're at the first commit"
+msgstr ""
+
+msgid "You're at the last commit"
+msgstr ""
+
 msgid "You're not allowed to %{tag_start}edit%{tag_end} files in this project directly. Please fork this project, make your changes there, and submit a merge request."
 msgstr ""
 

spec/frontend/diffs/components/app_spec.js

@@ -28,8 +28,14 @@ describe('diffs/components/app', () => {
   let wrapper;
   let mock;
 
-  function createComponent(props = {}, extendStore = () => {}) {
+  function createComponent(props = {}, extendStore = () => {}, provisions = {}) {
     const localVue = createLocalVue();
+    const provide = {
+      ...provisions,
+      glFeatures: {
+        ...(provisions.glFeatures || {}),
+      },
+    };
 
     localVue.use(Vuex);
 
@@ -52,6 +58,7 @@ describe('diffs/components/app', () => {
         showSuggestPopover: true,
         ...props,
       },
+      provide,
       store,
       methods: {
         isLatestVersion() {
@@ -82,7 +89,10 @@ describe('diffs/components/app', () => {
     window.mrTabs = oldMrTabs;
 
     // reset component
+    if (wrapper) {
       wrapper.destroy();
+      wrapper = null;
+    }
 
     mock.restore();
   });
@@ -455,76 +465,109 @@ describe('diffs/components/app', () => {
   });
 
   describe('keyboard shortcut navigation', () => {
-    const mappings = {
-      '[': -1,
-      k: -1,
-      ']': +1,
-      j: +1,
-    };
-    let spy;
-
-    describe('visible app', () => {
-      beforeEach(() => {
-        spy = jest.fn();
+    let spies = [];
+    let jumpSpy;
+    let moveSpy;
+
+    function setup(componentProps, featureFlags) {
+      createComponent(
+        componentProps,
+        ({ state }) => {
+          state.diffs.commit = { id: 'SHA123' };
+        },
+        { glFeatures: { mrCommitNeighborNav: true, ...featureFlags } },
+      );
 
-        createComponent({
-          shouldShow: true,
-        });
+      moveSpy = jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
+      jumpSpy = jest.fn();
+      spies = [jumpSpy, moveSpy];
       wrapper.setMethods({
-          jumpToFile: spy,
+        jumpToFile: jumpSpy,
       });
+    }
+
+    describe('visible app', () => {
+      it.each`
+        key    | name                         | spy  | args                           | featureFlags
+        ${'['} | ${'jumpToFile'}              | ${0} | ${[-1]}                        | ${{}}
+        ${'k'} | ${'jumpToFile'}              | ${0} | ${[-1]}                        | ${{}}
+        ${']'} | ${'jumpToFile'}              | ${0} | ${[+1]}                        | ${{}}
+        ${'j'} | ${'jumpToFile'}              | ${0} | ${[+1]}                        | ${{}}
+        ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'previous' }]} | ${{ mrCommitNeighborNav: true }}
+        ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'next' }]}     | ${{ mrCommitNeighborNav: true }}
+      `(
+        'calls `$name()` with correct parameters whenever the "$key" key is pressed',
+        ({ key, spy, args, featureFlags }) => {
+          setup({ shouldShow: true }, featureFlags);
+
+          return wrapper.vm.$nextTick().then(() => {
+            expect(spies[spy]).not.toHaveBeenCalled();
+
+            Mousetrap.trigger(key);
+
+            expect(spies[spy]).toHaveBeenCalledWith(...args);
           });
+        },
+      );
+
+      it.each`
+        key    | name                         | spy  | featureFlags
+        ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }}
+        ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }}
+      `(
+        'does not call `$name()` even when the correct key is pressed if the feature flag is disabled',
+        ({ key, spy, featureFlags }) => {
+          setup({ shouldShow: true }, featureFlags);
 
-      it.each(Object.keys(mappings))(
-        'calls `jumpToFile()` with correct parameter whenever pre-defined %s is pressed',
-        key => {
           return wrapper.vm.$nextTick().then(() => {
-            expect(spy).not.toHaveBeenCalled();
+            expect(spies[spy]).not.toHaveBeenCalled();
 
             Mousetrap.trigger(key);
 
-            expect(spy).toHaveBeenCalledWith(mappings[key]);
+            expect(spies[spy]).not.toHaveBeenCalled();
           });
         },
       );
 
-      it('does not call `jumpToFile()` when unknown key is pressed', done => {
-        wrapper.vm
-          .$nextTick()
-          .then(() => {
-            Mousetrap.trigger('d');
+      it.each`
+        key    | name                         | spy  | allowed
+        ${'d'} | ${'jumpToFile'}              | ${0} | ${['[', ']', 'j', 'k']}
+        ${'r'} | ${'moveToNeighboringCommit'} | ${1} | ${['x', 'c']}
+      `(
+        `does not call \`$name()\` when a key that is not one of \`$allowed\` is pressed`,
+        ({ key, spy }) => {
+          setup({ shouldShow: true }, { mrCommitNeighborNav: true });
 
-            expect(spy).not.toHaveBeenCalled();
-          })
-          .then(done)
-          .catch(done.fail);
+          return wrapper.vm.$nextTick().then(() => {
+            Mousetrap.trigger(key);
+
+            expect(spies[spy]).not.toHaveBeenCalled();
           });
+        },
+      );
     });
 
-    describe('hideen app', () => {
+    describe('hidden app', () => {
       beforeEach(() => {
-        spy = jest.fn();
+        setup({ shouldShow: false }, { mrCommitNeighborNav: true });
 
-        createComponent({
-          shouldShow: false,
-        });
-        wrapper.setMethods({
-          jumpToFile: spy,
+        return wrapper.vm.$nextTick().then(() => {
+          Mousetrap.reset();
         });
       });
 
-      it('stops calling `jumpToFile()` when application is hidden', done => {
-        wrapper.vm
-          .$nextTick()
-          .then(() => {
-            Object.keys(mappings).forEach(key => {
+      it.each`
+        key    | name                         | spy
+        ${'['} | ${'jumpToFile'}              | ${0}
+        ${'k'} | ${'jumpToFile'}              | ${0}
+        ${']'} | ${'jumpToFile'}              | ${0}
+        ${'j'} | ${'jumpToFile'}              | ${0}
+        ${'x'} | ${'moveToNeighboringCommit'} | ${1}
+        ${'c'} | ${'moveToNeighboringCommit'} | ${1}
+      `('stops calling `$name()` when the app is hidden', ({ key, spy }) => {
         Mousetrap.trigger(key);
 
-              expect(spy).not.toHaveBeenCalled();
-            });
-          })
-          .then(done)
-          .catch(done.fail);
+        expect(spies[spy]).not.toHaveBeenCalled();
       });
     });
   });

spec/frontend/diffs/components/commit_item_spec.js

@@ -13,6 +13,8 @@ const TEST_AUTHOR_EMAIL = 'test+test@gitlab.com';
 const TEST_AUTHOR_GRAVATAR = `${TEST_HOST}/avatar/test?s=40`;
 const TEST_SIGNATURE_HTML = '<a>Legit commit</a>';
 const TEST_PIPELINE_STATUS_PATH = `${TEST_HOST}/pipeline/status`;
+const NEXT_COMMIT_URL = `${TEST_HOST}/?commit_id=next`;
+const PREV_COMMIT_URL = `${TEST_HOST}/?commit_id=prev`;
 
 describe('diffs/components/commit_item', () => {
   let wrapper;
@@ -30,12 +32,24 @@ describe('diffs/components/commit_item', () => {
   const getCommitActionsElement = () => wrapper.find('.commit-actions');
   const getCommitPipelineStatus = () => wrapper.find(CommitPipelineStatus);
 
-  const mountComponent = propsData => {
+  const getCommitNavButtonsElement = () => wrapper.find('.commit-nav-buttons');
+  const getNextCommitNavElement = () =>
+    getCommitNavButtonsElement().find('.btn-group > *:last-child');
+  const getPrevCommitNavElement = () =>
+    getCommitNavButtonsElement().find('.btn-group > *:first-child');
+
+  const mountComponent = (propsData, featureFlags = {}) => {
     wrapper = mount(Component, {
       propsData: {
         commit,
         ...propsData,
       },
+      provide: {
+        glFeatures: {
+          mrCommitNeighborNav: true,
+          ...featureFlags,
+        },
+      },
       stubs: {
         CommitPipelineStatus: true,
       },
@@ -173,4 +187,132 @@ describe('diffs/components/commit_item', () => {
       expect(getCommitPipelineStatus().exists()).toBe(true);
     });
   });
+
+  describe('without neighbor commits', () => {
+    beforeEach(() => {
+      mountComponent({ commit: { ...commit, prev_commit_id: null, next_commit_id: null } });
+    });
+
+    it('does not render any navigation buttons', () => {
+      expect(getCommitNavButtonsElement().exists()).toEqual(false);
+    });
+  });
+
+  describe('with neighbor commits', () => {
+    let mrCommit;
+
+    beforeEach(() => {
+      mrCommit = {
+        ...commit,
+        next_commit_id: 'next',
+        prev_commit_id: 'prev',
+      };
+
+      mountComponent({ commit: mrCommit });
+    });
+
+    it('renders the commit navigation buttons', () => {
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+
+      mountComponent({
+        commit: { ...mrCommit, next_commit_id: null },
+      });
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+
+      mountComponent({
+        commit: { ...mrCommit, prev_commit_id: null },
+      });
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+    });
+
+    it('does not render the commit navigation buttons if the `mrCommitNeighborNav` feature flag is disabled', () => {
+      mountComponent({ commit: mrCommit }, { mrCommitNeighborNav: false });
+
+      expect(getCommitNavButtonsElement().exists()).toEqual(false);
+    });
+
+    describe('prev commit', () => {
+      const { location } = window;
+
+      beforeAll(() => {
+        delete window.location;
+        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
+      });
+
+      beforeEach(() => {
+        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
+      });
+
+      afterAll(() => {
+        window.location = location;
+      });
+
+      it('uses the correct href', () => {
+        const link = getPrevCommitNavElement();
+
+        expect(link.element.getAttribute('href')).toEqual(PREV_COMMIT_URL);
+      });
+
+      it('triggers the correct Vuex action on click', () => {
+        const link = getPrevCommitNavElement();
+
+        link.trigger('click');
+        return wrapper.vm.$nextTick().then(() => {
+          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({
+            direction: 'previous',
+          });
+        });
+      });
+
+      it('renders a disabled button when there is no prev commit', () => {
+        mountComponent({ commit: { ...mrCommit, prev_commit_id: null } });
+
+        const button = getPrevCommitNavElement();
+
+        expect(button.element.tagName).toEqual('BUTTON');
+        expect(button.element.hasAttribute('disabled')).toEqual(true);
+      });
+    });
+
+    describe('next commit', () => {
+      const { location } = window;
+
+      beforeAll(() => {
+        delete window.location;
+        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
+      });
+
+      beforeEach(() => {
+        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
+      });
+
+      afterAll(() => {
+        window.location = location;
+      });
+
+      it('uses the correct href', () => {
+        const link = getNextCommitNavElement();
+
+        expect(link.element.getAttribute('href')).toEqual(NEXT_COMMIT_URL);
+      });
+
+      it('triggers the correct Vuex action on click', () => {
+        const link = getNextCommitNavElement();
+
+        link.trigger('click');
+        return wrapper.vm.$nextTick().then(() => {
+          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({ direction: 'next' });
+        });
+      });
+
+      it('renders a disabled button when there is no next commit', () => {
+        mountComponent({ commit: { ...mrCommit, next_commit_id: null } });
+
+        const button = getNextCommitNavElement();
+
+        expect(button.element.tagName).toEqual('BUTTON');
+        expect(button.element.hasAttribute('disabled')).toEqual(true);
+      });
+    });
+  });
 });