From 8aaf31ad9af69eebbb45e29e24874a037d50df2a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 22 May 2023 15:30:47 -0600 Subject: [PATCH 1/4] Remove `realtime_approvals` feature flag Changelog: changed --- .../mixins/approvals.js | 10 +---- .../projects/merge_requests_controller.rb | 1 - .../development/realtime_approvals.yml | 8 ---- .../components/approvals/approvals_spec.js | 44 +++++-------------- 4 files changed, 11 insertions(+), 52 deletions(-) delete mode 100644 config/feature_flags/development/realtime_approvals.yml diff --git a/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js b/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js index 3228c09c9b6a9f..564e9321d54c50 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js +++ b/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js @@ -39,7 +39,7 @@ export default { }; }, skip() { - return !this.mr?.id || !this.isRealtimeEnabled; + return !this.mr?.id; }, updateQuery( _, @@ -63,14 +63,6 @@ export default { disableCommittersApproval: false, }; }, - computed: { - isRealtimeEnabled() { - // This mixin needs glFeatureFlagsMixin, but fatals if it's included here. - // Parents that include this mixin (approvals) should also include the - // glFeatureFlagsMixin mixin, or this will always be false. - return Boolean(this.glFeatures?.realtimeApprovals); - }, - }, methods: { clearError() { this.$emit('clearError'); diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 2f37382da732f6..c8f5cbe29f485e 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -46,7 +46,6 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:moved_mr_sidebar, project) push_frontend_feature_flag(:mr_experience_survey, project) push_frontend_feature_flag(:realtime_mr_status_change, project) - push_frontend_feature_flag(:realtime_approvals, project) push_frontend_feature_flag(:saved_replies, current_user) push_frontend_feature_flag(:code_quality_inline_drawer, project) push_frontend_feature_flag(:auto_merge_labels_mr_widget, project) diff --git a/config/feature_flags/development/realtime_approvals.yml b/config/feature_flags/development/realtime_approvals.yml deleted file mode 100644 index adb818e52bf5fb..00000000000000 --- a/config/feature_flags/development/realtime_approvals.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: realtime_approvals -introduced_by_url: 'https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114963' -rollout_issue_url: 'https://gitlab.com/gitlab-org/gitlab/-/issues/399539' -milestone: '16.0' -type: development -group: group::code review -default_enabled: true diff --git a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js index a07a60438fb70c..befb4c45cbb958 100644 --- a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js +++ b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js @@ -57,9 +57,6 @@ describe('MRWidget approvals', () => { const apolloProvider = createMockApollo(requestHandlers); const provide = { ...options.provide, - glFeatures: { - realtimeApprovals: options.provide?.glFeatures?.realtimeApprovals || false, - }, }; subscriptionHandlers.forEach(([document, stream]) => { @@ -386,42 +383,21 @@ describe('MRWidget approvals', () => { }); describe('realtime approvals update', () => { - describe('realtime_approvals feature disabled', () => { - beforeEach(() => { - jest.spyOn(console, 'warn').mockImplementation(); - createComponent(); - }); + const subscriptionApproval = { approved: true }; + const subscriptionResponse = { + data: { mergeRequestApprovalStateUpdated: subscriptionApproval }, + }; - it('does not subscribe to the approvals update socket', () => { - expect(mr.setApprovals).not.toHaveBeenCalled(); - mockedSubscription.next({}); - // eslint-disable-next-line no-console - expect(console.warn).toHaveBeenCalledWith( - expect.stringMatching('Mock subscription has no observer, this will have no effect'), - ); - expect(mr.setApprovals).not.toHaveBeenCalled(); - }); + beforeEach(() => { + createComponent(); }); - describe('realtime_approvals feature enabled', () => { - const subscriptionApproval = { approved: true }; - const subscriptionResponse = { - data: { mergeRequestApprovalStateUpdated: subscriptionApproval }, - }; + it('updates approvals when the subscription data is streamed to the Apollo client', () => { + expect(mr.setApprovals).not.toHaveBeenCalled(); - beforeEach(() => { - createComponent({ - provide: { glFeatures: { realtimeApprovals: true } }, - }); - }); + mockedSubscription.next(subscriptionResponse); - it('updates approvals when the subscription data is streamed to the Apollo client', () => { - expect(mr.setApprovals).not.toHaveBeenCalled(); - - mockedSubscription.next(subscriptionResponse); - - expect(mr.setApprovals).toHaveBeenCalledWith(subscriptionApproval); - }); + expect(mr.setApprovals).toHaveBeenCalledWith(subscriptionApproval); }); }); }); -- GitLab From b2a2db5454f087f67a77f4902c2974d3c2a2718a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 7 Jun 2023 15:40:58 -0600 Subject: [PATCH 2/4] Mock approvals subscriptions (CE) Unfortunately, because realtime approvals are no longer feature flagged, the existing mocks for the approvals widget (testing the approvals.js mixin) must be duplicated here, because the options widget always receives an unmocked subscription request. The tests for the actual approvals mixin (in approvals_spec.js) actually exercise the subscription feature, and the changes here merely mock the subscription so it doesn't constantly throw errors. As a part of mocking the subscription, this change also refactors the way the Apollo client is constructed to separate WHAT we are mocking from the construction of the client. The root cause of this is that our Apollo client mock utility does not properly mock subscriptions, which means we need to use `apolloProvider.defaultClient.setRequestHandler`, which splits the query mocks up from the subscription mocks. Because of that split, I've moved both lists of mocks to one place, so changes appear nearby to each other. --- .../mr_widget_options_spec.js | 48 ++++++++++++------- 1 file changed, 30 insertions(+), 18 deletions(-) diff --git a/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js b/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js index 266f69c8f2e668..f357735500b315 100644 --- a/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js +++ b/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js @@ -4,6 +4,7 @@ import MockAdapter from 'axios-mock-adapter'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import * as Sentry from '@sentry/browser'; +import { createMockSubscription as createMockApolloSubscription } from 'mock-apollo-client'; import approvedByCurrentUser from 'test_fixtures/graphql/merge_requests/approvals/approvals.query.graphql.json'; import getStateQueryResponse from 'test_fixtures/graphql/merge_requests/get_state.query.graphql.json'; import readyToMergeResponse from 'test_fixtures/graphql/merge_requests/states/ready_to_merge.query.graphql.json'; @@ -31,6 +32,7 @@ import securityReportMergeRequestDownloadPathsQuery from '~/vue_shared/security_ import getStateQuery from '~/vue_merge_request_widget/queries/get_state.query.graphql'; import readyToMergeQuery from 'ee_else_ce/vue_merge_request_widget/queries/states/ready_to_merge.query.graphql'; import approvalsQuery from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql'; +import approvedBySubscription from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql'; import userPermissionsQuery from '~/vue_merge_request_widget/queries/permissions.query.graphql'; import conflictsStateQuery from '~/vue_merge_request_widget/queries/states/conflicts.query.graphql'; import { faviconDataUrl, overlayDataUrl } from '../lib/utils/mock_data'; @@ -63,6 +65,7 @@ jest.mock('@sentry/browser', () => ({ Vue.use(VueApollo); describe('MrWidgetOptions', () => { + let mockedApprovalsSubscription; let stateQueryHandler; let queryResponse; let wrapper; @@ -94,8 +97,7 @@ describe('MrWidgetOptions', () => { }); const createComponent = (mrData = mockData, options = {}, data = {}, fullMount = true) => { - const mounting = fullMount ? mount : shallowMount; - + mockedApprovalsSubscription = createMockApolloSubscription(); queryResponse = { data: { project: { @@ -108,6 +110,31 @@ describe('MrWidgetOptions', () => { }, }; stateQueryHandler = jest.fn().mockResolvedValue(queryResponse); + + const mounting = fullMount ? mount : shallowMount; + const queryHandlers = [ + [approvalsQuery, jest.fn().mockResolvedValue(approvedByCurrentUser)], + [getStateQuery, stateQueryHandler], + [readyToMergeQuery, jest.fn().mockResolvedValue(readyToMergeResponse)], + [ + userPermissionsQuery, + jest.fn().mockResolvedValue({ + data: { project: { mergeRequest: { userPermissions: {} } } }, + }), + ], + [ + conflictsStateQuery, + jest.fn().mockResolvedValue({ data: { project: { mergeRequest: {} } } }), + ], + ...(options.apolloMock || []), + ]; + const subscriptionHandlers = [[approvedBySubscription, () => mockedApprovalsSubscription]]; + const apolloProvider = createMockApollo(queryHandlers); + + subscriptionHandlers.forEach(([document, stream]) => { + apolloProvider.defaultClient.setRequestHandler(document, stream); + }); + wrapper = mounting(MrWidgetOptions, { propsData: { mrData: { ...mrData }, @@ -120,22 +147,7 @@ describe('MrWidgetOptions', () => { }, ...options, - apolloProvider: createMockApollo([ - [approvalsQuery, jest.fn().mockResolvedValue(approvedByCurrentUser)], - [getStateQuery, stateQueryHandler], - [readyToMergeQuery, jest.fn().mockResolvedValue(readyToMergeResponse)], - [ - userPermissionsQuery, - jest.fn().mockResolvedValue({ - data: { project: { mergeRequest: { userPermissions: {} } } }, - }), - ], - [ - conflictsStateQuery, - jest.fn().mockResolvedValue({ data: { project: { mergeRequest: {} } } }), - ], - ...(options.apolloMock || []), - ]), + apolloProvider, }); return axios.waitForAll(); -- GitLab From 529b892a0b4a5974857ae25cd364a521e40b048a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 8 Jun 2023 21:06:18 -0600 Subject: [PATCH 3/4] Mock approvals subscriptions (EE) Unfortunately, because realtime approvals are no longer feature flagged, the existing mocks for the approvals widget (testing the approvals.js mixin) must be duplicated here, because the options widget always receives an unmocked subscription request. The tests for the actual approvals mixin (in approvals_spec.js) actually exercise the subscription feature, and the changes here merely mock the subscription so it doesn't constantly throw errors. As a part of mocking the subscription, this change also refactors the way the Apollo client is constructed to separate WHAT we are mocking from the construction of the client. The root cause of this is that our Apollo client mock utility does not properly mock subscriptions, which means we need to use `apolloProvider.defaultClient.setRequestHandler`, which splits the query mocks up from the subscription mocks. Because of that split, I've moved both lists of mocks to one place, so changes appear nearby to each other. Finally, in order for some subscriptions to work in this context, they must be uniquely mocked for every new query, so the list of mocks is stored per query name in a Symbolic-key list. This - and the link to the documentation for it - is noted in the code. --- .../ee_mr_widget_options_spec.js | 68 +++++++++++++------ 1 file changed, 48 insertions(+), 20 deletions(-) diff --git a/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js b/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js index 1f5642e70b3d73..571daf569809e4 100644 --- a/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js @@ -2,6 +2,7 @@ import { mount } from '@vue/test-utils'; import MockAdapter from 'axios-mock-adapter'; import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; +import { createMockSubscription as createMockApolloSubscription } from 'mock-apollo-client'; import approvedByCurrentUser from 'test_fixtures/graphql/merge_requests/approvals/approvals.query.graphql.json'; import getStateQueryResponse from 'test_fixtures/graphql/merge_requests/get_state.query.graphql.json'; @@ -54,6 +55,7 @@ import getStateQuery from '~/vue_merge_request_widget/queries/get_state.query.gr import readyToMergeQuery from 'ee_else_ce/vue_merge_request_widget/queries/states/ready_to_merge.query.graphql'; import mergeQuery from '~/vue_merge_request_widget/queries/states/new_ready_to_merge.query.graphql'; import approvalsQuery from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql'; +import approvedBySubscription from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql'; import securityReportSummaryQuery from 'ee/vue_shared/security_reports/graphql/mr_security_report_summary.graphql'; import mockData from './mock_data'; @@ -71,32 +73,58 @@ const COVERAGE_FUZZING_SELECTOR = '.js-coverage-fuzzing-widget'; const API_FUZZING_SELECTOR = '.js-api-fuzzing-widget'; describe('ee merge request widget options', () => { + const allSubscriptions = {}; let wrapper; let mock; const createComponent = (options) => { + const queryHandlers = [ + [approvalsQuery, jest.fn().mockResolvedValue(approvedByCurrentUser)], + [getStateQuery, jest.fn().mockResolvedValue(getStateQueryResponse)], + [readyToMergeQuery, jest.fn().mockResolvedValue(readyToMergeResponse)], + [ + securityReportMergeRequestDownloadPathsQuery, + jest.fn().mockResolvedValue({ data: securityReportMergeRequestDownloadPathsQueryResponse }), + ], + [securityReportSummaryQuery, jest.fn().mockResolvedValue({ data: { project: null } })], + [ + mergeQuery, + jest.fn().mockResolvedValue({ + data: { + project: { id: 1, mergeRequest: { id: 1, userPermissions: { canMerge: true } } }, + }, + }), + ], + ]; + const subscriptionHandlers = [ + [ + approvedBySubscription, + () => { + // Please see https://github.com/mike-gibson/mock-apollo-client#multiple-subscriptions + // for why subscriptions must be mocked this way, in this context + // Note that the keyed object -> array structure is so that: + // A) when necessary, we can publish (.next) events into the stream + // B) we can do that by name (per subscription) rather than as a single array of all subscriptions + const sym = Symbol.for('approvedBySubscription'); + const newSub = createMockApolloSubscription(); + const container = allSubscriptions[sym] || []; + + container.push(newSub); + allSubscriptions[sym] = container; + + return newSub; + }, + ], + ]; + const apolloProvider = createMockApollo(queryHandlers); + + subscriptionHandlers.forEach(([document, stream]) => { + apolloProvider.defaultClient.setRequestHandler(document, stream); + }); + wrapper = mount(MrWidgetOptions, { ...options, - apolloProvider: createMockApollo([ - [approvalsQuery, jest.fn().mockResolvedValue(approvedByCurrentUser)], - [getStateQuery, jest.fn().mockResolvedValue(getStateQueryResponse)], - [readyToMergeQuery, jest.fn().mockResolvedValue(readyToMergeResponse)], - [ - securityReportMergeRequestDownloadPathsQuery, - jest - .fn() - .mockResolvedValue({ data: securityReportMergeRequestDownloadPathsQueryResponse }), - ], - [securityReportSummaryQuery, jest.fn().mockResolvedValue({ data: { project: null } })], - [ - mergeQuery, - jest.fn().mockResolvedValue({ - data: { - project: { id: 1, mergeRequest: { id: 1, userPermissions: { canMerge: true } } }, - }, - }), - ], - ]), + apolloProvider, data() { return { loading: false, -- GitLab From aa2a13f9528195eab2531702ef51170dc0fc287f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 9 Jun 2023 12:54:06 -0600 Subject: [PATCH 4/4] Apply review comments - replace 'document' with 'query' - use permalink to the documentation --- .../vue_merge_request_widget/ee_mr_widget_options_spec.js | 6 +++--- .../components/approvals/approvals_spec.js | 4 ++-- .../vue_merge_request_widget/mr_widget_options_spec.js | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js b/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js index 571daf569809e4..1c3dbb21e8d24f 100644 --- a/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js +++ b/ee/spec/frontend/vue_merge_request_widget/ee_mr_widget_options_spec.js @@ -100,7 +100,7 @@ describe('ee merge request widget options', () => { [ approvedBySubscription, () => { - // Please see https://github.com/mike-gibson/mock-apollo-client#multiple-subscriptions + // Please see https://github.com/Mike-Gibson/mock-apollo-client/blob/c85746f1433b42af83ef6ca0d2904ccad6076666/README.md#multiple-subscriptions // for why subscriptions must be mocked this way, in this context // Note that the keyed object -> array structure is so that: // A) when necessary, we can publish (.next) events into the stream @@ -118,8 +118,8 @@ describe('ee merge request widget options', () => { ]; const apolloProvider = createMockApollo(queryHandlers); - subscriptionHandlers.forEach(([document, stream]) => { - apolloProvider.defaultClient.setRequestHandler(document, stream); + subscriptionHandlers.forEach(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, stream); }); wrapper = mount(MrWidgetOptions, { diff --git a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js index befb4c45cbb958..724f9ea0346a1a 100644 --- a/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js +++ b/spec/frontend/vue_merge_request_widget/components/approvals/approvals_spec.js @@ -59,8 +59,8 @@ describe('MRWidget approvals', () => { ...options.provide, }; - subscriptionHandlers.forEach(([document, stream]) => { - apolloProvider.defaultClient.setRequestHandler(document, stream); + subscriptionHandlers.forEach(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, stream); }); wrapper = shallowMount(Approvals, { diff --git a/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js b/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js index f357735500b315..656203b1d25b8d 100644 --- a/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js +++ b/spec/frontend/vue_merge_request_widget/mr_widget_options_spec.js @@ -131,8 +131,8 @@ describe('MrWidgetOptions', () => { const subscriptionHandlers = [[approvedBySubscription, () => mockedApprovalsSubscription]]; const apolloProvider = createMockApollo(queryHandlers); - subscriptionHandlers.forEach(([document, stream]) => { - apolloProvider.defaultClient.setRequestHandler(document, stream); + subscriptionHandlers.forEach(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, stream); }); wrapper = mounting(MrWidgetOptions, { -- GitLab