View licence summary abstraction periods

[robertparkinson]

https://eaflood.atlassian.net/browse/WATER-4329

Migrating the licence summary page from the water-abstraction-ui to the water-abstraction-system.

Show periods of abstraction

[Cruikshanks]

I'm around if you need to ping me about any of this!

[Cruikshanks]

This is less a convention and more a stipulation of mine! 😁

Where we are extracting data from the DB we never have a SELECT * even if it means listing out a dozen fields separately. I believe it's handy to be explicit about what data is needed. IMHO it also keeps the right side of performance because you are not pulling data across the network you don't need.

In Objection.js terms, a withGraphFetched() without a following .modifyGraph('licenceDocumentHeader', builder => { builder.select([]) }) is the equivalent of a SELECT * FROM licence_document_header`.

Would you mind adding in for both of these a modifyGraph() that lists out what fields we need from both tables?

[Cruikshanks]

The first thing I was going to ask was that if we have to put one of the clauses on a newline could we put all the clauses on a new line?

Suggested change

	if (!licenceVersions || licenceVersions.length === 0 || licenceVersions[0]?.licenceVersionPurposes === undefined ||
	licenceVersions[0]?.licenceVersionPurposes?.length === 0) {
	return null
	}
	if (
	!licenceVersions ||
	licenceVersions.length === 0 ||
	!licenceVersions[0]?.purposes ||
	licenceVersions[0]?.purposes?.length === 0
	) {
	return null
	}

Then spotted we have the exact same logic in _generatePurposes(). I don't normally worry about some duplication but as this is quite involved and within the same module should we also look to move this to its own method? For example

function _extractPurposes (licenceVersions) {
  if (
    !licenceVersions ||
    licenceVersions.length === 0 ||
    !licenceVersions[0]?.purposes ||
    licenceVersions[0]?.purposes?.length === 0
  ) {
    return null
  }

  return licenceVersions[0].purposes
}

Then I got to thinking did we need to check all the different states? Could we shortcut it? So, I threw this test together.

A bunch of stuff here I know so I'll try to summarise.

• Putting clauses on distinct lines if we have to wrap is a change I would wish applied
• Moving the logic to a helper function in the module is a strong suggestion but I can be argued out of it
• Using the short-cut version of helper function is merely an idea. A part of me quite likes we get a distinct null || [{}] if we use what you have

[robertparkinson]

it's not the same check. it's licenceVersions[0]?.licenceVersionPurposes vs licenceVersions[0]?.purposes === undefined I had started to pull everything into one large function called _generateAbstractionPeriodsAndPurposes and returned an object with the purposes and abstractionPeriods on it. perhaps that would be a better way to go?

[Cruikshanks]

My bad! Just putting each constraint on a separate line then (and I'll slink away)

[Cruikshanks]