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

Fix licence issues failing to insert for review #867

Merged
merged 12 commits into from
Mar 28, 2024

Conversation

Cruikshanks
Copy link
Member

@Cruikshanks Cruikshanks commented Mar 27, 2024

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

In recent testing of triggering an SROC two-part tariff bill run, we've seen errors thrown by the PersistAllocatedLicenceToResultsService.

After investigating it is not the issue. It was being asked to persist every issue for every element and return found by DetermineLicenceIssuesService in its issues field.

No de-duping of the issues is taking place so it's blowing past the 255-char limit for the DB field for some licences.

This causes the bill run to fail and the system app to crash (but we'll deal with that in another fix!)

This change will de-dupe the issues found before they are persisted to keep the entry to a minimum. But should a licence have every single issue the resulting value would still be more than 255 characters.

So, we also add a migration to alter the issues column in review_licences, review_charge_elements and review_returns from varchar(255) to text which has no limit (in practical terms).

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

In recent testing of triggering an SROC two-part tariff bill run, we've seen errors being thrown by the `PersistAllocatedLicenceToResultsService`.

After investigating it is not the issue. It was being asked to persist every issue for every element and return found by `DetermineLicenceIssuesService` in its `issues` field.

No de-duping of the issues is taking place so its blowing past the 255 char limit for the DB field for some licences.

This then causes the bill run to fail and the system app to crash, but we'll deal with that in another fix.

This change will de-dupe the issues found _before_ they are persisted in order to keep under the 255 char limit.
@Cruikshanks Cruikshanks added the bug Something isn't working label Mar 27, 2024
@Cruikshanks Cruikshanks self-assigned this Mar 27, 2024
@Cruikshanks Cruikshanks marked this pull request as ready for review March 27, 2024 15:02
Copy link
Contributor

@Beckyrose200 Beckyrose200 left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@Beckyrose200 Beckyrose200 left a comment

Choose a reason for hiding this comment

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

If the maximum the issues string can be in the DB is all the issues once then the string would look like this

'Abstraction outside period, Aggregate, Checking query, No returns received, Over abstraction, Overlap of charge dates, Returns received but not processed, Returns received late, Return split over charge references, Some returns not received, Unable to match return'

This still surpasses the 255 character limit on the db, so I think we need to change that to be bigger or drop it completely?

@Jozzey
Copy link
Contributor

Jozzey commented Mar 27, 2024

If the maximum the issues string can be in the DB is all the issues once then the string would look like this

'Abstraction outside period, Aggregate, Checking query, No returns received, Over abstraction, Overlap of charge dates, Returns received but not processed, Returns received late, Return split over charge references, Some returns not received, Unable to match return'

This still surpasses the 255 character limit on the db, so I think we need to change that to be bigger or drop it completely?

I think we should just drop the constraints on these varchar columns. They were added unintentionally and if you look at the rest of the DB there are no such constraints on the other varchar columns (not that I've checked them all).

As pointed out in the review, even with the duplicates removed if a licence had every issue we'd be trying to set `issues` as `'Abstraction outside period, Aggregate, Checking query, No returns received, Over abstraction, Overlap of charge dates, Returns received but not processed, Returns received late, Return split over charge references, Some returns not received, Unable to match return'`. This is more than 255 characters so the error would arise again.

So, if we alter the column types to be `text` there is no limit (in practical terms) to what we can insert. Just to be on the safe side we make the same change to the returns and charge elements tables as well.
Cruikshanks added a commit that referenced this pull request Mar 27, 2024
https://eaflood.atlassian.net/browse/WATER-4375

Spotted whilst working on [Fix licence issues failing to insert for review](#867). When `PersistAllocatedLicenceToResultsService` errored trying to insert a record it was causing an uncaught exception that was crashing the app.

After investigating the issue we have spotted it is because `MatchAndAllocateService` is using a `forEach()` to call asynchronous code.

- [Using async/await in a forEach loop (you can't)](https://blog.devgenius.io/using-async-await-in-a-foreach-loop-you-cant-c174b31999bd)
- [Do not use forEach with async-await](https://gist.github.com/joeytwiddle/37d2085425c049629b80956d3c618971)

If we switch to a `for...of` loop instead any errors thrown within the loop, even from `PersistAllocatedLicenceToResultsService` get caught as expected by the top level `try/catch` in `ProcessBillRun` and the bill run marked as errored.

Also, whilst looking into this we spotted that `DetermineLicenceIssuesService` as been made `async` even though it is doing nothing asynchronous. We use this opportunity to fix that as well.
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 b2e3966 into main Mar 28, 2024
6 checks passed
@Cruikshanks Cruikshanks deleted the fix-licence-issues-failing-to-insert branch March 28, 2024 12:13
Cruikshanks added a commit that referenced this pull request Mar 28, 2024
https://eaflood.atlassian.net/browse/WATER-4375

Spotted whilst working on [Fix licence issues failing to insert for review](#867). When `PersistAllocatedLicenceToResultsService` errored trying to insert a record it was causing an uncaught exception that was crashing the app.

After investigating the issue we have spotted it is because `MatchAndAllocateService` is using a `forEach()` to call asynchronous code.

- [Using async/await in a forEach loop (you can't)](https://blog.devgenius.io/using-async-await-in-a-foreach-loop-you-cant-c174b31999bd)
- [Do not use forEach with async-await](https://gist.github.com/joeytwiddle/37d2085425c049629b80956d3c618971)

If we switch to a `for...of` loop instead any errors thrown within the loop, even from `PersistAllocatedLicenceToResultsService` get caught as expected by the top level `try/catch` in `ProcessBillRun` and the bill run marked as errored.

Also, whilst looking into this we spotted that `DetermineLicenceIssuesService` has been made `async` even though it is doing nothing asynchronous. We use this opportunity to fix that as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants