Commit 9cd8f37f authored by Kushal Pandya's avatar Kushal Pandya

Merge branch 'ph/212334/fixTwoButtonReviewSubmit' into 'master'

Fixes two button submitting of merge request reviews

Closes #212334

See merge request gitlab-org/gitlab!43149
parents 39365331 4ccac68f
......@@ -18,11 +18,6 @@ export default {
type: Object,
required: true,
},
diffFile: {
type: Object,
required: false,
default: () => ({}),
},
line: {
type: Object,
required: false,
......
<script>
import { mapActions, mapGetters, mapState } from 'vuex';
import { GlButton, GlLoadingIcon, GlIcon } from '@gitlab/ui';
import { sprintf, n__ } from '~/locale';
import DraftsCount from './drafts_count.vue';
import PublishButton from './publish_button.vue';
import { mapActions, mapGetters } from 'vuex';
import { GlDropdown, GlDropdownItem, GlIcon } from '@gitlab/ui';
import PreviewItem from './preview_item.vue';
export default {
components: {
GlButton,
GlLoadingIcon,
GlDropdown,
GlDropdownItem,
GlIcon,
DraftsCount,
PublishButton,
PreviewItem,
},
computed: {
...mapGetters(['isNotesFetched']),
...mapGetters('batchComments', ['draftsCount', 'sortedDrafts']),
...mapState('batchComments', ['showPreviewDropdown']),
dropdownTitle() {
return sprintf(
n__('%{count} pending comment', '%{count} pending comments', this.draftsCount),
{ count: this.draftsCount },
);
},
},
watch: {
showPreviewDropdown() {
if (this.showPreviewDropdown && this.$refs.dropdown) {
this.$nextTick(() => this.$refs.dropdown.$el.focus());
}
},
},
mounted() {
document.addEventListener('click', this.onClickDocument);
},
beforeDestroy() {
document.removeEventListener('click', this.onClickDocument);
},
methods: {
...mapActions('batchComments', ['toggleReviewDropdown']),
...mapActions('batchComments', ['scrollToDraft']),
isLast(index) {
return index === this.sortedDrafts.length - 1;
},
onClickDocument({ target }) {
if (
this.showPreviewDropdown &&
!target.closest('.review-preview-dropdown, .js-publish-draft-button')
) {
this.toggleReviewDropdown();
}
},
},
};
</script>
<template>
<div
class="dropdown float-right review-preview-dropdown"
:class="{
show: showPreviewDropdown,
}"
<gl-dropdown
:header-text="n__('%d pending comment', '%d pending comments', draftsCount)"
dropup
toggle-class="qa-review-preview-toggle"
>
<gl-button
ref="dropdown"
type="button"
category="primary"
variant="success"
class="review-preview-dropdown-toggle qa-review-preview-toggle"
@click="toggleReviewDropdown"
>
{{ __('Finish review') }}
<drafts-count />
<gl-icon name="angle-up" />
</gl-button>
<div
class="dropdown-menu dropdown-menu-large dropdown-menu-right dropdown-open-top"
:class="{
show: showPreviewDropdown,
}"
<template #button-content>
{{ __('Pending comments') }}
<gl-icon class="dropdown-chevron" name="chevron-up" />
</template>
<gl-dropdown-item
v-for="(draft, index) in sortedDrafts"
:key="draft.id"
@click="scrollToDraft(draft)"
>
<div class="dropdown-title gl-display-flex gl-align-items-center">
<span class="gl-ml-auto">{{ dropdownTitle }}</span>
<gl-button
:aria-label="__('Close')"
type="button"
category="tertiary"
size="small"
class="dropdown-title-button gl-ml-auto gl-p-0!"
icon="close"
@click="toggleReviewDropdown"
/>
</div>
<div class="dropdown-content">
<ul v-if="isNotesFetched">
<li v-for="(draft, index) in sortedDrafts" :key="draft.id">
<preview-item :draft="draft" :is-last="isLast(index)" />
</li>
</ul>
<gl-loading-icon v-else size="lg" class="gl-mt-3 gl-mb-3" />
</div>
<div class="dropdown-footer">
<publish-button
:show-count="false"
:should-publish="true"
:label="__('Submit review')"
class="float-right gl-mr-3"
/>
</div>
</div>
</div>
<preview-item :draft="draft" :is-last="isLast(index)" />
</gl-dropdown-item>
</gl-dropdown>
</template>
<script>
import { mapActions, mapGetters } from 'vuex';
import { mapGetters } from 'vuex';
import { GlSprintf, GlIcon } from '@gitlab/ui';
import { IMAGE_DIFF_POSITION_TYPE } from '~/diffs/constants';
import { sprintf, __ } from '~/locale';
......@@ -78,7 +78,6 @@ export default {
},
},
methods: {
...mapActions('batchComments', ['scrollToDraft']),
getLineClasses(lineNumber) {
return getLineClasses(lineNumber);
},
......@@ -88,17 +87,7 @@ export default {
</script>
<template>
<button
type="button"
class="review-preview-item menu-item"
:class="[
componentClasses,
{
'is-last': isLast,
},
]"
@click="scrollToDraft(draft)"
>
<span>
<span class="review-preview-item-header">
<gl-icon class="flex-shrink-0" :name="iconName" />
<span
......@@ -139,5 +128,5 @@ export default {
>
<gl-icon class="gl-mr-3" name="status_success" /> {{ resolvedStatusMessage }}
</span>
</button>
</span>
</template>
<script>
import { mapActions, mapState } from 'vuex';
import { GlButton } from '@gitlab/ui';
import { __ } from '~/locale';
import DraftsCount from './drafts_count.vue';
export default {
......@@ -15,11 +14,6 @@ export default {
required: false,
default: false,
},
label: {
type: String,
required: false,
default: __('Finish review'),
},
category: {
type: String,
required: false,
......@@ -30,22 +24,14 @@ export default {
required: false,
default: 'success',
},
shouldPublish: {
type: Boolean,
required: true,
},
},
computed: {
...mapState('batchComments', ['isPublishing']),
},
methods: {
...mapActions('batchComments', ['publishReview', 'toggleReviewDropdown']),
...mapActions('batchComments', ['publishReview']),
onClick() {
if (this.shouldPublish) {
this.publishReview();
} else {
this.toggleReviewDropdown();
}
this.publishReview();
},
},
};
......@@ -59,7 +45,7 @@ export default {
:variant="variant"
@click="onClick"
>
{{ label }}
{{ __('Submit review') }}
<drafts-count v-if="showCount" />
</gl-button>
</template>
<script>
/* eslint-disable vue/no-v-html */
import { mapActions, mapState, mapGetters } from 'vuex';
import { GlModal, GlModalDirective, GlButton } from '@gitlab/ui';
import { sprintf, s__ } from '~/locale';
import { mapActions, mapGetters } from 'vuex';
import PreviewDropdown from './preview_dropdown.vue';
import PublishButton from './publish_button.vue';
export default {
components: {
GlButton,
GlModal,
PreviewDropdown,
},
directives: {
'gl-modal': GlModalDirective,
PublishButton,
},
computed: {
...mapGetters(['isNotesFetched']),
...mapState('batchComments', ['isDiscarding']),
...mapGetters('batchComments', ['draftsCount']),
},
watch: {
......@@ -27,45 +20,17 @@ export default {
},
},
methods: {
...mapActions('batchComments', ['discardReview', 'expandAllDiscussions']),
...mapActions('batchComments', ['expandAllDiscussions']),
},
modalId: 'discard-draft-review',
text: sprintf(
s__(
`BatchComments|You're about to discard your review which will delete all of your pending comments.
The deleted comments %{strong_start}cannot%{strong_end} be restored.`,
),
{
strong_start: '<strong>',
strong_end: '</strong>',
},
false,
),
};
</script>
<template>
<div v-show="draftsCount > 0">
<nav class="review-bar-component">
<div class="review-bar-content qa-review-bar">
<div class="review-bar-content qa-review-bar d-flex gl-justify-content-end">
<preview-dropdown />
<gl-button
v-gl-modal="$options.modalId"
:loading="isDiscarding"
class="qa-discard-review float-right"
>
{{ __('Discard review') }}
</gl-button>
<publish-button class="gl-ml-3" show-count />
</div>
</nav>
<gl-modal
:title="s__('BatchComments|Discard review?')"
:ok-title="s__('BatchComments|Delete all pending comments')"
:modal-id="$options.modalId"
title-tag="h4"
ok-variant="danger qa-modal-delete-pending-comments"
@ok="discardReview"
>
<p v-html="$options.text"></p>
</gl-modal>
</div>
</template>
......@@ -75,15 +75,6 @@ export const updateDiscussionsAfterPublish = ({ dispatch, getters, rootGetters }
}),
);
export const discardReview = ({ commit, getters }) => {
commit(types.REQUEST_DISCARD_REVIEW);
return service
.discard(getters.getNotesData.draftsDiscardPath)
.then(() => commit(types.RECEIVE_DISCARD_REVIEW_SUCCESS))
.catch(() => commit(types.RECEIVE_DISCARD_REVIEW_ERROR));
};
export const updateDraft = (
{ commit, getters },
{ note, noteText, resolveDiscussion, position, callback },
......@@ -108,8 +99,6 @@ export const scrollToDraft = ({ dispatch, rootGetters }, draft) => {
const draftID = `note_${draft.id}`;
const el = document.querySelector(`#${tabEl} #${draftID}`);
dispatch('closeReviewDropdown');
window.location.hash = draftID;
if (window.mrTabs.currentAction !== tab) {
......@@ -125,17 +114,6 @@ export const scrollToDraft = ({ dispatch, rootGetters }, draft) => {
}
};
export const toggleReviewDropdown = ({ dispatch, state }) => {
if (state.showPreviewDropdown) {
dispatch('closeReviewDropdown');
} else {
dispatch('openReviewDropdown');
}
};
export const openReviewDropdown = ({ commit }) => commit(types.OPEN_REVIEW_DROPDOWN);
export const closeReviewDropdown = ({ commit }) => commit(types.CLOSE_REVIEW_DROPDOWN);
export const expandAllDiscussions = ({ dispatch, state }) =>
state.drafts
.filter(draft => draft.discussion_id)
......
......@@ -11,13 +11,6 @@ export const REQUEST_PUBLISH_REVIEW = 'REQUEST_PUBLISH_REVIEW';
export const RECEIVE_PUBLISH_REVIEW_SUCCESS = 'RECEIVE_PUBLISH_REVIEW_SUCCESS';
export const RECEIVE_PUBLISH_REVIEW_ERROR = 'RECEIVE_PUBLISH_REVIEW_ERROR';
export const REQUEST_DISCARD_REVIEW = 'REQUEST_DISCARD_REVIEW';
export const RECEIVE_DISCARD_REVIEW_SUCCESS = 'RECEIVE_DISCARD_REVIEW_SUCCESS';
export const RECEIVE_DISCARD_REVIEW_ERROR = 'RECEIVE_DISCARD_REVIEW_ERROR';
export const RECEIVE_DRAFT_UPDATE_SUCCESS = 'RECEIVE_DRAFT_UPDATE_SUCCESS';
export const OPEN_REVIEW_DROPDOWN = 'OPEN_REVIEW_DROPDOWN';
export const CLOSE_REVIEW_DROPDOWN = 'CLOSE_REVIEW_DROPDOWN';
export const TOGGLE_RESOLVE_DISCUSSION = 'TOGGLE_RESOLVE_DISCUSSION';
......@@ -43,16 +43,6 @@ export default {
[types.RECEIVE_PUBLISH_REVIEW_ERROR](state) {
state.isPublishing = false;
},
[types.REQUEST_DISCARD_REVIEW](state) {
state.isDiscarding = true;
},
[types.RECEIVE_DISCARD_REVIEW_SUCCESS](state) {
state.isDiscarding = false;
state.drafts = [];
},
[types.RECEIVE_DISCARD_REVIEW_ERROR](state) {
state.isDiscarding = false;
},
[types.RECEIVE_DRAFT_UPDATE_SUCCESS](state, data) {
const index = state.drafts.findIndex(draft => draft.id === data.id);
......@@ -60,12 +50,6 @@ export default {
state.drafts.splice(index, 1, processDraft(data));
}
},
[types.OPEN_REVIEW_DROPDOWN](state) {
state.showPreviewDropdown = true;
},
[types.CLOSE_REVIEW_DROPDOWN](state) {
state.showPreviewDropdown = false;
},
[types.TOGGLE_RESOLVE_DISCUSSION](state, draftId) {
state.drafts = state.drafts.map(draft => {
if (draft.id === draftId) {
......
......@@ -4,6 +4,4 @@ export default () => ({
drafts: [],
isPublishing: false,
currentlyPublishingDrafts: [],
isDiscarding: false,
showPreviewDropdown: false,
});
---
title: Improve two button review submit in merge requests
merge_request: 43149
author:
type: changed
......@@ -265,6 +265,11 @@ msgid_plural "%d open issues"
msgstr[0] ""
msgstr[1] ""
msgid "%d pending comment"
msgid_plural "%d pending comments"
msgstr[0] ""
msgstr[1] ""
msgid "%d personal project will be removed and cannot be restored."
msgid_plural "%d personal projects will be removed and cannot be restored."
msgstr[0] ""
......@@ -424,11 +429,6 @@ msgid_plural "%{count} participants"
msgstr[0] ""
msgstr[1] ""
msgid "%{count} pending comment"
msgid_plural "%{count} pending comments"
msgstr[0] ""
msgstr[1] ""
msgid "%{count} related %{pluralized_subject}: %{links}"
msgstr ""
......@@ -3987,15 +3987,6 @@ msgstr ""
msgid "BambooService|You must set up automatic revision labeling and a repository trigger in Bamboo."
msgstr ""
msgid "BatchComments|Delete all pending comments"
msgstr ""
msgid "BatchComments|Discard review?"
msgstr ""
msgid "BatchComments|You're about to discard your review which will delete all of your pending comments. The deleted comments %{strong_start}cannot%{strong_end} be restored."
msgstr ""
msgid "Be careful. Changing the project's namespace can have unintended side effects."
msgstr ""
......@@ -9069,9 +9060,6 @@ msgstr ""
msgid "Discard draft"
msgstr ""
msgid "Discard review"
msgstr ""
msgid "DiscordService|Discord Notifications"
msgstr ""
......@@ -11323,9 +11311,6 @@ msgstr ""
msgid "Finish editing this message first!"
msgstr ""
msgid "Finish review"
msgstr ""
msgid "Finish setting up your dedicated account for %{group_name}."
msgstr ""
......@@ -18578,6 +18563,9 @@ msgstr ""
msgid "Pending"
msgstr ""
msgid "Pending comments"
msgstr ""
msgid "People without permission will never get a notification and won't be able to comment."
msgstr ""
......
......@@ -75,8 +75,6 @@ module QA
view 'app/assets/javascripts/batch_comments/components/review_bar.vue' do
element :review_bar
element :discard_review
element :modal_delete_pending_comments
end
view 'app/assets/javascripts/notes/components/note_form.vue' do
......
......@@ -41,7 +41,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
write_comment
page.within('.review-bar-content') do
click_button 'Finish review'
click_button 'Submit review'
end
......@@ -64,18 +63,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
expect(page).to have_selector('.note:not(.draft-note)', text: 'Line is wrong')
end
it 'discards review' do
write_comment
click_button 'Discard review'
click_button 'Delete all pending comments'
wait_for_requests
expect(page).not_to have_selector('.draft-note-component')
end
it 'deletes draft note' do
write_comment
......@@ -149,7 +136,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
write_reply_to_discussion(resolve: true)
page.within('.review-bar-content') do
click_button 'Finish review'
click_button 'Submit review'
end
......@@ -192,7 +178,6 @@ RSpec.describe 'Merge request > Batch comments', :js do
write_reply_to_discussion(button_text: 'Start a review', unresolve: true)
page.within('.review-bar-content') do
click_button 'Finish review'
click_button 'Submit review'
end
......
......@@ -43,22 +43,6 @@ describe('Batch comments draft preview item component', () => {
);
});
it('adds is last class', () => {
createComponent(true);
expect(vm.$el.classList).toContain('is-last');
});
it('scrolls to draft on click', () => {
createComponent();
jest.spyOn(vm.$store, 'dispatch').mockImplementation();
vm.$el.click();
expect(vm.$store.dispatch).toHaveBeenCalledWith('batchComments/scrollToDraft', vm.draft);
});
describe('for file', () => {
it('renders file path', () => {
createComponent(false, { file_path: 'index.js', file_hash: 'abc', position: {} });
......
......@@ -29,17 +29,6 @@ describe('Batch comments publish button component', () => {
expect(vm.$store.dispatch).toHaveBeenCalledWith('batchComments/publishReview', undefined);
});
it('dispatches toggleReviewDropdown when shouldPublish is false on click', () => {
vm.shouldPublish = false;
vm.$el.click();
expect(vm.$store.dispatch).toHaveBeenCalledWith(
'batchComments/toggleReviewDropdown',
undefined,
);
});
it('sets loading when isPublishing is true', done => {
vm.$store.state.batchComments.isPublishing = true;
......
import Vue from 'vue';
import { mountComponentWithStore } from 'helpers/vue_mount_component_helper';
import Vuex from 'vuex';
import { shallowMount, createLocalVue } from '@vue/test-utils';
import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
import PreviewDropdown from '~/batch_comments/components/preview_dropdown.vue';
import { createStore } from '~/mr_notes/stores';
import '~/behaviors/markdown/render_gfm';
import { createDraft } from '../mock_data';
const localVue = createLocalVue();
localVue.use(Vuex);
describe('Batch comments publish dropdown component', () => {
let vm;
let Component;
let wrapper;
function createComponent(extendStore = () => {}) {
function createComponent() {
const store = createStore();
store.state.batchComments.drafts.push(createDraft(), { ...createDraft(), id: 2 });
extendStore(store);
vm = mountComponentWithStore(Component, { store });
wrapper = shallowMount(PreviewDropdown, {
store,
});
}
beforeAll(() => {
Component = Vue.extend(PreviewDropdown);
});
afterEach(() => {
vm.$destroy();
});
it('toggles dropdown when clicking button', done => {
createComponent();
jest.spyOn(vm.$store, 'dispatch');
vm.$el.querySelector('.review-preview-dropdown-toggle').click();
expect(vm.$store.dispatch).toHaveBeenCalledWith(
'batchComments/toggleReviewDropdown',
expect.anything(),
);
setImmediate(() => {
expect(vm.$el.classList).toContain('show');
done();
});
});
it('toggles dropdown when clicking body', () => {
createComponent();
vm.$store.state.batchComments.showPreviewDropdown = true;
jest.spyOn(vm.$store, 'dispatch').mockImplementation();
document.body.click();
expect(vm.$store.dispatch).toHaveBeenCalledWith(
'batchComments/toggleReviewDropdown',
undefined,
);
wrapper.destroy();
});
it('renders list of drafts', () => {
createComponent(store => {
Object.assign(store.state.notes, {
isNotesFetched: true,
});
});
expect(vm.$el.querySelectorAll('.dropdown-content li').length).toBe(2);
});
it('adds is-last class to last item', () => {
createComponent(store => {
Object.assign(store.state.notes, {
isNotesFetched: true,
});
});
expect(vm.$el.querySelectorAll('.dropdown-content li')[1].querySelector('.is-last')).not.toBe(
null,
);
});
it('renders draft count in dropdown title', () => {
createComponent();
expect(vm.$el.querySelector('.dropdown-title').textContent).toContain('2 pending comments');
expect(wrapper.findAll(GlDropdownItem).length).toBe(2);
});
it('renders publish button in footer', () => {
it('renders draft count in dropdown title', () => {
createComponent();
expect(vm.$el.querySelector('.dropdown-footer .js-publish-draft-button')).not.toBe(null);
expect(wrapper.find(GlDropdown).props('headerText')).toEqual('2 pending comments');
});
});
......@@ -199,42 +199,6 @@ describe('Batch comments store actions', () => {
});
});
describe('discardReview', () => {
it('commits mutations', done => {
const getters = {
getNotesData: { draftsDiscardPath: TEST_HOST },
};
const commit = jest.fn();
mock.onAny().reply(200);
actions
.discardReview({ getters, commit })
.then(() => {
expect(commit.mock.calls[0]).toEqual(['REQUEST_DISCARD_REVIEW']);
expect(commit.mock.calls[1]).toEqual(['RECEIVE_DISCARD_REVIEW_SUCCESS']);
})
.then(done)
.catch(done.fail);
});
it('commits error mutations', done => {
const getters = {
getNotesData: { draftsDiscardPath: TEST_HOST },
};
const commit = jest.fn();
mock.onAny().reply(500);
actions
.discardReview({ getters, commit })
.then(() => {
expect(commit.mock.calls[0]).toEqual(['REQUEST_DISCARD_REVIEW']);
expect(commit.mock.calls[1]).toEqual(['RECEIVE_DISCARD_REVIEW_ERROR']);
})
.then(done)
.catch(done.fail);
});
});
describe('updateDraft', () => {
let getters;
......@@ -284,56 +248,6 @@ describe('Batch comments store actions', () => {
});
});
describe('toggleReviewDropdown', () => {
it('dispatches openReviewDropdown', done => {
testAction(
actions.toggleReviewDropdown,
null,
{ showPreviewDropdown: false },
[],
[{ type: 'openReviewDropdown' }],
done,
);
});
it('dispatches closeReviewDropdown when showPreviewDropdown is true', done => {
testAction(
actions.toggleReviewDropdown,
null,
{ showPreviewDropdown: true },
[],
[{ type: 'closeReviewDropdown' }],
done,
);
});
});
describe('openReviewDropdown', () => {
it('commits OPEN_REVIEW_DROPDOWN', done => {
testAction(
actions.openReviewDropdown,
null,
null,
[{ type: 'OPEN_REVIEW_DROPDOWN' }],
[],
done,
);
});
});
describe('closeReviewDropdown', () => {
it('commits CLOSE_REVIEW_DROPDOWN', done => {
testAction(
actions.closeReviewDropdown,
null,
null,
[{ type: 'CLOSE_REVIEW_DROPDOWN' }],
[],
done,
);
});
});
describe('expandAllDiscussions', () => {
it('dispatches expandDiscussion for all drafts', done => {
const state = {
......@@ -383,9 +297,7 @@ describe('Batch comments store actions', () => {
actions.scrollToDraft({ dispatch, rootGetters }, draft);
expect(dispatch.mock.calls[0]).toEqual(['closeReviewDropdown']);
expect(dispatch.mock.calls[1]).toEqual([
expect(dispatch.mock.calls[0]).toEqual([
'expandDiscussion',
{ discussionId: '1' },
{ root: true },
......
......@@ -89,42 +89,6 @@ describe('Batch comments mutations', () => {
});
});
describe(types.REQUEST_DISCARD_REVIEW, () => {
it('sets isDiscarding to true', () => {
mutations[types.REQUEST_DISCARD_REVIEW](state);
expect(state.isDiscarding).toBe(true);
});
});
describe(types.RECEIVE_DISCARD_REVIEW_SUCCESS, () => {
it('emptys drafts array', () => {
state.drafts.push('test');
mutations[types.RECEIVE_DISCARD_REVIEW_SUCCESS](state);
expect(state.drafts).toEqual([]);
});
it('sets isDiscarding to false', () => {
state.isDiscarding = true;
mutations[types.RECEIVE_DISCARD_REVIEW_SUCCESS](state);
expect(state.isDiscarding).toBe(false);
});
});
describe(types.RECEIVE_DISCARD_REVIEW_ERROR, () => {
it('updates isDiscarding to false', () => {
state.isDiscarding = true;
mutations[types.RECEIVE_DISCARD_REVIEW_ERROR](state);
expect(state.isDiscarding).toBe(false);
});
});
describe(types.RECEIVE_DRAFT_UPDATE_SUCCESS, () => {
it('updates draft in store', () => {
state.drafts.push({ id: 1 });
......@@ -140,20 +104,4 @@ describe('Batch comments mutations', () => {
]);
});
});
describe(types.OPEN_REVIEW_DROPDOWN, () => {
it('sets showPreviewDropdown to true', () => {
mutations[types.OPEN_REVIEW_DROPDOWN](state);
expect(state.showPreviewDropdown).toBe(true);
});
});
describe(types.CLOSE_REVIEW_DROPDOWN, () => {
it('sets showPreviewDropdown to false', () => {
mutations[types.CLOSE_REVIEW_DROPDOWN](state);
expect(state.showPreviewDropdown).toBe(false);
});
});
});
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