fix: clearing date input while company card assignment doesn't show field required error #76889
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
126
to
133
| onInputChange={(value) => { | ||
| setErrorText(''); | ||
| if (!value) { | ||
| setErrorText(translate('common.error.fieldRequired')); | ||
| } else { | ||
| setErrorText(''); | ||
| } | ||
| setStartDate(value); | ||
| }} |
Contributor
There was a problem hiding this comment.
Issue: Inconsistent validation logic with submit function
There's a logical inconsistency between this validation and the submit function:
- Line 53 uses
isRequiredFulfilled(startDate)to validate - Line 127 uses
!valueto validate
The isRequiredFulfilled function likely checks for more than just falsy values (e.g., it may trim whitespace). This means:
- A user could enter whitespace-only input and not see an error in the input handler
- But they would get an error on submit
Recommendation:
Suggested change
| onInputChange={(value) => { | |
| setErrorText(''); | |
| if (!value) { | |
| setErrorText(translate('common.error.fieldRequired')); | |
| } else { | |
| setErrorText(''); | |
| } | |
| setStartDate(value); | |
| }} | |
| onInputChange={(value) => { | |
| if (!isRequiredFulfilled(value)) { | |
| setErrorText(translate('common.error.fieldRequired')); | |
| } else { | |
| setErrorText(''); | |
| } | |
| setStartDate(value); | |
| }} |
This ensures validation is consistent throughout the component.
Contributor
There was a problem hiding this comment.
Let's address this as I also mentioned it in my review
Comment on lines
127
to
128
| if (!value) { | ||
| setErrorText(translate('common.error.fieldRequired')); |
Contributor
There was a problem hiding this comment.
Potential issue: Error shows only when custom option is selected
The validation at line 127 will show the error immediately when the input is cleared. However, this only matters when dateOptionSelected === CONST.COMPANY_CARD.TRANSACTION_START_DATE_OPTIONS.CUSTOM (line 120) since the DatePicker is conditionally rendered.
Current behavior is correct, but consider: if a user clears the date, switches to "From Beginning", then switches back to "Custom", the error will still be displayed (due to errorText state persisting) even though startDate now has a default value (line 49).
Recommendation: Clear the error in handleSelectDateOption when custom option is selected:
const handleSelectDateOption = (dateOption: string) => {
setErrorText('');
setDateOptionSelected(dateOption);
if (dateOption === CONST.COMPANY_CARD.TRANSACTION_START_DATE_OPTIONS.FROM_BEGINNING) {
return;
}
setStartDate(format(new Date(), CONST.DATE.FNS_FORMAT_STRING));
};Actually, looking at line 44, this is already handled! This is good. ✓
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where pressing the cross/clear button on the date input field in the company card assignment flow did not display a "field required" error message when the field became empty.
Key Changes:
- Added validation logic in the
onInputChangecallback of the DatePicker component to check if the value is empty and set the appropriate error message
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
samranahm
changed the title
Contributor
Author
|
@hungvu193 All yours. |
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeScreen.Recording.2025-12-07.at.21.28.13.moviOS: HybridAppScreen.Recording.2025-12-07.at.21.02.28.moviOS: mWeb SafariScreen.Recording.2025-12-07.at.21.04.39.movMacOS: Chrome / SafariScreen.Recording.2025-12-07.at.12.15.35.mov |
pecanoro
approved these changes
Dec 8, 2025
Contributor
|
🚀 Deployed to staging by https://github.com/pecanoro in version: 9.2.74-0 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation of Change
On input change check if there is no value and display field required error
Fixed Issues
$ #76108
PROPOSAL: #76108 (comment)
Tests
Pre-condition: Have a direct card feed added as company card
Offline tests
Same as Tests
QA Steps
Same as Tests
// TODO: These must be filled out, or the issue title must include "[No QA]."
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.Native.mp4
Android: mWeb Chrome
Android.mWeb.chrome.mp4
iOS: Native
IOS.Native.mp4
iOS: mWeb Safari
IOS.mWeb.safari.mp4
MacOS: Chrome / Safari
macOS.Chrome.mp4