From 93c614ce9243c0743176a6e406a55f9dcb9bedf3 Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Tue, 20 Nov 2018 12:37:57 +0000
Subject: [PATCH] Show toggle tree button on merge request commits

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/53019
---
 .../javascripts/diffs/components/app.vue      | 55 ++++---------------
 .../diffs/components/commit_widget.vue        |  2 +-
 .../diffs/components/compare_versions.vue     | 23 +++++---
 app/assets/stylesheets/pages/issuable.scss    |  1 -
 .../stylesheets/pages/merge_requests.scss     |  1 +
 changelogs/unreleased/mr-file-tree-commit.yml |  5 ++
 locale/gitlab.pot                             | 15 +----
 .../merge_request/user_sees_versions_spec.rb  |  2 -
 spec/javascripts/diffs/components/app_spec.js | 40 --------------
 .../diffs/components/compare_versions_spec.js | 21 +++++++
 10 files changed, 58 insertions(+), 107 deletions(-)
 create mode 100644 changelogs/unreleased/mr-file-tree-commit.yml

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 0b7838292b2..22da38ce7a5 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -55,8 +55,6 @@ export default {
       diffViewType: state => state.diffs.diffViewType,
       mergeRequestDiffs: state => state.diffs.mergeRequestDiffs,
       mergeRequestDiff: state => state.diffs.mergeRequestDiff,
-      latestVersionPath: state => state.diffs.latestVersionPath,
-      startVersion: state => state.diffs.startVersion,
       commit: state => state.diffs.commit,
       targetBranchName: state => state.diffs.targetBranchName,
       renderOverflowWarning: state => state.diffs.renderOverflowWarning,
@@ -75,24 +73,6 @@ export default {
         path: '',
       };
     },
-    notAllCommentsDisplayed() {
-      if (this.commit) {
-        return __('Only comments from the following commit are shown below');
-      } else if (this.startVersion) {
-        return __(
-          "Not all comments are displayed because you're comparing two versions of the diff.",
-        );
-      }
-      return __(
-        "Not all comments are displayed because you're viewing an old version of the diff.",
-      );
-    },
-    showLatestVersion() {
-      if (this.commit) {
-        return __('Show latest version of the diff');
-      }
-      return __('Show latest version');
-    },
     canCurrentUserFork() {
       return this.currentUser.can_fork === true && this.currentUser.can_create_merge_request;
     },
