Handle REPLACED charge v's with different inv. acc #179
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-3950 During the supplementary billing process as we iterate through the charge versions we do so in invoice account then licence order. This is so we can use 'iterating to the next one' as a trigger to know when to persist the current billing invoice and billing licence records we're dealing with. Because we have the invoice account and licence ID's during this process it made it simple to look at the last bill run and extract any matching debit billing transactions that might now need crediting. But during test a scenario came up that we're not managing. A charge version can be created that supercedes the previous one. It supercedes the previous, because the the start date of the new charge version is the same as the previous i.e. the previous charge version should be completely ignored. This most often happens because the billing account was wrong. So, the new charge version will be linked to a different billing account. The problem is when the previous charge version has already been billed. We need to include a credit in the new bill run we're generating for it. But our 'prudent' inclusion of the invoice account ID in our query means we're overlooking the previous debit to the old billing invoice account. The end result is 2 invoices to 2 billing accounts on different bill runs, for the same licence and abstraction period. This change resolves the problem, though goodness knows how!
Modelled on `FetchChargeVersionsService` this returns those charge versions that display as REPLACED in the UI (superseded) in the DB. We don't return the same amount of data and linked records. Unlike the `FetchChargeVersionsService`, the results we return won't be used as the basis for calculating the transaction lines. Instead, they'll be used to identify existing billing transaction lines.
Cruikshanks
force-pushed
the
fetch-all-debits-from-previous
branch
from
168f23f to
8dda923
Compare
Just to make it distinct and consistent with `FetchReplacedChargeVersionsService`.
We now need to fetch and reverse previous billing transactions in 2 places. We were already doing this work in `ProcessBillingTransactionsService`. So, we move that work into a new service as a first step. Then we'll refactor `ProcessBillingTransactionsService` to use our new service.
We had dropped data we thought we didn't need. But in testing we found that the CreateTransactionPresenter for the charging module was not getting the properties is was expecting.
In testing we hit on a scenario where the `FetchReplacedChargeVersionsService` might pull through charge versions with invoice account ID's that have already been processed as part of the main `ProcessBillingBatchService`. When this happened an error was thrown because it tried to insert into `billing_invoices` a record with the same invoice account ID and billing batch ID as already exists. If a replaced charge version is linked to an invoice account that is also linked to an approved invoice account it will already have been processed.
Alongside the other services we have added, this change provides a working solution to the replaced charge versions problem. I have tested multiple replacements of a charge version where the billing account has been changed, within a single licence, and for partial parts of the year. It seems to be working! This started as an exercise in getting the sudo-code PR into something that more closely resembled what is needed. But it's ended up with a working solution. There is probably some more refactoring we can do to improve things. We could certainly in more documentation and missing unit tests. But hopefully what is here will provide a base on which to make those changes.
|
@Jozzey @StuAA78 I meant to just push the notes we had but instead started to have a play 😁. With my changes, this is the kind of scenario I have seen it working locally for
It was correctly adding the credits which it wasn't doing before. There is still work to be done to complete the PR. But hopefully a working PR makes more sense than some rough notes 🤷 |
Cruikshanks
changed the title
We decided that it made more sense for `ProcessReplacedChargeVersionsService` to return `true` if any transactions were generated and `false` if they weren't -- this avoids a double negative situation in `ProcessBillingBatchService` where we were essentially saying "if nothing was not generated then...". We therefore flip the service's return value.
StuAA78
approved these changes
Apr 13, 2023
Jozzey
approved these changes
Apr 13, 2023
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-3950
During the supplementary billing process as we iterate through the charge versions we do so in invoice account and then licence order. This is so we can use 'iterating to the next one' as a trigger to know when to persist the current billing invoice and billing licence records we're dealing with.
Because we have the invoice account and licence ID's during this process it made it simple to look at the last bill run and extract any matching debit billing transactions that might now need crediting.
But during test, a scenario came up that we're not managing. A charge version can be created that supersedes the previous one. It supersedes the previous because the start date of the new charge version is the same as the previous i.e. the previous charge version should be completely ignored.
This most often happens because the billing account was wrong. So, the new charge version will be linked to a different billing account. The problem is when the previous charge version has already been billed. We need to include a credit in the new bill run we're generating for it. But our 'prudent' inclusion of the invoice account ID in our query means we're overlooking the previous debit to the old billing invoice account.
The end result is 2 invoices to 2 billing accounts on different bill runs, for the same licence and abstraction period.
This change resolves the problem, though goodness knows how!