Merge branch...

Merge branch '205401-cannot-view-content-of-moved-file-without-changes-in-merge-request' into 'master'

Add UI to load the full file for `renamed`-type files

See merge request gitlab-org/gitlab!28448

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

@@ -128,6 +128,7 @@ export default {
       <no-preview-viewer v-else-if="noPreview" />
       <diff-viewer
         v-else
+        :diff-file="diffFile"
         :diff-mode="diffMode"
         :diff-viewer-mode="diffViewerMode"
         :new-path="diffFile.new_path"

app/assets/javascripts/diffs/constants.js

@@ -61,3 +61,22 @@ export const DIFFS_PER_PAGE = 20;
 
 export const DIFF_COMPARE_BASE_VERSION_INDEX = -1;
 export const DIFF_COMPARE_HEAD_VERSION_INDEX = -2;
+
+// State machine states
+export const STATE_IDLING = 'idle';
+export const STATE_LOADING = 'loading';
+export const STATE_ERRORED = 'errored';
+
+// State machine transitions
+export const TRANSITION_LOAD_START = 'LOAD_START';
+export const TRANSITION_LOAD_ERROR = 'LOAD_ERROR';
+export const TRANSITION_LOAD_SUCCEED = 'LOAD_SUCCEED';
+export const TRANSITION_ACKNOWLEDGE_ERROR = 'ACKNOWLEDGE_ERROR';
+
+export const RENAMED_DIFF_TRANSITIONS = {
+  [`${STATE_IDLING}:${TRANSITION_LOAD_START}`]: STATE_LOADING,
+  [`${STATE_LOADING}:${TRANSITION_LOAD_ERROR}`]: STATE_ERRORED,
+  [`${STATE_LOADING}:${TRANSITION_LOAD_SUCCEED}`]: STATE_IDLING,
+  [`${STATE_ERRORED}:${TRANSITION_LOAD_START}`]: STATE_LOADING,
+  [`${STATE_ERRORED}:${TRANSITION_ACKNOWLEDGE_ERROR}`]: STATE_IDLING,
+};

app/assets/javascripts/diffs/store/actions.js

@@ -656,11 +656,6 @@ export function switchToFullDiffFromRenamedFile({ commit, dispatch, state }, { d
       commit(types.SET_CURRENT_VIEW_DIFF_FILE_LINES, { filePath: diffFile.file_path, lines });
 
       dispatch('startRenderDiffsQueue');
-    })
-    .catch(error => {
-      dispatch('receiveFullDiffError', diffFile.file_path);
-
-      throw error;
     });
 }
 

app/assets/javascripts/notes/components/diff_with_note.vue

