From 875f4780b35ba4007d0d7a9c370c8329e0832488 Mon Sep 17 00:00:00 2001 From: Becky Ransome <91424856+Beckyrose200@users.noreply.github.com> Date: Thu, 29 Feb 2024 15:04:58 +0000 Subject: [PATCH] Determine match and allocate issues and status https://eaflood.atlassian.net/browse/WATER-4378 During the refinement of future two-part tariff tickets, it was noted that the status of a licence could be changed by the user from a 'review' status to a 'ready' status. Before this was noted we were working out the licence status based on the issues the licence has with its returns and charge elements. This is done on every page where the licence status is being shown. Now that we know a status can be overwritten regardless of its issues, we now need to persist the licence status during the first time the engine works it out and show the persisted status instead of working it out each time. This will allow users to manually change the status. The same thing applies to the charge elements. The status of the element is determined by the issues on it. This status at the moment is worked out every time it is displayed on a page. We have since learnt through refinement that a user can allocate volumes on the element and this will change the status from 'review' to 'ready'. This is regardless of the issues that remain on a charge element. So with this new information, it has made it clear that we now need to persist the initial status of the charge element to allow this to be overwritten when needed. This opened up a lot of discussions about how we could refactor the review tables to work more effectively with how the two-part tariff engine works now. When we first built the tables we did so with the knowledge we had at the time. Now that we have worked on more tickets and started building actual pages we realised we could persist the data better. TLDR: Review tables updated to add status and issues --- ...ocate-returns-to-charge-element.service.js | 25 +- .../determine-licence-issues.service.js | 190 +++++++++++ .../match-and-allocate.service.js | 6 +- .../prepare-return-logs.service.js | 25 -- ...-returns-to-charge-element.service.test.js | 7 +- .../determine-licence-issues.service.test.js | 304 ++++++++++++++++++ .../match-and-allocate.service.test.js | 6 + .../prepare-return-logs.service.test.js | 3 +- 8 files changed, 532 insertions(+), 34 deletions(-) create mode 100644 app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js create mode 100644 test/services/bill-runs/two-part-tariff/determine-licence-issues.service.test.js diff --git a/app/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.js b/app/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.js index ef7a9889d..0579b3400 100644 --- a/app/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.js +++ b/app/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.js @@ -118,10 +118,33 @@ function _matchLines (chargeElement, returnSubmissionLines) { }) } +/** + * Checks a return record for potential issues based on specific criteria and flags it accordingly + */ +function _checkReturnForIssues (matchedReturn) { + if (matchedReturn.nilReturn) { + return true + } + + if (matchedReturn.underQuery) { + return true + } + + if (matchedReturn.status !== 'completed') { + return true + } + + if (matchedReturn.returnSubmissions.length === 0 || matchedReturn.returnSubmissions[0].returnSubmissionLines.length === 0) { + return true + } + + return false +} + function _processCompletedReturns (chargeElement, matchingReturns, chargePeriod, chargeReference) { matchingReturns.forEach((matchedReturn, i) => { // We don't allocate returns with issues - if (matchedReturn.issues) { + if (_checkReturnForIssues(matchedReturn)) { return } diff --git a/app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js b/app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js new file mode 100644 index 000000000..f698792dd --- /dev/null +++ b/app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js @@ -0,0 +1,190 @@ +'use strict' + +/** + * Determines the issues on a licences for a two-part tariff bill run + * @module DetermineLicenceIssuesService + */ + +// A list of issues that would put a licence into a status of 'review' +const REVIEW_STATUSES = [ + 'Aggregate factor', 'Checking query', 'Overlap of charge dates', 'Returns received but not processed', + 'Returns split over charge references', 'Unable to match returns' +] + +/** + * Determines the issues on a licence charge elements and return logs and sets the status based on them + * + * @param {module:LicenceModel} licence - the two-part tariff licence included in the bill run + */ +async function go (licence) { + const { returnLogs: licenceReturnLogs, chargeVersions } = licence + + const allReturnIssues = _determineReturnLogsIssues(licenceReturnLogs, licence) + const allElementIssues = _determineChargeElementsIssues(chargeVersions, licenceReturnLogs) + + licence.status = _determineLicenceStatus(allElementIssues, allReturnIssues) +} + +function _determineChargeElementsIssues (chargeVersions, licenceReturnLogs) { + const allElementIssues = [] + + chargeVersions.forEach((chargeVersion) => { + const { chargeReferences } = chargeVersion + + chargeReferences.forEach((chargeReference) => { + const { chargeElements } = chargeReference + + chargeElements.forEach((chargeElement) => { + const { returnLogs } = chargeElement + + const { elementIssues, status } = _elementIssues(chargeReference, chargeElement, licenceReturnLogs, returnLogs) + + chargeElement.issues = elementIssues + chargeElement.status = status + allElementIssues.push(...elementIssues) + }) + }) + }) + + return allElementIssues +} + +function _determineLicenceStatus (allElementIssues, allReturnIssues) { + const allLicenceIssues = [...allElementIssues, ...allReturnIssues] + + // If a licence has more than one issue, or has 1 issue that is in the `REVIEW_STATUSES` array the licence status is + // set to 'Review' otherwise its 'Ready' + if (allLicenceIssues.length > 1 || REVIEW_STATUSES.includes(allLicenceIssues[0])) { + return 'review' + } else { + return 'ready' + } +} + +function _determineReturnLogsIssues (returnLogs, licence) { + const allReturnsIssues = [] + + returnLogs.forEach((returnLog) => { + const returnLogIssues = _returnLogIssues(returnLog, licence) + + returnLog.issues = returnLogIssues + allReturnsIssues.push(...returnLogIssues) + }) + + return allReturnsIssues +} + +function _determineReturnSplitOverChargeReference (licence, returnLog) { + let chargeReferenceCounter = 0 + const { chargeVersions } = licence + + chargeVersions.forEach((chargeVersion) => { + const { chargeReferences } = chargeVersion + + chargeReferences.forEach((chargeReference) => { + const { chargeElements } = chargeReference + + // We do a .some here as we only care if the returnLog is present in the chargeReference at least once. If the + // return is present we increase our chargeReference counter by 1 to tally up how many unique chargeReference have + // matched to the return + const returnLogInChargeReference = chargeElements.some((chargeElement) => { + const { returnLogs: chargeElementReturnLogs } = chargeElement + + return chargeElementReturnLogs.some((chargeElementReturnLog) => { + return chargeElementReturnLog.returnId === returnLog.id + }) + }) + + if (returnLogInChargeReference) { + chargeReferenceCounter++ + } + }) + }) + + return chargeReferenceCounter > 1 +} + +function _elementIssues (chargeReference, chargeElement, licenceReturnLogs, returnLogs) { + let status = 'ready' + const elementIssues = [] + + // Issue Aggregate factor + if (chargeReference.aggregate !== 1) { + elementIssues.push('Aggregate factor') + status = 'review' + } + + // Issue Overlap of charge dates + if (chargeElement.chargeDatesOverlap) { + elementIssues.push('Overlap of charge dates') + status = 'review' + } + + // Issue Some returns not received + if (_someReturnsNotReceived(returnLogs, licenceReturnLogs)) { + elementIssues.push('Some returns not received') + } + + // Unable to match return + if (returnLogs.length < 1) { + elementIssues.push('Unable to match return') + status = 'review' + } + + return { elementIssues, status } +} + +function _returnLogIssues (returnLog, licence) { + const returnLogIssues = [] + + // Abstraction outside period issue + if (returnLog.abstractionOutsidePeriod) { + returnLogIssues.push('Abstraction outside period') + } + + // Checking query issue + if (returnLog.underQuery) { + returnLogIssues.push('Checking query') + } + + // No returns received + if (returnLog.status === 'due') { + returnLogIssues.push('No returns received') + } + + // Over abstraction + if (returnLog.quantity > returnLog.allocatedQuantity) { + returnLogIssues.push('Over abstraction') + } + + // Returns received but not processed + if (returnLog.status === 'received') { + returnLogIssues.push('Returns received but not processed') + } + + // Returns received late + if (returnLog.receivedDate > returnLog.dueDate) { + returnLogIssues.push('Returns received late') + } + + // Returns split over charge references + if (_determineReturnSplitOverChargeReference(licence, returnLog)) { + returnLogIssues.push('Return split over charge references') + } + + return returnLogIssues +} + +function _someReturnsNotReceived (returnLogs, licenceReturnLogs) { + const returnLogIds = returnLogs.map((returnLog) => { + return returnLog.returnId + }) + + return licenceReturnLogs.some((licenceReturnLog) => { + return returnLogIds.includes(licenceReturnLog.id) && licenceReturnLog.status === 'due' + }) +} + +module.exports = { + go +} diff --git a/app/services/bill-runs/two-part-tariff/match-and-allocate.service.js b/app/services/bill-runs/two-part-tariff/match-and-allocate.service.js index 5c05e9c7e..a3ddc23ef 100644 --- a/app/services/bill-runs/two-part-tariff/match-and-allocate.service.js +++ b/app/services/bill-runs/two-part-tariff/match-and-allocate.service.js @@ -9,6 +9,7 @@ const MatchReturnsToChargeElementService = require('./match-returns-to-charge-el const PrepareChargeVersionService = require('./prepare-charge-version.service.js') const PrepareReturnLogsService = require('./prepare-return-logs.service.js') const PersistAllocatedLicenceToResultsService = require('./persist-allocated-licence-to-results.service.js') +const DetermineLicenceIssuesService = require('./determine-licence-issues.service.js') /** * Performs the two-part tariff matching and allocating @@ -36,7 +37,7 @@ async function go (billRun, billingPeriod) { } async function _process (licences, billingPeriod, billRun) { - for (const licence of licences) { + licences.forEach(async (licence) => { await PrepareReturnLogsService.go(licence, billingPeriod) const { chargeVersions, returnLogs } = licence @@ -64,8 +65,9 @@ async function _process (licences, billingPeriod, billRun) { }) }) + await DetermineLicenceIssuesService.go(licence) await PersistAllocatedLicenceToResultsService.go(billRun.id, licence) - } + }) } module.exports = { diff --git a/app/services/bill-runs/two-part-tariff/prepare-return-logs.service.js b/app/services/bill-runs/two-part-tariff/prepare-return-logs.service.js index d2973f5d8..902a276ae 100644 --- a/app/services/bill-runs/two-part-tariff/prepare-return-logs.service.js +++ b/app/services/bill-runs/two-part-tariff/prepare-return-logs.service.js @@ -29,29 +29,6 @@ function _abstractionOutsidePeriod (returnAbstractionPeriods, returnLine) { return !periodsOverlap(returnAbstractionPeriods, [{ startDate, endDate }]) } -/** - * Checks a return record for potential issues based on specific criteria and flags it accordingly - */ -function _checkReturnForIssues (returnLog) { - if (returnLog.nilReturn) { - return true - } - - if (returnLog.underQuery) { - return true - } - - if (returnLog.status !== 'completed') { - return true - } - - if (returnLog.returnSubmissions.length === 0 || returnLog.returnSubmissions[0].returnSubmissionLines.length === 0) { - return true - } - - return false -} - async function _prepareReturnLogs (licence, billingPeriod) { licence.returnLogs = await FetchReturnLogsForLicenceService.go(licence.licenceRef, billingPeriod) @@ -86,8 +63,6 @@ function _prepReturnsForMatching (returnLogs, billingPeriod) { returnLog.abstractionPeriods = abstractionPeriods returnLog.abstractionOutsidePeriod = abstractionOutsidePeriod returnLog.matched = false - - returnLog.issues = (_checkReturnForIssues(returnLog)) }) } diff --git a/test/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.test.js b/test/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.test.js index b2013b675..37e5ddd3c 100644 --- a/test/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.test.js +++ b/test/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.test.js @@ -224,7 +224,7 @@ describe('Allocate Returns to Charge Element Service', () => { describe('with a return that has issues', () => { beforeEach(() => { testData = _generateTestData() - testData.matchingReturns[0].issues = true + testData.matchingReturns[0].underQuery = true }) it('does not allocate anything from the return log', () => { @@ -428,7 +428,7 @@ describe('Allocate Returns to Charge Element Service', () => { }) }) -function _generateTestData (returnStatus = 'complete') { +function _generateTestData (returnStatus = 'completed') { // Data not required for the tests has been excluded from the generated data const chargeElement = { authorisedAnnualQuantity: 32, @@ -505,9 +505,8 @@ function _generateTestData (returnStatus = 'complete') { // If a returns status isn't `complete` it won't have any return submission lines and the `issues` will be `true` const matchingReturns = [ { - returnSubmissions: returnStatus === 'complete' ? returnSubmissions : [], + returnSubmissions: returnStatus === 'completed' ? returnSubmissions : [], allocatedQuantity: 0, - issues: returnStatus !== 'complete', status: returnStatus } ] diff --git a/test/services/bill-runs/two-part-tariff/determine-licence-issues.service.test.js b/test/services/bill-runs/two-part-tariff/determine-licence-issues.service.test.js new file mode 100644 index 000000000..1fd296968 --- /dev/null +++ b/test/services/bill-runs/two-part-tariff/determine-licence-issues.service.test.js @@ -0,0 +1,304 @@ +'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 + +// Thing under test +const DetermineLicenceIssuesService = require('../../../../app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js') + +describe('Determine Licence Issues Service', () => { + describe('when given a licence', () => { + let licence + + describe('that has multiple issues', () => { + beforeEach(() => { + licence = _generateMultipleIssuesLicenceData() + }) + + describe('on the returns', () => { + it('sets all the issues on the returns object', () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.returnLogs[0].issues).to.equal(['Abstraction outside period', 'Checking query', 'Over abstraction', 'Returns received late', 'Return split over charge references']) + expect(licence.returnLogs[1].issues).to.equal(['No returns received']) + expect(licence.returnLogs[2].issues).to.equal(['Returns received but not processed']) + }) + + it("sets the status of the licence to 'review'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.status).to.equal('review') + }) + }) + + describe('on the charge elements', () => { + it('sets all the issues on the element object', () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].issues).to.equal(['Aggregate factor', 'Overlap of charge dates', 'Some returns not received']) + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[1].issues).to.equal(['Aggregate factor', 'Unable to match return']) + }) + + it("sets the status of the licence to 'review'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.status).to.equal('review') + }) + + it("sets the status of the charge element to 'review'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].status).to.equal('review') + }) + }) + }) + + describe('that has 1 issue on the returns', () => { + describe("and the issues is a 'review' status", () => { + // Note: a licence can't have 1 issue on a charge element as the only issue on the element that is not a + // 'Review' status is `Some returns not received`. This is when a return has a status of `due`, and if this + // issue is present then the return will have an issue of `No returns received`, making the total issues on the + // licence 2 and not 1 + beforeEach(() => { + licence = _generateOneIssueLicenceData('review') + }) + + it("sets the status on the licence to 'review'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.status).to.equal('review') + }) + + it("sets the status on the element to 'ready'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].status).to.equal('ready') + }) + }) + + describe("and the issues is a 'ready' status", () => { + beforeEach(() => { + licence = _generateOneIssueLicenceData('ready') + }) + + it("sets the status on the licence to 'ready'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.status).to.equal('ready') + }) + + it("sets the status on the element to 'ready'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].status).to.equal('ready') + }) + }) + }) + + describe('that has no issues', () => { + beforeEach(() => { + licence = _generateNoIssuesLicenceData() + }) + + it("sets the licence status as 'ready'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.status).to.equal('ready') + }) + + it("sets the element status as 'ready'", () => { + DetermineLicenceIssuesService.go(licence) + + expect(licence.chargeVersions[0].chargeReferences[0].chargeElements[0].status).to.equal('ready') + }) + }) + }) +}) + +function _generateNoIssuesLicenceData () { + return { + chargeVersions: [ + { + chargeReferences: [ + { + aggregate: 1, + chargeElements: [ + { + chargeDatesOverlap: false, + returnLogs: [ + { + returnId: '1234' + } + ] + } + ] + } + ] + } + ], + returnLogs: [ + { + id: '1234', + abstractionOutsidePeriod: false, + underQuery: false, + status: 'completed', + quantity: 1, + allocatedQuantity: 1, + receivedDate: new Date('2024 01 01'), + dueDate: new Date('2024 01 01') + } + ] + } +} + +function _generateOneIssueLicenceData (status) { + if (status === 'review') { + return { + chargeVersions: [ + { + chargeReferences: [ + { + aggregate: 1, + chargeElements: [ + { + chargeDatesOverlap: false, + returnLogs: [ + { + returnId: '1234' + } + ] + } + ] + } + ] + } + ], + returnLogs: [ + { + id: '1234', + abstractionOutsidePeriod: false, + underQuery: true, + status: 'completed', + quantity: 1, + allocatedQuantity: 1, + receivedDate: new Date('2024 01 01'), + dueDate: new Date('2024 01 01') + } + ] + } + } else { + return { + chargeVersions: [ + { + chargeReferences: [ + { + aggregate: 1, + chargeElements: [ + { + chargeDatesOverlap: false, + returnLogs: [ + { + returnId: '1234' + } + ] + } + ] + } + ] + } + ], + returnLogs: [ + { + id: '1234', + abstractionOutsidePeriod: false, + underQuery: false, + status: 'completed', + quantity: 1, + allocatedQuantity: 0, + receivedDate: new Date('2024 01 01'), + dueDate: new Date('2024 01 01') + } + ] + } + } +} + +function _generateMultipleIssuesLicenceData () { + return { + chargeVersions: [ + { + chargeReferences: [ + { + aggregate: 1.25, + chargeElements: [ + { + chargeDatesOverlap: true, + returnLogs: [ + { + returnId: '1234' + }, + { + returnId: '5678' + } + ] + }, + { + chargeDatesOverlap: false, + returnLogs: [] + } + ] + }, + { + aggregate: 1.25, + chargeElements: [ + { + chargeDatesOverlap: false, + returnLogs: [ + { + returnId: '1234' + } + ] + } + ] + } + ] + } + ], + returnLogs: [ + { + id: '1234', + abstractionOutsidePeriod: true, + underQuery: true, + status: 'completed', + quantity: 1, + allocatedQuantity: 0, + receivedDate: new Date('2024 02 01'), + dueDate: new Date('2024 01 01') + }, + { + id: '5678', + abstractionOutsidePeriod: false, + underQuery: false, + status: 'due', + quantity: 0, + allocatedQuantity: 0, + receivedDate: null, + dueDate: new Date('2024 01 01') + }, + { + id: '91011', + abstractionOutsidePeriod: false, + underQuery: false, + status: 'received', + quantity: 0, + allocatedQuantity: 0, + receivedDate: new Date('2024 01 01'), + dueDate: new Date('2024 01 01') + } + ] + } +} diff --git a/test/services/bill-runs/two-part-tariff/match-and-allocate.service.test.js b/test/services/bill-runs/two-part-tariff/match-and-allocate.service.test.js index cb86e764a..451ae5749 100644 --- a/test/services/bill-runs/two-part-tariff/match-and-allocate.service.test.js +++ b/test/services/bill-runs/two-part-tariff/match-and-allocate.service.test.js @@ -10,6 +10,7 @@ const { expect } = Code // Things we need to stub const AllocateReturnsToChargeElementService = require('../../../../app/services/bill-runs/two-part-tariff/allocate-returns-to-charge-element.service.js') +const DetermineLicenceIssuesService = require('../../../../app/services/bill-runs/two-part-tariff/determine-licence-issues.service.js') const FetchLicencesService = require('../../../../app/services/bill-runs/two-part-tariff/fetch-licences.service.js') const MatchReturnsToChargeElementService = require('../../../../app/services/bill-runs/two-part-tariff/match-returns-to-charge-element.service.js') const PrepareChargeVersionService = require('../../../../app/services/bill-runs/two-part-tariff/prepare-charge-version.service.js') @@ -52,6 +53,7 @@ describe('Match And Allocate Service', () => { Sinon.stub(PrepareReturnLogsService, 'go') Sinon.stub(PrepareChargeVersionService, 'go') Sinon.stub(AllocateReturnsToChargeElementService, 'go') + Sinon.stub(DetermineLicenceIssuesService, 'go') Sinon.stub(PersistAllocatedLicenceToResultsService, 'go') }) @@ -72,6 +74,7 @@ describe('Match And Allocate Service', () => { expect(MatchReturnsToChargeElementService.go.called).to.be.true() expect(AllocateReturnsToChargeElementService.go.called).to.be.true() + expect(DetermineLicenceIssuesService.go.called).to.be.true() expect(PersistAllocatedLicenceToResultsService.go.called).to.be.true() }) @@ -97,6 +100,7 @@ describe('Match And Allocate Service', () => { expect(MatchReturnsToChargeElementService.go.called).to.be.true() expect(AllocateReturnsToChargeElementService.go.called).to.be.false() + expect(DetermineLicenceIssuesService.go.called).to.be.true() expect(PersistAllocatedLicenceToResultsService.go.called).to.be.true() }) @@ -115,6 +119,7 @@ describe('Match And Allocate Service', () => { Sinon.stub(PrepareChargeVersionService, 'go') Sinon.stub(MatchReturnsToChargeElementService, 'go') Sinon.stub(AllocateReturnsToChargeElementService, 'go') + Sinon.stub(DetermineLicenceIssuesService, 'go') Sinon.stub(PersistAllocatedLicenceToResultsService, 'go') }) @@ -131,6 +136,7 @@ describe('Match And Allocate Service', () => { expect(PrepareChargeVersionService.go.called).to.be.false() expect(MatchReturnsToChargeElementService.go.called).to.be.false() expect(AllocateReturnsToChargeElementService.go.called).to.be.false() + expect(DetermineLicenceIssuesService.go.called).to.be.false() expect(PersistAllocatedLicenceToResultsService.go.called).to.be.false() }) diff --git a/test/services/bill-runs/two-part-tariff/prepare-return-logs.service.test.js b/test/services/bill-runs/two-part-tariff/prepare-return-logs.service.test.js index 4b0f53486..e0bcc6a1f 100644 --- a/test/services/bill-runs/two-part-tariff/prepare-return-logs.service.test.js +++ b/test/services/bill-runs/two-part-tariff/prepare-return-logs.service.test.js @@ -88,8 +88,7 @@ describe('Prepare Return Logs Service', () => { } ], abstractionOutsidePeriod: false, - matched: false, - issues: false + matched: false }) }) })