Refactor ProcessBillingBatchService - first pass
#214
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
We've spotted ways we can refactor `ProcessBillingBatchService` to make it more performant. This is a first pass at this.
StuAA78
added
the
housekeeping
We start by moving the billing invoice generation outside of the main loop and updating `_generateInvoiceData` to simply look up the billing invoice instead of generating it.
We refactor our fetching of the invoice accounts into a separate `FetchInvoiceAccountsService`, which seperates out our logic for deduping invoice account ids and also allows us to stub it to test that the fetch error trapping works
Now that `GenerateBillingInvoiceService` no longer hits the db we no longer need to test that we error trap a rejected promise, so we remove the now-unneeded test
We previously put the invoice accounts in an array and used `.find()` to pull the required invoice account from it. We change this so that we now store the invoice accounts in a keyed object which simplifies retrieval; we can simply use `invoiceAccounts[invoiceAccountId]` to get the invoice account we want
Cruikshanks
requested changes
May 11, 2023
app/services/supplementary-billing/fetch-invoice-accounts.service.js
Outdated
Show resolved
Hide resolved
app/services/supplementary-billing/process-billing-batch.service.js
Outdated
Show resolved
Hide resolved
Since we only really need to get the invoice account numbers from the db, we rework `FetchInvoiceAccountsService` to only return the numbers along with the ids for later indexing. To avoid confusion we amend the service to be `FetchInvoiceAccountNumbers` which makes it very clear that what it returns isn't an array of `InvoiceAccountModel` instances but simply an array of objects which each contain the account number and id.
Cruikshanks
previously approved these changes
May 11, 2023
Jozzey
requested changes
May 11, 2023
test/services/supplementary-billing/fetch-invoice-account-numbers.service.test.js
Outdated
Show resolved
Hide resolved
StuAA78
added a commit
that referenced
this pull request
May 12, 2023
https://eaflood.atlassian.net/browse/WATER-3996 [After refactoring the `ProcessBillingBatchService` to fetch and store all invoice account numbers prior to the main loop](#214), we found an issue where this could result in multiple billing invoices being generated for a single invoice account due to us generating a billing invoice within the main loop, one for each charge version regardless of whether or not one was previously generated for the invoice account. We fix this by generating our billing invoices (one per invoice account) prior to entering the main loop. We then simply retrieve the pre-generated billing invoice within the loop.
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-3996
While reviewing
ProcessBillingBatchServicewe realised there is scope to make it more performant. This is a first pass which tackles the issue of us fetching invoice accounts one at a time within the main loop. We refactor so that we now fetch all the invoice accounts we will need in a single query prior to entering the main loop and hold them in a keyed object:This means that we can easily retrieve a given invoice account once we're in the main loop: