Fetch only relevant previous transactions in supp. #1778
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-4201 > Part of the work to support two-part tariff supplementary bill runs As part of [Move reusable supplementary billing services](#1760) we moved `FetchPreviousTransactionsService` so it could be reused by both the standard and two-part tariff supplementary billing engines. We just assumed on face value that we would fetch the previous transactions for a billing period, let the transactions compare and cancel out as normal, and any credits and debits that shake out at the end are what make up the bill run. But testing straight away threw up an issue with this assumption. Imagine the billing period is 2022-23 and we have licence `01/123`. A transaction was added to the annual bill run for 214 billable days and the authorised volume of 18.14ML. It also adds a compensation charge transaction. This is the 'first part charge'. Then the two-part tariff annual bill run is created for the same period. To that a transaction is added for 0 billable days and the authorised volume of 18.14ML. This is the 'second part charge'. A change is then made in the period, so the licence is flagged for two-part tariff supplementary. After the 2PT supplementary bill run is generated a user in the review pages allocates 30ML to the entry, then continues the bill run. At this point the fetch previous before this change was finding 3 debit transactions. | Bill run | Days | Volume | Charge | |----------|------|--------|--------| | Annual | 214 | 18.184 | 256.50 | | Annual | 214 | 18.184 | 0.00 | | 2PT Anl. | 0 | 18.184 | 256.50 | None of them match off against each other. So, they were then compared against the transaction generated for the two-part tariff supplementary. | Bill run | Days | Volume | Charge | |----------|------|--------|--------| | 2PT supp | 0 | 30 | 256.50 | None of the previous is a match, so no lines get cancelled. The end result is a credit of £513 against a debit of £256, with all 4 lines on it. The problem is `FetchPreviousTransactionsService` now needs to be 'billing specific'. When fetching previous transactions for a standard supplementary bill run, it should only be looking at annual and supplementary bill runs. For a two-part tariff bill run, it should only be looking at previous 2PT annual and supplementary. With this change, in our scenario the fetch will return only this transaction. | Bill run | Days | Volume | |----------|------|--------| | 2PT Anl. | 0 | 18.184 | It still wouldn't match to the one generated, but that is expected because the result needs to be the following. | Bill run | Days | Volume | |----------|------|--------| | 2PT Anl. | 0 | 18.184 | | 2PT supp | 0 | 30 |
After reviewing the current unit tests, we realised they were very much focused on specifics of the query, i.e. if the licence ID passed in has no bill licences, assert we return no results. To ensure we are fetching the _right_ previous transactions for the supplementary bill run being created, its purpose would be better expressed and tested via scenarios. We also wanted to limit the amount of records we were creating in the DB. So, we've created this seeder that generates the environment we are trying to deal with. We have 1 billing account with 2 licences. Both licences have been included in a standard and two-part tariff annual bill run. One of the licences has also been included in a standard and two-part tariff supplementary bill run. When testing the `FetchPreviousTransactionsService`, if the scenario is we are creating a two-part tariff supplementary bill run, then it should only return the 'two-part-tariff' transactions for each licence when called. The standard transactions must be ignored, and vis-versa. We'll use the seeder to create _all_ the records, and then fire all our unit tests to ensure it is only returning the ones we expect. For the first time, we'll also clean up the data generated by the seeder. We name the method `zap()`, so tha we can place it alphabetically below `seed()` in the module!
We need to be able to tell fetch the type of bill run we are generating. We could pass it through as another flag via `ProcessSupplementaryTransactionsService`, but it would be another thing we're passing through, just for it to pass through to FetchPrevious. Better, if we call FetchPrevious from the service that _knows_ the bill run type being generated, and just pass the result through to `ProcessSupplementaryTransactionsService`.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Cruikshanks
deleted the
fix-fetch-previous-transactions-for-supplementary
branch
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-4201
As part of Move reusable supplementary billing services we moved
FetchPreviousTransactionsServiceso it could be reused by both the standard and two-part tariff supplementary billing engines.We just assumed on face value that we would fetch the previous transactions for a billing period, let the transactions compare and cancel out as usual, and any credits and debits that shake out at the end make up the bill run.
But testing straight away threw up an issue with this assumption.
Imagine the billing period is 2022-23 and we have licence
01/123. A transaction was added to the annual bill run for 214 billable days and the authorised volume of 18.14ML. It also adds a compensation charge transaction. This is the 'first part charge'.Then the two-part tariff annual bill run is created for the same period. To that a transaction is added for 0 billable days and the authorised volume of 18.14ML. This is the 'second part charge'.
A change is then made for the period (authorised bumped to 30ML), so the licence is flagged for two-part tariff supplementary. After the 2PT supplementary bill run is generated, a user in the review pages allocates 30ML to the entry and then continues the bill run.
At this point, the 'fetch previous' before this change was finding three debit transactions.
None of them matched each other, so they were then compared against the transaction generated for the two-part tariff supplementary.
None of the previous is a match, so no lines get cancelled. The result is a credit of £513 against a debit of £256, with all four lines on it.
The problem is that
FetchPreviousTransactionsServicenow needs to be 'billing specific'. When fetching previous transactions for a standard supplementary bill run, it should only look at annual and supplementary bill runs. For a two-part tariff bill run, it should only look at the previous 2PT annual and supplementary bills.With this change, the fetch will return only this transaction in our scenario.
It still wouldn't match the one generated, but that is expected because the result needs to be the following.