From 9823b0b1f6a6c45eedde333443fe5766c5cf6bc3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 23 Mar 2023 17:58:46 -0600 Subject: [PATCH 1/5] Add realtime_approvals feature flag --- app/controllers/projects/merge_requests_controller.rb | 1 + config/feature_flags/development/realtime_approvals.yml | 8 ++++++++ 2 files changed, 9 insertions(+) create mode 100644 config/feature_flags/development/realtime_approvals.yml diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index dbcbd2467b651f..f72f0ccb593792 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -47,6 +47,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:single_file_file_by_file, 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(:hide_create_issue_resolve_all, project) diff --git a/config/feature_flags/development/realtime_approvals.yml b/config/feature_flags/development/realtime_approvals.yml new file mode 100644 index 00000000000000..8245764a8ea90a --- /dev/null +++ b/config/feature_flags/development/realtime_approvals.yml @@ -0,0 +1,8 @@ +--- +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: '15.11' +type: development +group: group::code review +default_enabled: false -- GitLab From abe82653c9a3150725e0a6e236c12551c43affca Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 16 Mar 2023 17:14:26 -0600 Subject: [PATCH 2/5] Add subscription for approval widget --- .../queries/approvals.subscription.graphql | 17 ++++++++ .../mixins/approvals.js | 41 ++++++++++++++++++- .../queries/approvals.subscription.graphql | 38 +++++++++++++++++ 3 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql create mode 100644 ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql diff --git a/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql b/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql new file mode 100644 index 00000000000000..d5092d9ae1af94 --- /dev/null +++ b/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql @@ -0,0 +1,17 @@ +#import "~/graphql_shared/fragments/user.fragment.graphql" + +subscription mergeRequestApprovalStateUpdated($issuableId: IssuableID!) { + mergeRequestApprovalStateUpdated(issuableId: $issuableId) { + ... on MergeRequest { + id + approvedBy { + nodes { + ...User + } + } + userPermissions { + canApprove + } + } + } +} 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 7e658e77d37e2e..64ebc3924e3667 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js +++ b/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js @@ -1,5 +1,11 @@ -import { createAlert } from '~/alert'; +import mergeRequestApprovalStateUpdated from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql'; import approvedByQuery from 'ee_else_ce/vue_merge_request_widget/components/approvals/queries/approvals.query.graphql'; + +import { createAlert } from '~/alert'; + +import { convertToGraphQLId } from '../../graphql_shared/utils'; +import { TYPENAME_MERGE_REQUEST } from '../../graphql_shared/constants'; + import { FETCH_ERROR } from '../components/approvals/messages'; export default { @@ -25,6 +31,31 @@ export default { message: FETCH_ERROR, }); }, + subscribeToMore: { + document() { + return mergeRequestApprovalStateUpdated; + }, + variables() { + return { + issuableId: convertToGraphQLId(TYPENAME_MERGE_REQUEST, this.mr.id), + }; + }, + skip() { + return !this.mr?.id || !this.isRealtimeEnabled; + }, + updateQuery( + _, + { + subscriptionData: { + data: { mergeRequestApprovalStateUpdated: queryResult }, + }, + }, + ) { + if (queryResult) { + this.mr.setApprovals(queryResult); + } + }, + }, }, }, data() { @@ -34,6 +65,14 @@ 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/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql new file mode 100644 index 00000000000000..cce91b3004edfe --- /dev/null +++ b/ee/app/assets/javascripts/vue_merge_request_widget/components/approvals/queries/approvals.subscription.graphql @@ -0,0 +1,38 @@ +#import "~/graphql_shared/fragments/user.fragment.graphql" + +subscription mergeRequestApprovalStateUpdatedEE($issuableId: IssuableID!) { + mergeRequestApprovalStateUpdated(issuableId: $issuableId) { + ... on MergeRequest { + id + approved + approvalsLeft + approvalsRequired + approvalState { + suggestedApprovers { + nodes { + ...User + } + } + invalidApproversRules { + id + name + } + rules { + id + approved + approvalsRequired + name + type + } + } + approvedBy { + nodes { + ...User + } + } + userPermissions { + canApprove + } + } + } +} -- GitLab From d9f807bf9f9b4c4cb4cb8972036d85f842fd1516 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 4 May 2023 12:59:27 -0600 Subject: [PATCH 3/5] Switch from function getter to direct object access MR Review: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114963/diffs#note_1376112936 --- .../javascripts/vue_merge_request_widget/mixins/approvals.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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 64ebc3924e3667..3228c09c9b6a9f 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js +++ b/app/assets/javascripts/vue_merge_request_widget/mixins/approvals.js @@ -32,9 +32,7 @@ export default { }); }, subscribeToMore: { - document() { - return mergeRequestApprovalStateUpdated; - }, + document: mergeRequestApprovalStateUpdated, variables() { return { issuableId: convertToGraphQLId(TYPENAME_MERGE_REQUEST, this.mr.id), -- GitLab From 8f3831f8d01fab0b12da9b2a37763b6373dfc3c5 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 4 May 2023 13:01:56 -0600 Subject: [PATCH 4/5] Update feature flag milestone MR Review: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114963/diffs#note_1375764459 --- config/feature_flags/development/realtime_approvals.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/feature_flags/development/realtime_approvals.yml b/config/feature_flags/development/realtime_approvals.yml index 8245764a8ea90a..b2a4131b0f6c01 100644 --- a/config/feature_flags/development/realtime_approvals.yml +++ b/config/feature_flags/development/realtime_approvals.yml @@ -2,7 +2,7 @@ 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: '15.11' +milestone: '16.0' type: development group: group::code review default_enabled: false -- GitLab From 2742335ed2cb98188f9a5f3bcc5c2d3f3675c1d9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 9 May 2023 10:59:44 -0600 Subject: [PATCH 5/5] Test Apollo subscription for real-time approvals updates --- .../components/approvals/approvals_spec.js | 82 ++++++++++++++++--- 1 file changed, 70 insertions(+), 12 deletions(-) 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 e78e1be7882f8a..a07a60438fb70c 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 @@ -2,6 +2,7 @@ import Vue, { nextTick } from 'vue'; import VueApollo from 'vue-apollo'; import { GlButton, GlSprintf } from '@gitlab/ui'; import { shallowMount } from '@vue/test-utils'; +import { createMockSubscription as createMockApolloSubscription } from 'mock-apollo-client'; import approvedByCurrentUser from 'test_fixtures/graphql/merge_requests/approvals/approvals.query.graphql.json'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; @@ -16,6 +17,7 @@ import { } from '~/vue_merge_request_widget/components/approvals/messages'; import eventHub from '~/vue_merge_request_widget/event_hub'; import approvedByQuery 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 { createCanApproveResponse } from 'jest/approvals/mock_data'; Vue.use(VueApollo); @@ -42,21 +44,36 @@ const testApprovals = () => ({ }); describe('MRWidget approvals', () => { + let mockedSubscription; let wrapper; let service; let mr; - const createComponent = (props = {}, response = approvedByCurrentUser) => { - const requestHandlers = [[approvedByQuery, jest.fn().mockResolvedValue(response)]]; + const createComponent = (options = {}, responses = { query: approvedByCurrentUser }) => { + mockedSubscription = createMockApolloSubscription(); + + const requestHandlers = [[approvedByQuery, jest.fn().mockResolvedValue(responses.query)]]; + const subscriptionHandlers = [[approvedBySubscription, () => mockedSubscription]]; const apolloProvider = createMockApollo(requestHandlers); + const provide = { + ...options.provide, + glFeatures: { + realtimeApprovals: options.provide?.glFeatures?.realtimeApprovals || false, + }, + }; + + subscriptionHandlers.forEach(([document, stream]) => { + apolloProvider.defaultClient.setRequestHandler(document, stream); + }); wrapper = shallowMount(Approvals, { apolloProvider, propsData: { mr, service, - ...props, + ...options.props, }, + provide, stubs: { GlSprintf, }, @@ -97,6 +114,7 @@ describe('MRWidget approvals', () => { isOpen: true, state: 'open', targetProjectFullPath: 'gitlab-org/gitlab', + id: 1, iid: '1', }; @@ -114,7 +132,7 @@ describe('MRWidget approvals', () => { mr.isOpen = false; - createComponent({}, response); + createComponent({}, { query: response }); await waitForPromises(); }); @@ -128,7 +146,7 @@ describe('MRWidget approvals', () => { const response = JSON.parse(JSON.stringify(approvedByCurrentUser)); response.data.project.mergeRequest.approvedBy.nodes = []; - createComponent({}, response); + createComponent({}, { query: response }); await waitForPromises(); }); @@ -146,7 +164,7 @@ describe('MRWidget approvals', () => { describe('and MR is unapproved', () => { beforeEach(async () => { - createComponent({}, canApproveResponse); + createComponent({}, { query: canApproveResponse }); await waitForPromises(); }); @@ -167,7 +185,7 @@ describe('MRWidget approvals', () => { describe('with no approvers', () => { beforeEach(async () => { canApproveResponse.data.project.mergeRequest.approvedBy.nodes = []; - createComponent({}, canApproveResponse); + createComponent({}, { query: canApproveResponse }); await nextTick(); }); @@ -186,7 +204,7 @@ describe('MRWidget approvals', () => { canApproveResponse.data.project.mergeRequest.approvedBy.nodes[0].id = 2; - createComponent({}, canApproveResponse); + createComponent({}, { query: canApproveResponse }); await waitForPromises(); }); @@ -202,7 +220,7 @@ describe('MRWidget approvals', () => { describe('when approve action is clicked', () => { beforeEach(async () => { - createComponent({}, canApproveResponse); + createComponent({}, { query: canApproveResponse }); await waitForPromises(); }); @@ -259,7 +277,7 @@ describe('MRWidget approvals', () => { beforeEach(async () => { const response = JSON.parse(JSON.stringify(approvedByCurrentUser)); - createComponent({}, response); + createComponent({}, { query: response }); await waitForPromises(); }); @@ -320,7 +338,7 @@ describe('MRWidget approvals', () => { beforeEach(async () => { optionalApprovalsResponse.data.project.mergeRequest.userPermissions.canApprove = true; - createComponent({}, optionalApprovalsResponse); + createComponent({}, { query: optionalApprovalsResponse }); await waitForPromises(); }); @@ -335,7 +353,7 @@ describe('MRWidget approvals', () => { describe('and cannot approve', () => { beforeEach(async () => { - createComponent({}, optionalApprovalsResponse); + createComponent({}, { query: optionalApprovalsResponse }); await nextTick(); }); @@ -366,4 +384,44 @@ describe('MRWidget approvals', () => { }); }); }); + + describe('realtime approvals update', () => { + describe('realtime_approvals feature disabled', () => { + beforeEach(() => { + jest.spyOn(console, 'warn').mockImplementation(); + createComponent(); + }); + + 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(); + }); + }); + + describe('realtime_approvals feature enabled', () => { + const subscriptionApproval = { approved: true }; + const subscriptionResponse = { + data: { mergeRequestApprovalStateUpdated: subscriptionApproval }, + }; + + beforeEach(() => { + createComponent({ + provide: { glFeatures: { realtimeApprovals: true } }, + }); + }); + + 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); + }); + }); + }); }); -- GitLab