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.56k stars 2.9k forks source link

[Hold for #49445][$250] Profile - The incorrect magic code sign is not displayed when wrong code is inserted #50653

Closed lanitochka17 closed 3 weeks ago

lanitochka17 commented 1 month 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.48 Reproducible in staging?: Y Reproducible in production?: Y 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): Yokabdk+new223@gmail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Click "Account setting" > "Profile"
  3. Click "Contact method"
  4. Click "New Contact Method" and enter a secondary email address that has not been used to sign in to an Expensify account.
  5. observe that Magic code has been sent to your email
  6. Insert wrong magic code

Expected Result:

An incorrect magic code sign is shown, allowing the user to easily identify the error and edit the magic code

Actual Result:

The error message that says "Failed to add the contact method" will be shown, but the exact error will not be visible to the user

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/df5b93fe-5c64-4279-8016-21841f7c3c25

fail

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021846210499801072215
  • Upwork Job ID: 1846210499801072215
  • Last Price Increase: 2024-10-15
Issue OwnerCurrent Issue Owner: @akinwale
melvin-bot[bot] commented 1 month ago

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

Shahidullah-Muffakir commented 1 month ago

Edited by proposal-police: This proposal was edited at 2023-10-06T15:45:00Z.

Proposal

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

When adding a new contact method (email), if the user enters an incorrect Magic code during the email verification step, no clear error message is displayed. Instead, the user is redirected to the next page without being informed that the Magic code was wrong. The expected behavior is to show a clear error message and prevent the user from moving forward.

What is the root cause of that problem?

As we are using addNewContactMethod action in the NewContactMethodPage:

https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L58

  1. In the addNewContactMethod function, when there’s a failure, the error is saved under the addedLogin key: Here
  2. In the NewContactMethodPage component, the code is incorrectly accessing the validateLogin key, which always returns an empty object, rather than the addedLogin key: Here and Here
  3. we are not memoizing this clearError function using useCallback: https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L150 which case to the run the following useEffect in each state update: https://github.com/Expensify/App/blob/780e09e10fbd33f9806e5e1b30a22bd3703de742/src/components/ValidateCodeActionModal/ValidateCodeForm/BaseValidateCodeForm.tsx#L128-L130 so even if there are errors, this useEffect is clearing them.

As a result, the error object remains empty, and the user is redirected without seeing the error.

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

  1. Update the NewContactMethodPage component to use the addedLogin key instead of validateLogin in both places.
  1. Modify the error message in the addNewContactMethod function: Update this line: here to ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('contacts.genericFailureMessages.validateSecondaryLogin')

  2. if there is an error don't allow user to go to the next page, add this check: if(validateLoginError) return; here: https://github.com/Expensify/App/blob/e4969b22e7dbb1f6a200cc587ced4ab665415c0e/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L58 as:

    const addNewContactMethod = useCallback(
        (magicCode: string) => {
            User.addNewContactMethod(pendingContactAction?.contactMethod ?? '', magicCode);
            if(validateLoginError) return;
            Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS.route);
        },
        [pendingContactAction?.contactMethod],
    );
  3. wrap this clearError function in the useCallback: https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L150

https://github.com/user-attachments/assets/5d24a3c1-8103-4a48-b107-57b6f26048ed

OfstadC commented 1 month ago

Testing now

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

mkzie2 commented 1 month ago

Proposal

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

The error message that says "Failed to add the contact method" will be shown, but the exact error will not be visible to the user

What is the root cause of that problem?

We always navigate back to the list contact method page after we submit the magic code

https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L59-L65

Another problem here is we added the addedLogin error and pending field here and here

But we get the error with validateLogin here

https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L43

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

We should only navigate back to the list contact method page if we enter the correct magic code.

  1. Add a new callback in addNewContactMethod function and call the callback after the API is complete
function addNewContactMethod(contactMethod: string, validateCode = '', callback = () => {}) {
...
API.write(WRITE_COMMANDS.ADD_NEW_CONTACT_METHOD, parameters, {optimisticData, successData, failureData}).then(callback);
  1. Move the call Navigate here as the callback param to addNewContactMethod function and only navigate back if we don't have any error
const addNewContactMethod = useCallback(
    (magicCode: string) => {
        User.addNewContactMethod(pendingContactAction?.contactMethod ?? '', magicCode, () => {
            if (Object.values(validateLoginError).length) {
                return;
            }
            Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS.route);
        });
    },
    [pendingContactAction?.contactMethod, validateLoginError],
);

https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L59-L65

  1. We should get the error from addedLogin field here or we can change the field in addNewContactMethod to validateLogin

https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L43

What alternative solutions did you explore? (Optional)

For points 1 and 2 we can return the promise in addNewContactMethod and do the navigate logic in this promise

https://github.com/Expensify/App/blob/2d9a28ff3e56f449b9bfce0bb1cec5c3e0cb91c2/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L59-L65

OfstadC commented 1 month ago

@akinwale Could you review the proposals by Monday? Thank you! 😃

Shahidullah-Muffakir commented 1 month ago

Since refactoring is currently taking place on the same files and they are implementing a different modal, I suggest we hold off on this issue until those changes are completed. PR

hungvu193 commented 1 month ago

Yes, @OfstadC please hold this one for https://github.com/Expensify/App/pull/49445, we fixed this issue there

OfstadC commented 3 weeks ago

@hungvu193 - looks like https://github.com/Expensify/App/pull/49445 has been deployed. Do we need to do anything else for this issue? Or does that cover this fix?

hungvu193 commented 3 weeks ago

Verified it's fixed on Staging. We can close this issue 😄

https://github.com/user-attachments/assets/b2f936ed-668a-4570-98e8-fc52f4b46024

OfstadC commented 3 weeks ago

Thank youuu!