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

[HOLD for payment 2024-10-17] [$250] Next Steps - "Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions #47271

Closed
1 of 6 tasks
isagoico opened this issue Aug 12, 2024 · 58 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff

Comments

@isagoico
Copy link

isagoico commented Aug 12, 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!


Reproducible in staging?: Yes
Reproducible in production?: Yes

If this was caught during regression testing, add the test name, ID and link from TestRail: https://github.com/Expensify/Expensify/issues/417883 - Being added here

Email or phone of affected tester (no customers): applausetester+ig8@applause.expensifail.com

Logs: https://stackoverflow.com/c/expensify/questions/4856
Issue reported by: Applause - Internal team

Action Performed:

Preconditions:

  • Workspace approval & payments are enabled
  • Bank account setup has a "Unfinished" setup and bank account is pending validation.
  • Employee has submitted a report > the report is approved and is "awaiting payment"
  1. Navigate to the workspace chat
  2. Click on the report that is awaiting payment
  3. Check the next steps banner

Expected Result:

The next steps banner indicates "Waiting for userAdmin to finish setting up a business bank account" when a report is pending payment and the admin has not finished setting up the business bank account in the workspace.

Actual Result:

"Waiting for [userAdmin] to pay expense(s)." is displayed.

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

image

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~018c20eb05dc94ed77
  • Upwork Job ID: 1823304297972705293
  • Last Price Increase: 2024-08-13
Issue OwnerCurrent Issue Owner: @dylanexpensify
@isagoico isagoico added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 12, 2024
Copy link

melvin-bot bot commented Aug 12, 2024

Current assignee @dylanexpensify is eligible for the Bug assigner, not assigning anyone new.

@Nodebrute
Copy link
Contributor

Proposal

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

"Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions

What is the root cause of that problem?

Currently, we don't have any logic to determine if the owner is setting up a bank account. We simply display the following message: Waiting for [userAdmin] to pay expense(s).

optimisticNextStep = {
type,
icon: CONST.NEXT_STEP.ICONS.HOURGLASS,
message: [
{
text: 'Waiting for ',
},
{
text: reimburserDisplayName,
type: 'strong',
},
{
text: ' to ',
},
{
text: 'pay',
},
{
text: ' %expenses.',
},
],
};

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

Add another check in following case to show this message "Waiting for userAdmin to finish setting up a business bank account"

case CONST.REPORT.STATUS_NUM.APPROVED:

