MitchExpensify commented 6 months ago

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

Action Performed:

Submit a request as User A to User B
As User B click "Pay with Expensify" on the request (Do nothing)
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 https://github.com/Expensify/App/issues/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:

B:

Workaround:

None

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] 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

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @joekaufmanexpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

melvin-bot[bot] commented 6 months ago

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

MitchExpensify commented 6 months ago

Slack convo https://expensify.slack.com/archives/C05NJ4SLBMF/p1707362622166499

joekaufmanexpensify commented 6 months ago

Discussing

MitchExpensify commented 6 months ago

Let's run with this issue!

MitchExpensify commented 6 months ago

Let's hunt down proposals @joekaufmanexpensify 🦾

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

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

ikevin127 commented 6 months ago

[!NOTE] Considering the amount of work involved - I'm willing to work on this for no less than $1000.

Proposal

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

The Pay button in ND has two sets of options:

pressing the button brings up payment options
pressing on the drop down brings up options to pay elsewhere

This UX is pretty confusing.

What is the root cause of that problem?

New feature / refactoring / clean-up.

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

From https://github.com/Expensify/App/issues/33967: follow the specs / conversation carefully and build on @tgolen's draft PR https://github.com/Expensify/App/pull/34689 to accomplish the Expected result as stated.

Next steps in moving forward using the draft PR in order to get this shipped -> from https://github.com/Expensify/App/issues/33967#issuecomment-1915035254:

There are multiple sets of options as mentioned in the original description. One set of options is in front of the KYCPaywall (pay with expensify, pay elsewhere), and one set of options is behind the KYCPaywall (bank account, personal bank account, debit card). In order to have all options in a single dropdown, then the way the dropdown interacts with the KYCPaywall needs to be refactored.

brandonhenry commented 6 months ago

Proposal

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

The problem is the inconsistency and clutter in the payment options presented in New Dot when a user attempts to pay a request. The current implementation separates payment methods into different actions, which can confuse users. The goal is to streamline the payment process into a single, cohesive flow.

What is the root cause of that problem?

The root cause is the current design, which does not unify payment options under a single action. There's a lack of intuitive UI flow that remembers the user's previous payment preferences, leading to a repetitive selection process for each transaction.

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

Redesign the payment button to a single dropdown labeled "Pay ", which will consolidate all payment options into one list.
Implement a feature to remember the user's payment preference based on the first payment by request type (IOUs vs. Expenses) and default to that method for subsequent payments unless the user specifies otherwise.
For expenses, default the payment method to be workspace-specific, using the workspace default settings if set and the user has access to it.
Ensure that this preference is set at the workspace level and does not cross-apply to other workspaces to maintain individual workspace preferences.
ensure implementation properly aligns to specifications

HarlemSquirrel commented 6 months ago

Proposal

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

There are two sets of options presented in the payment button dropdown shown to users who receive a request. This can cause confusion so we would like to consolidate the options. Clicking either the button text or the chevron should show all available options.

What is the root cause of that problem?

The button options are set on AddPaymentMethodMenu has two items in the buttonOptions array. The second sets of options which includes "elsewhere" is only accessible by clicking on the chevron.

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

We should remove the second item in buttonOptions and add the elsewhere option to AddPaymentMethodMenu.

melvin-bot[bot] commented 6 months ago

📣 @HarlemSquirrel! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Make sure you've read and understood the contributing guidelines.
Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:
```
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>
```

HarlemSquirrel commented 6 months ago

Contributor details Your Expensify account email: kevin@mccormack.tech Upwork Profile Link: https://www.upwork.com/freelancers/~01435a01fa4343ac93

melvin-bot[bot] commented 6 months ago

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

joekaufmanexpensify commented 6 months ago

Proposals pending review

tgolen commented 6 months ago

