[PER-10643] Add edtf date and time validation - #1056
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1056 +/- ##
==========================================
+ Coverage 52.19% 52.64% +0.44%
==========================================
Files 354 354
Lines 12089 12199 +110
Branches 2185 2220 +35
==========================================
+ Hits 6310 6422 +112
+ Misses 5555 5549 -6
- Partials 224 228 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2a3db6f to
215f0c0
Compare
f3c2c87 to
374ded6
Compare
|
@cecilia-donnelly @slifty I have managed to rebase this one on the branch that's been checked out from too :D |
44786c5 to
6de3071
Compare
215f0c0 to
a71e024
Compare
6de3071 to
1c74479
Compare
|
Thanks for the branch management! For others reading this (QA), this needs a feature flag to be turned on: |
cecilia-donnelly
left a comment
There was a problem hiding this comment.
I love the way this is looking! The "sometime before"/"sometime after" language for an open ended range is very cool.
- I did see that if I enter February 29 in a non-leap-year, the error is "Day must be between 1 and 31" which is not exactly what we want to express there. I'm not sure what this should be, but maybe @omnignorant knows.
- If I leave both start and end dates empty and indicate that this is a range then I get "sometime before .." which isn't exactly right either
- If I have an open range (no start or end dates), then remove the "range" indicator, the new value is an empty string which gets an error from the backend. Maybe that's fine, but maybe we should instead send "Unknown"? QA/design question.
- the sidebar behaves slightly differently from the modal for invalid data. The sidebar allows you to click "save" with only a time filled (no date) while the modal blocks that path. The time-only value doesn't save, but in the sidebar the user gets a "We could not update" error in a toast instead of the nicer inline "A complete date is required when time is provided."
None of these are showstoppers, but if possible would be nice to fix!
a71e024 to
3876198
Compare
1c74479 to
fe7dd07
Compare
3876198 to
b6b99b9
Compare
|
When the date and time fields are undefined, we send null to the BE PATCH for the displayTime property and then we show the date field empty in the sidebar.
|
fe7dd07 to
57c3de4
Compare
slifty
left a comment
There was a problem hiding this comment.
I might be coming at this with the wrong design philosophy, so please forgive me for possibly questioning defined specification!
I think right now we have code that is silently attempting to make inferences about user intent which may or may not actually be correct.
I'd encourage we reconsider this approach, and instead simply convey "this isn't correct, you need to fix it by X" (or possibly offering a suggested correction).
All that said, if we do decide to keep attempted inference we should make sure that the logic for save / serialization matches the logic for formatting / rendering.
|
@aasandei-vsp and I talked about the null problem in More succinctly:
|
cecilia-donnelly
left a comment
There was a problem hiding this comment.
Officially requesting changes so this doesn't show up in my "review requested" list anymore. The specific change requested is to remove the fallback in the file list behind the feature flag.
I have created a commit specifically for this situation, making it easier to review. |
There are indeed a lot of scenarios and dealing with each one of them proved to be slightly complex. Your suggestions are great and correct, but at some point it was decided that we need to silently make some decisions for the user, as not everyone is savvy when it comes to edtf dates. I'll be addressing the rest of the comments one by one. |
57c3de4 to
67edbff
Compare
cecilia-donnelly
left a comment
There was a problem hiding this comment.
The code makes sense but there are a couple things I found while testing it out:
- Clear date doesn't work because somewhere we're sending an empty string instead of explicit null
- I can't enter a month value like "3" -- the sidebar simply doesn't show anything if I try. "03" works, but I should be able to enter a plain "3" (or any other single digit) and have it be padded with a zero afterward
- It looks like the same is happening for hours
- It lets me enter a value like "2026-XX-10" but then I can't set it to "uncertain" or "approximate." That might be an EDTF restriction, do you know?
I would go back through and do some testing again -- I know this has been waiting a while so maybe there are some odd interactions?
Also I see that you put the keyboard interactions back, disregard my inline comments! Thank you!
| } | ||
|
|
||
| if (message.includes('complete date is required')) { | ||
| return 'A complete date is required when time is provided.'; |
| [class.disabled]="disabled" | ||
| (click)="toggleDatepicker()" | ||
| (keydown.enter)="toggleDatepicker()" | ||
| (keydown.space)="toggleDatepicker()" |
There was a problem hiding this comment.
It's true that at some point in the past Claude removed them for some reason and I didn't notice and I'm glad you did! Now it's all in there, thank you for having such a thorough look!
| [class.disabled]="disabled" | ||
| (click)="toggleTimepicker()" | ||
| (keydown.enter)="toggleTimepicker()" | ||
| (keydown.space)="toggleTimepicker()" |
There was a problem hiding this comment.
Same question - why remove this? (Maybe this isn't acting as a button anymore?)
There was a problem hiding this comment.
It's true that at some point in the past Claude removed them for some reason and I didn't notice and I'm glad you did! Now it's all in there, thank you for having such a thorough look!
Clear date+only one digit Unspecified digits+qualifiers The library's grammar treats "unspecified digits" (X) and "qualification" ( ~ / ? / % ) as separate, non-combinable features, so it throws a syntax error on anything like 2026-XX-10~ — regardless of where the qualifier is placed. That error was being caught and surfaced as the generic "date is not valid" message, which is why you could enter 2026-XX-10 but not mark it approximate/uncertain. I verified the backend happily accepts and stores these strings (2026-XX-10~ and friends), so the restriction was purely FE. The fix works around the library by stripping the trailing qualifier before parsing/validating, then reattaching the flags to the model ourselves — so 2026-XX-10~ now serializes, saves, and parses back into the picker correctly, in both directions. One intentional boundary: a qualifier combined with a time (e.g. 2026-01-01T10:00:00~) is still rejected — the library doesn't support that combination either, and since an unspecified date and a time can't coexist anyway, we kept that blocked rather than silently allowing it. |
bad07a3 to
a3a7e5a
Compare
a3a7e5a to
452da47
Compare
cecilia-donnelly
left a comment
There was a problem hiding this comment.
Thanks for this, @aasandei-vsp! I tested on dev this time to avoid any local environment oddities. I did find one edge case. When I put in "2021-2-29" (i.e., a leap day in a non-leap year) instead of erroring, the UI autocorrected to "2021-03-01" (no doubt a Date library fix). I think we want to give an error there. Final little things!
452da47 to
5381112
Compare
|
So much here! So close! Thank you for this long list of tests. I did find another weird case. I made a new folder on dev ("edtf-tests") for the testing. I intended to test on a record in that folder, but accidentally ran one on the folder itself. I tried to edit the date from "1990-09" to "1990-07." I got a 200 response from the PATCH call in devtools with the correct new value, but the main (top of screen) error toast said that it couldn't "update the item you selected" and in fact it did not. In the console I see
Some notes that aren't blockers for this PR, but would be improvements:
|
5381112 to
6458a27
Compare
This is quite an edge case, thank you for looking into it. I couldn't get it to reproduce on my end, but the error you pasted helped me track it down. You were right that the save itself worked; the problem was in the code that syncs the server's response back onto the folder afterward. It matches the response to the folder by folderId, but the folder in memory holds that id as a number while the response comes back with it as a string, so the match failed and we ended up trying to update undefined — which is what reverted your change and threw that error.
We can definitely have a range with qualifiers. The difference is the FE accepts the start date to have a qualifier and the end date to have time attached to it, while the BE does not accept this combination. My take would be just to leave it like this, because the user does get an error shown. Otherwise, I can look into it and see how to sync with the BE. But I feel that there might still be validation differences between BE and FE and not sure we could address them all. NOTES
|
cecilia-donnelly
left a comment
There was a problem hiding this comment.
Hurray! Let's go to QA on this one. Thanks for the detailed response to my last comment.
QA InstructionsSummaryThis pull request implements new Extended Date/Time Format (EDTF) validation logic for managing date and time inputs in both a sidebar inline picker and a modal. The changes include:
Test Environment Setup
Test ScenariosBasic Date Entry
Single-Digit Month/Day
Invalid Characters
Out-of-Range Values
Impossible Calendar Days
Time Input
Unspecified Digits (Modal Only)
Qualifiers (Modal Only)
Date Ranges (Modal Only)
Clearing Dates and Times
Save Gating and Persistence
Regression Risks
Things to Watch For
By following these test instructions, you'll cover all functional and edge case scenarios introduced in this pull request. Generated by QA Instructions Action |
Left-pad single-digit month and day values with zero instead of
X-padding them, since a single 1-9 digit can only be a complete value
('5' -> '05', no longer '5X'). Require a complete date whenever a time
is provided and surface a dedicated friendly error message for that
case. Add a generic getSegmentError helper that the datepicker and
timepicker inputs use to build their inline field error messages
(invalid characters and out-of-range checks, skipped while the value
is still being typed).
Issue: PER-10643
Instead of silently rejecting invalid input, the year, month and day segments now emit whatever the user typed and surface a per-field error message below the input (invalid characters, out-of-range month or day). The day is re-validated when the month changes, and picking a date from the calendar clears all errors. The error styling lives in new shared input-error-state and input-error-message mixins. Issue: PER-10643
Instead of silently rejecting invalid input, the hours, minutes and seconds segments now emit whatever the user typed and surface a per-field error message below the input. The hour is re-validated when the AM/PM/24H format changes. Issue: PER-10643
The inline-error restructure dropped role="button", tabindex, and the keydown.enter/space handlers from the calendar and clock toggle buttons, leaving them mouse-only. Restore them and add keyboard-toggle tests. Issue: PER-10643
Persist null instead of '' when a date is cleared, and render an
explicitly-null displayTime as empty ("Click to add date"). Widen
displayTime to string | null and add specs.
Issue: PER-10643
With the flag on, displayTime is authoritative (the migration backfills it before the flip), so a missing value means the date was cleared and nothing is shown. With the flag off, the file list keeps its fallback so deployed code is unchanged. Issue: PER-10643
Move the picker's display formatting into EdtfService so the preview
and the serialized EDTF are built from the same padding helpers — a
single-digit month now reads as the month itself ("1" -> January /
1985-01) in both. A lone "0" month or day is rejected with a clear
range error instead of being silently serialized to "0X".
Issue: PER-10643
The edtf library rejects a qualifier (~/?/%) combined with unspecified (X) digits, so approximate/uncertain dates with a blank month or day (e.g. 2026-XX-10~) failed validation, even though the backend accepts them. Strip the qualifier before parsing/validating and reattach the flags ourselves so the combination round-trips both ways. Issue: PER-10643
An impossible day such as Feb 29 in a non-leap year was accepted and silently autocorrected on save (the edtf library rolls it forward via a JS Date overflow, e.g. 2021-02-29 → 2021-03-01). Validate the day against the month and year and reject it instead, and pad a single-digit month so the day is checked against the right month rather than January. Show each segment's specific message inline on the offending field (day-for-month, out-of-range, lone "0") while the footer keeps the generic "not valid" message, and move the day/month segment validation into EdtfService. Issue: PER-10643
Normalize folderId to a string on both sides of the lookup and guard the update so an unmatched response can never crash an otherwise-successful save. Issue: PER-10643
6458a27 to
80e59ec
Compare
|
@cecilia-donnelly Thank you for putting this up for QA. I have added a manual test for the edge case you found with the new folder. |



Manual test cases — EDTF date/time input
Setup: log in, upload a file, click it.
EXPECTED: sidebar with a Date section. Unless a test says "inline picker", open the edit modal via More options for each test. The modal footer shows a live preview of the resulting EDTF string when valid, or a red error message when invalid, and the Save button is disabled while invalid.
Warning
The date and time inputs are the same shared component in both the inline sidebar picker and the More options modal. Every test that only exercises those fields — Basic date entry, Single-digit month/day, Invalid characters, Out-of-range values, Impossible calendar days, and Time — must be run in both places, because their commit behavior differs: the modal disables Save while invalid, whereas the inline picker lets you attempt the save and surfaces the error as a toast. Qualifiers, Unknown, and Date ranges exist only in the modal.
Note
Field-specific messages (invalid characters, out-of-range month/day/hours/minutes/seconds, day-for-month) appear inline below the offending input, with that input highlighted red. The modal footer separately shows the generic
The date entered is not valid. Please check the values and try again.whenever anything is invalid (with Save disabled) — the specific message is not duplicated there. Two exceptions with no inline field surface keep their specific text in the footer:A complete date is required when time is provided.andThe date range is not valid. Please make sure the start date is before the end date.Basic date entry
(run in both the inline picker and the modal)
Year only
2026in the year, leave month and day empty.2026. Save is enabled.Year and month
2026in the year,05in the month, leave day empty.2026-05. Save is enabled.Full date
2026,05,20.2026-05-20. Save is enabled.Auto-advance between segments
2026in the year.05in the month.Backspace navigation between segments
2026/05/20, then place the cursor in the empty day input and press Backspace until day is empty, then Backspace again.Calendar picker
Single-digit month/day (entered without zero-padding)
(run in both the inline picker and the modal)
Single-digit month is accepted and zero-padded
2026, month2(single digit), day15.2026-02-15— the month is zero-padded automatically even though you typed2. Save is enabled.02.Single-digit day is accepted and zero-padded
2026, month05, day5(single digit).2026-05-05. Save is enabled.Both month and day single-digit
2026, month3, day7.2026-03-07.Single-digit month, year-month only (no day)
2026, month4, leave day empty.2026-04.No premature error while a single digit is typed
2in the month input and stop.Single-digit month is used for day validation
2026, month2(single digit), day30.That day does not exist in the selected month and year.below the day input — the day is validated against February, not January. (This is the regression that was fixed: an un-padded month must not be treated as01.)2021, month2, day29.That day does not exist in the selected month and year.Single-digit
1month is treated as January2026, month1(single digit), day31.2026-01-31.Lone
0month is rejected (inline, on the month field)2026, month0(just a zero), day15.Month must be between 1 and 12.appears below the month input and the month field is highlighted red. A lone0is treated as an unfinished value, not a valid month. In the modal the footer shows the generic error and Save is disabled; in the inline picker a save attempt surfaces the error.Lone
0day is rejected (inline, on the day field)2026, month05, day0(just a zero).Day must be between 1 and 31.appears below the day input and the day field is highlighted red (same behavior as above).Lone
0month is attributed to the month field, not the day2024, month0, day31.Month must be between 1 and 12.appears (on the month field). The day field does not showThat day does not exist…— an invalid month is not mislabeled as a day error.Invalid characters
(run in both the inline picker and the modal)
Invalid characters in date and time
19abin the year, thenabin the hours.The date contains invalid characters./The time contains invalid characters.below the inputs and red styling.Out-of-range values
(run in both the inline picker and the modal)
Out-of-range month
13in the month input.Month must be between 1 and 12.appears below the input.12.Out-of-range day
2026, month01, then type32in the day input.Day must be between 1 and 31.appears below the input.31.Out-of-range hours (AM/PM mode)
AMorPM, type13in the hours input.Hour must be between 1 and 12.appears below the time input.10.13again (error reappears), then toggle the format until it shows24H(leave the hours as13).13is valid in 24-hour mode — without editing the hours value. Toggling back toAM/PMmakesHour must be between 1 and 12.reappear.Out-of-range hours (24H mode)
24H, type24in the hours input.Hour must be between 0 and 23.appears below the time input.23.24again (error reappears), then toggle the format toAM(leave the hours as24).Hour must be between 1 and 12.—24is invalid in both modes, but the message reflects the active format's range. (A 12-hour clock's valid hours are1–12; midnight is12 AMand noon is12 PM, so there is no0and the range is intentionally 1–12, not 0–12.)Out-of-range minutes
60in the minutes input.Minutes must be between 0 and 59.appears below the time input.59.Out-of-range seconds
60in the seconds input.Seconds must be between 0 and 59.appears below the time input.59.Impossible calendar days
(run in both the inline picker and the modal)
Feb 29 in a non-leap year
2021, month02, then type29in the day input.That day does not exist in the selected month and year.appears below the day input (the value is not silently autocorrected to2021-03-01). Save is disabled.2024.2024-02-29.Feb 29 with a single-digit month
2021, month2(single digit), then type29in the day input.That day does not exist in the selected month and year.(this previously slipped through with a single-digit month).Re-validation when month changes
2026, month01, day31(valid).04.That day does not exist in the selected month and year.appears (the day is re-checked against the new month).Time
(run in both the inline picker and the modal)
Format toggle (AM / PM / 24H)
AM→PM→24H→AM. The hour is re-validated against the new format (e.g.13is invalid in AM/PM but valid in 24H).Midnight and noon
12:00:00withAM.T00:00:00…).12:00:00withPM.T12:00:00…).Seconds optional
00in the saved value.Time requires a complete date
2026only (no month/day), then enter a time.A complete date is required when time is provided.in the footer and Save is disabled.Unspecified digits (X)
(modal only)
Unknown month with a known day
2026, leave month empty, enter day11.2026-XX-11. Save enabled.Partial year
198in the year (3 digits), leave month/day empty.198X.19in the year (2 digits).19XX.Unknown year with a known month
05.XXXX-05.Qualifiers
(modal only)
Approximate
2026/05/20, toggle Approximate on.2026-05-20~. Save enabled.Uncertain
2026-05-20?.Approximate + Uncertain (combined)
2026-05-20%.Qualifier with unspecified digits
2026, leave month empty, enter day11, toggle Approximate on.2026-XX-11~. Save enabled.11, and Approximate is still on (the value round-trips).Qualifier combined with a time is rejected
2026/05/20, enter a time (e.g.10:30), then toggle Approximate (or Uncertain) on.The date entered is not valid. Please check the values and try again.and Save is disabled (a qualifier with a time is unsupported).Unknown
XXXX-XX-XX. Save enabled.Unknown is mutually exclusive with Approximate/Uncertain
Date ranges
(modal only)
Basic range
2026/05and end2027/06.2026-05/2027-06. Save enabled.Range with time on both sides
2026/05/20with time10:30:00, and end2027/06/15with time14:00:00.2026-05-20T10:30:00+HH:MM/2027-06-15T14:00:00+HH:MM. Save enabled.Range with time on only one side
2026/05/20with time10:30:00, and end2027/06/15with no time.2026-05-20T10:30:00+HH:MM/2027-06-15(a timed start with a date-only end is allowed). Save enabled.2026-05-20/2027-06-15T14:00:00+HH:MM. Save enabled.A range side with a time still requires a complete date on that side
2026only (no month/day) but add a time to the start, and a valid end date.A complete date is required when time is provided.and Save is disabled (the requirement applies per side).Range validation (start after end)
2027and end2026.The date range is not valid. Please make sure the start date is before the end date.in the footer and Save is disabled.Open-ended range — "sometime after"
2026-05/... In the sidebar it reads Sometime after.Open-ended range — "sometime before"
../2027-06. In the sidebar it reads Sometime before.Per-side qualifiers on a range
2026-05~/2027-06?.Unspecified digits on one side of a range
2026/blank month/11with Approximate, and end2027/01.2026-XX-11~/2027-01. Round-trips on reopen.Unknown on one side of a range
Clearing
Clear date and time (inline picker)
Clear start / clear end (modal)
Save gating & persistence
Save disabled while invalid (modal)
13).Cancel discards changes
Round-trip persistence
New folder persistence