Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Held requests] [$250] Duplicate expense report displayed when employee submit expense #47539

Closed
3 of 6 tasks
IuliiaHerets opened this issue Aug 16, 2024 · 53 comments
Closed
3 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Reviewing Has a PR in review

Comments

@IuliiaHerets
Copy link

IuliiaHerets commented Aug 16, 2024

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.21-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4863120&group_by=cases:section_id&group_order=asc&group_id=309128
Email or phone of affected tester (no customers): gocemate+workspaceemployee159@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  1. Login as the owner of the workspace
  2. Create a workspace
  3. Invite the approver and employee
  4. Navigate to more features
  5. Enable "workflows"
  6. On the "Workflow" editor - enable "Add Approvals"
  7. Set the Approver account as the Approver

Steps:

  1. As employee submit 2 expenses to the workspace chat
  2. As Approver Hold one of the expenses and partially approve the report
  3. As employee submit another expense to the workspace chat

Expected Result:

There should be only one new expense present on workspace chat, along with expense that was put on hold

Actual Result:

Duplicate expense report displayed when employee submit another expense while there is a expense with hold status
Workspace avatar turn to offline image

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6573373_1723759627357.Recording__3736.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01da56a170fe42f065
  • Upwork Job ID: 1824490735723577485
  • Last Price Increase: 2024-09-04
Issue OwnerCurrent Issue Owner: @
@IuliiaHerets IuliiaHerets added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@IuliiaHerets
Copy link
Author

We think that this bug might be related to #wave-collect - Release 1

@IuliiaHerets
Copy link
Author

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Aug 16, 2024
@melvin-bot melvin-bot bot changed the title Hold - Duplicate expense report displayed when employee submit expense [$250] Hold - Duplicate expense report displayed when employee submit expense Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 16, 2024
Copy link

melvin-bot bot commented Aug 16, 2024

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

@excile1
Copy link

excile1 commented Aug 17, 2024

Proposal

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

After holding one of the submitted expenses and partially approving it, submitting a subsequent expense creates a duplicate report in the chat.

What is the root cause of that problem?

The root cause of this problem, unfortunately, seems to be the data sent from the API through Pusher. I explored the request that is being sent from the application, and it seems to be correct. The API response, though, includes both the correct data (the data that was submitted to the API) and also seems to include some extra "ghost" reportAction_{chatRoomID} and report_{chatRoomID} rows, that don't include the iouReportId and say that the workspace owes €0.00. And then this broken extra data gets pushed to the employee's side through Pusher, which is what causes the bug, because the subsequently submitted expenses can't link to the previous one without iouReportId.

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

It is possible to fix it through a workaround, like forcing it to ReportActions.openReport (or any other means to force it to reload the latest data), but it looks like the fix for the root cause must come from the API side.

What alternative solutions did you explore? (Optional)

(this is not an alternative solution, but just another thing I had to fix before being able to exactly reproduce this issue)
When I tried to reproduce the bug first, I noticed that the behavior was a bit different from the one in the video. For some reason, on the build I compiled from the latest version on this repo, this very same issue (extra report appearing in chat) appears even when expenses are supposed to be grouped, without holding them or doing anything extra. For whatever reason, this doesn't seem to happen on the staging build, but I'll include the fix I had to make in here in case the Contributor+ or other Contributors run into the same issue.

This seems to be caused by the canAddOrDeleteTransactions function. Currently, if the report is an expense report and the current user account isn't a manager (which an employee in a workspace isn't), then it returns false and causes any new expenses to duplicate on the employee's side. It can be fixed by altering this if statement here:

