Skip to content

Commit

Permalink
Migrate legacy send bill run functionality (#771)
Browse files Browse the repository at this point in the history
https://eaflood.atlassian.net/browse/WATER-4390

We have been working on replacing the legacy SROC annual billing engine. It is slow and prone to error leading to the team having to devote resources every year to helping the billing & data team get the annual bill runs through.

Like with our supplementary billing engine, we focused on the first part of the process; creating the bill run record and then generating the transactions to send to the [charging-module-api (CHA)](https://github.com/DEFRA/sroc-charging-module-api). With that done we then pass control back to the legacy code to refresh the data and mark the bill run as **READY**.

But our testing demonstrated that even with this part of the process replaced annual bill runs can still prove to be problematic due to the shoddy way the previous team implemented them. For example, when we send the POST request to the legacy `/refresh` endpoint it kicks off a process to request the 'generated' billing information from the CHA. This entails sending a request for every bill in the bill run and then updating our local data with the result.

You can't **SEND** a bill run until this step is complete. When you view the bill run all the values will be £0.00 and the download CSV will be working. Yet the legacy code still marks the bill run as **READY**. Users familiar with the service may know they need to leave it a while for the numbers to refresh. What they probably don't know is the CHA is getting hammered and the service is busy with this process.

Ideally, they should wait for this process to complete before starting a new bill run. But there is no way to know that. So, the next bill run will take longer to complete and you increase the risk of an error.

We felt this situation was ridiculous so opted to amend the [water-abstraction-service](https://github.com/DEFRA/water-abstraction-service) to solve this. (See [Stop marking bill runs as ready when they are not](DEFRA/water-abstraction-service#2442)).

It worked but inadvertently broke the legacy 'send a bill run' process (the final step in the bill run journey). When you confirm and send a bill run it tells the CHA to send the bill run to SOP. The CHA does a range of things behind the scenes one of which is to generate a transaction reference for each invoice. Again we need that information on our side but only that.

This is where things go 🦇💩! The legacy service does this by calling the refresh invoices job again!! So, we're going through the whole process of sending a request for every invoice, updating values we already have including dealing with updating the transactions all over again. _All of this_ for something we can get making ***one*** request to the CHA (a request the legacy process already does before going through all the invoices again).

We didn't know the legacy code reused this job. So, when we amended it to set the bill run status to **READY** at the end of the process we broke sending because in that scenario we are setting the status back from **SENT** to **READY**.

We've implemented a 'hack' to get around this. But enough is enough! With this change, we take over the 'send bill run process' and resolve this properly.
  • Loading branch information
Cruikshanks authored Mar 17, 2024
1 parent 1d457d9 commit 9d2c828
Show file tree
Hide file tree
Showing 11 changed files with 839 additions and 224 deletions.
39 changes: 32 additions & 7 deletions app/controllers/bill-runs.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ const CancelBillRunService = require('../services/bill-runs/cancel-bill-run.serv
const CreateBillRunValidator = require('../validators/create-bill-run.validator.js')
const ReviewBillRunService = require('../services/bill-runs/two-part-tariff/review-bill-run.service.js')
const ReviewLicenceService = require('../services/bill-runs/two-part-tariff/review-licence.service.js')
const SendBillRunService = require('../services/bill-runs/send-bill-run.service.js')
const StartBillRunProcessService = require('../services/bill-runs/start-bill-run-process.service.js')
const SubmitCancelBillRunService = require('../services/bill-runs/submit-cancel-bill-run.service.js')
const SubmitSendBillRunService = require('../services/bill-runs/submit-send-bill-run.service.js')
const ViewBillRunService = require('../services/bill-runs/view-bill-run.service.js')

async function cancel (request, h) {
Expand Down Expand Up @@ -55,6 +57,18 @@ async function review (request, h) {
})
}

async function send (request, h) {
const { id } = request.params

const pageData = await SendBillRunService.go(id)

return h.view('bill-runs/send.njk', {
pageTitle: "You're about to send this bill run",
activeNavBar: 'bill-runs',
...pageData
})
}

async function reviewLicence (request, h) {
const { id: billRunId, licenceId } = request.params

Expand All @@ -72,13 +86,7 @@ async function submitCancel (request, h) {

try {
// NOTE: What we are awaiting here is for the SubmitCancelBillRunService to update the status of the bill run to
// `cancel'. If the bill run run is deemed small enough, we'll also wait for the cancelling to complete before
// we redirect. This avoids users always having to refresh the bill runs page to get rid of bill runs that have
// finished cancelling 1 second after the request is submitted.
//
// But for larger bill runs, especially annual, after the bill run status has been updated control will be returned
// to here and the cancel process will then happen in the background. Because of this if we didn't wrap the call
// in a try/catch and the process errored, we'd get an unhandled exception which will bring the service down!
// `cancel'.
await SubmitCancelBillRunService.go(id)

return h.redirect('/billing/batch/list')
Expand All @@ -87,6 +95,21 @@ async function submitCancel (request, h) {
}
}

async function submitSend (request, h) {
const { id } = request.params

try {
// NOTE: What we are awaiting here is for the SubmitSendBillRunService to update the status of the bill run to
// `sending'.
await SubmitSendBillRunService.go(id)

// Redirect to the legacy processing page
return h.redirect(`/billing/batch/${id}/processing`)
} catch (error) {
return Boom.badImplementation(error.message)
}
}

async function view (request, h) {
const { id } = request.params

Expand All @@ -103,6 +126,8 @@ module.exports = {
create,
review,
reviewLicence,
send,
submitCancel,
submitSend,
view
}
50 changes: 50 additions & 0 deletions app/presenters/bill-runs/send-bill-run.presenter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
'use strict'

/**
* Formats the bill run data ready for presenting in the send bill run confirmation page
* @module SendBillRunPresenter
*/

const {
capitalize,
formatBillRunType,
formatChargeScheme,
formatFinancialYear,
formatLongDate
} = require('../base.presenter.js')

/**
* Prepares and processes bill run data for presentation
*
* @param {module:BillRunModel} billRun - an instance of `BillRunModel`
*
* @returns {Object} - the prepared bill run data to be passed to the send bill run confirmation page
*/
function go (billRun) {
const {
batchType,
billRunNumber,
createdAt,
id,
region,
scheme,
status,
summer,
toFinancialYearEnding
} = billRun

return {
billRunId: id,
billRunNumber,
billRunStatus: status,
billRunType: formatBillRunType(batchType, scheme, summer),
chargeScheme: formatChargeScheme(scheme),
dateCreated: formatLongDate(createdAt),
financialYear: formatFinancialYear(toFinancialYearEnding),
region: capitalize(region.displayName)
}
}

module.exports = {
go
}
26 changes: 26 additions & 0 deletions app/routes/bill-runs.routes.js
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,32 @@ const routes = [
},
description: 'Review a two-part tariff licence'
}
},
{
method: 'GET',
path: '/bill-runs/{id}/send',
handler: BillRunsController.send,
options: {
auth: {
access: {
scope: ['billing']
}
},
description: 'Confirm (send) a bill run'
}
},
{
method: 'POST',
path: '/bill-runs/{id}/send',
handler: BillRunsController.submitSend,
options: {
auth: {
access: {
scope: ['billing']
}
},
description: 'Submit bill run (send) confirmation'
}
}
]

Expand Down
51 changes: 51 additions & 0 deletions app/services/bill-runs/send-bill-run.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict'

/**
* Orchestrates fetching and presenting the data needed for the send bill run confirmation page
* @module SendBillRunService
*/

const BillRunModel = require('../../models/bill-run.model.js')
const SendBillRunPresenter = require('../../presenters/bill-runs/send-bill-run.presenter.js')

/**
* Orchestrates fetching and presenting the data needed for the send bill run confirmation page
*
* @param {string} id - The UUID of the bill run to send
*
* @returns {Promise<Object}> an object representing the `pageData` needed by the send bill run template. It contains
* details of the bill run.
*/
async function go (id) {
const billRun = await _fetchBillRun(id)

const pageData = SendBillRunPresenter.go(billRun)

return pageData
}

async function _fetchBillRun (id) {
return BillRunModel.query()
.findById(id)
.select([
'id',
'batchType',
'billRunNumber',
'createdAt',
'scheme',
'status',
'summer',
'toFinancialYearEnding'
])
.withGraphFetched('region')
.modifyGraph('region', (builder) => {
builder.select([
'id',
'displayName'
])
})
}

module.exports = {
go
}
118 changes: 118 additions & 0 deletions app/services/bill-runs/submit-send-bill-run.service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
'use strict'

/**
* Orchestrates the sending of a bill run
* @module SubmitCancelBillRunService
*/

const BillModel = require('../../models/bill.model.js')
const BillRunModel = require('../../models/bill-run.model.js')
const ExpandedError = require('../../errors/expanded.error.js')
const { calculateAndLogTimeTaken, timestampForPostgres } = require('../../lib/general.lib.js')
const ChargingModuleSendBillRunRequest = require('../../requests/charging-module/send-bill-run.request.js')
const ChargingModuleViewBillRunRequest = require('../../requests/charging-module/view-bill-run.request.js')

/**
* Orchestrates the sending of a bill run
*
* After checking that the bill run has a status that can be sent (only ready bill runs can be sent)
* we set the status of the bill run to `sending`. At this point we return control to the controller so that it can
* redirect the user to the bill runs page.
*
* Meantime now in the background, we send a request to the Charging Module API to send the bill run there. Once that
* process is complete we get the updated bill run summary and extract the generated bill numbers and transaction file
* reference from it.
*
* It can take the CHA more than a second to complete the send process for larger bill runs. This is why we hand back
* control to the UI early so we don't block the user or cause a timeout.
*
* @param {string} billRunId - UUID of the bill run to be sent
*
* @returns {Promise} the promise returned is not intended to resolve to any particular value
*/
async function go (billRunId) {
const billRun = await _fetchBillRun(billRunId)

if (billRun.status !== 'ready') {
return
}

await _updateStatus(billRunId, 'sending')

_sendBillRun(billRun)
}

/**
* This service handles the POST request from the confirm sent bill run page. We _could_ have included the
* externalId as a hidden field in the form and included it in the request. This would give the impression we could
* avoid a query to the DB.
*
* But we need to ensure no one exploits the `POST /bill-runs/{id}/send` endpoint. So, we always have to fetch the bill
* run to check its status is not one that prevents us deleting it.
*/
async function _fetchBillRun (id) {
return BillRunModel.query()
.findById(id)
.select([
'id',
'externalId',
'status'
])
}

async function _fetchChargingModuleBillRun (externalId) {
const result = await ChargingModuleViewBillRunRequest.send(externalId)

if (!result.succeeded) {
const error = new ExpandedError(
'Charging Module view bill run request failed',
{ billRunExternalId: externalId, responseBody: result.response.body }
)

throw error
}

return result.response.body.billRun
}

async function _sendBillRun (billRun) {
const startTime = process.hrtime.bigint()

const { id: billRunId, externalId } = billRun

await ChargingModuleSendBillRunRequest.send(externalId)

const externalBillRun = await _fetchChargingModuleBillRun(externalId)

await _updateInvoiceData(externalBillRun)

await _updateBillRunData(billRun, externalBillRun)

calculateAndLogTimeTaken(startTime, 'Send bill run complete', { billRunId })
}

async function _updateBillRunData (billRun, externalBillRun) {
const { transactionFileReference } = externalBillRun

return billRun.$query().patch({ status: 'sent', transactionFileReference })
}

async function _updateInvoiceData (externalBillRun) {
const { invoices } = externalBillRun

for (const invoice of invoices) {
const { id, transactionReference: invoiceNumber } = invoice

await BillModel.query().patch({ invoiceNumber }).where('externalId', id)
}
}

async function _updateStatus (billRunId, status) {
return BillRunModel.query()
.findById(billRunId)
.patch({ status, updatedAt: timestampForPostgres() })
}

module.exports = {
go
}
Loading

0 comments on commit 9d2c828

Please sign in to comment.