[Payment due @brunovjk] Allow several date formats for CSV company cards#91933
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
melvin-bot
Bot
requested review from
brunovjk and
trjExpensify
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcec5e1ec3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Essential bug fix for CSV company card import rollout. 👍
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return format(date, CONST.DATE.FNS_FORMAT_STRING); | ||
| } | ||
| // Also try format parsing on the shortened input | ||
| for (const dateFormat of CSV_DATE_FORMATS) { |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The for-loop that iterates over CSV_DATE_FORMATS and calls parse/isValid/format is duplicated (lines 39-43 and lines 54-58). Since this file is being newly created as a shared utility, this is a good opportunity to extract the repeated pattern into a small helper.
Suggested fix -- extract a helper function:
function tryParseWithFormats(dateString: string): string | null {
for (const dateFormat of CSV_DATE_FORMATS) {
const parsedDate = parse(dateString, dateFormat, new Date());
if (isValid(parsedDate)) {
return format(parsedDate, CONST.DATE.FNS_FORMAT_STRING);
}
}
return null;
}Then call tryParseWithFormats(trimmedInput) and tryParseWithFormats(shortInput) instead of repeating the loop.
Reviewed at: bcec5e1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
NAB: Do you think it's worth making a change here in this PR, or perhaps in a follow-up?
cc: @Gonals
There was a problem hiding this comment.
This is fine. It is just a handful of lines 🤷
| // Excel thinks that 1900 was a leap year and adds an extra day to account for that | ||
| if (/^\d+$/.test(trimmedInput)) { | ||
| const inputInt = parseInt(trimmedInput, 10); | ||
| if (inputInt > 0 && inputInt < 100000) { |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The magic number 100000 is used as an upper bound for valid Excel serial date numbers without being defined as a named constant. Since this file is a new shared utility, consider extracting it to improve readability.
Suggested fix:
// Excel serial date 100000 corresponds to ~2173-10-14, well beyond any reasonable CSV date
const MAX_EXCEL_SERIAL_DATE = 100000;Then use inputInt < MAX_EXCEL_SERIAL_DATE in the condition.
Reviewed at: bcec5e1 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
NAB: Do you think it's worth making a change here in this PR, or perhaps in a follow-up?
Same here @Gonals
There was a problem hiding this comment.
Same. I think this is fine 🤷
|
@Gonals, the card appears after a while, but the 3 transactions don't show up: Screen.Recording.2026-05-29.at.10.18.10.movIs this correct? I can't access the issue. |
|
Friendly bump @Gonals. Thanks |
Transactions should only display for assigned cards :) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp91933_ios_native.moviOS: mWeb Safari91933_ios_web.movMacOS: Chrome / Safari91933_web_chrome.mov |
melvin-bot
Bot
changed the title
|
🎯 @brunovjk, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
@Gonals lint is failing |
All good now! |
Co-authored-by: Lucien Akchoté <lucien@expensify.com>
|
All yours @lakchote |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/lakchote in version: 9.4.18-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — docs update required ✅This PR changes user-facing import behavior: company card CSV/spreadsheet imports now accept the Date column in several common formats (year-first, US month-first, European day-first, month-name, and Excel serial date numbers), normalizing them to The existing help article Import Company Card Transactions From a Spreadsheet lists Date as a required column but didn't document any accepted formats, so I added an FAQ entry covering the supported formats. Draft help site PR: #94376 What I checked
@Gonals, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
6 participants
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/641979
PROPOSAL:
Tests
Go to a workspace > Company Cards > Add Cards > Import transactions from File. Use this file:
Expensify upload (2).csv
Follow the import process
Confirm the card displays properly. It may take a bit for it to show.
Offline tests
None
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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