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

Add missing CRM_V2 table constraints #404

Merged
merged 4 commits into from
Sep 5, 2023
Merged

Conversation

Cruikshanks
Copy link
Member

@Cruikshanks Cruikshanks commented Sep 4, 2023

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

We are migrating the change billing account address functionality from the legacy code to this project. In our test DB, we previously added migrations to recreate the CRM_V2 tables addresses, companies and invoice_account_addresses. But to keep things simple we usually don't bother with constraints as unless we really need them, we prefer to avoid using them.

Well, in the new Billing Account change address service we're adding we do! 🤦 We depend on the constraints to raise ON CONFLICT errors for our logic to work.

So, in order to test our new service we need to add the missing constraints to our migrations. We're doing that change separately because Add new Billing Account change address service is already pretty big 😬.


When we did apply the constraints it highlighted we had a flaw in our model tests. The top-level beforeEach() should only have been cleaning the DB.

The model tests also had to be updated to deal with creating 2 invoice address records using the same default data; we can't have multiple records with the same start date and invoice account ID.

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

We are working on migrating the change billing account address functionality from the legacy code to this project. We previously added migrations to recreate the CRM_V2 tables `addresses`, `companies` and `invoice_account_addresses` in our test DB. But to keep things simple we normally don't bother with constraints as unless we really need them, we prefer to avoid using them.

Well, in the [new Billing Account change address service](#400) we're adding we do! 🤦 We depend on the constraints to raise [ON CONFLICT errors](https://www.postgresql.org/docs/current/sql-insert.html#SQL-ON-CONFLICT) for our logic to work.

So, in order to test our new service we need to add the missing constraints to our migrations. We're doing that change separately because [Add new Billing Account change address service](#400) is already pretty big 😬.
@Cruikshanks Cruikshanks added the housekeeping Refactoring, tidying up or other work which supports the project label Sep 4, 2023
@Cruikshanks Cruikshanks self-assigned this Sep 4, 2023
@Cruikshanks Cruikshanks marked this pull request as ready for review September 4, 2023 21:23
@Cruikshanks Cruikshanks requested review from Jozzey, StuAA78 and Beckyrose200 and removed request for Jozzey, StuAA78 and Beckyrose200 September 4, 2023 21:23
@Cruikshanks Cruikshanks marked this pull request as draft September 4, 2023 21:26
Adding the constraints highlighted we had a flaw in our model tests. The top-level `beforeEach()` should only have been cleaning the DB.

With that sorted we then had to deal with creating 2 invoice address records using the same default data; we can't have multiple records with the same start date and invoice account ID.
@Cruikshanks Cruikshanks marked this pull request as ready for review September 4, 2023 21:57
Copy link
Contributor

@Jozzey Jozzey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Cruikshanks Cruikshanks merged commit e73dbf3 into main Sep 5, 2023
@Cruikshanks Cruikshanks deleted the add-missing-crm-constraints branch September 5, 2023 09:35
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.

2 participants