Fix error calculating auth. and billable days #156
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-3906 In the first round of testing we came across a scenario where our `FormatSrocTransactionLineService` service calculated the `authorisedDays` and `billableDays` as `null`. This change fixes the issue.
Plus avoids confusion with the actual billableDays value.
Again, trying to move away from the use of `billable` to something more generic.
Previously we were adding properties to our abstraction period objects to be used in subsequent methods. It's not red line, but it is something you try and avoid. With this refactoring we avoid mutating the abstraction periods and act on the results of the methods directly.
We think we don't need these elements any more. So, we are commenting them out before we start running it through our unit tests to confirm if that is the case.
These will be rewritten to use our ace scenarios!
Our function for calculating abstraction periods didn't work when the reference period started and ended the same year. We have amended the logic so that instead of checking the start and end day/month and creating dates accordingly, we simply create a start and end date using the reference period's start year, and fix the end date's year if needed. Doing this means we also need to create an additional abstraction period to consider but we will look at refactoring to only do this if necessary
As per our team style, we split the one-line filter onto two lines
Our tests were breaking due to changes we'd made -- we now pass in the billing period instead of the financial year, so we simply change what we pass in our tests to get them working again
Jozzey
previously approved these changes
Mar 8, 2023
test/services/supplementary-billing/calculate-authorised-and-billable-days.service.test.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Claxton <[email protected]>
Cruikshanks
added a commit
that referenced
this pull request
Mar 9, 2023
https://eaflood.atlassian.net/browse/WATER-3906 Having implemented our [Fix error calculating auth. and billable days](#156) we clocked that we were calculating charge periods, option flags, and the authorised and billable days multiple times, when they do not change between iterations of the charge elements in a charge version. We only want to be executing our services when needed as it reduces complexity and the chance of an error occurring. It also means we get a slight performance bump. So, this change refactors how the `ProcessBillingBatchService` and the `FormatSrocTransactionLineService` interact together to create the transaction lines for a billing batch.
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
In the first round of testing, we came across a scenario where our
FormatSrocTransactionLineServiceservice calculated theauthorisedDaysandbillableDaysasnull.After some investigation we realised
AbstractionBillingPeriodServicewould take a billing period and a charge purpose and return 1 or 2 abstraction periods with the billable days calculated. But it didn't take into account any overlap between the abstraction periods it determined. It also didn't account for any overlap across all the charge purposes a charge element has.FormatSrocTransactionLineServicewas doing its own logic for determining the abstraction period. It was also looking across the charge purposes and dealing with any overlap. But when determining the abstraction period it hadn't accounted for something like 01-JAN to 30-JUN. A period like that needs to be factored in twice, especially when determining the authorised days. This is because if, for example, the billing period is 01-APR-2023 to 31-MAR-2023 it overlaps the first few months and the last few months.We realised we need to combine the logic of
AbstractionBillingPeriodServicewithFormatSrocTransactionLineServiceuse ofConsolidateDateRangesService.So, this change adds
CalculateAuthorisedAndBillableDaysService, which completely replacesAbstractionBillingPeriodServiceand the logic inFormatSrocTransactionLineService.This is the most complex bit of the whole process (that we've encountered) and we're hopeful we now have it nailed. This change also adds lots of documentation to try and help our future selves! 😀