The Great Rename #97
Labels
housekeeping
Refactoring, tidying up or other work which supports the project
Comments
Cruikshanks
added
the
housekeeping
Cruikshanks
added a commit
to DEFRA/water-abstraction-system
that referenced
this issue
Sep 16, 2023
DEFRA/water-abstraction-team#97 We hit a snag as part of the **Great Rename**! We want `water.charge_elements` to be `ChargeReferences`. We then want `water.charge_purposes` to be `ChargeElements`. But there is a relationship between the 2 that has to be defined in `ChargeReferenceModel`. The problem is when you run `const query = await ChargeReferenceModel.query().innerJoinRelated('chargeElements')` you get an error from [Objection.js](https://vincit.github.io/objection.js). ``` select "charge_elements".* from "water"."charge_elements" inner join "water"."charge_purposes" as "charge_elements" on "charge_elements"."charge_element_id" = "charge_elements"."charge_element_id" - table name "charge_elements" specified more than once ``` If we can get Objection to alias `charge_elements` as `charge_references` in our query then we would remove the error. To do this we need to use `alias()` in the custom query builder used by the model. Currently, all our models extend `LegacyBaseModel` which uses `SchemaQueryBuilder` to ensure all legacy models apply a schema. This change refactors `SchemaQueryBuilder` to also handle applying a table alias when one exists.
This was referenced Sep 16, 2023
Cruikshanks
added a commit
to DEFRA/water-abstraction-system
that referenced
this issue
Sep 17, 2023
DEFRA/water-abstraction-team#97 We hit a snag as part of the **Great Rename**! We want `water.charge_elements` to be `ChargeReferences`. We then want `water.charge_purposes` to be `ChargeElements`. But there is a relationship between the 2 that has to be defined in `ChargeReferenceModel`. The problem is when you run `const query = await ChargeReferenceModel.query().innerJoinRelated('chargeElements')` you get an error from [Objection.js](https://vincit.github.io/objection.js). ``` select "charge_elements".* from "water"."charge_elements" inner join "water"."charge_purposes" as "charge_elements" on "charge_elements"."charge_element_id" = "charge_elements"."charge_element_id" - table name "charge_elements" specified more than once ``` If we can get Objection to alias `charge_elements` as `charge_references` in our query then we would remove the error. To do this we need to use `alias()` in the custom query builder used by the model. Currently, all our models extend `LegacyBaseModel` which uses `SchemaQueryBuilder` to ensure all legacy models apply a schema. This change refactors `SchemaQueryBuilder` to also handle applying a table alias when one exists.
Cruikshanks
added a commit
to DEFRA/water-abstraction-system
that referenced
this issue
Sep 18, 2023
DEFRA/water-abstraction-team#97 Enough is enough! Too many of our conversations when discussing this service follow the pattern of > "The _charge version_, known as the _charge information_ in the UI, links to multiple _charge references_, which are _charge elements_ in the code. Each of these has multiple _charge purposes_, which are _charge elements_ in the UI. These records are used when generating a `bill run` which is a `billing batch` in the DB. A `bill run` is made up of `billing invoices` that are shown as `bills` in the UI". This insanity is because things were either built before the content design had agreed on what they should be called, or they've changed over time but the code hasn't. We're not going to go back and fix all the legacy code. But we have this confusion in the [water-abstraction-system](https://github.com/DEFRA) because we named our models to match the tables, which is the norm. We could rename the tables and our models to match the UI but then we'd break _everything_. So, we are going to do what is within our control. We will rename our models and services to match how things are referred to in the UI (even though some of those names don't help!) Hopefully, this will reduce conversations like our example and help bring new team members on board. We'll still need to be aware of the name disconnect between the code and the DB. But at least our models will act as documentation for what is _actually_ what. Anyone trying to match something from the UI to something in the code will have a much easier time if we rename everything. **Renames** - `BillingBatch` to `BillRun` - `BillingInvoice` to `Bill` - `BillingInvoiceLicence` to `BillLicence` - `BillingTransaction` to `Transaction` - `BillingChargeCategory` to `ChargeCategory` - `ChargeElement` to `ChargeReference` - `ChargePurpose` to `ChargeElement` - `ChargeVersionWorkflow` to `Workflow` - `PurposesUse` to `Purpose`
Cruikshanks
added a commit
to DEFRA/water-abstraction-system
that referenced
this issue
Nov 12, 2023
DEFRA/water-abstraction-team#97 We overlooked this in [the Great Rename](DEFRA/water-abstraction-team#97) because it isn't in the `water` schema. In the UI and amongst the team what the DB calls 'Invoice Account' is referred to as 'Billing Account'. In our latest changes which focused on building new bill run pages we had to reference this table and model a lot, which for anyone new is going to look confusing. For example, see `app/services/bills/fetch-billing-account.service.js`. ```javascript /** * Fetches the billing account and associated details for a given invoice account ID * @module FetchBillingAccountService */ const InvoiceAccount = require('../../models/crm-v2/invoice-account.model.js') ``` So, we're sorting that out in this change, renaming the invoice account model to `BillingAccountModel`.
Cruikshanks
added a commit
to DEFRA/water-abstraction-system
that referenced
this issue
Nov 12, 2023
DEFRA/water-abstraction-team#97 We overlooked this in [the Great Rename](DEFRA/water-abstraction-team#97) because it isn't in the `water` schema. In the UI and amongst the team what the DB calls 'Invoice Account' is referred to as 'Billing Account'. In our latest changes which focused on building new bill run pages, we had to reference this table and model a lot, which for anyone new is going to look confusing. For example, see `app/services/bills/fetch-billing-account.service.js`. ```javascript /** * Fetches the billing account and associated details for a given invoice account ID * @module FetchBillingAccountService */ const InvoiceAccount = require('../../models/crm-v2/invoice-account.model.js') ``` So, we're sorting that out in this change, renaming the invoice account model to `BillingAccountModel`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Enough is enough!
Too many of our conversations when discussing this service follow the pattern of
This insanity is because things were either built before the content design had agreed on what they should be called, or they've changed over time but the code hasn't.
We're not going to go back and fix all the legacy code. But we have this confusion in the water-abstraction-system because we named our models to match the tables, which is the norm. We could rename the tables and our models to match the UI but then we'd break everything.
So, we are going to do what is within our control. We will rename our models and services to match how things are referred to in the UI (even though some of those names don't help!) Hopefully, this will reduce conversations like our example and help bring new team members on board. We'll still need to be aware of the name disconnect between the code and the DB. But at least our models will act as documentation for what is actually what.
Anyone trying to match something from the UI to something in the code will have a much easier time if we rename everything.
The text was updated successfully, but these errors were encountered: