Cancel agreement via Sales API#2358
Conversation
https://eaflood.atlassian.net/browse/IWTF-5037 We want to notify GovPay when an agreement is cancelled So that we’re not relying on the CRM for this logic It’s done when we don’t have special handling in packages/sales-api-service/src/services/recurring-payments.service.js for omitting the cancellation date when it’s user cancelled cancellation is initiated by a POST from the Sales API to GovPay API unless by refund using the 3-step CRM process all possible responses from Gov Pay are handled appropriately
|
|
|
||
| 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') |
There was a problem hiding this comment.
Can we add a test to make sure this validates now?
| throw new Error('cancel error') | ||
| }) | ||
| await expect(govUkPayApi.cancelRecurringPaymentAgreement(123)).rejects.toEqual(Error('cancel error')) | ||
| expect(consoleErrorSpy).toHaveBeenCalled() |
There was a problem hiding this comment.
Would prefer to confirm what error message the spy has been called with
| return null | ||
| } | ||
|
|
||
| const cancelGovPayAgreement = async agreementId => { |
There was a problem hiding this comment.
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
| entitiesToPersist.push(linkedPermission) | ||
| } | ||
|
|
||
| await persist(entitiesToPersist) |
There was a problem hiding this comment.
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.
| expect(govUkPayApi.cancelRecurringPaymentAgreement).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should not throw when GovPay returns 404 (agreement already cancelled)', async () => { |
There was a problem hiding this comment.
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
| expect(govUkPayApi.cancelRecurringPaymentAgreement).toHaveBeenCalledWith(recurringPayment.agreementId) | ||
| }) | ||
|
|
||
| it('should not call cancelRecurringPaymentAgreement on GovPay when agreementId does not exist', async () => { |
There was a problem hiding this comment.
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 cancelRecurringPayment() instead
| await expect(cancelRecurringPayment('id', 'User Cancelled')).resolves.toBeDefined() | ||
| }) | ||
|
|
||
| it('should still persist to CRM when GovPay returns 400', async () => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Same goes for handling the 404 response above
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 400, statusText: 'Bad Request' }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
| expect(persist).toHaveBeenCalled() |
There was a problem hiding this comment.
Can we check what persist() is being called with?
| govUkPayApi.cancelRecurringPaymentAgreement.mockResolvedValueOnce({ ok: false, status: 404, statusText: 'Not Found' }) | ||
|
|
||
| await cancelRecurringPayment('id', 'User Cancelled') | ||
| expect(persist).toHaveBeenCalled() |
There was a problem hiding this comment.
Can we check what persist() is being called with?
| const record = await dynamicsClient.retrieveRequest({ | ||
| key: recurringPaymentId, | ||
| collection: RecurringPayment.definition.dynamicsCollection, | ||
| select: ['_defra_activepermission_value'] | ||
| }) |
There was a problem hiding this comment.
Usually we define these queries in dynamics-lib, then call them from the applications as needed through the dynamicsClient. Have a look at findLinkedRecurringPayment() further down in this file for an example of the pattern in action.
2 participants
https://eaflood.atlassian.net/browse/IWTF-5037
We want to notify GovPay when an agreement is cancelled so that we’re not relying on the CRM for this logic