Tidy up two-part tariff review code and routes #1443
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
added a commit
that referenced
this pull request
Oct 24, 2024
https://eaflood.atlassian.net/browse/WATER-4057 We currently display a billing account's contact details when viewing a bill in a bill run. This is achieved by the `FetchBillingAccountService` and the `ViewBillPresenter`. We have to show the _exact_ same details in the _exact_ same format as part of reviewing the return match and allocation results in a two-part part tariff bill run. And we strongly suspect there will be more pages in the coming months that will also need to display this. We've already started a [tidy up of our two-part tariff review code](#1443). Moving both the fetch _and_ the presentation logic to somewhere reusable will help with that and put us in a better position for future changes. Previously, when we have needed to fetch various pieces of data in order to make a determination about something on a model we've done it on the model directly using modifiers and instance methods. This change refactors our existing billing account detail logic to follow this pattern.
Cruikshanks
added a commit
that referenced
this pull request
Oct 24, 2024
https://eaflood.atlassian.net/browse/WATER-4057 We currently display a billing account's contact details when viewing a bill in a bill run. This is achieved by the `FetchBillingAccountService` and the `ViewBillPresenter`. We have to show the _exact_ same details in the _exact_ same format when reviewing the return match and allocation results in a two-part tariff bill run. We strongly suspect that more pages will need to display this in the coming months. We've already started a [tidy up of our two-part tariff review code](#1443). Moving the fetch _and_ the presentation logic to somewhere reusable will help and put us in a better position for future changes. Previously, when we needed to fetch various pieces of data to make a determination about something on a model, we've done it on the model directly using modifiers and instance methods. This change refactors our existing billing account detail logic to follow this pattern.
This just updates the controllers and routes
This means we can use it to directly find the match instead of bill run and licence ID.
Cruikshanks
added a commit
to DEFRA/water-abstraction-acceptance-tests
that referenced
this pull request
Nov 7, 2024
https://eaflood.atlassian.net/browse/WATER-4739 For a detailed explanation of the tidy-up work, see [Tidy up two-part tariff review code and routes](DEFRA/water-abstraction-system#1443). In short, 12 months after starting the work, knowing what we know now, we'd have structured the code differently. That's tidy up work has affected some of the view, which means some of the tests are failing because they can no longer identify the elements to select. This change updates the review tests to get them working again.
The reason we added the formatter was because the acceptance tests highlighted we were doing a mix of things depending on - if the factors were set (not 1) - if the factors were set but they had been amended to 1 (they've been unset) Basically, they were hiding and displaying, and the logic was having to mix the original and amended values to control this behaviour. We've now simplified it. If you can amend the factors, we _always_ display them. Plus, we show the amended and original so the two values can be easily compared. This simplifies the logic, which is what allowed us to move a chunk of the 'adjustments' code into a shared formatter.
Either correcting element data attributes or adding them to support the acceptance tests.
Cruikshanks
added a commit
to DEFRA/water-abstraction-acceptance-tests
that referenced
this pull request
Nov 7, 2024
https://eaflood.atlassian.net/browse/WATER-4739 For a detailed explanation of the tidy-up work, see [Tidy up two-part tariff review code and routes](DEFRA/water-abstraction-system#1443). In short, 12 months after starting the work, we'd have structured the code differently, knowing what we know now. That tidy-up work has affected some of the views, which means some of the tests are failing because they can no longer identify the elements to select. This change updates the review tests to get them working again.
Cruikshanks
added a commit
that referenced
this pull request
Nov 9, 2024
https://eaflood.atlassian.net/browse/WATER-4739 Following on from our work in [Tidy up two-part tariff review code and routes](#1443) this change moves the two-part tariff match & allocate logic into it's own folder. This will hopefully help us distinguish what aspect of two-part tariff a module relates to. Unlike the previous change though, we're having to turn a blind eye to any issues or ways we can clean things up. Time is not on our side! So, it's just moving the modules.
Cruikshanks
added a commit
that referenced
this pull request
Nov 10, 2024
https://eaflood.atlassian.net/browse/WATER-4739 Following on from our work in [Tidy up two-part tariff review code and routes](#1443), this change moves the two-part tariff match and allocate logic into its own folder. This will hopefully help us distinguish what aspect of the two-part tariff engine a module relates to. Unlike the previous change, though, we're having to ignore any issues or ways to clean things up. Time is not on our side! So, it's just moving the modules.
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-4739
We have completed the annual billing engine for the two-part tariff. The matching and allocating engine, including the pages used to review the results, has been tested.
However, we started this work more than 12 months ago when our understanding of what was required was at 0%! We were also in the infancy of adding and working with pages in water-abstraction-system.
So, we made choices that, in hindsight, we would have done differently. Essentially, all our code to support reviewing a bill run is mixed in with the code to generate the matching and allocating results and the bill itself. From a maintenance viewpoint, it becomes hard to understand the context of the various modules because they have all been thrown into the same basket.
We’re now about to add a bunch more code to deliver WATER-4201 .
To ensure the project remains within our levels of tolerance for code maintenance, we need to do some prep work before starting WATER-4201 .
That prep work includes
We also include lots of refactoring and housekeeping, simplifying both the modules and their tests, and ensuring better consistency. We've also managed to correct some issues found, for example, inconsistency with nav menu selection, error bookmarks not working, and some errant spacing.