@all for your proposals, please be sure you have looked at the code and understand how the KYCPaywall is used and what refactorings are necessary to have all options be in a single dropdown. You can see my comments in https://github.com/Expensify/App/issues/33967#issuecomment-1915035254 for a little more context.

tgolen commented 6 months ago

@ikevin127 it sounds like you are on the right track! Do you have any idea of what needs to be done with the KYCPaywall?

HarlemSquirrel commented 6 months ago

Are we talking about KYCWall?

Looks like we no longer want to save the preferred payment method so we can change it to do the same thing as when the button is pressed to call selectPaymentType() or remove the options dropdown by swapping ButtonWithDropdownMenu for Button

ikevin127 commented 6 months ago

@tgolen Won't pretend to fully understand the system but from a quick look at the code I can tell the following:

We're using the AddPaymentMethodMenu component within the KYCWall which in the case where a user goes with the Pay with Expensify option (selected from dropdown) -> the dropdown options will be: Personal / Business BA (if the user can use any of the 2) and Card. When one of these are chosen -> user needs to pass the KYCWall (if didn't already).

https://github.com/Expensify/App/blob/bea6aafa5e1b4a80462c3da1f3c56325d3a3977d/src/components/AddPaymentMethodMenu.js#L68

Within the SettlementButton we use the KYCWall which exposes the following dropdown options:
- Pay with Expensify: if chosen and we click on the button (not dropdown) -> the Personal / Business BA (if the user can use any of the 2) and Card options are presented which require passing the KYCWall (if didn't already)
- Pay elsewhere: if chosen from the dropdown and we click on the button (not dropdown) -> KYCWall is not involved and the request will be marked as paid right away.

This being said, if we're looking at the following Expected result:

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

then we would need to pass all the options and adjust the current logic of the KYCWall from within the SettlementButton to display all of the above options when clicked on the dropdown arrow and selecting any -> the button (left side) will call the action of the selected dropdown option as we currently do for both non KYC / KYCWall options.

This can be a pretty congnitively complex endeavour since it would involve carefully considering cases like:

remembering the payment preference based on the first payment by request type (IOUs vs Expenses)
when defaulting payment method for expenses, use the workspace default if one is set in workspace settings
when defaulting payment method for expenses, make it workspace specific
if user has not paid using any of the options before (first time paying) and clicks on the left side of the button -> show the dropdown

but an intresting one to take on nonetheless!

brandonhenry commented 6 months ago

@tgolen makes sense! here is my updated proposal, with more details on what I think needs to change in order to meet spec:

Updated Technical Proposal

Problem Statement

The 'Pay' button currently presents disjointed payment options, which we want to consolidate into a single dropdown for a smoother user experience.

Examination of the Current Code

The KYCWall component acts as a gatekeeper for payment actions, ensuring that users meet KYC requirements. The AddPaymentMethodMenu component presents payment options to the user.

KYCWall Component (KYCWall.js):
- Currently handles the display logic of payment methods and KYC checks.
- Interacts with the AddPaymentMethodMenu to present payment options.
- Source: KYCWall.js
AddPaymentMethodMenu Component (AddPaymentMethodMenu.js):
- Renders the popover menu with payment options.
- Needs to be refactored to include a unified list of payment methods.
- Source: AddPaymentMethodMenu.js

Proposed Changes

I propose refactoring these components to create a unified dropdown while ensuring compliance with KYC requirements.

Integrate Payment Options into a Single Dropdown:

Refactor the menuItems prop within AddPaymentMethodMenu to include all payment methods post-KYC check.

Implement dynamic rendering based on KYC status and user preferences.

menuItems={[
...PaymentUtils.getPaymentMethods(bankAccountList, fundList, shouldShowPersonalBankAccountOption, shouldIncludeDebitCard).map(paymentMethod => ({
 text: translate(paymentMethod.textKey),
 icon: paymentMethod.icon,
 onSelected: () => selectPaymentMethod(paymentMethod.type),
})),
]}

Enhance State Management:
- Use the Onyx library to manage and remember user's payment preferences.
- Reference ONYXKEYS to set and retrieve user preferences.
```
Onyx.set(ONYXKEYS.USER_PAYMENT_PREFERENCES, { lastUsedMethod: selectedMethodType });
```
Address First-Time Payment Flow:
- For first-time payments, trigger the dropdown and prompt the user to select a payment method.
- Incorporate a check for first-time users:
```
if (PaymentUtils.isFirstTimePaying(userWallet)) {
showPaymentMethodSelection();
}
```
Workspace-Specific Defaults:
- Modify the logic to set and retrieve workspace-specific defaults.
- Integrate with backend services for workspace preferences:
```
API.getWorkspacePreferences(policyID).then((preferences) => {
setWorkspacePaymentDefaults(preferences.defaultPaymentMethod);
});
```
Refactor KYCPaywall Interaction:
- Alter how the KYCPaywall interacts with the dropdown, especially considering the unified payment options.
- Consider moving the KYCPaywall check to the onItemSelected event to ensure seamless user experience.

Additional Considerations

The KYCWall component is crucial because it determines whether the user should be shown the AddPaymentMethodMenu or be taken through the KYC flow. The key to refactoring this interaction is to ensure that the KYCPaywall's logic seamlessly integrates with the dropdown's display logic, enabling or disabling payment options as necessary.

Refactor KYCPaywall Interaction:

KYCPaywall and Dropdown Integration:

Enhance the selectPaymentMethod callback within KYCWall to handle KYC validations before setting the selected payment method. This ensures that the KYC flow is completed before a payment method is considered valid.

const selectPaymentMethod = useCallback((paymentMethod: PaymentMethod) => {
if (PaymentUtils.needsKYC(paymentMethod)) {
 Wallet.startKYCFlow(paymentMethod);
 return;
}
onSelectPaymentMethod(paymentMethod);
// ... existing logic ...
}, [onSelectPaymentMethod]);

Conditional Rendering in AddPaymentMethodMenu:

Incorporate KYC status checks directly in the rendering logic of the AddPaymentMethodMenu. We could use a new prop to indicate which payment methods are available post-KYC and conditionally render them.

menuItems={PaymentMethods.getAvailableMethods(userWallet, reimbursementAccount).map((method) => ({
text: translate(method.name),
icon: method.icon,
onSelected: () => {
 if (method.requiresKYC && !userWallet.hasPassedKYC) {
   Wallet.promptForKYC();
   return;
 }
 onItemSelected(method.type);
},
}))}

User Feedback for KYC-Required Methods:

In the AddPaymentMethodMenu, provide feedback for payment methods that require KYC. This could be a tooltip or a disabled state with an explanation that appears when the user tries to select an option that requires KYC completion.

menuItems={...}
renderItem={(item) => (
<MenuItem
 icon={item.icon}
 text={item.text}
 isDisabled={item.requiresKYC && !userWallet.hasPassedKYC}
 onPress={() => {
   if (item.requiresKYC && !userWallet.hasPassedKYC) {
     showAlert('Please complete KYC to use this payment method.');
     return;
   }
   item.onSelected();
 }}
/>
)}

By integrating these changes, we ensure that the KYCPaywall remains an integral part of the payment process without disrupting the user experience of having all payment options in one place.

This is just a rough idea that I was able to gather from my few hours of going through these components, but I believe this is on the right track for what is being requested. Happen to dive in further if needed!

allroundexperts commented 6 months ago

Looking into the proposals today 👀

joekaufmanexpensify commented 6 months ago

Great. TY!

allroundexperts commented 6 months ago

Thanks for your proposals everyone. While @ikevin127 seems to be moving on in the right direction, I feel like his proposal is too abstract.

@brandonhenry's proposal on the other hand has a lot more detail on how he'll be achieving the unification of the payment options. As such, let's go with them.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 6 months ago

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

melvin-bot[bot] commented 6 months ago

📣 @brandonhenry You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

brandonhenry commented 6 months ago

Thanks! I should have this PR ready by EOW. Will update if anything comes up

joekaufmanexpensify commented 6 months ago

Great. TY!

brandonhenry commented 6 months ago

This one is pretty much done, just testing multiple areas / making sure I have been proper with my changes :)

