From 49c9d9405b1e385a346ed7555e33e34daa0c1cc3 Mon Sep 17 00:00:00 2001 From: Rebecca Ransome Date: Wed, 20 Dec 2023 13:54:20 +0000 Subject: [PATCH 01/17] Persisting the results data from the two-part tariff match and allocate service https://eaflood.atlassian.net/browse/WATER-4188 As part of the work we have been doing, we've been hacking away at a branch in a mob to build the two-part-tariff service. As a result of this, we have some refactoring to do to tidy up the work we have done in our mob. We are keeping track of the refactoring to do in this issue DEFRA/water-abstraction-team#105. This PR will extract the `persistAllocatedLicencesToResultsService`out of our hacky branch and into its own PR along with the unit tests. From 021dffd17ed19cd8cfd956dae5b27715ffa6c9dc Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 21 Dec 2023 16:59:33 +0000 Subject: [PATCH 02/17] Create `PersistAllocatedLicencesToResultsService` --- ...t-allocated-licences-to-results.service.js | 131 ++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js new file mode 100644 index 0000000000..ed55fd3c27 --- /dev/null +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -0,0 +1,131 @@ +'use strict' + +/** + * Persists the results from the `allocateReturnsToLicenceService` into the DB + * @module PersistAllocatedLicencesToResultsService + */ +const { generateUUID } = require('../../../lib/general.lib.js') +const ReviewChargeElementResultModel = require('../../../models/review-charge-element-result.model.js') +const ReviewReturnResultModel = require('../../../models/review-return-result.model.js') +const ReviewResultModel = require('../../../models/review-result.model.js') + +/** + * Perisists the returnLogs and chargeElements processed from the `allocateReturnsToLicenceService` + * + * @param {String} billRunId UUID of the bill run this bill will be linked to + * @param {Object[]} licences licences with returns data to persist + * + */ +async function go (billRunId, licences) { + for (const licence of licences) { + const { chargeVersions, returnLogs } = licence + + const reviewReturnResultIds = await _persistReturnLogs(returnLogs, billRunId, licence) + + for (const chargeVersion of chargeVersions) { + const { chargeReferences } = chargeVersion + + for (const chargeReference of chargeReferences) { + const { chargeElements } = chargeReference + + for (const chargeElement of chargeElements) { + await _persistChargeElement(billRunId, licence, chargeVersion, chargeReference, chargeElement, reviewReturnResultIds) + } + } + } + } +} + +async function _persistChargeElement (billRunId, licence, chargeVersion, chargeReference, chargeElement, reviewReturnResultIds) { + const reviewChargeElementResultId = await _persistReviewChargeElementResult(chargeElement, chargeReference) + + // Persisting the charge elements that have a matching return + if (chargeElement.returnLogs.length > 0) { + for (const returnLog of chargeElement.returnLogs) { + const { reviewReturnResultId } = reviewReturnResultIds.find((reviewReturnResultIds) => { + return reviewReturnResultIds.returnId === returnLog.returnId + }) + + await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, reviewReturnResultId) + } + } else { + // Perisisting the charge element without any matching returns + await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, null) + } +} + +async function _persistReturnLogs (returnLogs, billRunId, licence) { + const reviewReturnResultIds = [] + + for (const returnLog of returnLogs) { + const reviewReturnResultId = generateUUID() + + await _persistReviewReturnResult(reviewReturnResultId, returnLog) + reviewReturnResultIds.push({ returnId: returnLog.id, reviewReturnResultId }) + + // Persisting the unmatched return logs + if (returnLog.matched === false) { + _persistReviewResult(billRunId, licence, null, null, null, reviewReturnResultId) + } + } + + return reviewReturnResultIds +} + +async function _persistReviewChargeElementResult (chargeElement, chargeReference) { + const reviewChargeElementResultId = generateUUID() + + const data = { + id: reviewChargeElementResultId, + chargeElementId: chargeElement.id, + allocated: chargeElement.allocatedQuantity, + aggregate: chargeReference.aggregate ?? 1, + chargeDatesOverlap: chargeElement.chargeDatesOverlap + } + + await ReviewChargeElementResultModel.query().insert(data) + + return reviewChargeElementResultId +} + +async function _persistReviewResult (billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, reviewReturnResultId) { + const data = { + billRunId, + licenceId: licence.id, + chargeVersionId: chargeVersion?.id, + chargeReferenceId: chargeReference?.id, + chargePeriodStartDate: chargeVersion?.chargePeriod.startDate, + chargePeriodEndDate: chargeVersion?.chargePeriod.endDate, + chargeVersionChangeReason: chargeVersion?.changeReason.description, + reviewChargeElementResultId, + reviewReturnResultId + } + + await ReviewResultModel.query().insert(data) +} + +async function _persistReviewReturnResult (reviewReturnResultId, returnLog) { + const data = { + id: reviewReturnResultId, + returnId: returnLog.id, + returnReference: returnLog.returnRequirement, + startDate: returnLog.startDate, + endDate: returnLog.endDate, + dueDate: returnLog.dueDate, + receivedDate: returnLog.receivedDate, + status: returnLog.status, + underQuery: returnLog.underQuery, + nilReturn: returnLog.nilReturn, + description: returnLog.description, + purposes: returnLog.purposes, + quantity: returnLog.quantity, + allocated: returnLog.allocatedQuantity, + abstractionOutsidePeriod: returnLog.abstractionOutsidePeriod + } + + await ReviewReturnResultModel.query().insert(data) +} + +module.exports = { + go +} From 552462eb09de83cb277f02ccd201f1ffcf802f46 Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 21 Dec 2023 17:06:15 +0000 Subject: [PATCH 03/17] Fix typos --- .../persist-allocated-licences-to-results.service.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index ed55fd3c27..544da5c5c9 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -10,7 +10,7 @@ const ReviewReturnResultModel = require('../../../models/review-return-result.mo const ReviewResultModel = require('../../../models/review-result.model.js') /** - * Perisists the returnLogs and chargeElements processed from the `allocateReturnsToLicenceService` + * Persists the returnLogs and chargeElements processed from the `allocateReturnsToLicenceService` * * @param {String} billRunId UUID of the bill run this bill will be linked to * @param {Object[]} licences licences with returns data to persist @@ -49,7 +49,7 @@ async function _persistChargeElement (billRunId, licence, chargeVersion, chargeR await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, reviewReturnResultId) } } else { - // Perisisting the charge element without any matching returns + // Persisting the charge element without any matching returns await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, null) } } From d59c054792d81dc544d9032face349f9a7b89d66 Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Fri, 22 Dec 2023 11:47:00 +0000 Subject: [PATCH 04/17] Refactor `reviewReturnResultId` now it's generated in the prep service --- ...t-allocated-licences-to-results.service.js | 31 ++++++------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index 544da5c5c9..9913675a2c 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -20,7 +20,7 @@ async function go (billRunId, licences) { for (const licence of licences) { const { chargeVersions, returnLogs } = licence - const reviewReturnResultIds = await _persistReturnLogs(returnLogs, billRunId, licence) + await _persistReturnLogs(returnLogs, billRunId, licence) for (const chargeVersion of chargeVersions) { const { chargeReferences } = chargeVersion @@ -29,24 +29,20 @@ async function go (billRunId, licences) { const { chargeElements } = chargeReference for (const chargeElement of chargeElements) { - await _persistChargeElement(billRunId, licence, chargeVersion, chargeReference, chargeElement, reviewReturnResultIds) + await _persistChargeElement(billRunId, licence, chargeVersion, chargeReference, chargeElement) } } } } } -async function _persistChargeElement (billRunId, licence, chargeVersion, chargeReference, chargeElement, reviewReturnResultIds) { +async function _persistChargeElement (billRunId, licence, chargeVersion, chargeReference, chargeElement) { const reviewChargeElementResultId = await _persistReviewChargeElementResult(chargeElement, chargeReference) // Persisting the charge elements that have a matching return if (chargeElement.returnLogs.length > 0) { - for (const returnLog of chargeElement.returnLogs) { - const { reviewReturnResultId } = reviewReturnResultIds.find((reviewReturnResultIds) => { - return reviewReturnResultIds.returnId === returnLog.returnId - }) - - await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, reviewReturnResultId) + for (const chargeElementReturnLog of chargeElement.returnLogs) { + await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, chargeElementReturnLog.reviewReturnResultId) } } else { // Persisting the charge element without any matching returns @@ -55,21 +51,14 @@ async function _persistChargeElement (billRunId, licence, chargeVersion, chargeR } async function _persistReturnLogs (returnLogs, billRunId, licence) { - const reviewReturnResultIds = [] - for (const returnLog of returnLogs) { - const reviewReturnResultId = generateUUID() + await _persistReviewReturnResult(returnLog) - await _persistReviewReturnResult(reviewReturnResultId, returnLog) - reviewReturnResultIds.push({ returnId: returnLog.id, reviewReturnResultId }) - - // Persisting the unmatched return logs + // Persists the unmatched return logs. The matched return logs will be persisted when processing the charge elements if (returnLog.matched === false) { - _persistReviewResult(billRunId, licence, null, null, null, reviewReturnResultId) + _persistReviewResult(billRunId, licence, null, null, null, returnLog.reviewReturnResultId) } } - - return reviewReturnResultIds } async function _persistReviewChargeElementResult (chargeElement, chargeReference) { @@ -104,9 +93,9 @@ async function _persistReviewResult (billRunId, licence, chargeVersion, chargeRe await ReviewResultModel.query().insert(data) } -async function _persistReviewReturnResult (reviewReturnResultId, returnLog) { +async function _persistReviewReturnResult (returnLog) { const data = { - id: reviewReturnResultId, + id: returnLog.reviewReturnResultId, returnId: returnLog.id, returnReference: returnLog.returnRequirement, startDate: returnLog.startDate, From 085404f2dc0a078fa6f0ea88e452f57acec44096 Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Fri, 22 Dec 2023 13:24:05 +0000 Subject: [PATCH 05/17] Update prepare service to generate the `reviewReturnResultId` --- .../prepare-licences-for-allocation.service.js | 5 ++++- .../prepare-licences-for-allocation.service.test.js | 4 +++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js index 47e82fc391..a621f9c22a 100644 --- a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js +++ b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js @@ -8,7 +8,7 @@ const DetermineAbstractionPeriodService = require('../determine-abstraction-periods.service.js') const DetermineChargePeriodService = require('../determine-charge-period.service.js') const FetchReturnLogsForLicenceService = require('./fetch-return-logs-for-licence.service.js') -const { periodsOverlap } = require('../../../lib/general.lib.js') +const { generateUUID, periodsOverlap } = require('../../../lib/general.lib.js') /** * For each licence finds returns for the billing period and prepares them and the licence's charge elements @@ -118,6 +118,9 @@ function _prepReturnsForMatching (returnLogs, billingPeriod) { returnLog.abstractionPeriods = abstractionPeriods returnLog.abstractionOutsidePeriod = abstractionOutsidePeriod returnLog.matched = false + // `reviewReturnResultId` will be the `id` of in the `reviewReturnResults` table for each return log, and is used + // to identify the matched return log when populating the `reviewResults` table for a charge element + returnLog.reviewReturnResultId = generateUUID() }) } diff --git a/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js b/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js index 036c89c58a..44661626ab 100644 --- a/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js +++ b/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js @@ -88,7 +88,9 @@ describe('Prepare Licences For Allocation Service', () => { } ], abstractionOutsidePeriod: false, - matched: false + matched: false, + // `reviewReturnResultId` is a randomly generated UUID + reviewReturnResultId: licence.returnLogs[0].reviewReturnResultId }) }) }) From 373bd4640fa601d1d66a3bba7efd408a00d57ddb Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 28 Dec 2023 17:25:12 +0000 Subject: [PATCH 06/17] Add `public` schema to DB helper --- test/support/helpers/database.helper.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/support/helpers/database.helper.js b/test/support/helpers/database.helper.js index d73751ead6..4498e1c9da 100644 --- a/test/support/helpers/database.helper.js +++ b/test/support/helpers/database.helper.js @@ -19,7 +19,7 @@ const { db } = require('../../../db/db.js') * identity columns. For example, if a table relies on an incrementing ID the query will reset that to 1. */ async function clean () { - const schemas = ['crm_v2', 'idm', 'water', 'returns'] + const schemas = ['crm_v2', 'idm', 'public', 'water', 'returns'] for (const schema of schemas) { const tables = await _tableNames(schema) From 3a1e2ac5defd1f0d2c6aab85972c1a64df639f4e Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 28 Dec 2023 22:10:10 +0000 Subject: [PATCH 07/17] Create unit tests for persisting service --- ...ocated-licences-to-results.service.test.js | 220 ++++++++++++++++++ 1 file changed, 220 insertions(+) create mode 100644 test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js diff --git a/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js new file mode 100644 index 0000000000..db4361682a --- /dev/null +++ b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js @@ -0,0 +1,220 @@ +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') + +const { describe, it, beforeEach } = exports.lab = Lab.script() +const { expect } = Code + +// Test helpers +const DatabaseHelper = require('../../../support/helpers/database.helper.js') +const { generateUUID } = require('../../../../app/lib/general.lib.js') +const { generateReturnLogId } = require('../../../support/helpers/return-log.helper.js') +const ReviewResultModel = require('../../../../app/models/review-result.model.js') + +// Thing under test +const PersistAllocatedLicencesToResultsService = require('../../../../app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js') + +describe('Persist Allocated Licences to Results service', () => { + const billRunId = generateUUID() + + beforeEach(async () => { + await DatabaseHelper.clean() + }) + + describe('when there are valid records to be persisted', () => { + let testLicences + + describe('with a charge element that has been matched to a return', () => { + beforeEach(() => { + testLicences = _generateData() + }) + + it('persists the data into the results tables', async () => { + await PersistAllocatedLicencesToResultsService.go(billRunId, testLicences) + + const result = await ReviewResultModel.query() + .where('licenceId', testLicences[0].id) + .withGraphFetched('reviewChargeElementResults') + .withGraphFetched('reviewReturnResults') + + expect(result[0].billRunId).to.equal(billRunId) + expect(result[0].licenceId).to.equal(testLicences[0].id) + expect(result[0].chargeVersionId).to.equal(testLicences[0].chargeVersions[0].id) + expect(result[0].chargeReferenceId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].id) + expect(result[0].chargePeriodStartDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.startDate) + expect(result[0].chargePeriodEndDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.endDate) + expect(result[0].chargeVersionChangeReason).to.equal(testLicences[0].chargeVersions[0].changeReason.description) + expect(result[0].reviewReturnResultId).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) + + expect(result[0].reviewChargeElementResults.chargeElementId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id) + expect(result[0].reviewChargeElementResults.allocated).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity) + // As the aggregate is null on the charge reference the service returns 1 + expect(result[0].reviewChargeElementResults.aggregate).to.equal(1) + expect(result[0].reviewChargeElementResults.chargeDatesOverlap).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap) + + expect(result[0].reviewReturnResults.id).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) + expect(result[0].reviewReturnResults.returnId).to.equal(testLicences[0].returnLogs[0].id) + expect(result[0].reviewReturnResults.returnReference).to.equal(testLicences[0].returnLogs[0].returnRequirement) + expect(result[0].reviewReturnResults.startDate).to.equal(testLicences[0].returnLogs[0].startDate) + expect(result[0].reviewReturnResults.endDate).to.equal(testLicences[0].returnLogs[0].endDate) + expect(result[0].reviewReturnResults.dueDate).to.equal(testLicences[0].returnLogs[0].dueDate) + expect(result[0].reviewReturnResults.receivedDate).to.equal(testLicences[0].returnLogs[0].receivedDate) + expect(result[0].reviewReturnResults.status).to.equal(testLicences[0].returnLogs[0].status) + expect(result[0].reviewReturnResults.underQuery).to.equal(testLicences[0].returnLogs[0].underQuery) + expect(result[0].reviewReturnResults.nilReturn).to.equal(testLicences[0].returnLogs[0].nilReturn) + expect(result[0].reviewReturnResults.description).to.equal(testLicences[0].returnLogs[0].description) + expect(result[0].reviewReturnResults.purposes).to.equal(testLicences[0].returnLogs[0].purposes) + expect(result[0].reviewReturnResults.quantity).to.equal(testLicences[0].returnLogs[0].quantity) + expect(result[0].reviewReturnResults.allocated).to.equal(testLicences[0].returnLogs[0].allocatedQuantity) + expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal(testLicences[0].returnLogs[0].abstractionOutsidePeriod) + }) + }) + + describe('with an aggregate value set and a charge element that has NOT been matched to a return', () => { + beforeEach(() => { + const aggregate = 0.5 + const returnMatched = false + + testLicences = _generateData(aggregate, returnMatched) + }) + + it('persists the data into the results tables', async () => { + await PersistAllocatedLicencesToResultsService.go(billRunId, testLicences) + + const result = await ReviewResultModel.query() + .where('licenceId', testLicences[0].id) + .withGraphFetched('reviewChargeElementResults') + .withGraphFetched('reviewReturnResults') + + expect(result[0].billRunId).to.equal(billRunId) + expect(result[0].licenceId).to.equal(testLicences[0].id) + expect(result[0].chargeVersionId).to.be.null() + expect(result[0].chargeReferenceId).to.be.null() + expect(result[0].chargePeriodStartDate).to.be.null() + expect(result[0].chargePeriodEndDate).to.be.null() + expect(result[0].chargeVersionChangeReason).to.be.null() + expect(result[0].reviewChargeElementResultId).to.be.null() + expect(result[0].reviewReturnResultId).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) + + expect(result[0].reviewChargeElementResults).to.be.null() + + expect(result[0].reviewReturnResults.id).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) + expect(result[0].reviewReturnResults.returnId).to.equal(testLicences[0].returnLogs[0].id) + expect(result[0].reviewReturnResults.returnReference).to.equal(testLicences[0].returnLogs[0].returnRequirement) + expect(result[0].reviewReturnResults.startDate).to.equal(testLicences[0].returnLogs[0].startDate) + expect(result[0].reviewReturnResults.endDate).to.equal(testLicences[0].returnLogs[0].endDate) + expect(result[0].reviewReturnResults.dueDate).to.equal(testLicences[0].returnLogs[0].dueDate) + expect(result[0].reviewReturnResults.receivedDate).to.equal(testLicences[0].returnLogs[0].receivedDate) + expect(result[0].reviewReturnResults.status).to.equal(testLicences[0].returnLogs[0].status) + expect(result[0].reviewReturnResults.underQuery).to.equal(testLicences[0].returnLogs[0].underQuery) + expect(result[0].reviewReturnResults.nilReturn).to.equal(testLicences[0].returnLogs[0].nilReturn) + expect(result[0].reviewReturnResults.description).to.equal(testLicences[0].returnLogs[0].description) + expect(result[0].reviewReturnResults.purposes).to.equal(testLicences[0].returnLogs[0].purposes) + expect(result[0].reviewReturnResults.quantity).to.equal(testLicences[0].returnLogs[0].quantity) + expect(result[0].reviewReturnResults.allocated).to.equal(testLicences[0].returnLogs[0].allocatedQuantity) + expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal(testLicences[0].returnLogs[0].abstractionOutsidePeriod) + + expect(result[1].billRunId).to.equal(billRunId) + expect(result[1].licenceId).to.equal(testLicences[0].id) + expect(result[1].chargeVersionId).to.equal(testLicences[0].chargeVersions[0].id) + expect(result[1].chargeReferenceId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].id) + expect(result[1].chargePeriodStartDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.startDate) + expect(result[1].chargePeriodEndDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.endDate) + expect(result[1].chargeVersionChangeReason).to.equal(testLicences[0].chargeVersions[0].changeReason.description) + expect(result[1].reviewReturnResultId).to.be.null() + + expect(result[1].reviewChargeElementResults.chargeElementId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id) + expect(result[1].reviewChargeElementResults.allocated).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity) + expect(result[1].reviewChargeElementResults.aggregate).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].aggregate) + expect(result[1].reviewChargeElementResults.chargeDatesOverlap).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap) + + expect(result[1].reviewReturnResults).to.be.null() + }) + }) + }) + + describe('when there are NO records to persist', () => { + const testLicences = [] + + it('does not throw an error', () => { + expect(async () => await PersistAllocatedLicencesToResultsService.go(billRunId, testLicences)).to.not.throw() + }) + }) +}) + +function _generateData (aggregate = null, returnMatched = true) { + const returnId = generateReturnLogId() + const reviewReturnResultId = generateUUID() + + const chargeElementReturnLogs = [ + { + allocatedQuantity: 32, + returnId, + reviewReturnResultId + } + ] + + // All data not required for the tests has been excluded from the generated data + const dataToPersist = [ + { + id: generateUUID(), + chargeVersions: [ + { + id: generateUUID(), + changeReason: { + description: 'Strategic review of charges (SRoC)' + }, + chargeReferences: [ + { + id: generateUUID(), + aggregate, + chargeElements: [ + { + id: generateUUID(), + returnLogs: returnMatched ? chargeElementReturnLogs : [], + allocatedQuantity: returnMatched ? 32 : 0, + chargeDatesOverlap: false + } + ] + } + ], + chargePeriod: { + startDate: new Date('2022-04-01'), + endDate: new Date('2023-03-31') + } + } + ], + returnLogs: [ + { + id: returnId, + returnRequirement: '10021668', + description: 'DRAINS ETC-DEEPING FEN AND OTHER LINKED SITES', + startDate: new Date('2022-04-01'), + endDate: new Date('2023-03-31'), + receivedDate: new Date('2023-03-01'), + dueDate: new Date('2023-04-28'), + status: 'completed', + underQuery: false, + purposes: [ + { + tertiary: { + code: '400', + description: 'Spray Irrigation - Direct' + } + } + ], + nilReturn: false, + quantity: 32, + allocatedQuantity: returnMatched ? 32 : 0, + abstractionOutsidePeriod: false, + matched: returnMatched, + reviewReturnResultId + } + ] + } + ] + + return dataToPersist +} From d1381c6ea710312f8fa773d0a01114dafbab781a Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 28 Dec 2023 22:20:09 +0000 Subject: [PATCH 08/17] Fix migration scripts --- .../20231122121546_create-review-charge-element-results.js | 4 ++-- .../public/20231122145716_create-review-return-results.js | 4 ++-- db/migrations/public/20231122152504_create-review-results.js | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/db/migrations/public/20231122121546_create-review-charge-element-results.js b/db/migrations/public/20231122121546_create-review-charge-element-results.js index 9606e1d21c..9b123d7688 100644 --- a/db/migrations/public/20231122121546_create-review-charge-element-results.js +++ b/db/migrations/public/20231122121546_create-review-charge-element-results.js @@ -11,8 +11,8 @@ exports.up = function (knex) { // Data table.uuid('charge_element_id').notNullable() - table.decimal('allocated').defaultTo(0) - table.decimal('aggregate').defaultTo(1) + table.decimal('allocated', null, null).defaultTo(0) + table.decimal('aggregate', null, null).defaultTo(1) table.boolean('charge_dates_overlap').defaultTo(false) // Automatic timestamps diff --git a/db/migrations/public/20231122145716_create-review-return-results.js b/db/migrations/public/20231122145716_create-review-return-results.js index 1e6659cf19..e1d6fcdb9f 100644 --- a/db/migrations/public/20231122145716_create-review-return-results.js +++ b/db/migrations/public/20231122145716_create-review-return-results.js @@ -21,8 +21,8 @@ exports.up = function (knex) { table.boolean('nil_return').defaultTo(false) table.string('description') table.jsonb('purposes') - table.decimal('quantity').defaultTo(0) - table.decimal('allocated').defaultTo(0) + table.decimal('quantity', null, null).defaultTo(0) + table.decimal('allocated', null, null).defaultTo(0) table.boolean('abstraction_outside_period').defaultTo(false) // Automatic timestamps diff --git a/db/migrations/public/20231122152504_create-review-results.js b/db/migrations/public/20231122152504_create-review-results.js index 8b69c0e92f..e94b0d184f 100644 --- a/db/migrations/public/20231122152504_create-review-results.js +++ b/db/migrations/public/20231122152504_create-review-results.js @@ -12,8 +12,8 @@ exports.up = function (knex) { // Data table.uuid('bill_run_id').notNullable() table.uuid('licence_id').notNullable() - table.uuid('charge_version_id').notNullable() - table.uuid('charge_reference_id').notNullable() + table.uuid('charge_version_id') + table.uuid('charge_reference_id') table.date('charge_period_start_date') table.date('charge_period_end_date') table.string('charge_version_change_reason') From 98a057b57f8649d347633f7b126fd1075dad6dce Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Thu, 28 Dec 2023 22:28:17 +0000 Subject: [PATCH 09/17] Tidy up --- ...ocated-licences-to-results.service.test.js | 36 ++++++++++++++----- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js index db4361682a..f88a640730 100644 --- a/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js +++ b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js @@ -48,11 +48,17 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[0].chargeVersionChangeReason).to.equal(testLicences[0].chargeVersions[0].changeReason.description) expect(result[0].reviewReturnResultId).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) - expect(result[0].reviewChargeElementResults.chargeElementId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id) - expect(result[0].reviewChargeElementResults.allocated).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity) + expect(result[0].reviewChargeElementResults.chargeElementId).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id + ) + expect(result[0].reviewChargeElementResults.allocated).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity + ) // As the aggregate is null on the charge reference the service returns 1 expect(result[0].reviewChargeElementResults.aggregate).to.equal(1) - expect(result[0].reviewChargeElementResults.chargeDatesOverlap).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap) + expect(result[0].reviewChargeElementResults.chargeDatesOverlap).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap + ) expect(result[0].reviewReturnResults.id).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) expect(result[0].reviewReturnResults.returnId).to.equal(testLicences[0].returnLogs[0].id) @@ -68,7 +74,9 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[0].reviewReturnResults.purposes).to.equal(testLicences[0].returnLogs[0].purposes) expect(result[0].reviewReturnResults.quantity).to.equal(testLicences[0].returnLogs[0].quantity) expect(result[0].reviewReturnResults.allocated).to.equal(testLicences[0].returnLogs[0].allocatedQuantity) - expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal(testLicences[0].returnLogs[0].abstractionOutsidePeriod) + expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal( + testLicences[0].returnLogs[0].abstractionOutsidePeriod + ) }) }) @@ -114,7 +122,9 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[0].reviewReturnResults.purposes).to.equal(testLicences[0].returnLogs[0].purposes) expect(result[0].reviewReturnResults.quantity).to.equal(testLicences[0].returnLogs[0].quantity) expect(result[0].reviewReturnResults.allocated).to.equal(testLicences[0].returnLogs[0].allocatedQuantity) - expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal(testLicences[0].returnLogs[0].abstractionOutsidePeriod) + expect(result[0].reviewReturnResults.abstractionOutsidePeriod).to.equal( + testLicences[0].returnLogs[0].abstractionOutsidePeriod + ) expect(result[1].billRunId).to.equal(billRunId) expect(result[1].licenceId).to.equal(testLicences[0].id) @@ -125,10 +135,18 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[1].chargeVersionChangeReason).to.equal(testLicences[0].chargeVersions[0].changeReason.description) expect(result[1].reviewReturnResultId).to.be.null() - expect(result[1].reviewChargeElementResults.chargeElementId).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id) - expect(result[1].reviewChargeElementResults.allocated).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity) - expect(result[1].reviewChargeElementResults.aggregate).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].aggregate) - expect(result[1].reviewChargeElementResults.chargeDatesOverlap).to.equal(testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap) + expect(result[1].reviewChargeElementResults.chargeElementId).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id + ) + expect(result[1].reviewChargeElementResults.allocated).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].allocatedQuantity + ) + expect(result[1].reviewChargeElementResults.aggregate).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].aggregate + ) + expect(result[1].reviewChargeElementResults.chargeDatesOverlap).to.equal( + testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap + ) expect(result[1].reviewReturnResults).to.be.null() }) From aad4dfc2fc994d22be716c21b341acdcec52c752 Mon Sep 17 00:00:00 2001 From: Rebecca Ransome Date: Wed, 3 Jan 2024 10:43:27 +0000 Subject: [PATCH 10/17] Updated comment --- .../prepare-licences-for-allocation.service.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js index a621f9c22a..4e2652398f 100644 --- a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js +++ b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js @@ -118,8 +118,8 @@ function _prepReturnsForMatching (returnLogs, billingPeriod) { returnLog.abstractionPeriods = abstractionPeriods returnLog.abstractionOutsidePeriod = abstractionOutsidePeriod returnLog.matched = false - // `reviewReturnResultId` will be the `id` of in the `reviewReturnResults` table for each return log, and is used - // to identify the matched return log when populating the `reviewResults` table for a charge element + // `reviewReturnResultId` is the `id` in the `reviewReturnResults` table for each return log. It helps + // to identify the matched return log when populating the `reviewResults` table for a charge element. returnLog.reviewReturnResultId = generateUUID() }) } From f29f5363a8dfea8d24c4867c805809fd497e0f02 Mon Sep 17 00:00:00 2001 From: Jason Claxton <30830544+Jozzey@users.noreply.github.com> Date: Mon, 8 Jan 2024 14:25:35 +0000 Subject: [PATCH 11/17] Add comment to migration to explain the decimal Co-authored-by: Alan Cruikshanks --- .../20231122121546_create-review-charge-element-results.js | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrations/public/20231122121546_create-review-charge-element-results.js b/db/migrations/public/20231122121546_create-review-charge-element-results.js index 9b123d7688..d8fc4509f8 100644 --- a/db/migrations/public/20231122121546_create-review-charge-element-results.js +++ b/db/migrations/public/20231122121546_create-review-charge-element-results.js @@ -11,6 +11,7 @@ exports.up = function (knex) { // Data table.uuid('charge_element_id').notNullable() + // Specifying `null, null` creates a decimal column that can store numbers of any precision and scale table.decimal('allocated', null, null).defaultTo(0) table.decimal('aggregate', null, null).defaultTo(1) table.boolean('charge_dates_overlap').defaultTo(false) From 4df73d19c3e792eab60c05babe0208594f1caeb4 Mon Sep 17 00:00:00 2001 From: Jason Claxton <30830544+Jozzey@users.noreply.github.com> Date: Mon, 8 Jan 2024 14:25:56 +0000 Subject: [PATCH 12/17] Add comment to migration to explain the decimal Co-authored-by: Alan Cruikshanks --- .../public/20231122145716_create-review-return-results.js | 1 + 1 file changed, 1 insertion(+) diff --git a/db/migrations/public/20231122145716_create-review-return-results.js b/db/migrations/public/20231122145716_create-review-return-results.js index e1d6fcdb9f..4e2db69e18 100644 --- a/db/migrations/public/20231122145716_create-review-return-results.js +++ b/db/migrations/public/20231122145716_create-review-return-results.js @@ -21,6 +21,7 @@ exports.up = function (knex) { table.boolean('nil_return').defaultTo(false) table.string('description') table.jsonb('purposes') + // Specifying `null, null` creates a decimal column that can store numbers of any precision and scale table.decimal('quantity', null, null).defaultTo(0) table.decimal('allocated', null, null).defaultTo(0) table.boolean('abstraction_outside_period').defaultTo(false) From 85b16006f33c516ef0b57ed9390c57198a7be6cb Mon Sep 17 00:00:00 2001 From: Jason Claxton <30830544+Jozzey@users.noreply.github.com> Date: Mon, 8 Jan 2024 14:27:30 +0000 Subject: [PATCH 13/17] Add Alan's comment Co-authored-by: Alan Cruikshanks --- ...ersist-allocated-licences-to-results.service.js | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index 9913675a2c..7c6fe4874f 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -10,11 +10,19 @@ const ReviewReturnResultModel = require('../../../models/review-return-result.mo const ReviewResultModel = require('../../../models/review-result.model.js') /** - * Persists the returnLogs and chargeElements processed from the `allocateReturnsToLicenceService` + * Persists results of matching and allocating return logs to licence charge elements for a two-part tariff bill run * - * @param {String} billRunId UUID of the bill run this bill will be linked to - * @param {Object[]} licences licences with returns data to persist + * Each licence will have a `returnLogs` property that contains all return logs linked to the licence for the billing + * period of the bill run. It will also have a `chargeVersions` property, which within each one it will have details of + * which charge elements matched to which return logs and whether anything was allocated to the charge element from + * them. * + * We need to persist all this information ready for use in the review screens that our users use to asses if the + * matching and allocating looks correct or if any issues need resolving first. + * + * @param {String} billRunId - the ID of the two-part tariff bill run being generated + * @param {Object[]} licences - the two-part tariff licences included in the bill run, along with their match and + * allocation results */ async function go (billRunId, licences) { for (const licence of licences) { From a68d696e859b390ce64dfb12f15294150fbbf142 Mon Sep 17 00:00:00 2001 From: Jason Claxton Date: Mon, 8 Jan 2024 15:15:44 +0000 Subject: [PATCH 14/17] Move where `reviewReturnResultId` gets set --- ...t-allocated-licences-to-results.service.js | 61 +++++++++++++++---- ...prepare-licences-for-allocation.service.js | 5 +- ...ocated-licences-to-results.service.test.js | 11 +--- ...re-licences-for-allocation.service.test.js | 4 +- 4 files changed, 54 insertions(+), 27 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index 7c6fe4874f..910d9e6c8a 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -28,7 +28,7 @@ async function go (billRunId, licences) { for (const licence of licences) { const { chargeVersions, returnLogs } = licence - await _persistReturnLogs(returnLogs, billRunId, licence) + const reviewReturnResultIds = await _persistReturnLogs(returnLogs, billRunId, licence) for (const chargeVersion of chargeVersions) { const { chargeReferences } = chargeVersion @@ -37,20 +37,45 @@ async function go (billRunId, licences) { const { chargeElements } = chargeReference for (const chargeElement of chargeElements) { - await _persistChargeElement(billRunId, licence, chargeVersion, chargeReference, chargeElement) + await _persistChargeElement( + billRunId, + licence, + chargeVersion, + chargeReference, + chargeElement, + reviewReturnResultIds + ) } } } } } -async function _persistChargeElement (billRunId, licence, chargeVersion, chargeReference, chargeElement) { +async function _persistChargeElement ( + billRunId, + licence, + chargeVersion, + chargeReference, + chargeElement, + reviewReturnResultIds +) { const reviewChargeElementResultId = await _persistReviewChargeElementResult(chargeElement, chargeReference) // Persisting the charge elements that have a matching return if (chargeElement.returnLogs.length > 0) { - for (const chargeElementReturnLog of chargeElement.returnLogs) { - await _persistReviewResult(billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, chargeElementReturnLog.reviewReturnResultId) + for (const returnLog of chargeElement.returnLogs) { + const { reviewReturnResultId } = reviewReturnResultIds.find((reviewReturnResultIds) => { + return reviewReturnResultIds.returnId === returnLog.returnId + }) + + await _persistReviewResult( + billRunId, + licence, + chargeVersion, + chargeReference, + reviewChargeElementResultId, + reviewReturnResultId + ) } } else { // Persisting the charge element without any matching returns @@ -59,14 +84,21 @@ async function _persistChargeElement (billRunId, licence, chargeVersion, chargeR } async function _persistReturnLogs (returnLogs, billRunId, licence) { + const reviewReturnResultIds = [] + for (const returnLog of returnLogs) { - await _persistReviewReturnResult(returnLog) + const reviewReturnResultId = generateUUID() + + await _persistReviewReturnResult(reviewReturnResultId, returnLog) + reviewReturnResultIds.push({ returnId: returnLog.id, reviewReturnResultId }) - // Persists the unmatched return logs. The matched return logs will be persisted when processing the charge elements + // Persisting the unmatched return logs if (returnLog.matched === false) { - _persistReviewResult(billRunId, licence, null, null, null, returnLog.reviewReturnResultId) + _persistReviewResult(billRunId, licence, null, null, null, reviewReturnResultId) } } + + return reviewReturnResultIds } async function _persistReviewChargeElementResult (chargeElement, chargeReference) { @@ -85,7 +117,14 @@ async function _persistReviewChargeElementResult (chargeElement, chargeReference return reviewChargeElementResultId } -async function _persistReviewResult (billRunId, licence, chargeVersion, chargeReference, reviewChargeElementResultId, reviewReturnResultId) { +async function _persistReviewResult ( + billRunId, + licence, + chargeVersion, + chargeReference, + reviewChargeElementResultId, + reviewReturnResultId +) { const data = { billRunId, licenceId: licence.id, @@ -101,9 +140,9 @@ async function _persistReviewResult (billRunId, licence, chargeVersion, chargeRe await ReviewResultModel.query().insert(data) } -async function _persistReviewReturnResult (returnLog) { +async function _persistReviewReturnResult (reviewReturnResultId, returnLog) { const data = { - id: returnLog.reviewReturnResultId, + id: reviewReturnResultId, returnId: returnLog.id, returnReference: returnLog.returnRequirement, startDate: returnLog.startDate, diff --git a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js index 4e2652398f..47e82fc391 100644 --- a/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js +++ b/app/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.js @@ -8,7 +8,7 @@ const DetermineAbstractionPeriodService = require('../determine-abstraction-periods.service.js') const DetermineChargePeriodService = require('../determine-charge-period.service.js') const FetchReturnLogsForLicenceService = require('./fetch-return-logs-for-licence.service.js') -const { generateUUID, periodsOverlap } = require('../../../lib/general.lib.js') +const { periodsOverlap } = require('../../../lib/general.lib.js') /** * For each licence finds returns for the billing period and prepares them and the licence's charge elements @@ -118,9 +118,6 @@ function _prepReturnsForMatching (returnLogs, billingPeriod) { returnLog.abstractionPeriods = abstractionPeriods returnLog.abstractionOutsidePeriod = abstractionOutsidePeriod returnLog.matched = false - // `reviewReturnResultId` is the `id` in the `reviewReturnResults` table for each return log. It helps - // to identify the matched return log when populating the `reviewResults` table for a charge element. - returnLog.reviewReturnResultId = generateUUID() }) } diff --git a/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js index f88a640730..8d647df1d1 100644 --- a/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js +++ b/test/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.test.js @@ -46,7 +46,6 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[0].chargePeriodStartDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.startDate) expect(result[0].chargePeriodEndDate).to.equal(testLicences[0].chargeVersions[0].chargePeriod.endDate) expect(result[0].chargeVersionChangeReason).to.equal(testLicences[0].chargeVersions[0].changeReason.description) - expect(result[0].reviewReturnResultId).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) expect(result[0].reviewChargeElementResults.chargeElementId).to.equal( testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].id @@ -60,7 +59,6 @@ describe('Persist Allocated Licences to Results service', () => { testLicences[0].chargeVersions[0].chargeReferences[0].chargeElements[0].chargeDatesOverlap ) - expect(result[0].reviewReturnResults.id).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) expect(result[0].reviewReturnResults.returnId).to.equal(testLicences[0].returnLogs[0].id) expect(result[0].reviewReturnResults.returnReference).to.equal(testLicences[0].returnLogs[0].returnRequirement) expect(result[0].reviewReturnResults.startDate).to.equal(testLicences[0].returnLogs[0].startDate) @@ -104,11 +102,9 @@ describe('Persist Allocated Licences to Results service', () => { expect(result[0].chargePeriodEndDate).to.be.null() expect(result[0].chargeVersionChangeReason).to.be.null() expect(result[0].reviewChargeElementResultId).to.be.null() - expect(result[0].reviewReturnResultId).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) expect(result[0].reviewChargeElementResults).to.be.null() - expect(result[0].reviewReturnResults.id).to.equal(testLicences[0].returnLogs[0].reviewReturnResultId) expect(result[0].reviewReturnResults.returnId).to.equal(testLicences[0].returnLogs[0].id) expect(result[0].reviewReturnResults.returnReference).to.equal(testLicences[0].returnLogs[0].returnRequirement) expect(result[0].reviewReturnResults.startDate).to.equal(testLicences[0].returnLogs[0].startDate) @@ -164,13 +160,11 @@ describe('Persist Allocated Licences to Results service', () => { function _generateData (aggregate = null, returnMatched = true) { const returnId = generateReturnLogId() - const reviewReturnResultId = generateUUID() const chargeElementReturnLogs = [ { allocatedQuantity: 32, - returnId, - reviewReturnResultId + returnId } ] @@ -227,8 +221,7 @@ function _generateData (aggregate = null, returnMatched = true) { quantity: 32, allocatedQuantity: returnMatched ? 32 : 0, abstractionOutsidePeriod: false, - matched: returnMatched, - reviewReturnResultId + matched: returnMatched } ] } diff --git a/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js b/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js index 44661626ab..036c89c58a 100644 --- a/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js +++ b/test/services/bill-runs/two-part-tariff/prepare-licences-for-allocation.service.test.js @@ -88,9 +88,7 @@ describe('Prepare Licences For Allocation Service', () => { } ], abstractionOutsidePeriod: false, - matched: false, - // `reviewReturnResultId` is a randomly generated UUID - reviewReturnResultId: licence.returnLogs[0].reviewReturnResultId + matched: false }) }) }) From f30b909fcee093512bf00f9ea099199a759b99b9 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 16:34:43 +0000 Subject: [PATCH 15/17] Move where ID is generated Co-authored-by: Jason Claxton Co-authored-by: Becky Ransome --- .../persist-allocated-licences-to-results.service.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index 910d9e6c8a..ab4aa8660f 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -87,9 +87,7 @@ async function _persistReturnLogs (returnLogs, billRunId, licence) { const reviewReturnResultIds = [] for (const returnLog of returnLogs) { - const reviewReturnResultId = generateUUID() - - await _persistReviewReturnResult(reviewReturnResultId, returnLog) + const reviewReturnResultId = await _persistReviewReturnResult(returnLog) reviewReturnResultIds.push({ returnId: returnLog.id, reviewReturnResultId }) // Persisting the unmatched return logs @@ -140,7 +138,9 @@ async function _persistReviewResult ( await ReviewResultModel.query().insert(data) } -async function _persistReviewReturnResult (reviewReturnResultId, returnLog) { +async function _persistReviewReturnResult (returnLog) { + const reviewReturnResultId = generateUUID() + const data = { id: reviewReturnResultId, returnId: returnLog.id, @@ -160,6 +160,8 @@ async function _persistReviewReturnResult (reviewReturnResultId, returnLog) { } await ReviewReturnResultModel.query().insert(data) + + return reviewReturnResultId } module.exports = { From f287c73ab65060eb1160266356b64c5aac2771be Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Mon, 8 Jan 2024 16:47:07 +0000 Subject: [PATCH 16/17] Update to use DB Id rather than pre-generated one We can avoid bringing in an extra module and let the DB generate the ID by using [returning()](https://vincit.github.io/objection.js/api/query-builder/find-methods.html#returning). We initially avoided it because it gave the impression that it ignores what we ask the DB to return and instead returns everything. But [this issue](https://github.com/Vincit/objection.js/issues/1773) and our own testing confirmed Objection.js is just combining what we passed in with what we asked the DB to return, which gives the impression the DB is sending back more than we wanted. --- .../persist-allocated-licences-to-results.service.js | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index ab4aa8660f..b81ee8026c 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -4,7 +4,7 @@ * Persists the results from the `allocateReturnsToLicenceService` into the DB * @module PersistAllocatedLicencesToResultsService */ -const { generateUUID } = require('../../../lib/general.lib.js') + const ReviewChargeElementResultModel = require('../../../models/review-charge-element-result.model.js') const ReviewReturnResultModel = require('../../../models/review-return-result.model.js') const ReviewResultModel = require('../../../models/review-result.model.js') @@ -100,17 +100,14 @@ async function _persistReturnLogs (returnLogs, billRunId, licence) { } async function _persistReviewChargeElementResult (chargeElement, chargeReference) { - const reviewChargeElementResultId = generateUUID() - const data = { - id: reviewChargeElementResultId, chargeElementId: chargeElement.id, allocated: chargeElement.allocatedQuantity, aggregate: chargeReference.aggregate ?? 1, chargeDatesOverlap: chargeElement.chargeDatesOverlap } - await ReviewChargeElementResultModel.query().insert(data) + const { id: reviewChargeElementResultId } = await ReviewChargeElementResultModel.query().insert(data).returning('id') return reviewChargeElementResultId } @@ -139,10 +136,7 @@ async function _persistReviewResult ( } async function _persistReviewReturnResult (returnLog) { - const reviewReturnResultId = generateUUID() - const data = { - id: reviewReturnResultId, returnId: returnLog.id, returnReference: returnLog.returnRequirement, startDate: returnLog.startDate, @@ -159,7 +153,7 @@ async function _persistReviewReturnResult (returnLog) { abstractionOutsidePeriod: returnLog.abstractionOutsidePeriod } - await ReviewReturnResultModel.query().insert(data) + const { id: reviewReturnResultId } = await ReviewReturnResultModel.query().insert(data).returning('id') return reviewReturnResultId } From 8535cab5b9a8cda85123a8401767e63d44100844 Mon Sep 17 00:00:00 2001 From: Jason Claxton <30830544+Jozzey@users.noreply.github.com> Date: Mon, 8 Jan 2024 17:06:29 +0000 Subject: [PATCH 17/17] Add comment to explain process Co-authored-by: Alan Cruikshanks --- .../persist-allocated-licences-to-results.service.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js index b81ee8026c..d844381c3d 100644 --- a/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js +++ b/app/services/bill-runs/two-part-tariff/persist-allocated-licences-to-results.service.js @@ -64,6 +64,11 @@ async function _persistChargeElement ( // Persisting the charge elements that have a matching return if (chargeElement.returnLogs.length > 0) { for (const returnLog of chargeElement.returnLogs) { + // When we persist the review result we need the Id's for both the charge element and return log's review result + // records. Though it looks like we're iterating return logs here, these are copies assigned during matching and + // allocation. We don't create `ReviewReturnResult` records until this service is called, and those are based + // on the `returnLogs` property of each licence. Hence, we need to pass in the ID's created and search them for + // a match in order to get the `reviewReturnResultId`. const { reviewReturnResultId } = reviewReturnResultIds.find((reviewReturnResultIds) => { return reviewReturnResultIds.returnId === returnLog.returnId })