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

6 files changed, 216 insertions, 3 deletions

diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js
index 7be44a26ded..cf7d8df5405 100644
--- a/spec/javascripts/diffs/components/app_spec.js
+++ b/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);
diff --git a/spec/javascripts/diffs/components/commit_item_spec.js b/spec/javascripts/diffs/components/commit_item_spec.js
new file mode 100644
index 00000000000..627fb8c490a
--- /dev/null
+++ b/spec/javascripts/diffs/components/commit_item_spec.js
@@ -0,0 +1,128 @@
+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);
+    });
+  });
+});
diff --git a/spec/javascripts/diffs/components/commit_widget_spec.js b/spec/javascripts/diffs/components/commit_widget_spec.js
new file mode 100644
index 00000000000..951eb57255d
--- /dev/null
+++ b/spec/javascripts/diffs/components/commit_widget_spec.js
@@ -0,0 +1,24 @@
+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);
+  });
+});
diff --git a/spec/javascripts/diffs/mock_data/diff_with_commit.js b/spec/javascripts/diffs/mock_data/diff_with_commit.js
new file mode 100644
index 00000000000..98393a20583
--- /dev/null
+++ b/spec/javascripts/diffs/mock_data/diff_with_commit.js
@@ -0,0 +1,12 @@
+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 },
+  );
+}
diff --git a/spec/javascripts/fixtures/merge_requests_diffs.rb b/spec/javascripts/fixtures/merge_requests_diffs.rb
index ddce00bc0fe..afe34b834b0 100644
--- a/spec/javascripts/fixtures/merge_requests_diffs.rb
+++ b/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)
diff --git a/spec/serializers/commit_entity_spec.rb b/spec/serializers/commit_entity_spec.rb
index 04247c78549..35215e06f5f 100644
--- a/spec/serializers/commit_entity_spec.rb
+++ b/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