brandonhenry commented 6 months ago

@joekaufmanexpensify so I was doing some digging on this and had a question since this is my first ticket as a contributor. I was wondering how thorough my PR needs to be? I was digging around, and noticed that is a bit of a confusing matrix between the two types of payments options

Specifically, I need to update this line

https://github.com/Expensify/App/blob/e9fb49d9f41972a04bf07905b228314a3fdce8d4/src/types/onyx/OriginalMessage.ts#L5

to include CONST.PAYMENT_METHODS in order for me to consolidate the dropdowns.

https://github.com/Expensify/App/blob/9aa0509b3b155e9adbb95d8a32f78f7d3822c350/src/CONST.js#L435

That said, I have the UI working exactly like it works right now, but the visuals match what is requested in this ticket. Should I stop there since functionality is the same or should I keep digging / remove / rework how the system is setup to handle the different payment options (which now that i write this out, sounds like it may have some backend implications)

brandonhenry commented 6 months ago

Pushed up my PR and explained what I meant in the SettlementButton file in a comment https://github.com/Expensify/App/pull/37174/files

joekaufmanexpensify commented 6 months ago

Generally, we want PRs to be very thorough. But curious what @allroundexperts and @bondydaa think about this specific question , as I'm not exactly sure of the implications of the two options here.

