[HOLD for payment 2024-11-14] [$250] Track expense - Add receipt modal is missing on confirmation page when sharing with accountant

[IuliiaHerets]

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: 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): [email protected]
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

• Account has no workspace.

1. Go to staging.new.expensify.com
2. Go to self DM.
3. Create a track manual expense.
4. Click Share it with my accountant.
5. Note that it shows Add receipt modal on the confirmation page.
6. Close the RHP.
7. 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:

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

Screenshots/Videos

Bug6624486_1728052918727.20241004_224100.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021851283420523124529
• Upwork Job ID: 1851283420523124529
• Last Price Increase: 2024-10-29

Issue Owner

Current Issue Owner: @eVoloshchak

[melvin-bot]

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

[melvin-bot]

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]

👋 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:

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

[NikkiWines]

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]

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

App/src/components/MoneyRequestConfirmationList.tsx

Line 1042 in 98da3ce

prevProps.shouldDisplayReceipt === nextProps.shouldDisplayReceipt,

Screen.Recording.2024-10-04.at.23.50.48.mov

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

App/src/components/MoneyRequestConfirmationList.tsx

Line 1042 in 98da3ce

prevProps.shouldDisplayReceipt === nextProps.shouldDisplayReceipt,

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2024-10-04.at.23.57.14.mov

[NikkiWines]

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]

@NikkiWines the #49142 was deployed to production 4 days ago

[NikkiWines]

yes sorry linked the wrong PR my bad

[nkdengineer]

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]

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 #49248. Reverted the changes locally and the receipt view is consistently there.

cc: @daledah @roryabraham @blazejkustra @mkhutornyi

cc: @jasperhuangg as you're deployer

[jasperhuangg]

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

Screen.Recording.2024-10-04.at.12.19.58.PM.mov

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

[MelvinBot]

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

[NikkiWines]

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 #50266

Screen.Recording.2024-10-04.at.15.27.37.mov

[NikkiWines]

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.

[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 9.0.58-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: app crash when opening split view #51244

If no regressions arise, payment will be issued on 2024-11-14. 🎊

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

• @eVoloshchak requires payment through NewDot Manual Requests
• @daledah requires payment (Needs manual offer from BZ)

[melvin-bot]

@eVoloshchak @mallenexpensify The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @eVoloshchak owed $250 via NewDot
• ROLE: @daledah paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

• 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/1851283420523124529/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[mallenexpensify]

@eVoloshchak @NikkiWines , are payments due here? I reviewed the issue, there's a lot going on. Looks like it might be related to a revert, many PRs are linked above.

[mallenexpensify]

@eVoloshchak 👀 plz

[eVoloshchak]

@mallenexpensify, the original PR was reverted, so the follow-up PR was needed.

• This is ready for payment
• Not sure if BZ checklist is applicable here, since the original issue (that has caused this bug) is a simple migration

[mallenexpensify]

Thanks @eVoloshchak , one last thing before paying.. what's the reason that the two folks who worked on the offending PR didn't fix this? The OG issues was $75 and the fix for the PR from that is $250, trying to reconcile and see if any process or docs need to be updated. Thx

[melvin-bot]

@eVoloshchak, @NikkiWines, @mallenexpensify, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[mallenexpensify]

@eVoloshchak 👀 on the above plz
#50246 (comment)

[eVoloshchak]

what's the reason that the two folks who worked on the offending PR didn't fix this?

@mallenexpensify,
@daledah was the author of both the original PR and the follow-up one. Not sure why I was assigned as C+ for the review of the follow-up PR, and not the original C+

• (this is a mistake on my part, I could have checked that this is a follow-up and hence should have the original C+ before reviewing).
• OR. We can add it to the processes that when we find out that that particular issue is a regression caused by a PR, C+ from that PR is automagically assigned to the issue

The fix is outside the 14 days regression period from when the original PR was deployed to production, so no idea on how @daledah's and mine payments should be handled

[mallenexpensify]

It looks like this bug was reported on Oct 4, the same day the offending PR hit production.

Am I missing something?

[eVoloshchak]

You're right @mallenexpensify, I was looking at the PR creation day. In that case, this should have just been handled in #49109

[mallenexpensify]

Thx @eVoloshchak . I'm asking all the questions to see if there's a process we can update cuz we don't want this happening too often, there'll always be a gap between a bug being reported and the offending PR being found though.

Since we didn't catch it here, the default is for the full amounts to be paid.

@daledah @eVoloshchak do either of you (or anyone else) have recommendation on how we might be able to avoid similar situations as this in the future? Thx

[mallenexpensify]

Contributor: @daledah paid $250 via Upwork
Contributor+: @eVoloshchak due $250 via NewDot

[melvin-bot]

@eVoloshchak, @NikkiWines, @mallenexpensify, @daledah Eep! 4 days overdue now. Issues have feelings too...

[eVoloshchak]

do either of you (or anyone else) have recommendation on how we might be able to avoid similar situations as this in the future? Thx

@mallenexpensify, the only one that comes to mind is

We can add it to the processes that when we find out that that particular issue is a regression caused by a PR, C+ from that PR is automatically assigned to the issue

[mallenexpensify]

Thx @eVoloshchak , I just checked the C+ process doc and we're covered there

• When possible, it’s best practice to assign the new issue to those assigned to the original GH.
  • If the C+ assigned to the original issue/PR is able to take over assignment of the new one, the default is that compensation is not due for the C+ who was initially assigned to the new issue.
• If the original C+ is not able to take over assignment (ie. if the bug is determined to be time-sensitive), the new C+ should review the PR to fix the regression
  • For these instances, the default compensation will be $250

[JmillsExpensify]

$250 approved for @eVoloshchak