@@ -184,10 +164,8 @@ export default {
     <div v-if="isLoading" class="loading"><gl-loading-icon /></div>
     <div v-else id="diffs" :class="{ active: shouldShow }" class="diffs tab-pane">
       <compare-versions
-        v-if="showCompareVersions"
         :merge-request-diffs="mergeRequestDiffs"
         :merge-request-diff="mergeRequestDiff"
-        :start-version="startVersion"
         :target-branch="targetBranch"
       />
 
@@ -199,34 +177,23 @@ export default {
         :email-patch-path="emailPatchPath"
       />
 
-      <div
-        v-if="commit || startVersion || (mergeRequestDiff && !mergeRequestDiff.latest)"
-        class="mr-version-controls"
-      >
-        <div class="content-block comments-disabled-notif clearfix">
-          <i class="fa fa-info-circle"></i> {{ notAllCommentsDisplayed }}
-          <div class="pull-right">
-            <a :href="latestVersionPath" class="btn btn-sm"> {{ showLatestVersion }} </a>
-          </div>
-        </div>
-      </div>
-
-      <commit-widget v-if="commit" :commit="commit" />
-
       <div
         :data-can-create-note="getNoteableData.current_user.can_create_note"
         class="files d-flex prepend-top-default"
       >
         <div v-show="showTreeList" class="diff-tree-list"><tree-list /></div>
-        <div v-if="diffFiles.length > 0" class="diff-files-holder">
-          <diff-file
-            v-for="file in diffFiles"
-            :key="file.newPath"
-            :file="file"
-            :can-current-user-fork="canCurrentUserFork"
-          />
+        <div class="diff-files-holder">
+          <commit-widget v-if="commit" :commit="commit" />
+          <template v-if="diffFiles.length > 0">
+            <diff-file
+              v-for="file in diffFiles"
+              :key="file.newPath"
+              :file="file"
+              :can-current-user-fork="canCurrentUserFork"
+            />
+          </template>
+          <no-changes v-else />
         </div>
-        <no-changes v-else />
       </div>
     </div>
   </div>
diff --git a/app/assets/javascripts/diffs/components/commit_widget.vue b/app/assets/javascripts/diffs/components/commit_widget.vue
index 3fb11813d68..d45f91c7023 100644
--- a/app/assets/javascripts/diffs/components/commit_widget.vue
+++ b/app/assets/javascripts/diffs/components/commit_widget.vue
@@ -28,7 +28,7 @@ export default {
 </script>
 
 <template>
-  <div class="info-well prepend-top-default">
+  <div class="info-well w-100">
     <div class="well-segment">
       <ul class="blob-commit-info">
         <commit-item :commit="commit" />
diff --git a/app/assets/javascripts/diffs/components/compare_versions.vue b/app/assets/javascripts/diffs/components/compare_versions.vue
index 3ccd111f1cb..f75345d31f8 100644
--- a/app/assets/javascripts/diffs/components/compare_versions.vue
+++ b/app/assets/javascripts/diffs/components/compare_versions.vue
@@ -1,6 +1,6 @@
 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
-import { GlTooltipDirective } from '@gitlab/ui';
+import { GlTooltipDirective, GlLink, GlButton } from '@gitlab/ui';
 import { __ } from '~/locale';
 import { getParameterValues, mergeUrlParams } from '~/lib/utils/url_utility';
 import Icon from '~/vue_shared/components/icon.vue';
@@ -10,6 +10,8 @@ export default {
   components: {
     CompareVersionsDropdown,
     Icon,
+    GlLink,
+    GlButton,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -20,13 +22,9 @@ export default {
       required: true,
     },
     mergeRequestDiff: {
-      type: Object,
-      required: true,
-    },
-    startVersion: {
       type: Object,
       required: false,
-      default: null,
+      default: () => ({}),
     },
     targetBranch: {
       type: Object,
@@ -35,7 +33,7 @@ export default {
     },
   },
   computed: {
-    ...mapState('diffs', ['commit', 'showTreeList']),
+    ...mapState('diffs', ['commit', 'showTreeList', 'startVersion', 'latestVersionPath']),
     ...mapGetters('diffs', ['isInlineView', 'isParallelView', 'hasCollapsedFile']),
     comparableDiffs() {
       return this.mergeRequestDiffs.slice(1);
@@ -102,7 +100,18 @@ export default {
           class="mr-version-compare-dropdown"
         />
       </div>
+      <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">
+        <gl-button
+          v-if="commit || startVersion"
+          :href="latestVersionPath"
+          class="append-right-8 js-latest-version"
+        >
+          {{ __('Show latest version') }}
+        </gl-button>
         <a v-show="hasCollapsedFile" class="btn btn-default append-right-8" @click="expandAllFiles">
           {{ __('Expand all') }}
         </a>
diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss
index 415583cc1c7..78c5ae9ae63 100644
--- a/app/assets/stylesheets/pages/issuable.scss
+++ b/app/assets/stylesheets/pages/issuable.scss
@@ -22,7 +22,6 @@
   .detail-page-header,
   .page-content-header,
   .commit-box,
-  .info-well,
   .commit-ci-menu,
   .files-changed-inner,
   .limited-header-width,
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index fa6afbf81de..b075009b57c 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -717,6 +717,7 @@
   .mr-version-menus-container {
     display: flex;
     align-items: center;
+    flex-wrap: wrap;
     padding: 16px;
     z-index: 199;
     white-space: nowrap;
diff --git a/changelogs/unreleased/mr-file-tree-commit.yml b/changelogs/unreleased/mr-file-tree-commit.yml
new file mode 100644
index 00000000000..e0d47e6e61f
--- /dev/null
+++ b/changelogs/unreleased/mr-file-tree-commit.yml
@@ -0,0 +1,5 @@
+---
+title: Show tree collapse button for merge request commit diffs
+merge_request:
+author:
+type: fixed
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 7af36b049d8..71504692b01 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -4252,12 +4252,6 @@ msgstr ""
 msgid "None"
 msgstr ""
 
-msgid "Not all comments are displayed because you're comparing two versions of the diff."
-msgstr ""
-
-msgid "Not all comments are displayed because you're viewing an old version of the diff."
-msgstr ""
-
 msgid "Not allowed to merge"
 msgstr ""
 
@@ -4395,9 +4389,6 @@ msgstr ""
 msgid "Only admins"
 msgstr ""
 
-msgid "Only comments from the following commit are shown below"
-msgstr ""
-
 msgid "Only mirror protected branches"
 msgstr ""
 
@@ -5764,9 +5755,6 @@ msgstr ""
 msgid "Show latest version"
 msgstr ""
 
-msgid "Show latest version of the diff"
-msgstr ""
-
 msgid "Show parent pages"
 msgstr ""
 
@@ -7053,6 +7041,9 @@ msgstr ""
 msgid "View the documentation"
 msgstr ""
 
+msgid "Viewing commit"
+msgstr ""
+
 msgid "Visibility and access controls"
 msgstr ""
 
diff --git a/spec/features/merge_request/user_sees_versions_spec.rb b/spec/features/merge_request/user_sees_versions_spec.rb
index 92db4f44098..f7512294bef 100644
--- a/spec/features/merge_request/user_sees_versions_spec.rb
+++ b/spec/features/merge_request/user_sees_versions_spec.rb
@@ -66,7 +66,6 @@ describe 'Merge request > User sees versions', :js do
 
     it 'shows comments that were last relevant at that version' do
       expect(page).to have_content '5 changed files'
-      expect(page).to have_content 'Not all comments are displayed'
 
       position = Gitlab::Diff::Position.new(
         old_path: ".gitmodules",
@@ -112,7 +111,6 @@ describe 'Merge request > User sees versions', :js do
       )
       expect(page).to have_content '4 changed files'
       expect(page).to have_content '15 additions 6 deletions'
-      expect(page).to have_content 'Not all comments are displayed'
 
       position = Gitlab::Diff::Position.new(
         old_path: ".gitmodules",
diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js
index 3c9b5ee0176..1a0b7612ee9 100644
--- a/spec/javascripts/diffs/components/app_spec.js
+++ b/spec/javascripts/diffs/components/app_spec.js
@@ -3,7 +3,6 @@ import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper
 import { TEST_HOST } from 'spec/test_constants';
 import App from '~/diffs/components/app.vue';
 import createDiffsStore from '../create_diffs_store';
-import getDiffWithCommit from '../mock_data/diff_with_commit';
 
 describe('diffs/components/app', () => {
   const oldMrTabs = window.mrTabs;
@@ -40,43 +39,4 @@ describe('diffs/components/app', () => {
   it('does not show commit info', () => {
     expect(vm.$el).not.toContainElement('.blob-commit-info');
   });
-
-  it('shows comments message, with commit', done => {
-    vm.$store.state.diffs.commit = getDiffWithCommit().commit;
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$el).toContainText('Only comments from the following commit are shown below');
-        expect(vm.$el).toContainElement('.blob-commit-info');
-      })
-      .then(done)
-      .catch(done.fail);
-  });
-
-  it('shows comments message, with old mergeRequestDiff', done => {
-    vm.$store.state.diffs.mergeRequestDiff = { latest: false };
-    vm.$store.state.diffs.targetBranch = 'master';
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$el).toContainText(
-          "Not all comments are displayed because you're viewing an old version of the diff.",
-        );
-      })
-      .then(done)
-      .catch(done.fail);
-  });
-
-  it('shows comments message, with startVersion', done => {
-    vm.$store.state.diffs.startVersion = 'test';
-
-    vm.$nextTick()
-      .then(() => {
-        expect(vm.$el).toContainText(
-          "Not all comments are displayed because you're comparing two versions of the diff.",
-        );
-      })
-      .then(done)
-      .catch(done.fail);
-  });
 });
diff --git a/spec/javascripts/diffs/components/compare_versions_spec.js b/spec/javascripts/diffs/components/compare_versions_spec.js
index d9d7f61785f..75c66e9ca82 100644
--- a/spec/javascripts/diffs/components/compare_versions_spec.js
+++ b/spec/javascripts/diffs/components/compare_versions_spec.js
@@ -3,6 +3,7 @@ import CompareVersionsComponent from '~/diffs/components/compare_versions.vue';
 import store from '~/mr_notes/stores';
 import { createComponentWithStore } from 'spec/helpers/vue_mount_component_helper';
 import diffsMockData from '../mock_data/merge_request_diffs';
+import getDiffWithCommit from '../mock_data/diff_with_commit';
 
 describe('CompareVersions', () => {
   let vm;
@@ -122,4 +123,24 @@ describe('CompareVersions', () => {
       expect(vm.isWhitespaceVisible()).toBe(true);
     });
   });
+
+  describe('commit', () => {
+    beforeEach(done => {
+      vm.$store.state.diffs.commit = getDiffWithCommit().commit;
+      vm.mergeRequestDiffs = [];
+
+      vm.$nextTick(done);
+    });
+
+    it('renders latest version button', () => {
+      expect(vm.$el.querySelector('.js-latest-version').textContent.trim()).toBe(
+        'Show latest version',
+      );
+    });
+
+    it('renders short commit ID', () => {
+      expect(vm.$el.textContent).toContain('Viewing commit');
+      expect(vm.$el.textContent).toContain(vm.commit.short_id);
+    });
+  });
 });