Skip to content

feat: support ISO 8601 (YYYY-MM-DD) date format - #876

Open
rapsealk wants to merge 10 commits into
opencats:masterfrom
rapsealk:feat/ymd-date-format
Open

rapsealk wants to merge 10 commits into
opencats:masterfrom
rapsealk:feat/ymd-date-format

Conversation

@rapsealk

@rapsealk rapsealk commented Sep 17, 2026 •

Copy link
Copy Markdown

Summary

This PR adds a third site-wide date format, YYYY-MM-DD (ISO 8601), alongside the existing MM-DD-YYYY (US) and DD-MM-YYYY (UK) options.

The option is available in Settings → Administration → Localization, the installer and the new-install wizard. When selected, it applies to displayed dates (SQL DATE_FORMAT output), date pickers, form validation and parsing, date extra fields (display, edit, data grid and export), calendar add/edit, email template date variables and the job order report period line.

The default behavior is unchanged: existing installations keep their current format.

Motivation

YYYY-MM-DD is the standard or common date format in many locales (for example Korea, Japan, China, Sweden, Lithuania and Canada), and it is unambiguous for international teams. The current D-M-Y/M-D-Y boolean doesn't allow it.

Implementation

  • No schema migration. site.date_format_ddmmyy (INT(1)) now stores 0 (M-D-Y), 1 (D-M-Y) or 2 (Y-M-D). Existing 0/1 values keep their meaning.
  • CATSSession::getDateFormat() returns the existing DATE_FORMAT_MMDDYY / DATE_FORMAT_DDMMYY / DATE_FORMAT_YYYYMMDD flag. isDateDMY() is kept for existing callers.
  • Site::setLocalization() and CATSSession::setTimeDateLocalization() accept either a date format flag or the previous boolean D-M-Y value (DateUtility::normalizeDateFormat()).
  • Sessions created before the upgrade keep their date format: the old _dateDMY property is still declared and is migrated to _dateFormat in CATSSession::__wakeup().
  • Dashboard weekly graph labels use d/m (D-M-Y), m-d (Y-M-D) or m/d (M-D-Y).
  • New DateUtility helpers replace the repeated isDateDMY() ? … : … ternaries:
    • getDateFormatFromSiteValue(), getSiteValueFromDateFormat(), getDateFormatFromFormValue()
    • getJsDateFormat() / getPhpDateFormat() (default to the session format, like getTimeFormat())
    • formatStoredDate() converts extra field dates, which are still stored as MM-DD-YY, for display.
  • DatabaseConnection rewrites %m-%d-%y / %m/%d/%Y etc. to %Y-%m-%d / %Y/%m/%d in Y-M-D mode, just as it already does for D-M-Y.
  • Y-M-D uses a 4-digit year, which DateUtility::validate()/convert() (DATE_FORMAT_YYYYMMDD) and calendarDateInput.js (YYYY-MM-DD) already support.

