[HOLD for payment 2024-02-07] [$500] Search Page opens in Background when delete Reciept Confirm Modal is visible

[m-natarajan]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.3.83-5
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @ishpaul777
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1697143133874499

Action Performed:

1. Navigate to Distance Request money and request money from Location "X" to Location "Y" in a workspace.
2. Navigation to Money Request Details and Open the Map Image
3. Click on Threedot and Click Delete
4. Navigate to search page using CMD+K

Expected Result:

Attachment modal is also closed.

Actual Result:

Attachment modal remains open and Search Page is open in background.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

Screen.Recording.2023-10-13.at.2.05.30.AM.mov

delete.search.mp4

MacOS: Desktop

Screen.Recording.2023-10-13.at.2.18.06.AM.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01f56f13a4f213744d
• Upwork Job ID: 1712594130617856000
• Last Price Increase: 2023-11-02
• Automatic offers:
• aimane-chnaif | Reviewer | 28062789

[melvin-bot]

Triggered auto assignment to @trjExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01f56f13a4f213744d

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[zukilover]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search Page opens in Background when delete Reciept Confirm Modal is visible

What is the root cause of that problem?

When opening the search modal, closeConfirmModal is only closing the delete confirmation modal itself, and not reading from is isNavigating to trigger the removal of the attachment modal.

App/src/components/AttachmentModal.js

Lines 218 to 221 in cb4b8a0

	const closeConfirmModal = useCallback(() => {
	setIsAttachmentInvalid(false);
	setIsDeleteReceiptConfirmModalVisible(false);
	}, []);

What changes do you think we should make in order to solve the problem?

Dismiss attachment modal if it is navigating to /search (opening search modal), e.g.:

const closeConfirmModal = useCallback((isNavigating) => {
        setIsAttachmentInvalid(false);
        setIsDeleteReceiptConfirmModalVisible(false);
        if (isNavigating) {
            Navigation.dismissModal(props.report.reportID);
        }
    }, []);

Result:

modal.mp4

[tienifr]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search Page opens in Background when delete Reciept Confirm Modal is visible

What is the root cause of that problem?

When the modal is open, we store the onclose function, and clear it when the modal is close. But in this case, we have two modals are open at the same time, so when we open the second modal, onClose on first modal will be replaced by the one of second modal -> When we navigate to search page, just the onClose of second modal is executed

App/src/components/Modal/BaseModal.js

Lines 87 to 90 in be18d1e

	Modal.setCloseModal(onClose);
	} else if (wasVisible && !isVisible) {
	Modal.willAlertModalBecomeVisible(false);
	Modal.setCloseModal(null);

What changes do you think we should make in order to solve the problem?

As we can see there are more than 1 modal are open at the same time, that why we should store their onClose in the object (closeModals = {key1: func, key2: func,...}).
When the modal is mounted, we will assign the free key to identify this modal (the key that don't have value). If the modal is open, we will store the onClose to this key, and clear it when the modal is close

After users press CMD + K to open search page we will execute Modal.close()

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Line 201 in be18d1e

Modal.close(() => Navigation.navigate(ROUTES.SEARCH));

In Modal.close, we will execute all the onClose functions, then reset this object to empty ({})

[redpanda-bit]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Search page opens behind attachment modal. Leaving search page not usable unless the modal is manually closed.

What is the root cause of that problem?

This is a navigation error where the modal screen (ReportAttachments) remains mounted when navigating to the search page. No Navigation.dismissModal is called and due to this the search screen is pushed on top of the ReportAttachment modal.

What changes do you think we should make in order to solve the problem?

We need to dismiss both, confirmation and attachment modals, in order to accomplish this I propose to create a new function Navigation.dismissAllModals. This new function within Navigation.js would remove any modal screens from the navigation state.

// https://github.com/Expensify/App/blob/main/src/libs/actions/Modal.ts
function closeAll(callback) {
    Navigation.dismissAllModals();
    callback();
}

// https://github.com/Expensify/App/blob/main/src/libs/Navigation/AppNavigator/AuthScreens.js#L201
Modal.closeAll(() => {
    Navigation.navigate(ROUTES.SEARCH);
});

// https://github.com/Expensify/App/blob/main/src/libs/Navigation/Navigation.js
function dismissAllModals() {
    navigationRef.current.dispatch((state) => {
        let newState = {...state};
        newState.routes = removeModalScreens(newState.routes);
        newState.index = newState.routes.length - 1;
        const action = getActionFromState(newState, linkingConfig.config);
        return action;
    });
}

What alternative solutions did you explore? (Optional)

Please also consider this solution. Modal.closeAll can also only remove the 2 top most modals if the user experience is guaranteed to be 2 modals max.

function closeAll(callback: () => void) {
    close(() => {}); // 1 
    Navigation.dismissAllModals(); // 2
    callback();  // navigate
}

[ishpaul777]

I have proposed the exact same solution @tienifr in #27277 (comment) a while ago

[melvin-bot]

@trjExpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

If another similar instance of this problem was solved by #27639 previously, I'm curious if this is a regression then? CC: @bernhardoj @s77rt as you have experience with this :)

[s77rt]

Not a regression. The plan was that any given time there should be only one modal open. But now this does not seem to be the case any more.

[trjExpensify]

Cool, well I think this is pretty low value anyhow. The reality of anyone seriously clicking CMD+K to "search" while also in the process of of completing the delete receipt confirmation modal is slim to none:

1. Navigate to Distance Request money and request money from Location "X" to Location "Y" in a workspace.
2. Navigation to Money Request Details and Open the Map Image
3. Click on Threedot and Click Delete
   4 Navigate to search page using CMD+K

Given that, I'm inclined to close this issue as we have far more pressing issues in the repo. What's your thoughts?

[melvin-bot]

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

[aimane-chnaif]

I think this should be fixed. This bug itself is not critical but another weird bug (critical) happening after following repro step.

Report screen got frozen. After switching to another chat and come back, it's unfrozon but when click receipt image, got stuck again.

Screen.Recording.2023-10-19.at.5.28.08.PM.mov

[trjExpensify]

I don't think you can call that critical given the steps to get here are pretty farfetched to happen in reality. But fair enough, if the app is freezing as a result we can look at it.

[melvin-bot]

@trjExpensify, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[trjExpensify]

Proposals awaiting a review.

[melvin-bot]

@trjExpensify @aimane-chnaif this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

This issue has not been updated in over 15 days. @trjExpensify, @jasperhuangg, @aimane-chnaif, @tsa321 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[trjExpensify]

PR is in review. Can we get an update on progress please?

[jasperhuangg]

Looks like review has been progressing steadily in the last few days on #33203

[trjExpensify]

That hit staging yesterday! What are the next steps here?

[aimane-chnaif]

Waiting for production hit

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.33-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Fix cannot open search page by shortcut if delete receipt confirm modal is visible #33203

If no regressions arise, payment will be issued on 2024-02-07. 🎊

For reference, here are some details about the assignees on this issue:

• @aimane-chnaif requires payment automatic offer (Reviewer)
• @tsa321 requires payment (Needs manual offer from BZ)
• @ishpaul777 requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@aimane-chnaif] The PR that introduced the bug has been identified. Link to the PR:
• [@aimane-chnaif] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@aimane-chnaif] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@aimane-chnaif] Determine if we should create a regression test for this bug.
• [@aimane-chnaif] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@trjExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[aimane-chnaif]

This is old issue and not able to find offending PR. Since this was rare user case, hard to catch.

Regression Test Proposal

1. Create manual request with receipt.
2. Open transaction detail page and click receipt preview to open receipt modal.
3. Click three dots menu and click delete receipt to open delete confirm modal.
4. Press CMD + K to open Search page
5. Verify that both modals are closed and then Search page shows
6. Close Search page and repeat Step 3
7. Press ESC
8. Verify that only delete confirmation modal is closed and receipt modal keeps showing

[trjExpensify]

Agree it's edge case, and a regression test would be extraneous.

Payment summary as follows:

• @ishpaul777: $50 for the bug report (it was reported prior to the program change) - offer sent!
• @tsa321: $500 for the fix - offer sent!
• @aimane-chnaif: $500 for C+ - paid!

[tsa321]

@trjExpensify Thank you, I have accepted the offer...

[ishpaul777]

@trjExpensify Accepted offer! Thank you! 🙇‍♂️

[trjExpensify]

@ishpaul777 - paid
@tsa321 - paid

Closing, thanks everyone!