Skip to content

Commit

Permalink
Fix annual billing not excluding workflow licences (#763)
Browse files Browse the repository at this point in the history 
https://eaflood.atlassian.net/browse/WATER-4379

We are working to replace the legacy SROC annual billing engine with one in this project that exploits what we did with SROC supplementary billing.

The first pass of testing has highlighted a discrepancy with the bill amounts. When we looked into it we found the new engine was still including licences that have records in workflow as part of the calculation.

Our new engine is only returning billing accounts that have applicable charge versions. What we failed to do was then filter again the charge versions returned for each billing account.

The scenario we found was a billing account linked to 2 licences, one of which was in workflow. The billing account was included because it has a charge version linked to a licence that isn't in workflow.

But then we fetched for the billing account _all_ the charge versions including the one linked to a licence in workflow.

We should be doing the same filtering in both places. This fix ensures that.
  • Loading branch information
@Cruikshanks
Cruikshanks authored Feb 26, 2024
1 parent e8a5bba commit 7686f0f
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 95 deletions.
70 changes: 43 additions & 27 deletions app/services/bill-runs/annual/fetch-billing-accounts.service.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,6 @@ async function _fetchNew (regionId, billingPeriod) {
'billingAccounts.id',
'billingAccounts.accountNumber'
])
// NOTE: The WHERE EXISTS clause is a beast of a query in its own right so moved it to a function and have it return
// the QueryBuilder instance `whereExists()` can use.
// Bill runs are formed of 'bills' which are a 1-to-1 with billing accounts. But whether a billing account should
// be included is _all_ based on the charge versions they are linked to. So, all the work of filtering what will be
// considered is done there.
.whereExists(_whereExistsClause(regionId, billingPeriod))
.orderBy([
{ column: 'billingAccounts.accountNumber' }
Expand All @@ -55,25 +50,20 @@ async function _fetchNew (regionId, billingPeriod) {
.modifyGraph('chargeVersions', (builder) => {
builder
.select([
'id',
'scheme',
'startDate',
'endDate',
'billingAccountId',
'status'
])
.where('scheme', 'sroc')
.where('startDate', '<=', billingPeriod.endDate)
.where('status', 'current')
.where(() => {
builder
.whereNull('endDate')
.orWhere('endDate', '>=', billingPeriod.startDate)
})
.orderBy([
{ column: 'licenceId', order: 'ASC' },
{ column: 'startDate', order: 'ASC' }
'chargeVersions.id',
'chargeVersions.scheme',
'chargeVersions.startDate',
'chargeVersions.endDate',
'chargeVersions.billingAccountId',
'chargeVersions.status'
])

_whereClauseForChargeVersions(builder, regionId, billingPeriod)

builder.orderBy([
{ column: 'licenceId', order: 'ASC' },
{ column: 'startDate', order: 'ASC' }
])
})
.withGraphFetched('chargeVersions.licence')
.modifyGraph('chargeVersions.licence', (builder) => {
Expand Down Expand Up @@ -135,12 +125,24 @@ async function _fetchNew (regionId, billingPeriod) {
})
}

function _whereExistsClause (regionId, billingPeriod) {
return ChargeVersionModel.query()
.select(1)
/**
* Where clause to use when fetching charge versions within the main query
*
* We have to filter the applicable charge versions for a number of reasons and need to do it twice; once when
* determining which billing accounts to fetch and again, when grabbing their related charge versions.
*
* So, we have moved the 'WHERE' clause to its own function that we can then reuse.
*
* @param {module:QueryBuilder} query - an instance of the Objection QueryBuilder being generated
* @param {string} regionId - UUID of the region being billed that the licences must be linked to
* @param {Object} billingPeriod - Object with a `startDate` and `endDate` property representing the period being billed
*
* @returns {module:QueryBuilder} the builder instance passed in with the additional `where` clauses added
*/
function _whereClauseForChargeVersions (query, regionId, billingPeriod) {
return query
.innerJoinRelated('licence')
.where('licence.regionId', regionId)
.whereColumn('chargeVersions.billingAccountId', 'billingAccounts.id')
.where('chargeVersions.scheme', 'sroc')
.where('chargeVersions.startDate', '<=', billingPeriod.endDate)
.where('chargeVersions.status', 'current')
Expand Down Expand Up @@ -172,6 +174,20 @@ function _whereExistsClause (regionId, billingPeriod) {
)
}

/**
* Bill runs are formed of 'bills' which are a 1-to-1 with billing accounts. But whether a billing account should
* be included is _all_ based on the charge versions they are linked to. So, all the work of filtering what will be
* considered is done here by combining a `select(1)` with our `_whereClauseForChargeVersions()` function.
*/
function _whereExistsClause (regionId, billingPeriod) {
let query = ChargeVersionModel.query().select(1)

query = _whereClauseForChargeVersions(query, regionId, billingPeriod)
query.whereColumn('chargeVersions.billingAccountId', 'billingAccounts.id')

return query
}

module.exports = {
go
}
158 changes: 90 additions & 68 deletions test/services/bill-runs/annual/fetch-billing-accounts.service.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const FetchBillingAccountsService = require('../../../../app/services/bill-runs/
describe('Fetch Billing Accounts service', () => {
const billingPeriod = currentFinancialYear()

let billingAccount
let billingAccountId
let licence
let licenceId
let region
Expand All @@ -41,20 +43,19 @@ describe('Fetch Billing Accounts service', () => {

licence = await LicenceHelper.add({ regionId })
licenceId = licence.id

billingAccount = await BillingAccountHelper.add()
billingAccountId = billingAccount.id
})

describe('when there are billing accounts that should be considered for annual billing', () => {
let billingAccount
let changeReason
let chargeCategory
let chargeElement
let chargeReference
let chargeVersion

beforeEach(async () => {
billingAccount = await BillingAccountHelper.add()
const { id: billingAccountId } = billingAccount

changeReason = await ChangeReasonHelper.add({ triggersMinimumCharge: true })

const { id: licenceId, licenceRef } = licence
Expand All @@ -80,83 +81,104 @@ describe('Fetch Billing Accounts service', () => {
expect(results).to.have.length(1)

expect(results[0]).to.be.instanceOf(BillingAccountModel)
expect(results[0].id).to.equal(billingAccount.id)
expect(results[0].id).to.equal(billingAccountId)
expect(results[0].accountNumber).to.equal(billingAccount.accountNumber)
})

it('includes the related charge versions in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)
describe('that have applicable related charge versions', () => {
it('includes the charge versions in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)

const { chargeVersions } = results[0]
const { chargeVersions } = results[0]

expect(chargeVersions[0].id).to.equal(chargeVersion.id)
expect(chargeVersions[0].scheme).to.equal('sroc')
expect(chargeVersions[0].startDate).to.equal(new Date('2023-11-01'))
expect(chargeVersions[0].endDate).to.be.null()
expect(chargeVersions[0].billingAccountId).to.equal(billingAccount.id)
expect(chargeVersions[0].status).to.equal('current')
})
expect(chargeVersions[0].id).to.equal(chargeVersion.id)
expect(chargeVersions[0].scheme).to.equal('sroc')
expect(chargeVersions[0].startDate).to.equal(new Date('2023-11-01'))
expect(chargeVersions[0].endDate).to.be.null()
expect(chargeVersions[0].billingAccountId).to.equal(billingAccountId)
expect(chargeVersions[0].status).to.equal('current')
})

it('includes the related licence and region in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)
it('includes the licence and region in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)

const { licence } = results[0].chargeVersions[0]
const { licence } = results[0].chargeVersions[0]

expect(licence.id).to.equal(licence.id)
expect(licence.licenceRef).to.equal(licence.licenceRef)
expect(licence.waterUndertaker).to.equal(false)
expect(licence.historicalAreaCode).to.equal('SAAR')
expect(licence.regionalChargeArea).to.equal('Southern')
expect(licence.region.id).to.equal(regionId)
expect(licence.region.chargeRegionId).to.equal('W')
})
expect(licence.id).to.equal(licence.id)
expect(licence.licenceRef).to.equal(licence.licenceRef)
expect(licence.waterUndertaker).to.equal(false)
expect(licence.historicalAreaCode).to.equal('SAAR')
expect(licence.regionalChargeArea).to.equal('Southern')
expect(licence.region.id).to.equal(regionId)
expect(licence.region.chargeRegionId).to.equal('W')
})

it('includes the related change reason in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)
it('includes the change reason in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)

const { changeReason } = results[0].chargeVersions[0]

const { changeReason } = results[0].chargeVersions[0]
expect(changeReason.id).to.equal(changeReason.id)
expect(changeReason.triggersMinimumCharge).to.equal(changeReason.triggersMinimumCharge)
})

it('includes the charge references, charge category and charge elements in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)

expect(changeReason.id).to.equal(changeReason.id)
expect(changeReason.triggersMinimumCharge).to.equal(changeReason.triggersMinimumCharge)
const { chargeReferences } = results[0].chargeVersions[0]

expect(chargeReferences).to.have.length(1)

expect(chargeReferences[0]).to.equal({
id: chargeReference.id,
source: 'non-tidal',
loss: 'low',
volume: 6.819,
adjustments: {
s126: null, s127: false, s130: false, charge: null, winter: false, aggregate: '0.562114443'
},
additionalCharges: { isSupplyPublicWater: true },
description: 'Mineral washing',
chargeCategory: {
id: chargeCategory.id,
reference: chargeCategory.reference,
shortDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model'
},
chargeElements: [{
id: chargeElement.id,
abstractionPeriodStartDay: 1,
abstractionPeriodStartMonth: 4,
abstractionPeriodEndDay: 31,
abstractionPeriodEndMonth: 3
}]
})
})
})

it('includes the related charge references, charge category and charge elements in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)
describe('that have inapplicable related charge versions', () => {
beforeEach(async () => {
const licenceInWorkflow = await LicenceHelper.add({ regionId })
const { id: licenceInWorkflowId, licenceRef } = licenceInWorkflow

const { chargeReferences } = results[0].chargeVersions[0]

expect(chargeReferences).to.have.length(1)

expect(chargeReferences[0]).to.equal({
id: chargeReference.id,
source: 'non-tidal',
loss: 'low',
volume: 6.819,
adjustments: {
s126: null, s127: false, s130: false, charge: null, winter: false, aggregate: '0.562114443'
},
additionalCharges: { isSupplyPublicWater: true },
description: 'Mineral washing',
chargeCategory: {
id: chargeCategory.id,
reference: chargeCategory.reference,
shortDescription: 'Low loss, non-tidal, restricted water, up to and including 5,000 ML/yr, Tier 1 model'
},
chargeElements: [{
id: chargeElement.id,
abstractionPeriodStartDay: 1,
abstractionPeriodStartMonth: 4,
abstractionPeriodEndDay: 31,
abstractionPeriodEndMonth: 3
}]
await ChargeVersionHelper.add({ billingAccountId, licenceId: licenceInWorkflowId, licenceRef })
await WorkflowHelper.add({ licenceId: licenceInWorkflowId })
})

it('excludes the charge versions in each result', async () => {
const results = await FetchBillingAccountsService.go(regionId, billingPeriod)

const { chargeVersions } = results[0]

expect(chargeVersions.length).to.equal(1)
expect(chargeVersions[0].id).to.equal(chargeVersion.id)
})
})
})

describe('when there are no billing accounts that should be considered for the annual bill run', () => {
describe("because all their charge versions do not have a status of 'current", () => {
beforeEach(async () => {
await ChargeVersionHelper.add({ status: 'draft', licenceId })
await ChargeVersionHelper.add({ status: 'draft', billingAccountId, licenceId })
})

it('returns empty results', async () => {
Expand All @@ -168,7 +190,7 @@ describe('Fetch Billing Accounts service', () => {

describe("because all their charge versions are for the 'alcs' (presroc) scheme", () => {
beforeEach(async () => {
await ChargeVersionHelper.add({ scheme: 'alcs', licenceId })
await ChargeVersionHelper.add({ scheme: 'alcs', billingAccountId, licenceId })
})

it('returns empty results', async () => {
Expand All @@ -180,11 +202,11 @@ describe('Fetch Billing Accounts service', () => {

describe('because all their charge versions have start dates after the billing period', () => {
beforeEach(async () => {
const financialEndYear = billingPeriod.endDate.getFullYear()
const financialStartYear = billingPeriod.endDate.getFullYear()

// This creates an charge version with a start date after the billing period
await ChargeVersionHelper.add(
{ startDate: new Date(financialEndYear, 8, 15), licenceId }
{ startDate: new Date(financialStartYear, 8, 15), billingAccountId, licenceId }
)
})

Expand All @@ -197,11 +219,11 @@ describe('Fetch Billing Accounts service', () => {

describe('because all their charge versions have end dates before the billing period', () => {
beforeEach(async () => {
const financialStartYear = billingPeriod.endDate.getFullYear()
const financialEndYear = billingPeriod.startDate.getFullYear()

// This creates an charge version with a end date before the billing period starts
await ChargeVersionHelper.add(
{ endDate: new Date(financialStartYear, 2, 31), licenceId }
{ endDate: new Date(financialEndYear, 2, 31), billingAccountId, licenceId }
)
})

Expand All @@ -217,7 +239,7 @@ describe('Fetch Billing Accounts service', () => {
const { id: licenceId } = await LicenceHelper.add({ regionId: 'e117b501-e3c1-4337-ad35-21c60ed9ad73' })

// This creates an charge version linked to a licence with an different region than selected
await ChargeVersionHelper.add({ licenceId })
await ChargeVersionHelper.add({ billingAccountId, licenceId })
})

it('returns empty results', async () => {
Expand All @@ -232,7 +254,7 @@ describe('Fetch Billing Accounts service', () => {
const { id: licenceId } = await LicenceHelper.add({ revokedDate: new Date('2023-02-01') })

// This creates a charge version linked to a licence that ends before the billing period
await ChargeVersionHelper.add({ licenceId })
await ChargeVersionHelper.add({ billingAccountId, licenceId })
})

it('returns empty results', async () => {
Expand All @@ -244,7 +266,7 @@ describe('Fetch Billing Accounts service', () => {

describe('because they are all linked to licences in workflow', () => {
beforeEach(async () => {
await ChargeVersionHelper.add({ licenceId })
await ChargeVersionHelper.add({ billingAccountId, licenceId })
await WorkflowHelper.add({ licenceId })
})

Expand Down

0 comments on commit 7686f0f

Please sign in to comment.