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

[$250] [CVP] New User who is submitted an IOU pays via BBA, then see’s `Approve' #42322

Closed
1 of 6 tasks
m-natarajan opened this issue May 16, 2024 · 32 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented May 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: 1.4.74-4
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: @danielrvidal
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1715816298574129

Action Performed:

  1. As user A send a IOU request to new user
  2. Click "Pay" from the new user email
  3. User is taken to the IOU report
  4. Select the option "Pay with expensify"
  5. Select option "Business bank account"
  6. This will create a workspace and moves the IOU to a workspace chat
  7. Cancel the VBBA set up and navigate to the workspace chat

Expected Result:

The expense should see pay rather than approve.

Actual Result:

There is an option to approve the expense, even though it’s a brand new workspace and thus shouldn’t have approval.

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

upgradeerrors.mp4
Recording.79.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cc2fb178d0dea7a3
  • Upwork Job ID: 1792680301683040256
  • Last Price Increase: 2024-06-10
  • Automatic offers:
    • DylanDylann | Reviewer | 102721547
    • bernhardoj | Contributor | 102721548
Issue OwnerCurrent Issue Owner: @hoangzinh
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 16, 2024
Copy link

melvin-bot bot commented May 16, 2024

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

@bernhardoj
Copy link
Contributor

bernhardoj commented May 17, 2024

Proposal

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

The money request preview shows approve button even though it's a new workspace which has approval disabled by default.

What is the root cause of that problem?

This has the same root cause as #39171.

We already have a condition here to not show the approve button if the approval is disabled (isOnSubmitAndClosePolicy).

App/src/libs/actions/IOU.ts

Lines 5978 to 5982 in e8ae3c5

const isOnInstantSubmitPolicy = PolicyUtils.isInstantSubmitEnabled(policy);
const isOnSubmitAndClosePolicy = PolicyUtils.isSubmitAndClose(policy);
if (isOnInstantSubmitPolicy && isOnSubmitAndClosePolicy) {
return false;
}

However, the auto-reporting frequency needs to be instant (isOnInstantSubmitPolicy) to not show the approval button. In our case, the frequency is empty, so the condition is never met. Later after we receive the BE response, the frequency is set to instant.

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

Remove isOnInstantSubmitPolicy condition

Or change the condition to accept either isOnInstantSubmitPolicy or isOnSubmitAndClosePolicy.
if (isOnInstantSubmitPolicy || isOnSubmitAndClosePolicy) {

We need to fix on the BE too because the next step indicating for the admin to approve it first.

What alternative solutions did you explore? (Optional)

Set autoReportingFrequency to instant optimistically when creating the workspace in createWorkspaceFromIOUPayment.
autoReportingFrequency: CONST.POLICY.AUTO_REPORTING_FREQUENCIES.INSTANT,

@melvin-bot melvin-bot bot added the Overdue label May 20, 2024
Copy link

melvin-bot bot commented May 20, 2024

@kevinksullivan Whoops! This issue is 2 days overdue. Let's get this updated quick!

@kevinksullivan
Copy link
Contributor

This is bottom up case, so adding to collect project

@melvin-bot melvin-bot bot removed the Overdue label May 20, 2024
@kevinksullivan kevinksullivan changed the title New User who is submitted an IOU pays via BBA, then see’s `Approve' [CVP] New User who is submitted an IOU pays via BBA, then see’s `Approve' May 20, 2024
@kevinksullivan kevinksullivan added the External Added to denote the issue can be worked on by a contributor label May 20, 2024
@melvin-bot melvin-bot bot changed the title [CVP] New User who is submitted an IOU pays via BBA, then see’s `Approve' [$250] [CVP] New User who is submitted an IOU pays via BBA, then see’s `Approve' May 20, 2024
Copy link

melvin-bot bot commented May 20, 2024

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

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

melvin-bot bot commented May 20, 2024

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

@DylanDylann
Copy link
Contributor

DylanDylann commented May 22, 2024

This RCA and solution of this issue is similar to this one that I approved
@bernhardoj's proposal #39171 (comment). @kevinksullivan Do you think we should close this one and reopen the previous issue to save effort because both I and @bernhardoj have context about this problem?

@hoangzinh
Copy link
Contributor

Do you think we should close this one and reopen #39171

I don't think we should. The previous issue is closed because that issue is not reproducible, if we re-open that issue, the reproduce steps there can't be used, also expectation.

But I'm happy to let @DylanDylann continuous review this issue as a C+.

Copy link

melvin-bot bot commented May 27, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label May 27, 2024
Copy link

melvin-bot bot commented May 28, 2024

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

@hoangzinh
Copy link
Contributor

cc @kevinksullivan what do you think on my comment here #42322 (comment)

@melvin-bot melvin-bot bot removed the Overdue label May 29, 2024
Copy link

melvin-bot bot commented May 30, 2024

@hoangzinh @kevinksullivan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented Jun 3, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Jun 3, 2024
Copy link

melvin-bot bot commented Jun 3, 2024

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

@hoangzinh
Copy link
Contributor

@melvin-bot melvin-bot bot removed the Overdue label Jun 4, 2024
@kevinksullivan
Copy link
Contributor

I agree we should just keep up with the same issue. @hoangzinh if you want to pass to @DylanDylann you can, or you can stay on as C+.

@hoangzinh
Copy link
Contributor

Yes, please @kevinksullivan can you assign @DylanDylann as C+ so he can start reviewing this issue? Thanks

Copy link

melvin-bot bot commented Jun 10, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Jun 10, 2024

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

@DylanDylann
Copy link
Contributor

Taking over as C+

@DylanDylann
Copy link
Contributor

Hi @bernhardoj Could you reproduce this issue any mỏe?

@bernhardoj
Copy link
Contributor

bernhardoj commented Jun 11, 2024

Yes, I can still repro it.

Screen.Recording.2024-06-11.at.23.07.47.mov

Btw, if you notice in the video, the approve button is later changed to the pay button. That's because CreateWorkspaceFromIOUPayment API sets the auto-reporting frequency to instant which will make canApproveIOU returns false.

I mentioned before in my proposal that the auto reporting frequency is weekly, so looks like the BE sends a different response this time.
image

I have added an alternative to set the frequency to instant optimistically. It'd be easier to test this while offline.

@DylanDylann
Copy link
Contributor

Screenshot 2024-06-12 at 13 46 14

@bernhardoj A bit curious, when I log in as a new user, I only see an option for payment "pay .. elsewhere"

@bernhardoj
Copy link
Contributor

Ah, the pay with expensify is for USD only, the currency is always converted to my local currency, so I manually update the code to always show the pay with expensify option.

const canUseWallet = !isExpenseReport && !isInvoiceReport && currency === CONST.CURRENCY.USD;

@DylanDylann
Copy link
Contributor

@bernhardoj Your alternative proposal looks good to me. Let's go with @bernhardoj's proposal

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jun 12, 2024

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

@melvin-bot melvin-bot bot removed the Overdue label Jun 12, 2024
@amyevans
Copy link
Contributor

Agreed we can set it optimistically to take care of this, assigning @bernhardoj and @DylanDylann

@amyevans amyevans assigned bernhardoj and DylanDylann and unassigned hoangzinh Jun 13, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jun 13, 2024
Copy link

melvin-bot bot commented Jun 13, 2024

📣 @DylanDylann 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Jun 13, 2024

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@bernhardoj
Copy link
Contributor

This PR already set the autoReportingFrequency to instant, I guess nothing to do here then.

Copy link

melvin-bot bot commented Jun 13, 2024

@amyevans @kevinksullivan @bernhardoj @DylanDylann this issue is now 4 weeks old, please consider:

  • Finding a contributor to fix the bug
  • Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
  • If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

@amyevans
Copy link
Contributor

Okay got it, I think we should just close here then. Sorry this is the second time this has happened 😬

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 External Added to denote the issue can be worked on by a contributor
Projects
Archived in project
Development

No branches or pull requests

6 participants