From f3901842493c58faba71ad0812bf3102790a1b9f Mon Sep 17 00:00:00 2001 From: Paul Slaughter Date: Wed, 26 Sep 2018 08:28:50 +0000 Subject: Resolve "Commit details are not displayed when reviewing an MR commit by commit" --- app/assets/javascripts/diffs/components/app.vue | 7 ++ .../javascripts/diffs/components/commit_item.vue | 119 +++++++++++++++++++ .../javascripts/diffs/components/commit_widget.vue | 40 +++++++ .../projects/merge_requests/diffs_controller.rb | 2 +- app/serializers/commit_entity.rb | 17 ++- app/serializers/diffs_entity.rb | 7 +- app/views/projects/commits/_commit.html.haml | 4 + .../merge_requests/diffs/_commit_widget.html.haml | 4 + .../unreleased/49329-mr-show-commit-details.yml | 5 + locale/gitlab.pot | 6 + spec/javascripts/diffs/components/app_spec.js | 8 +- .../diffs/components/commit_item_spec.js | 128 +++++++++++++++++++++ .../diffs/components/commit_widget_spec.js | 24 ++++ .../diffs/mock_data/diff_with_commit.js | 12 ++ spec/javascripts/fixtures/merge_requests_diffs.rb | 14 ++- spec/serializers/commit_entity_spec.rb | 33 ++++++ 16 files changed, 420 insertions(+), 10 deletions(-) create mode 100644 app/assets/javascripts/diffs/components/commit_item.vue create mode 100644 app/assets/javascripts/diffs/components/commit_widget.vue create mode 100644 changelogs/unreleased/49329-mr-show-commit-details.yml create mode 100644 spec/javascripts/diffs/components/commit_item_spec.js create mode 100644 spec/javascripts/diffs/components/commit_widget_spec.js create mode 100644 spec/javascripts/diffs/mock_data/diff_with_commit.js 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 { + + 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 @@ + + + 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 @@ + + + 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 -- cgit v1.2.1