Inc. changes to agreements & charges in supp bill (#207)

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

Author: Cruikshanks

app/services/supplementary-billing/process-billing-transactions.service.js

@@ -39,21 +39,55 @@ async function go (calculatedTransactions, billingInvoice, billingInvoiceLicence
 }
 
 /**
+ * Compares a calculated transaction to the transactions from previous bill runs
+ *
  * Takes a single calculated debit transaction and checks to see if the provided array of reversed (credit) transactions
- * contains a transaction that will cancel it out, returning `true` or `false` to indicate if it does or doesn't. Since
- * the calculated debit transactions have not yet been sent to the Charging Module, we look at `chargeType`,
- * `chargeCategoryCode` and `billableDays` as any given combination of these will always result in the same value coming
- * back from the Charging Module.
+ * contains a transaction that will cancel it out, returning `true` or `false` to indicate if it does or doesn't.
+ *
+ * We compare those properties which determine the charge value calculated by the charging module. If the calculated
+ * transaction's properties matches one in reversedTransactions we return true. This will tell the calling method
+ * to not include the calculated transaction in the bill run. We also remove the matched transaction from
+ * reversedTransactions.
+ *
+ * The key properties are charge type, category code, and billable days. But we also need to compare agreements and
+ * additional charges because if those have changed, we'll need to credit the previous transaction and calculate the
+ * new debit value. Because what we are checking does not match up to what you see in the UI we have this reference
+ *
+ * - Abatement agreement - section126Factor
+ * - Two-part tariff agreement - section127Agreement
+ * - Canal and River Trust agreement - section130Agreement
+ * - Aggregate - aggregateFactor
+ * - Charge Adjustment - adjustmentFactor
+ * - Winter discount - isWinterOnly
+ *
+ * - Additional charges - isSupportedSource
+ * - Additional charges - supportedSourceName
+ * - Additional charges - isWaterCompanyCharge
  *
  * NOTE: This function will mutate the provided array of reversed transactions if one of the transactions in it will
  * cancel the calculated transaction; in this case, we remove the reversed transaction from the array as it can only
  * cancel one calculated transaction.
  */
 function _cancelCalculatedTransaction (calculatedTransaction, reversedTransactions) {
+  // When we put together this matching logic our instincts were to try and do something 'better' than this long,
+  // chained && statement. But whatever we came up with was
+  // - more complex
+  // - less performant
+  // We found this easy to see what properties are being compared. Plus the moment something doesn't match we bail. So,
+  // much as it feels 'wrong', we are sticking with it!
   const result = reversedTransactions.findIndex((reversedTransaction) => {
     return reversedTransaction.chargeType === calculatedTransaction.chargeType &&
       reversedTransaction.chargeCategoryCode === calculatedTransaction.chargeCategoryCode &&
-      reversedTransaction.billableDays === calculatedTransaction.billableDays
+      reversedTransaction.billableDays === calculatedTransaction.billableDays &&
+      reversedTransaction.section126Factor === calculatedTransaction.section126Factor &&
+      reversedTransaction.section127Agreement === calculatedTransaction.section127Agreement &&
+      reversedTransaction.section130Agreement === calculatedTransaction.section130Agreement &&
+      reversedTransaction.aggregateFactor === calculatedTransaction.aggregateFactor &&
+      reversedTransaction.adjustmentFactor === calculatedTransaction.adjustmentFactor &&
+      reversedTransaction.isWinterOnly === calculatedTransaction.isWinterOnly &&
+      reversedTransaction.isSupportedSource === calculatedTransaction.isSupportedSource &&
+      reversedTransaction.supportedSourceName === calculatedTransaction.supportedSourceName &&
+      reversedTransaction.isWaterCompanyCharge === calculatedTransaction.isWaterCompanyCharge
   })
 
   if (result === -1) {