Refactor updateWorkspaceAvatar, deleteWorkspaceAvatar, & updateGeneralSettings to pass policy data#83458
Conversation
melvin-bot
bot
requested review from
JmillsExpensify and
parasharrajat
and removed request for
a team
|
@parasharrajat 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] |
| value: { | ||
| errorFields, | ||
| name: policy.name, | ||
| outputCurrency: policy.outputCurrency, |
There was a problem hiding this comment.
We don't revert the name/currency, so I did it here.
| @@ -1779,13 +1770,10 @@ function updateGeneralSettings(policyID: string | undefined, name: string, curre | |||
| currency, | |||
| }; | |||
| finallyRates[rateID] = { | |||
There was a problem hiding this comment.
We use merge, so we don't need the whole object here
| failureRates[rateID] = { | ||
| ...currentRates[rateID], | ||
| pendingFields: {currency: null}, | ||
| currency: currentRates[rateID].currency, |
There was a problem hiding this comment.
This reverts the currency too when fail
| }; | ||
| finallyRates[rateID] = { | ||
| ...currentRates[rateID], | ||
| pendingFields: {currency: null}, |
There was a problem hiding this comment.
This applies the new currency even when fail, so I removed it
Code Review - PR #83458Bug: Missed call siteFile: This file imports updatePolicyGeneralSettings(policy.id, policy.name, CONST.CURRENCY.USD);After this PR's refactoring, Fix: Change line 98 to: updatePolicyGeneralSettings(policy, policy.name, CONST.CURRENCY.USD);Note: TypeScript should catch this since the type changed, but wanted to flag it explicitly. Other observations (non-blocking)
|
![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: 211fae2ad8
ℹ️ 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".
| if (!policyID || !policy.avatarURL || !policy.originalFileName) { | ||
| return; |
There was a problem hiding this comment.
onImageRemoved now returns unless policy.originalFileName is truthy, but Policy.originalFileName is optional and some existing policies can have an avatarURL without that field populated. In that state, tapping remove silently does nothing because deleteWorkspaceAvatar() is never called, leaving the workspace avatar undeletable for affected users.
Useful? React with 👍 / 👎.
| onSelect={(option: CurrencyListItem) => { | ||
| if (option.currencyCode !== paymentCurrency) { | ||
| updateGeneralSettings(personalPolicyID, personalPolicy?.name ?? '', option.currencyCode); | ||
| updateGeneralSettings(personalPolicy, personalPolicy?.name ?? '', option.currencyCode); |
There was a problem hiding this comment.
This page now calls updateGeneralSettings(personalPolicy, ...), but updateGeneralSettings() bails out when policy?.id is missing. Because the currency list is rendered immediately (no loading guard), a user can select a currency while usePolicy(personalPolicyID) is still undefined, get navigated back, and never send an update request. The previous personalPolicyID call path did not introduce this silent first-load no-op.
Useful? React with 👍 / 👎.
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
2 participants
Explanation of Change
Fixed Issues
$ #66574
PROPOSAL:
Tests
Same as QA Steps
Offline tests
Same as QA Steps
QA Steps
Prerequisite: have a workspace
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
web.mp4