Update 2PT check endpoint with alternate changes #582
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
Cruikshanks
added
the
housekeeping
Cruikshanks
force-pushed
the
update-check-two-part-service
branch
from
638ad31 to
6651ee3
Compare
|
I need #586 to be approved and merged for the unit tests in this change to pass 😁 |
https://eaflood.atlassian.net/browse/WATER-4057 Way, way back we built our very first iteration of a two-part tariff 'check' endpoint. The intent was to have some way for our QA team to being verifying how we were matching and allocating returns was correct. Along the journey we realised we needed to make major shift in how things were working which has since become known as the [alternate engine](#458). This started out as a 'spike' (does _this_ look right?), then was deployed on the branch to environments for double checking, was tweaked, tweaked a bit more, tweaked again, new requests catered for etc. _Now_ we're trying to figure out how we migrate from our hack to 'production-ready' unit tested code, still in the same branch. Recently, we lost the original alternate engine changes we made to the 'check' endpoint on that branch when we merged in `main`. That became the trigger to move out of the [alternate-2pt-engine](https://github.com/DEFRA/water-abstraction-system/tree/alternate-2pt-engine) branch those changes which are just related to the check endpoint and what the QA team need to keep an eye on things. This should mean we don't lose those changes again. Also, the team focused on refactoring can differentiate their changes from stuff we've been doing to keep the QA's happy! 😁 So, this change updates the 'check' endpoint to be the version we've actually been using and testing against. This includes what was needed to get it to work with our recent model changes. > When two-part tariff is completed, hopefully in the near future, we will delete all of this! 😈
This includes a feature we added to verify the matching & allocating for a single licence.
We need to tweak it to add `volume` to the charge reference instances it returns. But we have already made the same change to support allocating only up to the charge reference's authorised volume.
We don't care any more about timing in the verify version of the engine. We know it is fast enough and what we are doing here is not actually what we are doing in the 'production-ready' version we're now building.
The original intent of this endpoint was to verify the matching & allocating engine we were building. As time has gone (and thinking more of our work on the long lived [Implement alternate version of 2PT matching](#458) branch) though doing the same thing the check engine and actual engine were completely separate. This check endpoint should be using the same engine we are building. So, with this change we remove the duplicate elements, migrate to using the services the refactoring has built and merged into `main`, and use 'stand-ins' for those still in-flight. It'll mean as the real services are built and merged in we'll need to keep coming back and deleting the stand-ins. Plus what we have in `/check` becomes more about making the output match the scenario format. But it'll be more 'true'.
The real service won't do this because we don't care. But the verification service does, if only so we can match the scenario format. If we don't record which line matched to which return at the time of processing there is no way to be certain after the fact. So, for now we hack our stand-in service. But when the real one gets merged in and we start using it we'll just have to accept you won't be able to see this information in the verification output any more.
It got merged in whilst working on this branch!
The issues weren't being determined correctly because we now name a property `returnLogs` instead of `returns`.
With [Exclude ended licences in two-part tariff matching](#595) being merged we should always have a populated charge period so there is no need for the guard clause any more.
Cruikshanks
force-pushed
the
update-check-two-part-service
branch
from
e00cb5c to
e612e6b
Compare
Cruikshanks
added a commit
that referenced
this pull request
Dec 15, 2023
https://eaflood.atlassian.net/browse/WATER-4057 When we were updating the existing check endpoint to include changes made to support the alternate version of the matching and allocating engine (see [Update 2PT check endpoint with alternate changes](#582)) we confirmed our working by constantly pinging the `/check/two-part-licence/{licenceId}` endpoint. We overlooked checking `/check/two-part/{naldRegionId}` (in our defence this is a hack endpoint!) If we had we would have found that it is broken. This change fixes it.
Cruikshanks
added a commit
that referenced
this pull request
Dec 15, 2023
https://eaflood.atlassian.net/browse/WATER-4057 When we were updating the existing check endpoint to include changes made to support the alternate version of the matching and allocating engine (see [Update 2PT check endpoint with alternate changes](#582)) we confirmed our working by constantly pinging the `/check/two-part-licence/{licenceId}` endpoint. We overlooked checking `/check/two-part/{naldRegionId}` (in our defence this is a hack endpoint!) If we had we would have found that it is broken. This change fixes it.
Cruikshanks
added a commit
that referenced
this pull request
Jan 22, 2024
https://eaflood.atlassian.net/browse/WATER-4339 This follows on from [Update 2PT check endpoint with alternate changes](#582) where we applied the services we refactored out of our 2PT spike branch to the `/check` endpoint. The final set of services have now been refactored and added to `main`, so we can complete the work of replacing the stand-ins currently being used.
Cruikshanks
added a commit
that referenced
this pull request
Jan 25, 2024
https://eaflood.atlassian.net/browse/WATER-4339 This follows on from [Update 2PT check endpoint with alternate changes](#582) where we applied the services we refactored out of our 2PT spike branch to the `/check` endpoint. The final set of services have now been refactored and added to `main`, so we can complete the work of replacing the stand-ins currently being used.
Cruikshanks
added a commit
that referenced
this pull request
Jan 25, 2024
https://eaflood.atlassian.net/browse/WATER-4339 This follows on from [Update 2PT check endpoint with alternate changes](#582) where we applied the services we refactored out of our 2PT spike branch to the `/check` endpoint. The final set of services has now been refactored and added to `main`, so we can update the stand-ins service. We can also update the `TwoPartService` to use the new way of working, iterating all licences in one place and passing each licence to the new services. --- We can drop the `PrepareLicences` from stand-ins and use the real one. However, when verifying the results we find it useful to see which lines were allocated to which elements. This is not something we care about when generating the review results for real. The problem is this can only happen deep inside the allocation process. We don't want to add toggles to the production service just for this. We're on the cusp of having the pages ready to go at which point we expect the `/check` endpoint to become defunct. So, for now, we just copy the real `AllocateReturnsToChargeElementService` into our stand-ins and add the tweak which captures which lines were allocated to it.
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
Way, way back we built our very first iteration of a two-part tariff 'check' endpoint. The intent was to have some way for our QA team to begin verifying how we were matching and allocating returns correctly. Along the journey, we realised we needed to make a major shift in how things were working which has since become known as the alternate engine.
This started as a 'spike' (does this look right?), then was deployed on the branch to environments for double checking, was tweaked, tweaked a bit more, tweaked again, new requests catered for etc. Now we're trying to figure out how we migrate from our hack to 'production-ready' unit-tested code, still in the same branch.
Recently, we lost the original alternate engine changes we made to the 'check' endpoint on that branch when we merged in
main. That became the trigger to move out of the alternate-2pt-engine branch those changes which are just related to the check endpoint and what the QA team need to keep an eye on things.This should mean we won't lose those changes again. Also, the team focused on refactoring can differentiate their changes from stuff we've been doing to keep the QA's happy! 😁
So, this change updates the 'check' endpoint to be the version we've been using and testing against. This includes what was needed to get it to work with our recent model changes.