diff --git a/app/assets/javascripts/code_review/constants.js b/app/assets/javascripts/code_review/constants.js new file mode 100644 index 0000000000000000000000000000000000000000..d66dcdbeec97ef4358f797900afe063d5b728749 --- /dev/null +++ b/app/assets/javascripts/code_review/constants.js @@ -0,0 +1,2 @@ +// known events +export const EVT_MR_PREPARED = 'mr:asyncPreparationFinished'; diff --git a/app/assets/javascripts/code_review/event_hub.js b/app/assets/javascripts/code_review/event_hub.js new file mode 100644 index 0000000000000000000000000000000000000000..3e0c313f5e85f8e1c534e226917deeb43e851aaf --- /dev/null +++ b/app/assets/javascripts/code_review/event_hub.js @@ -0,0 +1,3 @@ +import eventHubFactory from '~/helpers/event_hub_factory'; + +export default eventHubFactory(); diff --git a/app/assets/javascripts/code_review/signals.js b/app/assets/javascripts/code_review/signals.js index 080879f4e1dc12429809a7004ae7cba86b0ef59d..eb0177eca2b29ec7926710e8a4a65e43d4ea7301 100644 --- a/app/assets/javascripts/code_review/signals.js +++ b/app/assets/javascripts/code_review/signals.js @@ -1,11 +1,11 @@ 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'; +import { EVT_MR_PREPARED } from './constants'; +import { getDerivedMergeRequestInformation } from './utils/merge_request'; + function required(name) { throw new Error(`${name} is a required argument`); } diff --git a/app/assets/javascripts/code_review/utils/merge_request.js b/app/assets/javascripts/code_review/utils/merge_request.js new file mode 100644 index 0000000000000000000000000000000000000000..f1768071cce94bf44aba7741c7008ab20b65c11f --- /dev/null +++ b/app/assets/javascripts/code_review/utils/merge_request.js @@ -0,0 +1,40 @@ +const endpointRE = /^(\/?(.+\/)+(.+)\/-\/merge_requests\/(\d+)).*$/i; + +function getVersionInfo({ endpoint } = {}) { + const dummyRoot = 'https://gitlab.com'; + const endpointUrl = new URL(endpoint, dummyRoot); + const params = Object.fromEntries(endpointUrl.searchParams.entries()); + + const { start_sha: startSha, diff_id: diffId } = params; + + return { + diffId, + startSha, + }; +} + +export function getDerivedMergeRequestInformation({ endpoint } = {}) { + let mrPath; + let namespace; + let project; + let id; + let diffId; + let startSha; + const matches = endpointRE.exec(endpoint); + + if (matches) { + [, mrPath, namespace, project, id] = matches; + ({ diffId, startSha } = getVersionInfo({ endpoint })); + + namespace = namespace.replace(/\/$/, ''); + } + + return { + mrPath, + namespace, + project, + id, + diffId, + startSha, + }; +} diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index c2fd1249ddee8116af048f9752f96659603084d6..fe0b1eca0caf7c2bfed2f0b820364b73fd151f87 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -22,6 +22,7 @@ import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import notesEventHub from '~/notes/event_hub'; import { DynamicScroller, DynamicScrollerItem } from 'vendor/vue-virtual-scroller'; +import { EVT_MR_PREPARED } from '~/code_review/constants'; import { MR_TREE_SHOW_KEY, ALERT_OVERFLOW_HIDDEN, @@ -36,7 +37,6 @@ import { TRACKING_WHITESPACE_HIDE, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, - EVT_MR_PREPARED, } from '../constants'; import diffsEventHub from '../event_hub'; diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 575cd05ceb898f74ef1ee91f26bfaa60d9e36b8e..4dd94a23d285b3f00d56b6ad17e7580427fad508 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -80,7 +80,6 @@ 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 bbc602aedf615d5070ccfff5cc63b6033cc0491f..b37fcd34beb0fa052bb5966384af4f5e8f016dbc 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -15,6 +15,8 @@ import notesEventHub from '~/notes/event_hub'; import { generateTreeList } from '~/diffs/utils/tree_worker_utils'; import { sortTree } from '~/ide/stores/utils'; import { containsSensitiveToken, confirmSensitiveAction } from '~/lib/utils/secret_detection'; +import { getDerivedMergeRequestInformation } from '~/code_review/utils/merge_request'; +import { EVT_MR_PREPARED } from '~/code_review/constants'; import { PARALLEL_DIFF_VIEW_TYPE, INLINE_DIFF_VIEW_TYPE, @@ -47,7 +49,6 @@ import { TRACKING_CLICK_SINGLE_FILE_SETTING, TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, - EVT_MR_PREPARED, FILE_DIFF_POSITION_TYPE, } from '../constants'; import { @@ -60,7 +61,6 @@ import { } from '../i18n'; import eventHub from '../event_hub'; import { markFileReview, setReviewsForMergeRequest } from '../utils/file_reviews'; -import { getDerivedMergeRequestInformation } from '../utils/merge_request'; import { queueRedisHllEvents } from '../utils/queue_events'; import * as types from './mutation_types'; import { diff --git a/app/assets/javascripts/diffs/utils/diff_file.js b/app/assets/javascripts/diffs/utils/diff_file.js index 98e1c1cc8493d6429cea7aec253557e6ae33aa11..695d89bbb9cb2fbd0d0c994c3b9fb187c60174e6 100644 --- a/app/assets/javascripts/diffs/utils/diff_file.js +++ b/app/assets/javascripts/diffs/utils/diff_file.js @@ -1,4 +1,5 @@ import { diffViewerModes as viewerModes } from '~/ide/constants'; +import { getDerivedMergeRequestInformation } from '~/code_review/utils/merge_request'; import { changeInPercent, numberToHumanSize } from '~/lib/utils/number_utils'; import { truncateSha } from '~/lib/utils/text_utility'; import { uuids } from '~/lib/utils/uuids'; @@ -9,7 +10,6 @@ import { DIFF_FILE_MANUAL_COLLAPSE, DIFF_FILE_AUTOMATIC_COLLAPSE, } from '../constants'; -import { getDerivedMergeRequestInformation } from './merge_request'; function fileSymlinkInformation(file, fileList) { const duplicates = fileList.filter((iteratedFile) => iteratedFile.file_hash === file.file_hash); diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js index bc81c0b0a05407711c3ef524cce511c65b82fb00..934b8d825a82ebaffa44e77d1d51d2642f545180 100644 --- a/app/assets/javascripts/diffs/utils/merge_request.js +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -1,20 +1,5 @@ import { ZERO_CHANGES_ALT_DISPLAY } from '../constants'; -const endpointRE = /^(\/?(.+\/)+(.+)\/-\/merge_requests\/(\d+)).*$/i; - -function getVersionInfo({ endpoint } = {}) { - const dummyRoot = 'https://gitlab.com'; - const endpointUrl = new URL(endpoint, dummyRoot); - const params = Object.fromEntries(endpointUrl.searchParams.entries()); - - const { start_sha: startSha, diff_id: diffId } = params; - - return { - diffId, - startSha, - }; -} - export function updateChangesTabCount({ count, badge = document.querySelector('.js-diffs-tab .gl-badge'), @@ -25,29 +10,3 @@ export function updateChangesTabCount({ badge.textContent = count || ZERO_CHANGES_ALT_DISPLAY; } } - -export function getDerivedMergeRequestInformation({ endpoint } = {}) { - let mrPath; - let namespace; - let project; - let id; - let diffId; - let startSha; - const matches = endpointRE.exec(endpoint); - - if (matches) { - [, mrPath, namespace, project, id] = matches; - ({ diffId, startSha } = getVersionInfo({ endpoint })); - - namespace = namespace.replace(/\/$/, ''); - } - - return { - mrPath, - namespace, - project, - id, - diffId, - startSha, - }; -} diff --git a/app/assets/javascripts/mr_notes/init.js b/app/assets/javascripts/mr_notes/init.js index 28f294589ae45856cc8724a999cd823be449b2ab..fd4fe72af5767fcf212ac14f8fc78d381d25950c 100644 --- a/app/assets/javascripts/mr_notes/init.js +++ b/app/assets/javascripts/mr_notes/init.js @@ -5,7 +5,7 @@ import eventHub from '~/notes/event_hub'; import { initReviewBar } from '~/batch_comments'; import { initDiscussionCounter } from '~/mr_notes/discussion_counter'; import { initOverviewTabCounter } from '~/mr_notes/init_count'; -import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request'; +import { getDerivedMergeRequestInformation } from '~/code_review/utils/merge_request'; import { getReviewsForMergeRequest } from '~/diffs/utils/file_reviews'; import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE } from '~/diffs/constants'; diff --git a/app/assets/javascripts/pages/projects/merge_requests/page.js b/app/assets/javascripts/pages/projects/merge_requests/page.js index 75e308e706fbac99eda369dbcc9ea99a1749497d..065bad3e94ad4c26a962d5ffaea71069f4f11ec3 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/page.js +++ b/app/assets/javascripts/pages/projects/merge_requests/page.js @@ -4,7 +4,7 @@ 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 codeReviewEventHub from '~/code_review/event_hub'; import store from '~/mr_notes/stores'; import initSidebarBundle from '~/sidebar/sidebar_bundle'; import { apolloProvider } from '~/graphql_shared/issuable_client'; @@ -19,7 +19,7 @@ export function initMrPage() { initMrNotes(); initShow(); initMrMoreDropdown(); - startCodeReviewMessaging({ signalBus: diffsEventHub }); + startCodeReviewMessaging({ signalBus: codeReviewEventHub }); } requestIdleCallback(() => { diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue index db48e68e8f623c78bbae86a464b5c520818245d5..c2f40ca9561d899136e431821cd7bbe7ce7a0cc5 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.vue @@ -17,6 +17,8 @@ import SmartInterval from '~/smart_interval'; import { TYPENAME_MERGE_REQUEST } from '~/graphql_shared/constants'; import { convertToGraphQLId } from '~/graphql_shared/utils'; import { setFaviconOverlay } from '~/lib/utils/favicon'; +import codeReviewEventHub from '~/code_review/event_hub'; +import { EVT_MR_PREPARED } from '~/code_review/constants'; import Loading from './components/loading.vue'; import MrWidgetAlertMessage from './components/mr_widget_alert_message.vue'; import MrWidgetPipelineContainer from './components/mr_widget_pipeline_container.vue'; @@ -299,9 +301,12 @@ export default { message: __('Unable to load the merge request widget. Try reloading the page.'), }), ); + + codeReviewEventHub.$on(EVT_MR_PREPARED, this.checkStatus); }, beforeDestroy() { eventHub.$off('mr.discussion.updated', this.checkStatus); + codeReviewEventHub.$off(EVT_MR_PREPARED, this.checkState); if (this.deploymentsInterval) { this.deploymentsInterval.destroy(); diff --git a/spec/frontend/code_review/signals_spec.js b/spec/frontend/code_review/signals_spec.js index 3758dd1222b56c37bcdfe3300a95a7175202899d..1fe70e5da82741cb8e21f56564f281adcd63ff7c 100644 --- a/spec/frontend/code_review/signals_spec.js +++ b/spec/frontend/code_review/signals_spec.js @@ -1,9 +1,10 @@ import { start } from '~/code_review/signals'; +import { EVT_MR_PREPARED } from '~/code_review/constants'; +import { getDerivedMergeRequestInformation } from '~/code_review/utils/merge_request'; + 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'); +jest.mock('~/code_review/utils/merge_request'); describe('~/code_review', () => { const io = diffsEventHub; diff --git a/spec/frontend/code_review/utils/merge_request_spec.js b/spec/frontend/code_review/utils/merge_request_spec.js new file mode 100644 index 0000000000000000000000000000000000000000..963de6dee69ae09d2e982f275280a358ea571748 --- /dev/null +++ b/spec/frontend/code_review/utils/merge_request_spec.js @@ -0,0 +1,79 @@ +import { getDerivedMergeRequestInformation } from '~/code_review/utils/merge_request'; + +describe('Merge Request utilities', () => { + const latestVersion = '/gitlab-org/gitlab-test/-/merge_requests/4/diffs'; + const comparePath = + '/gitlab-org/gitlab-test/-/merge_requests/4/diffs?diff_id=4\u0026start_sha=eb227b3e214624708c474bdab7bde7afc17cefcc'; + const derivedBaseInfo = { + mrPath: '/gitlab-org/gitlab-test/-/merge_requests/4', + namespace: 'gitlab-org', + project: 'gitlab-test', + id: '4', + }; + const derivedVersionInfo = { + diffId: '4', + startSha: 'eb227b3e214624708c474bdab7bde7afc17cefcc', + }; + const noVersion = { + diffId: undefined, + startSha: undefined, + }; + const unparseableEndpoint = { + mrPath: undefined, + namespace: undefined, + project: undefined, + id: undefined, + ...noVersion, + }; + + describe('getDerivedMergeRequestInformation', () => { + const bare = latestVersion; + + it.each` + argument | response + ${{ endpoint: `${bare}.json?searchParam=irrelevant` }} | ${{ ...derivedBaseInfo, ...noVersion }} + ${{}} | ${unparseableEndpoint} + ${{ endpoint: undefined }} | ${unparseableEndpoint} + ${{ endpoint: null }} | ${unparseableEndpoint} + `('generates the correct derived results based on $argument', ({ argument, response }) => { + expect(getDerivedMergeRequestInformation(argument)).toStrictEqual(response); + }); + + describe('sub-group namespace', () => { + it('extracts the entire namespace plus the project name', () => { + const { namespace, project } = getDerivedMergeRequestInformation({ + endpoint: `/some/deep/path/of/groups${bare}`, + }); + + expect(namespace).toBe('some/deep/path/of/groups/gitlab-org'); + expect(project).toBe('gitlab-test'); + }); + }); + + describe('version information', () => { + it('still gets the correct derived information', () => { + expect( + getDerivedMergeRequestInformation({ + endpoint: comparePath, + }), + ).toMatchObject(derivedBaseInfo); + }); + + it.each` + url | versionPart + ${comparePath} | ${derivedVersionInfo} + ${`${bare}?diff_id=${derivedVersionInfo.diffId}`} | ${{ ...derivedVersionInfo, startSha: undefined }} + ${`${bare}?start_sha=${derivedVersionInfo.startSha}`} | ${{ ...derivedVersionInfo, diffId: undefined }} + `( + 'generates the correct derived version information based on $url', + ({ url, versionPart }) => { + expect(getDerivedMergeRequestInformation({ endpoint: url })).toMatchObject(versionPart); + }, + ); + + it('extracts nothing if there is no available version-like information in the URL', () => { + expect(getDerivedMergeRequestInformation({ endpoint: bare })).toMatchObject(noVersion); + }); + }); + }); +}); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 387407a7e4d6cb08c6775f76968d9c336e26cdb5..405116f36c6002cbf1dde924ff410b9d27c3c071 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -5,11 +5,11 @@ import { useLocalStorageSpy } from 'helpers/local_storage_helper'; import { TEST_HOST } from 'helpers/test_constants'; import testAction from 'helpers/vuex_action_helper'; import { getDiffFileMock } from 'jest/diffs/mock_data/diff_file'; +import { EVT_MR_PREPARED } from '~/code_review/constants'; import { DIFF_VIEW_COOKIE_NAME, INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE, - EVT_MR_PREPARED, } from '~/diffs/constants'; import { LOAD_SINGLE_DIFF_FAILED, BUILDING_YOUR_MR, SOMETHING_WENT_WRONG } from '~/diffs/i18n'; import * as diffActions from '~/diffs/store/actions'; diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js index 11c0efb9a9c4d1c3f91177749ec6329aa125b1c8..12f94642cea09d4e75cc6c69eb5a3b34b7a5558f 100644 --- a/spec/frontend/diffs/utils/merge_request_spec.js +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -1,33 +1,7 @@ -import { - updateChangesTabCount, - getDerivedMergeRequestInformation, -} from '~/diffs/utils/merge_request'; +import { updateChangesTabCount } from '~/diffs/utils/merge_request'; import { ZERO_CHANGES_ALT_DISPLAY } from '~/diffs/constants'; -import { diffMetadata } from '../mock_data/diff_metadata'; describe('Merge Request utilities', () => { - const derivedBaseInfo = { - mrPath: '/gitlab-org/gitlab-test/-/merge_requests/4', - namespace: 'gitlab-org', - project: 'gitlab-test', - id: '4', - }; - const derivedVersionInfo = { - diffId: '4', - startSha: 'eb227b3e214624708c474bdab7bde7afc17cefcc', - }; - const noVersion = { - diffId: undefined, - startSha: undefined, - }; - const unparseableEndpoint = { - mrPath: undefined, - namespace: undefined, - project: undefined, - id: undefined, - ...noVersion, - }; - describe('updateChangesTabCount', () => { let dummyTab; let badge; @@ -77,55 +51,4 @@ describe('Merge Request utilities', () => { expect(dummyTab.textContent).toBe('42'); }); }); - - describe('getDerivedMergeRequestInformation', () => { - const bare = diffMetadata.latest_version_path; - - it.each` - argument | response - ${{ endpoint: `${bare}.json?searchParam=irrelevant` }} | ${{ ...derivedBaseInfo, ...noVersion }} - ${{}} | ${unparseableEndpoint} - ${{ endpoint: undefined }} | ${unparseableEndpoint} - ${{ endpoint: null }} | ${unparseableEndpoint} - `('generates the correct derived results based on $argument', ({ argument, response }) => { - expect(getDerivedMergeRequestInformation(argument)).toStrictEqual(response); - }); - - describe('sub-group namespace', () => { - it('extracts the entire namespace plus the project name', () => { - const { namespace, project } = getDerivedMergeRequestInformation({ - endpoint: `/some/deep/path/of/groups${bare}`, - }); - - expect(namespace).toBe('some/deep/path/of/groups/gitlab-org'); - expect(project).toBe('gitlab-test'); - }); - }); - - describe('version information', () => { - it('still gets the correct derived information', () => { - expect( - getDerivedMergeRequestInformation({ - endpoint: diffMetadata.merge_request_diffs[0].compare_path, - }), - ).toMatchObject(derivedBaseInfo); - }); - - it.each` - url | versionPart - ${diffMetadata.merge_request_diffs[0].compare_path} | ${derivedVersionInfo} - ${`${bare}?diff_id=${derivedVersionInfo.diffId}`} | ${{ ...derivedVersionInfo, startSha: undefined }} - ${`${bare}?start_sha=${derivedVersionInfo.startSha}`} | ${{ ...derivedVersionInfo, diffId: undefined }} - `( - 'generates the correct derived version information based on $url', - ({ url, versionPart }) => { - expect(getDerivedMergeRequestInformation({ endpoint: url })).toMatchObject(versionPart); - }, - ); - - it('extracts nothing if there is no available version-like information in the URL', () => { - expect(getDerivedMergeRequestInformation({ endpoint: bare })).toMatchObject(noVersion); - }); - }); - }); });