From d5e08513abf772d46f63fbb7e3419f332d6c3704 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 20 Mar 2024 12:07:52 +0000 Subject: [PATCH 1/4] Fix broken CheckLiveBillRunService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit https://eaflood.atlassian.net/browse/WATER-4379 We recently updated the `CheckLiveBillRunService` to support this project handling annual billing (see [Handle live bill run check for annual bill runs](https://github.com/DEFRA/water-abstraction-system/pull/761)). We thought all was well until we spotted some strange behaviour when testing. This app wouldn't create new SROC supplementary bill runs because `CheckLiveBillRunService` was claiming one was in progress. But a check of the DB found that wasn't the case. We added some extra debug logging, restarted the app and tried again ... and it worked fine! Ok, chalk it up to 'puters and their weird ways. Till it happened again. We looked again at the source and immediately spotted a flaw. But we thought it was unrelated. PR #761 updated the code in the service to this ```javascript const statuses = LIVE_STATUSES // Only one annual bill run per region and financial year is allowed. So, we include sent and sending in the statues // to check for if (batchType === 'annual') { statuses.push('sent', 'sending') } const numberOfLiveBillRuns = await BillRunModel.query() .select(1) .where({ regionId, toFinancialYearEnding, batchType, scheme: 'sroc' }) .whereIn('status', LIVE_STATUSES) .resultSize() return numberOfLiveBillRuns !== 0 ``` The flaw we spotted was `.whereIn('status', LIVE_STATUSES)`. Even though we'd created a new `statuses` object that we were updating if the batch type is 'annual', we weren't using it in our query. That line should have been `.whereIn('status', statuses)`. But hang on, our unit tests were saying everything is okay and working for both batch types. Huh!? 🫤 And then we had our 'a-ha' moment. 😁 This line `const statuses = LIVE_STATUSES` is not _copying_ `LIVE_STATUSES` to `statuses`. It is _setting_ `statuses` and `LIVE_STATUSES` to be the same thing. It is essentially the same thing as [Passing by reference vs passing by value](https://stackoverflow.com/a/430958/6117745). > Say I want to share a web page with you. If I tell you the URL, I'm passing by reference. You can use that URL to see the same web page I can see. If that page is changed, we both see the changes. If you delete the URL, all you're doing is destroying your reference to that page - you're not deleting the actual page itself. > > If I print out the page and give you the printout, I'm passing by value. Your page is a disconnected copy of the original. You won't see any subsequent changes, and any changes that you make (e.g. scribbling on your printout) will not show up on the original page. If you destroy the printout, you have destroyed your copy of the object - but the original web page remains intact. Our unit tests worked because the supplementary test runs _before_ the annual. When you run the annual it is causing the `LIVE_STATUSES` to get updated hence the query works and the test passes. Now, you can apply this same behaviour to an environment. When the app first starts `LIVE_STATUSES` won't include `'sending'` and `'sent'`. So, supplementary bill runs will go through. Then someone creates an annual bill run. Now `LIVE_STATUSES` does include those values and because it is declared outside of the scope of the function, it will stay that way for subsequent calls. This means the next supplementary bill run will fail because the query will return a result. Don't you love programming! 🤦😧😬 This change fixes the issue. From 87817e9189d45072161f33a5a88f88546a27f68f Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 20 Mar 2024 12:31:12 +0000 Subject: [PATCH 2/4] Copy LIVE_STATUSES Doing this our test will now fail. Whoopee! --- app/services/bill-runs/check-live-bill-run.service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/bill-runs/check-live-bill-run.service.js b/app/services/bill-runs/check-live-bill-run.service.js index cd9d8ca6b5..fc410fcdb3 100644 --- a/app/services/bill-runs/check-live-bill-run.service.js +++ b/app/services/bill-runs/check-live-bill-run.service.js @@ -26,7 +26,7 @@ const LIVE_STATUSES = ['processing', 'ready', 'review', 'queued'] * @returns {Boolean} Whether a "live" bill run exists */ async function go (regionId, toFinancialYearEnding, batchType) { - const statuses = LIVE_STATUSES + const statuses = [...LIVE_STATUSES] // Only one annual bill run per region and financial year is allowed. So, we include sent and sending in the statues // to check for From 44631153f8b3f4f57cd29cf86aca804a57661f17 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 20 Mar 2024 12:32:39 +0000 Subject: [PATCH 3/4] And now they pass again --- app/services/bill-runs/check-live-bill-run.service.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/bill-runs/check-live-bill-run.service.js b/app/services/bill-runs/check-live-bill-run.service.js index fc410fcdb3..310d340489 100644 --- a/app/services/bill-runs/check-live-bill-run.service.js +++ b/app/services/bill-runs/check-live-bill-run.service.js @@ -42,7 +42,7 @@ async function go (regionId, toFinancialYearEnding, batchType) { batchType, scheme: 'sroc' }) - .whereIn('status', LIVE_STATUSES) + .whereIn('status', statuses) .resultSize() return numberOfLiveBillRuns !== 0 From 902c4c61475002372ee5ac3fa8fe6f18260db92c Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Wed, 20 Mar 2024 12:37:12 +0000 Subject: [PATCH 4/4] Housekeeping - correct and update comments --- app/services/bill-runs/check-live-bill-run.service.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/services/bill-runs/check-live-bill-run.service.js b/app/services/bill-runs/check-live-bill-run.service.js index 310d340489..438aacd9a5 100644 --- a/app/services/bill-runs/check-live-bill-run.service.js +++ b/app/services/bill-runs/check-live-bill-run.service.js @@ -19,11 +19,11 @@ const LIVE_STATUSES = ['processing', 'ready', 'review', 'queued'] * The process is that an annual bill run is generated at the start of the financial year then multiple supplementary * bill runs to deal with any changes after the annual bill run has been processed. * - * @param {String} regionId The id of the region to be checked - * @param {Number} toFinancialYearEnding The financial year to be checked - * @param {String} batchType The bill run type to be checked + * @param {String} regionId - The UUID of the region to be checked + * @param {Number} toFinancialYearEnding - The financial year to be checked + * @param {String} batchType - The bill run type to be checked * - * @returns {Boolean} Whether a "live" bill run exists + * @returns {Promise} true if a live bill run is found else false */ async function go (regionId, toFinancialYearEnding, batchType) { const statuses = [...LIVE_STATUSES]