Testing

  • Unit tests: added testDateFormatMapping and testFormatStoredDate to DateUtilityTest, and SessionDateFormatTest (boolean setter values, migration of pre-upgrade sessions). UnitTests pass (124 tests) on PHP 8.5.
  • Integration tests: IntegrationTests pass (20 tests) against docker-compose-test.yml.
  • Manual checks with curl against the docker-compose-test.yml stack, logged in as testerRoot:
    • Localization settings: D-M-Y was preselected. Saving ymd stored date_format_ddmmyy = 2, and after logging back in YYYY-MM-DD (ISO) was preselected.
    • Candidate add page emitted CATSUserDateFormat = 'YYYY-MM-DD'. Submitting dateAvailable=2026-09-30 stored 2026-09-30. Submitting 09-30-26 was rejected with "Invalid availability date".
    • Candidate show/edit pages and the calendar date input render YYYY-MM-DD.
    • Date extra field: 2026-10-01 was stored as 10-01-26 (unchanged storage format), shown as 2026-10-01 on show/edit, and the data grid pagerRender/exportRender output was 2026-10-01.
  • Browser tests (Playwright, Chromium) in each of M-D-Y, D-M-Y and Y-M-D. All dates use days above 12, so a day/month swap would show:
    • Calendar add: clicking Oct 23 prefilled 10-23-26 / 23-10-26 / 2026-10-23. DB stored 2026-10-23, and the event panel showed the same format.
    • Calendar edit: the form prefilled the event date. Moving it to the 27th stored 2026-10-27.
    • Activity inline edit (js/activity.js → ajax/editActivity.php, the Day/Month mixed up when editing activities if UK locale set #344/Mismatch bei Date format. #52 path): Oct 14 → Oct 27 was displayed correctly before and after a reload, and DB stored 2026-10-27.
    • Dashboard weekly labels: 09/14 - 09/20 / 14/09 - 20/09 / 09-14 - 09-20.
    • Session upgrade: with the site in D-M-Y, a session file rewritten to the old shape (_dateDMY = true) still rendered DD-MM-YY, was re-saved with _dateFormat = D-M-Y and _dateDMY = null, and logged no deprecation.

Notes

  • The dashboard weekly graph image labels its x-axis 0…14 (artichow), so the week labels are not visible in the rendered image. This is not changed by this PR, and I have not checked whether master does the same.
  • Unrelated issue found while testing (present on master too): date-only values such as date_available are shifted by the time zone offset in DATE_FORMAT output (e.g. 2026-09-30 shown as 09-29-26 with OFFSET_GMT=2 and site time zone 0).

Related

🤖 Generated with Claude Code

Add YYYY-MM-DD (ISO 8601) alongside the existing MM-DD-YY (US) and
DD-MM-YY (UK) date formats.

The existing site.date_format_ddmmyy column now stores 0 (M-D-Y),
1 (D-M-Y) or 2 (Y-M-D), so no schema migration is needed and existing
installations keep their current format. The session exposes the
format as a DATE_FORMAT_* flag via getDateFormat(), and DateUtility
gains helpers to map it to site values, form values, and JS/PHP format
strings. isDateDMY() is kept for existing callers.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 04:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@RussH RussH self-assigned this Sep 17, 2026
@RussH
RussH self-requested a review September 17, 2026 09:39

@RussH RussH left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for contributing this — YYYY-MM-DD support is a useful addition to OpenCATS, and I really appreciate you taking the time to work through the existing date-handling paths rather than bolting on a separate implementation. I’d definitely like to see more contributions from you.

I am being a little pickier than usual at the moment, mainly because I’ve had to do some cleanup after a few recent merges, so I’d appreciate it if you could accommodate a few changes/enhancements before I approve this one:

Session compatibility: replacing the declared _dateDMY property means existing serialized sessions can produce a PHP dynamic-property deprecation and temporarily fall back to M-D-Y. Please preserve compatibility for users who are already logged in rather than relying on a logout/login after upgrade.
Dashboard: Dashboard.php still treats the format as DMY vs everything else, so YYYY-MM-DD currently falls back to M/D graph labels. Please update this to handle the new format properly.
Setter compatibility: Site::setLocalization() and CATSSession::setTimeDateLocalization() previously accepted a boolean DMY value. Please retain backward compatibility for existing callers while also supporting the new date-format constants.
Browser coverage: please give the actual browser paths a quick test, particularly Calendar add/edit and Activity inline edit. These are the areas I’m most cautious about because date parsing there has caused issues historically.

One integration point to be aware of: there is quite a lot of overlap with the Bootstrap 5.3 conversion currently underway in feature/bootstrap-5.3, including Calendar, Candidates, Contacts, Job Orders and the shared Activity modal. That’s not a blocker for this PR — just please keep the changes focused on the date-format functionality and avoid unrelated markup/template cleanup. I can then carry the date-format changes into the Bootstrap-converted versions when integrating it.

I haven’t found any obvious new security concerns, and overall I think the direction of the implementation is good. If you can address the points above, I should be much happier approving it.

Thanks again for taking the time to contribute this.

@rapsealk

rapsealk commented Sep 18, 2026 •

Copy link
Copy Markdown
Author

@RussH Thanks for the detailed comments! I’ll work through the points you mentioned and polish the PR accordingly. No rush on the review, so please take your time!

rapsealk and others added 3 commits September 18, 2026 12:14
Site::setLocalization() and CATSSession::setTimeDateLocalization() accept
the previous boolean D-M-Y argument again, alongside the date format
constants. Before this, true was silently stored as M-D-Y because the
constants are 1/2/4 and match() compares strictly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sessions serialized before _dateFormat was added still hold _dateDMY.
Declare it again (no dynamic-property deprecation) and migrate it to
_dateFormat in __wakeup(), so logged-in users keep D-M-Y without having
to log in again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 18, 2026 03:35
@rapsealk

Copy link
Copy Markdown
Author

@RussH Thanks again for the review. I've pushed one commit per point:

  • Session compatibility (92c45dd): _dateDMY is declared again, so there's no dynamic-property deprecation, and CATSSession::__wakeup() migrates it to _dateFormat. Users who are logged in during the upgrade keep D-M-Y without logging in again. I tested this with a real session file rewritten to the old shape, and there's a unit test for it too.
  • Dashboard (de0b765): weekly labels now use d/m for D-M-Y, m-d for Y-M-D and m/d for M-D-Y. Y-M-D renders as 09-14 - 09-20. If m/d or Y-m-d reads better to you, I can switch it.
  • Setter compatibility (dfc3c2e): Site::setLocalization() and CATSSession::setTimeDateLocalization() accept the old boolean again, as well as the date format constants. This also fixes a bug in the previous version: true was silently stored as M-D-Y, because the constants are 1/2/4 and match compares strictly.
  • Browser coverage: I tested Calendar add, Calendar edit and Activity inline edit in Chromium with Playwright, in all three formats, using days above 12 so a day/month swap would show. Every check passed, and the details are in the PR description.

The template changes are still limited to the date-format lines, so they should carry over to the Bootstrap 5.3 versions easily.

One thing I noticed that isn't changed here: the weekly dashboard graph image labels its x-axis 0…14, so the week labels don't show in the rendered image. I haven't checked whether master does the same.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 18, 2026 03:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@RussH

RussH commented Oct 6, 2026

Copy link
Copy Markdown
Member

@rapsealk Thanks again for this, and apologies that it has taken a little while to get back to it.

Firstly, I really appreciate the date localisation work here. Adding YYYY-MM-DD makes OpenCATS considerably more useful internationally and moves us away from being US/UK-centric. That is exactly the sort of improvement I’d like to see upstream, and if you identify any other relatively self-contained internationalisation or localisation gaps while working with OpenCATS, please do flag them.

I’ve also gone back through your response to my previous review. Thank you for addressing those points:

  • preserving existing serialized sessions via _dateDMY / __wakeup()
  • retaining backwards compatibility in the localisation setters
  • adding explicit Y-M-D handling
  • testing Calendar add/edit and Activity inline editing across all three formats

Those were the main concerns I had with the original implementation and your follow-up addressed them well.

Since then, however, there has been considerable churn on master. The Bootstrap 5.3 conversion has now landed, along with a number of security/authorisation changes and other modernisation work, so the PR is now sufficiently behind master that I don’t think it would be safe to merge without refreshing it first.

Could you please rebase/update the branch against current master and resolve the conflicts by retaining the current upstream Bootstrap and security/authorisation behaviour, applying only the date-format functionality from this PR?

One point from your earlier notes is now obsolete: the Artichow dashboard graph implementation has since been removed and replaced with Chart.js. There is therefore no need to preserve or further investigate the old Artichow 0…14 axis-label behaviour.

Your separate observation about date-only values being shifted by the timezone offset is useful, but I’d prefer not to expand this PR to fix it. If it is still reproducible against current master after the rebase, please flag it separately and we can deal with it as its own bug.

Once rebased, could you run the current test suite locally on both PHP 8.4.1 and PHP 8.5, including PHPUnit, integration and Behat tests?

It would also be useful to re-check the principal date paths against the current code:

  • Localisation settings
  • Candidate add/edit
  • Job Order add/edit
  • Calendar add/edit
  • Activity inline edit
  • Date extra fields
  • Existing M-D-Y and D-M-Y behaviour
  • New Y-M-D behaviour

If the existing Bootstrap Behat date-format scenarios can be extended to cover Y-M-D as part of the refresh, that would be particularly useful.

Please keep the refresh scoped to the YYYY-MM-DD feature rather than doing wider refactoring while resolving the conflicts.

I’m keen to roll the next OpenCATS release fairly soon, and I’d very much like to include this feature if we can get the refreshed branch cleanly rebased and tested beforehand.

Thanks again for the contribution — the underlying approach still looks good and this is a worthwhile improvement to OpenCATS.

RussH commented Oct 15, 2026

Copy link
Copy Markdown
Member

Hi @rapsealk, just a friendly follow-up on this one. I’m still keen to get YYYY-MM-DD support into OpenCATS, and I appreciate the work you’ve already put into it.

Have you had a chance to look at rebasing the branch onto current master and rerunning the tests? I know resolving conflicts after the recent Bootstrap and security changes may take a little time, so no pressure — it would just be helpful to know whether you’re planning to pick it up and roughly when.

I’m hoping to put together the next release soon and would be pleased to include this if we can get it refreshed and passing. Thanks again!

@rapsealk

Copy link
Copy Markdown
Author

@RussH Hi! Thanks for the feedback! I've been busy lately and haven't had a chance to revisit this, so it almost slipped my mind.

I'll be away until next weekend, so would you mind if I used Claude Code to work on this in the meantime? Thanks for your understanding! :)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 15, 2026 08:20
@rapsealk rapsealk changed the title feat: add YYYY-MM-DD date format option feat: support ISO 8601 (YYYY-MM-DD) date format Oct 15, 2026
@rapsealk
rapsealk requested a review from RussH October 15, 2026 08:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A dashboard integration-test fixture lacks the new session method, and calendar add/edit shows a two-digit year in YYYY-MM-DD mode.

2 open findings

🧠 Review effort: Lite

Comment thread lib/Dashboard.php
Comment thread modules/calendar/CalendarUI.js Outdated
Comment on lines +176 to +177
: userDateFormat == "YYYY-MM-DD"
? entry.year + "-" + monthString + "-" + dayString

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in 7302fb0.

RussH commented Oct 15, 2026

Copy link
Copy Markdown
Member

Hi @rapsealk, thanks for getting back to me so quickly, and for updating the branch!

First of all, there's absolutely no rush. Please enjoy your time away — this can wait until you're back and have the availability to work on it. I appreciate that this is a voluntary contribution.

And Claude Code is absolutely fine with me. I'm happy for contributors to use AI coding tools; the important thing is that the resulting changes are reviewed, kept focused and properly tested.

I can see you've now merged current master into the branch, which has cleared the merge conflicts. The update has surfaced two issues that will need addressing before approval:

  1. Dashboard integration test: ChartDataTest provides a session double with isDateDMY() but no getDateFormat(). The updated Dashboard::getPipelineData() calls getDateFormat(), so the test fixture needs updating to preserve its expected D-M-Y labels. See Copilot's finding.
  2. Calendar four-digit year: In YYYY-MM-DD mode, SetDateInputDate() still writes year % 100 into the visible year field. That turns 2026 into 26 in calendar add/edit. The shared helper needs to retain the full year for Y-M-D while preserving the existing behaviour for the other formats. See Copilot's finding.

The latest GitHub Actions run is marked action_required, so it doesn't yet confirm the tests pass. When you get back to this, could you also run the PHPUnit, integration and Behat suites on PHP 8.4.1 and 8.5, and recheck the main date-entry paths? As before, please keep the changes scoped to the date-format feature.

Thanks again — I'm still keen to include this improvement, but your availability comes first. No need to work on it while you're away.

…st session

- SetDateInputDate() now writes the picked year padded to the active
  format (YYYY for Y-M-D, YY otherwise) instead of always year % 100.
- ChartDataTest session double implements getDateFormat().
- Extend calendar and activities Behat date-format outlines with YMD.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 16, 2026 10:43
@rapsealk

Copy link
Copy Markdown
Author

@RussH Thanks for the review. Both points are fixed in 7302fb0:

  1. ChartDataTest: the session double now implements getDateFormat() and returns DATE_FORMAT_DDMMYY, so the D-M-Y labels are unchanged. Integration tests pass on 8.4.1 and 8.5.
  2. Calendar four-digit year: SetDateInputDate() now writes object.picked.yearPad (and sets defaultValue), which is already padded to the active format: YYYY for Y-M-D and YY for M-D-Y/D-M-Y. This is the same approach PickDisplayDay() uses, so calendar add/edit and every other caller are fixed in the shared helper.

I also added YMD to the existing Bootstrap Behat date-format outlines:

  • calendar.feature: create/edit/delete, with a new year column, so edit now asserts 2030 in Y-M-D
  • activities.feature: log and schedule together

The step definitions map MDY/DMY/YMD to 0/1/2.

Local results (test/runLocalTests.sh, PHP 8.4.1 and 8.5.10):

Suite 8.4.1 8.5
PHPUnit unit pass (258) 6 failures in InstallerPresentationTest (see below)
PHPUnit integration pass pass
Behat security 1423/1423 1423/1423
Behat default 182/183 180/183

All new Y-M-D calendar and activity examples pass, along with the existing M-D-Y and D-M-Y ones.

Remaining failures, which I believe come from my local environment rather than this PR:

  • InstallerPresentationTest on 8.5.10 fails on PHP 8.5's "non-canonical cast" deprecations ((integer)/(boolean) in Session.php, Candidates.php and Attachments.php). Those lines are identical on master.
  • importExport.feature (Map Data step) also fails on a clean master checkout in the same environment.
  • calendar.feature:137 (grouped events) and job-orders.feature:237 (delete from search result) don't touch the date-format paths. job-orders:237 passed on master locally, though, so I'll keep checking it. CI should settle it once the workflow run is approved.

The timezone shift on date-only values is not part of this PR, as agreed. I'll raise it separately if it still reproduces.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The change spans many date-handling paths across backend, frontend, installation, and reporting, warranting final human review.

2 open findings

🧠 Review effort: Lite

Copilot AI lite review requested due to automatic review settings October 16, 2026 10:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Critical defects remain in candidate merge date parsing and extra-field dates before approval.

4 open findings

🧠 Review effort: Lite

Comment thread lib/DatabaseConnection.php
Comment thread lib/ExtraFields.php
…lossy extra-field dates

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 16, 2026 11:11

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Calendar edit dates can use a two-digit year in Y-M-D mode, preventing the form from updating correctly.

4 open findings
1 resolved since last review

🧠 Review effort: Lite

Comment thread modules/calendar/CalendarUI.js Outdated

@RussH RussH left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks again for all the work you've put into this, particularly the additional fixes over the last couple of days. I appreciate how responsive you've been to the reviews.

I think we're reaching the point where the scope of testing and integration is becoming more than I'd reasonably expect for a contribution to add YYYY-MM-DD support.

Could you address the latest Calendar edit issue concerning the two-digit year? That looks like the last directly identifiable functional regression: Calendar::getEventArray() supplies a two-digit entry.year, whereas the new YYYY-MM-DD branch needs a four-digit year.

After that, I'm happy to take responsibility for the broader integration and regression testing on our side, including compatibility with the ongoing Bootstrap 5 work and any remaining legacy date-handling limitations.

I don't want to keep moving the goalposts on you. The contribution is appreciated, and I'd like to get it over the line.

Thanks again!

Calendar::getEventArray() returns a two-digit year, so the Y-M-D branch
built e.g. 26-10-23, which SetDateInputDate() rejects for YYYY-MM-DD.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings October 16, 2026 12:47
@rapsealk

Copy link
Copy Markdown
Author

Thanks @RussH, and thanks for taking on the broader integration testing. Fixed the calendar edit two-digit year in a291bcb: the Y-M-D branch now uses GetGoodYear(yearString), so editing an event prefills 2026-10-23 instead of 26-10-23. M-D-Y and D-M-Y are unchanged.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved review issues were identified, and the supplied assessments indicate approval readiness.

1 open finding
3 resolved since last review

🧠 Review effort: Lite

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.

3 participants