feat: Add all useful date formats for optimistic report name formulas#70617
Conversation
melvin-bot
Bot
requested review from
rojiphil
and removed request for
a team
|
@TaduJR When the PR is ready for review, please add the test videos as well and let me know. Thanks. |
|
Added Demo for MacOS Safari Thank you 🙏 |
| {token: 'S', value: getOrdinalSuffix(day)}, | ||
|
|
||
| // Time formats (longest to shortest) | ||
| {token: 'tt', value: meridiemUpperCase}, |
There was a problem hiding this comment.
Tests for tt fails as it considers t (i.e. number of days in month). Let us fix this. Also, curious as to why the test case did not catch this?
70617-web-chrome-issue-001.mp4
There was a problem hiding this comment.
Oh the issue was token processing order. The t token was defined before tt in the tokens array, so when both have the same processing priority, t was processed first. Fixed it by moving tt to appear before t in the token processing order within the time formats section.
| @@ -367,30 +386,115 @@ function formatDate(dateString: string | undefined, format = 'yyyy-MM-dd'): stri | |||
| const day = date.getDate(); | |||
| const monthNames = ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December']; | |||
| const shortMonthNames = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']; | |||
| const dayNames = ['Sunday', 'Monday', 'Tuesday', 'Wednesday', 'Thursday', 'Friday', 'Saturday']; | |||
There was a problem hiding this comment.
Don't we have to localize the day and month names?
| let result = format; | ||
|
|
||
| // Get time values for time formatting | ||
| const hours = date.getHours(); |
There was a problem hiding this comment.
Also, the time would be GMT here. Don't we have to use a local time instead?
|
In my opinion it'd be good idea to move datetime processing to new file - eg. |
TaduJR
force-pushed
the
feat-Add-all-useful-date-formats-for-optimistic-report-name-formulas
branch
from
be6bfb6 to
91c56c5
Compare
| const localTimezone = Intl.DateTimeFormat().resolvedOptions().timeZone; | ||
| const hours = parseInt(formatInTimeZone(date, localTimezone, 'H'), 10); | ||
| const minutes = parseInt(formatInTimeZone(date, localTimezone, 'm'), 10); | ||
| const seconds = parseInt(formatInTimeZone(date, localTimezone, 's'), 10); |
Codecov Report❌ Patch coverage is
... and 283 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Done. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppNot needed currently as we already verified in console logs in web version. Android: mWeb ChromeNot needed currently as we already verified in console logs in web version. iOS: HybridAppNot needed currently as we already verified in console logs in web version. iOS: mWeb SafariNot needed currently as we already verified in console logs in web version. MacOS: Chrome / Safari67921-web-chrome-test.mp4MacOS: DesktopNot needed currently as we already verified in console logs in web version. |
rojiphil
left a comment
rojiphil
left a comment
There was a problem hiding this comment.
Thanks @TaduJR for the updates. Just that the PR author checklist fails the check. Please have a look.
@neil-marcellini The code changes related to date formatting function LGTM. Unit tests also looks good. Although not able to test manually, I have also verified that the function call works via console logs.
All yours now. Thanks.
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
I have only reviewed the tests so far, but it looks like we need some changes.
| expect(result).toBe('15'); // 15:30 | ||
| }); | ||
|
|
||
| test('should format 12-hour with leading zero (h)', () => { |
There was a problem hiding this comment.
The assertion does not have a leading zero despite what the test says.
There was a problem hiding this comment.
It looks like we actually need to fix this on the backend as well. I created an issue.
There was a problem hiding this comment.
Great 👍. Also fixed the test
There was a problem hiding this comment.
Still needs to be fixed. I commented on the updated code.
| } as Transaction; | ||
| mockReportUtils.getReportTransactions.mockReturnValue([mockTransaction]); | ||
|
|
||
| // Format: hour without leading zero : minutes with space and am/pm |
There was a problem hiding this comment.
Why do the minutes have a space?
There was a problem hiding this comment.
Refactored with the new clean test
TaduJR
force-pushed
the
feat-Add-all-useful-date-formats-for-optimistic-report-name-formulas
branch
from
054bc63 to
f38ec00
Compare
…or-optimistic-report-name-formulas
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the update so far. Some of the expected results in the tests still need to be fixed.
| // 12-hour format: 3:30 PM (afternoon) and 9:05 AM (morning with leading zero) | ||
| setupMockDate(testDate); | ||
| expect(compute('{report:startdate:hh}', mockContextWithDate)).toBe('03'); | ||
| expect(compute('{report:startdate:h}', mockContextWithDate)).toBe('3'); |
There was a problem hiding this comment.
This needs to be '03'.
There was a problem hiding this comment.
Done with the specifications from the design docs.
|
|
||
| setupMockDate(morningDate); | ||
| expect(compute('{report:startdate:hh}', mockContextWithDate)).toBe('09'); | ||
| expect(compute('{report:startdate:h}', mockContextWithDate)).toBe('9'); |
| expect(result).toBe('15'); // 15:30 | ||
| }); | ||
|
|
||
| test('should format 12-hour with leading zero (h)', () => { |
There was a problem hiding this comment.
Still needs to be fixed. I commented on the updated code.
|
|
||
| setupMockDate(morningDate); | ||
| expect(compute('{report:startdate:HH}', mockContextWithDate)).toBe('09'); | ||
| expect(compute('{report:startdate:H}', mockContextWithDate)).toBe('9'); |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for the latest updates! A couple more tweaks needed, and questions. Looking pretty close, and I have now reviewed all of the code.
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
There are a couple more outstanding items. Please let me know when you have it updated, and Ill try to take a look as soon as I can.
I incorrectly thought that after j was replaced with a placeholder, the S token wouldn't be able to match. But you're correct - the regex /S/g matches S anywhere in the string, not just at the start. |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Thanks for fixing those last items. Good to go assuming we can get the CI checks to pass!
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
TaduJR
deleted the
feat-Add-all-useful-date-formats-for-optimistic-report-name-formulas
branch
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.2.38-0 🚀
|
|
@kavimuru Thank you for your comment! Please see the discussions on the issue pages |
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.38-5 🚀
|
TaduJR
restored the
feat-Add-all-useful-date-formats-for-optimistic-report-name-formulas
branch
6 participants
Explanation of Change
This PR implements comprehensive date format support for optimistic report name formulas, enabling the frontend to support the same extensive set of date format specifiers as the backend. The implementation replaces the previous hardcoded switch-case approach with a flexible token-based parsing system that can handle individual format tokens and their combinations.
Key improvements:
Supported format specifiers now include:
Fixed Issues
$ #67921
PROPOSAL: #67921 (comment)
Tests
- Enter formula: {report:startdate:MMMM}
- Verify full month name displays (e.g., "January")
- Enter formula: {report:startdate:MMM}
- Verify abbreviated month displays (e.g., "Jan")
- Enter formula: {report:startdate:MM}
- Verify month with leading zero (e.g., "01")
- Enter formula: {report:startdate:dddd}
- Verify full weekday name displays (e.g., "Wednesday")
- Enter formula: {report:startdate:jS}
- Verify day with ordinal suffix (e.g., "8th", "1st", "22nd")
- Enter formula: {report:startdate:W}
- Verify ISO week number displays (e.g., "02")
- Enter formula: {report:created:hh}
- Verify 12-hour format with leading zero (e.g., "03")
- Enter formula: {report:created:HH}
- Verify 24-hour format (e.g., "15")
- Enter formula: {report:created:A}
- Verify AM/PM displays correctly
- Enter formula: Report for {report:startdate:MMMM jS, yyyy}
- Verify displays as "Report for January 8th, 2025"
- Enter formula: {report:startdate:dddd, MMMM dd yyyy} at {report:created:hh}:{report:created:mm} {report:created:a}
- Verify displays as "Wednesday, January 08 2025 at 03:30 pm"
- Test with dates on the 1st, 2nd, 3rd, 11th, 12th, 13th, 21st, 22nd, 23rd, 31st
- Verify ordinal suffixes are correct (st, nd, rd, th)
- Test with different months to verify month names
- Test at midnight to verify 12-hour format shows "12" not "0"
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Mac Safari Demo
MacOS: Desktop