Update Process Batch service for debit only #133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Cruikshanks
force-pushed
the
spike-complete-debit-only-sroc-bill-run
branch
from
831f92c to
005a783
Compare
Cruikshanks
added a commit
that referenced
this pull request
Mar 2, 2023
https://eaflood.atlassian.net/browse/WATER-3906 We've spotted that we're not being consistent with our response handling of requests to the [sroc-charging-module-api](https://github.com/DEFRA/sroc-charging-module-api). We'd done some refactoring in [Refactor to use ChargingModuleRequestLib](#130) to the `_parseResult()` function of `app/lib/charging-module-request.lib.js`. The result was a calling service would receive something like ```javascript { succeeded: true, response: { body: { billRun: { id: ..., billRunNumber: ... } }, statusCode: ... } } ``` This started blowing up when we started work on the [Spike - Create a complete debit only SROC bill run](#133). This was because `app/services/supplementary-billing/initiate-billing-batch.service.js` was trying to access the result as below ```javascript const options = { scheme, batchType: type, externalId: chargingModuleBillRun.response.id } ``` And when it did blow up, our logs became spammed with hundreds of lines of [Got](https://github.com/sindresorhus/got) response info. To resolve this we came to the conclusion that, exceptions aside, what we want from **Got** is the response body as is, plus the status code. That's it. This applies to `2xx/3xx` and `4xx/5xx` responses. This change updates `app/lib/charging-module-request.lib.js` to ensure this. We could have done it in `app/lib/request.lib.js` but we will be using **Got** options that may not be applicable in all cases. So, by doing it at the `charging-module-request.lib.js` level we retain flexibility in how we make requests should we need it.
Cruikshanks
added a commit
that referenced
this pull request
Mar 3, 2023
https://eaflood.atlassian.net/browse/WATER-3906 When working on the [Spike - Create a complete debit only SROC bill run](#133) a request to the [sroc-charging-module-api](https://github.com/DEFRA/sroc-charging-module-api) failed, as in we got a `4xx/5xx` response. One of the things that made it hard to track down the cause was we are currently dumping the full response object [Got](https://github.com/sindresorhus/got) returns into the log. Should **Got** throw an exception, for example, when a network error occurs we'll want to see everything. But if the request fails because the Charging Module has returned an error response, we just want to see that information. So, this tweaks the logging in `RequestLib` to ensure this.
Cruikshanks
added a commit
that referenced
this pull request
Mar 3, 2023
https://eaflood.atlassian.net/browse/WATER-3906 We've spotted that we're not being consistent with our response handling of requests to the [sroc-charging-module-api](https://github.com/DEFRA/sroc-charging-module-api). We'd done some refactoring in [Refactor to use ChargingModuleRequestLib](#130) to the `_parseResult()` function of `app/lib/charging-module-request.lib.js`. The result was a calling service would receive something like ```javascript { succeeded: true, response: { body: { billRun: { id: ..., billRunNumber: ... } }, statusCode: ... } } ``` This started blowing up when we started work on the [Spike - Create a complete debit only SROC bill run](#133). This was because `app/services/supplementary-billing/initiate-billing-batch.service.js` was trying to access the result as below ```javascript const options = { scheme, batchType: type, externalId: chargingModuleBillRun.response.id } ``` To resolve this we came to the conclusion that, exceptions aside, what we want from [Got](https://github.com/sindresorhus/got) is the response body as is, plus the status code. That's it. This applies to `2xx/3xx` and `4xx/5xx` responses. This change updates `app/lib/charging-module-request.lib.js` to ensure this. We could have done it in `app/lib/request.lib.js` but we will be using **Got** options that may not be applicable in all cases. So, by doing it at the `charging-module-request.lib.js` level we retain flexibility in how we make requests should we need it. --- The bulk of this work involved tweaking `_parseResult()` to just return the things from the response we need, or so we thought! `app/services/health/info.service.js` expects to be able to access the headers returned from the Charging Module. They are 2 custom headers used to denote the specific version of the code running, and which Docker image is being used. So, we expanded our solution to include those as well. We also took the opportunity to make use of the **Got's** inbuilt functionality to do things like parse data to and from the external service, and formulate the full URL to use. This reduces the work we're doing, and the complexity of our solution. As part of this we cracked our confusion with `body` vs `json`. If you set the **Got** option [json:](https://github.com/sindresorhus/got/blob/main/documentation/2-options.md#json) then it _must_ be a JavaScript JSON object you set in the options and **Got** will handle the stringify. If you set [body:](https://github.com/sindresorhus/got/blob/main/documentation/2-options.md#body) then it _must_ be a string and you cannot use the `json:` option. Where we've had it wrong before is because we were either setting `body:` to a JSON object, or passing a stringified JSON object to `json:`. Other than that it was updating or fixing services that use `app/lib/charging-module-request.lib.js`.
Cruikshanks
added a commit
that referenced
this pull request
Mar 3, 2023
https://eaflood.atlassian.net/browse/WATER-3906 When working on the [Spike - Create a complete debit only SROC bill run](#133) a request to the [sroc-charging-module-api](https://github.com/DEFRA/sroc-charging-module-api) failed, as in we got a `4xx/5xx` response. One of the things that made it hard to track down the cause as we are currently dumping the full response object [Got](https://github.com/sindresorhus/got) returns into the log. Should **Got** throw an exception, for example, when a network error occurs we'll want to see everything. But if the request fails because the Charging Module has returned an error response, we just want to see that information. So, this tweaks the logging in `RequestLib` to ensure this. --- We also took advantage of making these changes to do some boy scout cleanup and remove the duplication across the `get()` and `post()` methods. Co-authored-by: Jason Claxton <30830544+Jozzey@users.noreply.github.com>
Cruikshanks
force-pushed
the
spike-complete-debit-only-sroc-bill-run
branch
2 times, most recently
from
8a069af to
8bb8949
Compare
Cruikshanks
added a commit
that referenced
this pull request
Mar 5, 2023
https://eaflood.atlassian.net/browse/WATER-3906 https://eaflood.atlassian.net/browse/WATER-3926 It is not a one-to-one mapping of transaction properties between our service and the [Charging Module API](https://github.com/DEFRA/sroc-charging-module-api). When we have data that we need to 'present' in a certain way we use the concept of presenters. As part of the [spike work](#133) we created one that 'presents' a WRLS transaction in the format needed to send as a request to the Charging Module API. This adds it to the project properly along with unit tests.
Cruikshanks
added a commit
that referenced
this pull request
Mar 5, 2023
https://eaflood.atlassian.net/browse/WATER-3906 https://eaflood.atlassian.net/browse/WATER-3926 It is not a one-to-one mapping of transaction properties between our service and the [Charging Module API](https://github.com/DEFRA/sroc-charging-module-api). When we have data that we need to 'present' in a certain way we use the concept of presenters. As part of the [spike work](#133) we created one that 'presents' a WRLS transaction in the format needed to send as a request to the Charging Module API. This adds it to the project properly along with unit tests.
Cruikshanks
force-pushed
the
spike-complete-debit-only-sroc-bill-run
branch
2 times, most recently
from
97200c0 to
59fa9b0
Compare
https://eaflood.atlassian.net/browse/WATER-3906 We have created the individual 'tools' needed to generate an SROC supplementary bill-run. By this, we mean the `billing_batch`, `billing_invoice`, `billing_invoice_licence` and `billing_transaction` records. Also, (though we're still not sure where it's used!) the event record. We have also worked to identify the charge versions to consider in an SROC supplementary bill-run, and how to communicate with the charging module API. Now it's time to pull it all together and create a complete bill-run, one that can be confirmed and sent in the WRLS service UI. The work to credit a previous bill run is still to be done. But we have everything we need to get us to a place where we folks can start testing and verifying debit-only bill-runs. So, we're starting by spiking a solution. We don't intend to merge this PR. But we need some space to work out how to hang together a solution from which we'll then extract our proper changes.
No longer needed
Cruikshanks
force-pushed
the
spike-complete-debit-only-sroc-bill-run
branch
from
b824de7 to
0e1eb16
Compare
Tested locally and working.
We know this service is going to be rewritten. But we need a working version so exploratory testing of the process can begin.
Cruikshanks
added
enhancement
|
The fact this is a spike and so we have added a bunch of |
Cruikshanks
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-3906
We have created the individual 'tools' needed to generate an SROC supplementary bill-run. By this, we mean the
billing_batch,billing_invoice,billing_invoice_licenceandbilling_transactionrecords. Also, (though we're still not sure where it's used!) the event record.We have also worked to identify the charge versions to consider in an SROC supplementary bill-run, and how to communicate with the charging module API. Now it's time to pull it all together and create a complete bill-run, one that can be confirmed and sent in the WRLS service UI.
The work to credit a previous bill run is still to be done. We also need to properly handle when a bill run is empty (no valid transactions were found) or the process errors. But we have everything we need to get us to a place where we folks can start testing and verifying debit-only bill-runs.
So, we've 'spiked' a solution and this change updates
ProcessBillingBatchServicewith it. There are no unit tests because we're still trying to decide on our final approach. But this change means testing can begin.