[$250] Account setting- Secondary account can't be added unless start over after an error

lanitochka17 commented 1 week 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.62-2 Reproducible in staging?: Y Reproducible in production?: Y 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 Issue reported by: Applause - Internal Team

Action Performed:

Prerequisite User A start a chat with User B All steps are done as User A

Navigate to http://www.staging.new.expensify.com/
Navigate to Setting > Profile> add user B as secondary contact
After magic code entered notice error for the fail
Click X of the error and navigate back once of RHP to edit the email
Edit the email to a new account (non existing user)
Magic code sent go to email and notice there is no email sent
Back to the app and navigate back twice from RHP to "New contact method" button
enter the same email address as step 5
Go to email and notice email received

Expected Result:Prerequisite

Email of magic code received in step 6 as the magic code is already sent

Actual Result:

Email for the magic code received only if the process steps from start "New contact method"

Workaround:

Unknown

Platforms:

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

[ ] Android: Standalone
[ ] Android: HybridApp
[ ] Android: mWeb Chrome
[ ] iOS: Standalone
[ ] iOS: HybridApp
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/d57adf0e-a6ec-41a0-a009-fe5d739d1434

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021858738599400775577
Upwork Job ID: 1858738599400775577
Last Price Increase: 2024-11-19

Issue Owner

Current Issue Owner: @thesahindia

melvin-bot[bot] commented 1 week ago

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

daledah commented 1 week ago

Proposal

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

Email for the magic code received only if the process steps from start "New contact method"

What is the root cause of that problem?

We only call the sendValidateCode() one time:

https://github.com/Expensify/App/blob/753de15139c000ed387be380362733ed3373bb44/src/components/ValidateCodeActionModal/index.tsx#L44-L51

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

We can expose a function resetFirstRenderRef in:

https://github.com/Expensify/App/blob/753de15139c000ed387be380362733ed3373bb44/src/components/ValidateCodeActionModal/index.tsx#L43

    const resetFirstRenderRef = () => {
        firstRenderRef.current = true;
    };
    useImperativeHandle(ref, () => ({resetFirstRenderRef}), []);

In:

https://github.com/Expensify/App/blob/753de15139c000ed387be380362733ed3373bb44/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L153

we can pass the ref prop like:

                <ValidateCodeActionModal
                    ref={validateModalRef}

then can use the ref in:

