[HOLD for payment 2024-10-10] [Report next steps audit] Next step in report header changes after returning online

[lanitochka17]

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.19.2
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Issue found when executing PR #44940

Action Performed:

precondition: create a control workspace, set "Manually approve all expenses" over to $10 and set advanced approval in OD:
boss owner (admin) submits to mini owner
mini owner (admin) submits boss owner
manager (employee) submits to mini owner forwards to mini owner over limit forwards (limit 100) to boss owner
employee 1 (employee) submits to manager
employee 2(employee) submits to manager

1. Log in as an employee 1
2. Disable the internet connection
3. Submit an expense below $100
4. Return online and observe the next step in report header
5. Log in as an manager
6. Approve the report
7. Log in as mini owner
8. Disable the internet connection
9. Approve the report
10. Return online

Expected Result:

The same next step in report header displayed offline and online: for step 4 it is "Waiting for Manager approve expense(s)", and for step 10 it is "No further action required!"

Actual Result:

When user returns online, the next step in report header changes from "Waiting for Manager approve expense(s)" to "Waiting for employee 1 submit their expense (s)" for step 4 and from "Waiting for Boss owner pay expense(s) to "No further action required!" for step 4

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

Bug6570207_1723486899933.Recording__667.mp4

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @trjExpensify

[melvin-bot]

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

[lanitochka17]

@trjExpensify 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

[lanitochka17]

We think that this bug might be related to #wave-control

[trjExpensify]

Put on the radar of @dylanexpensify @dangrous and co here as it's in dev, checking if these are internal or not: https://expensify.slack.com/archives/C06ML6X0W9L/p1723559615613289?thread_ts=1723491174.174309&cid=C06ML6X0W9L

[dangrous]

Okay I just tried replicating this - I'm not getting the same notes - for the step 4 one, I get waiting for expenses to automatically submit later today which I think is correct, so I don't think there's an issue there?

For step 10, I'm getting "no further action required" from the backend so that should be updated.

[dangrous]

Huh this is much more complicated than I thought on the backend. I've made a draft PR but struggling with some pieces, I will come back to it later.

It sounds like from Slack that this is okay to punt a little, since we're not holding on it for the end of the project (seems like a bug that existed before).

[dangrous]

