Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create return cycle dates library #1436

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

robertparkinson
Copy link
Collaborator

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

As part of the work to replace the legacy system NALD we need to generate return logs and return cycles. This PR creates a library for calculating dates related to return cycles and the unit tests to cover them.

Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for splitting this out. It's made reviewing the changes much easier.

And this means I have some feedback and suggested changes. Do let me know if you have any issues with them, or challenges.

Comment on lines +4 to +5
* General helper methods
* @module DatesLib
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* General helper methods
* @module DatesLib
* Helper methods to deal with return cycle dates
* @module ReturnCycleDatesLib

Comment on lines +8 to +9
const { returnCycleDates } = require('./static-lookups.lib.js')
const { formatDateObjectToISO } = require('./dates.lib.js')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { returnCycleDates } = require('./static-lookups.lib.js')
const { formatDateObjectToISO } = require('./dates.lib.js')
const { formatDateObjectToISO } = require('./dates.lib.js')
const { returnCycleDates } = require('./static-lookups.lib.js')

}

/**
* Given an arbitary date and if it is summer or all-year return the end date of that cycle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Given an arbitary date and if it is summer or all-year return the end date of that cycle
* Given an arbitrary date and if it is summer or all-year return the end date of that cycle

Comment on lines +54 to +71
function cycleDueDateByDate (date, summer) {
const year = date.getFullYear()
const month = date.getMonth()

if (summer) {
if (month > returnCycleDates.summer.endDate.month) {
return formatDateObjectToISO(new Date(`${year + 1}-${returnCycleDates.summer.dueDate.month + 1}-${returnCycleDates.summer.dueDate.day}`))
}

return formatDateObjectToISO(new Date(`${year}-${returnCycleDates.summer.dueDate.month + 1}-${returnCycleDates.summer.dueDate.day}`))
}

if (month > returnCycleDates.allYear.endDate.month) {
return formatDateObjectToISO(new Date(`${year + 1}-${returnCycleDates.allYear.dueDate.month + 1}-${returnCycleDates.allYear.dueDate.day}`))
}

return formatDateObjectToISO(new Date(`${year}-${returnCycleDates.allYear.dueDate.month + 1}-${returnCycleDates.allYear.dueDate.day}`))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This applies to a few of the methods in this file. Our linter is not picking this up but some of the lines have gone beyond 120 chars. I think we can resolve this and make things a bit clearer at the same time. My suggestion would be the following.

Suggested change
function cycleDueDateByDate (date, summer) {
const year = date.getFullYear()
const month = date.getMonth()
if (summer) {
if (month > returnCycleDates.summer.endDate.month) {
return formatDateObjectToISO(new Date(`${year + 1}-${returnCycleDates.summer.dueDate.month + 1}-${returnCycleDates.summer.dueDate.day}`))
}
return formatDateObjectToISO(new Date(`${year}-${returnCycleDates.summer.dueDate.month + 1}-${returnCycleDates.summer.dueDate.day}`))
}
if (month > returnCycleDates.allYear.endDate.month) {
return formatDateObjectToISO(new Date(`${year + 1}-${returnCycleDates.allYear.dueDate.month + 1}-${returnCycleDates.allYear.dueDate.day}`))
}
return formatDateObjectToISO(new Date(`${year}-${returnCycleDates.allYear.dueDate.month + 1}-${returnCycleDates.allYear.dueDate.day}`))
}
function cycleDueDateByDate (date, summer) {
const year = date.getFullYear()
const month = date.getMonth()
let cycleDueYear
let cycleDueMonth
let cycleDueDay
if (summer) {
cycleDueDay = returnCycleDates.summer.dueDate.day
cycleDueMonth = returnCycleDates.summer.dueDate.month + 1
cycleDueYear = month > returnCycleDates.summer.endDate.month ? year + 1 : year
} else {
cycleDueDay = returnCycleDates.allYear.dueDate.day
cycleDueMonth = returnCycleDates.allYear.dueDate.month + 1
cycleDueYear = month > returnCycleDates.allYear.endDate.month ? year + 1 : year
}
const cycleDueDate = new Date(`${cycleDueYear}-${cycleDueMonth}-${cycleDueDay}`)
return formatDateObjectToISO(cycleDueDate)
}

But open to other ideas. I think the current pattern is based on my harping on about the 'guard clause' pattern. But I think this is a good example of an exception to that pattern.

Comment on lines +16 to +18
const today = new Date()
const year = today.getFullYear()
const month = today.getMonth()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we have to amend our assertions based on what the current date is.

I'd rather make this explicit in the tests that we have to do this by

  • controlling what 'today' is in the tests
  • having an explicit describe(), for example, "when 'todays' month is greater than the return cycle end date's month"

Take a look at let clock = Sinon.useFakeTimers(testDate) (we use it in some of our tests, for example, test/lib/general.lib.test.js) to understand how we can control the date and time within a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants