diff options
| -rw-r--r-- | app/assets/javascripts/diffs/components/app.vue | 7 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/commit_item.vue | 119 | ||||
| -rw-r--r-- | app/assets/javascripts/diffs/components/commit_widget.vue | 40 | ||||
| -rw-r--r-- | app/controllers/projects/merge_requests/diffs_controller.rb | 2 | ||||
| -rw-r--r-- | app/serializers/commit_entity.rb | 17 | ||||
| -rw-r--r-- | app/serializers/diffs_entity.rb | 7 | ||||
| -rw-r--r-- | app/views/projects/commits/_commit.html.haml | 4 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/diffs/_commit_widget.html.haml | 4 | ||||
| -rw-r--r-- | changelogs/unreleased/49329-mr-show-commit-details.yml | 5 | ||||
| -rw-r--r-- | locale/gitlab.pot | 6 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/app_spec.js | 8 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/commit_item_spec.js | 128 | ||||
| -rw-r--r-- | spec/javascripts/diffs/components/commit_widget_spec.js | 24 | ||||
| -rw-r--r-- | spec/javascripts/diffs/mock_data/diff_with_commit.js | 12 | ||||
| -rw-r--r-- | spec/javascripts/fixtures/merge_requests_diffs.rb | 14 | ||||
| -rw-r--r-- | spec/serializers/commit_entity_spec.rb | 33 |
16 files changed, 420 insertions, 10 deletions
diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index bfb992340bc..fc41ee4b777 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/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" /> diff --git a/app/assets/javascripts/diffs/components/commit_item.vue b/app/assets/javascripts/diffs/components/commit_item.vue new file mode 100644 index 00000000000..5758588e82e --- /dev/null +++ b/app/assets/javascripts/diffs/components/commit_item.vue @@ -0,0 +1,119 @@ +<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"> + · + {{ 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> diff --git a/app/assets/javascripts/diffs/components/commit_widget.vue b/app/assets/javascripts/diffs/components/commit_widget.vue new file mode 100644 index 00000000000..cc8e72eb1c8 --- /dev/null +++ b/app/assets/javascripts/diffs/components/commit_widget.vue @@ -0,0 +1,40 @@ +<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> diff --git a/app/controllers/projects/merge_requests/diffs_controller.rb b/app/controllers/projects/merge_requests/diffs_controller.rb index 666e65b6c5e..3d9ade77467 100644 --- a/app/controllers/projects/merge_requests/diffs_controller.rb +++ b/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 diff --git a/app/serializers/commit_entity.rb b/app/serializers/commit_entity.rb index ce76659fa46..396e95a03c8 100644 --- a/app/serializers/commit_entity.rb +++ b/app/serializers/commit_entity.rb @@ -1,6 +1,7 @@ # 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 diff --git a/app/serializers/diffs_entity.rb b/app/serializers/diffs_entity.rb index 878cc5290bd..00dc55fc004 100644 --- a/app/serializers/diffs_entity.rb +++ b/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| diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 45b4f03fa0c..c6789e32dbe 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -1,3 +1,7 @@ +-#----------------------------------------------------------------- + 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 } diff --git a/app/views/projects/merge_requests/diffs/_commit_widget.html.haml b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml index dab95b97346..066c8d5dba6 100644 --- a/app/views/projects/merge_requests/diffs/_commit_widget.html.haml +++ b/app/views/projects/merge_requests/diffs/_commit_widget.html.haml @@ -1,3 +1,7 @@ +-#----------------------------------------------------------------- + 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 diff --git a/changelogs/unreleased/49329-mr-show-commit-details.yml b/changelogs/unreleased/49329-mr-show-commit-details.yml new file mode 100644 index 00000000000..23cfc0c675e --- /dev/null +++ b/changelogs/unreleased/49329-mr-show-commit-details.yml @@ -0,0 +1,5 @@ +--- +title: Show commit details for selected commit in MR diffs +merge_request: 21784 +author: +type: fixed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 1b66786a890..4d8f05a81cc 100644 --- a/locale/gitlab.pot +++ b/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 "" 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 |