-
Notifications
You must be signed in to change notification settings - Fork 87
updateSnapshots - remove picking only existing props #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
updateSnapshots - remove picking only existing props #638
Conversation
|
Requesting review from @parasharrajat and @iwiznia |
|
@FitseTLT Could you please update the tests for snapshots? |
What do you mean? @parasharrajat |
|
I mean to ask for updating the unit tests around the snapshots as we changed the functionality and Onyx is core lib so it is important we keep full coverage. |
iwiznia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, but agree on adding tests
|
Added @parasharrajat |
parasharrajat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
|
|
||
| const initialValue = {name: 'Fluffy'}; | ||
| const finalValue = {name: 'Kitty'}; | ||
| const finalValue = {name: 'Kitty', nickName: 'Fitse'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get it, how is this change exercising the changed code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Earlier, if we couldn't add new keys(change the Object structure) to the existing values in snapshots in Onyx. But now we can do that. Here in this test, a new key is now added which confirms that change work and allows adding new keys to the existing items in snapshots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nickName is a new prop that didn't exist in intialValue. Evidence: if u test it on main it will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it! Thanks for clarifying
Details
When updating snapshots from BE response we used to only pick existing props in the snapshot from the update object data but there are cases where the BE can add new props that didn't exist in the snapshot data so in this PR we are removing the picking.
Related Issues
Expensify/App#60116
Automated Tests
Manual Tests
Go to node_modules react-native-onyx and update this line
react-native-onyx/lib/OnyxUtils.ts
Line 1422 in fe8af83
to
Precondition: a workspace has an employee with an expense without receipt
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)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
2025-05-27.00-20-02.mp4
Android: mWeb Chrome
2025-05-27.00-20-33.mp4
iOS: Native
2025-05-26.22-59-26.mp4
iOS: mWeb Safari
2025-05-26.23-01-47.mp4
MacOS: Chrome / Safari
2025-05-26.22-57-34.mp4
MacOS: Desktop
2025-05-26.22-58-36.mp4