Persisting the results data from the two-part tariff match and allocate service #616
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
…te service https://eaflood.atlassian.net/browse/WATER-4188 As part of the work we have been doing, we've been hacking away at a branch in a mob to build the two-part-tariff service. As a result of this, we have some refactoring to do to tidy up the work we have done in our mob. We are keeping track of the refactoring to do in this issue DEFRA/water-abstraction-team#105. This PR will extract the `persistAllocatedLicencesToResultsService`out of our hacky branch and into its own PR along with the unit tests.
Cruikshanks
requested changes
Jan 8, 2024
db/migrations/public/20231122121546_create-review-charge-element-results.js
Show resolved
Hide resolved
app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Alan Cruikshanks <[email protected]>
Co-authored-by: Jason Claxton <[email protected]> Co-authored-by: Becky Ransome <[email protected]>
We can avoid bringing in an extra module and let the DB generate the ID by using [returning()](https://vincit.github.io/objection.js/api/query-builder/find-methods.html#returning). We initially avoided it because it gave the impression that it ignores what we ask the DB to return and instead returns everything. But [this issue](Vincit/objection.js#1773) and our own testing confirmed Objection.js is just combining what we passed in with what we asked the DB to return, which gives the impression the DB is sending back more than we wanted.
Cruikshanks
reviewed
Jan 8, 2024
app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js
Show resolved
Hide resolved
Co-authored-by: Alan Cruikshanks <[email protected]>
Cruikshanks
added a commit
that referenced
this pull request
Jan 27, 2024
Recently the team have found they repeatedly have to wipe the schemas, tables and views from their local `water_system_test` DB. We initially put it down to us changing migrations that might have already been run. > As a team we are happy for migrations that have yet to be run in production to be edited. Typically a migration is one of the first things created when working on a new feature but you don't know what you actually need until the feature is complete. But we now know it was when we started [Persisting the results data from the two-part tariff match and allocate service](#616). We added `public` to the list of schemas the database helper should clean but overlooked that is also where our migrations tables sit. So, we'd become locked in a cycle. - Attempt to run migrations and they fail (because they are trying to create things that already exist) - Delete schemas, and tables and views in `public` manually - Re-run migrations and all is well - Run unit tests which inadvertently delete migration records - All is well for awhile - Someone adds a new migration - Go back to step one This change fixes the issue by ensuring the Knex migration tables are ignored by the database helper when cleaning.
Cruikshanks
added a commit
that referenced
this pull request
Jan 27, 2024
Recently the team have found they repeatedly have to wipe the schemas, tables and views from their local `water_system_test` DB. We initially put it down to us changing migrations that might have already been run. > As a team we are happy for migrations that have yet to be run in production to be edited. Typically a migration is one of the first things created when working on a new feature but you don't know what you need until the feature is complete. But we now know it was when we started [Persisting the results data from the two-part tariff match and allocate service](#616). We added `public` to the list of schemas the database helper should clean but overlooked that is also where our migrations tables sit. So, we'd become locked in a cycle. - Attempt to run migrations and they fail (because they are trying to create things that already exist) - Delete schemas, then tables and views in `public` manually - Re-run migrations and all is well - Run unit tests which inadvertently delete migration records - All is well for awhile - Someone adds a new migration - Go back to step one This change fixes the issue by ensuring the Knex migration tables are ignored by the database helper when cleaning.
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-4188
As part of the work we have been doing, we've been hacking away at a branch in a mob to build the two-part-tariff service. As a result of this, we have some refactoring to do to tidy up the work we have done in our mob. We are keeping track of the refactoring to do in this issue DEFRA/water-abstraction-team#105. This PR will extract the
persistAllocatedLicencesToResultsServiceout of our hacky branch and into its own PR along with the unit tests.