Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor to use new view based WATER models #569

Merged
merged 12 commits into from
Dec 15, 2023

Conversation

Cruikshanks
Copy link
Member

@Cruikshanks Cruikshanks commented Dec 5, 2023

https://eaflood.atlassian.net/browse/WATER-4057

As part of the work we have been doing on two-part tariff, we will create all our new tables in the default public schema.

We have also decided that when there is a legacy table that we are still going to need we will create a View of it in the public schema. This allows us to correct any issues with naming conventions, strip out unused fields, and join entities currently sat in different schemas. The first example of this approach was done in PR #531 .

We created the new views in Create WATER schema views. Then in Create WATER schema models & helpers we added the new models and helpers that use them.

This is the final step in the process, we are refactoring any use of the legacy models to use the new ones and deleting all the legacy-based code.

@Cruikshanks Cruikshanks added the housekeeping Refactoring, tidying up or other work which supports the project label Dec 5, 2023
@Cruikshanks Cruikshanks self-assigned this Dec 5, 2023
Cruikshanks added a commit that referenced this pull request Dec 8, 2023
https://eaflood.atlassian.net/browse/WATER-4057

To make the work on two-part tariff and other areas of the service easier in the future we have been undergoing a large scale refactor of our models. We have realised we can make it seem to the project that we have just one schema, and that everything sits in `public` by using [updatable views](https://www.postgresql.org/docs/current/sql-createview.html) for any legacy table we have to deal with.

This has led to a whole refactor of the models so they are based on these new views rather than the legacy tables. During [Refactor to use new view based WATER models](#569), the last stage in refactoring, we hit the point we'd have to make a whole series of changes to the services that support generating mock data. For context, we created this to help support the [re-design of the view bill pages](https://eaflood.atlassian.net/browse/WATER-4131). Our design team needed to prototype and test a page that replicated the scenario we were trying to fix; a bill with more than 150 licences linked to it.

We created the mock endpoint to generate data that could be copied & pasted by the design team into the prototype. The thought was this would be 'a thing' we'd be asked again and again for in the future. In reality, we've only had 1 additional request, and that was to [generate returns review data based on how the old engine does it](#447). But this was needed to have something to design the new pages around. We've never got round to merging it because it will require a lot of unit testing, for something that will never be used again.

We've come to the conclusion generating mock data at best is only ever going to be a one-off exercise. There will be times we need code to generate complex 'data' (bill runs, charge versions, returns etc) in enough quantity to make tests in the prototype realistic. But once we've generated the data there will never be any additional requests. There is no point maintaining these endpoints and increasing our workload.

So, this change removes the existing mock endpoint and functionality.
Cruikshanks added a commit that referenced this pull request Dec 8, 2023
https://eaflood.atlassian.net/browse/WATER-4057

To make the work on two-part tariff and other areas of the service easier in the future we have been undergoing a large-scale refactor of our models. We have realised we can make it seem to the project that we have just one schema, and that everything sits in `public` by using [updatable views](https://www.postgresql.org/docs/current/sql-createview.html) for any legacy table we have to deal with.

This has led to a whole refactor of the models based on these new views rather than the legacy tables. During [Refactor to use new view-based WATER models](#569), the last stage in refactoring, we hit the point where we'd have to make a whole series of changes to the services that support generating mock data. For context, we created this to help support the [re-design of the view bill pages](https://eaflood.atlassian.net/browse/WATER-4131). Our design team needed to prototype and test a page that replicated the scenario we were trying to fix; a bill with more than 150 licences linked to it.

We created the mock endpoint to generate data that could be copied & pasted by the design team into the prototype. The thought was this would be 'a thing' we'd be repeatedly asked for in the future. In reality, we've only had 1 additional request, and that was to [generate returns review data based on how the old engine does it](#447). But this was needed to have something to design the new pages around. We've never got around to merging it because it will require a lot of unit testing, for something that will never be used again.

We've come to the conclusion generating mock data at best is only ever going to be a one-off exercise. There will be times we need code to generate complex 'data' (bill runs, charge versions, returns etc) in enough quantity to make tests in the prototype realistic. However once we've generated the data, there will be no additional requests. There is no point in maintaining these endpoints and increasing our workload.

So, this change removes the existing mock endpoint and functionality.
@Cruikshanks Cruikshanks force-pushed the refactor-to-use-new-water-models branch from 2e422aa to 2c9e05d Compare December 8, 2023 17:14
Beckyrose200 pushed a commit that referenced this pull request Dec 14, 2023
https://eaflood.atlassian.net/browse/WATER-4057

To make the work on two-part tariff and other areas of the service easier in the future we have been undergoing a large-scale refactor of our models. We have realised we can make it seem to the project that we have just one schema, and that everything sits in `public` by using [updatable views](https://www.postgresql.org/docs/current/sql-createview.html) for any legacy table we have to deal with.

This has led to a whole refactor of the models based on these new views rather than the legacy tables. During [Refactor to use new view-based WATER models](#569), the last stage in refactoring, we hit the point where we'd have to make a whole series of changes to the services that support generating mock data. For context, we created this to help support the [re-design of the view bill pages](https://eaflood.atlassian.net/browse/WATER-4131). Our design team needed to prototype and test a page that replicated the scenario we were trying to fix; a bill with more than 150 licences linked to it.

We created the mock endpoint to generate data that could be copied & pasted by the design team into the prototype. The thought was this would be 'a thing' we'd be repeatedly asked for in the future. In reality, we've only had 1 additional request, and that was to [generate returns review data based on how the old engine does it](#447). But this was needed to have something to design the new pages around. We've never got around to merging it because it will require a lot of unit testing, for something that will never be used again.

We've come to the conclusion generating mock data at best is only ever going to be a one-off exercise. There will be times we need code to generate complex 'data' (bill runs, charge versions, returns etc) in enough quantity to make tests in the prototype realistic. However once we've generated the data, there will be no additional requests. There is no point in maintaining these endpoints and increasing our workload.

So, this change removes the existing mock endpoint and functionality.
@Cruikshanks Cruikshanks force-pushed the refactor-to-use-new-water-models branch from b55c43a to 870cac7 Compare December 14, 2023 12:10
https://eaflood.atlassian.net/browse/WATER-4057

As part of the work we have been doing on two-part tariff, we will be creating all our new tables in the default `public` schema.

We have also decided that when there is a legacy table that we are still going to need we will create a [View](https://www.postgresql.org/docs/current/sql-createview.html) of it in the `public` schema. This allows us to correct any issues with naming conventions, strip out unused fields, and join entities currently sat in different schemas. The first example of this approach was done in PR #531 .

We created the new views in [Create WATER schema views](#551). Then in [Create WATER schema models & helpers](#560) we added the new models and helpers that use them.

This is the final step in the process, we are refactoring any use of the legacy models to use the new ones and deleting all the legacy-based code.
Having found a few more missed name changes, we would change them, then something else would break, we'd find more missed name changes, fix them, find another broken thing etc etc.

We believe we've now found them all. Tests all passing locally at this stage.
We went with a later time stamp to better match when the change was done and merged.

diff --git a/db/migrations/public/20231205174037_alter-bill-runs-view.js b/db/migrations/public/20231205174037_alter-bill-runs-view.js
deleted file mode 100644
index af07d8e..0000000
--- a/db/migrations/public/20231205174037_alter-bill-runs-view.js
+++ /dev/null
@@ -1,68 +0,0 @@
-'use strict'
-
-const viewName = 'bill_runs'
-
-exports.up = function (knex) {
-  return knex
-    .schema
-    .dropView(viewName)
-    .createView(viewName, (view) => {
-      view.as(knex('billing_batches').withSchema('water').select([
-        'billing_batch_id AS id',
-        'region_id',
-        'batch_type',
-        'from_financial_year_ending',
-        'to_financial_year_ending',
-        'status',
-        'invoice_count',
-        'credit_note_count',
-        'net_total',
-        'bill_run_number',
-        'error_code',
-        'external_id',
-        'is_summer AS summer',
-        'source',
-        'legacy_id',
-        'metadata',
-        'invoice_value',
-        'credit_note_value',
-        'transaction_file_reference',
-        'scheme',
-        'date_created AS created_at',
-        'date_updated AS updated_at'
-      ]))
-    })
-}
-
-exports.down = function (knex) {
-  return knex
-    .schema
-    .dropView(viewName)
-    .createView(viewName, (view) => {
-      // NOTE: We have commented out unused columns from the source table
-      view.as(knex('billing_batches').withSchema('water').select([
-        'billing_batch_id AS id',
-        'region_id',
-        'batch_type',
-        'from_financial_year_ending',
-        'to_financial_year_ending',
-        'status',
-        'invoice_count AS bill_count',
-        'credit_note_count',
-        'net_total',
-        'bill_run_number',
-        // 'error_code',
-        'external_id',
-        'is_summer AS summer',
-        'source',
-        'legacy_id',
-        'metadata',
-        'invoice_value',
-        'credit_note_value',
-        'transaction_file_reference',
-        'scheme',
-        'date_created AS created_at',
-        'date_updated AS updated_at'
-      ]))
-    })
-}
@Cruikshanks Cruikshanks force-pushed the refactor-to-use-new-water-models branch from 870cac7 to 060dcd3 Compare December 15, 2023 18:20
@Cruikshanks Cruikshanks marked this pull request as ready for review December 15, 2023 18:28
@Cruikshanks Cruikshanks merged commit 4d0b401 into main Dec 15, 2023
6 checks passed
@Cruikshanks Cruikshanks deleted the refactor-to-use-new-water-models branch December 15, 2023 18:28
Cruikshanks added a commit that referenced this pull request Apr 14, 2024
https://eaflood.atlassian.net/browse/WATER-3486

In [Remove licences with charge elements with approaching time limits from billing](#443) we added a background job to do just that. Then in [Refactor to use new view based WATER models](#569) we updated how we interact with the DB.

We updated the service which fetches the data for the job. But we didn't clock that this results in the job erroring because we are no longer passing in `licenceId`. Without it we error trying to create the `workflow` record in the DB because of a constraint on the field.

We spotted this when working on [New job to add new & updated licences to workflow](#903) so this change is mainly about fixing the bug.

But we also realised one of the reasons we'd failed to spot the issue was a lack of logging. So, we also updated the job to use our pattern of logging how long the process took. We also take an idea from the new job and set a flag on the `workflow` record so we can spot _why_ it was created.
Cruikshanks added a commit that referenced this pull request Apr 14, 2024
https://eaflood.atlassian.net/browse/WATER-3486

In [Remove licences with charge elements with approaching time limits from billing](#443) we added a background job to do just that. Then in [Refactor to use new view-based WATER models](#569) we updated how we interact with the DB.

We updated the service which fetches the data for the job. But we didn't spot that this results in the job erroring because we are no longer passing in `licenceId`. Without it, we error trying to create the `workflow` record in the DB because of a constraint on the field.

We spotted this when working on [New job to add new & updated licences to workflow](#903) so this change is mainly about fixing the bug.

But we also realised one of the reasons we'd failed to spot the issue was a lack of logging. So, we also updated the job to use our pattern of logging how long the process took. We also take an idea from the new job and set a flag on the `workflow` record so we can spot _why_ it was created.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant