feat: Reason for delayed registration in v2 birth #8533
Conversation
|
Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:
|
| if (typeof value === 'string') { | ||
| const formattedDate = value | ||
| .split('-') | ||
| .map((d: string) => d.padStart(2, '0')) |
There was a problem hiding this comment.
What happens to years here? Could you move this under utils.ts next to FormFieldGenerator? We could write test cases under utils.test.ts and run this function with different values.
I think with -- this returns 00-00-00
There was a problem hiding this comment.
Valid years are always of four digits. So, if there is a year which is of less than two digits, it should be considered invalid. In that sense, we don't need to make it four digits. What do you think?
There was a problem hiding this comment.
Sorry, let me rephrase.
Could we write few test cases for this function. If I understand correctly, it receives values from formik. Formik DateField returns a string. When DATE input is empty, '--' is returned.
When we are building on top of that, I'm more worried that the functionality goes undocumented. Tests could help us there if we are not changing DateField behavior.
Pseudocode:
const cases = [
{
expected: '--',
actual: // whatever you expect
},
{
expected: '01-02-',
actual: // whatever you expect
},
{
expected: '1-2-3',
actual: // whatever you expect
},
{
expected: '01-2-32',
actual: // whatever you expect
},
...,
];
cases.forEach((case) => {
// or snapshot if you prefer
expect(case.input).toBe(case.output)
})
| } | ||
| return acc | ||
| }, | ||
| { ...values } |
There was a problem hiding this comment.
I suppose this does not need to be destructured. Let's aim that we don't mutate as a principle
| /** | ||
| * Checks if the date is within `days` days in the past from now. | ||
| */ | ||
| isAfter: (days: number) => FieldAPI |
There was a problem hiding this comment.
Could inPast work? Then possible inFuture would work. (or
Would it make isBeforeNow -> isPast?
Something that would work both ways and we could unify the terms under.
daysInPast daysAgo daysAfter daysSince daysPassedSince
| * @returns adds 0 before single digit days and months to make them 2 digit | ||
| * @because ajv's `formatMaximum` and `formatMinimum` does not allow single digit day or months | ||
| */ | ||
| function getValuesWithFormattedDate( |
There was a problem hiding this comment.
Appreciate the comment 🙏 @ because might not be supported. It seems that we are going through all the values but only touching one. Would there be more specific names to functions, or could the implementation be more generic?
| @@ -95,17 +95,30 @@ export function eventHasAction(type: ActionDocument['type']) { | |||
| } | |||
| } | |||
|
|
|||
| type dateBoundary = { | |||
| isAfter: () => ({ | ||
| days: (days: number) => ({ | ||
| inPast: () => | ||
| addCondition(getDateRange(getDateFromNow(days), 'formatMinimum')), | ||
| inFuture: () => | ||
| addCondition(getDateRange(getDateFromNow(-days), 'formatMinimum')) | ||
| }), |
There was a problem hiding this comment.
Really like the abstraction effort! Is the isAfter nested with inPast and inFuture bit odd? could they be flattened or what do yout hink?
There was a problem hiding this comment.
isAfter and isBefore are the boundaries, whereas inPast and inFuture indicates if some boundary is n days in the past (n days before now) or n days in the future (n days after now).
We could ditch inPast and inFuture to make it refer to the past date by default, and future when days is -n .
…ncrvs-core into v2-birth-validators
No description provided.