Fix including unnecessary supp. billing trans. #210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
https://eaflood.atlassian.net/browse/WATER-3989 Recent testing found an issue following the change we made in [Inc. changes to agreements & charges in supp bill](#207). Although the final result was correct, we were always create invoices with transaction lines for scenarios we should have been returning an `EMPTY` bill run. The reason was our changed logic to compare calculated transaction lines to previous ones. It was always returning 'false' i.e. the lines were always different when they should be matching and cancelling each other out. The problem was an unexpected way the previous team was handling Section 130 agreements. Whether a charge version has one or not is held in the linked `charge_elements` table in the adjustments field, for example ```json {"s126": null, "s127": true, "s130": false, "charge": null, "winter": false, "aggregate": null} ``` We generate our calculated transaction lines using this source data so hold `section130Agreement` as a boolean. What we found when we looked at the `billing_transactions` table is that this is a varchar not a boolean. It looks like the initial build of PRESROC billing stored values other than 'true' or 'false'. But that then changed to 'true' and 'false' and these are the only values used for SROC. This means we weren't aware `FetchPreviousBillingTransactionsService` was return previous transactions with `section130Agreement` held as a string. So, when we came to compare it was causing it to always determine the transactions were different. So, this change fixes the issue by using [Knex raw](https://knexjs.org/guide/raw.html) to cast the value in our query to a boolean to match what we get and use in `FetchChargeVersionsService`. Doing at there will be the most performant and means it matches our expectations of what type `section130Agreement` will be.
StuAA78
requested changes
May 4, 2023
app/services/supplementary-billing/fetch-previous-billing-transactions.service.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Stuart Adair <[email protected]>
Cruikshanks
added a commit
that referenced
this pull request
May 16, 2023
https://eaflood.atlassian.net/browse/WATER-4010 We found another scenario where the bill run was always adding a credit and debit for a charge version on each bill run. The scenario was a new charge version is added which applies an abatement agreement (section 126 factor). The bill run gets run and sent. Another minor change is then made. No new debit and credit is needed. But when the engine grabs the previous debit, flips it to a credit and compares it against the calculated line it finds they don't match. This results in a debit and credit for the same amount appearing each time when they should be cancelled out and not appear at all. We tracked the problem down to the `charge_element` holding all the values as strings inside a JSONB field (`adjustments`). But when they get persisted to the `billing_transactions` table they are properly stored as a number. This is the same issue as [Fix including unnecessary supp. billing trans.](#210) only in reverse. Thanks legacy code! This change ensures when it comes to the comparison of `section126Factor` both previous and calculated and looking at the same data type.
Cruikshanks
added a commit
that referenced
this pull request
May 16, 2023
https://eaflood.atlassian.net/browse/WATER-4010 We found another scenario where the bill run always added credit and debit for the charge version on each bill run. The scenario was a new charge version was added which applied an abatement agreement (section 126 factor). The bill run gets run and sent. Another minor change is then made. No new debit and credit are needed. But when the engine grabs the previous debit, flips it to credit and compares it against the calculated line it finds they don't match. This results in a debit and credit for the same amount appearing each time when they should be cancelled out and not appear at all. We tracked the problem down to the `charge_element` holding all the values as strings inside a JSONB field (`adjustments`). But when they get persisted to the `billing_transactions` table they are properly stored as a number. This is the same issue as [Fix including unnecessary supp. billing trans.](#210) only in reverse. Thanks, legacy code! This change ensures when it comes to the comparison of `section126Factor` both previous and calculated and looking at the same data type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-3989
Recent testing found an issue following the change we made in Inc. changes to agreements & charges in supp bill. Although the final result was correct, we were always creating invoices with transaction lines for scenarios we should have been returning an
EMPTYbill run.The reason was our changed logic to compare calculated transaction lines to previous ones. It was always returning 'false' i.e. the lines were always different when they should be matching and cancelling each other out.
The problem was an unexpected way the previous team was handling Section 130 agreements. Whether a charge version has one or not is held in the linked
charge_elementstable in the adjustments field, for example{"s126": null, "s127": true, "s130": false, "charge": null, "winter": false, "aggregate": null}We generate our calculated transaction lines using this source data so hold
section130Agreementas a boolean. What we found when we looked at thebilling_transactionstable is that this is a varchar, not a boolean. It looks like the initial build of PRESROC billing stored values other than 'true' or 'false'. But that then changed to 'true' and 'false' and these are the only values used for SROC.This means we weren't aware
FetchPreviousBillingTransactionsServicewas returning previous transactions withsection130Agreementheld as a string. So, when we came to compare it was causing it to always determine the transactions were different.So, this change fixes the issue by using Knex raw to cast the value in our query to a boolean to match what we get and use in
FetchChargeVersionsService. Doing at there will be the most performant and means it matches our expectations of what typesection130Agreementwill be.