Track expense - Add receipt modal is missing on confirmation page when sharing with accountant

IuliiaHerets commented 2 weeks ago

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: 9.0.44-7 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5042074 Email or phone of affected tester (no customers): applausetester+kh0110008@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

Account has no workspace.

Go to staging.new.expensify.com
Go to self DM.
Create a track manual expense.
Click Share it with my accountant.
Note that it shows Add receipt modal on the confirmation page.
Close the RHP.
Click Share it with my accountant.

Expected Result:

Add receipt modal should appear on the confirmation page (production behavior).

Actual Result:

Add receipt modal is missing on the confirmation page when sharing with accountant.

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/9cdee5a6-924a-45d5-aa7a-946309976e62

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @mallenexpensify

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @NikkiWines (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @mallenexpensify (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.

github-actions[bot] commented 2 weeks ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

NikkiWines commented 2 weeks ago

Can reproduce locally ✅

Looks like PolicyUtils.isPaidGroupPolicy(policy) is intermittently returning true/false here which causes shouldShowReceiptEmptyState to switch between showing and not showing. Looking to see what might've changed with this logic recently (since that line was added 4mo ago). Maybe we're not passing the policy correctly 🤔

nkdengineer commented 2 weeks ago

Proposal

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

Add receipt modal is missing on the confirmation page when sharing with accountant.

What is the root cause of that problem?

For some reason when we click on Share it with my accountant. the second time, the policy draft is merged to Onyx after we open the MoneyRequestConfirmationList which causes the policyDraft is not updated because the memo function here doesn't include policyDraft. If we resize the window we can see the add receipt modal will appear

https://github.com/Expensify/App/blob/98da3ce3cfe7f430095f3b381099426f19cb708c/src/components/MoneyRequestConfirmationList.tsx#L1042

https://github.com/user-attachments/assets/1e947c31-2453-4932-a90e-e8b2b1dcf36d

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

We should add policyDraft to the compare in memo function but because we change the logic in this fine, the best solution is to migrate this component to use useOnyx and remove withOnyx

https://github.com/Expensify/App/blob/98da3ce3cfe7f430095f3b381099426f19cb708c/src/components/MoneyRequestConfirmationList.tsx#L1042

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/user-attachments/assets/c343dd64-9aeb-4651-b5b2-814acad52a0e

NikkiWines commented 2 weeks ago

Confirmed this isn't currently reproducible on prod (note: the testing steps need to be adjusted so that the user only has one workspace).

Also confirmed that policy is intermittently undefined, causing shouldShowReceiptEmptyState to be false on occasion.

Nodebrute commented 2 weeks ago

@NikkiWines the https://github.com/Expensify/App/pull/49142 was deployed to production 4 days ago

NikkiWines commented 2 weeks ago

yes sorry linked the wrong PR my bad

nkdengineer commented 2 weeks ago

Confirmed this isn't currently reproducible on prod (note: the testing steps need to be adjusted so that the user only has one workspace).

Also confirmed that policy is intermittently undefined, causing shouldShowReceiptEmptyState to be false on occasion.

@NikkiWines Yes, when I logged the policyID it's changed from -1 to draft policyID but the policyDraft is not updated

NikkiWines commented 2 weeks ago

Yeah, I noticed that in your proposal @nkdengineer - just as a note, technically this issue isn't external yet as we prefer to have these regressions fixed by the original PR authors.

I think it's actually a regression from https://github.com/Expensify/App/pull/49248. Reverted the changes locally and the receipt view is consistently there.

cc: @daledah @roryabraham @blazejkustra @mkhutornyi

cc: @jasperhuangg as you're deployer

jasperhuangg commented 2 weeks ago

Actually, I can't seem to reproduce this bug on staging anymore

https://github.com/user-attachments/assets/af97f16c-48ff-4f66-89fd-080115aae595

Gonna demote and add the Needs Repro label. It seems we aren't automatically selecting the workspace if you only have one.

MelvinBot commented 2 weeks ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

NikkiWines commented 2 weeks ago

I'm still able to repro on staging and main

staging: https://github.com/user-attachments/assets/6727fa55-ec16-4642-a292-aecaa6f9eefc

main: https://github.com/user-attachments/assets/8ea8aa14-8e9a-4551-b584-e56fcd9ecbf0

The issue is fixed by https://github.com/Expensify/App/pull/50266

https://github.com/user-attachments/assets/d49fa951-cb2d-41de-a7ed-ac7fdcc8b644

NikkiWines commented 2 weeks ago

Side note that this logic for the workspace selector might be too broad, as even if I only have one active policy (and several archived), I still see the workspace selector.

jasperhuangg commented 2 weeks ago

Cool, let's move forward with that revert, thanks for double-checking

jasperhuangg commented 2 weeks ago

We CP'd https://github.com/Expensify/App/pull/50266 which should fix the issue

jasperhuangg commented 2 weeks ago

https://github.com/user-attachments/assets/24b9485f-0e88-425a-a54e-bf2502fc7bb5

Confirmed fixed on staging

daledah commented 2 weeks ago

@NikkiWines @jasperhuangg I'll open a new PR soon.

daledah commented 2 weeks ago

@NikkiWines Here's the PR. Since this can be fixed by migrating withOnyx, please let me know if any other flow has the same bug so I can fix all at once in this PR.

NikkiWines commented 5 days ago

PR has been merged

melvin-bot[bot] commented 2 days ago

⚠️ 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.

daledah commented 1 day ago

@NikkiWines Looks like follow-up PR is reverted. I'll open a follow-up PR soon.

melvin-bot[bot] commented 1 day ago

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

melvin-bot[bot] commented 1 day ago

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

https://github.com/Expensify/App/pull/50327

If no regressions arise, payment will be issued on 2024-10-30. :confetti_ball:

melvin-bot[bot] commented 1 day ago

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:

[ ] [@NikkiWines] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@NikkiWines] 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:
[ ] [@NikkiWines] 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:
[ ] [@NikkiWines] Determine if we should create a regression test for this bug.
[ ] [@NikkiWines] 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.
[ ] [@mallenexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

NikkiWines commented 20 hours ago

The PR for this was reverted (see: https://github.com/Expensify/App/pull/51283) - a fix will need to be re-introduced.

Expensify / App