Move transaction matching logic to GeneralLib (#855)

https://eaflood.atlassian.net/browse/WATER-4403

We are currently working on updating the supplementary billing engine to incorporate improvements we made to the process when implementing annual billing.

One of the things we've been reminded of is that we have multiple services that use the same logic to determine if the charging information for 2 transactions is the same. It's a bit of a scary matching function which we flagged as a TODO to bring into a single place.

Now seems a good time to do just that and help simplify the supplementary process.

app/lib/general.lib.js

@@ -180,11 +180,67 @@ function timestampForPostgres () {
   return new Date().toISOString()
 }
 
+/**
+ * Compare key properties of 2 transactions and determine if they are a 'match'
+ *
+ * We compare those properties which determine the charge value calculated by the charging module. If the properties
+ * are the same we return true. Else we return false.
+ *
+ * This is used in the billing engines to determine 2 transactions within the same bill, often a debit and a credit,
+ * and whether they match. If they do we don't send either to the charge module or include them in the bill as they
+ * 'cancel' each other out.
+ *
+ * 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 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 - winterOnly
+ *
+ * - Additional charges - supportedSource
+ * - Additional charges - supportedSourceName
+ * - Additional charges - waterCompanyCharge
+ *
+ * @param {Object} left - First transaction to match
+ * @param {Object} right - Second transaction to match
+ *
+ * @returns {boolean} true if a match else false
+ */
+function transactionsMatch (left, right) {
+  // 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 also believe this makes it 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!
+  return left.chargeType === right.chargeType &&
+    left.chargeCategoryCode === right.chargeCategoryCode &&
+    left.billableDays === right.billableDays &&
+    left.section126Factor === right.section126Factor &&
+    left.section127Agreement === right.section127Agreement &&
+    left.section130Agreement === right.section130Agreement &&
+    left.aggregateFactor === right.aggregateFactor &&
+    left.adjustmentFactor === right.adjustmentFactor &&
+    left.winterOnly === right.winterOnly &&
+    left.supportedSource === right.supportedSource &&
+    left.supportedSourceName === right.supportedSourceName &&
+    left.waterCompanyCharge === right.waterCompanyCharge
+}
+
 module.exports = {
   calculateAndLogTimeTaken,
   currentTimeInNanoseconds,
   determineCurrentFinancialYear,
   generateUUID,
   periodsOverlap,
-  timestampForPostgres
+  timestampForPostgres,
+  transactionsMatch
 }

app/services/bill-runs/supplementary/fetch-previous-transactions.service.js

@@ -7,6 +7,7 @@
  */
 
 const { db } = require('../../../../db/db.js')
+const { transactionsMatch } = require('../../../lib/general.lib.js')
 
 /**
  * Fetches the previously billed transactions that match the bill, licence and year provided, removing any debits
@@ -42,7 +43,7 @@ function _cleanse (transactions) {
 
   credits.forEach((credit) => {
     const debitIndex = debits.findIndex((debit) => {
-      return _matchTransactions(debit, credit)
+      return transactionsMatch(debit, credit)
     })
 
     if (debitIndex > -1) {
@@ -53,55 +54,6 @@ function _cleanse (transactions) {
   return debits
 }
 
-/**
- * Compares a debit transaction to a credit transaction
- *
- * We compare those properties which determine the charge value calculated by the charging module. If the debit
- * transaction's properties matches the credit's we return true. This will tell the calling method
- * to remove the debit from the service's final results.
- *
- * 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 - winterOnly
- *
- * - Additional charges - supportedSource
- * - Additional charges - supportedSourceName
- * - Additional charges - waterCompanyCharge
- */
-function _matchTransactions (debit, credit) {
-  // TODO: This logic is a duplicate of what we are doing in
-  // app/services/supplementary-billing/process-transactions.service.js. This also means we are running the
-  // same kind of unit tests on 2 places. We need to refactor this duplication in the code and the tests out.
-
-  // When we put together this matching logic our instincts was 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!
-  return debit.chargeType === credit.chargeType &&
-    debit.chargeCategoryCode === credit.chargeCategoryCode &&
-    debit.billableDays === credit.billableDays &&
-    debit.section126Factor === credit.section126Factor &&
-    debit.section127Agreement === credit.section127Agreement &&
-    debit.section130Agreement === credit.section130Agreement &&
-    debit.aggregateFactor === credit.aggregateFactor &&
-    debit.adjustmentFactor === credit.adjustmentFactor &&
-    debit.winterOnly === credit.winterOnly &&
-    debit.supportedSource === credit.supportedSource &&
-    debit.supportedSourceName === credit.supportedSourceName &&
-    debit.waterCompanyCharge === credit.waterCompanyCharge
-}
-
 async function _fetch (licenceId, billingAccountId, financialYearEnding) {
   return db
     .select(

app/services/bill-runs/supplementary/process-transactions.service.js

@@ -8,6 +8,7 @@
  */
 
 const FetchPreviousTransactionsService = require('./fetch-previous-transactions.service.js')
+const { transactionsMatch } = require('../../../lib/general.lib.js')
 const ReverseTransactionsService = require('./reverse-transactions.service.js')
 
 /**
@@ -44,50 +45,13 @@ async function go (calculatedTransactions, bill, billLicence, billingPeriod) {
  * 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.
  *
- * 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 - winterOnly
- *
- * - Additional charges - supportedSource
- * - Additional charges - supportedSourceName
- * - Additional charges - waterCompanyCharge
- *
  * 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.section126Factor === calculatedTransaction.section126Factor &&
-      reversedTransaction.section127Agreement === calculatedTransaction.section127Agreement &&
-      reversedTransaction.section130Agreement === calculatedTransaction.section130Agreement &&
-      reversedTransaction.aggregateFactor === calculatedTransaction.aggregateFactor &&
-      reversedTransaction.adjustmentFactor === calculatedTransaction.adjustmentFactor &&
-      reversedTransaction.winterOnly === calculatedTransaction.winterOnly &&
-      reversedTransaction.supportedSource === calculatedTransaction.supportedSource &&
-      reversedTransaction.supportedSourceName === calculatedTransaction.supportedSourceName &&
-      reversedTransaction.waterCompanyCharge === calculatedTransaction.waterCompanyCharge
+    return transactionsMatch(reversedTransaction, calculatedTransaction)
   })
 
   if (result === -1) {

test/lib/general.lib.test.js

@@ -8,6 +8,9 @@ const Sinon = require('sinon')
 const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script()
 const { expect } = Code
 
+// Test helpers
+const TransactionHelper = require('../support/helpers/transaction.helper.js')
+
 // Thing under test
 const GeneralLib = require('../../app/lib/general.lib.js')
 
@@ -277,4 +280,185 @@ describe('GeneralLib', () => {
       expect(result).to.equal('2015-10-21T20:31:57.000Z')
     })
   })
+
+  describe('#transactionsMatch', () => {
+    let leftTransaction
+    let rightTransaction
+
+    beforeEach(() => {
+      // NOTE: The properties the function is comparing are; chargeType, chargeCategoryCode, billableDays,
+      // section126Factor, section127Agreement, section130Agreement, aggregateFactor, adjustmentFactor, winterOnly,
+      // supportedSource, supportedSourceName, waterCompanyCharge.
+      //
+      // We add IDs just so we can tell them apart!
+      leftTransaction = {
+        ...TransactionHelper.defaults(),
+        id: 'cba29373-d9a2-423e-8f36-83c13b07d925',
+        adjustmentFactor: 1,
+        aggregateFactor: 1,
+        chargeCategoryCode: '4.3.2',
+        section126Factor: 1,
+        section127Agreement: false,
+        supportedSource: false,
+        supportedSourceName: 'Severn',
+        waterCompanyCharge: false,
+        winterOnly: false
+      }
+      rightTransaction = { ...leftTransaction, id: '164eb779-4d2d-4578-bfbb-f07347e68171' }
+    })
+
+    describe('when the transactions match', () => {
+      it('returns true', () => {
+        const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+        expect(result).to.be.true()
+      })
+    })
+
+    describe('when the transactions do not match', () => {
+      describe('because the abatement agreement (section 126) is different', () => {
+        beforeEach(() => {
+          rightTransaction.section126Factor = 0.5
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the aggregate is different', () => {
+        beforeEach(() => {
+          rightTransaction.aggregateFactor = 0.5
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the billable days are different', () => {
+        beforeEach(() => {
+          rightTransaction.billableDays = 10
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the canal and river trust agreement (section 130) is different', () => {
+        beforeEach(() => {
+          rightTransaction.section130Agreement = true
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the charge adjustment is different', () => {
+        beforeEach(() => {
+          rightTransaction.adjustmentFactor = 0.5
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the charge category code is different', () => {
+        beforeEach(() => {
+          rightTransaction.chargeCategoryCode = '4.3.3'
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the charge type is different', () => {
+        beforeEach(() => {
+          rightTransaction.chargeType = 'compensation'
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the supported source differs (additional charge) is different', () => {
+        beforeEach(() => {
+          rightTransaction.supportedSource = true
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the supported source name differs (additional charge) is different', () => {
+        beforeEach(() => {
+          rightTransaction.supportedSourceName = 'source name'
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the two-part tariff agreement (section 127) is different', () => {
+        beforeEach(() => {
+          rightTransaction.section127Agreement = true
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the water company flag differs (additional charge) is different', () => {
+        beforeEach(() => {
+          rightTransaction.waterCompanyCharge = true
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+
+      describe('because the winter discount is different', () => {
+        beforeEach(() => {
+          rightTransaction.adjustmentFactor = true
+        })
+
+        it('returns false', () => {
+          const result = GeneralLib.transactionsMatch(leftTransaction, rightTransaction)
+
+          expect(result).to.be.false()
+        })
+      })
+    })
+  })
 })