Fix broken CheckLiveBillRunService

[Cruikshanks]

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).

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

  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.

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 causes 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.

[Jozzey]