We can use the check reimbursementAccount?.achData?.state === 'SETUP' to determine if the admin is in the process of setting up the bank account. Based on this check, we can then display the appropriate message of our choice.
(Pseudocode)

   if(reimbursementAccount?.achData?.state === 'SETUP'){
                optimisticNextStep = {
                    type,
                    icon: icon of our choice,
                    message: [
                        {
                            text: 'Waiting for ',
                        },
                        {
                            text: reimburserDisplayName,
                            type: 'strong',
                        },
............

We can select the appropriate state based on when we want the message to appear.

What alternative solutions did you explore? (Optional)

@dylanexpensify
Copy link
Contributor

Reviewing today!

@dylanexpensify dylanexpensify added the External Added to denote the issue can be worked on by a contributor label Aug 13, 2024
@melvin-bot melvin-bot bot changed the title Next Steps - "Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions [$250] Next Steps - "Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions Aug 13, 2024
Copy link

melvin-bot bot commented Aug 13, 2024

Job added to Upwork: https://www.upwork.com/jobs/~018c20eb05dc94ed77

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

melvin-bot bot commented Aug 13, 2024

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

@c3024
Copy link
Contributor

c3024 commented Aug 13, 2024

We can use the check reimbursementAccount?.achData?.state === 'SETUP'

When the account adding process has not been started, there is no field of achData in reimbursementAccount. Could you clarify all the conditions in which we will show this message of Waiting for userAdmin to finish setting up a business bank account.

@dylanexpensify
Copy link
Contributor

@c3024 sure! We will only show this when an admin payee has payments enabled, report needing to be reimbursed, and their bank account needing to be verified.

@dangrous
Copy link
Contributor

Is this issue just in the optimistic (front end) next step? Or is the one coming from the server also incorrect?

@dylanexpensify
Copy link
Contributor

@c3024 can you confirm? cc @isagoico

@c3024
Copy link
Contributor

c3024 commented Aug 14, 2024

It seems that we are adding the message that appears here only optimistically.

When a user submits a request or an approver approves it, the message in nextStep is hardcoded until the next actor (approver or payer) acts (either approves or pays).

For example, if we change the nextStep message to Waiting for admin to finish setting up a bank account for cases where an account setup is pending, the following sequence would occur:

  1. A user submits a request.
  2. The approver approves the expense.
  3. The admin payer sees Waiting for admin to finish setting up a bank account.
  4. The admin sets up their bank account.

Even after the bank account is set up, the message doesn't update because the message in nextStep only changes when the next user (in this case, the admin) takes action (e.g., paying the expense).

To address this, we may need to change the logic for displaying the message so that it depends solely on the account setup state if we want to show a different message when the account is not set up.

However, in my opinion, it might not be worth the effort to fix this bug. It may be better to leave the message as Waiting for the admin to pay expenses and not make any changes.

@dangrous
Copy link
Contributor

So does the backend sending out waiting for admin to finish setting up a bank account not update it for the report? I don't think we need to handle it optimistically necessarily, but if it's also not updating when the backend does, we should look into it.

@dangrous
Copy link
Contributor

Sounds like we're probably going to close this out as won't fix but confirming a couple things internally

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

@dangrous we're going to keep open based on this internal thread. Can we prioritize getting this sussed out so we can get the regression tests posted and wrap up sent? 🙏

@dangrous
Copy link
Contributor

oh whoops I got confused. I can update the backend to send "Waiting for x to pay expenses." To clarify, should we not be sending that in any situation? Or just in this particular one?

Copy link

melvin-bot bot commented Aug 19, 2024

@dylanexpensify, @c3024 Huh... This is 4 days overdue. Who can take care of this?

@dangrous dangrous self-assigned this Aug 20, 2024
@melvin-bot melvin-bot bot removed the Overdue label Aug 20, 2024
@dangrous dangrous 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 labels Aug 20, 2024
@dangrous
Copy link
Contributor

Making this internal for now, as I'm updating the backend, but we will need a front end update for the optimistic next step - making it say "waiting for admin to finish setting up a bank account" when we are waiting for that.

@dangrous
Copy link
Contributor

PR is up for back end, still confirming whether or not it's the best fix in that Slack thread but hopefully should be in review soon

@dangrous
Copy link
Contributor

backend is merged, not yet deployed. Once deployed, @c3024, are you interested in figuring out that front end logic? Or would you prefer we wait for contributor proposals

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 2, 2024
@dangrous
Copy link
Contributor

dangrous commented Oct 2, 2024

Assigned @DylanDylann to the PR for review and here as well!

@DylanDylann
Copy link
Contributor

Sure

@dylanexpensify
Copy link
Contributor

Mind giving us an update @DylanDylann 🙇‍♂️

@DylanDylann
Copy link
Contributor

Waiting for @dangrous to give the final review on PR

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 10, 2024
@melvin-bot melvin-bot bot changed the title [$250] Next Steps - "Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions [HOLD for payment 2024-10-17] [$250] Next Steps - "Waiting for userAdmin to finish setting up a business bank account" is not displayed with the correct conditions Oct 10, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Oct 10, 2024
Copy link

melvin-bot bot commented Oct 10, 2024

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

Copy link

melvin-bot bot commented Oct 10, 2024

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

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

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

  • @c3024 requires payment (Needs manual offer from BZ)
  • @DylanDylann requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Oct 10, 2024

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:

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

@dylanexpensify
Copy link
Contributor

Tomorrow!

@DylanDylann
Copy link
Contributor

DylanDylann commented Oct 17, 2024

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:

[@DylanDylann] The PR that introduced the bug has been identified. Link to the PR: New requirement, not a bug
[@DylanDylann] 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: NA
[@DylanDylann] 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: NA
[@DylanDylann] Determine if we should create a regression test for this bug. Yes
[@DylanDylann] 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.

Regression Test Proposal

  1. [User A] Create a policy with approvals enabled and approver as User A. Do not connect any account to this policy.
  2. [User B] Request an expense in the workspace chat.
  3. [User A] Approve the expense.
  4. [User A] Go to the expense report, if not opened already.
  5. [User A] Verify that the message below the header says "Waiting for an admin to finish setting up a bank account".

Do we agree 👍 or 👎

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 17, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

Payment Summary

Upwork Job

  • ROLE: @c3024 paid $(AMOUNT) via Upwork (LINK)
  • ROLE: @DylanDylann paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@dylanexpensify)

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

@dylanexpensify
Copy link
Contributor

@c3024 @DylanDylann sent offers - please accept!

@c3024
Copy link
Contributor

c3024 commented Oct 17, 2024

Accepted, thanks!

@dylanexpensify
Copy link
Contributor

@DylanDylann please accept!

@dylanexpensify dylanexpensify removed their assignment Oct 18, 2024
@dylanexpensify dylanexpensify added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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

@dylanexpensify
Copy link
Contributor

Hey @adelekennedy! I'm heading out on parental leave so reassigning this! All that's left is paying out @DylanDylann once they accept their offer. Job post found here! TY! 🙇‍♂️

@adelekennedy
Copy link

Payment made!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Internal Requires API changes or must be handled by Expensify staff
Projects
Status: Done
Development

No branches or pull requests

8 participants