Progress is being made! The trick here is that I think the existing tests were incorrect. I've pushed a fix to the linked PR, and I think we should be moving shortly, assuming @mountiny agrees with the changes I've made (which honestly I'm not confident on)

[trjExpensify]

PR is on staging Melv.

[dangrous]

This is on prod so we should be able to test and close out!

[dangrous]

oh actually i lied - it looks like there was a front end piece that we might need (I wrote NOTE: The optimistic action in the App will say the workspace owner not the mini owner. That will be fixed. This is for the response that comes from the server.) in the PR. So I can look into that shortly, unfortunately there's another bug that's preventing easy testing, but a fix for that is up.

This will get done soon though!

[dangrous]

Hi it's me, back with another question about expected behavior @trjExpensify @mountiny.

So - the logic here all works as we want it to. EXCEPT - the front end is calculating a different person as the "reimburser" than the back end. I'd love to know which you think is correct (I can also bring this to Slack)

In the following situation:

• Alice creates a workspace, she is the owner.
• She creates (as per the testing steps):
  • Bob (admin) submits to Clarissa
  • Clarissa (admin) submits to Bob
  • Debbie (employee) submits to Clarissa, forwards to Clarissa, over limit forwards to Bob
  • Eduardo (employee) submits to Debbie
  • Frank (employee) submits to Debbie

Who is the reimburser that we want to show here? Any admin should be able to reimburse (is that correct?) so would we show Alice, Bob, or Clarissa?

Right now the front end looks for a reimburser for the policy and if one isn't set up defaults to the owner. The backend, essentially, looks for a reimburser for the report (as in, the person who has taken a reimbursement action), and if one isn't set up, defaults to the manager.

I think the backend behavior is incorrect (I used the report's reimburserEmail but that isn't correct unless a reimbursement has occurred), but before I fix it, I want to make sure we want Alice to be shown as the reimburser on the front end. Because the testing steps above aren't super clear about the "correct" behavior, but mention showing both Bob and Clarissa, but not Alice.

What name/user do we want shown here?

[trjExpensify]

Who is the reimburser that we want to show here? Any admin should be able to reimburse (is that correct?) so would we show Alice, Bob, or Clarissa?

Who's been set as the reimburser on the workspace in the settings?

[dangrous]

Where can I see that? I don't see a reimburser set in old dot

[trjExpensify]

You need to be on Direct reimbursement and add a VBBA to reveal that field.

[dangrous]

Interesting... so who should we show if the workspace is NOT on direct reimbursement and/or doesn't have a VBBA? Since that's the situation in the repro steps

[trjExpensify]

Do we still have a reimburser set despite not having direct reimbursement configured? I'm not familiar with that in the code, but any of the admins can manually mark the report as reimbursed, it's not waiting on an assigned reimburser in that sense.

[dangrous]

okay let me see what happens if an employee is submitting to another employee (not admin). I think the backend will still default to an admin, not necessarily the owner, but it sounds like that's correct here - and so we need to update the front end to match as well.

Will have to figure out how it picks between admins if there is more than one, so we can replicate that too...

[mountiny]

@dangrous When you set a reimburser and then you change the reimbursement choice, the reimburserEmail is not not cleared i believe. Its how it worked in oldDot so maybe its colliding with the updated next steps logic

[dangrous]

I think the trick with this particular set up is that no reimburser is set at all? What's the default value for the field if it's never touched?

[trjExpensify]

Looking at the report next steps doc where a bunch of these were consolidated, I bet it was one of these for indirect reimbursement:

[dangrous]

Got it, so basically we want both the front end and the back end to pick an admin - and it doesn't really matter what admin. However, right now they're picking different ones.

Probably the easiest solution would be to have the backend also use the policy owner, since that's always going to be the same. Is the owner always an admin? I think so... How does that sound? If good, I can adjust the fallback on the backend, and then we should (finally) be done with this!

[trjExpensify]

Yep, the owner is always an admin.

Why are we randomly picking a named admin instead of just something generic when there's no designated reimburser? Like we had before highlighted in the screenshot? 🤔

[dangrous]

oh, like say "an admin" in the message? I think we were trying to always name the people who needed to take the action - that's the pattern in the updated nextSteps (cc @dylanexpensify). I think it's safe here to always pick the workspace owner, who will be valid in all cases. I'll see if I can make that backend change

EDIT: Draft PR below, just waiting on confirmation that this is the behavior we want, before putting it into review. It's otherwise ready.

[trjExpensify]

I think we were trying to always name the people who needed to take the action

I get that, but in this case it could be any admin. Consider this..

• A company uses Expensify. They reimburse expenses with Payroll, so outside of Expensify.
• The CFO pays for the subscription to Expensify on their corporate card, so they're workspace and billing owner.
• Cathy, the finance manager, is responsible for the day-to-day managing of expenses and processing reimbursements.
• All expense reports on this this workspace will now say "waiting for CFO to pay!" which is incorrect, and a total guess on our part.

So I think in the case of indirect reimbursement, we don't guess and refer to the collective with "an admin".

[dangrous]

That makes sense to me! Do we need confirmation from anyone else who worked on the project, or should I go ahead with that?

[trjExpensify]

Let's see. @dylanexpensify has been pinged, I think @JmillsExpensify helped as well. Any thoughts, guys?

With the report next steps project we killed the generic "Waiting for an admin to pay" message, so as a result, it's a bit of a crab shoot who we name when no reimburser is set (i.e with indirect reimbursement).

I'm suggesting here we bring that generic one back for that case.

[trjExpensify]

What are the next steps here, @dangrous?

[dylanexpensify]

Chiming in I agree with bringing back the generic "an admin" phrase for indirect. cc @mountiny to confirm if this works ok with the migration?

[dangrous]

So I have https://github.com/Expensify/Web-Expensify/pull/43483/files up for the backend - and #49424 for the front end. If there is a reimburser explicitly set for the policy, it will use their info, if not it will just say "an admin".

[trjExpensify]

How's this looking in review?

[dangrous]

BE is done, App should be hopefully done today! Waiting on reviewer testing

[dangrous]

App PR is merged! Not yet deployed, but soon

[dangrous]

on staging...

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

• Update Report Next Steps to fallback to "an admin" instead of policy owner if no reimburser set #49424

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

[melvin-bot]

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:

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

[dangrous]

No PR introduced this bug, just logic / edge cases. I don't think we need a new regression test, so can probably close out? (I don't think we need any payment for anyone either.)