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.03k stars 3.03k forks source link

[Due for payment 2025-02-19] [$250] iOS - Contact method - Nothing happens on tapping the main email & secondary contact in contact methods page #56355

Open vincdargento opened 1 week ago

vincdargento 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: v9.0.94-1 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: Yes, reproducible on both 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+ck22425@applause.expensifail.com Issue reported by: Applause Internal Team Device used: iPhone 16/iOS 18 App Component: User Settings

Action Performed:

  1. Launch iOS Hybrid or Standalone app
  2. Create new account (can be expensifail or gmail account)
  3. Go to Settings >> Profile >> Contact method
  4. Tap on main contact method
  5. Tap on New contact method
  6. Enter secondary contact
  7. Enter magic code
  8. Select the added new secondary method
  9. Enter magic code to validate secondary contact
  10. Tap on the secondary contact account

Expected Result:

  1. On step 4 - A page should open which says "This is your current default contact method. Before you can delete it, you'll need to choose another contact method and click “Set as default”."
  2. On step 10 - Tapping on secondary contact method should open a page where remove option is displayed

Actual Result:

Nothing happens on tapping the main email & secondary contact method in contact methods page

Note: On ND Standalone app - When logging with new gmail account, a page to enter magic code appears to verify the account. After verifying account, tapping on main contact method does not respond

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/7dc3f13b-8f59-4d85-8282-3881dbfb0bf5

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021886927951828663823
  • Upwork Job ID: 1886927951828663823
  • Last Price Increase: 2025-02-05
  • Automatic offers:
    • QichenZhu | Contributor | 106021589
Issue OwnerCurrent Issue Owner: @MitchExpensify
melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 1 week ago

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

QichenZhu commented 1 week ago

🚨 Edited by proposal-police: This proposal was edited at 2025-02-05 21:42:30 UTC.

Proposal

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

Fails to enter the contact details page.

What is the root cause of that problem?

When entering the contact details page, the effect below resets the code to unsent state.

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L170

It runs an Onyx merge command.

https://github.com/Expensify/App/blob/27780d5a3c15ce3141d6ed3035cb4ed218f8f265/src/libs/actions/User.ts#L323

loginList is undefined while a merge is pending. After the merge completes, the value becomes valid again.

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L52

The effect below sees this change from undefined to valid as the contact method just getting verified, so it navigates the page back to the contact list.

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L158-L160

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

Only reset the code sent state if the contact method hasn’t been verified yet.

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L170

useEffect(() => {
    if (loginData?.validatedDate) {
        return;
    }
    resetContactMethodValidateCodeSentState(contactMethod);
}, [contactMethod, loginData?.validatedDate]);

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)

To prevent the undefined value during the merge, pass {allowStaleData: true} to useOnyx.

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L52

const [loginList, loginListResult] = useOnyx(ONYXKEYS.LOGIN_LIST, {allowStaleData: true}); 
truph01 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?

https://github.com/Expensify/App/blob/adc61ce54171f0e18892b8eb5cc41e1e4c40e898/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.tsx#L154

to the following::

        if (prevValidatedDate || prevValidatedDate === undefined || !loginData?.validatedDate || !loginData) {

What alternative solutions did you explore? (Optional)

s77rt commented 1 week ago

@QichenZhu Thanks for the proposal. Your RCA is correct however the solution does not address the root cause. We should avoid updating that onyx key.

s77rt commented 1 week ago

@truph01 Thanks for the proposal. Your RCA is not correct.

This happens because prevValidatedDate is undefined at the time of navigation

This is not true

https://github.com/user-attachments/assets/3fc0f6b5-c4c5-4b9f-a9de-edd4e0192c88

s77rt commented 1 week ago

FWIW, this is a regression from https://github.com/Expensify/App/issues/55405. If we can alter the approved solution there or find another solution that would be probably easier/better.

QichenZhu commented 1 week ago

Proposal

Updated

@QichenZhu Thanks for the proposal. Your RCA is correct however the solution does not address the root cause. We should avoid updating that onyx key.

@s77rt Thanks for your advice! I’ve updated the main solution and moved the previous solution to the alternative section.

s77rt commented 1 week ago

@QichenZhu Thanks for the update. I think that solution may work but I was thinking of something a little different that is to reset the code sent state on the clean up effect. Can you check if we can go with that instead?

QichenZhu commented 1 week ago

@s77rt Thanks for the feedback! Yeah, it would be better if we could do it in the cleanup phase. But the problem is that cleanup code isn’t always guaranteed to run, for example, when you don’t exit the page gracefully.

s77rt commented 1 week ago

@QichenZhu Good point! Can you double check that your solution works well with accounts that are not verified too?

QichenZhu commented 1 week ago

@s77rt Tested on iOS, and it works for unverified accounts.

https://github.com/user-attachments/assets/2a50f9a3-c157-452f-b9a0-85ba0bf463b7

s77rt commented 1 week ago

@QichenZhu Can you check that after verifying the account we get navigated back correctly?

QichenZhu commented 1 week ago

@s77rt Yes, it navigates back correctly.

https://github.com/user-attachments/assets/b45af20b-e648-49f3-ab75-6689cede0530

s77rt commented 1 week ago

@QichenZhu Thank you! The proposal looks good to me.

🎀 👀 🎀 C+ reviewed Link to.proposal

melvin-bot[bot] commented 1 week ago

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

MarioExpensify commented 1 week ago

Very nice and detailed proposal @QichenZhu, let's move forward. Thanks @s77rt!

melvin-bot[bot] commented 1 week ago

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

melvin-bot[bot] commented 5 days ago

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

melvin-bot[bot] commented 5 days ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 9.0.96-1 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 2025-02-19. :confetti_ball:

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

melvin-bot[bot] commented 5 days ago

@s77rt @MitchExpensify @s77rt The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

s77rt commented 1 day ago

BugZero Checklist:

Bug classification Source of bug: - [ ] 1a. Result of the original design (eg. a case wasn't considered) - [X] 1b. Mistake during implementation - [ ] 1c. Backend bug - [ ] 1z. Other: Where bug was reported: - [X] 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging) - [ ] 2b. Reported on staging (eg. found during regression or PR testing) - [ ] 2d. Reported on a PR - [ ] 2z. Other: Who reported the bug: - [ ] 3a. Expensify user - [ ] 3b. Expensify employee - [ ] 3c. Contributor - [X] 3d. QA - [ ] 3z. Other:
Regression Test Proposal Template

Regression Test Proposal

Precondition:

Test:

  1. Go to Settings > Profile > Contact Method
  2. Tap "New contact method"
  3. Enter a secondary contact
  4. Enter the magic code sent to the primary contact
  5. Tap the secondary contact
  6. Enter the magic code sent to the secondary contact
  7. Tap the secondary contact again
  8. Verify that you land on the contact details page

Do we agree 👍 or 👎