Implement alternate version of 2PT matching #458
Closed
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
alternate-2pt-engine
branch
from
ab42675 to
d877735
Compare
Cruikshanks
force-pushed
the
alternate-2pt-engine
branch
4 times, most recently
from
4f8fb4d to
d025848
Compare
https://eaflood.atlassian.net/browse/WATER-4057 We've had a number of PR's merged and changes made building up a new SROC two-part tariff matching engine. We need to match returns to licences, via charge versions, in order to generate the two-part tariff bills for them. We're not using the legacy code as a reference for 2 reasons - it works in the context of matching 'returns -> charging'. We're being asked to use 'charging -> returns' - it's rubbish 😬 So, as a team we're having to build up our knowledge of how the matching should work and the engine itself as we go. However, the latest tranch of stories has asked us to deal with the scenario of allocating a return _across_ multiple charge references. This threw a spanner in the works because our current engine starts by matching returns to charge references. If a return is matched to more than one charge reference our current engine won't handle it. Out of this we've had further discussions about what it is the engine actually needs to do. Added to that we have a clearer idea of what the new returns review screens need in order to work. We've taken this information away, mulled it over, and iterated an alternate two-part tariff matching engine. It has been built with these factors in mind - returns must be matched at charge element level - a return is matched to a charge element by code (purpose) and overlapping abstraction period (it doesn't have to be exact) - a return can match and be allocated to one or more elements - we care about elements with no matches. We don't care about returns with no matches - we want to know about issues with the element - we want to know which returns were matched to an element - a return should be matched to the charge reference with the highest subsistence charge first - a return should be matched to the charge element with the highest authorised quantity first - we match a return's lines to a charge element based on the charge element's abstraction period (does the line's start and end date overlap the element's abstraction period) This isn't the final version of the engine. We've focused on making it work rather than clean, simple code and its output has been designed for testing purposes. But hopefully this version covers all the scenarios we now know about and is better able to handle those we don't!
Realised too late that what we get back from determine abstraction period service is an array of periods, not a single value! 🤦 So, needed to update the code to account for that resulting in a quick hack to the `_periodsOverlap()`. Whilst doing that spotted we had a glaring mistake in the old logic as well (startDate > startDate - 🤦🤦🤦)
Had overlooked updating the line matching use of _periodsOverlap()
Where a return might be allocated to multiple elements it is handy to see exactly which lines got allocated whilst we are verifying the engine.
Whilst we're testing the engine we tend to focus on a licence at a time. This means we either go searching through the output or we add a temporary clause to the main query. It'd be a lot easier if we could just ask for a single licence to be matched and allocated!
We have 'over allocated' and 'nothing allocated'. 'under allocated' felt more consistent.
This will just help us more easily line up the response with the expected behaviour in our scenarios.
In testing our scenarios we found we would need to allocate lines to more than 1 charge elements. So, with this change we enable part allocating a line to a charge element.
When we had fully allocated a line for one charge element we were setting unallocated to 0. If the return matched another element we'd then check `if (!line.unallocated)` intending only to match those lines which as yet had not been touched. Problem is `line.unallocated` also returns false, so we were resetting unallocated and allocating the line again.
Doh!
`'myProp' in myObject` appears to be the consensus for checking if a property exists nowadays. However, we need to negate. We prefer the simple approach to `if (!('myProp' in myObject))`.
We're assigning 'test IDs' to our records, including the returns to allow us to validate the output to a spreadsheet of scenarios we are now maintaining. But we realised we were not ordering the returns when querying for them, which means on different runs the order, and therefore the test IDs we assign could be different. So, we apply an order using the return period start date then the return requirement. This is the order we want to 'offer up' returns to elements in the matching anyway.
This came when we spotted that unmatched returns were not getting test IDs assigned to there lines. To sort this meant moving the test ID assignment into the prep returns method. After doing that it made sense to also move creating the `unallocated` property there as well which meant we could ditch our funky logic.
Co-authored-by: Becky Ransome <[email protected]>
Co-authored-by: Becky Ransome <[email protected]>
The endpoint for a single licence stays the same `http://localhost:8013/check/two-part-review/b690be23-3d05-4179-92cf-ba98743ad887`. But now if you remove the licence ID all the results for all the licences in the region that is hardcoded into the controller will be returned.
Co-authored-by: Becky Ransome <[email protected]>
|
All the changes needed from this PR have now been brought into main. We can close this spike PR down now. |
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
We've had a number of PRs merged and changes made building up a new SROC two-part tariff matching engine.
We need to match returns to licences, via charge versions, in order to generate the two-part tariff bills for them. We're not using the legacy code as a reference for 2 reasons.
So, as a team, we're having to build up our knowledge of how the matching should work and the engine itself as we go. However, the latest tranche of stories has asked us to deal with the scenario of allocating a return across multiple charge references. This threw a spanner in the works because our current engine starts by matching returns to charge references. If a return is matched to more than one charge reference our current engine won't handle it.
Out of this, we've had further discussions about what it is the engine actually needs to do. Added to that we have a clearer idea of what the new returns review screens need in order to work. We've taken this information away, mulled it over, and iterated an alternate two-part tariff matching engine.
It has been built with these factors in mind
This isn't the final version of the engine. We've focused on making it work rather than clean, simple code and its output has been designed for testing purposes. But hopefully, this version covers all the scenarios we now know about and is better able to handle those we don't!