if (isExpenseReport(moneyRequestReport) && currentUserAccountID !== moneyRequestReport?.managerID) {

Copy link

melvin-bot bot commented Aug 19, 2024

@sakluger, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Aug 19, 2024
@aimane-chnaif
Copy link
Contributor

@excile1 thanks for the proposal. We're not looking for workaround but fix the root cause.

@melvin-bot melvin-bot bot removed the Overdue label Aug 19, 2024
@excile1
Copy link

excile1 commented Aug 20, 2024

@excile1 thanks for the proposal. We're not looking for workaround but fix the root cause.

Wouldn't the root cause have to be fixed on the backend side though?

So, in the approveMoneyRequest the optimisticData generated in the frontend looks good (with the correct owed amount in report preview and also iouReportID in the report)
image

image

The problem seems to appear in the request response's onyxData first, where it returns both the correct and expected response, but also returns the incorrect owes €0.00 report preview (which is the one that gets sent to the other user, which is why the issue seems to appear)
image
image

@aimane-chnaif
Copy link
Contributor

Agree this is backend bug.
🎀👀🎀

Copy link

melvin-bot bot commented Aug 21, 2024

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

@Gonals Gonals added Internal Requires API changes or must be handled by Expensify staff and removed External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Aug 22, 2024
@melvin-bot melvin-bot bot added the Overdue label Aug 23, 2024
@sakluger
Copy link
Contributor

@Gonals can you check and confirm that this is a BE bug?

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Aug 23, 2024
@trjExpensify
Copy link
Contributor

CC: @robertjchen for eyes.

@Gonals Gonals added the Reviewing Has a PR in review label Sep 24, 2024
@trjExpensify
Copy link
Contributor

Encouraging news!

@sakluger
Copy link
Contributor

Looks like the PR is merged and deployed! Hopefully it fixes the bug.

@sakluger
Copy link
Contributor

@IuliiaHerets would you mind giving this one a retest?

@aimane-chnaif do you think that the work you did reviewing proposals on this one was enough to warrant payment? Thanks!

@sakluger sakluger added the retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause label Sep 27, 2024
@IuliiaHerets
Copy link
Author

@sakluger This issue is not repro anymore, build - v9.0.41-2

Recording.4192.mp4

@aimane-chnaif
Copy link
Contributor

I am fine with no payment here.
Let's close

@robertjchen
Copy link
Contributor

Thank you! 🙇

@robertjchen
Copy link
Contributor

Apparently still reproducible: https://expensify.slack.com/archives/C036QM0SLJK/p1727771195051329?thread_ts=1727720815.151679&cid=C036QM0SLJK

@sakluger
Copy link
Contributor

sakluger commented Oct 1, 2024

@Gonals any ideas what might still be causing this?

@trjExpensify trjExpensify moved this from Done to Release 2: Summer 2024 (Aug) in [#whatsnext] #wave-collect Oct 1, 2024
@Gonals
Copy link
Contributor

Gonals commented Oct 2, 2024

Dammit -.-

Well, I used my ideas in the previous fix. I'll try to reproduce in dev again (which I have not been able to so far). With that, I should be able to figure out what's up.

@robertjchen robertjchen changed the title [$250] [Held requests] - Duplicate expense report displayed when employee submit expense [$250] Duplicate expense report displayed when employee submit expense Oct 2, 2024
@trjExpensify trjExpensify changed the title [$250] Duplicate expense report displayed when employee submit expense [Held requests] [$250] Duplicate expense report displayed when employee submit expense Oct 2, 2024
@Gonals
Copy link
Contributor

Gonals commented Oct 2, 2024

Can reproduce on Staging. Still not in Dev, which makes it difficult to debug. I have a couple of ides that may help. I'll keep you posted

@Gonals
Copy link
Contributor

Gonals commented Oct 7, 2024

Ok. This is getting overwritten by a "newer" Onyx Update, which wrongly sets the iouReportID to the oldOne. I'm digging out the origin.

@Gonals
Copy link
Contributor

Gonals commented Oct 9, 2024

Still chasing this. It is NOT coming from this flow, but from a concurrent event, so it has become a way harder thing to chase.
I think I have it surrounded, though

@Gonals
Copy link
Contributor

Gonals commented Oct 9, 2024

Finally got a solution!
Buuuut... it has unearthed another (existing) bug. I'll create an issue for it.

@Gonals
Copy link
Contributor

Gonals commented Oct 9, 2024

New issue: https://github.com/Expensify/Auth/pull/12721

Fix for it already in review

@mvtglobally
Copy link

Issue not reproducible during KI retests. (First week)

@sakluger
Copy link
Contributor

@Gonals is this fixed now? If so, did any of your PRs have C+ reviews that require payment?

@Gonals
Copy link
Contributor

Gonals commented Oct 17, 2024

Yep! Should be fixed! No C+ reviews

@Gonals Gonals closed this as completed Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Help Wanted Apply this label when an issue is open to proposals by contributors Internal Requires API changes or must be handled by Expensify staff retest-weekly Apply this label if you want this issue tested on a Weekly basis by Applause Reviewing Has a PR in review
Projects
No open projects
Status: Polish
Development

No branches or pull requests

10 participants