Implement bulk change approver flow for Search page v2#77857
Implement bulk change approver flow for Search page v2#77857QichenZhu wants to merge 30 commits intoExpensify:mainfrom
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@Krishna2323 I'm having trouble making the React Compiler check happy.
The error messages are quite technical to me and I can't tell if they mean a good thing or bad thing. The guide doesn't cover them.
However, the plugin shows the components are optimized by React Compiler successfully. What do I need to do to fix them? cc @marcaaron |
QichenZhu
changed the title
melvin-bot
bot
requested review from
Krishna2323 and
joekaufmanexpensify
and removed request for
a team
|
@Krishna2323 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] |
|
@Krishna2323 I still need to do more testing. Feel free to do a preliminary review if you want. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return null; | ||
| } | ||
|
|
||
| const {avatar} = personalDetails?.[accountID] ?? {}; |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders.
The allApprovers array is computed by calling getAllApprovers() on every render without memoization. This creates a new array reference each time, causing the ApproverSelectionList component to re-render unnecessarily even when the underlying data hasn't changed.
Suggested fix:
const allApprovers = useMemo(() => getAllApprovers(), [
selectedReports,
allPolicies,
allReports,
personalDetails,
formatPhoneNumber,
translate,
icons.FallbackAvatar,
selectedApproverEmail,
]);
|
|
||
| return isPolicyAdmin(policy) && isAllowedToApproveExpenseReport(report, currentUserDetails.accountID, policy); | ||
| }); | ||
|
|
There was a problem hiding this comment.
❌ PERF-4 (docs)
Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders.
The approverTypes array is computed by calling getApproverTypes() on every render without memoization. This creates a new array reference each time, causing the SelectionList component to re-render unnecessarily even when the underlying data hasn't changed.
Suggested fix:
const approverTypes = useMemo(() => getApproverTypes(), [
selectedReports,
allPolicies,
onyxReports,
currentUserDetails.accountID,
selectedApproverType,
translate,
]);
| if (!report) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
❌ PERF-4 (docs)
Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders.
The confirmButtonOptions object is created on every render without memoization. This creates a new object reference each time, causing the SelectionList component to re-render unnecessarily even when the underlying data hasn't changed.
Suggested fix:
const confirmButtonOptions = useMemo(() => ({
showButton: true,
text: translate('iou.changeApprover.title'),
onConfirm: changeApprover,
}), [translate, changeApprover]);Note: You'll also need to memoize the changeApprover function with useCallback.
| }); | ||
|
|
||
| if (shouldShowBypassApproversOption) { | ||
| data.push({ |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders.
The listHeader JSX element is created on every render without memoization. This creates a new element reference each time, causing the SelectionList component to re-render unnecessarily even when the underlying data hasn't changed.
Suggested fix:
const listHeader = useMemo(() => (
<>
<Text style={[styles.ph5, styles.mb5]}>
{translate(selectedReports.length === 1 ? 'iou.changeApprover.subtitle' : 'iou.changeApprover.bulkSubtitle')}
</Text>
{selectedPolicies.length === 1 && (
<View style={[styles.ph5, styles.mb5, styles.renderHTML, styles.flexRow]}>
<RenderHTML
html={translate('iou.changeApprover.description', {
workflowSettingLink: `${environmentURL}/${ROUTES.WORKSPACE_WORKFLOWS.getRoute(selectedPolicies.at(0)?.id)}`,
})}
/>
</View>
)}
</>
), [styles, translate, selectedReports.length, selectedPolicies, environmentURL]);
| }, [hasLoadedApp, onyxReports, selectedReports]); | ||
|
|
||
| const getSelectedPolicies = () => { | ||
| const policies = new Map<string, Policy>(); |
There was a problem hiding this comment.
❌ PERF-4 (docs)
Objects and functions passed as props should be properly memoized to prevent unnecessary re-renders.
The selectedPolicies array is computed by calling getSelectedPolicies() on every render without memoization. This creates a new array reference each time, affecting the memoization of listHeader and other dependent values.
Suggested fix:
const selectedPolicies = useMemo(() => getSelectedPolicies(), [selectedReports, allPolicies]);
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Whats next project 👍
|
@QichenZhu could you please check the |
|
@Krishna2323, yes, it is. |
|
Reviewing... |
|
@QichenZhu please link these regression in the OP. |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppTest 1: Bulk Add Approvertest_1.mp4Test 2: Bulk Bypass Approvertest_2.mp4Test 3: Account C Visibility Rulestest_3.mp4Test 4: Single Non-Upgraded (Non-Control) Policy Selectedtest_4_5.mp4Test 5: Two Non-Upgraded (Non-Control) Policies Selectedtest_4_5.mp4Test 6: Mixed State Skip Behaviortest_6.mp4Android: mWeb ChromeTest 1: Bulk Add Approvertest_1.mp4Test 2: Bulk Bypass Approvertest_2.mp4Test 3: Account C Visibility Rulestest_3.mp4Test 4: Single Non-Upgraded (Non-Control) Policy Selectedtest_4_5.mp4Test 5: Two Non-Upgraded (Non-Control) Policies Selectedtest_4_5.mp4Test 6: Mixed State Skip Behaviortest_6.mp4iOS: HybridAppTest 1: Bulk Add Approvertest_1.mp4Test 2: Bulk Bypass Approvertest_2.mp4Test 3: Account C Visibility Rulestest_3.mp4Test 4: Single Non-Upgraded (Non-Control) Policy Selectedtest_4_5.mp4Test 5: Two Non-Upgraded (Non-Control) Policies Selectedtest_4_5.mp4Test 6: Mixed State Skip Behaviortest_6.mp4iOS: mWeb SafariTest 1: Bulk Add Approvertest_1.mp4Test 2: Bulk Bypass Approvertest_2.mp4Test 3: Account C Visibility Rulestest_3.mp4Test 4: Single Non-Upgraded (Non-Control) Policy Selectedtest_4_5.mp4Test 5: Two Non-Upgraded (Non-Control) Policies Selectedtest_4_5.mp4Test 6: Mixed State Skip Behaviortest_6.mp4MacOS: Chrome / SafariTest 1: Bulk Add Approvertest_1.mp4Test 2: Bulk Bypass Approvertest_2.mp4Test 3: Account C Visibility Rulestest_3_ab.mp4Test 4: Single Non-Upgraded (Non-Control) Policy Selectedtest_4_5.mp4Test 5: Two Non-Upgraded (Non-Control) Policies Selectedtest_4_5.mp4Test 6: Mixed State Skip Behaviortest_6.mp4 |
Krishna2323
left a comment
Krishna2323
left a comment
There was a problem hiding this comment.
LGTM and works well! 🚀
@QichenZhu please resolve the conflicts when you get a chance. Thanks!
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
These changes are looking great! Minor comments and then just the conflicts to address. Feels super close! Thanks for your patience on this one @QichenZhu
src/libs/SearchUIUtils.ts
Outdated
| * Do not use directly, use only via `getSections()` facade. | ||
| */ | ||
| function getAction(allActions: SearchTransactionAction[]) { | ||
| // VIEW should take precedence over CHANGE_APPROVER |
There was a problem hiding this comment.
It's not entirely clear what this method is doing or why "change approver" has special significance here.
src/libs/SearchUIUtils.ts
Outdated
| * @private | ||
| * Returns the main action that can be taken on a given transaction or report | ||
| * | ||
| * Do not use directly, use only via `getSections()` facade. |
There was a problem hiding this comment.
I think this comment can probably be removed as it's pretty vague. If someone wanted to use this it would not make much difference.
src/libs/SearchUIUtils.ts
Outdated
|
|
||
| /** | ||
| * @private | ||
| * Returns the main action that can be taken on a given transaction or report |
There was a problem hiding this comment.
How are we defining "main" here? Can we say this another way? I think it would improve the comment a bit.
| ); | ||
| } | ||
|
|
||
| // This actually clears selected reports as well |
There was a problem hiding this comment.
| // This actually clears selected reports as well | |
| // Note: This clears both reports and transactions |
|
Sorry if I missed it, but did we want to address the AI review comments about memoization? Please confirm @QichenZhu @Krishna2323 Thanks! |
|
Thanks for your review! Comments addressed. |
React Compiler Marker shows they are auto-memorized. 
|
|
@marcaaron this is ready :) |
This comment was marked as outdated.
This comment was marked as outdated.
|
The new |
|
@marcaaron, this is ready for your review. |
5 participants
Explanation of Change
Fixed Issues
$ #75220
$ #77372
PROPOSAL:
Tests
Same as QA Steps.
Offline tests
N/A.
QA Steps
Preconditions
Test 1: Bulk Add Approver
Test 2: Bulk Bypass Approver
Test 3: Account C Visibility Rules
A. Multi-policy selection (C should NOT appear)
B. Single-policy selection (C may appear)
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
Test 6: Mixed State Skip Behavior
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
Test 1: Bulk Add Approver
android-native-1.webm
Test 2: Bulk Bypass Approver
android-native-2.webm
Test 3: Account C Visibility Rules
android-native-3.webm
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
android-native-4.webm
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
android-native-5.webm
Test 6: Mixed State Skip Behavior
android-native-6.webm
Android: mWeb Chrome
Test 1: Bulk Add Approver
android-web-1.webm
Test 2: Bulk Bypass Approver
android-web-2.webm
Test 3: Account C Visibility Rules
android-web-3.webm
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
android-web-4.webm
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
android-web-5.webm
Test 6: Mixed State Skip Behavior
android-web-6.webm
iOS: Native
Test 1: Bulk Add Approver
ios-native-1.mov
Test 2: Bulk Bypass Approver
ios-native-2.mov
Test 3: Account C Visibility Rules
ios-native-3.mov
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
ios-native-4.mov
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
ios-native-5.mov
Test 6: Mixed State Skip Behavior
ios-native-6.mov
iOS: mWeb Safari
Test 1: Bulk Add Approver
ios-web-1.mov
Test 2: Bulk Bypass Approver
ios-web-2.mov
Test 3: Account C Visibility Rules
ios-web-3.mov
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
ios-web-4.mov
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
ios-web-5.mov
Test 6: Mixed State Skip Behavior
ios-web-6.mov
MacOS: Chrome / Safari
Test 1: Bulk Add Approver
mac-web-1.mov
Test 2: Bulk Bypass Approver
mac-web-2.mov
Test 3: Account C Visibility Rules
mac-web-3.mov
Test 4: Single Non-Upgraded (Non-Control) Policy Selected
mac-web-4.mov
Test 5: Two Non-Upgraded (Non-Control) Policies Selected
mac-web-5.mov
Test 6: Mixed State Skip Behavior
mac-web-6.mov