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.

Sign up for GitHub

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

Jump to bottom

The Great Rename #416

Merged
merged 14 commits into from
Sep 18, 2023
Merged

The Great Rename #416

merged 14 commits into from
Sep 18, 2023

Conversation

Cruikshanks
Copy link
Member

@Cruikshanks Cruikshanks commented Sep 9, 2023 •
edited
Loading

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 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 Cruikshanks added the housekeeping Refactoring, tidying up or other work which supports the project label Sep 9, 2023
@Cruikshanks Cruikshanks self-assigned this Sep 9, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 10, 2023
View reviewed changes
app/services/data/mock/generate-bill-run.service.js
const { address, name } = GenerateMockDataService.go()

billingInvoice.accountAddress = address
billingInvoice.contact = name
bill.accountAddress = address

Check failure

Code scanning / CodeQL

Insecure randomness

This uses a cryptographically insecure random number generated at [Math.random()](1) in a security context. This uses a cryptographically insecure random number generated at [Math.random()](2) in a security context. This uses a cryptographically insecure random number generated at [Math.random()](3) in a security context.
@Cruikshanks Cruikshanks force-pushed the the-great-rename branch 3 times, most recently from 747c6ab to ff405cf Compare September 12, 2023 15:14
@Cruikshanks
The Great Rename
f04406d 
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".

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.
@Cruikshanks
BillingBatch to BillRun
637f9b8
@Cruikshanks
BillingInvoice to Bill
5bb72f2
@Cruikshanks
BillingInvoiceLicence to BillLicence
657db51
@Cruikshanks
BillingTransaction to Transaction
1ff356d
@Cruikshanks
BillingChargeCategory to ChargeCategory
b0c3c06
@Cruikshanks
ChargeVersion to ChargeInformation
ebb0b72
@Cruikshanks
ChargeElement to ChargeReference
fe4ac26
@Cruikshanks
ChargeVersionWorkflow to Workflow
a455b55
@Cruikshanks
Revert ChargeVersion name change
1ec0946 
We have just come back from 3 days of all the delivery team and its users referring to these as Charge Versions. In fact, their example formed the basis for an idea of doing the same thing for 'return versions'.

Clearly, that is how everyone refers to them, even if the UI page has the title 'Charge information'.

So, we revert that name change.
@Cruikshanks
PurposesUse to Purpose
02075df
@Cruikshanks
Missed updates
3e0e072
@Cruikshanks
ChargePurpose to ChargeElement
88aab9a
@Cruikshanks Cruikshanks marked this pull request as ready for review September 18, 2023 08:27
@Cruikshanks Cruikshanks merged commit 712c22b into main Sep 18, 2023
@Cruikshanks Cruikshanks deleted the the-great-rename branch September 18, 2023 08:28
Cruikshanks added a commit that referenced this pull request Jan 17, 2024
@Cruikshanks
Fix issues with reissue post The Great Rename
80bc97e 
https://eaflood.atlassian.net/browse/WATER-4336

Whilst working on updating our acceptance tests due to changes in the view bill run and view bill screens we found even after accounting for the changes, the SROC reissue test still failed.

After investigation we were able to identify changes made during the [The Great Rename](#416) had broken the reissue engine.

Prior to the rename the reissue engine was depending on shared fields between records. For example, a `water.billing_invoice` and a `crm_v2.invoice_account` shared the same fields; `invoice_account_id` and `invoice_account_number`.

So, you could pass an instance of both to a service like `GenerateBillService` and as far as it is concerned it is dealing with a 'billing account' instance because it has the `invoice_account_id` and `invoice_account_number` properties it expects.

```javascript
const billingInvoice = {
  billingInvoiceId: 'e1b40699-3b57-422c-96c3-3732e86312fc'
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}

const invoiceAccount = {
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}
```

Post the rename this is no longer the case. Now we have the following

```javascript
const billingInvoice = {
  id: 'e1b40699-3b57-422c-96c3-3732e86312fc'
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}

const invoiceAccount = {
  id: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}
```

> We use field names pre the great rename to help explain the issue. But this is not what they would actually be.

It's `invoiceAccountId` on the billing invoice and just `id` on the invoice account. `GenerateBillService` can no longer be given both types of instance and expect to give the same result.

So, this fixes the issue by generating objects that represent what `GenerateBillService` and `GenerateBillLicenceService` expect.
@Cruikshanks Cruikshanks mentioned this pull request Jan 17, 2024
Merged
Cruikshanks added a commit that referenced this pull request Jan 17, 2024
@Cruikshanks
Fix issues with reissue post The Great Rename (#664)
b492692 
https://eaflood.atlassian.net/browse/WATER-4336

Whilst working on updating our acceptance tests due to changes in the view bill run and view bill screens we found even after accounting for the changes, the SROC reissue test still failed.

After investigation, we were able to identify changes made during the [The Great Rename](#416) had broken the reissue engine.

Before the rename the reissue engine depended on shared fields between records. For example, a `water.billing_invoice` and a `crm_v2.invoice_account` shared the same fields; `invoice_account_id` and `invoice_account_number`.

So, you could pass an instance of both to a service like `GenerateBillService` and as far as it is concerned it is dealing with a 'billing account' instance because it has the `invoice_account_id` and `invoice_account_number` properties it expects.

```javascript
const billingInvoice = {
  billingInvoiceId: 'e1b40699-3b57-422c-96c3-3732e86312fc',
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}

const invoiceAccount = {
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}
```

Post the rename this is no longer the case. Now we have the following

```javascript
const billingInvoice = {
  id: 'e1b40699-3b57-422c-96c3-3732e86312fc',
  invoiceAccountId: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}

const invoiceAccount = {
  id: '53c85d8c-d5fc-48db-9c59-aa36995c0e4b',
  invoiceAccountNumber: 'A00000002A'
}
```

> We use field names before the great rename to help explain the issue. But this is not what they are in the code anymore.

It's `invoiceAccountId` on the billing invoice and just `id` on the invoice account. `GenerateBillService` can no longer be given both types of instances and expect to get the same result.

So, this fixes the issue by generating objects that represent what `GenerateBillService` and `GenerateBillLicenceService` expect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Cruikshanks Cruikshanks

Labels
housekeeping Refactoring, tidying up or other work which supports the project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Cruikshanks