@@ -116,6 +116,7 @@ export default {
     </div>
     <div v-else>
       <diff-viewer
+        :diff-file="discussion.diff_file"
         :diff-mode="diffMode"
         :diff-viewer-mode="diffViewerMode"
         :new-path="discussion.diff_file.new_path"

app/assets/javascripts/vue_shared/components/diff_viewer/diff_viewer.vue

@@ -7,6 +7,10 @@ import ModeChanged from './viewers/mode_changed.vue';
 
 export default {
   props: {
+    diffFile: {
+      type: Object,
+      required: true,
+    },
     diffMode: {
       type: String,
       required: true,
@@ -92,6 +96,7 @@ export default {
   <div v-if="viewer" class="diff-file preview-container">
     <component
       :is="viewer"
+      :diff-file="diffFile"
       :diff-mode="diffMode"
       :new-path="fullNewPath"
       :old-path="fullOldPath"

app/assets/javascripts/vue_shared/components/diff_viewer/viewers/renamed.vue

+<script>
+import { mapActions } from 'vuex';
+import { GlAlert, GlLink, GlLoadingIcon, GlSprintf } from '@gitlab/ui';
+
+import { __ } from '~/locale';
+import {
+  TRANSITION_LOAD_START,
+  TRANSITION_LOAD_ERROR,
+  TRANSITION_LOAD_SUCCEED,
+  TRANSITION_ACKNOWLEDGE_ERROR,
+  STATE_IDLING,
+  STATE_LOADING,
+  STATE_ERRORED,
+  RENAMED_DIFF_TRANSITIONS,
+} from '~/diffs/constants';
+import { truncateSha } from '~/lib/utils/text_utility';
+
+export default {
+  STATE_LOADING,
+  STATE_ERRORED,
+  TRANSITIONS: RENAMED_DIFF_TRANSITIONS,
+  uiText: {
+    showLink: __('Show file contents'),
+    commitLink: __('View file @ %{commitSha}'),
+    description: __('File renamed with no changes.'),
+    loadError: __('Unable to load file contents. Try again later.'),
+  },
+  components: {
+    GlAlert,
+    GlLink,
+    GlLoadingIcon,
+    GlSprintf,
+  },
+  props: {
+    diffFile: {
+      type: Object,
+      required: true,
+    },
+  },
+  data: () => ({
+    state: STATE_IDLING,
+  }),
+  computed: {
+    shortSha() {
+      return truncateSha(this.diffFile.content_sha);
+    },
+    canLoadFullDiff() {
+      return this.diffFile.alternate_viewer.name === 'text';
+    },
+  },
+  methods: {
+    ...mapActions('diffs', ['switchToFullDiffFromRenamedFile']),
+    transition(transitionEvent) {
+      const key = `${this.state}:${transitionEvent}`;
+
+      if (this.$options.TRANSITIONS[key]) {
+        this.state = this.$options.TRANSITIONS[key];
+      }
+    },
+    is(state) {
+      return this.state === state;
+    },
+    switchToFull() {
+      this.transition(TRANSITION_LOAD_START);
+
+      this.switchToFullDiffFromRenamedFile({ diffFile: this.diffFile })
+        .then(() => {
+          this.transition(TRANSITION_LOAD_SUCCEED);
+        })
+        .catch(() => {
+          this.transition(TRANSITION_LOAD_ERROR);
+        });
+    },
+    clickLink(event) {
+      if (this.canLoadFullDiff) {
+        event.preventDefault();
+
+        this.switchToFull();
+      }
+    },
+    dismissError() {
+      this.transition(TRANSITION_ACKNOWLEDGE_ERROR);
+    },
+  },
+};
+</script>
+
 <template>
-  <div class="nothing-here-block">{{ __('File moved') }}</div>
+  <div class="nothing-here-block">
+    <gl-loading-icon v-if="is($options.STATE_LOADING)" />
+    <template v-else>
+      <gl-alert
+        v-show="is($options.STATE_ERRORED)"
+        class="gl-mb-5 gl-text-left"
+        variant="danger"
+        @dismiss="dismissError"
+        >{{ $options.uiText.loadError }}</gl-alert
+      >
+      <span test-id="plaintext">{{ $options.uiText.description }}</span>
+      <gl-link :href="diffFile.view_path" @click="clickLink">
+        <span v-if="canLoadFullDiff">{{ $options.uiText.showLink }}</span>
+        <gl-sprintf v-else :message="$options.uiText.commitLink">
+          <template #commitSha>{{ shortSha }}</template>
+        </gl-sprintf>
+      </gl-link>
+    </template>
+  </div>
 </template>

changelogs/unreleased/upgrade-renamed-diff-show-full-diff-for-renamed-files.yml

+---
+title: Add a link to the `renamed` viewer to fully expand the renamed file (if it's text)
+merge_request: 28448
+author:
+type: added

doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md

@@ -82,6 +82,10 @@ to expand the entire file.
 
 ![Incrementally expand merge request diffs](img/incrementally_expand_merge_request_diffs_v12_2.png)
 
+[Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/205401) in GitLab 13.1, when viewing a
+merge request's **Changes** tab, if a certain file was only renamed, you can expand it to see the
+entire content by clicking **Show file contents**.
+
 ### Ignore whitespace changes in Merge Request diff view
 
 If you click the **Hide whitespace changes** button, you can see the diff

locale/gitlab.pot

@@ -9491,6 +9491,9 @@ msgstr ""
 msgid "File name"
 msgstr ""
 
+msgid "File renamed with no changes."
+msgstr ""
+
 msgid "File sync capacity"
 msgstr ""
 
@@ -19671,6 +19674,9 @@ msgstr ""
 msgid "Show file browser"
 msgstr ""
 
+msgid "Show file contents"
+msgstr ""
+
 msgid "Show latest version"
 msgstr ""
 
@@ -23061,6 +23067,9 @@ msgstr ""
 msgid "Unable to generate new instance ID"
 msgstr ""
 
+msgid "Unable to load file contents. Try again later."
+msgstr ""
+
 msgid "Unable to load the diff"
 msgstr ""
 
@@ -23983,6 +23992,9 @@ msgstr[1] ""
 msgid "View file @ "
 msgstr ""
 
+msgid "View file @ %{commitSha}"
+msgstr ""
+
 msgid "View full dashboard"
 msgstr ""
 

spec/frontend/diffs/store/actions_spec.js

@@ -1255,7 +1255,6 @@ describe('DiffsStoreActions', () => {
 
   describe('switchToFullDiffFromRenamedFile', () => {
     const SUCCESS_URL = 'fakehost/context.success';
-    const ERROR_URL = 'fakehost/context.error';
     const testFilePath = 'testpath';
     const updatedViewerName = 'testviewer';
     const preparedLine = { prepared: 'in-a-test' };
@@ -1311,27 +1310,6 @@ describe('DiffsStoreActions', () => {
         },
       );
     });
-
-    describe('error', () => {
-      beforeEach(() => {
-        renamedFile = { ...testFile, context_lines_path: ERROR_URL };
-        mock.onGet(ERROR_URL).reply(500);
-      });
-
-      it('dispatches the error handling action', () => {
-        const rejected = testAction(
-          switchToFullDiffFromRenamedFile,
-          { diffFile: renamedFile },
-          null,
-          [],
-          [{ type: 'receiveFullDiffError', payload: testFilePath }],
-        );
-
-        return rejected.catch(error =>
-          expect(error).toEqual(new Error('Request failed with status code 500')),
-        );
-      });
-    });
   });
 
   describe('setFileCollapsed', () => {

spec/frontend/notes/components/diff_with_note_spec.js

-import { mount } from '@vue/test-utils';
+import { shallowMount } from '@vue/test-utils';
 import DiffWithNote from '~/notes/components/diff_with_note.vue';
 import { createStore } from '~/mr_notes/stores';
 
@@ -37,7 +37,7 @@ describe('diff_with_note', () => {
     beforeEach(() => {
       const diffDiscussion = getJSONFixture(discussionFixture)[0];
 
-      wrapper = mount(DiffWithNote, {
+      wrapper = shallowMount(DiffWithNote, {
         propsData: {
           discussion: diffDiscussion,
         },
@@ -76,7 +76,10 @@ describe('diff_with_note', () => {
   describe('image diff', () => {
     beforeEach(() => {
       const imageDiscussion = getJSONFixture(imageDiscussionFixture)[0];
-      wrapper = mount(DiffWithNote, { propsData: { discussion: imageDiscussion }, store });
+      wrapper = shallowMount(DiffWithNote, {
+        propsData: { discussion: imageDiscussion, diffFile: {} },
+        store,
+      });
     });
 
     it('shows image diff', () => {

spec/frontend/vue_shared/components/diff_viewer/diff_viewer_spec.js

@@ -8,6 +8,7 @@ describe('DiffViewer', () => {
   const requiredProps = {
     diffMode: 'replaced',
     diffViewerMode: 'image',
+    diffFile: {},
     newPath: GREEN_BOX_IMAGE_URL,
     newSha: 'ABC',
     oldPath: RED_BOX_IMAGE_URL,
@@ -71,16 +72,27 @@ describe('DiffViewer', () => {
     });
   });
 
-  it('renders renamed component', () => {
+  describe('renamed file', () => {
+    it.each`
+      altViewer
+      ${'text'}
+      ${'notText'}
+    `('renders the renamed component when the alternate viewer is $altViewer', ({ altViewer }) => {
       createComponent({
         ...requiredProps,
+        diffFile: {
+          content_sha: '',
+          view_path: '',
+          alternate_viewer: { name: altViewer },
+        },
         diffMode: 'renamed',
         diffViewerMode: 'renamed',
         newPath: 'test.abc',
         oldPath: 'testold.abc',
       });
 
-    expect(vm.$el.textContent).toContain('File moved');
+      expect(vm.$el.textContent).toContain('File renamed with no changes.');
+    });
   });
 
   it('renders mode changed component', () => {

spec/frontend/vue_shared/components/diff_viewer/viewers/renamed_spec.js

+import Vuex from 'vuex';
+import { createLocalVue, shallowMount, mount } from '@vue/test-utils';
+import Renamed from '~/vue_shared/components/diff_viewer/viewers/renamed.vue';
+import {
+  TRANSITION_LOAD_START,
+  TRANSITION_LOAD_ERROR,
+  TRANSITION_LOAD_SUCCEED,
+  TRANSITION_ACKNOWLEDGE_ERROR,
+  STATE_IDLING,
+  STATE_LOADING,
+  STATE_ERRORED,
+} from '~/diffs/constants';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
+
+function createRenamedComponent({
+  props = {},
+  methods = {},
+  store = new Vuex.Store({}),
+  deep = false,
+}) {
+  const mnt = deep ? mount : shallowMount;
+
+  return mnt(Renamed, {
+    propsData: { ...props },
+    localVue,
+    store,
+    methods,
+  });
+}
+
+describe('Renamed Diff Viewer', () => {
+  const DIFF_FILE_COMMIT_SHA = 'commitsha';
+  const DIFF_FILE_SHORT_SHA = 'commitsh';
+  const DIFF_FILE_VIEW_PATH = `blob/${DIFF_FILE_COMMIT_SHA}/filename.ext`;
+  let diffFile;
+  let wrapper;
+
+  beforeEach(() => {
+    diffFile = {
+      content_sha: DIFF_FILE_COMMIT_SHA,
+      view_path: DIFF_FILE_VIEW_PATH,
+      alternate_viewer: {
+        name: 'text',
+      },
+    };
+  });
+
+  afterEach(() => {
+    if (wrapper) {
+      wrapper.destroy();
+      wrapper = null;
+    }
+  });
+
+  describe('is', () => {
+    beforeEach(() => {
+      wrapper = createRenamedComponent({ props: { diffFile } });
+    });
+
+    it.each`
+      state        | request      | result
+      ${'idle'}    | ${'idle'}    | ${true}
+      ${'idle'}    | ${'loading'} | ${false}
+      ${'idle'}    | ${'errored'} | ${false}
+      ${'loading'} | ${'loading'} | ${true}
+      ${'loading'} | ${'idle'}    | ${false}
+      ${'loading'} | ${'errored'} | ${false}
+      ${'errored'} | ${'errored'} | ${true}
+      ${'errored'} | ${'idle'}    | ${false}
+      ${'errored'} | ${'loading'} | ${false}
+    `(
+      'returns the $result for "$request" when the state is "$state"',
+      ({ request, result, state }) => {
+        wrapper.vm.state = state;
+
+        expect(wrapper.vm.is(request)).toEqual(result);
+      },
+    );
+  });
+
+  describe('transition', () => {
+    beforeEach(() => {
+      wrapper = createRenamedComponent({ props: { diffFile } });
+    });
+
+    it.each`
+      state        | transition                      | result
+      ${'idle'}    | ${TRANSITION_LOAD_START}        | ${STATE_LOADING}
+      ${'idle'}    | ${TRANSITION_LOAD_ERROR}        | ${STATE_IDLING}
+      ${'idle'}    | ${TRANSITION_LOAD_SUCCEED}      | ${STATE_IDLING}
+      ${'idle'}    | ${TRANSITION_ACKNOWLEDGE_ERROR} | ${STATE_IDLING}
+      ${'loading'} | ${TRANSITION_LOAD_START}        | ${STATE_LOADING}
+      ${'loading'} | ${TRANSITION_LOAD_ERROR}        | ${STATE_ERRORED}
+      ${'loading'} | ${TRANSITION_LOAD_SUCCEED}      | ${STATE_IDLING}
+      ${'loading'} | ${TRANSITION_ACKNOWLEDGE_ERROR} | ${STATE_LOADING}
+      ${'errored'} | ${TRANSITION_LOAD_START}        | ${STATE_LOADING}
+      ${'errored'} | ${TRANSITION_LOAD_ERROR}        | ${STATE_ERRORED}
+      ${'errored'} | ${TRANSITION_LOAD_SUCCEED}      | ${STATE_ERRORED}
+      ${'errored'} | ${TRANSITION_ACKNOWLEDGE_ERROR} | ${STATE_IDLING}
+    `(
+      'correctly updates the state to "$result" when it starts as "$state" and the transition is "$transition"',
+      ({ state, transition, result }) => {
+        wrapper.vm.state = state;
+
+        wrapper.vm.transition(transition);
+
+        expect(wrapper.vm.state).toEqual(result);
+      },
+    );
+  });
+
+  describe('switchToFull', () => {
+    let store;
+
+    beforeEach(() => {
+      store = new Vuex.Store({
+        modules: {
+          diffs: {
+            namespaced: true,
+            actions: { switchToFullDiffFromRenamedFile: () => {} },
+          },
+        },
+      });
+
+      jest.spyOn(store, 'dispatch');
+
+      wrapper = createRenamedComponent({ props: { diffFile }, store });
+    });
+
+    afterEach(() => {
+      store = null;
+    });
+
+    it('calls the switchToFullDiffFromRenamedFile action when the method is triggered', () => {
+      store.dispatch.mockResolvedValue();
+
+      wrapper.vm.switchToFull();
+
+      return wrapper.vm.$nextTick().then(() => {
+        expect(store.dispatch).toHaveBeenCalledWith('diffs/switchToFullDiffFromRenamedFile', {
+          diffFile,
+        });
+      });
+    });
+
+    it.each`
+      after            | resolvePromise         | resolution
+      ${STATE_IDLING}  | ${'mockResolvedValue'} | ${'successful'}
+      ${STATE_ERRORED} | ${'mockRejectedValue'} | ${'rejected'}
+    `(
+      'moves through the correct states during a $resolution request',
+      ({ after, resolvePromise }) => {
+        store.dispatch[resolvePromise]();
+
+        expect(wrapper.vm.state).toEqual(STATE_IDLING);
+
+        wrapper.vm.switchToFull();
+
+        expect(wrapper.vm.state).toEqual(STATE_LOADING);
+
+        return (
+          wrapper.vm
+            // This tick is needed for when the action (promise) finishes
+            .$nextTick()
+            // This tick waits for the state change in the promise .then/.catch to bubble into the component
+            .then(() => wrapper.vm.$nextTick())
+            .then(() => {
+              expect(wrapper.vm.state).toEqual(after);
+            })
+        );
+      },
+    );
+  });
+
+  describe('clickLink', () => {
+    let event;
+
+    beforeEach(() => {
+      event = {
+        preventDefault: jest.fn(),
+      };
+    });
+
+    it.each`
+      alternateViewer | stops    | handled
+      ${'text'}       | ${true}  | ${'should'}
+      ${'nottext'}    | ${false} | ${'should not'}
+    `(
+      'given { alternate_viewer: { name: "$alternateViewer" } }, the click event $handled be handled in the component',
+      ({ alternateViewer, stops }) => {
+        wrapper = createRenamedComponent({
+          props: {
+            diffFile: {
+              ...diffFile,
+              alternate_viewer: { name: alternateViewer },
+            },
+          },
+        });
+
+        jest.spyOn(wrapper.vm, 'switchToFull').mockImplementation(() => {});
+
+        wrapper.vm.clickLink(event);
+
+        if (stops) {
+          expect(event.preventDefault).toHaveBeenCalled();
+          expect(wrapper.vm.switchToFull).toHaveBeenCalled();
+        } else {
+          expect(event.preventDefault).not.toHaveBeenCalled();
+          expect(wrapper.vm.switchToFull).not.toHaveBeenCalled();
+        }
+      },
+    );
+  });
+
+  describe('dismissError', () => {
+    let transitionSpy;
+
+    beforeEach(() => {
+      wrapper = createRenamedComponent({ props: { diffFile } });
+      transitionSpy = jest.spyOn(wrapper.vm, 'transition');
+    });
+
+    it(`transitions the component with "${TRANSITION_ACKNOWLEDGE_ERROR}"`, () => {
+      wrapper.vm.dismissError();
+
+      expect(transitionSpy).toHaveBeenCalledWith(TRANSITION_ACKNOWLEDGE_ERROR);
+    });
+  });
+
+  describe('output', () => {
+    it.each`
+      altViewer    | nameDisplay
+      ${'text'}    | ${'"text"'}
+      ${'nottext'} | ${'"nottext"'}
+      ${undefined} | ${undefined}
+      ${null}      | ${null}
+    `(
+      'with { alternate_viewer: { name: $nameDisplay } }, renders the component',
+      ({ altViewer }) => {
+        const file = { ...diffFile };
+
+        file.alternate_viewer.name = altViewer;
+        wrapper = createRenamedComponent({ props: { diffFile: file } });
+
+        expect(wrapper.find('[test-id="plaintext"]').text()).toEqual(
+          'File renamed with no changes.',
+        );
+      },
+    );
+
+    it.each`
+      altType      | linkText
+      ${'text'}    | ${'Show file contents'}
+      ${'nottext'} | ${`View file @ ${DIFF_FILE_SHORT_SHA}`}
+    `(
+      'includes a link to the full file for alternate viewer type "$altType"',
+      ({ altType, linkText }) => {
+        const file = { ...diffFile };
+        const clickMock = jest.fn().mockImplementation(() => {});
+
+        file.alternate_viewer.name = altType;
+        wrapper = createRenamedComponent({
+          deep: true,
+          props: { diffFile: file },
+          methods: {
+            clickLink: clickMock,
+          },
+        });
+
+        const link = wrapper.find('a');
+
+        expect(link.text()).toEqual(linkText);
+        expect(link.attributes('href')).toEqual(DIFF_FILE_VIEW_PATH);
+
+        link.trigger('click');
+
+        expect(clickMock).toHaveBeenCalled();
+      },
+    );
+  });
+});