Clearly display diff statistics for MRs

Put the statistics in the compare-versions header for the entire MR
Put them in the file header for each individual file

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

@@ -6,6 +6,7 @@ import { polyfillSticky } from '~/lib/utils/sticky';
 import Icon from '~/vue_shared/components/icon.vue';
 import CompareVersionsDropdown from './compare_versions_dropdown.vue';
 import SettingsDropdown from './settings_dropdown.vue';
+import DiffStats from './diff_stats.vue';
 
 export default {
   components: {
@@ -14,6 +15,7 @@ export default {
     GlLink,
     GlButton,
     SettingsDropdown,
+    DiffStats,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -35,8 +37,15 @@ export default {
     },
   },
   computed: {
-    ...mapState('diffs', ['commit', 'showTreeList', 'startVersion', 'latestVersionPath']),
-    ...mapGetters('diffs', ['hasCollapsedFile']),
+    ...mapGetters('diffs', ['hasCollapsedFile', 'diffFilesLength']),
+    ...mapState('diffs', [
+      'commit',
+      'showTreeList',
+      'startVersion',
+      'latestVersionPath',
+      'addedLines',
+      'removedLines',
+    ]),
     comparableDiffs() {
       return this.mergeRequestDiffs.slice(1);
     },
@@ -104,6 +113,11 @@ export default {
         <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">
+        <diff-stats
+          :diff-files-length="diffFilesLength"
+          :added-lines="addedLines"
+          :removed-lines="removedLines"
+        />
         <gl-button
           v-if="commit || startVersion"
           :href="latestVersionPath"

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

@@ -9,6 +9,7 @@ import { GlTooltipDirective } from '@gitlab/ui';
 import { truncateSha } from '~/lib/utils/text_utility';
 import { __, s__, sprintf } from '~/locale';
 import EditButton from './edit_button.vue';
+import DiffStats from './diff_stats.vue';
 
 export default {
   components: {
@@ -16,6 +17,7 @@ export default {
     EditButton,
     Icon,
     FileIcon,
+    DiffStats,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -202,6 +204,7 @@ export default {
       v-if="!diffFile.submodule && addMergeRequestButtons"
       class="file-actions d-none d-sm-block"
     >
+      <diff-stats :added-lines="diffFile.added_lines" :removed-lines="diffFile.removed_lines" />
       <template v-if="diffFile.blob && diffFile.blob.readable_text">
         <button
           :disabled="!diffHasDiscussions(diffFile)"

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

+<script>
+import Icon from '~/vue_shared/components/icon.vue';
+import { n__ } from '~/locale';
+
+export default {
+  components: { Icon },
+  props: {
+    addedLines: {
+      type: Number,
+      required: true,
+    },
+    removedLines: {
+      type: Number,
+      required: true,
+    },
+    diffFilesLength: {
+      type: Number,
+      required: false,
+      default: null,
+    },
+  },
+  computed: {
+    filesText() {
+      return n__('File', 'Files', this.diffFilesLength);
+    },
+    isCompareVersionsHeader() {
+      return Boolean(this.diffFilesLength);
+    },
+  },
+};
+</script>
+
+<template>
+  <div
+    class="diff-stats"
+    :class="{
+      'is-compare-versions-header d-none d-lg-inline-flex': isCompareVersionsHeader,
+      'd-inline-flex': !isCompareVersionsHeader,
+    }"
+  >
+    <div v-if="diffFilesLength !== null" class="diff-stats-group">
+      <icon name="doc-code" class="diff-stats-icon text-secondary" />
+      <strong>{{ diffFilesLength }} {{ filesText }}</strong>
+    </div>
+    <div class="diff-stats-group cgreen">
+      <icon name="file-addition" class="diff-stats-icon" /> <strong>{{ addedLines }}</strong>
+    </div>
+    <div class="diff-stats-group cred">
+      <icon name="file-deletion" class="diff-stats-icon" /> <strong>{{ removedLines }}</strong>
+    </div>
+  </div>
+</template>

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

@@ -19,8 +19,8 @@ export default {
     };
   },
   computed: {
-    ...mapState('diffs', ['tree', 'addedLines', 'removedLines', 'renderTreeList']),
-    ...mapGetters('diffs', ['allBlobs', 'diffFilesLength']),
+    ...mapState('diffs', ['tree', 'renderTreeList']),
+    ...mapGetters('diffs', ['allBlobs']),
     filteredTreeList() {
       const search = this.search.toLowerCase().trim();
 
@@ -90,13 +90,6 @@ export default {
         {{ s__('MergeRequest|No files found') }}
       </p>
     </div>
-    <div v-once class="pt-3 pb-3 text-center">
-      {{ n__('%d changed file', '%d changed files', diffFilesLength) }}
-      <div>
-        <span class="cgreen"> {{ n__('%d addition', '%d additions', addedLines) }} </span>
-        <span class="cred"> {{ n__('%d deleted', '%d deletions', removedLines) }} </span>
-      </div>
-    </div>
   </div>
 </template>
 

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -11,6 +11,8 @@ const storedTreeShow = localStorage.getItem(MR_TREE_SHOW_KEY);
 
 export default () => ({
   isLoading: true,
+  addedLines: null,
+  removedLines: null,
   endpoint: '',
   basePath: '',
   commit: null,

app/assets/stylesheets/framework/files.scss

@@ -51,6 +51,7 @@
       position: absolute;
       top: 5px;
       right: 15px;
+      margin-left: auto;
 
       .btn {
         padding: 0 10px;
@@ -324,6 +325,7 @@ span.idiff {
   &,
   .file-holder & {
     display: flex;
+    flex-wrap: wrap;
     align-items: center;
     justify-content: space-between;
     background-color: $gray-light;
@@ -365,16 +367,12 @@ span.idiff {
     margin: 0 10px 0 0;
   }
 
-  .file-actions {
-    white-space: nowrap;
-
-    .btn {
-      padding: 0 10px;
-      font-size: 13px;
-      line-height: 28px;
-      display: inline-block;
-      float: none;
-    }
+  .file-actions .btn {
+    padding: 0 10px;
+    font-size: 13px;
+    line-height: 28px;
+    display: inline-block;
+    float: none;
   }
 
   @include media-breakpoint-down(xs) {

app/assets/stylesheets/pages/diff.scss

@@ -501,6 +501,25 @@
   }
 }
 
+.diff-stats {
+  align-items: center;
+  padding: 0 .25rem;
+
+  .diff-stats-group {
+    padding: 0 .25rem;
+  }
+
+  svg.diff-stats-icon {
+    vertical-align: text-bottom;
+  }
+
+  &.is-compare-versions-header {
+    .diff-stats-group {
+      padding: 0 .5rem;
+    }
+  }
+}
+
 .file-content .diff-file {
   margin: 0;
   border: 0;

changelogs/unreleased/52347-lines-changed-statistics-is-not-easily-visible-in-mr-changes-view.yml

+---
+title: Show MR statistics in diff comparisons
+merge_request: !24569
+author:
+type: changed

locale/gitlab.pot

@@ -22,16 +22,6 @@ msgstr ""
 msgid " or "
 msgstr ""
 
-msgid "%d addition"
-msgid_plural "%d additions"
-msgstr[0] ""
-msgstr[1] ""
-
-msgid "%d changed file"
-msgid_plural "%d changed files"
-msgstr[0] ""
-msgstr[1] ""
-
 msgid "%d commit"
 msgid_plural "%d commits"
 msgstr[0] ""
@@ -42,11 +32,6 @@ msgid_plural "%d commits behind"
 msgstr[0] ""
 msgstr[1] ""
 
-msgid "%d deleted"
-msgid_plural "%d deletions"
-msgstr[0] ""
-msgstr[1] ""
-
 msgid "%d exporter"
 msgid_plural "%d exporters"
 msgstr[0] ""
@@ -3214,6 +3199,11 @@ msgstr ""
 msgid "Fields on this page are now uneditable, you can configure"
 msgstr ""
 
+msgid "File"
+msgid_plural "Files"
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "File added"
 msgstr ""
 

spec/features/merge_request/user_sees_versions_spec.rb

@@ -42,7 +42,7 @@ describe 'Merge request > User sees versions', :js do
         expect(page).to have_content 'latest version'
       end
 
-      expect(page).to have_content '8 changed files'
+      expect(page).to have_content '8 Files'
     end
 
     it_behaves_like 'allows commenting',
@@ -76,7 +76,7 @@ describe 'Merge request > User sees versions', :js do
     end
 
     it 'shows comments that were last relevant at that version' do
-      expect(page).to have_content '5 changed files'
+      expect(page).to have_content '5 Files'
 
       position = Gitlab::Diff::Position.new(
         old_path: ".gitmodules",
@@ -120,8 +120,15 @@ describe 'Merge request > User sees versions', :js do
         diff_id: merge_request_diff3.id,
         start_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9'
       )
-      expect(page).to have_content '4 changed files'
-      expect(page).to have_content '15 additions 6 deletions'
+      expect(page).to have_content '4 Files'
+
+      additions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-addition')
+                              .ancestor('.diff-stats-group').text
+      deletions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-deletion')
+                              .ancestor('.diff-stats-group').text
+
+      expect(additions_content).to eq '15'
+      expect(deletions_content).to eq '6'
 
       position = Gitlab::Diff::Position.new(
         old_path: ".gitmodules",
@@ -141,8 +148,14 @@ describe 'Merge request > User sees versions', :js do
     end
 
     it 'show diff between new and old version' do
-      expect(page).to have_content '4 changed files'
-      expect(page).to have_content '15 additions 6 deletions'
+      additions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-addition')
+                              .ancestor('.diff-stats-group').text
+      deletions_content = page.find('.diff-stats.is-compare-versions-header .diff-stats-group svg.ic-file-deletion')
+                              .ancestor('.diff-stats-group').text
+
+      expect(page).to have_content '4 Files'
+      expect(additions_content).to eq '15'
+      expect(deletions_content).to eq '6'
     end
 
     it 'returns to latest version when "Show latest version" button is clicked' do
@@ -150,7 +163,7 @@ describe 'Merge request > User sees versions', :js do
       page.within '.mr-version-dropdown' do
         expect(page).to have_content 'latest version'
       end
-      expect(page).to have_content '8 changed files'
+      expect(page).to have_content '8 Files'
     end
 
     it_behaves_like 'allows commenting',
@@ -176,7 +189,7 @@ describe 'Merge request > User sees versions', :js do
         find('.btn-default').click
         click_link 'version 1'
       end
-      expect(page).to have_content '0 changed files'
+      expect(page).to have_content '0 Files'
     end
   end
 
@@ -202,7 +215,7 @@ describe 'Merge request > User sees versions', :js do
         expect(page).to have_content 'version 1'
       end
 
-      expect(page).to have_content '0 changed files'
+      expect(page).to have_content '0 Files'
     end
   end
 

spec/javascripts/diffs/components/compare_versions_spec.js

@@ -10,6 +10,10 @@ describe('CompareVersions', () => {
   const targetBranch = { branchName: 'tmp-wine-dev', versionIndex: -1 };
 
   beforeEach(() => {
+    store.state.diffs.addedLines = 10;
+    store.state.diffs.removedLines = 20;
+    store.state.diffs.diffFiles.push('test');
+
     vm = createComponentWithStore(Vue.extend(CompareVersionsComponent), store, {
       mergeRequestDiffs: diffsMockData,
       mergeRequestDiff: diffsMockData[0],

spec/javascripts/diffs/components/diff_file_header_spec.js

@@ -24,6 +24,10 @@ describe('diff_file_header', () => {
 
   beforeEach(() => {
     const diffFile = diffDiscussionMock.diff_file;
+
+    diffFile.added_lines = 2;
+    diffFile.removed_lines = 1;
+
     props = {
       diffFile: { ...diffFile },
       canCurrentUserFork: false,

spec/javascripts/diffs/components/diff_stats_spec.js

+import { shallowMount } from '@vue/test-utils';
+import DiffStats from '~/diffs/components/diff_stats.vue';
+
+describe('diff_stats', () => {
+  it('does not render a group if diffFileLengths is not passed in', () => {
+    const wrapper = shallowMount(DiffStats, {
+      propsData: {
+        addedLines: 1,
+        removedLines: 2,
+      },
+    });
+    const groups = wrapper.findAll('.diff-stats-group');
+
+    expect(groups.length).toBe(2);
+  });
+
+  it('shows amount of files changed, lines added and lines removed when passed all props', () => {
+    const wrapper = shallowMount(DiffStats, {
+      propsData: {
+        addedLines: 100,
+        removedLines: 200,
+        diffFilesLength: 300,
+      },
+    });
+    const additions = wrapper.find('icon-stub[name="file-addition"]').element.parentNode;
+    const deletions = wrapper.find('icon-stub[name="file-deletion"]').element.parentNode;
+    const filesChanged = wrapper.find('icon-stub[name="doc-code"]').element.parentNode;
+
+    expect(additions.textContent).toContain('100');
+    expect(deletions.textContent).toContain('200');
+    expect(filesChanged.textContent).toContain('300');
+  });
+});

spec/javascripts/diffs/components/tree_list_spec.js

@@ -35,12 +35,6 @@ describe('Diffs tree list component', () => {
     vm.$destroy();
   });
 
-  it('renders diff stats', () => {
-    expect(vm.$el.textContent).toContain('1 changed file');
-    expect(vm.$el.textContent).toContain('10 additions');
-    expect(vm.$el.textContent).toContain('20 deletions');
-  });
-
   it('renders empty text', () => {
     expect(vm.$el.textContent).toContain('No files found');
   });