[HOLD for payment 2024-09-10] [$250] iOS - Scan - Unable to delete receipt

[mvtglobally]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v9.0.16-5
Reproducible in staging?: Y
Reproducible in production?:
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):applausetester+kh040803@applause.expensifail.com
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause- Internal team
Slack conversation:

Action Performed:

1. Launch New Expensify app.
2. Go to workspace chat.
3. Submit an expense with a receipt.
4. Go to transaction thread.
5. Tap on the receipt.
6. Tap 3-dot menu.
7. Tap Delete receipt.

Expected Result:

Delete receipt modal will appear.

Actual Result:

Delete receipt modal does not appear. Unable to delete receipt.

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

Add any screenshot/video evidence

Bug6562931_1722908349762.ScreenRecording_08-06-2024_09-28-34_1.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01d0e9a861da3dc3f7
• Upwork Job ID: 1823033534353897628
• Last Price Increase: 2024-08-12

[melvin-bot]

Triggered auto assignment to @greg-schroeder (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[mvtglobally]

We this this might be related to #wave-control

[bernhardoj]

Proposal

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

Can't delete receipt because the delete confirmation modal doesn't show.

What is the root cause of that problem?

This happens after #45762 where we call the onSelected callback of popover immediately. The onSelected of the delete receipt option will show another modal and without waiting for the previous modal to hide (which we did before the PR), the second modal won't show.

App/src/components/AttachmentModal.tsx

Lines 425 to 433 in f7e265a

	if (TransactionUtils.hasReceipt(transaction) && !TransactionUtils.isReceiptBeingScanned(transaction) && canEditReceipt && !TransactionUtils.hasMissingSmartscanFields(transaction)) {
	menuItems.push({
	icon: Expensicons.Trashcan,
	text: translate('receipt.deleteReceipt'),
	onSelected: () => {
	setIsDeleteReceiptConfirmModalVisible(true);
	},
	});
	}

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

We have been fixing this issue by using Modal.close to wait for the modal to completely close, however, in our case, calling Modal.close will close the transaction receipt modal too. Instead of using Modal.close one by one, I propose to take a different approach to fix #45417.

Before the PR, the popover item onSelected is called when the modal is completely closed and this will stay as the default behavior. To fix #45417, we can have a new prop for the menu items to tell whether we want to wait for the modal to hide or not, we can call it selectOnModalHide.

Here is the detail:

1. Revert fix(PopoverMenu): fix opening external links #45762 and any changes that add Modal.close to fix the regression that comes after the PR.
2. In selectItem, if selectOnModalHide is false, then call onSelected immediately, otherwise, call it when the modal hides.
   

   App/src/components/PopoverMenu.tsx

   Lines 126 to 129 in f7e265a

   	} else {
   	onItemSelected(selectedItem, index);
   	selectedItem.onSelected?.();
   	}

if (selectedItem.selectOnModalHide !== false) {
    selectedItemIndex.current = index;
} else {
    selectedItem.onSelected?.();
}

3. Then, we can pass selectOnModalHide as false to any popover menu that we want to select the item immediately.
   Usage example:

menuItems.push({
    icon: Expensicons.Link,
    text: 'Open Link',
    selectOnModalHide: false,
    onSelected: () => {
        Link.openLink(getQuickBooksOnlineSetupLink('DF6835925B6E1C85'), CONST.NEW_EXPENSIFY_URL);
    },
});

[arosiclair]

@zfurtak can you take a look at this? It looks like it's caused by our changes in #45762

[zfurtak]

hello 😊
Yesterday I was looking into this case, but haven't found a suitable solution yet, as this exact screen has crazy re-render logic with 3 modals at the same moment. Still thinking... 🤔
I'm not a big fan of reverting changes, as the previous solution was a bit of a workaround. However it might be a good idea to add a flag with a default value and only change it where it's needed. This will make it easier for contributors to work with PopoverMenu in the future.
I will continue my investigation today 👍

[melvin-bot]

@greg-schroeder Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[greg-schroeder]

@zfurtak @arosiclair do you guys think this should be sent to External or will it be taken care of as a sort of regression?

[arosiclair]

Yeah this is external. Assigning @zfurtak

[melvin-bot]

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

[melvin-bot]

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

[zfurtak]

hello 😊
I managed to find a solution for this problem without reverting previous changes and in the same time being easier to maintain in the future 🚀

My idea is to add flag to every item that needs calling after the modal is hidden and for this specific case (deleting receipt) I had to changed code in Modal component as the logic was adjusted to a fix from the future and was causing a problem here.
I created a PR, which is yet in draft as I need one internal review to push it further 🤓

[arosiclair]

@fedirjh @arosiclair Could you mark #47341 as ready for review? 🙏 I'm going to take this over from @zfurtak because she is OOO but I'm not the author and can't mark it 😓

Done

[melvin-bot]

@arosiclair, @greg-schroeder, @fedirjh, @zfurtak Whoops! This issue is 2 days overdue. Let's get this updated quick!

[fedirjh]

PR in review.

[melvin-bot]

@arosiclair @greg-schroeder @fedirjh @zfurtak 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!

[arosiclair]

In review

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[greg-schroeder]

Just tracking for record keeping purposes - original PR was reverted.

New PR is in review

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[zfurtak]

The issue is fixed by this PR.

[greg-schroeder]

Thanks @zfurtak

[greg-schroeder]

Regression period 2024-09-10

[greg-schroeder]

Will prep this for Tuesday

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @fedirjh owed $125 via NewDot
• Contributor: @zfurtak is from an agency-contributor and not due payment

BugZero Checklist (@greg-schroeder)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1823033534353897628/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[greg-schroeder]

@fedirjh I believe you're paid via NewDot now, right? If so, you can make a manual request for $125 for the C+ role. $250 original price x 50% given the regression/revert.

[garrettmknight]

$125 approved for @fedirjh