From 95b58f41dcd50d50976c69f9f0ceaba6996431d8 Mon Sep 17 00:00:00 2001 From: Olena Horal-Koretska Date: Fri, 5 Nov 2021 20:38:10 +0200 Subject: [PATCH 1/2] Replace `window.confirm` with `GlModal` confirmation Changelog: changed --- .../diffs/components/diff_line_note_form.vue | 9 +-- .../confirm_via_gl_modal.js | 44 +++++++++------ .../user_posts_diff_notes_spec.rb | 6 +- .../components/diff_line_note_form_spec.js | 55 ++++++++++++------- 4 files changed, 70 insertions(+), 44 deletions(-) diff --git a/app/assets/javascripts/diffs/components/diff_line_note_form.vue b/app/assets/javascripts/diffs/components/diff_line_note_form.vue index 591800ee9be2ef..9d355c96af1a3b 100644 --- a/app/assets/javascripts/diffs/components/diff_line_note_form.vue +++ b/app/assets/javascripts/diffs/components/diff_line_note_form.vue @@ -2,6 +2,7 @@ import { mapState, mapGetters, mapActions } from 'vuex'; import { s__ } from '~/locale'; import diffLineNoteFormMixin from '~/notes/mixins/diff_line_note_form'; +import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import MultilineCommentForm from '../../notes/components/multiline_comment_form.vue'; import { @@ -177,16 +178,16 @@ export default { 'saveDiffDiscussion', 'setSuggestPopoverDismissed', ]), - handleCancelCommentForm(shouldConfirm, isDirty) { + async handleCancelCommentForm(shouldConfirm, isDirty) { if (shouldConfirm && isDirty) { const msg = s__('Notes|Are you sure you want to cancel creating this comment?'); - // eslint-disable-next-line no-alert - if (!window.confirm(msg)) { + const confirmed = await confirmAction(msg); + + if (!confirmed) { return; } } - this.cancelCommentForm({ lineCode: this.line.line_code, fileHash: this.diffFileHash, diff --git a/app/assets/javascripts/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal.js b/app/assets/javascripts/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal.js index f0908a60ac53d5..fdd0e045d0707f 100644 --- a/app/assets/javascripts/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal.js +++ b/app/assets/javascripts/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal.js @@ -1,25 +1,9 @@ import Vue from 'vue'; -export function confirmViaGlModal(message, element) { +export function confirmAction(message, { primaryBtnVariant, primaryBtnText } = {}) { return new Promise((resolve) => { let confirmed = false; - const props = {}; - - const confirmBtnVariant = element.getAttribute('data-confirm-btn-variant'); - - if (confirmBtnVariant) { - props.primaryVariant = confirmBtnVariant; - } - const screenReaderText = - element.querySelector('.gl-sr-only')?.textContent || - element.querySelector('.sr-only')?.textContent || - element.getAttribute('aria-label'); - - if (screenReaderText) { - props.primaryText = screenReaderText; - } - const component = new Vue({ components: { ConfirmModal: () => import('./confirm_modal.vue'), @@ -28,7 +12,10 @@ export function confirmViaGlModal(message, element) { return h( 'confirm-modal', { - props, + props: { + primaryVariant: primaryBtnVariant, + primaryText: primaryBtnText, + }, on: { confirmed() { confirmed = true; @@ -45,3 +32,24 @@ export function confirmViaGlModal(message, element) { }).$mount(); }); } + +export function confirmViaGlModal(message, element) { + const primaryBtnConfig = {}; + + const confirmBtnVariant = element.getAttribute('data-confirm-btn-variant'); + + if (confirmBtnVariant) { + primaryBtnConfig.primaryBtnVariant = confirmBtnVariant; + } + + const screenReaderText = + element.querySelector('.gl-sr-only')?.textContent || + element.querySelector('.sr-only')?.textContent || + element.getAttribute('aria-label'); + + if (screenReaderText) { + primaryBtnConfig.primaryBtnText = screenReaderText; + } + + return confirmAction(message, primaryBtnConfig); +} diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index dcd289c76270f7..0289be93b6c500 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -238,8 +238,10 @@ def should_allow_commenting(line_holder, diff_side = nil, asset_form_reset: true def should_allow_dismissing_a_comment(line_holder, diff_side = nil) write_comment_on_line(line_holder, diff_side) - accept_confirm do - find('.js-close-discussion-note-form').click + find('.js-close-discussion-note-form').click + + page.within('.modal') do + find('.js-modal-action-primary').click end assert_comment_dismissal(line_holder) diff --git a/spec/frontend/diffs/components/diff_line_note_form_spec.js b/spec/frontend/diffs/components/diff_line_note_form_spec.js index 104a003b12b7e5..0ccf996e220061 100644 --- a/spec/frontend/diffs/components/diff_line_note_form_spec.js +++ b/spec/frontend/diffs/components/diff_line_note_form_spec.js @@ -1,10 +1,18 @@ import { shallowMount } from '@vue/test-utils'; +import { nextTick } from 'vue'; import DiffLineNoteForm from '~/diffs/components/diff_line_note_form.vue'; import { createStore } from '~/mr_notes/stores'; import NoteForm from '~/notes/components/note_form.vue'; +import { confirmAction } from '~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal'; import { noteableDataMock } from '../../notes/mock_data'; import diffFileMockData from '../mock_data/diff_file'; +jest.mock('~/lib/utils/confirm_via_gl_modal/confirm_via_gl_modal', () => { + return { + confirmAction: jest.fn(), + }; +}); + describe('DiffLineNoteForm', () => { let wrapper; let diffFile; @@ -36,49 +44,56 @@ describe('DiffLineNoteForm', () => { }); }; + const findNoteForm = () => wrapper.findComponent(NoteForm); + describe('methods', () => { beforeEach(() => { wrapper = createComponent(); }); describe('handleCancelCommentForm', () => { + afterEach(() => { + confirmAction.mockReset(); + }); + it('should ask for confirmation when shouldConfirm and isDirty passed as truthy', () => { - jest.spyOn(window, 'confirm').mockReturnValue(false); + confirmAction.mockResolvedValueOnce(false); - wrapper.vm.handleCancelCommentForm(true, true); + findNoteForm().vm.$emit('cancelForm', true, true); - expect(window.confirm).toHaveBeenCalled(); + expect(confirmAction).toHaveBeenCalled(); }); - it('should ask for confirmation when one of the params false', () => { - jest.spyOn(window, 'confirm').mockReturnValue(false); + it('should not ask for confirmation when one of the params false', () => { + confirmAction.mockResolvedValueOnce(false); - wrapper.vm.handleCancelCommentForm(true, false); + findNoteForm().vm.$emit('cancelForm', true, false); - expect(window.confirm).not.toHaveBeenCalled(); + expect(confirmAction).not.toHaveBeenCalled(); - wrapper.vm.handleCancelCommentForm(false, true); + findNoteForm().vm.$emit('cancelForm', false, true); - expect(window.confirm).not.toHaveBeenCalled(); + expect(confirmAction).not.toHaveBeenCalled(); }); - it('should call cancelCommentForm with lineCode', (done) => { - jest.spyOn(window, 'confirm').mockImplementation(() => {}); + it('should call cancelCommentForm with lineCode', async () => { + confirmAction.mockResolvedValueOnce(true); jest.spyOn(wrapper.vm, 'cancelCommentForm').mockImplementation(() => {}); jest.spyOn(wrapper.vm, 'resetAutoSave').mockImplementation(() => {}); - wrapper.vm.handleCancelCommentForm(); - expect(window.confirm).not.toHaveBeenCalled(); - wrapper.vm.$nextTick(() => { - expect(wrapper.vm.cancelCommentForm).toHaveBeenCalledWith({ - lineCode: diffLines[1].line_code, - fileHash: wrapper.vm.diffFileHash, - }); + findNoteForm().vm.$emit('cancelForm', true, true); + + await nextTick(); + + expect(confirmAction).toHaveBeenCalled(); - expect(wrapper.vm.resetAutoSave).toHaveBeenCalled(); + await nextTick(); - done(); + expect(wrapper.vm.cancelCommentForm).toHaveBeenCalledWith({ + lineCode: diffLines[1].line_code, + fileHash: wrapper.vm.diffFileHash, }); + expect(wrapper.vm.resetAutoSave).toHaveBeenCalled(); }); }); -- GitLab From 600e24fec14facd94e2642aa6b62933d0832ab36 Mon Sep 17 00:00:00 2001 From: Coung Ngo Date: Mon, 15 Nov 2021 16:14:14 +0000 Subject: [PATCH 2/2] Apply 1 suggestion(s) to 1 file(s) --- spec/features/merge_request/user_posts_diff_notes_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/merge_request/user_posts_diff_notes_spec.rb b/spec/features/merge_request/user_posts_diff_notes_spec.rb index 0289be93b6c500..79e46e69157876 100644 --- a/spec/features/merge_request/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_request/user_posts_diff_notes_spec.rb @@ -241,7 +241,7 @@ def should_allow_dismissing_a_comment(line_holder, diff_side = nil) find('.js-close-discussion-note-form').click page.within('.modal') do - find('.js-modal-action-primary').click + click_button 'OK' end assert_comment_dismissal(line_holder) -- GitLab