diff --git a/app/assets/javascripts/code_review/signals.js b/app/assets/javascripts/code_review/signals.js new file mode 100644 index 0000000000000000000000000000000000000000..101b7996bb5a4539c07f3f4c540de28c11ea3800 --- /dev/null +++ b/app/assets/javascripts/code_review/signals.js @@ -0,0 +1,51 @@ +import createApolloClient from '../lib/graphql'; + +import { getDerivedMergeRequestInformation } from '../diffs/utils/merge_request'; +import { EVT_MR_PREPARED } from '../diffs/constants'; + +import getMr from '../graphql_shared/queries/merge_request.query.graphql'; +import mrPreparation from '../graphql_shared/subscriptions/merge_request_prepared.subscription.graphql'; + +function required(name) { + throw new Error(`${name} is a required argument`); +} + +async function observeMergeRequestFinishingPreparation({ apollo, signaler }) { + const { namespace, project, id: iid } = getDerivedMergeRequestInformation({ + endpoint: document.location.pathname, + }); + const projectPath = `${namespace}/${project}`; + + if (projectPath && iid) { + const currentStatus = await apollo.query({ + query: getMr, + variables: { projectPath, iid }, + }); + const { id: gqlMrId, preparedAt } = currentStatus.data.project.mergeRequest; + let preparationObservable; + let preparationSubscriber; + + if (!preparedAt) { + preparationObservable = apollo.subscribe({ + query: mrPreparation, + variables: { + issuableId: gqlMrId, + }, + }); + + preparationSubscriber = preparationObservable.subscribe((preparationUpdate) => { + if (preparationUpdate.data.mergeRequestMergeStatusUpdated?.preparedAt) { + signaler.$emit(EVT_MR_PREPARED); + preparationSubscriber.unsubscribe(); + } + }); + } + } +} + +export async function start({ + signalBus = required('signalBus'), + apolloClient = createApolloClient(), +} = {}) { + await observeMergeRequestFinishingPreparation({ signaler: signalBus, apollo: apolloClient }); +} diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index bfabac3123b7d88519f70e660838acc43b9b5c85..02307150e2f52c1c4a4b90c873fbfe15a8429d9e 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -41,6 +41,7 @@ import { TRACKING_WHITESPACE_HIDE, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, + EVT_MR_PREPARED, } from '../constants'; import diffsEventHub from '../event_hub'; @@ -470,8 +471,10 @@ export default { diffsEventHub.$on('diffFilesModified', this.setDiscussions); notesEventHub.$on('fetchedNotesData', this.rereadNoteHash); } + diffsEventHub.$on(EVT_MR_PREPARED, this.fetchData); }, unsubscribeFromEvents() { + diffsEventHub.$off(EVT_MR_PREPARED, this.fetchData); if (this.glFeatures.singleFileFileByFile) { notesEventHub.$off('fetchedNotesData', this.rereadNoteHash); diffsEventHub.$off('diffFilesModified', this.setDiscussions); diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index a459def6b4b9f05eb8e199f8a2c99544c4b319b8..063e36fa7fb30a6eada91307006cd2a9ab9dba5f 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -79,6 +79,7 @@ export const RENAMED_DIFF_TRANSITIONS = { }; // MR Diffs known events +export const EVT_MR_PREPARED = 'mr:asyncPreparationFinished'; export const EVT_EXPAND_ALL_FILES = 'mr:diffs:expandAllFiles'; export const EVT_PERF_MARK_FILE_TREE_START = 'mr:diffs:perf:fileTreeStart'; export const EVT_PERF_MARK_FILE_TREE_END = 'mr:diffs:perf:fileTreeEnd'; diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 2a7b4da684b43a14797996fd86b70e6c385453fa..0668551902a7b9e29a89c2049c055d8c656b2807 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -49,6 +49,7 @@ import { TRACKING_CLICK_SINGLE_FILE_SETTING, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, + EVT_MR_PREPARED, } from '../constants'; import { DISCUSSION_SINGLE_DIFF_FAILED, LOAD_SINGLE_DIFF_FAILED } from '../i18n'; import eventHub from '../event_hub'; @@ -287,10 +288,14 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { }) .catch((error) => { if (error.response.status === HTTP_STATUS_NOT_FOUND) { - createAlert({ - message: __('Building your merge request. Wait a few moments, then refresh this page.'), + const alert = createAlert({ + message: __( + 'Building your merge request… This page will update when the build is complete.', + ), variant: VARIANT_WARNING, }); + + eventHub.$once(EVT_MR_PREPARED, () => alert.dismiss()); } else { throw error; } diff --git a/app/assets/javascripts/graphql_shared/queries/merge_request.query.graphql b/app/assets/javascripts/graphql_shared/queries/merge_request.query.graphql new file mode 100644 index 0000000000000000000000000000000000000000..a6ef593516297bb027646b46b762fa2f2a375a25 --- /dev/null +++ b/app/assets/javascripts/graphql_shared/queries/merge_request.query.graphql @@ -0,0 +1,9 @@ +query mergeRequestId($projectPath: ID!, $iid: String!) { + project(fullPath: $projectPath) { + id + mergeRequest(iid: $iid) { + id + preparedAt + } + } +} diff --git a/app/assets/javascripts/graphql_shared/subscriptions/merge_request_prepared.subscription.graphql b/app/assets/javascripts/graphql_shared/subscriptions/merge_request_prepared.subscription.graphql new file mode 100644 index 0000000000000000000000000000000000000000..ba658f56ebd6103f5fac5534393d9541456d484a --- /dev/null +++ b/app/assets/javascripts/graphql_shared/subscriptions/merge_request_prepared.subscription.graphql @@ -0,0 +1,8 @@ +subscription mergeRequestPrepared($issuableId: IssuableID!) { + mergeRequestMergeStatusUpdated(issuableId: $issuableId) { + ... on MergeRequest { + id + preparedAt + } + } +} diff --git a/app/assets/javascripts/pages/projects/merge_requests/page.js b/app/assets/javascripts/pages/projects/merge_requests/page.js index fbd45f4bd7d3f964b4b452e73e3491a1adeb662d..552e75da9b8f85ffc44e86bd9d0289ba26b87803 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/page.js +++ b/app/assets/javascripts/pages/projects/merge_requests/page.js @@ -3,6 +3,8 @@ import VueApollo from 'vue-apollo'; import initMrNotes from 'ee_else_ce/mr_notes'; import StickyHeader from '~/merge_requests/components/sticky_header.vue'; import { initIssuableHeaderWarnings } from '~/issuable'; +import { start as startCodeReviewMessaging } from '~/code_review/signals'; +import diffsEventHub from '~/diffs/event_hub'; import store from '~/mr_notes/stores'; import initSidebarBundle from '~/sidebar/sidebar_bundle'; import { apolloProvider } from '~/graphql_shared/issuable_client'; @@ -15,6 +17,7 @@ Vue.use(VueApollo); export function initMrPage() { initMrNotes(); initShow(); + startCodeReviewMessaging({ signalBus: diffsEventHub }); } requestIdleCallback(() => { diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8f934560f07fc5abbea82e6b5de772b82ac3101e..118b8a4d9fc307613238892adf91e7c8acf99ee7 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8167,7 +8167,7 @@ msgstr "" msgid "BuildArtifacts|Loading artifacts" msgstr "" -msgid "Building your merge request. Wait a few moments, then refresh this page." +msgid "Building your merge request… This page will update when the build is complete." msgstr "" msgid "Built-in" diff --git a/spec/frontend/code_review/signals_spec.js b/spec/frontend/code_review/signals_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..03c3580860eed54adb07291427eeb03670967bcd --- /dev/null +++ b/spec/frontend/code_review/signals_spec.js @@ -0,0 +1,145 @@ +import { start } from '~/code_review/signals'; + +import diffsEventHub from '~/diffs/event_hub'; +import { EVT_MR_PREPARED } from '~/diffs/constants'; +import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request'; + +jest.mock('~/diffs/utils/merge_request'); + +describe('~/code_review', () => { + const io = diffsEventHub; + + beforeAll(() => { + getDerivedMergeRequestInformation.mockImplementation(() => ({ + namespace: 'x', + project: 'y', + id: '1', + })); + }); + + describe('start', () => { + it.each` + description | argument + ${'no event hub is provided'} | ${{}} + ${'no parameters are provided'} | ${undefined} + `('throws an error if $description', async ({ argument }) => { + await expect(() => start(argument)).rejects.toThrow('signalBus is a required argument'); + }); + + describe('observeMergeRequestFinishingPreparation', () => { + const callArgs = {}; + const apollo = {}; + let querySpy; + let apolloSubscribeSpy; + let subscribeSpy; + let nextSpy; + let unsubscribeSpy; + let observable; + + beforeEach(() => { + querySpy = jest.fn(); + apolloSubscribeSpy = jest.fn(); + subscribeSpy = jest.fn(); + unsubscribeSpy = jest.fn(); + nextSpy = jest.fn(); + observable = { + next: nextSpy, + subscribe: subscribeSpy.mockReturnValue({ + unsubscribe: unsubscribeSpy, + }), + }; + + querySpy.mockResolvedValue({ + data: { project: { mergeRequest: { id: 'gql:id:1', preparedAt: 'x' } } }, + }); + apolloSubscribeSpy.mockReturnValue(observable); + + apollo.query = querySpy; + apollo.subscribe = apolloSubscribeSpy; + + callArgs.signalBus = io; + callArgs.apolloClient = apollo; + }); + + it('does not query at all if the page does not seem like a merge request', async () => { + getDerivedMergeRequestInformation.mockImplementationOnce(() => ({})); + + await start(callArgs); + + expect(querySpy).not.toHaveBeenCalled(); + expect(apolloSubscribeSpy).not.toHaveBeenCalled(); + }); + + describe('on a merge request page', () => { + it('requests the preparedAt (and id) for the current merge request', async () => { + await start(callArgs); + + expect(querySpy).toHaveBeenCalledWith( + expect.objectContaining({ + variables: { + projectPath: 'x/y', + iid: '1', + }, + }), + ); + }); + + it('does not subscribe to any updates if the preparedAt value is already populated', async () => { + await start(callArgs); + + expect(apolloSubscribeSpy).not.toHaveBeenCalled(); + }); + + describe('if the merge request is still asynchronously preparing', () => { + beforeEach(() => { + querySpy.mockResolvedValue({ + data: { project: { mergeRequest: { id: 'gql:id:1', preparedAt: null } } }, + }); + }); + + it('subscribes to updates', async () => { + await start(callArgs); + + expect(apolloSubscribeSpy).toHaveBeenCalledWith( + expect.objectContaining({ variables: { issuableId: 'gql:id:1' } }), + ); + expect(observable.subscribe).toHaveBeenCalled(); + }); + + describe('when the MR has been updated', () => { + let emitSpy; + let behavior; + + beforeEach(() => { + emitSpy = jest.spyOn(diffsEventHub, '$emit'); + nextSpy.mockImplementation((data) => behavior?.(data)); + subscribeSpy.mockImplementation((handler) => { + behavior = handler; + + return { unsubscribe: unsubscribeSpy }; + }); + }); + + it('does nothing if the MR has not yet finished preparing', async () => { + await start(callArgs); + + observable.next({ data: { mergeRequestMergeStatusUpdated: { preparedAt: null } } }); + + expect(unsubscribeSpy).not.toHaveBeenCalled(); + expect(emitSpy).not.toHaveBeenCalled(); + }); + + it('emits an event and unsubscribes when the MR is prepared', async () => { + await start(callArgs); + + observable.next({ data: { mergeRequestMergeStatusUpdated: { preparedAt: 'x' } } }); + + expect(unsubscribeSpy).toHaveBeenCalled(); + expect(emitSpy).toHaveBeenCalledWith(EVT_MR_PREPARED); + }); + }); + }); + }); + }); + }); +}); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 7f9cd1a274d398a9dc656d33d5017072a911f5a8..f883aea764fd203e6d45b6fe17f5b5e01656fdf0 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -9,6 +9,7 @@ import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE, + EVT_MR_PREPARED, } from '~/diffs/constants'; import { LOAD_SINGLE_DIFF_FAILED } from '~/diffs/i18n'; import * as diffActions from '~/diffs/store/actions'; @@ -396,23 +397,46 @@ describe('DiffsStoreActions', () => { ); }); - it('should show a warning on 404 reponse', async () => { - mock.onGet(endpointMetadata).reply(HTTP_STATUS_NOT_FOUND); + describe('on a 404 response', () => { + let dismissAlert; - await testAction( - diffActions.fetchDiffFilesMeta, - {}, - { endpointMetadata, diffViewType: 'inline', showWhitespace: true }, - [{ type: types.SET_LOADING, payload: true }], - [], - ); + beforeAll(() => { + dismissAlert = jest.fn(); - expect(createAlert).toHaveBeenCalledTimes(1); - expect(createAlert).toHaveBeenCalledWith({ - message: expect.stringMatching( - 'Building your merge request. Wait a few moments, then refresh this page.', - ), - variant: 'warning', + mock.onGet(endpointMetadata).reply(HTTP_STATUS_NOT_FOUND); + createAlert.mockImplementation(() => ({ dismiss: dismissAlert })); + }); + + it('should show a warning', async () => { + await testAction( + diffActions.fetchDiffFilesMeta, + {}, + { endpointMetadata, diffViewType: 'inline', showWhitespace: true }, + [{ type: types.SET_LOADING, payload: true }], + [], + ); + + expect(createAlert).toHaveBeenCalledTimes(1); + expect(createAlert).toHaveBeenCalledWith({ + message: expect.stringMatching( + 'Building your merge request… This page will update when the build is complete.', + ), + variant: 'warning', + }); + }); + + it("should attempt to close the alert if the MR reports that it's been prepared", async () => { + await testAction( + diffActions.fetchDiffFilesMeta, + {}, + { endpointMetadata, diffViewType: 'inline', showWhitespace: true }, + [{ type: types.SET_LOADING, payload: true }], + [], + ); + + diffsEventHub.$emit(EVT_MR_PREPARED); + + expect(dismissAlert).toHaveBeenCalled(); }); });