Account for null values in prepareRequestPayload#87259
Account for null values in prepareRequestPayload#87259NikkiWines wants to merge 6 commits intomainfrom
Conversation
|
@sobitneupane 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] |
![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: b845b821fd
ℹ️ 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".
| const value = data[key]; | ||
|
|
||
| if (value === undefined) { | ||
| if (value === undefined || value === null) { |
There was a problem hiding this comment.
This new value === null filter drops explicit nulls from all requests, but some commands rely on sending a nullable field to clear server-side state. For example, SET_POLICY_TAG_APPROVER intentionally sends approver: null when unselecting an approver (src/libs/actions/Policy/Tag.ts:1283-1289), and the param is defined as required-but-nullable (src/libs/API/parameters/SetPolicyTagApproverParams.ts:1-5). After this change, that key is omitted entirely, so the backend cannot distinguish “clear this value” from “no update,” which can leave stale data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤔 I'm pretty sure this behavior is broken for that flow (and others) then, since we're not passing null but rather the stringified version of it
There was a problem hiding this comment.
Looking at the web code, we'd just end up throwing an error here the value doesn't match the expected format 😄
There was a problem hiding this comment.
And, actually, with this fix the backend will check Request::getString('approver') which will return '' and then createOrUpdateApprovalRule receives '', which will return false in strlen and give us the behavior we actually want - so this fixes that bug too :D
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
yay finally - @sobitneupane this is ready for a review 🙇 |
1 participant
Explanation of Change
We were coercing
nullvalues to"null"Fixed Issues
$ https://github.com/Expensify/Expensify/issues/616693
PROPOSAL: N/A internal issue
Tests
before change:
also verified locally that the
'<REDACTED>'value was in fact nullafter changes:
No malformed request 🎊
Offline tests
N/A
QA Steps
SigninUserattemptMalformed REQUEST: 'authToken', ignoring (should match ...warning (like this one)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