Implement SROC invoice reissuing #256
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
https://eaflood.atlassian.net/browse/WATER-4036 We have created the new services we need to send the appropriate requests to the Charging Module. We now implement invoice reissuing for SROC.
We will be adding our new service into the existing process flow with the intention that it will return a Boolean to state whether any changes were made, similar to how we currently have a `isBatchPopulated` boolean. We therefore simplify the existing logic to remove all conditionality, and instead put all our results in an array which we then check for any `true` results. This means when we introduce our service it's simply one more result to add to the array with no additional logic required. We will also be timing the reissuing stage separately from the current processing stage, so we make a small change to the point where we get the start time of the process so we can insert our new service before it.
We noticed that `ProcessBillingBatchService` was missing docs. We quickly add some in as a placeholder to flesh out later
We we creating our new entities on the original billing batch and not the new one! We update our code to fix this
We were simply using the `isCredit` of the source transaction instead of checking if it should be flipped to cancel out the original. We therefore fix this
When an invoice has been rebilled, we mark its rebilling state as `rebilled` and update its original billing invoice id to either its own value, or leave it as the existing value. We add in this step and ensure it's tested
If getting invoices fails, we silently swallow the error and return `false`, as we can continue with the rest of the billing process. While setting up the initial tests we were also getting invoices outside of the try/catch to ensure any errors in this stage would show up in our terminal. We now have enough unit tests in place that we no longer need this so we remove it. We also add a TODO to consider adding logging of errors here
Our test CM data included a lot of data returned by the CM but of no use to our tests. We therefore cut it for brevity
We update our unit tests to count the number of invoices, invoice licences and transactions we persist
We update our "retrieve or generate billing invoice" function to correctly generate one billing invoice for each combination of invoice account id and external id. This is correct because we have nested iteration of source invoice and CM reissue invoice which therefore requires a separate billing invoice for each iteration; previously we were only creating one billing invoice per invoice account id which didn't account for the CM reissue invoice iteration. While doing this we also refactor the function (along with the "retrieve or generate billing invoice licence" function) for clarity
The first thing we do is fetch the invoices to be reissued from the db, and if there are none then we return `false` so we can continue with processing the billing batch. If fetching invoices throws an error for some reason then we are okay to simply log the error and return `false` so we can continue, since no changes have yet been made (either to the db or to the Charging Module data) that require the billing batch to be marked as errored. This was being done in a top-level try/catch, which caused some clutter and slightly hid what was going on. We move this down into the `_fetchInvoicesToReissue` function so at the top level we simply handle it as "if there are no invoices to reissue, return false".
To simplify the `go` function, we move the bulk of the main loop into a separate `_handleSourceInvoice()` function. This is anticipation of moving it into an entirely separate service.
`ReissueInvoicesService` is currently doing a lot -- fetching invoices to be reissued, processing them and persisting them. To aid separation of concerns we split the fetching invoices into its own service and the processing of invoices into another. This leaves `ReissueInvoicesService` with the job of calling the service that fetches the invoices; iterating over them to pass them across to our new `ReissueInvoiceService` and collating the returned data; and persisting all data at the end of the process. Breaking the work up in this way also makes our testing easier as we can more easily stub what we need to. Note that at this stage we haven't yet divided up the tests.
We previously made changes to `ProcessBillingBatchService` in advance of us calling our reissuing service from it. However this doesn't really belong in this PR. We therefore revert the changes in anticipation of re-adding them in our future "add reissuing to billing batch processing" PR
As pointed out in review, we don't need the original billing batch at all. We can infer the region we're interested in from the target region; and we were incorrectly checking the original billing batch for CM invoices. We therefore remove all uses of it from our code.
Cruikshanks
requested changes
Jul 2, 2023
The services for sending requests to the Charging Module will never themselves throw an error; instead the response they return will set `succeeded` to `false`. We therefore dispense with the try-catch blocks around the calls and instead check `succeeded` to see whether or not there was an error
For consistency within our service we rename `transactions` to `billingTransactions`
In review it was commented that when we add the billing invoices, invoice licences and transactions in `newData` to `dataToPersist` the iteration was obfuscating what was happening. We therefore replace and add the data directly.
Remove unneeded param from docs Co-authored-by: Alan Cruikshanks <[email protected]>
The variable name `chargingModuleReissueResponses` was confusing as it implies multiple responses coming back from the CM, when instead it is a single response comprised of multiple objects with reissue invoice details. We therefore rename it to `chargingModuleReissueResponse` (singular) and where we previously referred to each element of it as `chargingModuleReissueResponse`, we now use `chargingModuleReissueInvoiceInfo` to better reflect what we're looking at
Cruikshanks
requested changes
Jul 4, 2023
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
The Charging Module does the work of determining whether transactions are credits or debits. We can therefore refactor the service so that instead of working this out for ourselves based on whether we're looking at the cancelling invoice or the reissuing invoice, we can simply use the transaction's credit flag.
We refactor the unit tests to create entries in the db instead of simply creating models, and add some additional tests.
Cruikshanks
requested changes
Jul 6, 2023
test/services/supplementary-billing/reissue-invoice.service.test.js
Outdated
Show resolved
Hide resolved
…st.js Co-authored-by: Alan Cruikshanks <[email protected]>
Cruikshanks
added a commit
that referenced
this pull request
Jul 6, 2023
When working on [Implement SROC invoice reissuing](#256) we had to deal with a situation where we wanted to throw an error that contained details of the records in the current context. But we were faced with the issue that we knew the error would eventually find it's way to our notifiers. If we include the details in the error message we break Errbit's ability to group instances of an error. But the standard [Error constructor](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error) doesn't support passing in additional values. To avoid making things complicated for ourselves we did this ```javascript const error = new Error('Charging Module reissue request failed') error.billingBatchExternalId = billingBatchExternalId error.invoiceExternalId = invoiceExternalId throw error ``` We know both Pino our logger will include these extra properties in its output and Errbit can still do its stuff. Then when working on [Fix omfg() in HandleErroredBillingBatch](#295) we did a review of all our `omfg()` calls. This led us to find this in `InitiateBillingBatchService` ```javascript if (liveBillRunExists) { throw Error(`Batch already live for region ${regionId}`) } ``` This is an example of an error message that will block Errbit from grouping these errors. We could fix this by doing what we did in `ReissueInvoiceService`. But it's probably time to add our own custom error that allows us to do it properly. This change adds the error and applies it where applicable.
Cruikshanks
added a commit
that referenced
this pull request
Jul 6, 2023
When working on [Implement SROC invoice reissuing](#256) we had to deal with a situation where we wanted to throw an error that contained details of the records in the current context. But we were faced with the issue that we knew the error would eventually find its way to our notifiers. If we include the details in the error message we break Errbit's ability to group instances of an error. But the standard [Error constructor](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Error) doesn't support passing in additional values. To avoid making things complicated for ourselves we did this ```javascript const error = new Error('Charging Module reissue request failed') error.billingBatchExternalId = billingBatchExternalId error.invoiceExternalId = invoiceExternalId throw error ``` We know both Pino our logger will include these extra properties in its output and Errbit can still do its stuff. Then when working on [Fix omfg() in HandleErroredBillingBatch](#295) we did a review of all our `omfg()` calls. This led us to find this in `InitiateBillingBatchService` ```javascript if (liveBillRunExists) { throw Error(`Batch already live for region ${regionId}`) } ``` This is an example of an error message that will block Errbit from grouping these errors. We could fix this by doing what we did in `ReissueInvoiceService`. But it's probably time to add our own custom error that allows us to do it properly. This change adds the error and applies it where applicable.
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-4036
We have created the new services we need to send the appropriate requests to the Charging Module. We now implement invoice reissuing for SROC.
This is done with
ReissueInvoicesService, which callsFetchInvoicesToBeReissuedServiceto get the invoices for reissuing then passes each one toReissueInvoiceService(note singularInvoice) which handles the actual reissuing of the invoice. The resulting invoices, invoice licences and transactions are persisted, and atruevalue returned to indicate that db changes have been made.The intent is that this service will be called from
ProcessBillingBatchService, and its return value used to set theisBatchPopulatedboolean. This PR initially made some changes toProcessBillingBatchServiceto accommodate this; however due to a merge conflict (and the fact that the changes don't really belong in this PR) the changes in theRefactor to accommodate future servicecommit have been reverted. They will be reinstated in the future PR which adds reissuing to the process billing batch process.