Commit f3901842 authored by Paul Slaughter's avatar Paul Slaughter Committed by Phil Hughes

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

parent c7fcb01b
...@@ -9,6 +9,7 @@ import ChangedFiles from './changed_files.vue'; ...@@ -9,6 +9,7 @@ import ChangedFiles from './changed_files.vue';
import DiffFile from './diff_file.vue'; import DiffFile from './diff_file.vue';
import NoChanges from './no_changes.vue'; import NoChanges from './no_changes.vue';
import HiddenFilesWarning from './hidden_files_warning.vue'; import HiddenFilesWarning from './hidden_files_warning.vue';
import CommitWidget from './commit_widget.vue';
export default { export default {
name: 'DiffsApp', name: 'DiffsApp',
...@@ -19,6 +20,7 @@ export default { ...@@ -19,6 +20,7 @@ export default {
DiffFile, DiffFile,
NoChanges, NoChanges,
HiddenFilesWarning, HiddenFilesWarning,
CommitWidget,
}, },
props: { props: {
endpoint: { endpoint: {
...@@ -208,6 +210,11 @@ export default { ...@@ -208,6 +210,11 @@ export default {
</div> </div>
</div> </div>
<commit-widget
v-if="commit"
:commit="commit"
/>
<changed-files <changed-files
:diff-files="diffFiles" :diff-files="diffFiles"
/> />
......
<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>
<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>
...@@ -23,7 +23,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic ...@@ -23,7 +23,7 @@ class Projects::MergeRequests::DiffsController < Projects::MergeRequests::Applic
@diffs.write_cache @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 end
def define_diff_vars def define_diff_vars
......
# frozen_string_literal: true # frozen_string_literal: true
class CommitEntity < API::Entities::Commit class CommitEntity < API::Entities::Commit
include MarkupHelper
include RequestAwareEntity include RequestAwareEntity
expose :author, using: UserEntity expose :author, using: UserEntity
...@@ -9,11 +10,19 @@ class CommitEntity < API::Entities::Commit ...@@ -9,11 +10,19 @@ class CommitEntity < API::Entities::Commit
GravatarService.new.execute(commit.author_email) # rubocop: disable CodeReuse/ServiceClass GravatarService.new.execute(commit.author_email) # rubocop: disable CodeReuse/ServiceClass
end end
expose :commit_url do |commit| expose :commit_url do |commit, options|
project_commit_url(request.project, commit) project_commit_url(request.project, commit, params: options.fetch(:commit_url_params, {}))
end end
expose :commit_path do |commit| expose :commit_path do |commit, options|
project_commit_path(request.project, commit) 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
end end
...@@ -15,8 +15,11 @@ class DiffsEntity < Grape::Entity ...@@ -15,8 +15,11 @@ class DiffsEntity < Grape::Entity
merge_request&.target_branch merge_request&.target_branch
end end
expose :commit do |diffs| expose :commit do |diffs, options|
options[:commit] CommitEntity.represent options[:commit], options.merge(
type: :full,
commit_url_params: { merge_request_iid: merge_request&.iid }
)
end end
expose :merge_request_diff, using: MergeRequestDiffEntity do |diffs| expose :merge_request_diff, using: MergeRequestDiffEntity do |diffs|
......
-#-----------------------------------------------------------------
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) - view_details = local_assigns.fetch(:view_details, false)
- merge_request = local_assigns.fetch(:merge_request, nil) - merge_request = local_assigns.fetch(:merge_request, nil)
- project = local_assigns.fetch(:project) { merge_request&.project } - project = local_assigns.fetch(:project) { merge_request&.project }
......
-#-----------------------------------------------------------------
WARNING: Please keep changes up-to-date with the following files:
- `assets/javascripts/diffs/components/commit_widget.vue`
-#-----------------------------------------------------------------
- if @commit - if @commit
.info-well.d-none.d-sm-block.prepend-top-default .info-well.d-none.d-sm-block.prepend-top-default
.well-segment .well-segment
......
---
title: Show commit details for selected commit in MR diffs
merge_request: 21784
author:
type: fixed
...@@ -1738,6 +1738,9 @@ msgstr "" ...@@ -1738,6 +1738,9 @@ msgstr ""
msgid "CommitMessage|Add %{file_name}" msgid "CommitMessage|Add %{file_name}"
msgstr "" msgstr ""
msgid "CommitWidget|authored"
msgstr ""
msgid "Commits" msgid "Commits"
msgstr "" msgstr ""
...@@ -6355,6 +6358,9 @@ msgstr "" ...@@ -6355,6 +6358,9 @@ msgstr ""
msgid "Toggle Sidebar" msgid "Toggle Sidebar"
msgstr "" msgstr ""
msgid "Toggle commit description"
msgstr ""
msgid "Toggle discussion" msgid "Toggle discussion"
msgstr "" msgstr ""
......
...@@ -3,6 +3,7 @@ import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper ...@@ -3,6 +3,7 @@ import { mountComponentWithStore } from 'spec/helpers/vue_mount_component_helper
import { TEST_HOST } from 'spec/test_constants'; import { TEST_HOST } from 'spec/test_constants';
import App from '~/diffs/components/app.vue'; import App from '~/diffs/components/app.vue';
import createDiffsStore from '../create_diffs_store'; import createDiffsStore from '../create_diffs_store';
import getDiffWithCommit from '../mock_data/diff_with_commit';
describe('diffs/components/app', () => { describe('diffs/components/app', () => {
const oldMrTabs = window.mrTabs; const oldMrTabs = window.mrTabs;
...@@ -36,12 +37,17 @@ describe('diffs/components/app', () => { ...@@ -36,12 +37,17 @@ describe('diffs/components/app', () => {
vm.$destroy(); vm.$destroy();
}); });
it('does not show commit info', () => {
expect(vm.$el).not.toContainElement('.blob-commit-info');
});
it('shows comments message, with commit', done => { it('shows comments message, with commit', done => {
vm.$store.state.diffs.commit = {}; vm.$store.state.diffs.commit = getDiffWithCommit().commit;
vm.$nextTick() vm.$nextTick()
.then(() => { .then(() => {
expect(vm.$el).toContainText('Only comments from the following commit are shown below'); expect(vm.$el).toContainText('Only comments from the following commit are shown below');
expect(vm.$el).toContainElement('.blob-commit-info');
}) })
.then(done) .then(done)
.catch(done.fail); .catch(done.fail);
......
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(/&#x000A;/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);
});
});
});
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);
});
});
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 },
);
}
...@@ -9,6 +9,7 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type ...@@ -9,6 +9,7 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
let(:project) { create(:project, :repository, namespace: namespace, path: 'merge-requests-project') } 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(:merge_request) { create(:merge_request, :with_diffs, source_project: project, target_project: project, description: '- [ ] Task List Item') }
let(:path) { "files/ruby/popen.rb" } let(:path) { "files/ruby/popen.rb" }
let(:selected_commit) { merge_request.all_commits[0] }
let(:position) do let(:position) do
Gitlab::Diff::Position.new( Gitlab::Diff::Position.new(
old_path: path, old_path: path,
...@@ -33,6 +34,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type ...@@ -33,6 +34,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
remove_repository(project) remove_repository(project)
end 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| 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(: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) create(:note_on_merge_request, author: admin, project: project, noteable: merge_request)
...@@ -47,13 +56,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type ...@@ -47,13 +56,14 @@ describe Projects::MergeRequests::DiffsController, '(JavaScript fixtures)', type
private 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, get :show,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project, project_id: project,
id: merge_request.to_param, id: merge_request.to_param,
format: :json, format: :json,
view: view view: view,
**extra_params
expect(response).to be_success expect(response).to be_success
store_frontend_fixture(response, fixture_file_name) store_frontend_fixture(response, fixture_file_name)
......
...@@ -51,4 +51,37 @@ describe CommitEntity do ...@@ -51,4 +51,37 @@ describe CommitEntity do
it 'exposes gravatar url that belongs to author' do it 'exposes gravatar url that belongs to author' do
expect(subject.fetch(:author_gravatar_url)).to match /gravatar/ expect(subject.fetch(:author_gravatar_url)).to match /gravatar/
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment