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
| // 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
|
@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. |
|
@Gonals one potential concern: I’m not 100% sure this can happen in our exact CSV import flow, so could you please confirm whether this is a real risk here? Thank you very much. |
3 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