Implement bulk change approver flow for Search page v2#77857
Implement bulk change approver flow for Search page v2#77857marcaaron merged 32 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 |
|
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. |
|
Sorry for the delay here. Please feel free to ping me next time to get my attention. |
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks great. Thanks for your patience on this one @QichenZhu
| } | ||
|
|
||
| const isApprovalEnabled = policy?.approvalMode && policy.approvalMode !== CONST.POLICY.APPROVAL_MODE.OPTIONAL; | ||
| if (report && policy && isPolicyAdmin(policy) && isExpenseReportUtil(report) && isProcessingReport(report) && !isMoneyRequestReportPendingDeletion(report) && isApprovalEnabled) { |
There was a problem hiding this comment.
NAB, why isExpenseReportUtil and not isExpenseReport 🤔
|
🚧 @marcaaron has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
|
Yes, help site changes are needed. The bulk "Change approver" feature added in this PR is not covered in the existing documentation. I've created a draft PR with the required updates: #87192 Changes made to 3 articles:
All UI labels match the exact strings in |
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.3.53-0 🚀
Bundle Size Analysis (Sentry): |
|
Deploy Blocker #87213 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.53-7 🚀
|
8 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