Merge branch...

Merge branch '256035-empty-changes-tab-on-merge-requests-should-not-show-file-browser-and-display-options' into 'master'

Hide file browser and display options from empty state changes tab

See merge request gitlab-org/gitlab!43467

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

@@ -145,8 +145,6 @@ export default {
       isBatchLoading: (state) => state.diffs.isBatchLoading,
       diffFiles: (state) => state.diffs.diffFiles,
       diffViewType: (state) => state.diffs.diffViewType,
-      mergeRequestDiffs: (state) => state.diffs.mergeRequestDiffs,
-      mergeRequestDiff: (state) => state.diffs.mergeRequestDiff,
       commit: (state) => state.diffs.commit,
       renderOverflowWarning: (state) => state.diffs.renderOverflowWarning,
       numTotalFiles: (state) => state.diffs.realSize,
@@ -186,17 +184,16 @@ export default {
       return this.currentUser.can_fork === true && this.currentUser.can_create_merge_request;
     },
     renderDiffFiles() {
-      return (
-        this.diffFiles.length > 0 ||
-        (this.startVersion &&
-          this.startVersion.version_index === this.mergeRequestDiff.version_index)
-      );
+      return this.diffFiles.length > 0;
+    },
+    renderFileTree() {
+      return this.renderDiffFiles && this.showTreeList;
     },
     hideFileStats() {
       return this.treeWidth <= TREE_HIDE_STATS_WIDTH;
     },
     isLimitedContainer() {
-      return !this.showTreeList && !this.isParallelView && !this.isFluidLayout;
+      return !this.renderFileTree && !this.isParallelView && !this.isFluidLayout;
     },
     isDiffHead() {
       return parseBoolean(getParameterByName('diff_head'));
@@ -259,7 +256,7 @@ export default {
       this.adjustView();
     },
     isLoading: 'adjustView',
-    showTreeList: 'adjustView',
+    renderFileTree: 'adjustView',
   },
   mounted() {
     this.setBaseConfig({
@@ -467,7 +464,6 @@ export default {
     <div v-if="isLoading || !isTreeLoaded" class="loading"><gl-loading-icon size="lg" /></div>
     <div v-else id="diffs" :class="{ active: shouldShow }" class="diffs tab-pane">
       <compare-versions
-        :merge-request-diffs="mergeRequestDiffs"
         :is-limited-container="isLimitedContainer"
         :diff-files-count-text="numTotalFiles"
       />
@@ -495,7 +491,7 @@ export default {
         class="files d-flex gl-mt-2"
       >
         <div
-          v-if="showTreeList"
+          v-if="renderFileTree"
           :style="{ width: `${treeWidth}px` }"
           class="diff-tree-list js-diff-tree-list px-3 pr-md-0"
         >

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

@@ -22,10 +22,6 @@ export default {
     GlTooltip: GlTooltipDirective,
   },
   props: {
-    mergeRequestDiffs: {
-      type: Array,
-      required: true,
-    },
     isLimitedContainer: {
       type: Boolean,
       required: false,
@@ -44,6 +40,7 @@ export default {
       'diffCompareDropdownSourceVersions',
     ]),
     ...mapState('diffs', [
+      'diffFiles',
       'commit',
       'showTreeList',
       'startVersion',
@@ -51,12 +48,15 @@ export default {
       'addedLines',
       'removedLines',
     ]),
-    showDropdowns() {
-      return !this.commit && this.mergeRequestDiffs.length;
-    },
     toggleFileBrowserTitle() {
       return this.showTreeList ? __('Hide file browser') : __('Show file browser');
     },
+    hasChanges() {
+      return this.diffFiles.length > 0;
+    },
+    hasSourceVersions() {
+      return this.diffCompareDropdownSourceVersions.length > 0;
+    },
   },
   created() {
     this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
@@ -82,6 +82,7 @@ export default {
       }"
     >
       <gl-button
+        v-if="hasChanges"
         v-gl-tooltip.hover
         variant="default"
         icon="file-tree"
@@ -90,8 +91,12 @@ export default {
         :selected="showTreeList"
         @click="setShowTreeList({ showTreeList: !showTreeList })"
       />
+      <div v-if="commit">
+        {{ __('Viewing commit') }}
+        <gl-link :href="commit.commit_url" class="monospace">{{ commit.short_id }}</gl-link>
+      </div>
       <gl-sprintf
-        v-if="showDropdowns"
+        v-else-if="hasSourceVersions"
         class="d-flex align-items-center compare-versions-container"
         :message="s__('MergeRequest|Compare %{target} and %{source}')"
       >
@@ -109,11 +114,7 @@ export default {
           />
         </template>
       </gl-sprintf>
-      <div v-else-if="commit">
-        {{ __('Viewing commit') }}
-        <gl-link :href="commit.commit_url" class="monospace">{{ commit.short_id }}</gl-link>
-      </div>
-      <div class="inline-parallel-buttons d-none d-md-flex ml-auto">
+      <div v-if="hasChanges" class="inline-parallel-buttons d-none d-md-flex ml-auto">
         <diff-stats
           :diff-files-count-text="diffFilesCountText"
           :added-lines="addedLines"

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

@@ -14,7 +14,31 @@ export default {
     },
   },
   computed: {
+    ...mapGetters('diffs', [
+      'diffCompareDropdownTargetVersions',
+      'diffCompareDropdownSourceVersions',
+    ]),
     ...mapGetters(['getNoteableData']),
+    selectedSourceVersion() {
+      return this.diffCompareDropdownSourceVersions.find((x) => x.selected);
+    },
+    sourceName() {
+      if (!this.selectedSourceVersion || this.selectedSourceVersion.isLatestVersion) {
+        return this.getNoteableData.source_branch;
+      }
+
+      return this.selectedSourceVersion.versionName;
+    },
+    selectedTargetVersion() {
+      return this.diffCompareDropdownTargetVersions.find((x) => x.selected);
+    },
+    targetName() {
+      if (!this.selectedTargetVersion || this.selectedTargetVersion.version_index < 0) {
+        return this.getNoteableData.target_branch;
+      }
+
+      return this.selectedTargetVersion.versionName || '';
+    },
   },
 };
 </script>
@@ -26,14 +50,16 @@ export default {
     </div>
     <div class="col-12">
       <div class="text-content text-center">
-        <gl-sprintf :message="__('No changes between %{sourceBranch} and %{targetBranch}')">
-          <template #sourceBranch>
-            <span class="ref-name">{{ getNoteableData.source_branch }}</span>
-          </template>
-          <template #targetBranch>
-            <span class="ref-name">{{ getNoteableData.target_branch }}</span>
-          </template>
-        </gl-sprintf>
+        <div data-testid="no-changes-message">
+          <gl-sprintf :message="__('No changes between %{source} and %{target}')">
+            <template #source>
+              <span class="ref-name">{{ sourceName }}</span>
+            </template>
+            <template #target>
+              <span class="ref-name">{{ targetName }}</span>
+            </template>
+          </gl-sprintf>
+        </div>
         <div class="text-center">
           <gl-button :href="getNoteableData.new_blob_path" variant="success" category="primary">{{
             __('Create commit')

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

@@ -58,14 +58,18 @@ export const diffCompareDropdownTargetVersions = (state, getters) => {
 
 export const diffCompareDropdownSourceVersions = (state, getters) => {
   // Appended properties here are to make the compare_dropdown_layout easier to reason about
-  return state.mergeRequestDiffs.map((v, i) => ({
-    ...v,
-    href: v.version_path,
-    commitsText: n__(`%d commit,`, `%d commits,`, v.commits_count),
-    versionName:
-      i === 0
+  return state.mergeRequestDiffs.map((v, i) => {
+    const isLatestVersion = i === 0;
+
+    return {
+      ...v,
+      href: v.version_path,
+      commitsText: n__(`%d commit,`, `%d commits,`, v.commits_count),
+      isLatestVersion,
+      versionName: isLatestVersion
         ? __('latest version')
         : sprintf(__(`version %{versionIndex}`), { versionIndex: v.version_index }),
-    selected: v.version_index === getters.selectedSourceIndex,
-  }));
+      selected: v.version_index === getters.selectedSourceIndex,
+    };
+  });
 };

changelogs/unreleased/256035-empty-changes-tab-on-merge-requests-should-not-show-file-browser-a.yml

+---
+title: Remove diff display preferences and file tree from changes empty state
+merge_request: 43467
+author:
+type: fixed

locale/gitlab.pot

@@ -18935,7 +18935,7 @@ msgstr ""
 msgid "No changes"
 msgstr ""
 
-msgid "No changes between %{sourceBranch} and %{targetBranch}"
+msgid "No changes between %{source} and %{target}"
 msgstr ""
 
 msgid "No child epics match applied filters"

spec/features/merge_request/user_sees_versions_spec.rb

@@ -191,7 +191,7 @@ RSpec.describe 'Merge request > User sees versions', :js do
         find('.btn-default').click
         click_link 'version 1'
       end
-      expect(page).to have_content '0 files'
+      expect(page).to have_content 'No changes between version 1 and version 1'
     end
   end
 
@@ -217,7 +217,7 @@ RSpec.describe 'Merge request > User sees versions', :js do
         expect(page).to have_content 'version 1'
       end
 
-      expect(page).to have_content '0 files'
+      expect(page).to have_content 'No changes between version 1 and version 1'
     end
   end
 

spec/frontend/diffs/components/app_spec.js

@@ -147,31 +147,21 @@ describe('diffs/components/app', () => {
     });
   });
 
-  it('adds container-limiting classes when showFileTree is false with inline diffs', () => {
-    createComponent({}, ({ state }) => {
-      state.diffs.showTreeList = false;
-      state.diffs.isParallelView = false;
-    });
-
-    expect(wrapper.find('.container-limited.limit-container-width').exists()).toBe(true);
-  });
-
-  it('does not add container-limiting classes when showFileTree is false with inline diffs', () => {
-    createComponent({}, ({ state }) => {
-      state.diffs.showTreeList = true;
-      state.diffs.isParallelView = false;
-    });
-
-    expect(wrapper.find('.container-limited.limit-container-width').exists()).toBe(false);
-  });
-
-  it('does not add container-limiting classes when isFluidLayout', () => {
-    createComponent({ isFluidLayout: true }, ({ state }) => {
-      state.diffs.isParallelView = false;
-    });
-
-    expect(wrapper.find('.container-limited.limit-container-width').exists()).toBe(false);
-  });
+  it.each`
+    props                      | state                                                              | expected
+    ${{ isFluidLayout: true }} | ${{ isParallelView: false }}                                       | ${false}
+    ${{}}                      | ${{ isParallelView: false }}                                       | ${true}
+    ${{}}                      | ${{ showTreeList: true, diffFiles: [{}], isParallelView: false }}  | ${false}
+    ${{}}                      | ${{ showTreeList: false, diffFiles: [{}], isParallelView: false }} | ${true}
+    ${{}}                      | ${{ showTreeList: false, diffFiles: [], isParallelView: false }}   | ${true}
+  `(
+    'uses container-limiting classes ($expected) with state ($state) and props ($props)',
+    ({ props, state, expected }) => {
+      createComponent(props, ({ state: origState }) => Object.assign(origState.diffs, state));
+
+      expect(wrapper.find('.container-limited.limit-container-width').exists()).toBe(expected);
+    },
+  );
 
   it('displays loading icon on loading', () => {
     createComponent({}, ({ state }) => {
@@ -249,7 +239,9 @@ describe('diffs/components/app', () => {
     });
 
     it('sets width of tree list', () => {
-      createComponent();
+      createComponent({}, ({ state }) => {
+        state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }];
+      });
 
       expect(wrapper.find('.js-diff-tree-list').element.style.width).toEqual('320px');
     });
@@ -283,15 +275,6 @@ describe('diffs/components/app', () => {
       expect(wrapper.find(NoChanges).exists()).toBe(false);
       expect(wrapper.findAll(DiffFile).length).toBe(1);
     });
-
-    it('does not render empty state when versions match', () => {
-      createComponent({}, ({ state }) => {
-        state.diffs.startVersion = mergeRequestDiff;
-        state.diffs.mergeRequestDiff = mergeRequestDiff;
-      });
-
-      expect(wrapper.find(NoChanges).exists()).toBe(false);
-    });
   });
 
   describe('keyboard shortcut navigation', () => {
@@ -517,6 +500,7 @@ describe('diffs/components/app', () => {
   describe('diffs', () => {
     it('should render compare versions component', () => {
       createComponent({}, ({ state }) => {
+        state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }];
         state.diffs.mergeRequestDiffs = diffsMockData;
         state.diffs.targetBranchName = 'target-branch';
         state.diffs.mergeRequestDiff = mergeRequestDiff;
@@ -525,7 +509,8 @@ describe('diffs/components/app', () => {
       expect(wrapper.find(CompareVersions).exists()).toBe(true);
       expect(wrapper.find(CompareVersions).props()).toEqual(
         expect.objectContaining({
-          mergeRequestDiffs: diffsMockData,
+          isLimitedContainer: false,
+          diffFilesCountText: null,
         }),
       );
     });
@@ -593,9 +578,17 @@ describe('diffs/components/app', () => {
       expect(wrapper.find(DiffFile).exists()).toBe(true);
     });
 
-    it('should render tree list', () => {
+    it("doesn't render tree list when no changes exist", () => {
       createComponent();
 
+      expect(wrapper.find(TreeList).exists()).toBe(false);
+    });
+
+    it('should render tree list', () => {
+      createComponent({}, ({ state }) => {
+        state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }];
+      });
+
       expect(wrapper.find(TreeList).exists()).toBe(true);
     });
   });

spec/frontend/diffs/components/compare_versions_spec.js

@@ -11,32 +11,34 @@ localVue.use(Vuex);
 
 describe('CompareVersions', () => {
   let wrapper;
+  let store;
   const targetBranchName = 'tmp-wine-dev';
 
   const createWrapper = (props) => {
-    const store = createStore();
-    const mergeRequestDiff = diffsMockData[0];
-
-    store.state.diffs.addedLines = 10;
-    store.state.diffs.removedLines = 20;
-    store.state.diffs.diffFiles.push('test');
-    store.state.diffs.targetBranchName = targetBranchName;
-    store.state.diffs.mergeRequestDiff = mergeRequestDiff;
-    store.state.diffs.mergeRequestDiffs = diffsMockData;
-
     wrapper = mount(CompareVersionsComponent, {
       localVue,
       store,
       propsData: {
         mergeRequestDiffs: diffsMockData,
-        diffFilesCountText: null,
+        diffFilesCountText: '1',
         ...props,
       },
     });
   };
+  const findLimitedContainer = () => wrapper.find('.container-limited.limit-container-width');
+  const findCompareSourceDropdown = () => wrapper.find('.mr-version-dropdown');
+  const findCompareTargetDropdown = () => wrapper.find('.mr-version-compare-dropdown');
 
   beforeEach(() => {
-    createWrapper();
+    store = createStore();
+    const mergeRequestDiff = diffsMockData[0];
+
+    store.state.diffs.addedLines = 10;
+    store.state.diffs.removedLines = 20;
+    store.state.diffs.diffFiles.push('test');
+    store.state.diffs.targetBranchName = targetBranchName;
+    store.state.diffs.mergeRequestDiff = mergeRequestDiff;
+    store.state.diffs.mergeRequestDiffs = diffsMockData;
   });
 
   afterEach(() => {
@@ -45,6 +47,10 @@ describe('CompareVersions', () => {
   });
 
   describe('template', () => {
+    beforeEach(() => {
+      createWrapper();
+    });
+
     it('should render Tree List toggle button with correct attribute values', () => {
       const treeListBtn = wrapper.find('.js-toggle-tree-list');
 
@@ -54,8 +60,8 @@ describe('CompareVersions', () => {
     });
 
     it('should render comparison dropdowns with correct values', () => {
-      const sourceDropdown = wrapper.find('.mr-version-dropdown');
-      const targetDropdown = wrapper.find('.mr-version-compare-dropdown');
+      const sourceDropdown = findCompareSourceDropdown();
+      const targetDropdown = findCompareTargetDropdown();
 
       expect(sourceDropdown.exists()).toBe(true);
       expect(targetDropdown.exists()).toBe(true);
@@ -63,16 +69,6 @@ describe('CompareVersions', () => {
       expect(targetDropdown.find('button').html()).toContain(targetBranchName);
     });
 
-    it('should not render comparison dropdowns if no mergeRequestDiffs are specified', () => {
-      createWrapper({ mergeRequestDiffs: [] });
-
-      const sourceDropdown = wrapper.find('.mr-version-dropdown');
-      const targetDropdown = wrapper.find('.mr-version-compare-dropdown');
-
-      expect(sourceDropdown.exists()).toBe(false);
-      expect(targetDropdown.exists()).toBe(false);
-    });
-
     it('should render view types buttons with correct values', () => {
       const inlineBtn = wrapper.find('#inline-diff-btn');
       const parallelBtn = wrapper.find('#parallel-diff-btn');
@@ -88,22 +84,34 @@ describe('CompareVersions', () => {
     it('adds container-limiting classes when showFileTree is false with inline diffs', () => {
       createWrapper({ isLimitedContainer: true });
 
-      const limitedContainer = wrapper.find('.container-limited.limit-container-width');
-
-      expect(limitedContainer.exists()).toBe(true);
+      expect(findLimitedContainer().exists()).toBe(true);
     });
 
     it('does not add container-limiting classes when showFileTree is false with inline diffs', () => {
       createWrapper({ isLimitedContainer: false });
 
-      const limitedContainer = wrapper.find('.container-limited.limit-container-width');
+      expect(findLimitedContainer().exists()).toBe(false);
+    });
+  });
 
-      expect(limitedContainer.exists()).toBe(false);
+  describe('noChangedFiles', () => {
+    beforeEach(() => {
+      store.state.diffs.diffFiles = [];
+    });
+
+    it('should not render Tree List toggle button when there are no changes', () => {
+      createWrapper();
+
+      const treeListBtn = wrapper.find('.js-toggle-tree-list');
+
+      expect(treeListBtn.exists()).toBe(false);
     });
   });
 
   describe('setInlineDiffViewType', () => {
     it('should persist the view type in the url', () => {
+      createWrapper();
+
       const viewTypeBtn = wrapper.find('#inline-diff-btn');
       viewTypeBtn.trigger('click');
 
@@ -113,6 +121,7 @@ describe('CompareVersions', () => {
 
   describe('setParallelDiffViewType', () => {
     it('should persist the view type in the url', () => {
+      createWrapper();
       const viewTypeBtn = wrapper.find('#parallel-diff-btn');
       viewTypeBtn.trigger('click');
 
@@ -121,11 +130,14 @@ describe('CompareVersions', () => {
   });
 
   describe('commit', () => {
-    beforeEach((done) => {
-      wrapper.vm.$store.state.diffs.commit = getDiffWithCommit().commit;
-      wrapper.mergeRequestDiffs = [];
+    beforeEach(() => {
+      store.state.diffs.commit = getDiffWithCommit().commit;
+      createWrapper();
+    });
 
-      wrapper.vm.$nextTick(done);
+    it('does not render compare dropdowns', () => {
+      expect(findCompareSourceDropdown().exists()).toBe(false);
+      expect(findCompareTargetDropdown().exists()).toBe(false);
     });
 
     it('renders latest version button', () => {
@@ -137,4 +149,16 @@ describe('CompareVersions', () => {
       expect(wrapper.text()).toContain(wrapper.vm.commit.short_id);
     });
   });
+
+  describe('with no versions', () => {
+    beforeEach(() => {
+      store.state.diffs.mergeRequestDiffs = [];
+      createWrapper();
+    });
+
+    it('does not render compare dropdowns', () => {
+      expect(findCompareSourceDropdown().exists()).toBe(false);
+      expect(findCompareTargetDropdown().exists()).toBe(false);
+    });
+  });
 });

spec/frontend/diffs/components/no_changes_spec.js

-import { createLocalVue, shallowMount } from '@vue/test-utils';
+import { createLocalVue, shallowMount, mount } from '@vue/test-utils';
 import Vuex from 'vuex';
 import { GlButton } from '@gitlab/ui';
 import { createStore } from '~/mr_notes/stores';
 import NoChanges from '~/diffs/components/no_changes.vue';
+import diffsMockData from '../mock_data/merge_request_diffs';
 
-describe('Diff no changes empty state', () => {
-  let vm;
+const localVue = createLocalVue();
+localVue.use(Vuex);
 
-  function createComponent(extendStore = () => {}) {
-    const localVue = createLocalVue();
-    localVue.use(Vuex);
+const TEST_TARGET_BRANCH = 'foo';
+const TEST_SOURCE_BRANCH = 'dev/update';
 
-    const store = createStore();
-    extendStore(store);
+describe('Diff no changes empty state', () => {
+  let wrapper;
+  let store;
 
-    vm = shallowMount(NoChanges, {
+  function createComponent(mountFn = shallowMount) {
+    wrapper = mountFn(NoChanges, {
       localVue,
       store,
       propsData: {
@@ -23,26 +25,61 @@ describe('Diff no changes empty state', () => {
     });
   }
 
+  beforeEach(() => {
+    store = createStore();
+    store.state.diffs.mergeRequestDiff = {};
+    store.state.notes.noteableData = {
+      target_branch: TEST_TARGET_BRANCH,
+      source_branch: TEST_SOURCE_BRANCH,
+    };
+    store.state.diffs.mergeRequestDiffs = diffsMockData;
+  });
+
   afterEach(() => {
-    vm.destroy();
+    wrapper.destroy();
+    wrapper = null;
   });
 
+  const findMessage = () => wrapper.find('[data-testid="no-changes-message"]');
+
   it('prevents XSS', () => {
-    createComponent((store) => {
-      // eslint-disable-next-line no-param-reassign
-      store.state.notes.noteableData = {
-        source_branch: '<script>alert("test");</script>',
-        target_branch: '<script>alert("test");</script>',
-      };
-    });
+    store.state.notes.noteableData = {
+      source_branch: '<script>alert("test");</script>',
+      target_branch: '<script>alert("test");</script>',
+    };
 
-    expect(vm.find('script').exists()).toBe(false);
+    createComponent();
+
+    expect(wrapper.find('script').exists()).toBe(false);
   });
 
   describe('Renders', () => {
     it('Show create commit button', () => {
       createComponent();
-      expect(vm.find(GlButton).exists()).toBe(true);
+
+      expect(wrapper.find(GlButton).exists()).toBe(true);
     });
+
+    it.each`
+      expectedText                                                            | sourceIndex                       | targetIndex
+      ${`No changes between ${TEST_SOURCE_BRANCH} and ${TEST_TARGET_BRANCH}`} | ${null}                           | ${null}
+      ${`No changes between ${TEST_SOURCE_BRANCH} and version 1`}             | ${diffsMockData[0].version_index} | ${1}
+      ${`No changes between version 3 and version 2`}                         | ${3}                              | ${2}
+      ${`No changes between version 3 and ${TEST_TARGET_BRANCH}`}             | ${3}                              | ${-1}
+    `(
+      'renders text "$expectedText" (sourceIndex=$sourceIndex and targetIndex=$targetIndex)',
+      ({ expectedText, targetIndex, sourceIndex }) => {
+        if (targetIndex !== null) {
+          store.state.diffs.startVersion = { version_index: targetIndex };
+        }
+        if (sourceIndex !== null) {
+          store.state.diffs.mergeRequestDiff.version_index = sourceIndex;
+        }
+
+        createComponent(mount);
+
+        expect(findMessage().text()).toBe(expectedText);
+      },
+    );
   });
 });

spec/frontend/diffs/store/getters_versions_dropdowns_spec.js

@@ -136,6 +136,7 @@ describe('Compare diff version dropdowns', () => {
       ...firstDiff,
       href: firstDiff.version_path,
       commitsText: `${firstDiff.commits_count} commits,`,
+      isLatestVersion: true,
       versionName: 'latest version',
       selected: true,
     };
@@ -144,6 +145,9 @@ describe('Compare diff version dropdowns', () => {
       selectedSourceIndex: expectedShape.version_index,
     });
     expect(sourceVersions[0]).toEqual(expectedShape);
-    expect(sourceVersions[1].selected).toBe(false);
+    expect(sourceVersions[1]).toMatchObject({
+      selected: false,
+      isLatestVersion: false,
+    });
   });
 });