refactor schedule inputs

[dlqqq]

Description

• Refactors ScheduleInputs component and the corresponding UI model to remove duplicate state
• Removes duplicate logic in handling state changes
• Implements more robust coercion logic, i.e. populating inputs when switching to/from easy scheduling
• Automatically validates on mount and removes validation errors on unmount
• triggers validation on change

Demo (outdated)

Screen.Recording.2022-11-02.at.12.58.14.PM.mov

Limitations

• Coercion logic has been kept as simple as possible to avoid maintainability issues. It parses each of the CRON terms via parseInt(), and uses a default only if the value cannot be parsed.
  • This means that inputting an invalid CRON expression like 99 99 * * MON-FRI, then switching to "weekday" results in the time being set to 99:99.
  • In other words, coercion generates an invalid output for invalid inputs.
  • I personally don't see this as a common edge case worth handling, as it requires the user to immediately input an invalid value and then switch intervals.
  • Furthermore, validation errors will prevent submission of an invalid coerced value

Related issues

• Closes refactor ScheduleInputs component #251
• Closes Job definition schedule errors are not reinstated when switching from JD to job and back #262

[github-actions]

👈 Launch a Binder on branch dlqqq/jupyter-scheduler/refactor-schedule-inputs

[JasonWeill]

Feedback from UX testing:

The "minutes of hour" interfere with the "time of day". If I select Hour, then pick 55, then switch to Day, the time is 00:55, but I had expected 00:00. Notably, this isn't bidirectional; if I change the time of day to 01:20, then switch back to Hour, the "minutes of hour" are 55, not 20. I think it would be better to keep "minutes of hour" and "time of day" (as hour and minute) as two separate state items.

Minor: 24-hour time is properly hh:mm, so 1:20 am is 01:20, but when I enter 01:20 and switch to a different interval (e.g., day to weekday, or vice versa) I see 1:20.

If I enter 33 as the day of the month, before I blur the control, I see helper text that the job will not run in months with fewer than 33 days:

When I blur the control, I see the expected 1–31 value validation message even after I refocus the control.

It might be better to impose errors on change, not just on blur, to prevent this.

Similarly, entering an invalid time causes a confusing helper text message, instead of an error:

[dlqqq]

The "minutes of hour" interfere with the "time of day". If I select Hour, then pick 55, then switch to Day, the time is 00:55, but I had expected 00:00. Notably, this isn't bidirectional; if I change the time of day to 01:20, then switch back to Hour, the "minutes of hour" are 55, not 20. I think it would be better to keep "minutes of hour" and "time of day" (as hour and minute) as two separate state items.

@jweill-aws They already are (model.scheduleMinute v.s. model.scheduleClock). I'm not able to reproduce this issue locally either:

Screen.Recording.2022-11-02.at.2.07.43.PM.mov

[JasonWeill]

I also don't see the "minute of hour" propagating to "time of day" bug anymore after checking out your code and rebuilding. Must have been a problem on my end.

[dlqqq]

Discussed this with @ellisonbg, we think it is better to validate on change rather than on blur. Just pushed this change.

[JasonWeill]

00:00 is displayed in 12-hour time in the helper text as "0:00 AM" instead of "12:00 AM"

[JasonWeill]

I created a job description to run at 2 minutes past the hour. When I edit it, I see "2" when I choose "Hour," and when I choose "Day", "Weekday", or "Week", the time is set to 00:02, which I hadn't expected.

Screen.Recording.2022-11-02.at.3.33.05.PM.mov

[dlqqq]

00:00 is displayed in 12-hour time in the helper text as "0:00 AM" instead of "12:00 AM"

@jweill-aws Fixed.

I created a job description to run at 2 minutes past the hour. When I edit it, I see "2" when I choose "Hour," and when I choose "Day", "Weekday", or "Week", the time is set to 00:02, which I hadn't expected.

@jweill-aws I don't consider this a bug; it's more of an implementation detail of how coercion works. The user will have to edit the clock field if they go from a schedule running on an hourly basis to one running on a daily/weekly basis. We make the most logical choice (preserving as many cron terms as we can), but obviously this isn't going to cover cases in which coercion isn't even well-defined.

[JasonWeill]

Technically, days of the week like MON (1) and FRI (5) are also valid cron terms, but this function would coerce them to 0 (Sunday).

[dlqqq]

Agreed, this is expected of the current implementation.