Inc. changes to agreements & charges in supp bill #207
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 When a new charge version is added to a licence, users can make changes to both agreements and charges linked to the licence via the charge version. So, for example, when the SROC charge version is first applied the licence does **not** have a two part tariff agreement (2PT). But later in the year one is added. To do this the business will add a new charge version and apply the 2PT agreement to it. Though the number of billable days calculated won't change, the charge that the [charging-module-api](https://gothub.com/DEFRA/sroc-charging-module-api) returns will. So, we need to credit the previous transaction and calculate a new debit. At the moment, our engine does not look at changes to agreements and charges. So, it will compare the billable days, see no change and cancel both the previous and calculated transaction lines before we get a value from the charging module. This change updates the logic we use for comparing previous transactions to those we've calculated to include agreements and charges. If there has been a change, the lines won't match and get cancelled so will then progress to the next step; sending them to the charging module to calculate their charge value.
We need to add new tests to cover checking when the agreements and charges differ between previous and calculated transactions. It's going to need more transaction data to be passed through to the service, and continuing to use fixed data is going to make the test file massive. So, we make a change to move the test data logic to be generator methods instead.
Because we are using a generator now we spotted we could deal with a previous NOTE tag.
This is where we will add our new tests.
Decided it would be clearer to put the matching tests in their own section. So, first pass is to cover what we do now.
Realised we didn't have these properties which we need in order to manipulate in our tests.
We now have tests to cover all the other conditions. Just need to make the code make them pass now!
Plus amend the documentation.
|
Unfortunately, a relatively small code change has resulted in a number of tests being added. Hence the size of the PR. Sorry folks! 😳😬 |
Cruikshanks
added a commit
that referenced
this pull request
May 4, 2023
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.
Cruikshanks
added a commit
that referenced
this pull request
May 4, 2023
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 creating 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 returning 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.
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
When a new charge version is added to a licence, users can change agreements and charges linked to the licence via the charge version.
So, for example, when the SROC charge version is first applied the licence does not have a two-part tariff agreement (2PT). But later in the year, one is added. To do this the business will add a new charge version and apply the 2PT agreement.
Though the number of billable days calculated won't change, the charge that the charging-module-api returns will. So, we need to credit the previous transaction and calculate a new debit. At the moment, our engine does not look at changes to agreements and charges. So, it will compare the billable days, see no change and cancel both the previous and calculated transaction lines before we get a value from the charging module.
This change updates the logic for comparing previous transactions to those we've calculated to include agreements and charges. If there has been a change, the lines won't match and get cancelled so will then progress to the next step; sending them to the charging module to calculate their charge value.