From c457cac1348ed255baab801afd75e6199756147d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 7 Jul 2022 14:18:03 -0600 Subject: [PATCH 01/10] Warn before closing the Diffs if there's a pending batch review Changelog: added --- .../javascripts/batch_comments/constants.js | 3 + .../javascripts/batch_comments/events.js | 3 + .../stores/modules/batch_comments/actions.js | 34 +++++++++- .../javascripts/diffs/components/app.vue | 68 +++++++++++++++++++ app/assets/javascripts/diffs/constants.js | 3 + app/assets/javascripts/diffs/i18n.js | 2 + locale/gitlab.pot | 3 + 7 files changed, 113 insertions(+), 3 deletions(-) create mode 100644 app/assets/javascripts/batch_comments/events.js diff --git a/app/assets/javascripts/batch_comments/constants.js b/app/assets/javascripts/batch_comments/constants.js index 5e026251e0b4f0..f3854d5edb7602 100644 --- a/app/assets/javascripts/batch_comments/constants.js +++ b/app/assets/javascripts/batch_comments/constants.js @@ -3,3 +3,6 @@ export const DISCUSSION_TAB = 'notes'; export const SHOW_TAB = 'show'; export const REVIEW_BAR_VISIBLE_CLASS_NAME = 'review-bar-visible'; + +// Known events for the Batch Comments app +export const EVT_BATCH_COMMENTS_UPDATED = 'app::code review::batch comments::pending review'; diff --git a/app/assets/javascripts/batch_comments/events.js b/app/assets/javascripts/batch_comments/events.js new file mode 100644 index 00000000000000..3e0c313f5e85f8 --- /dev/null +++ b/app/assets/javascripts/batch_comments/events.js @@ -0,0 +1,3 @@ +import eventHubFactory from '~/helpers/event_hub_factory'; + +export default eventHubFactory(); diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js index a44b9827fe9b57..49f893dacc401e 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js @@ -1,11 +1,28 @@ import { isEmpty } from 'lodash'; + import createFlash from '~/flash'; import { scrollToElement } from '~/lib/utils/common_utils'; import { __ } from '~/locale'; -import { CHANGES_TAB, DISCUSSION_TAB, SHOW_TAB } from '../../../constants'; + +import { + CHANGES_TAB, + DISCUSSION_TAB, + SHOW_TAB, + EVT_BATCH_COMMENTS_UPDATED, +} from '../../../constants'; import service from '../../../services/drafts_service'; +import BatchCommentsEventHub from '../../../events'; + import * as types from './mutation_types'; +function broadcastDraftsStatus(drafts) { + if (drafts.length) { + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); + } else { + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); + } +} + export const saveDraft = ({ dispatch }, draft) => dispatch('saveNote', { ...draft, isDraft: true }, { root: true }); @@ -15,6 +32,9 @@ export const addDraftToDiscussion = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + + broadcastDraftsStatus([res]); + return res; }) .catch(() => { @@ -29,6 +49,9 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + + broadcastDraftsStatus([res]); + return res; }) .catch(() => { @@ -37,11 +60,13 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => }); }); -export const deleteDraft = ({ commit, getters }, draft) => +export const deleteDraft = ({ state, commit, getters }, draft) => service .deleteDraft(getters.getNotesData.draftsPath, draft.id) .then(() => { commit(types.DELETE_DRAFT, draft.id); + + broadcastDraftsStatus(state.drafts); }) .catch(() => createFlash({ @@ -55,6 +80,7 @@ export const fetchDrafts = ({ commit, getters, state, dispatch }) => .then((res) => res.data) .then((data) => commit(types.SET_BATCH_COMMENTS_DRAFTS, data)) .then(() => { + broadcastDraftsStatus(state.drafts); state.drafts.forEach((draft) => { if (!draft.line_code) { dispatch('convertToDiscussion', draft.discussion_id, { root: true }); @@ -87,13 +113,15 @@ export const publishReview = ({ commit, dispatch, getters }, noteData = {}) => { .catch(() => commit(types.RECEIVE_PUBLISH_REVIEW_ERROR)); }; -export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGetters }) => { +export const updateDiscussionsAfterPublish = async ({ state, dispatch, getters, rootGetters }) => { await dispatch( 'fetchDiscussions', { path: getters.getNotesData.discussionsPath }, { root: true }, ); + broadcastDraftsStatus(state.drafts); + dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, { root: true, }); diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 530f3a3a7f73f1..15f8915fd67c70 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -15,11 +15,14 @@ import createFlash from '~/flash'; import { isSingleViewStyle } from '~/helpers/diffs_helper'; import { helpPagePath } from '~/helpers/help_page_helper'; import { parseBoolean } from '~/lib/utils/common_utils'; +import { machine as fsm } from '~/lib/utils/finite_state_machine'; import { updateHistory } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; +import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; +import batchCommentsEventHub from '~/batch_comments/events'; import notesEventHub from '~/notes/event_hub'; import { TREE_LIST_WIDTH_STORAGE_KEY, @@ -39,7 +42,12 @@ import { TRACKING_WHITESPACE_HIDE, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, + STATE_IDLING, + STATE_PENDING_REVIEW, + TRANSITION_HAS_PENDING_REVIEW, + TRANSITION_NO_REVIEW, } from '../constants'; +import { INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS } from '../i18n'; import diffsEventHub from '../event_hub'; import { reviewStatuses } from '../utils/file_reviews'; @@ -179,6 +187,9 @@ export default { default: false, }, }, + i18n: { + INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS, + }, data() { const treeWidth = parseInt(localStorage.getItem(TREE_LIST_WIDTH_STORAGE_KEY), 10) || INITIAL_TREE_WIDTH; @@ -374,6 +385,7 @@ export default { queueRedisHllEvents(events, { verifyCap: true }); this.subscribeToVirtualScrollingEvents(); + this.subscribeToCrossAppCommunicationEvents(); }, beforeCreate() { diffsApp.instrument(); @@ -408,11 +420,28 @@ export default { } }, ); + + this.batchCommentsFSM = fsm({ + initial: STATE_IDLING, + states: { + [STATE_IDLING]: { + on: { + [TRANSITION_HAS_PENDING_REVIEW]: STATE_PENDING_REVIEW, + }, + }, + [STATE_PENDING_REVIEW]: { + on: { + [TRANSITION_NO_REVIEW]: STATE_IDLING, + }, + }, + }, + }); }, beforeDestroy() { diffsApp.deinstrument(); this.unsubscribeFromEvents(); this.removeEventListeners(); + this.unsubscribeFromCrossAppCommunicationEvents(); diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash); diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex); @@ -445,6 +474,12 @@ export default { notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); }, + subscribeToCrossAppCommunicationEvents() { + batchCommentsEventHub.$on(EVT_BATCH_COMMENTS_UPDATED, this.togglePendingComments); + }, + unsubscribeFromCrossAppCommunicationEvents() { + batchCommentsEventHub.$off(EVT_BATCH_COMMENTS_UPDATED, this.togglePendingComments); + }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, @@ -608,6 +643,39 @@ export default { reloadPage() { window.location.reload(); }, + togglePendingComments(hasDraftComments) { + const closeInterrupt = (event) => { + event.preventDefault(); + + // This is the correct way to write backwards-compatible beforeunload listeners + /* eslint-disable-next-line no-return-assign, no-param-reassign */ + return (event.returnValue = this.$options.i18n.INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS); + }; + + /* + * This stuff is a lot trickier than it looks. + * + * Mandatory reading: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event + * Some notable sentences: + * - "[...] browsers may not display prompts created in beforeunload event handlers unless the + * page has been interacted with, or may even not display them at all." + * - "Especially on mobile, the beforeunload event is not reliably fired." + * - "The beforeunload event is not compatible with the back/forward cache (bfcache) [...] + * It is recommended that developers listen for beforeunload only in this scenario, and only + * when they actually have unsaved changes, so as to minimize the effect on performance." + * + * Please ensure that this is really not working before you modify it, because there are a LOT + * of scenarios where browser behavior will make it _seem_ like it's not working, but it actually + * is under the right combination of contexts. + */ + if (hasDraftComments && this.batchCommentsFSM.is(STATE_IDLING)) { + this.batchCommentsFSM.send(TRANSITION_HAS_PENDING_REVIEW); + window.addEventListener('beforeunload', closeInterrupt, { capture: true }); + } else if (!hasDraftComments) { + this.batchCommentsFSM.send(TRANSITION_NO_REVIEW); + window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); + } + }, }, minTreeWidth: MIN_TREE_WIDTH, maxTreeWidth: window.innerWidth / 2, diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 6c0c9c4e1d07f7..1cc96ef3d54b80 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -71,12 +71,15 @@ export const DIFF_FILE_MANUAL_COLLAPSE = 'manual'; export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; export const STATE_ERRORED = 'errored'; +export const STATE_PENDING_REVIEW = 'pending_comments'; // State machine transitions export const TRANSITION_LOAD_START = 'LOAD_START'; export const TRANSITION_LOAD_ERROR = 'LOAD_ERROR'; export const TRANSITION_LOAD_SUCCEED = 'LOAD_SUCCEED'; export const TRANSITION_ACKNOWLEDGE_ERROR = 'ACKNOWLEDGE_ERROR'; +export const TRANSITION_HAS_PENDING_REVIEW = 'PENDING_REVIEW'; +export const TRANSITION_NO_REVIEW = 'NO_REVIEW'; export const RENAMED_DIFF_TRANSITIONS = { [`${STATE_IDLING}:${TRANSITION_LOAD_START}`]: STATE_LOADING, diff --git a/app/assets/javascripts/diffs/i18n.js b/app/assets/javascripts/diffs/i18n.js index e617890af2e3b6..507d1d5d4750a4 100644 --- a/app/assets/javascripts/diffs/i18n.js +++ b/app/assets/javascripts/diffs/i18n.js @@ -47,3 +47,5 @@ export const CONFLICT_TEXT = { 'Conflict: This file was added both in the source and target branches, but with different contents.', ), }; + +export const INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS = __('There are unsubmitted review comments.'); diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 2a80ac4969427e..2ba70cde3f6b14 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39296,6 +39296,9 @@ msgstr "" msgid "There are several size limits in place." msgstr "" +msgid "There are unsubmitted review comments." +msgstr "" + msgid "There is already a repository with that name on disk" msgstr "" -- GitLab From 2821a0cca3b843ca9186fae804d97b630bbd8266 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 14 Jul 2022 02:52:20 -0600 Subject: [PATCH 02/10] Test that the Batch Comments app alerts interested parties to changes --- .../modules/batch_comments/actions_spec.js | 139 +++++++++++++----- 1 file changed, 104 insertions(+), 35 deletions(-) diff --git a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js index 9f50b12bac2448..084cbdecde00d3 100644 --- a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js +++ b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js @@ -3,19 +3,27 @@ import { TEST_HOST } from 'helpers/test_constants'; import testAction from 'helpers/vuex_action_helper'; import service from '~/batch_comments/services/drafts_service'; import * as actions from '~/batch_comments/stores/modules/batch_comments/actions'; +import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; +import BatchCommentsEventHub from '~/batch_comments/events'; import axios from '~/lib/utils/axios_utils'; describe('Batch comments store actions', () => { let res = {}; + let commit; + let emit; let mock; beforeEach(() => { mock = new MockAdapter(axios); + commit = jest.fn(); + emit = jest.spyOn(BatchCommentsEventHub, '$emit'); }); afterEach(() => { res = {}; mock.restore(); + commit.mockReset(); + emit.mockRestore(); }); describe('saveDraft', () => { @@ -29,17 +37,28 @@ describe('Batch comments store actions', () => { }); describe('addDraftToDiscussion', () => { - it('commits ADD_NEW_DRAFT if no errors returned', () => { - res = { id: 1 }; - mock.onAny().reply(200, res); + describe('if no errors returned', () => { + it('commits ADD_NEW_DRAFT', () => { + res = { id: 1 }; + mock.onAny().reply(200, res); + + return testAction( + actions.addDraftToDiscussion, + { endpoint: TEST_HOST, data: 'test' }, + null, + [{ type: 'ADD_NEW_DRAFT', payload: res }], + [], + ); + }); - return testAction( - actions.addDraftToDiscussion, - { endpoint: TEST_HOST, data: 'test' }, - null, - [{ type: 'ADD_NEW_DRAFT', payload: res }], - [], - ); + it('broadcasts that there are draft comments', async () => { + res = { id: 1 }; + mock.onAny().reply(200, res); + + await actions.addDraftToDiscussion({ commit }, { endpoint: TEST_HOST, data: 'test' }); + + expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, true); + }); }); it('does not commit ADD_NEW_DRAFT if errors returned', () => { @@ -56,17 +75,28 @@ describe('Batch comments store actions', () => { }); describe('createNewDraft', () => { - it('commits ADD_NEW_DRAFT if no errors returned', () => { - res = { id: 1 }; - mock.onAny().reply(200, res); + describe('if no errors returned', () => { + it('commits ADD_NEW_DRAFT if no errors returned', () => { + res = { id: 1 }; + mock.onAny().reply(200, res); + + return testAction( + actions.createNewDraft, + { endpoint: TEST_HOST, data: 'test' }, + null, + [{ type: 'ADD_NEW_DRAFT', payload: res }], + [], + ); + }); - return testAction( - actions.createNewDraft, - { endpoint: TEST_HOST, data: 'test' }, - null, - [{ type: 'ADD_NEW_DRAFT', payload: res }], - [], - ); + it('broadcasts that there are draft comments', async () => { + res = { id: 1 }; + mock.onAny().reply(200, res); + + await actions.createNewDraft({ commit }, { endpoint: TEST_HOST, data: 'test' }); + + expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, true); + }); }); it('does not commit ADD_NEW_DRAFT if errors returned', () => { @@ -93,22 +123,42 @@ describe('Batch comments store actions', () => { }; }); - it('commits DELETE_DRAFT if no errors returned', () => { - const commit = jest.fn(); - const context = { - getters, - commit, - }; - res = { id: 1 }; - mock.onAny().reply(200); + describe('if no errors returned', () => { + it('commits DELETE_DRAFT if no errors returned', () => { + const context = { + getters, + commit, + }; + res = { id: 1 }; + mock.onAny().reply(200); + + return actions.deleteDraft(context, { id: 1 }).then(() => { + expect(commit).toHaveBeenCalledWith('DELETE_DRAFT', 1); + }); + }); - return actions.deleteDraft(context, { id: 1 }).then(() => { - expect(commit).toHaveBeenCalledWith('DELETE_DRAFT', 1); + it.each` + description | drafts | broadcast + ${'if there are still drafts'} | ${[1, 2]} | ${true} + ${'if there are no more drafts'} | ${[]} | ${false} + `("broadcasts '$broadcast' $description", async ({ drafts, broadcast }) => { + const context = { + state: { + drafts, + }, + getters, + commit, + }; + res = { id: 1 }; + mock.onAny().reply(200); + + await actions.deleteDraft(context, { id: 1 }); + + expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, broadcast); }); }); it('does not commit DELETE_DRAFT if errors returned', () => { - const commit = jest.fn(); const context = { getters, commit, @@ -133,7 +183,6 @@ describe('Batch comments store actions', () => { }); it('commits SET_BATCH_COMMENTS_DRAFTS with returned data', () => { - const commit = jest.fn(); const dispatch = jest.fn(); const context = { getters, @@ -155,13 +204,11 @@ describe('Batch comments store actions', () => { describe('publishReview', () => { let dispatch; - let commit; let getters; let rootGetters; beforeEach(() => { dispatch = jest.fn(); - commit = jest.fn(); getters = { getNotesData: { draftsPublishPath: TEST_HOST, discussionsPath: TEST_HOST }, }; @@ -199,12 +246,34 @@ describe('Batch comments store actions', () => { }); }); + describe('updateDiscussionsAfterPublish', () => { + it.each` + description | drafts | broadcast + ${'if there are drafts'} | ${[1, 2]} | ${true} + ${'if there are no drafts'} | ${[]} | ${false} + `("broadcasts '$broadcast' $description", async ({ drafts, broadcast }) => { + const context = { + state: { drafts }, + dispatch: () => {}, + getters: { + getNotesData: () => ({ discussionsPath: '' }), + }, + rootGetters: { + discussionsStructuredByLineCode: {}, + }, + }; + + await actions.updateDiscussionsAfterPublish(context); + + expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, broadcast); + }); + }); + describe('updateDraft', () => { let getters; service.update = jest.fn(); service.update.mockResolvedValue({ data: { id: 1 } }); - const commit = jest.fn(); let context; let params; -- GitLab From d555baf1e98fa4307e4269ff98e1f56ed1291018 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 14 Jul 2022 02:52:45 -0600 Subject: [PATCH 03/10] Test that Diffs app responds to broadcasted changes in Batch Comments --- spec/frontend/diffs/components/app_spec.js | 73 ++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 96f2ac1692cc8c..542c7eccadac50 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -6,6 +6,11 @@ import Vue, { nextTick } from 'vue'; import Vuex from 'vuex'; import setWindowLocation from 'helpers/set_window_location_helper'; import { TEST_HOST } from 'spec/test_constants'; + +import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; +import BatchCommentsEventHub from '~/batch_comments/events'; + +import { STATE_IDLING, STATE_PENDING_REVIEW } from '~/diffs/constants'; import App from '~/diffs/components/app.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; @@ -702,4 +707,72 @@ describe('diffs/components/app', () => { ); }); }); + + describe('communicating with other apps', () => { + let aEL; + let rEL; + + beforeEach(() => { + aEL = jest.spyOn(window, 'addEventListener'); + rEL = jest.spyOn(window, 'removeEventListener'); + createComponent(); + }); + + afterEach(() => { + aEL.mockRestore(); + rEL.mockRestore(); + }); + + describe('batch comments (aka draft reviews)', () => { + describe('when batch comments broadcasts that there are pending comments', () => { + it('moves into the `pending review` state regardless of initial state', () => { + // If the FSM is just idling, it should move to the pending state + wrapper.vm.batchCommentsFSM.value = STATE_IDLING; + + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); + + expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_PENDING_REVIEW); + + // If the FSM is already in the pending state, it should still be there after the broadcast + wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; + + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); + + expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_PENDING_REVIEW); + }); + + it('attaches a listener to the `beforeunload` window event if it was previously idle (no pending comments)', () => { + wrapper.vm.batchCommentsFSM.value = STATE_IDLING; + + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); + + expect(aEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + }); + + it('does not attach an event listener to the `beforeunload` window event if the state was already pending comments', () => { + wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; + + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); + + expect(aEL).not.toBeCalled(); + }); + }); + + describe('when batch comments broadcasts that there are no pending comments', () => { + it('moves into the `idling` state', () => { + wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; + + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); + + expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_IDLING); + }); + + it('removes the listener on the `beforeunload` window event', () => { + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); + + expect(rEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + }); + }); + }); + }); }); -- GitLab From be8fe8836e531f41e07bc21af3553e0b8fc7d88b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 18 Jul 2022 19:25:48 -0600 Subject: [PATCH 04/10] Make RSpec accept the popup when leaving a pending comment --- spec/features/merge_request/batch_comments_spec.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index fafaea8ac6853f..f892b01e6247f3 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -101,7 +101,7 @@ write_diff_comment - visit_overview + visit_overview_with_pending_comment end it 'can add comment to review' do @@ -232,6 +232,14 @@ def visit_overview wait_for_requests end + def visit_overview_with_pending_comment + accept_alert do + visit project_merge_request_path(merge_request.project, merge_request) + end + + wait_for_requests + end + def write_diff_comment(**params) click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']")) -- GitLab From d304c282f9e1c01ddf9fbb26d189174be3a26a5c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 22 Jul 2022 22:36:19 -0600 Subject: [PATCH 05/10] Fix duplicated test description string --- .../stores/modules/batch_comments/actions_spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js index 084cbdecde00d3..441a15c42469ba 100644 --- a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js +++ b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js @@ -76,7 +76,7 @@ describe('Batch comments store actions', () => { describe('createNewDraft', () => { describe('if no errors returned', () => { - it('commits ADD_NEW_DRAFT if no errors returned', () => { + it('commits ADD_NEW_DRAFT', () => { res = { id: 1 }; mock.onAny().reply(200, res); @@ -124,7 +124,7 @@ describe('Batch comments store actions', () => { }); describe('if no errors returned', () => { - it('commits DELETE_DRAFT if no errors returned', () => { + it('commits DELETE_DRAFT', () => { const context = { getters, commit, -- GitLab From 7f0ce0fa4fa0d4cfbda9c4e82f1eec62d9ddbb1e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 22 Jul 2022 23:35:08 -0600 Subject: [PATCH 06/10] Replace anonymous blocking function with regular function `addEventListener` and `removeEventListener` always (correctly) treat anonymous functions as new code, so they can be duplicated (not a problem in our case) and the same anonymous function won't be removed by removeEventListener. --- .../javascripts/diffs/components/app.vue | 26 ++++++++++--------- 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 15f8915fd67c70..5e80dda280276c 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -62,6 +62,14 @@ import NoChanges from './no_changes.vue'; import TreeList from './tree_list.vue'; import VirtualScrollerScrollSync from './virtual_scroller_scroll_sync'; +function closeInterrupt(event) { + event.preventDefault(); + + // This is the correct way to write backwards-compatible beforeunload listeners + /* eslint-disable-next-line no-return-assign, no-param-reassign */ + return (event.returnValue = INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS); +} + export default { name: 'DiffsApp', components: { @@ -187,9 +195,6 @@ export default { default: false, }, }, - i18n: { - INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS, - }, data() { const treeWidth = parseInt(localStorage.getItem(TREE_LIST_WIDTH_STORAGE_KEY), 10) || INITIAL_TREE_WIDTH; @@ -644,13 +649,10 @@ export default { window.location.reload(); }, togglePendingComments(hasDraftComments) { - const closeInterrupt = (event) => { - event.preventDefault(); - - // This is the correct way to write backwards-compatible beforeunload listeners - /* eslint-disable-next-line no-return-assign, no-param-reassign */ - return (event.returnValue = this.$options.i18n.INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS); - }; + const block = () => + window.addEventListener('beforeunload', closeInterrupt, { capture: true }); + const unblock = () => + window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); /* * This stuff is a lot trickier than it looks. @@ -670,10 +672,10 @@ export default { */ if (hasDraftComments && this.batchCommentsFSM.is(STATE_IDLING)) { this.batchCommentsFSM.send(TRANSITION_HAS_PENDING_REVIEW); - window.addEventListener('beforeunload', closeInterrupt, { capture: true }); + block(); } else if (!hasDraftComments) { this.batchCommentsFSM.send(TRANSITION_NO_REVIEW); - window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); + unblock(); } }, }, -- GitLab From 5ba91968143d666fa7b09f721a06fadaa1d4bc53 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 26 Jul 2022 00:41:51 -0600 Subject: [PATCH 07/10] Use an inline expression to indicate whether there are drafts --- .../batch_comments/stores/modules/batch_comments/actions.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js index 49f893dacc401e..ebb2b9d53964ea 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js @@ -16,11 +16,7 @@ import BatchCommentsEventHub from '../../../events'; import * as types from './mutation_types'; function broadcastDraftsStatus(drafts) { - if (drafts.length) { - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); - } else { - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); - } + BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, drafts.length > 0); } export const saveDraft = ({ dispatch }, draft) => -- GitLab From 1648509c19f0f70b45b25ed49da5f4fa7e8c27be Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 26 Jul 2022 00:44:19 -0600 Subject: [PATCH 08/10] Add a link to the standard way to create robust beforeunload handlers --- app/assets/javascripts/diffs/components/app.vue | 1 + 1 file changed, 1 insertion(+) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 5e80dda280276c..debc06b8371d1f 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -66,6 +66,7 @@ function closeInterrupt(event) { event.preventDefault(); // This is the correct way to write backwards-compatible beforeunload listeners + // https://developer.chrome.com/blog/page-lifecycle-api/#the-beforeunload-event /* eslint-disable-next-line no-return-assign, no-param-reassign */ return (event.returnValue = INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS); } -- GitLab From 46a43ecc7d55af8dcb5b3c8967c9cf4d7a6caef2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 26 Jul 2022 01:31:33 -0600 Subject: [PATCH 09/10] Move beforeunload alert to the BatchComments app --- .../batch_comments/components/review_bar.vue | 28 ++++ .../javascripts/batch_comments/constants.js | 3 - app/assets/javascripts/batch_comments/i18n.js | 3 + .../stores/modules/batch_comments/actions.js | 25 +--- .../javascripts/diffs/components/app.vue | 71 --------- app/assets/javascripts/diffs/i18n.js | 2 - .../components/review_bar_spec.js | 47 +++++- .../modules/batch_comments/actions_spec.js | 139 +++++------------- spec/frontend/diffs/components/app_spec.js | 72 --------- 9 files changed, 108 insertions(+), 282 deletions(-) create mode 100644 app/assets/javascripts/batch_comments/i18n.js diff --git a/app/assets/javascripts/batch_comments/components/review_bar.vue b/app/assets/javascripts/batch_comments/components/review_bar.vue index 3cd1a2525e9a72..111b670596b98b 100644 --- a/app/assets/javascripts/batch_comments/components/review_bar.vue +++ b/app/assets/javascripts/batch_comments/components/review_bar.vue @@ -2,10 +2,20 @@ import { mapActions, mapGetters } from 'vuex'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { REVIEW_BAR_VISIBLE_CLASS_NAME } from '../constants'; +import { PREVENT_LEAVING_PENDING_REVIEW } from '../i18n'; import PreviewDropdown from './preview_dropdown.vue'; import PublishButton from './publish_button.vue'; import SubmitDropdown from './submit_dropdown.vue'; +function closeInterrupt(event) { + event.preventDefault(); + + // This is the correct way to write backwards-compatible beforeunload listeners + // https://developer.chrome.com/blog/page-lifecycle-api/#the-beforeunload-event + /* eslint-disable-next-line no-return-assign, no-param-reassign */ + return (event.returnValue = PREVENT_LEAVING_PENDING_REVIEW); +} + export default { components: { PreviewDropdown, @@ -25,8 +35,26 @@ export default { }, mounted() { document.body.classList.add(REVIEW_BAR_VISIBLE_CLASS_NAME); + /* + * This stuff is a lot trickier than it looks. + * + * Mandatory reading: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event + * Some notable sentences: + * - "[...] browsers may not display prompts created in beforeunload event handlers unless the + * page has been interacted with, or may even not display them at all." + * - "Especially on mobile, the beforeunload event is not reliably fired." + * - "The beforeunload event is not compatible with the back/forward cache (bfcache) [...] + * It is recommended that developers listen for beforeunload only in this scenario, and only + * when they actually have unsaved changes, so as to minimize the effect on performance." + * + * Please ensure that this is really not working before you modify it, because there are a LOT + * of scenarios where browser behavior will make it _seem_ like it's not working, but it actually + * is under the right combination of contexts. + */ + window.addEventListener('beforeunload', closeInterrupt, { capture: true }); }, beforeDestroy() { + window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); document.body.classList.remove(REVIEW_BAR_VISIBLE_CLASS_NAME); }, methods: { diff --git a/app/assets/javascripts/batch_comments/constants.js b/app/assets/javascripts/batch_comments/constants.js index f3854d5edb7602..5e026251e0b4f0 100644 --- a/app/assets/javascripts/batch_comments/constants.js +++ b/app/assets/javascripts/batch_comments/constants.js @@ -3,6 +3,3 @@ export const DISCUSSION_TAB = 'notes'; export const SHOW_TAB = 'show'; export const REVIEW_BAR_VISIBLE_CLASS_NAME = 'review-bar-visible'; - -// Known events for the Batch Comments app -export const EVT_BATCH_COMMENTS_UPDATED = 'app::code review::batch comments::pending review'; diff --git a/app/assets/javascripts/batch_comments/i18n.js b/app/assets/javascripts/batch_comments/i18n.js new file mode 100644 index 00000000000000..6cdbf00f9cadb3 --- /dev/null +++ b/app/assets/javascripts/batch_comments/i18n.js @@ -0,0 +1,3 @@ +import { __ } from '~/locale'; + +export const PREVENT_LEAVING_PENDING_REVIEW = __('There are unsubmitted review comments.'); diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js index ebb2b9d53964ea..863d2a99972214 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js @@ -4,21 +4,11 @@ import createFlash from '~/flash'; import { scrollToElement } from '~/lib/utils/common_utils'; import { __ } from '~/locale'; -import { - CHANGES_TAB, - DISCUSSION_TAB, - SHOW_TAB, - EVT_BATCH_COMMENTS_UPDATED, -} from '../../../constants'; +import { CHANGES_TAB, DISCUSSION_TAB, SHOW_TAB } from '../../../constants'; import service from '../../../services/drafts_service'; -import BatchCommentsEventHub from '../../../events'; import * as types from './mutation_types'; -function broadcastDraftsStatus(drafts) { - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, drafts.length > 0); -} - export const saveDraft = ({ dispatch }, draft) => dispatch('saveNote', { ...draft, isDraft: true }, { root: true }); @@ -29,8 +19,6 @@ export const addDraftToDiscussion = ({ commit }, { endpoint, data }) => .then((res) => { commit(types.ADD_NEW_DRAFT, res); - broadcastDraftsStatus([res]); - return res; }) .catch(() => { @@ -46,8 +34,6 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => .then((res) => { commit(types.ADD_NEW_DRAFT, res); - broadcastDraftsStatus([res]); - return res; }) .catch(() => { @@ -56,13 +42,11 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => }); }); -export const deleteDraft = ({ state, commit, getters }, draft) => +export const deleteDraft = ({ commit, getters }, draft) => service .deleteDraft(getters.getNotesData.draftsPath, draft.id) .then(() => { commit(types.DELETE_DRAFT, draft.id); - - broadcastDraftsStatus(state.drafts); }) .catch(() => createFlash({ @@ -76,7 +60,6 @@ export const fetchDrafts = ({ commit, getters, state, dispatch }) => .then((res) => res.data) .then((data) => commit(types.SET_BATCH_COMMENTS_DRAFTS, data)) .then(() => { - broadcastDraftsStatus(state.drafts); state.drafts.forEach((draft) => { if (!draft.line_code) { dispatch('convertToDiscussion', draft.discussion_id, { root: true }); @@ -109,15 +92,13 @@ export const publishReview = ({ commit, dispatch, getters }, noteData = {}) => { .catch(() => commit(types.RECEIVE_PUBLISH_REVIEW_ERROR)); }; -export const updateDiscussionsAfterPublish = async ({ state, dispatch, getters, rootGetters }) => { +export const updateDiscussionsAfterPublish = async ({ dispatch, getters, rootGetters }) => { await dispatch( 'fetchDiscussions', { path: getters.getNotesData.discussionsPath }, { root: true }, ); - broadcastDraftsStatus(state.drafts); - dispatch('diffs/assignDiscussionsToDiff', rootGetters.discussionsStructuredByLineCode, { root: true, }); diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index debc06b8371d1f..530f3a3a7f73f1 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -15,14 +15,11 @@ import createFlash from '~/flash'; import { isSingleViewStyle } from '~/helpers/diffs_helper'; import { helpPagePath } from '~/helpers/help_page_helper'; import { parseBoolean } from '~/lib/utils/common_utils'; -import { machine as fsm } from '~/lib/utils/finite_state_machine'; import { updateHistory } from '~/lib/utils/url_utility'; import { __ } from '~/locale'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; -import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; -import batchCommentsEventHub from '~/batch_comments/events'; import notesEventHub from '~/notes/event_hub'; import { TREE_LIST_WIDTH_STORAGE_KEY, @@ -42,12 +39,7 @@ import { TRACKING_WHITESPACE_HIDE, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, - STATE_IDLING, - STATE_PENDING_REVIEW, - TRANSITION_HAS_PENDING_REVIEW, - TRANSITION_NO_REVIEW, } from '../constants'; -import { INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS } from '../i18n'; import diffsEventHub from '../event_hub'; import { reviewStatuses } from '../utils/file_reviews'; @@ -62,15 +54,6 @@ import NoChanges from './no_changes.vue'; import TreeList from './tree_list.vue'; import VirtualScrollerScrollSync from './virtual_scroller_scroll_sync'; -function closeInterrupt(event) { - event.preventDefault(); - - // This is the correct way to write backwards-compatible beforeunload listeners - // https://developer.chrome.com/blog/page-lifecycle-api/#the-beforeunload-event - /* eslint-disable-next-line no-return-assign, no-param-reassign */ - return (event.returnValue = INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS); -} - export default { name: 'DiffsApp', components: { @@ -391,7 +374,6 @@ export default { queueRedisHllEvents(events, { verifyCap: true }); this.subscribeToVirtualScrollingEvents(); - this.subscribeToCrossAppCommunicationEvents(); }, beforeCreate() { diffsApp.instrument(); @@ -426,28 +408,11 @@ export default { } }, ); - - this.batchCommentsFSM = fsm({ - initial: STATE_IDLING, - states: { - [STATE_IDLING]: { - on: { - [TRANSITION_HAS_PENDING_REVIEW]: STATE_PENDING_REVIEW, - }, - }, - [STATE_PENDING_REVIEW]: { - on: { - [TRANSITION_NO_REVIEW]: STATE_IDLING, - }, - }, - }, - }); }, beforeDestroy() { diffsApp.deinstrument(); this.unsubscribeFromEvents(); this.removeEventListeners(); - this.unsubscribeFromCrossAppCommunicationEvents(); diffsEventHub.$off('scrollToFileHash', this.scrollVirtualScrollerToFileHash); diffsEventHub.$off('scrollToIndex', this.scrollVirtualScrollerToIndex); @@ -480,12 +445,6 @@ export default { notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); }, - subscribeToCrossAppCommunicationEvents() { - batchCommentsEventHub.$on(EVT_BATCH_COMMENTS_UPDATED, this.togglePendingComments); - }, - unsubscribeFromCrossAppCommunicationEvents() { - batchCommentsEventHub.$off(EVT_BATCH_COMMENTS_UPDATED, this.togglePendingComments); - }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, @@ -649,36 +608,6 @@ export default { reloadPage() { window.location.reload(); }, - togglePendingComments(hasDraftComments) { - const block = () => - window.addEventListener('beforeunload', closeInterrupt, { capture: true }); - const unblock = () => - window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); - - /* - * This stuff is a lot trickier than it looks. - * - * Mandatory reading: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event - * Some notable sentences: - * - "[...] browsers may not display prompts created in beforeunload event handlers unless the - * page has been interacted with, or may even not display them at all." - * - "Especially on mobile, the beforeunload event is not reliably fired." - * - "The beforeunload event is not compatible with the back/forward cache (bfcache) [...] - * It is recommended that developers listen for beforeunload only in this scenario, and only - * when they actually have unsaved changes, so as to minimize the effect on performance." - * - * Please ensure that this is really not working before you modify it, because there are a LOT - * of scenarios where browser behavior will make it _seem_ like it's not working, but it actually - * is under the right combination of contexts. - */ - if (hasDraftComments && this.batchCommentsFSM.is(STATE_IDLING)) { - this.batchCommentsFSM.send(TRANSITION_HAS_PENDING_REVIEW); - block(); - } else if (!hasDraftComments) { - this.batchCommentsFSM.send(TRANSITION_NO_REVIEW); - unblock(); - } - }, }, minTreeWidth: MIN_TREE_WIDTH, maxTreeWidth: window.innerWidth / 2, diff --git a/app/assets/javascripts/diffs/i18n.js b/app/assets/javascripts/diffs/i18n.js index 507d1d5d4750a4..e617890af2e3b6 100644 --- a/app/assets/javascripts/diffs/i18n.js +++ b/app/assets/javascripts/diffs/i18n.js @@ -47,5 +47,3 @@ export const CONFLICT_TEXT = { 'Conflict: This file was added both in the source and target branches, but with different contents.', ), }; - -export const INTERRUPT_CLOSE_PENDING_BATCH_COMMENTS = __('There are unsubmitted review comments.'); diff --git a/spec/frontend/batch_comments/components/review_bar_spec.js b/spec/frontend/batch_comments/components/review_bar_spec.js index f50db6ab210bb8..8b0a3f851b70a9 100644 --- a/spec/frontend/batch_comments/components/review_bar_spec.js +++ b/spec/frontend/batch_comments/components/review_bar_spec.js @@ -6,6 +6,8 @@ import createStore from '../create_batch_comments_store'; describe('Batch comments review bar component', () => { let store; let wrapper; + let aEL; + let rEL; const createComponent = (propsData = {}) => { store = createStore(); @@ -18,25 +20,54 @@ describe('Batch comments review bar component', () => { beforeEach(() => { document.body.className = ''; + + aEL = jest.spyOn(window, 'addEventListener'); + rEL = jest.spyOn(window, 'removeEventListener'); }); afterEach(() => { + aEL.mockRestore(); + rEL.mockRestore(); wrapper.destroy(); }); - it('it adds review-bar-visible class to body when review bar is mounted', async () => { - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + describe('when mounted', () => { + it('it adds review-bar-visible class to body', async () => { + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + + createComponent(); + + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + }); - createComponent(); + it('it adds a blocking handler to the `beforeunload` window event', () => { + expect(aEL).not.toBeCalled(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + createComponent(); + + expect(aEL).toHaveBeenCalledTimes(1); + expect(aEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + }); }); - it('it removes review-bar-visible class to body when review bar is destroyed', async () => { - createComponent(); + describe('before destroyed', () => { + it('it removes review-bar-visible class to body', async () => { + createComponent(); - wrapper.destroy(); + wrapper.destroy(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + }); + + it('it removes the blocking handler from the `beforeunload` window event', () => { + createComponent(); + + expect(rEL).not.toBeCalled(); + + wrapper.destroy(); + + expect(rEL).toHaveBeenCalledTimes(1); + expect(rEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + }); }); }); diff --git a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js index 441a15c42469ba..9f50b12bac2448 100644 --- a/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js +++ b/spec/frontend/batch_comments/stores/modules/batch_comments/actions_spec.js @@ -3,27 +3,19 @@ import { TEST_HOST } from 'helpers/test_constants'; import testAction from 'helpers/vuex_action_helper'; import service from '~/batch_comments/services/drafts_service'; import * as actions from '~/batch_comments/stores/modules/batch_comments/actions'; -import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; -import BatchCommentsEventHub from '~/batch_comments/events'; import axios from '~/lib/utils/axios_utils'; describe('Batch comments store actions', () => { let res = {}; - let commit; - let emit; let mock; beforeEach(() => { mock = new MockAdapter(axios); - commit = jest.fn(); - emit = jest.spyOn(BatchCommentsEventHub, '$emit'); }); afterEach(() => { res = {}; mock.restore(); - commit.mockReset(); - emit.mockRestore(); }); describe('saveDraft', () => { @@ -37,28 +29,17 @@ describe('Batch comments store actions', () => { }); describe('addDraftToDiscussion', () => { - describe('if no errors returned', () => { - it('commits ADD_NEW_DRAFT', () => { - res = { id: 1 }; - mock.onAny().reply(200, res); - - return testAction( - actions.addDraftToDiscussion, - { endpoint: TEST_HOST, data: 'test' }, - null, - [{ type: 'ADD_NEW_DRAFT', payload: res }], - [], - ); - }); - - it('broadcasts that there are draft comments', async () => { - res = { id: 1 }; - mock.onAny().reply(200, res); - - await actions.addDraftToDiscussion({ commit }, { endpoint: TEST_HOST, data: 'test' }); + it('commits ADD_NEW_DRAFT if no errors returned', () => { + res = { id: 1 }; + mock.onAny().reply(200, res); - expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, true); - }); + return testAction( + actions.addDraftToDiscussion, + { endpoint: TEST_HOST, data: 'test' }, + null, + [{ type: 'ADD_NEW_DRAFT', payload: res }], + [], + ); }); it('does not commit ADD_NEW_DRAFT if errors returned', () => { @@ -75,28 +56,17 @@ describe('Batch comments store actions', () => { }); describe('createNewDraft', () => { - describe('if no errors returned', () => { - it('commits ADD_NEW_DRAFT', () => { - res = { id: 1 }; - mock.onAny().reply(200, res); - - return testAction( - actions.createNewDraft, - { endpoint: TEST_HOST, data: 'test' }, - null, - [{ type: 'ADD_NEW_DRAFT', payload: res }], - [], - ); - }); - - it('broadcasts that there are draft comments', async () => { - res = { id: 1 }; - mock.onAny().reply(200, res); - - await actions.createNewDraft({ commit }, { endpoint: TEST_HOST, data: 'test' }); + it('commits ADD_NEW_DRAFT if no errors returned', () => { + res = { id: 1 }; + mock.onAny().reply(200, res); - expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, true); - }); + return testAction( + actions.createNewDraft, + { endpoint: TEST_HOST, data: 'test' }, + null, + [{ type: 'ADD_NEW_DRAFT', payload: res }], + [], + ); }); it('does not commit ADD_NEW_DRAFT if errors returned', () => { @@ -123,42 +93,22 @@ describe('Batch comments store actions', () => { }; }); - describe('if no errors returned', () => { - it('commits DELETE_DRAFT', () => { - const context = { - getters, - commit, - }; - res = { id: 1 }; - mock.onAny().reply(200); - - return actions.deleteDraft(context, { id: 1 }).then(() => { - expect(commit).toHaveBeenCalledWith('DELETE_DRAFT', 1); - }); - }); - - it.each` - description | drafts | broadcast - ${'if there are still drafts'} | ${[1, 2]} | ${true} - ${'if there are no more drafts'} | ${[]} | ${false} - `("broadcasts '$broadcast' $description", async ({ drafts, broadcast }) => { - const context = { - state: { - drafts, - }, - getters, - commit, - }; - res = { id: 1 }; - mock.onAny().reply(200); - - await actions.deleteDraft(context, { id: 1 }); + it('commits DELETE_DRAFT if no errors returned', () => { + const commit = jest.fn(); + const context = { + getters, + commit, + }; + res = { id: 1 }; + mock.onAny().reply(200); - expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, broadcast); + return actions.deleteDraft(context, { id: 1 }).then(() => { + expect(commit).toHaveBeenCalledWith('DELETE_DRAFT', 1); }); }); it('does not commit DELETE_DRAFT if errors returned', () => { + const commit = jest.fn(); const context = { getters, commit, @@ -183,6 +133,7 @@ describe('Batch comments store actions', () => { }); it('commits SET_BATCH_COMMENTS_DRAFTS with returned data', () => { + const commit = jest.fn(); const dispatch = jest.fn(); const context = { getters, @@ -204,11 +155,13 @@ describe('Batch comments store actions', () => { describe('publishReview', () => { let dispatch; + let commit; let getters; let rootGetters; beforeEach(() => { dispatch = jest.fn(); + commit = jest.fn(); getters = { getNotesData: { draftsPublishPath: TEST_HOST, discussionsPath: TEST_HOST }, }; @@ -246,34 +199,12 @@ describe('Batch comments store actions', () => { }); }); - describe('updateDiscussionsAfterPublish', () => { - it.each` - description | drafts | broadcast - ${'if there are drafts'} | ${[1, 2]} | ${true} - ${'if there are no drafts'} | ${[]} | ${false} - `("broadcasts '$broadcast' $description", async ({ drafts, broadcast }) => { - const context = { - state: { drafts }, - dispatch: () => {}, - getters: { - getNotesData: () => ({ discussionsPath: '' }), - }, - rootGetters: { - discussionsStructuredByLineCode: {}, - }, - }; - - await actions.updateDiscussionsAfterPublish(context); - - expect(emit).toHaveBeenCalledWith(EVT_BATCH_COMMENTS_UPDATED, broadcast); - }); - }); - describe('updateDraft', () => { let getters; service.update = jest.fn(); service.update.mockResolvedValue({ data: { id: 1 } }); + const commit = jest.fn(); let context; let params; diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 542c7eccadac50..cafa12e10b1d90 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -7,10 +7,6 @@ import Vuex from 'vuex'; import setWindowLocation from 'helpers/set_window_location_helper'; import { TEST_HOST } from 'spec/test_constants'; -import { EVT_BATCH_COMMENTS_UPDATED } from '~/batch_comments/constants'; -import BatchCommentsEventHub from '~/batch_comments/events'; - -import { STATE_IDLING, STATE_PENDING_REVIEW } from '~/diffs/constants'; import App from '~/diffs/components/app.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; @@ -707,72 +703,4 @@ describe('diffs/components/app', () => { ); }); }); - - describe('communicating with other apps', () => { - let aEL; - let rEL; - - beforeEach(() => { - aEL = jest.spyOn(window, 'addEventListener'); - rEL = jest.spyOn(window, 'removeEventListener'); - createComponent(); - }); - - afterEach(() => { - aEL.mockRestore(); - rEL.mockRestore(); - }); - - describe('batch comments (aka draft reviews)', () => { - describe('when batch comments broadcasts that there are pending comments', () => { - it('moves into the `pending review` state regardless of initial state', () => { - // If the FSM is just idling, it should move to the pending state - wrapper.vm.batchCommentsFSM.value = STATE_IDLING; - - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); - - expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_PENDING_REVIEW); - - // If the FSM is already in the pending state, it should still be there after the broadcast - wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; - - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); - - expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_PENDING_REVIEW); - }); - - it('attaches a listener to the `beforeunload` window event if it was previously idle (no pending comments)', () => { - wrapper.vm.batchCommentsFSM.value = STATE_IDLING; - - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); - - expect(aEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); - }); - - it('does not attach an event listener to the `beforeunload` window event if the state was already pending comments', () => { - wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; - - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, true); - - expect(aEL).not.toBeCalled(); - }); - }); - - describe('when batch comments broadcasts that there are no pending comments', () => { - it('moves into the `idling` state', () => { - wrapper.vm.batchCommentsFSM.value = STATE_PENDING_REVIEW; - - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); - - expect(wrapper.vm.batchCommentsFSM.value).toBe(STATE_IDLING); - }); - - it('removes the listener on the `beforeunload` window event', () => { - BatchCommentsEventHub.$emit(EVT_BATCH_COMMENTS_UPDATED, false); - - expect(rEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); - }); - }); - }); - }); }); -- GitLab From 5848fc6aa72e301aedcf5236be813ca347c40033 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 26 Jul 2022 10:53:27 -0600 Subject: [PATCH 10/10] Review cleanup - "revert" added blank line to not touch a file - Remove unused event hub factory - Expand variable names for better readability --- .../javascripts/batch_comments/events.js | 3 -- .../components/review_bar_spec.js | 28 +++++++++++-------- spec/frontend/diffs/components/app_spec.js | 1 - 3 files changed, 16 insertions(+), 16 deletions(-) delete mode 100644 app/assets/javascripts/batch_comments/events.js diff --git a/app/assets/javascripts/batch_comments/events.js b/app/assets/javascripts/batch_comments/events.js deleted file mode 100644 index 3e0c313f5e85f8..00000000000000 --- a/app/assets/javascripts/batch_comments/events.js +++ /dev/null @@ -1,3 +0,0 @@ -import eventHubFactory from '~/helpers/event_hub_factory'; - -export default eventHubFactory(); diff --git a/spec/frontend/batch_comments/components/review_bar_spec.js b/spec/frontend/batch_comments/components/review_bar_spec.js index 8b0a3f851b70a9..f98e0a4c64a35e 100644 --- a/spec/frontend/batch_comments/components/review_bar_spec.js +++ b/spec/frontend/batch_comments/components/review_bar_spec.js @@ -6,8 +6,8 @@ import createStore from '../create_batch_comments_store'; describe('Batch comments review bar component', () => { let store; let wrapper; - let aEL; - let rEL; + let addEventListenerSpy; + let removeEventListenerSpy; const createComponent = (propsData = {}) => { store = createStore(); @@ -21,13 +21,13 @@ describe('Batch comments review bar component', () => { beforeEach(() => { document.body.className = ''; - aEL = jest.spyOn(window, 'addEventListener'); - rEL = jest.spyOn(window, 'removeEventListener'); + addEventListenerSpy = jest.spyOn(window, 'addEventListener'); + removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); }); afterEach(() => { - aEL.mockRestore(); - rEL.mockRestore(); + addEventListenerSpy.mockRestore(); + removeEventListenerSpy.mockRestore(); wrapper.destroy(); }); @@ -41,12 +41,14 @@ describe('Batch comments review bar component', () => { }); it('it adds a blocking handler to the `beforeunload` window event', () => { - expect(aEL).not.toBeCalled(); + expect(addEventListenerSpy).not.toBeCalled(); createComponent(); - expect(aEL).toHaveBeenCalledTimes(1); - expect(aEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + expect(addEventListenerSpy).toHaveBeenCalledTimes(1); + expect(addEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); }); }); @@ -62,12 +64,14 @@ describe('Batch comments review bar component', () => { it('it removes the blocking handler from the `beforeunload` window event', () => { createComponent(); - expect(rEL).not.toBeCalled(); + expect(removeEventListenerSpy).not.toBeCalled(); wrapper.destroy(); - expect(rEL).toHaveBeenCalledTimes(1); - expect(rEL).toBeCalledWith('beforeunload', expect.any(Function), { capture: true }); + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); }); }); }); diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index cafa12e10b1d90..96f2ac1692cc8c 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -6,7 +6,6 @@ import Vue, { nextTick } from 'vue'; import Vuex from 'vuex'; import setWindowLocation from 'helpers/set_window_location_helper'; import { TEST_HOST } from 'spec/test_constants'; - import App from '~/diffs/components/app.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue'; import CompareVersions from '~/diffs/components/compare_versions.vue'; -- GitLab