https://github.com/Expensify/App/blob/753de15139c000ed387be380362733ed3373bb44/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L52

    const handleValidateMagicCode = useCallback(
        (values: FormOnyxValues<typeof ONYXKEYS.FORMS.NEW_CONTACT_METHOD_FORM>) => {
            if (values.phoneOrEmail !== pendingContactAction?.contactMethod) {
                validateModalRef.current?.resetFirstRenderRef?.();
            }

or

https://github.com/Expensify/App/blob/753de15139c000ed387be380362733ed3373bb44/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L133

                        <InputWrapper
                            onValueChange={()=>{
                                validateModalRef.current?.resetFirstRenderRef?.();
                            }}

What alternative solutions did you explore? (Optional)

Shahidullah-Muffakir commented 1 week ago

Proposal

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

When a user goes back and again click on the Add, the magic code is not being sent.

What is the root cause of that problem?

The root cause stems from the original implementation usingfirstRenderRef.current which was added to solve the double-sending problem because the ValidateCodeActionModal component renders multiple times when it first mounts
Each render was triggering the useEffect, causing multiple sendValidateCode() calls
new problem created by the fix
- The firstRenderRef.currentsolution only allows the code to be sent on the very first render of the component
- Once set to false, it never resets, so returning to the modal never triggers a new code send https://github.com/Expensify/App/blob/a6f1348d08a441f4f9b48a8ef05e5e95c96b53d7/src/components/ValidateCodeActionModal/index.tsx#L48
  What changes do you think we should make in order to solve the problem?
  
  We should replace firstRenderRef with hasCodeBeenSentRef that resets when the modal closes:

const hasCodeBeenSentRef = useRef(false);

const hide = useCallback(() => {
    clearError();
    onClose();
    hasCodeBeenSentRef.current = false;  // Reset when modal closes
}, [onClose, clearError]);

useEffect(() => {
    if (!isVisible || hasMagicCodeBeenSent || hasCodeBeenSentRef.current) {
        return;
    }
    hasCodeBeenSentRef.current = true;
    sendValidateCode();
}, [isVisible, sendValidateCode, hasMagicCodeBeenSent]);

What alternative solutions did you explore? (Optional)

we can just add this on the hide function(which is also called when user go back), so when the this page closes, we are setting it back to true: firstRenderRef.current = true;

it is same as my main approach, the difference is just the naming of variable, in the first approach i think the naming of variable is more readable

huult commented 1 week ago

Edited by proposal-police: This proposal was edited at 2024-11-16 06:39:01 UTC.

Proposal

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

Secondary account can't be added unless start over after an error

What is the root cause of that problem?

As per the current logic, we check if it’s not the first render, and if it’s not the first render, we don’t send the validate code. This is to prevent sending the validate code multiple times. https://github.com/Expensify/App/blob/29d054cea0d6e84ec44f522c0d31590a973bfb87/src/components/ValidateCodeActionModal/index.tsx#L52-L59

But when we input the wrong validate code, go back, and change to a new email, firstRenderRef still exists as false because we didn't update it when the email changed. As a result, the function to send the validate code will not execute because it returns early due to firstRenderRef, and this issue occurs.

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

To resolve this issue, we should check if the email has changed, and if so, we must reset firstRenderRef. The code should be modified like this:

1 We need to store the previous phone number or email to compare when the 'Add' button is clicked. We must check this in the add function because the user is able to re-enter the email after it has been changed.

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L51
+    const previousPhoneOrEmail = useRef('');
+    const [isChangedPhoneOrEmail, setIsChangedPhoneOrEmail] = useState(false);

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L52
      const handleValidateMagicCode = useCallback((values: FormOnyxValues<typeof ONYXKEYS.FORMS.NEW_CONTACT_METHOD_FORM>) => {
        const phoneLogin = LoginUtils.getPhoneLogin(values.phoneOrEmail);
        const validateIfnumber = LoginUtils.validateNumber(phoneLogin);
        const submitDetail = (validateIfnumber || values.phoneOrEmail).trim().toLowerCase();

+        setIsChangedPhoneOrEmail(previousPhoneOrEmail?.current !== submitDetail);
+        previousPhoneOrEmail.current = submitDetail;
        User.addPendingContactMethod(submitDetail);
        setIsValidateCodeActionModalVisible(true);
    }, []);
// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L159
      <ValidateCodeActionModal
          ... 
+          isChangedPhoneOrEmail={isChangedPhoneOrEmail}
      />

2 In ValidateCodeActionModal, we need check if isChangedPhoneOrEmail is then we need reset firstRenderRef

// src/components/ValidateCodeActionModal/index.tsx#L45
+    useEffect(() => {
+        if (!isChangedPhoneOrEmail) {
+            return;
+        }
+        firstRenderRef.current = true;
+    }, [isChangedPhoneOrEmail]);

We need to reset isChangedPhoneOrEmail after sending the validation code to ensure the email validation works correctly.

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L172
    <ValidateCodeActionModal
            ...
            sendValidateCode={() => {
+                 setIsChangedPhoneOrEmail(false);
                  User.requestValidateCodeAction();
            }}
            descriptionPrimary={translate('contacts.enterMagicCode', {contactMethod})}
            isChangedPhoneOrEmail={isChangedPhoneOrEmail}
      />

Test branch

POC

https://github.com/user-attachments/assets/781ebc91-0f34-41c3-833a-0a548e9d908e

What alternative solutions did you explore? (Optional)

Alternatively, if we want to send the validate code only once, we can add the email that the code was sent to into a list and compare it when the 'Add' button is clicked. This list will reset when the modal is closed.

Create a list to add a phone number or email that sent a validation code

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L51
+    const previousPhoneOrEmailList = useRef<string[]>([]);
+    const [isChangedPhoneOrEmail, setIsChangedPhoneOrEmail] = useState(false);

Compare the phone number or email in the list. If it's a new phone number or email, set setIsChangedPhoneOrEmail to true to send the validation code; otherwise, set it to false to not send the code.

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L52
    const handleValidateMagicCode = useCallback((values: FormOnyxValues<typeof ONYXKEYS.FORMS.NEW_CONTACT_METHOD_FORM>) => {
        const phoneLogin = LoginUtils.getPhoneLogin(values.phoneOrEmail);
        const validateIfnumber = LoginUtils.validateNumber(phoneLogin);
        const submitDetail = (validateIfnumber || values.phoneOrEmail).trim().toLowerCase();

+        if (!previousPhoneOrEmailList.current.includes(submitDetail)) {
+            setIsChangedPhoneOrEmail(true);
+            previousPhoneOrEmailList.current.push(submitDetail);
+        } else {
+            setIsChangedPhoneOrEmail(false);
+        }
        User.addPendingContactMethod(submitDetail);
        setIsValidateCodeActionModalVisible(true);
    }, []);

Send isChangedPhoneOrEmail to ValidateCodeActionModal to determine whether to reset firstRenderRef

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L159
      <ValidateCodeActionModal
          ... 
+          isChangedPhoneOrEmail={isChangedPhoneOrEmail}
      />

In ValidateCodeActionModal, we need check if isChangedPhoneOrEmail is then we need reset firstRenderRef

// src/components/ValidateCodeActionModal/index.tsx#L45
+    useEffect(() => {
+        if (!isChangedPhoneOrEmail) {
+            return;
+        }
+        firstRenderRef.current = true;
+    }, [isChangedPhoneOrEmail]);

We need to reset isChangedPhoneOrEmail after sending the validation code to ensure the email validation works correctly.

// src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx#L172
    <ValidateCodeActionModal
            ...
            sendValidateCode={() => {
+                 setIsChangedPhoneOrEmail(false);
                  User.requestValidateCodeAction();
            }}
            descriptionPrimary={translate('contacts.enterMagicCode', {contactMethod})}
            isChangedPhoneOrEmail={isChangedPhoneOrEmail}
      />

POC

https://github.com/user-attachments/assets/2622d62a-e453-4a53-aa93-42d0b9364aa9

melvin-bot[bot] commented 1 week ago

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

strepanier03 commented 1 week ago

Working on testing this one now.

strepanier03 commented 1 week ago

Repro'd with steps as described.

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

daledah commented 1 week ago

Proposal updated

thesahindia commented 1 week ago

@Shahidullah-Muffakir's proposal looks good to me!

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

📣 @Shahidullah-Muffakir 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 📖

daledah commented 1 week ago

@yuwenmemon @thesahindia The selected proposal does not match the expected behavior in the OP, it always call ResendValidateCode when opening the ValidateCodeActionModal, but the expected behavior is that, we only call "ResendValidateCode" if user enter new email.

Shahidullah-Muffakir commented 1 week ago

@yuwenmemon @thesahindia The selected proposal does not match the expected behavior in the OP, it always call ResendValidateCode when opening the ValidateCodeActionModal, but the expected behavior is that, we only call "ResendValidateCode" if user enter new email.

I don't think it is the expected behavior, as a user if i click on the Add button, i want to receive a majic code, even if the email did not change.

daledah commented 1 week ago

I think we need the confirmation from internal team here

huult commented 1 week ago

I think it should send a magic link only once per email added, similar to the alternative solution in the video. This is useful to avoid spamming by sending too many verification codes

thesahindia commented 6 days ago

@yuwenmemon, could we please confirm the expected result here?

melvin-bot[bot] commented 21 hours ago

@yuwenmemon, @strepanier03, @thesahindia, @Shahidullah-Muffakir Huh... This is 4 days overdue. Who can take care of this?

Expensify / App