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
4.02k stars 3.01k forks source link

[$250] Expensify Card - Limit input page shows error after incorrect magic code is entered #56363

Open vincdargento opened 5 days ago

vincdargento commented 5 days 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.94-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Email or phone of affected tester (no customers): applausetester+230932@applause.expensifail.com Issue reported by: Applause Internal Team Device used: Mac 15.0 / Chrome App Component: Workspace Settings

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. Go to workspace settings > Expensify Card. 3, Click Issue card.
  3. Fill in the details and reach the confirmation page.
  4. On confirmation page, enter incorrect magic code.
  5. Clear the magic code input and enter the correct magic code.
  6. Go to workspace settings > Members.
  7. Click + New card.
  8. Select card type (virtual or physical) > Next.
  9. Click Next.

Expected Result:

Limit input page will not show error.

Actual Result:

Limit input page shows error - You must be a domain admin to create the virtual card.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/53d50139-be5f-4e90-bbcc-85a3b840fda5

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021887634706743746932
  • Upwork Job ID: 1887634706743746932
  • Last Price Increase: 2025-02-06
Issue OwnerCurrent Issue Owner: @hungvu193
melvin-bot[bot] commented 5 days ago

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

NJ-2020 commented 5 days ago

🚨 Edited by proposal-police: This proposal was edited at 2025-02-07 01:49:32 UTC.

Proposal

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

Expensify Card - Limit input page shows error after incorrect magic code is entered

What is the root cause of that problem?

When we enter invalid magic code, the BE response with this error without passing the policy id i.e issueNewExpensifyCard only instead of issueNewExpensifyCard_policyID:

Image

And inside the validate code action modal we only showing an error: Oops... something went wrong and your request could not be completed. Please try again later.

And when we close the validation modal we clear the error with the policy id https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/Card.ts#L403-L405

But since the BE doesn't response with policy id, it won't clear the errors

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

We should clear the issueNewExpensifyCard only like the BE response https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/Card.ts#L403-L405

Onyx.merge(`${ONYXKEYS.COLLECTION.ISSUE_NEW_EXPENSIFY_CARD}`, {errors: null});
Onyx.merge(`${ONYXKEYS.COLLECTION.ISSUE_NEW_EXPENSIFY_CARD}${policyID}`, {errors: null});

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

N/A

What alternative solutions did you explore? (Optional)

We should fix this in the BE passing the policy id in the response

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

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

hungvu193 commented 3 days ago

Likely a Backend issue. I'll take a look this weekend.

thelullabyy commented 2 days ago

Proposal

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

The limit input page shows an error after an incorrect magic code is entered

What is the root cause of that problem?

In this PR, we decided to use many ISSUE_NEW_EXPENSIFYCARD{policyID} fields (adding policyID to the end) to distinguish the data between multiple workspaces.

But we forgot to update the BE to return the errors to correct ISSUE_NEW_EXPENSIFY_CARD_{policyID} field. The BE always returns the errors to ISSUE_NEW_EXPENSIFY_CARD field

Image

Incoincident, ISSUE_NEW_EXPENSIFY_CARD is the source of data of the form

https://github.com/Expensify/App/blob/b0b7709ca7e8bbcc4bb1c6588e570170ac9931d9/src/pages/workspace/expensifyCard/issueNew/LimitStep.tsx#L83

It means that FormProvider will get errors from ISSUE_NEW_EXPENSIFY_CARD field automatically and display it

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

  1. Update the BE to return errors to the correct ISSUE_NEW_EXPENSIFYCARD{policyID} field (with the correct policyID)

  2. I search on the code base and see that ISSUE_NEW_EXPENSIFY_CARD_FORM is used in 2 places: Limit Step and Card Name Step.

Image

I suggest using other form fields in 2 places. Using ISSUE_NEW_EXPENSIFY_CARD_FORM in two pages is not suitable because these two pages are independent of each other, If we use a form field in two places It means that all fields in that form will be shared with each other.

One more thing, currently ISSUE_NEW_EXPENSIFY_CARD_FORM matched to issueNewExpensifyCard field on Onyx but issueNewExpensifyCard is no longer used in our App anymore because of https://github.com/Expensify/App/pull/55482 (or on some edge case that the BE still return issueNewExpensifyCard field to FE like in this issue). These things may cause unexpected things to Limit Step and Card Name Step

Let's create two new form fields and use them in these places:

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

None

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

hungvu193 commented 8 hours ago

Using ISSUE_NEW_EXPENSIFY_CARD_FORM in two pages is not suitable because these two pages are independent of each other, If we use a form field in two places It means that all fields in that form will be shared with each other.

What's the problem of sharing the formKey between screens? We're still doing it till now, you can easily find duplicate formKey almost everywhere. Ie: ISSUE_NEW_EXPENSIFY_CARD_FORM, NETSUITE_CUSTOM_SEGMENT_ADD_FORM, ....etc

thelullabyy commented 7 hours ago

We're still doing it till now, you can easily find duplicate formKey almost everywhere

Yes, we use the same Onyx form key in multiple forms in other places, but it only causes problems in some special cases like this bug (or some other unexpected things).

If there are any errors saved in ISSUE_NEW_EXPENSIFY_CARD_FORM, It will be displayed on both pages that I think it shouldn't happen because Limit step and card name step shouldn't have any mutual error

Using an independent Onyx form key will be the safest way with no downsides. This is the reason why I suggest that in my proposal

thelullabyy commented 7 hours ago

We have 2 ways to go:

  1. If we apply my solution, we can ensure that this bug is fixed without BE change and it will prevent us from unexpected things due to using the same Onyx form key in multiple places
  2. Fixing on BE only, It may take a long time to solve this problem (we need to remove ISSUE_NEW_EXPENSIFY_CARD totally).
hungvu193 commented 3 hours ago

Change the formKey only hides the error from the user interface the error is still there inside Onyx storage. Let's fix this from BE by returning the correct errorKey (with policy id).

🎀 👀 🎀

melvin-bot[bot] commented 3 hours ago

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