Implement confirmation modal when user tries to create multiple empty reports#71609
Conversation
e552640 to
c500c8a
Compare
199c5b3 to
2f1d70b
Compare
2f1d70b to
0b909c6
Compare
|
@hungvu193 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] |
|
Will review on Monday |
There was a problem hiding this comment.
Did we confirm this translation elsewhere?
And I thought we should also generate other transaction files right? Not only es and en.
There was a problem hiding this comment.
I think the other translations can be generated by a workflow which can be started by an internal eng.
| if (isConfirmLoading) { | ||
| return; | ||
| } | ||
|
|
||
| setIsConfirmLoading(true); | ||
|
|
||
| const result = onConfirm(); | ||
|
|
||
| // If the result is not a Promise, convert it to one for consistent handling | ||
| const resultPromise = result instanceof Promise ? result : Promise.resolve(); | ||
|
|
||
| resultPromise | ||
| .catch(() => {}) | ||
| .finally(() => { | ||
| setIsConfirmLoading(false); | ||
| setIsVisible(false); |
There was a problem hiding this comment.
just wonder why we need this handle here? 🤔
AFAIK, after pressing Confirm, we navigate to the empty report immediately. Is there any case that user can press Cancel in that meantime?
For this line:
const resultPromise = result instanceof Promise ? result : Promise.resolve();
We can use onConfirm?.() no?
There was a problem hiding this comment.
Makes sense, updated accordingly.
| * This excludes reports that are being deleted or have errors. | ||
| */ | ||
| function hasEmptyReportsForPolicy(reports: OnyxCollection<Report>, policyID: string | undefined, accountID: number): boolean { | ||
| if (!policyID || !Number.isFinite(accountID)) { |
There was a problem hiding this comment.
I'm not sure if isFinite is the correct way. What if accountID equals CONST.DEFAULT_NUMBER_ID?
| if (!policyID || !Number.isFinite(accountID)) { | |
| if (!policyID || accountID === CONST.DEFAULT_NUMBER_ID) { |
| const inferredWorkspacePolicy = useMemo(() => { | ||
| if (activePolicy && activePolicy.isPolicyExpenseChatEnabled && isPaidGroupPolicy(activePolicy)) { | ||
| return activePolicy; | ||
| } | ||
|
|
||
| if (groupPoliciesWithChatEnabled.length === 1) { | ||
| return groupPoliciesWithChatEnabled.at(0); | ||
| } | ||
|
|
||
| return undefined; | ||
| }, [activePolicy, groupPoliciesWithChatEnabled]); | ||
|
|
||
| const inferredWorkspaceID = inferredWorkspacePolicy?.id; |
There was a problem hiding this comment.
it looks like we also repeat this block of code more than 1 time, can we create util funciton for it?
| const reports = toCollection(buildReport()); | ||
|
|
||
| expect(hasEmptyReportsForPolicy(reports, undefined, accountID)).toBe(false); | ||
| expect(hasEmptyReportsForPolicy(reports, policyID, Number.NaN)).toBe(false); |
There was a problem hiding this comment.
Let's update the test here to cover the case when accountID equals CONST.DEFAULT_NUMBER_ID
| clearLastSearchParams(); | ||
| } | ||
|
|
||
| const createdReportID = createNewReport(currentUserPersonalDetails, inferredWorkspaceID); |
There was a problem hiding this comment.
Let's fix the linting here
| return; | ||
| } | ||
|
|
||
| const createdReportID = createNewReport(currentUserPersonalDetails, inferredWorkspaceID); |
There was a problem hiding this comment.
Same here. it looks like we need to merge main, createNewReport has been updated elsewhere
|
@ShridharGoel It looks like I can still create empty report from Global create, can you double check? Screen.Recording.2025-10-07.at.15.05.39.mov |
…ionButtonAndPopover
|
@hungvu193 Looks like two extra params got added in the hasEmptyReportsForPolicy call from FloatingActionButtonAndPopover, maybe while resolving conflicts. I've updated it now. |
|
Cool. Ty. That was fast. Lemme retest. |
|
Lint is still failing |
|
@hungvu193 Can you check now? |
|
@puneetlath Please take a look. |
|
✋ 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/puneetlath in version: 9.2.33-0 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.2.33-4 🚀
|
Explanation of Change
Implement confirmation modal when user tries to create multiple empty reports.
Fixed Issues
$ #70999
PROPOSAL: #70999 (comment)
Tests
Offline tests
QA Steps
Same as tests.
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
Screen.Recording.2025-10-02.at.5.30.00.PM.mp4
Android: mWeb Chrome
Screen.Recording.2025-10-02.at.4.42.31.PM.mp4
iOS: Native
Simulator.Screen.Recording.-.iPhone.16.Plus.-.2025-10-02.at.22.49.25.-.FPS.-.Videobolt.net.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.16.Plus.-.2025-10-02.at.21.56.56.-.COMPRESS.mp4
MacOS: Chrome / Safari
Screen.Recording.2025-10-02.at.3.34.29.PM.1.online-video-cutter.com.1.mp4
MacOS: Desktop
Screen.Recording.2025-10-02.at.11.07.13.PM.mp4