bondydaa commented 6 months ago

I'm not sure I understand the question (or the system enough maybe 😅). feel free to tag @allroundexperts in a comment on your PR if you did something but aren't sure about it and we can discuss it there.

joekaufmanexpensify commented 6 months ago

@brandonhenry @allroundexperts is there an ETA for this to get to full review? I see the PR is still a WIP, and there are some outstanding Q's on it.

brandonhenry commented 6 months ago

@joekaufmanexpensify should be able to get this out EOW - there are some outstanding questions I had on the PR for @allroundexperts - need before continuing the checklist

allroundexperts commented 6 months ago

@joekaufmanexpensify so I was doing some digging on this and had a question since this is my first ticket as a contributor. I was wondering how thorough my PR needs to be? I was digging around, and noticed that is a bit of a confusing matrix between the two types of payments options

Specifically, I need to update this line

https://github.com/Expensify/App/blob/e9fb49d9f41972a04bf07905b228314a3fdce8d4/src/types/onyx/OriginalMessage.ts#L5

to include CONST.PAYMENT_METHODS in order for me to consolidate the dropdowns.

https://github.com/Expensify/App/blob/9aa0509b3b155e9adbb95d8a32f78f7d3822c350/src/CONST.js#L435

That said, I have the UI working exactly like it works right now, but the visuals match what is requested in this ticket. Should I stop there since functionality is the same or should I keep digging / remove / rework how the system is setup to handle the different payment options (which now that i write this out, sounds like it may have some backend implications)

Generally, I think over-engineered work brings in problems down the road. If your work is having backend implications, its best to mention that when writing the proposal. For this case though, can you elaborate on why we'll need the backend changes?

brandonhenry commented 6 months ago

@allroundexperts wasn't saying that we will need BE changes (and didn't think so in my proposal either), just wanting to confirm that there aren't a need (I personally don't think so tbh). I'd also like to confirm my changes have met the intentions of the original request. Will have more details on the PR

joekaufmanexpensify commented 5 months ago

Looks like PR is pending review from C+

mallenexpensify commented 5 months ago

@brandonhenry per CONTRIBUTING.md,

New contributors are limited to working on one job at a time, however experienced contributors may work on numerous jobs simultaneously.

