-
Notifications
You must be signed in to change notification settings - Fork 4
Cancel agreement via Sales API #2358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,5 +35,5 @@ export const cancelRecurringPaymentRequestParamsSchema = Joi.object({ | |
| }) | ||
|
|
||
| export const cancelRecurringPaymentRequestQuerySchema = Joi.object({ | ||
| reason: Joi.string().required().valid('Payment Failure', 'User Cancelled') | ||
| reason: Joi.string().required().valid('Payment Failure', 'User Cancelled', 'Business Cancelled') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test to make sure this validates now? |
||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,7 +52,8 @@ jest.mock('@defra-fish/dynamics-lib', () => ({ | |
| findDueRecurringPayments: jest.fn(), | ||
| findRecurringPaymentsByAgreementId: jest.fn(() => ({ toRetrieveRequest: () => {} })), | ||
| dynamicsClient: { | ||
| retrieveMultipleRequest: jest.fn(() => ({ value: [] })) | ||
| retrieveMultipleRequest: jest.fn(() => ({ value: [] })), | ||
| retrieveRequest: jest.fn(() => ({ _defra_activepermission_value: 'mock-permission-id' })) | ||
| }, | ||
| persist: jest.fn(), | ||
| findRecurringPaymentByPermissionId: jest.fn(() => ({ toRetrieveRequest: () => {} })), | ||
|
|
@@ -70,7 +71,8 @@ jest.mock('@defra-fish/connectors-lib', () => ({ | |
| } | ||
| })), | ||
| govUkPayApi: { | ||
| getRecurringPaymentAgreementInformation: jest.fn() | ||
| getRecurringPaymentAgreementInformation: jest.fn(), | ||
| cancelRecurringPaymentAgreement: jest.fn() | ||
| } | ||
| })) | ||
|
|
||
|
|
@@ -895,6 +897,18 @@ describe('recurring payments service', () => { | |
| }) | ||
|
|
||
| describe('cancelRecurringPayment', () => { | ||
| const mockPermission = new Permission() | ||
| mockPermission.isRecurringPayment = true | ||
|
|
||
| beforeEach(() => { | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValue({ ok: true, status: 204 }) | ||
| dynamicsClient.retrieveRequest.mockResolvedValue({ _defra_activepermission_value: 'mock-permission-id' }) | ||
| findById.mockImplementation(entityType => { | ||
| if (entityType === Permission) return mockPermission | ||
| return null | ||
| }) | ||
| }) | ||
|
|
||
| it('should call findById with RecurringPayment and the provided id', async () => { | ||
| retrieveGlobalOptionSets.mockReturnValueOnce({ cached: jest.fn().mockResolvedValue({ definition: 'mock-def' }) }) | ||
| findById.mockReturnValueOnce(getMockRecurringPayment()) | ||
|
|
@@ -911,7 +925,7 @@ describe('recurring payments service', () => { | |
| expect(getGlobalOptionSetValue).toHaveBeenCalledWith(RecurringPayment.definition.mappings.cancelledReason.ref, reason) | ||
| }) | ||
|
|
||
| it('should set cancelledDate when reason is not User Cancelled and call persist with the updated RecurringPayment', async () => { | ||
| it('should set cancelledDate when reason is Payment Failure and call persist with the updated RecurringPayment', async () => { | ||
| retrieveGlobalOptionSets.mockReturnValueOnce({ | ||
| cached: jest.fn().mockResolvedValue({ | ||
| defra_cancelledreasons: { | ||
|
|
@@ -938,11 +952,12 @@ describe('recurring payments service', () => { | |
| ...recurringPayment, | ||
| cancelledReason, | ||
| cancelledDate: expect.stringMatching(/^\d{4}-\d{2}-\d{2}$/) | ||
| }) | ||
| }), | ||
| mockPermission | ||
| ]) | ||
| }) | ||
|
|
||
| it('should not set cancelledDate when reason is User Cancelled', async () => { | ||
| it('should set cancelledDate when reason is User Cancelled', async () => { | ||
| retrieveGlobalOptionSets.mockReturnValueOnce({ | ||
| cached: jest.fn().mockResolvedValue({ | ||
| defra_cancelledreasons: { | ||
|
|
@@ -970,11 +985,128 @@ describe('recurring payments service', () => { | |
| expect.objectContaining({ | ||
| ...recurringPayment, | ||
| cancelledReason, | ||
| cancelledDate: null | ||
| }) | ||
| cancelledDate: expect.stringMatching(/^\d{4}-\d{2}-\d{2}$/) | ||
| }), | ||
| mockPermission | ||
|
Comment on lines
960
to
+990
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need this test any more since there shouldn't be any difference in behaviour between these reasons now |
||
| ]) | ||
| }) | ||
|
|
||
| it('should call cancelRecurringPaymentAgreement on GovPay when agreementId exists', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(govUkPayApi.cancelRecurringPaymentAgreement).toHaveBeenCalledWith(recurringPayment.agreementId) | ||
| }) | ||
|
|
||
| it('should not call cancelRecurringPaymentAgreement on GovPay when agreementId does not exist', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes sense to me - but also makes me think that if we aren't doing the cancellation in GOV.UK Pay, we probably shouldn't be doing the rest of the cancellation stuff either. So maybe this should be a condition that just throws an error for all of |
||
| const recurringPayment = getMockRecurringPayment({ agreementId: undefined }) | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
|
|
||
| await cancelRecurringPayment('id', 'Payment Failure') | ||
|
|
||
| expect(govUkPayApi.cancelRecurringPaymentAgreement).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should not throw when GovPay returns 404 (agreement already cancelled)', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it really return a 404 if the agreement has already been cancelled? Seems odd that we wouldn't be able to distinguish between a cancelled agreement and the agreement ID just being wrong |
||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 404, statusText: 'Not Found' }) | ||
|
|
||
| await expect(cancelRecurringPayment('id', 'User Cancelled')).resolves.toBeDefined() | ||
| }) | ||
|
|
||
| it('should still persist to CRM when GovPay returns 404', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 404, statusText: 'Not Found' }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
| expect(persist).toHaveBeenCalled() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we check what |
||
| }) | ||
|
|
||
| it('should not throw when GovPay returns 400 (agreement in invalid state)', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 400, statusText: 'Bad Request' }) | ||
|
|
||
| await expect(cancelRecurringPayment('id', 'User Cancelled')).resolves.toBeDefined() | ||
| }) | ||
|
|
||
| it('should still persist to CRM when GovPay returns 400', async () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to do this? I'm concerned this could mean the CRM would show a record as cancelled when the agreement might still be active in GOV.UK Pay (although then I guess we would still not be requesting the payment when it would have been due, so maybe it's the same result). How does the CRM handle this situation currently?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same goes for handling the 404 response above |
||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 400, statusText: 'Bad Request' }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
| expect(persist).toHaveBeenCalled() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we check what |
||
| }) | ||
|
|
||
| it('should throw when GovPay returns an unexpected error', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ | ||
| ok: false, | ||
| status: 500, | ||
| statusText: 'Internal Server Error', | ||
| text: jest.fn().mockResolvedValue('Server error') | ||
| }) | ||
|
|
||
| await expect(cancelRecurringPayment('id', 'User Cancelled')).rejects.toThrow('Failed to cancel GovPay agreement') | ||
| }) | ||
|
|
||
| it('should set isRecurringPayment to false on the linked permission', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(mockPermission.isRecurringPayment).toBe(false) | ||
| }) | ||
|
|
||
| it('should persist the linked permission alongside the recurring payment', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(persist).toHaveBeenCalledWith([expect.any(RecurringPayment), mockPermission]) | ||
| }) | ||
|
|
||
| it('should query dynamics for the active permission lookup value', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(dynamicsClient.retrieveRequest).toHaveBeenCalledWith({ | ||
| key: 'id', | ||
| collection: RecurringPayment.definition.dynamicsCollection, | ||
| select: ['_defra_activepermission_value'] | ||
| }) | ||
| }) | ||
|
|
||
| it('should look up the permission by the id returned from dynamics', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| dynamicsClient.retrieveRequest.mockResolvedValueOnce({ _defra_activepermission_value: 'specific-permission-id' }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(findById).toHaveBeenCalledWith(Permission, 'specific-permission-id') | ||
| }) | ||
|
|
||
| it('should still persist when no linked permission is found', async () => { | ||
| const recurringPayment = getMockRecurringPayment() | ||
| findById.mockReturnValueOnce(recurringPayment) | ||
| dynamicsClient.retrieveRequest.mockResolvedValueOnce({ _defra_activepermission_value: null }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
|
|
||
| expect(persist).toHaveBeenCalledWith([expect.any(RecurringPayment)]) | ||
| }) | ||
|
|
||
| it('should raise an error when there are no matches', async () => { | ||
| findById.mockReturnValueOnce(undefined) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { | |
| findDueRecurringPayments, | ||
| findRecurringPaymentsByAgreementId, | ||
| persist, | ||
| Permission, | ||
| RecurringPayment, | ||
| findRecurringPaymentByPermissionId, | ||
| retrieveGlobalOptionSets | ||
|
|
@@ -21,6 +22,7 @@ import { getGlobalOptionSetValue } from './reference-data.service.js' | |
| import moment from 'moment' | ||
| import { AWS, govUkPayApi } from '@defra-fish/connectors-lib' | ||
| import db from 'debug' | ||
| import { StatusCodes } from 'http-status-codes' | ||
| const debug = db('sales:recurring') | ||
| const { sqs, docClient } = AWS() | ||
|
|
||
|
|
@@ -170,21 +172,57 @@ export const cancelRecurringPayment = async (id, reason) => { | |
| const recurringPayment = await findById(RecurringPayment, id) | ||
| if (recurringPayment) { | ||
| const data = recurringPayment | ||
| const isUserCancelled = reason === 'User Cancelled' | ||
|
|
||
| if (!isUserCancelled) { | ||
| data.cancelledDate = new Date().toISOString().split('T')[0] | ||
| data.cancelledDate = new Date().toISOString().split('T')[0] | ||
| data.cancelledReason = await getGlobalOptionSetValue(RecurringPayment.definition.mappings.cancelledReason.ref, reason) | ||
|
|
||
| if (data.agreementId) { | ||
| await cancelGovPayAgreement(data.agreementId) | ||
| } | ||
|
|
||
| data.cancelledReason = await getGlobalOptionSetValue(RecurringPayment.definition.mappings.cancelledReason.ref, reason) | ||
| const updatedRecurringPayment = Object.assign(new RecurringPayment(), data) | ||
| await persist([updatedRecurringPayment]) | ||
| const entitiesToPersist = [updatedRecurringPayment] | ||
|
|
||
| const linkedPermission = await getLinkedPermission(id) | ||
| if (linkedPermission) { | ||
| linkedPermission.isRecurringPayment = false | ||
| entitiesToPersist.push(linkedPermission) | ||
| } | ||
|
|
||
| await persist(entitiesToPersist) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering what kind of logging/error handling we have if the cancellation fails at this stage, since would be awkward to have a situation where it's cancelled on GOV.UK Pay but not in the CRM. I think it'll go just into the queue and try to save again later, but might be worth doing some testing around it. |
||
| return updatedRecurringPayment | ||
| } else { | ||
| throw new Error('Invalid id provided for recurring payment cancellation') | ||
| } | ||
| } | ||
|
|
||
| const getLinkedPermission = async recurringPaymentId => { | ||
| const record = await dynamicsClient.retrieveRequest({ | ||
| key: recurringPaymentId, | ||
| collection: RecurringPayment.definition.dynamicsCollection, | ||
| select: ['_defra_activepermission_value'] | ||
| }) | ||
|
Comment on lines
+200
to
+204
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually we define these queries in dynamics-lib, then call them from the applications as needed through the |
||
| const permissionId = record._defra_activepermission_value | ||
| if (permissionId) { | ||
| return findById(Permission, permissionId) | ||
| } | ||
| return null | ||
| } | ||
|
|
||
| const cancelGovPayAgreement = async agreementId => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suspect this is just me being a massive pedant, but it's GOV.UK Pay, not GovPay - I know we've been a bit inconsistent about this, but I'd prefer to use the full name, especially if the connector is called govUkPayApi. Same goes for various console logs, test names, etc |
||
| const response = await govUkPayApi.cancelRecurringPaymentAgreement(agreementId) | ||
| if (response.ok) { | ||
| debug('Successfully cancelled GovPay agreement: %s', agreementId) | ||
| } else if (response.status === StatusCodes.NOT_FOUND) { | ||
| debug('GovPay agreement not found (already cancelled or does not exist): %s', agreementId) | ||
| } else if (response.status === StatusCodes.BAD_REQUEST) { | ||
| debug('GovPay agreement cannot be cancelled (invalid state): %s', agreementId) | ||
| } else { | ||
| const body = await response.text().catch(() => 'Unable to read response body') | ||
| throw new Error(`Failed to cancel GovPay agreement ${agreementId}: ${response.status} ${response.statusText} - ${body}`) | ||
| } | ||
| } | ||
|
|
||
| const determineRecurringPaymentName = (transactionRecord, contact) => { | ||
| const [dueYear] = transactionRecord.payment.recurring.nextDueDate.split('-') | ||
| return [contact.firstName, contact.lastName, dueYear].join(' ') | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to confirm what error message the spy has been called with