Merge branch '49329-mr-show-commit-details' into 'master'

Resolve "Commit details are not displayed when reviewing an MR commit by commit"

See merge request gitlab-org/gitlab-ce!21784

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

@@ -9,6 +9,7 @@ import ChangedFiles from './changed_files.vue';
 import DiffFile from './diff_file.vue';
 import NoChanges from './no_changes.vue';
 import HiddenFilesWarning from './hidden_files_warning.vue';
+import CommitWidget from './commit_widget.vue';
 
 export default {
   name: 'DiffsApp',
@@ -19,6 +20,7 @@ export default {
     DiffFile,
     NoChanges,
     HiddenFilesWarning,
+    CommitWidget,
   },
   props: {
     endpoint: {
@@ -208,6 +210,11 @@ export default {
         </div>
       </div>
 
+      <commit-widget
+        v-if="commit"
+        :commit="commit"
+      />
+
       <changed-files
         :diff-files="diffFiles"
       />

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

+<script>
+import tooltip from '~/vue_shared/directives/tooltip';
+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 CIIcon from '~/vue_shared/components/ci_icon.vue';
+import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
+
+/**
+ * CommitItem
+ *
+ * -----------------------------------------------------------------
+ * WARNING: Please keep changes up-to-date with the following files:
+ * - `views/projects/commits/_commit.html.haml`
+ * -----------------------------------------------------------------
+ *
+ * This Component was cloned from a HAML view. For the time being they
+ * coexist, but there is an issue to remove the duplication.
+ * https://gitlab.com/gitlab-org/gitlab-ce/issues/51613
+ *
+ */
+export default {
+  directives: {
+    tooltip,
+  },
+  components: {
+    UserAvatarLink,
+    Icon,
+    ClipboardButton,
+    CIIcon,
+    TimeAgoTooltip,
+  },
+  props: {
+    commit: {
+      type: Object,
+      required: true,
+    },
+  },
+  computed: {
+    authorName() {
+      return (this.commit.author && this.commit.author.name) || this.commit.authorName;
+    },
+    authorUrl() {
+      return (this.commit.author && this.commit.author.webUrl) || `mailto:${this.commit.authorEmail}`;
+    },
+    authorAvatar() {
+      return (this.commit.author && this.commit.author.avatarUrl) || this.commit.authorGravatarUrl;
+    },
+  },
+};
+</script>
+
+<template>
+  <li class="commit flex-row js-toggle-container">
+    <user-avatar-link
+      :link-href="authorUrl"
+      :img-src="authorAvatar"
+      :img-alt="authorName"
+      :img-size="36"
+      class="avatar-cell d-none d-sm-block"
+    />
+    <div class="commit-detail flex-list">
+      <div class="commit-content qa-commit-content">
+        <a
+          :href="commit.commitUrl"
+          class="commit-row-message item-title"
+          v-html="commit.titleHtml"
+        ></a>
+
+        <span class="commit-row-message d-block d-sm-none">
+          &middot;
+          {{ commit.shortId }}
+        </span>
+
+        <button
+          v-if="commit.descriptionHtml"
+          class="text-expander js-toggle-button"
+          type="button"
+          :aria-label="__('Toggle commit description')"
+        >
+          <icon
+            :size="12"
+            name="ellipsis_h"
+          />
+        </button>
+
+        <div class="commiter">
+          <a
+            :href="authorUrl"
+            v-text="authorName"
+          ></a>
+          {{ s__('CommitWidget|authored') }}
+          <time-ago-tooltip
+            :time="commit.authoredDate"
+          />
+        </div>
+
+        <pre
+          v-if="commit.descriptionHtml"
+          class="commit-row-description js-toggle-content append-bottom-8"
+          v-html="commit.descriptionHtml"
+        ></pre>
+      </div>
+      <div class="commit-actions flex-row d-none d-sm-flex">
+        <div class="commit-sha-group">
+          <div
+            class="label label-monospace"
+            v-text="commit.shortId"
+          ></div>
+          <clipboard-button
+            :text="commit.id"
+            :title="__('Copy commit SHA to clipboard')"
+            class="btn btn-default"
+          />
+        </div>
+      </div>
+    </div>
+  </li>
+</template>

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

+<script>
+import CommitItem from './commit_item.vue';
+
+/**
+ * CommitWidget
+ *
+ * -----------------------------------------------------------------
+ * WARNING: Please keep changes up-to-date with the following files:
+ * - `views/projects/merge_requests/diffs/_commit_widget.html.haml`
+ * -----------------------------------------------------------------
+ *
+ * This Component was cloned from a HAML view. For the time being,
+ * they coexist, but there is an issue to remove the duplication.
+ * https://gitlab.com/gitlab-org/gitlab-ce/issues/51613
+ *
+ */
+export default {
+  components: {
+    CommitItem,
+  },
+  props: {
+    commit: {
+      type: Object,
+      required: true,
+    },
+  },
+};
+</script>
+
+<template>
+  <div class="info-well prepend-top-default">
+    <div class="well-segment">
+      <ul class="blob-commit-info">
+        <commit-item
+          :commit="commit"
+        />
+      </ul>
+    </div>
+  </div>
+</template>

app/controllers/projects/merge_requests/diffs_controller.rb

@@ -23,7 +23,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
 
     @diffs.write_cache
 
-    render json: DiffsSerializer.new(current_user: current_user).represent(@diffs, additional_attributes)
+    render json: DiffsSerializer.new(current_user: current_user, project: @merge_request.project).represent(@diffs, additional_attributes)
   end
 
   def define_diff_vars

app/serializers/commit_entity.rb

 # frozen_string_literal: true
 
 class CommitEntity < API::Entities::Commit
+  include MarkupHelper
   include RequestAwareEntity
 
   expose :author, using: UserEntity
@@ -9,11 +10,19 @@ class CommitEntity < API::Entities::Commit
     GravatarService.new.execute(commit.author_email) # rubocop: disable CodeReuse/ServiceClass
   end
 
-  expose :commit_url do |commit|
-    project_commit_url(request.project, commit)
+  expose :commit_url do |commit, options|
+    project_commit_url(request.project, commit, params: options.fetch(:commit_url_params, {}))
   end
 
-  expose :commit_path do |commit|
-    project_commit_path(request.project, commit)
+  expose :commit_path do |commit, options|
+    project_commit_path(request.project, commit, params: options.fetch(:commit_url_params, {}))
+  end
+
+  expose :description_html, if: { type: :full } do |commit|
+    markdown_field(commit, :description)
+  end
+
+  expose :title_html, if: { type: :full } do |commit|
+    markdown_field(commit, :title)
   end
 end

app/serializers/diffs_entity.rb

@@ -15,8 +15,11 @@ class DiffsEntity < Grape::Entity
     merge_request&.target_branch
   end
 
-  expose :commit do |diffs|
-    options[:commit]
+  expose :commit do |diffs, options|
+    CommitEntity.represent options[:commit], options.merge(
+      type: :full,
+      commit_url_params: { merge_request_iid: merge_request&.iid }
+    )
   end
 
   expose :merge_request_diff, using: MergeRequestDiffEntity do |diffs|

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`
+-#-----------------------------------------------------------------
 - view_details  = local_assigns.fetch(:view_details, false)
 - merge_request = local_assigns.fetch(:merge_request, nil)
 - project       = local_assigns.fetch(:project) { merge_request&.project }

app/views/projects/merge_requests/diffs/_commit_widget.html.haml

+-#-----------------------------------------------------------------
+  WARNING: Please keep changes up-to-date with the following files:
+  - `assets/javascripts/diffs/components/commit_widget.vue`
+-#-----------------------------------------------------------------
 - if @commit
   .info-well.d-none.d-sm-block.prepend-top-default
     .well-segment

changelogs/unreleased/49329-mr-show-commit-details.yml

+---
+title: Show commit details for selected commit in MR diffs
+merge_request: 21784
+author:
+type: fixed

locale/gitlab.pot

@@ -1738,6 +1738,9 @@ msgstr ""
 msgid "CommitMessage|Add %{file_name}"
 msgstr ""
 
+msgid "CommitWidget|authored"
+msgstr ""
+
 msgid "Commits"
 msgstr ""
 
@@ -6355,6 +6358,9 @@ msgstr ""
 msgid "Toggle Sidebar"
 msgstr ""
 
+msgid "Toggle commit description"
+msgstr ""
+
 msgid "Toggle discussion"
 msgstr ""
 

spec/javascripts/diffs/components/app_spec.js

@@ -3,6 +3,7 @@ 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;
@@ -36,12 +37,17 @@ describe('diffs/components/app', () => {
     vm.$destroy();
   });
 
+  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 = {};
+    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);

spec/javascripts/diffs/components/commit_item_spec.js

+import Vue from 'vue';
+import { TEST_HOST } from 'spec/test_constants';
+import mountComponent from 'spec/helpers/vue_mount_component_helper';
+import { trimText } from 'spec/helpers/vue_component_helper';
+import { getTimeago } from '~/lib/utils/datetime_utility';
+import CommitItem from '~/diffs/components/commit_item.vue';
+import getDiffWithCommit from '../mock_data/diff_with_commit';
+
+const TEST_AUTHOR_NAME = 'test';
+const TEST_AUTHOR_EMAIL = 'test+test@gitlab.com';
+const TEST_AUTHOR_GRAVATAR = `${TEST_HOST}/avatar/test?s=36`;
+
+const getTitleElement = vm => vm.$el.querySelector('.commit-row-message.item-title');
+const getDescElement = vm => vm.$el.querySelector('pre.commit-row-description');
+const getDescExpandElement = vm => vm.$el.querySelector('.commit-content .text-expander.js-toggle-button');
+const getShaElement = vm => vm.$el.querySelector('.commit-sha-group');
+const getAvatarElement = vm => vm.$el.querySelector('.user-avatar-link');
+const getCommitterElement = vm => vm.$el.querySelector('.commiter');
+
+describe('diffs/components/commit_widget', () => {
+  const Component = Vue.extend(CommitItem);
+  const timeago = getTimeago();
+  const { commit } = getDiffWithCommit();
+
+  let vm;
+
+  beforeEach(() => {
+    vm = mountComponent(Component, {
+      commit: getDiffWithCommit().commit,
+    });
+  });
+
+  it('renders commit title', () => {
+    const titleElement = getTitleElement(vm);
+
+    expect(titleElement).toHaveAttr('href', commit.commitUrl);
+    expect(titleElement).toHaveText(commit.titleHtml);
+  });
+
+  it('renders commit description', () => {
+    const descElement = getDescElement(vm);
+    const descExpandElement = getDescExpandElement(vm);
+
+    const expected = commit.descriptionHtml.replace(/
/g, '');
+
+    expect(trimText(descElement.innerHTML)).toEqual(trimText(expected));
+    expect(descExpandElement).not.toBeNull();
+  });
+
+  it('renders commit sha', () => {
+    const shaElement = getShaElement(vm);
+    const labelElement = shaElement.querySelector('.label');
+    const buttonElement = shaElement.querySelector('button');
+
+    expect(labelElement.textContent).toEqual(commit.shortId);
+    expect(buttonElement).toHaveData('clipboard-text', commit.id);
+  });
+
+  it('renders author avatar', () => {
+    const avatarElement = getAvatarElement(vm);
+    const imgElement = avatarElement.querySelector('img');
+
+    expect(avatarElement).toHaveAttr('href', commit.author.webUrl);
+    expect(imgElement).toHaveClass('s36');
+    expect(imgElement).toHaveAttr('alt', commit.author.name);
+    expect(imgElement).toHaveAttr('src', commit.author.avatarUrl);
+  });
+
+  it('renders committer text', () => {
+    const committerElement = getCommitterElement(vm);
+    const nameElement = committerElement.querySelector('a');
+
+    const expectTimeText = timeago.format(commit.authoredDate);
+    const expectedText = `${commit.author.name} authored ${expectTimeText}`;
+
+    expect(trimText(committerElement.textContent)).toEqual(expectedText);
+    expect(nameElement).toHaveAttr('href', commit.author.webUrl);
+    expect(nameElement).toHaveText(commit.author.name);
+  });
+
+  describe('without commit description', () => {
+    beforeEach(done => {
+      vm.commit.descriptionHtml = '';
+
+      vm.$nextTick()
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('hides description', () => {
+      const descElement = getDescElement(vm);
+      const descExpandElement = getDescExpandElement(vm);
+
+      expect(descElement).toBeNull();
+      expect(descExpandElement).toBeNull();
+    });
+  });
+
+  describe('with no matching user', () => {
+    beforeEach(done => {
+      vm.commit.author = null;
+      vm.commit.authorEmail = TEST_AUTHOR_EMAIL;
+      vm.commit.authorName = TEST_AUTHOR_NAME;
+      vm.commit.authorGravatarUrl = TEST_AUTHOR_GRAVATAR;
+
+      vm.$nextTick()
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('renders author avatar', () => {
+      const avatarElement = getAvatarElement(vm);
+      const imgElement = avatarElement.querySelector('img');
+
+      expect(avatarElement).toHaveAttr('href', `mailto:${TEST_AUTHOR_EMAIL}`);
+      expect(imgElement).toHaveAttr('alt', TEST_AUTHOR_NAME);
+      expect(imgElement).toHaveAttr('src', TEST_AUTHOR_GRAVATAR);
+    });
+
+    it('renders committer text', () => {
+      const committerElement = getCommitterElement(vm);
+      const nameElement = committerElement.querySelector('a');
+
+      expect(nameElement).toHaveAttr('href', `mailto:${TEST_AUTHOR_EMAIL}`);
+      expect(nameElement).toHaveText(TEST_AUTHOR_NAME);
+    });
+  });
+});

spec/javascripts/diffs/components/commit_widget_spec.js

+import Vue from 'vue';
+import mountComponent from 'spec/helpers/vue_mount_component_helper';
+import CommitWidget from '~/diffs/components/commit_widget.vue';
+import getDiffWithCommit from '../mock_data/diff_with_commit';
+
+describe('diffs/components/commit_widget', () => {
+  const Component = Vue.extend(CommitWidget);
+  const { commit } = getDiffWithCommit();
+
+  let vm;
+
+  beforeEach(() => {
+    vm = mountComponent(Component, {
+      commit: getDiffWithCommit().commit,
+    });
+  });
+
+  it('renders commit item', () => {
+    const commitElement = vm.$el.querySelector('li.commit');
+
+    expect(commitElement).not.toBeNull();
+    expect(commitElement).toContainText(commit.shortId);
+  });
+});

spec/javascripts/diffs/mock_data/diff_with_commit.js

+import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
+
+const FIXTURE = 'merge_request_diffs/with_commit.json';
+
+preloadFixtures(FIXTURE);
+
+export default function getDiffWithCommit() {
+  return convertObjectPropsToCamelCase(
+    getJSONFixture(FIXTURE),
+    { deep: true },
+  );
+}

spec/javascripts/fixtures/merge_requests_diffs.rb

@@ -9,6 +9,7 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
   let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') }
   let(:merge_request) { create(:merge_request, :with_diffs, source_project: project, target_project: project, description: '- [ ] Task List Item') }
   let(:path) { "files/ruby/popen.rb" }
+  let(:selected_commit) { merge_request.all_commits[0] }
   let(:position) do
     Gitlab::Diff::Position.new(
       old_path: path,
@@ -33,6 +34,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
     remove_repository(project)
   end
 
+  it 'merge_request_diffs/with_commit.json' do |example|
+    # Create a user that matches the selected commit author
+    # This is so that the "author" information will be populated
+    create(:user, email: selected_commit.author_email, name: selected_commit.author_name)
+
+    render_merge_request(example.description, merge_request, commit_id: selected_commit.sha)
+  end
+
   it 'merge_request_diffs/inline_changes_tab_with_comments.json' do |example|
     create(:diff_note_on_merge_request, project: project, author: admin, position: position, noteable: merge_request)
     create(:note_on_merge_request, author: admin, project: project, noteable: merge_request)
@@ -47,13 +56,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
 
   private
 
-  def render_merge_request(fixture_file_name, merge_request, view: 'inline')
+  def render_merge_request(fixture_file_name, merge_request, view: 'inline', **extra_params)
     get :show,
       namespace_id: project.namespace.to_param,
       project_id: project,
       id: merge_request.to_param,
       format: :json,
-      view: view
+      view: view,
+      **extra_params
 
     expect(response).to be_success
     store_frontend_fixture(response, fixture_file_name)

spec/serializers/commit_entity_spec.rb

@@ -51,4 +51,37 @@ describe CommitEntity do
   it 'exposes gravatar url that belongs to author' do
     expect(subject.fetch(:author_gravatar_url)).to match /gravatar/
   end
+
+  context 'when type is not set' do
+    it 'does not expose extra properties' do
+      expect(subject).not_to include(:description_html)
+      expect(subject).not_to include(:title_html)
+    end
+  end
+
+  context 'when type is "full"' do
+    let(:entity) do
+      described_class.new(commit, request: request, type: :full)
+    end
+
+    it 'exposes extra properties' do
+      expect(subject).to include(:description_html)
+      expect(subject).to include(:title_html)
+      expect(subject.fetch(:description_html)).not_to be_nil
+      expect(subject.fetch(:title_html)).not_to be_nil
+    end
+  end
+
+  context 'when commit_url_params is set' do
+    let(:entity) do
+      params = { merge_request_iid: 3 }
+
+      described_class.new(commit, request: request, commit_url_params: params)
+    end
+
+    it 'adds commit_url_params to url and path' do
+      expect(subject[:commit_path]).to include "?merge_request_iid=3"
+      expect(subject[:commit_url]).to include "?merge_request_iid=3"
+    end
+  end
 end