[$500] Expense - Green dot disappears from LHN when paying one out of many unpaid reports

lanitochka17 commented 9 months 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: 1.4.27-1 Reproducible in staging?: Y Reproducible in production?: Y 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: Applause - Internal Team Slack conversation:

Action Performed:

Precondition: There is a Collect workspace on Old Dot with admin and employee pre-steps to have multiple reports for testing -

Go to the workspace settings > More features > Enable Workflows
Go to the Workflows tab in the LHN
Enable Delay submissions
Keep the default workflow of Submit and Close

[Employee] Create a manual request in the workspace chat and submit it
[Employee] Create another manual request and submit it
{Admin] Go to workspace chat with employee
[Admin] Pay the second submitted report (submitted in Step 2)
Note that the LHN does not show green dot
[Admin] Click on the preview of first submitted report (Step 1) and return to the main chat

Expected Result:

In Step 4, green dot should still appear for the workspace chat in LHN because there is still unpaid report

Actual Result:

In Step 4, LHN for the workspace chat with unpaid report does not show green dot after paying one of the reports. It only shows green dot when revisiting main chat from the expense report

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/9fa5d1b7-103b-457a-8c67-b280b0ddeba8

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01fffa54d6d963be62
Upwork Job ID: 1748072099859210240
Last Price Increase: 2024-01-18

Issue Owner

Current Issue Owner: @roryabraham

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

melvin-bot[bot] commented 9 months ago

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

lanitochka17 commented 9 months ago

#wave6-collect-submitters

tienifr commented 9 months ago

Proposal

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

In Step 4, LHN for the workspace chat with unpaid report does not show green dot after paying one of the reports. It only shows green dot when revisiting main chat from the expense report.

What is the root cause of that problem?

We're setting the hasOutstandingChildRequest to false immediately here when paying money request, without checking whether there're still more outstanding child requests that are unpaid.

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

Check that there're no other outstanding child requests first before setting hasOutstandingChildRequest to false in optimistic data above.

There're a couple of ways to do this:

- Fix the back-end to return hasOutstandingChildRequest: true properly for the unpaid IOU report (currently it's returning hasOutstandingChildRequest: false even though those IOU reports are unpaid)
- Then in here, check the list of IOU report that corresponds to the report preview report actions in the chat report, and only set hasOutstandingChildRequest to false if no other IOU reports have hasOutstandingChildRequest: true
Or we can use conditions similar to this to check if the REPORTPREVIEW needs to be paid (instead of using the hasOutstandingChildRequest on the IOU report)

What alternative solutions did you explore? (Optional)

We can add a back-end field outstandingChildRequestCount that is the total count of outstanding child requests (similar idea to the childVisibleActionCount of the reportAction). When we pay the request, in optimistic data we reduce the outstandingChildRequestCount by 1, and only set hasOutstandingChildRequest to false if after paying this request, the outstandingChildRequestCount becomes 0.

allroundexperts commented 9 months ago

I think Option 2 from @tienifr's proposal should work fine and be inline with the code that we are already using.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @roryabraham, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

peterdbarkerUK commented 9 months ago

Confirming priority in slack

mountiny commented 9 months ago

@roryabraham I believe to implement this properly, we would need to create App version of this Auth method and make sure we use this in backend when the report is reimbursed to update the GBR correctly.

roryabraham commented 9 months ago

I believe to implement this properly, we would need to create App version of this Auth method and make sure we use this in backend when the report is reimbursed to update the GBR correctly.

we do have ReportUtils::requiresAttentionFromCurrentUser which seems about the same – I wish these didn't have to be re-implemented and we could just have some shared logic in a C++ function that we could use in both places, but that's probably out-of-scope for this issue...

Fix the back-end to return hasOutstandingChildRequest: true properly for the unpaid IOU report (currently it's returning hasOutstandingChildRequest: false even though those IOU reports are unpaid)

This sounds like it needs to be fixed, regardless. I think I prefer @tienifr's solution #1. I'm happy to assign him, but he'll be blocked on the back-end change.

Now I'm starting to think more seriously about creating some shared C++ libs for non-trivial business logic like this that's very input/output oriented and needs to be kept in sync across layers. But as that would be a new pattern, we need to discuss it further.

Sorry I don't have a more decisive conclusion here just yet – it's very late for me so I want to look at this one more time with fresher eyes.

melvin-bot[bot] commented 9 months ago

@peterdbarkerUK, @allroundexperts, @roryabraham Eep! 4 days overdue now. Issues have feelings too...

allroundexperts commented 9 months ago

Hi @roryabraham! Just following up on above. Did you get a chance to look at this again with a fresh pair of eyes?