Include volume in FetchChargeVersionsService #586
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-4057 This is related to our work building a new SROC two-part tariff matching and allocating engine. We've been asked to make a change to the engine. Now when it allocates returns to charge elements it should take into account the authorised volume of the charge reference the charge element is under. For example, if we had the following scenario - `CR01 - authorised 100` - `CE01 - authorised 75` - `CE02 - authorised 75` - `RT01 - submitted 150` The original understanding was that we would be able to allocate all 150ML across the 2 charge elements. Now, when the total allocation across all charge elements within a charge reference reach the charge references authorised we have to stop. So, the result would be - `CR01 - authorised 100` - `CE01 - authorised 75 / allocated 75` - `CE02 - authorised 75 / allocated 25` - `RT01 - submitted 150 / allocated 100` In order to be able to do this we need to know what the authorised volume of the charge reference is. So, this change adds that to what we get back when calling `FetchChargeVersionsService`.
Cruikshanks
added
the
housekeeping
Where something is 'many', for example, an array of things it helps to use a plural variable name to denote there is more than one thing; `results`. When we are just dealing with a single things, for example, an instance of something then use a singular variable name; `result`.
We never change `billingPeriod` so we can make it a const at the top of the tests.
It's really just the regionID we need to set for data setup in later tests.
We create it within a number of the tests but don't actually amend any of its properties. This means we can just instantiate it once at the start of the tests. We also only use its ID which means we only need to hole `chargeCategoryId` as a variable.
We think it's a easier to follow the test and what exactly is being compared if the expectation is in the `expect()` directly. If we needed to check that a result was repeatedly a specific value then it makes sense to put that in a variable. But in this case we are just doing it the once. We also remove the interstitial variables which were being used to check values against. We assume this was to be able to check the ID's matched to confirm the right records were being linked. As we create the records we can set those without the need to hold things in variables.
Variables used throughout the tests and the main `beforeEach` that cleans the DB should be declared before any tests (directly under the top level `describe()`)
The main test at the top of the file already confirms this is the case. There is an alternate way though we can document what this test is trying to highlight but we'll cover that in subsequent changes.
The main test at the top of the file already confirms this is the case. Assuming we were testing this for 'documentation' purposes it was already covered implicitly. If the current test describes, 'and the start date' + 'is after the billing period end`, were combined with 'returns no records' it is implicit that if the start date is _before_ the billing period end then we would get results.
The main test at the top of the file already confirms this is the case.
By moving this test into a block where the same substantial test setup is already happening, and tweaking it a bit, we can cover the same scenario whilst removing a lot of duplicate setup code.
This was referred to in previous changes. We can document behaviour implicitly by explicitly testing certain cases. For example, if we explicitly test that if the region code is different we get no results, it is implicit that if the region code is the same then we do get results. Both cases are documented, and our generic 'if all things are good we get results' provides the cover to ensure this.
Yes, we know we've already done this one but we lost it in a copy and paste!
The test is right that if the scheme on the charge version doesn't match then no results would be returned. But you could set the scheme to SROC and still get no results due to the lack of also a charge reference set as 2PT.
We're using static guid's for the other elements so it's consistent to also do the same for the charge version.
The service has code to also return the purpose against each element. But this was not being set in the tests or checked for.
Spotted we were not checking the charge category we also include in the results. Not sure why Lab didn't flag this.
This is already defaulting to this date in the helper.
We were only creating a single charge version before so there is no way the test could be checking that the order of the charge versions depended on the reference of the licence they are linked to.
Copied the code, then made changes to the original, then pasted the copied code.
Jozzey
requested changes
Dec 15, 2023
test/services/bill-runs/two-part-tariff/fetch-charge-versions.service.test.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Jason Claxton <[email protected]>
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-4057
This is related to our work building a new SROC two-part tariff matching and allocating engine.
We've been asked to make a change to the engine. Now when it allocates returns to charge elements it should take into account the authorised volume of the charge reference the charge element is under.
For example, if we had the following scenario
CR01 - authorised 100CE01 - authorised 75CE02 - authorised 75RT01 - submitted 150The original understanding was that we would be able to allocate all 150ML across the 2 charge elements. Now, when the total allocation across all charge elements within a charge reference reaches the charge references authorised we have to stop. So, the result would be
CR01 - authorised 100CE01 - authorised 75 / allocated 75CE02 - authorised 75 / allocated 25RT01 - submitted 150 / allocated 100To be able to do this we need to know what the authorised volume of the charge reference is. So, this change adds to what we get back when calling
FetchChargeVersionsService.We also took the opportunity to update the existing unit tests to bring them in line with our conventions.