Fix re-processing REPLACED charge versions

[Cruikshanks]

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 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.

[Cruikshanks]

The SonarCloud issue might be valid. But it's highlighting code we haven't touched and don't want to play with until we have more unit test coverage. So, not gonna close it. But am ignoring it for now.

[Jozzey]