Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix re-processing REPLACED charge versions (#192)
https://eaflood.atlassian.net/browse/WATER-3950 This change was the result of looking into an edge case found during testing. We'll try to explain with an example. But to summarise. The new process for handling **REPLACED** charge versions we just added, was processing a charge version that had already been processed. This led to a credit being added to the billing batch that shouldn't be. Our fix, though small in how it affects `ProcessBillingBatchService`, is to scrap the `ProcessReplacedChargeVersionsService` and deal with **REPLACED** charge versions outside of the main process. Instead, our primary `FetchChargeVersionsService` will now return both **REPLACED** and **APPROVED** records. ## Before the fix So, imagine we have licence `01/123/456` and linked to it are 2 charge versions both for the same account. > For the purposes of this the abstraction period is all year and the charge reference is the same for each charge version. Also, the charge version ID below ascends in the order they are added. But we list them in the tables in the order the UI will display them. ### Bill run 1 |ID |Invoice account|Start|End |Status | |-----|---------------|-----|-----|--------| |CHG02|ACC88 |01/09| - |APPROVED| |CHG01|ACC88 |01/04|31/08|APPROVED| The annual bill run was generated resulting in |ID |Charge Version|Invoice account|Billable days|Debit or Credit?| |-----|--------------|---------------|-------------|----------------| |TRN01|CHG01 |ACC88 |153 |Debit | |TRN02|CHG02 |ACC88 |212 |Debit | ### Bill run 2 Then, a new charge version linked to a different billing account is added which replaces `CHG01`. |ID |Invoice account|Start|End |Status | |-----|---------------|-----|-----|--------| |CHG02|ACC88 |01/09| - |APPROVED| |CHG03|ACC99 |01/04|31/08|APPROVED| |CHG01|ACC88 |01/04|31/08|REPLACED| The supplementary bill run is generated resulting in |ID |Charge Version|Invoice account|Billable days|Debit or Credit?| |-----|--------------|---------------|-------------|----------------| |TRN03|CHG01 |ACC88 |153 |Credit | |TRN04|CHG03 |ACC99 |153 |Debit | The first thing to note is `CHG02` doesn't make an appearance. It's worth understanding why because this is part of the reason this edge case eventually fails. Prior to this change we only fetched **APPROVED** charge versions and calculated their billable days. We then compared this with previous billing transactions. So, when processing `CHG02` we find `TRN01` and `TRN02`. This is because we're looking for previous transactions for `ACC88`, the invoice account linked to `CHG02`. We then compare them to our calculated transaction. `TRN02` matches (same licence, invoice account and billable days), so we cancel both out of the bill run. `TRN01` doesn't match (212 to 153 billable days) so is reversed (debit to credit) and included in the bill run. Because `TRN01` is being included we need to create a billing invoice record for `ACC88` as part of this bill run. `CHG03` goes through the same process. Only we find no previous transactions for `ACC99`. So nothing gets cancelled out and the calculated transaction is included in the bill run. ### Bill run 3 Another new charge version is added linked to `ACC99` which replaces `CHG03`. A possible reason for this is a mistake with the details entered for `CHG03`. |ID |Invoice account|Start|End |Status | |-----|---------------|-----|-----|--------| |CHG02|ACC88 |01/09| - |APPROVED| |CHG04|ACC99 |01/04|31/08|APPROVED| |CHG03|ACC99 |01/04|31/08|REPLACED| |CHG01|ACC88 |01/04|31/08|REPLACED| The supplementary bill run is generated. It should result in an empty bill run. Instead, it was resulting in this |ID |Charge Version|Invoice account|Billable days|Debit or Credit?| |-----|--------------|---------------|-------------|----------------| |TRN05|CHG02 |ACC88 |212 |Credit | It should be empty because the change hasn't impacted what we have previously billed. `ACC88` got their credit for April to August in Bill run 2. `ACC99` was correctly charged for the same period in the same bill run. `CHG04` has not changed the abstraction period or the billing account and as it replaces `CHG03` it is for exactly the same period. But we have ended up with a credit. This is because the new stage in the process we added to handle **REPLACED** charge versions that have invoice accounts that have _not been processed_ has gone wrong. Remember why `CHG02` didn't make an appearance in bill run 2? For the same reason, it doesn't appear in bill run 3. What has changed is the previous transactions returned as part of that process. Now, it finds `TRN01`, `TRN02`, and `TRN03`. Before we even compare our calculated lines against these results we first cleanse them of matching debits and credits. This is because if we have a debit matched by a credit for the invoice account, we can safely assume that is something which has already been dealt with as part of a previous change to the charge versions. That is true for this case. `TRN01` which is a debit from bill run 1, was credited by `TRN03` as part of bill run 2. So, all that is left to compare `CHG02` against is `TRN02`. They cancel each other out which means at this point in the process there is _nothing to bill_ for `ACC88`. Because of this we _do not_ create a billing invoice record for `ACC88`. `CHG04` then gets processed. We find `TRN04` as part of that, compare it to our calculated transaction, and get a match so both get cancelled out. At this point, we have nothing in the bill run which is correct. But then we kick off our new replaced charge versions process. That grabs any **REPLACED** charge versions flagged for supplementary billing that as yet have not been processed. We determine this by cross-checking their invoice account with those in billing invoices. And there is the flaw! We have processed `ACC88` as part of looking at `CHG02`. But we didn't create a billing invoice. So, `CHG01` gets returned and processed. The `ProcessReplacedChargeVersionsService` doesn't do any calculations. But it does go looking for previous transactions for the invoice account. It will return `TRN01`, `TRN02`, and `TRN03`. As before `TRN01` and `TRN03` will cancel each other out. But now `TRN02` has nothing to cancel it out, which means we add it to the bill run. 😱🤦😩 ## Explaining the fix Once we realised what was going on our initial plan was to add another query to `ProcessReplacedChargeVersionsService` to filter out **REPLACED** charge versions with invoice accounts also linked to **APPROVED** charge versions. But with `FetchChargeVersions`, `FetchReplacedChargeVersions` and this new one we'd be running almost identical queries 3 times. What if we could process **REPLACED** charge versions at the same time as **APPROVED** ones? In [Reverse previous SROC billing batches in supplementary bill run process](#186) we realised only pulling out previous debit transactions was causing things to go wrong. Using the same premise, if we pulled _all_ charge versions through would the whole compare and cancel idea work better? It would certainly reduce the complexity and improve performance. Our testing confirmed this. ### Bill run 3 (fixed) Remember the situation prior to running bill run 3. |ID |Invoice account|Start|End |Status | |-----|---------------|-----|-----|--------| |CHG02|ACC88 |01/09| - |APPROVED| |CHG04|ACC99 |01/04|31/08|APPROVED| |CHG03|ACC99 |01/04|31/08|REPLACED| |CHG01|ACC88 |01/04|31/08|REPLACED| Our expectation is an empty bill run. > If you run this example through manually you actually get a 1p credit. This is just the result of a rounding in the overall calculation and expected! `ProcessBillingBatchService` will look at `CHG01`. We've added some logic that says 'this is replaced - do not bother calculating' which means it will iterate to `CHG02`. That will generate a calculated line. `CHG03` comes next but before we process it we trigger a call to finalise the current billing invoice for `ACC88`. That's where we look at previous transactions and return `TRN01`, `TRN02`, and `TRN03`. As we covered earlier, `TRN01` and `TRN03` cancel each other out and our calculated line when matched to `TRN02` also gets cancelled. This leaves us with nothing to bill which is what we want. And as we've removed the `ProcessReplacedChargeVersionsService` step, we're no longer making the mistake of adding something that's been processed. ### Nothing broken We are also still handling the scenario `ProcessReplacedChargeVersionsService` was built to handle. Imagine we had a single all-year charge version linked to `ACC66` debited in the annual bill run. We then replace it with a charge version linked to a new invoice account. |ID |Invoice account|Start|End |Status | |-----|---------------|-----|-----|--------| |CHG06|ACC77 |01/09| - |APPROVED| |CHG05|ACC66 |01/04| - |REPLACED| When the supplementary billing is run previously `CHG05` would have been handled by the `ProcessReplacedChargeVersionsService`. Now, it's included in our main process. When we finalise the billing invoice for `ACC66` we'll see that debit, reverse it and include it in the bill run. With nothing calculated and no other transactions, it won't get cancelled out.
- Loading branch information
1 parent
29b2da2
commit 4e9eae0