search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.15k stars 2.64k forks source link

[HOLD for payment 2024-07-25] [$250] [Payment card / Subscription] When adding Payment card, there is a slight delay before it shows #44904

Open lanitochka17 opened 2 weeks ago

lanitochka17 commented 2 weeks 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: 9.0.4-6 Reproducible in staging?: Y Reproducible in production?: N 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 https://github.com/Expensify/App/pull/44361

Action Performed:

  1. Access staging.new.expensify.com
  2. Sign into a valid account (Beta enabled)
  3. Access https://staging.new.expensify.com/settings/subscription/add-payment-card
  4. Input valid card data & Save

Expected Result:

User expects that after saving the card data immediately loads and displays

Actual Result:

Several seconds go by before the card actually appears on the Sub page, leading the user to think the details did not save

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7f761c91-7774-4285-bf1a-7ddd04c2c4d5

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b996dd4f4317410f
  • Upwork Job ID: 1809282225502876268
  • Last Price Increase: 2024-07-05
  • Automatic offers:
    • dominictb | Contributor | 103066195
Issue OwnerCurrent Issue Owner: @sonialiap
melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @mountiny (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 2 weeks ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 2 weeks ago

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

mountiny commented 2 weeks ago

Not a blocker since this is behind beta

@blimpich I hope you dont mind I will assign you.

Are we not handling this optimistically? Feels like we should, but if not there should be some loader

blimpich commented 2 weeks ago

Yeah this is a bug, we should be handling this optimistically.

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

cretadn22 commented 2 weeks ago

Proposal

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

When adding Payment card, there is a slight delay before it shows

What is the root cause of that problem?

In the addSubscriptionPaymentCard function, we do not include card information in the optimistic data. Additionally, we do not update the successData when successful or the error in the failure data.

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

In the addSubscriptionPaymentCard function, we must incorporate optimisticData to mirror the backend's response. Furthermore, we should update the successData upon successful execution and the failureData in case of errors.

What alternative solutions did you explore? (Optional)

Additionally, we could display a skeleton while awaiting the response from the backend (By using addPaymentCardForm.loading from Onyx)

dominictb commented 1 week ago

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?

    const [formData] = useOnyx(ONYXKEYS.FORMS.ADD_PAYMENT_CARD_FORM);
    const prevFormDataSetupComplete = usePrevious(!!formData?.setupComplete);

    useEffect(() => {
        if (prevFormDataSetupComplete || !formData?.setupComplete) {
            return;
        }

        PaymentMethods.continueSetup();
    }, [prevFormDataSetupComplete, formData?.setupComplete]);

if the API is called successfully, the setupComplete is set to true, based on that, we can auto close the form.

sonialiap commented 1 week ago

@getusha what do you think of the above proposals?

melvin-bot[bot] commented 1 week ago

@sonialiap, @blimpich, @getusha Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

getusha commented 1 week ago

thanks for the proposal @dominictb The solution you provided works and i see that's what we're doing in AddDebitCardPage, could you expand more on your RCA and why the solution makes it work? thanks!

dominictb commented 1 week ago

@getusha I updated my proposal to show how the solution works.

getusha commented 1 week ago

@dominictb's proposal looks good to me 🎀 👀 🎀 C+ reviewed!

melvin-bot[bot] commented 1 week ago

Current assignee @blimpich is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

getusha commented 1 week ago

@blimpich do we want to handle this optimistically? i assume the backend will need to validate the card?

This is the behavior when adding a bank account

https://github.com/Expensify/App/assets/75031127/80b62c3f-9ddd-4392-86d9-99acb070e879

After applying @dominictb's solution the form will stay until it finishes loading, same as ^

blimpich commented 1 week ago

Yes we should be handling this optimistically.

blimpich commented 1 week ago

@getusha Do you have any thoughts on @cretadn22's proposal?

getusha commented 1 week ago

The proposal sounds good, though I would like to see more details on the implementation. We also need to change the offline UX pattern. it's currently blocking form. @cretadn22 could you update your proposal to include that too?

Yes we should be handling this optimistically.

@blimpich I suggest just showing a loading indicator on the submit button, that'll allow us to show an appropriate error message (if any) while in the form, and to keep the current offline pattern as I see the backend does some validation on the card details. That way makes the most sense to me. Please check the Add a Debit Card page - settings/wallet/add-debit-card - i think it's similar to this page.

trjExpensify commented 1 week ago

I tend to agree with @getusha here, Submitting the Add payment card and Change payment currency forms were both pattern C in the design doc as well because of these third party API calls. So when offline, the Submit button on these forms is greyed out.

So I think we should remain on the form with a loading indicator on the submit button after they click it. If the API call is successful, the modal closes and the details should be updated on Subscription page (removing this feeling of there being a delay right now). If it fails, the modal doesn't close, and the errors are displayed in the form to fix using the standard form errors pattern.

@mountiny @shawnborton do ya'll agree? (Vit, I saw you asked about optimistic above)

shawnborton commented 1 week ago

That works for me!

trjExpensify commented 1 week ago

Great stuff, so @getusha can we move this on for a proposal review from Ben and get the PR going? Thanks!

blimpich commented 1 week ago

I think we're waiting on @cretadn22 to update their proposal with more details, so that it aligns with what we want here.

mountiny commented 1 week ago

Yeah that sounds good if this should be pattern C

cretadn22 commented 1 week ago

@blimpich I'm unable to add a payment card successfully. Could you please share mock card data that I can use for testing?

getusha commented 1 week ago

@blimpich we can proceed with @dominictb's proposal since we decided to stick with pattern C

@dominictb could you update your proposal to include https://github.com/Expensify/App/issues/45124 too? thanks!

blimpich commented 1 week ago

Ah yes sorry my mistake. Sounds good @getusha 👍

melvin-bot[bot] commented 1 week ago

📣 @dominictb 🎉 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 📖

blimpich commented 1 week ago

@dominictb please still update your proposal like @getusha said to include a fix for this issue, since they are basically the same issue 🙂

Thank you @getusha for your suggestion on how to make this better, I agree this is the ideal solution to arrive at 👍

dominictb commented 1 week ago

@blimpich Do we have the card or the mock data for testing?

I just created draft PR but I need the card to test and record screens in PR author checklist.

blimpich commented 1 week ago

@dominictb DM'ed you with steps on how to do it, requires some special steps to use test data.

dominictb commented 1 week ago

@blimpich Thanks. I am working on it.

dominictb commented 1 week ago

@blimpich I tried to set up the environment as you mentioned. It works well. But when I sign in with the old account (not the first sign-in), there is no magic code sent to my email. Do you know what issue in here? I need to use old account because with new account, page https://dev.new.expensify.com:8082/settings/subscription is displayed as "Not here page".

Updated: I fixed this issue.

dominictb commented 1 week ago

@blimpich I encountered issue when changing billing payment: When the API is called successfully, the onyxData responded by BE is empty:

https://github.com/user-attachments/assets/c4c6df4f-25f3-4856-8427-4f742102aace

blimpich commented 1 week ago

@dominictb sorry I forgot to mention that all magic codes when using a Expensify engineer's local backend will be 000000. You won't get any emails unless I manually run a particular script on my backend.

For the missing onyxData, that appears to be a bug. Thank you for catching that! I will get that fixed, but it will probably take a few days or longer.

blimpich commented 1 week ago

For now lets keep this issue/PR focused on adding a payment card, we'll use this issue to handle the change currency flow internally since this will require backend changes.

trjExpensify commented 6 days ago

@dominictb do you think we can have the draft PR in review today?

dominictb commented 5 days ago

cc @trjExpensify @blimpich

trjExpensify commented 5 days ago

Nice one! Can you complete the video/screenshots in the OP of the PR linked above? Thanks!

dominictb commented 5 days ago

@blimpich I found another BE bug when calling API AddPaymentCard. When we already added the payment card 1, its findList['101'].accountData.additionalData.isBillingCard is true. Then I add the new payment card 2, BE only sets findList['102'].accountData.additionalData.isBillingCard to true but does not setfindList['101'].accountData.additionalData.isBillingCard to false unless we refresh the page.

blimpich commented 5 days ago

@dominictb thank you for reporting the bug! I believe that has the same root cause as the first BE bug. Lets come back to that once the first BE bug is completed 👍

melvin-bot[bot] commented 2 days ago

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

melvin-bot[bot] commented 2 days ago

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

If no regressions arise, payment will be issued on 2024-07-25. :confetti_ball:

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

melvin-bot[bot] commented 2 days ago

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: