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 3228c09c9b6a9f23b65b26bf29ccc3568aae5620..564e9321d54c506f632a45d7653ecebd403113c0 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 2f37382da732f675f23e1e88c71bd985fffba005..c8f5cbe29f485e31b55320812addb13adf2feeb2 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 adb818e52bf5fb56abf737958226fdae5e5f4c56..0000000000000000000000000000000000000000 --- 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/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 1f5642e70b3d7330cf0586708b3f13462a16f550..1c3dbb21e8d24f1738237d795137bc3581cb89d0 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/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 + // 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(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, 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, 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 a07a60438fb70c14deaf79baf09d41c12b0a72f5..724f9ea0346a1aade6650a3e64f24915c30d1724 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,13 +57,10 @@ describe('MRWidget approvals', () => { const apolloProvider = createMockApollo(requestHandlers); const provide = { ...options.provide, - glFeatures: { - realtimeApprovals: options.provide?.glFeatures?.realtimeApprovals || false, - }, }; - subscriptionHandlers.forEach(([document, stream]) => { - apolloProvider.defaultClient.setRequestHandler(document, stream); + subscriptionHandlers.forEach(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, stream); }); wrapper = shallowMount(Approvals, { @@ -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); }); }); }); 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 266f69c8f2e66854420e021f05459e7c6195d5e5..656203b1d25b8d25d550f343d35e196b2b764aa7 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(([query, stream]) => { + apolloProvider.defaultClient.setRequestHandler(query, 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();