You currently have 6 issues open with non merged or closed. Please do not submit proposals for new issues until all six PRs have been merged.

Also from CONTRIBUTING.md

Daily updates on weekdays are highly recommended. If you know you won’t be able to provide updates within 48 hours, please comment on the PR or issue stating how long you plan to be out so that we may plan accordingly. We understand everyone needs a little vacation here and there. Any issue that doesn't receive an update for 5 days (including weekend days) may be considered abandoned and the original contract terminated.

Please keep the issue and PR moving along with urgency, along with all others. You were assigned here on February 21 and it's now April 1.

brandonhenry commented 5 months ago

Of course @mallenexpensify 3 are currently awaiting review. I will ping on all and push to get those merged this week

Also, I did pause on submitting proposals. I had so many already however 😅

brandonhenry commented 4 months ago

This is still in review. Just passed back to @allroundexperts

joekaufmanexpensify commented 4 months ago

PR still in review

brandonhenry commented 4 months ago

Was out sick today but plan to get this one back into review tomorrow - have been running into issues while QA testing

anmurali commented 4 months ago

Can someone share a video of how this works post-fix? From the dev environment perhaps? We are actively discussing this design again in slack, and we want to make sure where we landed there is how we fixed it here.

shawnborton commented 4 months ago

Yeah, based on what we are discussing, the idea is that the large part of the Split button is for the last used option, and the arrow shows all available options. In the case where it's a first time user, both parts of the button show the same drop down menu but perhaps the big part's label just says "Pay $XX"

So to make that more clear:

If you've never payed using this button before:
- Big part of button says "Pay $XX" and tapping it reveals all menu options (Personal, Business, Debit, Elsewhere)
- Tapping the arrow reveals all menu options (Personal, Business, Debit, Elsewhere)
If you have payed using this button before
- Big part of button shows last used payment method via "Pay with Expensify" or "Pay elsewhere"
- Tapping the arrow reveals all menu options (Personal, Business, Debit, Elsewhere)

Does that capture it correctly @anmurali @dannymcclain?

dannymcclain commented 4 months ago

@shawnborton Yeah I think so.

The only thing I would add is: do we want to clarify the specific payment method used last vs just Pay with Expensify? So perhaps something like Pay with Bortibank (1234) or Pay with Visa (5678)? Or would tapping Pay with Expensify just bring up any of those options you already have stored?

shawnborton commented 4 months ago

Ah yeah good point. I don't feel too strongly, @anmurali do you have an opinion on that?

brandonhenry commented 4 months ago

@shawnborton how about the portion of the flow when you do not have a payment method added? Currently in prod, if you don't have a payment, it will show you the second set of options seen in the original post

These are not actual payment options but "Add Payment Method options". My current implementation combines the two menus, checking if you have added a payment option, and displays Add X Payment option if you haven't (but all are always shown as in your post above)

Thoughts?

I also need to test this out to make sure that when you add a card, it changes from add to pay. Do we have test credit card/ bank account details or do I need to obtain test accounts somehow?

Cc. @anmurali @dannymcclain @allroundexperts

shawnborton commented 4 months ago

My current implementation combines the two menus, checking if you have added a payment option, and displays Add X Payment option if you haven't (but all are always shown as in https://github.com/Expensify/App/issues/36301#issuecomment-2061625926)

My thinking is that if you have never added a payment option, we could just use a generic phrase like "Pay $XX" and tapping that part of the button, or the arrow, would launch the same menu of all options. Does that make sense and sound like a good idea?

Expensify / App

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

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Proposal

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

What is the root cause of that problem?

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

Proposal

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

What is the root cause of that problem?

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

Proposal

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

What is the root cause of that problem?

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

Updated Technical Proposal

Problem Statement

Examination of the Current Code

Proposed Changes

Additional Considerations

Refactor KYCPaywall Interaction: