Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Always include required fields (with empty string if not present) so Auth validation can catch them | ||
| // For non-required fields, only include if they have a value | ||
| if (requiredSignerFields.has(fieldName) || reimbursementAccountDraft?.[fieldName]) { | ||
| signerDetails[fieldName] = SafeString(reimbursementAccountDraft?.[fieldName]); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The SafeString() function is applied to required fields even when they might be undefined, but there is no documentation explaining what SafeString(undefined) returns. This could lead to unexpected behavior if SafeString(undefined) does not return an empty string as intended.
Suggested fix: Add a comment explaining the behavior, or make the intent explicit by using a ternary operator to return an empty string when the field is not present.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
|
||
| // These fields are required by the backend and must always be included in the request | ||
| // so that proper validation errors can be returned if they're missing | ||
| const requiredSignerFields = new Set<string>([FULL_NAME, DATE_OF_BIRTH, JOB_TITLE]); |
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
When isUserBeneficialOwner is true, the code explicitly sets FULL_NAME, DATE_OF_BIRTH, and ADDRESS to empty strings (line 50-52 in the original file), but JOB_TITLE is now always required via requiredSignerFields. This may be inconsistent with the intended behavior.
Consider whether JOB_TITLE should be conditionally required based on isUserBeneficialOwner, or if the current behavior is correct. If JOB_TITLE should always be required regardless of beneficial owner status, consider adding it to the explicit initialization block for consistency.
Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![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: 6131c9f7fc
ℹ️ 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".
| // Always include required fields (with empty string if not present) so Auth validation can catch them | ||
| // For non-required fields, only include if they have a value | ||
| if (requiredSignerFields.has(fieldName) || reimbursementAccountDraft?.[fieldName]) { | ||
| signerDetails[fieldName] = SafeString(reimbursementAccountDraft?.[fieldName]); |
There was a problem hiding this comment.
When DOWNLOADED_PDS_AND_FSG is present, this now runs through SafeString, which converts booleans to strings (e.g., 'true'). The API/type definitions expect a boolean for this field (see ReimbursementAccount and EnterSignerInfo flows), so sending a string can cause backend validation to reject the payload or mis-handle the value. This regression only appears for this field because it’s the lone boolean in signerDetailsFields.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
This PR fixes an issue where required signer fields (
signerFullName,signerDateOfBirth,signerJobTitle) were being silently omitted from the API request payload when not present in the form draft. This caused the Auth backend to receive incomplete data and fail with cryptic validation errors like "Invalid inputs" instead of properly identifying which required fields were missing.Fix:
requiredSignerFieldsconstant defining which fields must always be sent to the backendSafeString()to ensure consistent string handlingFixed Issues
$ https://github.com/Expensify/Expensify/issues/593959
PROPOSAL: N/A
Tests
signerDateOfBirth,signerFullName, andsignerJobTitleare present in the payloadOffline tests
N/A - This change affects form submission, which requires network connectivity
QA Steps
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
N/A - Backend/utility change only, no UI changes
Android: mWeb Chrome
N/A - Backend/utility change only, no UI changes
iOS: Native
N/A - Backend/utility change only, no UI changes
iOS: mWeb Safari
N/A - Backend/utility change only, no UI changes
MacOS: Chrome / Safari
N/A - Backend/utility change only, no UI changes