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

HIGH [$750]: Clean up the payment options on Pay button in New Dot #36301

Closed
6 tasks done
MitchExpensify opened this issue Feb 11, 2024 · 160 comments · Fixed by #48639
Closed
6 tasks done

HIGH [$750]: Clean up the payment options on Pay button in New Dot #36301

MitchExpensify opened this issue Feb 11, 2024 · 160 comments · Fixed by #48639
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

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Feb 11, 2024

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: v1.4.39-7
Reproducible in staging?: Y
Reproducible in production?: Y
Expensify/Expensify Issue URL: #33967
Issue reported by: @anmurali
Slack conversation: NA -

Action Performed:

  1. Submit a request as User A to User B
  2. As User B click "Pay with Expensify" on the request (Do nothing)
  3. As User B click the “⌄” down chevron beside "Pay with Expensify" (Do nothing)

Expected Result:

The same options should be presented when clicking "Pay with Expensify" or the “⌄” down chevron beside it.

This is the new design we want to implement lifted from #33967:

Pull all the options into one single dropdown labeled Pay <currency><amount>, which when pressed opens for the user to choose:

Pay with business bank account
Pay with personal bank account
Pay with debit card
Pay elsewhere

  • Let's remember payment preference based on the first payment by request type. E.g. IOUs vs Expenses.

Said another way, if you paid an IOU using a debit card, then next time you pay an IOU, unless you pressed on the down caret (dropdown), we assume you mean the same payment device and process the payment with the debit card. But if you then pay an expense in a workspace, we don't assume but ask you to choose a payment method.

  • When defaulting payment method for expenses, use the workspace default if one is set in workspace settings.

If there is a default at the workspace level, and the admin has access to that, use that.

  • When defaulting payment method for expenses, make it workspace specific.

Said another way, if you pay an expense on workspace A, we default you to that payment device the next time (unless you press the dropdown). But then if you go to pay an expense on workspace B for the first time, we do not assume.

Actual Result:

Two different sets of options are presented to the user:

A:
image

B:
image

Workaround:

None

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~016cb0dade2c28979f
  • Upwork Job ID: 1758260187340271616
  • Last Price Increase: 2024-06-11
@MitchExpensify MitchExpensify added Daily KSv2 NewFeature Something to build that is a new item. Bug Something is broken. Auto assigns a BugZero manager. labels Feb 11, 2024
Copy link

melvin-bot bot commented Feb 11, 2024

Copy link

melvin-bot bot commented Feb 11, 2024

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 Weekly KSv2 labels Feb 11, 2024
@MitchExpensify
Copy link
Contributor Author

@greg-schroeder greg-schroeder removed their assignment Feb 13, 2024
@joekaufmanexpensify
Copy link
Contributor

Discussing

@MitchExpensify
Copy link
Contributor Author

Let's run with this issue!

@MitchExpensify
Copy link
Contributor Author

Let's hunt down proposals @joekaufmanexpensify 🦾

@joekaufmanexpensify joekaufmanexpensify added External Added to denote the issue can be worked on by a contributor and removed NewFeature Something to build that is a new item. labels Feb 14, 2024
Copy link

melvin-bot bot commented Feb 14, 2024

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

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

melvin-bot bot commented Feb 14, 2024

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

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 14, 2024
@joekaufmanexpensify joekaufmanexpensify added External Added to denote the issue can be worked on by a contributor 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 Feb 15, 2024
@melvin-bot melvin-bot bot changed the title Clean up the payment options on Pay button in New Dot [$500] Clean up the payment options on Pay button in New Dot Feb 15, 2024
Copy link

melvin-bot bot commented Feb 15, 2024

Job added to Upwork: https://www.upwork.com/jobs/~016cb0dade2c28979f

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

melvin-bot bot commented Feb 15, 2024

Current assignee @allroundexperts is eligible for the External assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Sep 4, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@bondydaa
Copy link
Contributor

bondydaa commented Sep 4, 2024

@brandonhenry @allroundexperts are you able to take a look at those reports above?

And we also have this one that popped up #48560 which I'm not sure if related or not... ideally I don't think we want/need to revert right now but if #37174 is the root cause maybe we do 🤷

@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 4, 2024

FWIW, I think the Test/QA steps in #37174 are super light given the changes here that span a lot further than just pay options for iouReports. I'm not surprised this keeps getting reverted.

Revisiting the OP to try and make sense of this one.

Pull all the options into one single dropdown labeled Pay , which when pressed opens for the user to choose:

Pay with business bank account
Pay with personal bank account
Pay with debit card
Pay elsewhere

I assume those are the pay options for iouReports only, but it still suggests the button should be a button displayed with a menu for Pay $amount v.

Then in the OP, there’s some additional logic changes for the stickiness..

  • Let’s remember payment preference based on the first payment by request type. E.g. IOUs vs Expenses.
    Said another way, if you paid an IOU using a debit card, then next time you pay an IOU, unless you pressed on the down caret (dropdown), we assume you mean the same payment device and process the payment with the debit card. But if you then pay an expense in a workspace, we don’t assume but ask you to choose a payment method.

This makes sense, as some pay button options on iouReports aren’t applicable or available on expenseReports on workspace expenses (i.e PBA or Debit Card). Your preferred payment method for each of those report types can conceivably be different.

Did this get tested and implemented in the PR, or is it outstanding?

But if you then pay an expense in a workspace, we don’t assume but ask you to choose a payment method

I think this last line in that bullet doesn't make sense though. It also contradicts the next bullet down, so let's move on to that bullet.

  • When defaulting payment method for expenses, use the workspace default if one is set in workspace settings.
    If there is a default at the workspace level, and the admin has access to that, use that.

This sounds like something we are doing already by using the VBBA set in the workspace settings when you’re the reimburser viewing an expenseReport that needs reimbursing. You can choose to Pay elsewhere to manually mark it as reimbursed, but you can’t choose a different bank account to pay the expense report?

  • When defaulting payment method for expenses, make it workspace specific.
    Said another way, if you pay an expense on workspace A, we default you to that payment device the next time (unless you press the dropdown). But then if you go to pay an expense on workspace B for the first time, we do not assume.

Same here, doesn't this come out of the box with setting a VBBA to reimburse from in the workspace settings?

Overall, it seems like this has got a bit wonky in implementation perhaps. I'm leaning towards reverting and revisiting this with a clearer picture of the scope of implementation.

@JmillsExpensify
Copy link

Agree with Tom

@brandonhenry
Copy link
Contributor

brandonhenry commented Sep 4, 2024

@trjExpensify I agree.. and I also brought up in the PR the many weird discrepancies..
I think this being my first ticket - I didn't have all the understanding of the system. Nevertheless, tried to work through it with what knowledge I had. Those are great callouts and I agree that the design on this was hard to follow..

@allroundexperts was reviewer here, do you have thoughts on how to proceed? I mentioned here during PR how I was confused on the number of options needed but you wanted us to align on making this look exactly like staging (prior to merge).

There's a very good chance our QA missed things here considering the wide encompassing nature, but my knowledge is limited and I don't think my QA along would have caught all the areas. That said, can we agree on a plan to get this implemented exactly how the design team would like, in all cases? Happy to open a new PR and own that, but would like further guidance on every workflow this touches

Cc. @JmillsExpensify @bondydaa @joekaufmanexpensify

@trjExpensify
Copy link
Contributor

I just came across another issue, we're failing to pay any expense reports in product. Investigation here.

@mountiny can you help us revert this PR and CP it?

@trjExpensify
Copy link
Contributor

do you have thoughts on how to proceed?

  1. Revert the necessary PR/PRs so we unbreak the product
  2. Take the high level design back to Slack and crystalise mockups for every flow and instance of the pay button and payment method options for:
    • iouReports
    • expenseReports
    • invoiceReports
    • Pay button behaviour inside each of those report types
    • Pay button behaviour on the REPORTPREVIEW of each of those report types
    • Updates to lastPaymentMethod "sticky" logic for each report type
  3. Agree on the detailed implementation and rollout plan to accommodate where all the changes need to be made on the FE and BE.
  4. Write manual tests for each scenario, to add those to the PR to be tested and assist during the development to check the intended behaviour
  5. Update this issue OP to reflect the finalised scope of work.
  6. Code it, review it, ship it.

@joekaufmanexpensify
Copy link
Contributor

Maybe a quick design doc could even make sense? The amount of discussion on this issue about implementation, and the slack threads linked out to already further discussing kinda shows this isn't really a straightforward change. It might benefit from making sure we are holistically considering all angles of the solution.

@trjExpensify
Copy link
Contributor

100%

@joekaufmanexpensify
Copy link
Contributor

Going to bring next steps to slack very soon.

@joekaufmanexpensify joekaufmanexpensify added Daily KSv2 and removed Weekly KSv2 Reviewing Has a PR in review labels Sep 9, 2024
@joekaufmanexpensify
Copy link
Contributor

Discussing

@joekaufmanexpensify
Copy link
Contributor

Okay, we discussed this internally and here's where we landed. We're going to:

  • Pay out the fully bounty here to both the @brandonhenry and @allroundexperts. A lot of work went into this one, and in hindsight, this was a much more complex change than initially described in the OP of the issue. Any issues with the implementation would have likely been obviated had we designed this most holistically, rather than going straight to a GH issue.
  • Close the GH issue.
  • Take a fresh look at this internally, with a full design doc to make sure we handle all of the permutations of the various impacted flows before moving to implementation.

LMK if anyone has questions on that. Otherwise will tackle the payment steps later today!

@joekaufmanexpensify
Copy link
Contributor

Payment summary here:

@joekaufmanexpensify
Copy link
Contributor

@brandonhenry offer for $750 sent via Upwork

@joekaufmanexpensify
Copy link
Contributor

@allroundexperts you're good to request payment here

@brandonhenry
Copy link
Contributor

@joekaufmanexpensify thank you!!

@joekaufmanexpensify
Copy link
Contributor

@brandonhenry $750 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

Closing this one out as all that's left is for @allroundexperts to request payment in Newdot. We'll be continuing discussion on the longer term solution for this in Slack!

@JmillsExpensify
Copy link

$750 approved for @allroundexperts

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
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.