Fix stuck navigation & Re-add Contact Method Details pages #15751

aldo-expensify · 2023-03-08T19:37:43Z

Details

Re-implemented all of the changes from this PR (#15204) which was reverted, but this PR fixes the regression and adds back the old changes

To fix the bug, a functionality to keep track when navigations transitions are in progress was removed: b659e1b . That code was originally fixing this issue #14426, so we should check that we are not introducing the double tapping bug again.

TL;DR about the bug

There is a double navigation happening:
- Navigate to ROUTES.SETTINGS_CONTACT_METHODS when we confirm deleting the phone number in the modal (here)
- Inside the render function we navigate to ROUTES.SETTINGS_CONTACT_METHODS if the login data is not found (here). The login data is not found because the successData of the request removes it from Onyx.
ScreenWrapper reacts to the 'transitionStart' event setting Navigation.setIsNavigating(true); (here)
ScreenWrapper calls Navigation.setIsNavigating(false); on 'transitionEnd', but this never happens because we unsubscribe to these events before getting the 'transitionEnd' event.

Fixed Issues

$ #15725
$ #15203
$ #11575

Tests

Along with the below steps, also perform:

Connected to the staging API (couldn't reproduce connected to the dev API):

Login in NewDot and go to /settings/profile/contact-methods
Add a phone number (no need to validate)
You should be taken back to the "Contact methods" page:
Click on your phone number contact method and then delete it
Verify that you are taken back to the "Contact methods" page
Verify that you can tap in any of the menu entries and you get navigated

Verify that no errors appear in the JS console

Offline tests

Login in NewDot and go to /settings/profile/contact-methods
Add a phone number (no need to validate)
You should be taken back to the "Contact methods" page:
Click on your phone number contact method
GO OFFLINE
Click the delete button and confirm in the modal
Verify that you are taken back to the "Contact methods" page
Verify that the deleted contact method is still there with strike-through

QA Steps

Along with the below steps, also perform:

Login in NewDot and go to /settings/profile/contact-methods
Add a phone number (no need to validate)
You should be taken back to the "Contact methods" page:
Click on your phone number contact method and then delete it
Verify that you are taken back to the "Contact methods" page
Verify that you can tap in any of the menu entries and you get navigated

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-02-27.at.7.27.38.PM.mov

Mobile Web - Chrome

Screen.Recording.2023-03-01.at.1.59.44.PM.mov

Mobile Web - Safari

Screen.Recording.2023-03-01.at.2.17.43.PM.mov

Desktop

Screen.Recording.2023-02-27.at.7.43.14.PM.mov

iOS

Screen.Recording.2023-03-01.at.2.17.43.PM.mov

Android

Screen.Recording.2023-03-01.at.1.55.45.PM.mov

MelvinBot · 2023-03-08T19:38:22Z

@parasharrajat @neil-marcellini One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

aldo-expensify · 2023-03-08T19:39:43Z

cc @roryabraham @parasharrajat @syedsaroshfarrukhdot In case you have a better understanding of these navigation transitions and can say if this makes sense or not 🙏

aldo-expensify · 2023-03-08T19:40:12Z

cc @marcaaron since I think you know about Navigation stuff

aldo-expensify · 2023-03-08T19:50:17Z

Making it a draft again because I noticed that this has an unwanted effect: if you are offline, successData doesn't remove the contact method from Onyx and you are not navigated back until you come back offline.

Without this PR, when offline, we land here (correct behaviour):

With this PR, when offline, we land here:

aldo-expensify · 2023-03-08T19:55:31Z

The code navigating automatically to the main contacts methods page when the contact method is not found is here:

App/src/pages/settings/Profile/Contacts/ContactMethodDetailsPage.js

Lines 146 to 149 in 4237497

    
           if (!contactMethod || !loginData) { 
        
               Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS); 
        
               return null; 
        
           }

So the problem seems to be cause by this double navigation, I see two options for now:

Don't navigate if the contact method is not found, but show some page mentioning that the information was not found, or
Try to fix ScreenWrapper and Navigation.isNavigating stuff so it doesn't get stuck if we navigate twice.

pd: maybe we shouldn't be navigating back inside the render method, and that check should be in componentDidUpdate

parasharrajat · 2023-03-08T20:08:09Z

#15751 (comment)

Which one is the correct behavior?

I will focus on

Try to fix ScreenWrapper and Navigation.isNavigating stuff so it doesn't get stuck if we navigate twice.

If ScreenWrapper is getting unmounted before we can set the flags then we should move the logic of setting Navigation.setIsNavigating(value) to the parent component which is never mounted.

I haven't validated the root cause but I believe ya 😉

We can move this setup to top-level navigators in AuthScreens.js & PublicScreens.js.

App/src/libs/Navigation/AppNavigator/AuthScreens.js

Line 167 in 4237497

<RootStack.Navigator

from

App/src/components/ScreenWrapper/index.js

Lines 41 to 43 in 4237497

    
           this.unsubscribeTransitionStart = this.props.navigation.addListener('transitionStart', () => { 
        
               Navigation.setIsNavigating(true); 
        
           });

we can use screenListeners for this purpose https://reactnavigation.org/docs/navigation-events#screenlisteners-prop-on-the-navigator.

aldo-expensify · 2023-03-08T20:16:11Z

Update:

We now redirect in componentDidUpdate if the login data is not found (instead of in render())
I added a loading indicator in render if the login information is not available.

This seems to fix the bug correctly without making the offline experience worse.

aldo-expensify · 2023-03-08T20:28:57Z

Which one is the correct behavior?

The one on top (edited the comment to make it more clear)

If ScreenWrapper is getting unmounted before we can set the flags then we should move the logic of setting Navigation.setIsNavigating(value) to the parent component which is never mounted.

I haven't validated the root cause but I believe ya 😉

We can move this setup to top-level navigators in AuthScreens.js & PublicScreens.js.

Oh, that sounds like an interest approach, I can give it a try.

Besides that, maybe we should avoid navigating during rendering anyway. What do you think about that? I feel like the rendering function should be exclusively about rendering (showing data) and not triggering side-effects.

parasharrajat · 2023-03-08T20:33:20Z

Besides that, maybe we should avoid navigating during rendering anyway. What do you think about that? I feel like the rendering function should be exclusively about rendering (showing data) and not triggering side-effects.

Yeah, I agree. But there are no rules for this in react. React is functional so anything works.

You can also just render any function as JSX that function be a redirector. I think react-router has a component that does the same.

Your refactor looks good that is how it should be done for this component.

aldo-expensify · 2023-03-09T00:08:12Z

@parasharrajat about this suggestion #15751 (comment)

The difficulty I see is that ScreenWrapper has this state.didScreenTransitionEnd that changes on 'transitionEnd'.

If we moved the event listening part to higher in the tree to AuthScreens.js & PublicScreens.js, I'm not sure how would ScreenWrapper get notified when the transition ends. ScreenWrapper is used in many places, so drilling a state like that from AuthScreens.js & PublicScreens.js until it gets to ScreenWrapper sounds like a looot of work. The other option would be to keep this state in Onyx so we can access it easily in ScreenWrapper, but I'm not sure if there could be some bad consequence of using Onyx as storage here. Do you have some thoughts on that?

aldo-expensify · 2023-03-09T01:37:14Z

The PR adding the ContactMehodDetailsPage got reverted here: #15761

parasharrajat · 2023-03-09T02:43:29Z

I think we can keep the transitionEnd event handler in screenWrapper component which is responsible for didTransitionEnd. I am only thinking to move the isNavigaying flag setter to high level. Thus both level the AuthScreens and screenWrapper will have event listener.

Beamanator · 2023-03-09T16:21:17Z

This seems to work quite well, I don't think I've ever noticed the loading indicator, have you @aldo-expensify ?

Beamanator · 2023-03-09T16:53:28Z

FYI I re-added pretty much everything from my old PR into this one: #15777 just to see if I could reproduce the bug and I haven't been able to, would either of y'all (@aldo-expensify @parasharrajat ) mind trying to reproduce on that PR?

aldo-expensify · 2023-03-09T17:51:19Z

This seems to work quite well, I don't think I've ever noticed the loading indicator, have you @aldo-expensify ?

not really, it takes you our of the view fast if the login data is not there, but I put it just in case somethings makes it take longer.

aldo-expensify · 2023-03-09T17:51:28Z

FYI I re-added pretty much everything from my old PR into this one: #15777 just to see if I could reproduce the bug and I haven't been able to, would either of y'all (@aldo-expensify @parasharrajat ) mind trying to reproduce on that PR?

I'll give it a try

aldo-expensify · 2023-03-09T17:51:45Z

I think we can keep the transitionEnd event handler in screenWrapper component which is responsible for didTransitionEnd. I am only thinking to move the isNavigaying flag setter to high level. Thus both level the AuthScreens and screenWrapper will have event listener.

I see, that should be much easier, I think we should do that.

…phone-number

parasharrajat · 2023-03-28T18:31:57Z

Present on Main as well.

BUG: Web: Remove Confirm modal position is wrong.

parasharrajat · 2023-03-28T18:47:53Z

BUG: Web: While offline the verify button is interacting even though it is disabled visually.

Step:
2. Put the device offline.
3. Go to the existing phone number contact.
4. Add a magic code and verify.
5. Go back to the contact details.
5. Notice that verify button is visually disabled but you can still interact with it or resubmit.

screen-2023-03-29_00.15.16.mp4

parasharrajat · 2023-03-28T18:48:22Z

@aldo-expensify Can you please look into the above bugs so we can speed up this PR review?

parasharrajat · 2023-03-28T19:21:21Z

cc: @Beamanator ⬆️

parasharrajat · 2023-03-28T22:14:48Z

Screenshots

🔲 iOS / native

screen-2023-03-29_04.01.20.mp4

🔲 iOS / Safari

screen-2023-03-29_04.05.07.mp4

🔲 MacOS / Desktop

screen-2023-03-29_04.07.52.mp4

🔲 MacOS / Chrome

screen-2023-03-29_03.25.41.mp4

🔲 Android / Chrome

screen-2023-03-29_03.42.19.mp4

🔲 Android / native

screen-2023-03-29_03.27.47.mp4

parasharrajat · 2023-03-28T22:53:13Z

For Notes

Bug: Following platforms do not submit the verify magic code from on press of the done/tick button on the Virtual keyboard.

iOS Native.
iOS Safari.
Android Native.

#15751 (comment) (Confirmation required).

parasharrajat

There are a few minor bugs that does not affect the main functionality and can be solved in follow-up PR. I think this PR is urgent so let's merge it.

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

cc: @Beamanator @neil-marcellini @cristipaval

🎀 👀 🎀 C+ reviewed

parasharrajat · 2023-03-28T23:02:02Z

Should I report the above bugs on Slack or we will handle them in follow-ups?

aldo-expensify · 2023-03-29T10:01:30Z

Present on Main as well.

BUG: Web: Remove Confirm modal position is wrong.

this one looks very bad, I'll fix it now.

aldo-expensify · 2023-03-29T10:02:11Z

Present on Main as well.
BUG: Web: Remove Confirm modal position is wrong.

this one looks very bad, I'll fix it now.

nevermind, that is also in staging

Beamanator · 2023-03-29T10:32:10Z

@parasharrajat thanks for finding a few follow-up bugs we need to handle! I'd say we don't need to report them in slack, we can handle them in follow-ups in one of the related issues 👍 I'll probably create a new issue for the follow-ups

neil-marcellini

NAB, let's tackle these in a follow up, but here are a couple things that I find odd.

We should definitely center the confirmation modal for removing a contact method
Why aren't we using the Form component? We should use it for every form and if it doesn't work then we should modify it.
Which offline UX pattern are we using for adding a phone number? I ask because when I click "Send validation" I have no feedback that anything happened. IMO that's not acceptable.

Let's merge this thing!

OSBotify · 2023-03-29T11:08:45Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

Beamanator · 2023-03-29T11:12:55Z

Thanks @neil-marcellini 👍 Yes I agree about those follow-ups, I will start some convos related to those in a few hours 💪

OSBotify · 2023-03-29T20:23:25Z

🚀 Deployed to staging by https://github.com/neil-marcellini in version: 1.2.92-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-03-31T11:09:06Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.92-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

sobitneupane · 2023-06-13T05:00:13Z

This issue was not considered in this PR.

Issue: Contact Method - Validation error message doesn't disappear upon requesting new magic code

fedirjh · 2023-07-24T11:14:48Z

Hi there, we have a minor bug in IOS native. When the user clicks on the remove button, the keyboard reopens for a brief second after navigation.

ikevin127 · 2024-08-20T18:02:16Z

src/libs/actions/User.js

+ * @param {String} contactMethod - The contact method the user is trying to verify
+ * @param {String} validateCode
+ */
+function validateSecondaryLogin(contactMethod, validateCode) {


Looks like the validateSecondaryLogin was missing the successData data for the case where, when we have a non-validated account, add a secondary login, and then verify the secondary login, the BE actually sets the secondary login as the primary login.

It can be verified from an email received after verifying, telling that the primary login has been changed to the secondary login. So, when we try to set the secondary login as the primary login, the BE responds with a message that the secondary login is already the primary login.

Leading to this issue:

[C+ Checklist Needs Completion] [$250] Contact method - Secondary contact method can not select Set as default #46350

which was fixed by PR:

When verifying secondary login, set it as the primary login if primary login isn't validated yet #47132

more details on the fix approach can be found in the proposal.

No need to call Navigate.navigate

4237497

aldo-expensify requested a review from a team as a code owner March 8, 2023 19:37

aldo-expensify self-assigned this Mar 8, 2023

melvin-bot bot requested review from neil-marcellini and parasharrajat and removed request for a team March 8, 2023 19:38

aldo-expensify marked this pull request as draft March 8, 2023 19:46

Redirect on componentDidUpdate instead of render

99a10ac

aldo-expensify marked this pull request as ready for review March 8, 2023 20:19

aldo-expensify marked this pull request as draft March 9, 2023 01:36

Add everything back that got reverted

a2056b5

Beamanator added 2 commits March 10, 2023 12:20

Fix lint and missing LoginField

05274b2

Merge branch 'main' of github.com:Expensify/App into aldo_fix-delete-…

46b9f43

…phone-number

parasharrajat approved these changes Mar 28, 2023

View reviewed changes

cristipaval approved these changes Mar 29, 2023

View reviewed changes

neil-marcellini approved these changes Mar 29, 2023

View reviewed changes

neil-marcellini merged commit 195482f into main Mar 29, 2023

neil-marcellini deleted the aldo_fix-delete-phone-number branch March 29, 2023 11:04

Beamanator mentioned this pull request Mar 29, 2023

[Tracker] Contact Method Flows Polish #16666

Closed

22 tasks

NikkiWines mentioned this pull request Mar 29, 2023

Add secondary login validation by magic code #15793

Closed

55 tasks

OSBotify mentioned this pull request Mar 29, 2023

Deploy Checklist: New Expensify 2023-03-29 #16701

Closed

21 tasks

MelvinBot mentioned this pull request Mar 31, 2023

[HOLD for payment 2023-04-07] [HOLD for payment 2023-03-17] Web - Profile - If user deletes the number without validating, page becomes unresponsive #15725

Closed

6 tasks

dhairyasenjaliya mentioned this pull request Apr 21, 2023

[HOLD for payment 2023-05-05] [$1000] Settings - Contact method menu item right arrow icon is not vertically centre align #17756

Closed

6 tasks

sobitneupane mentioned this pull request Jun 13, 2023

[HOLD for payment 2023-06-13] [$1000] Contact Method - Validation error message doesn't disappear upon requesting new magic code #19471

Closed

6 tasks

fedirjh mentioned this pull request Jul 24, 2023

[HOLD for payment 2023-07-17] [$1000] IOS - Contact Method - keyboard open again for a second when user click on remove button #20608

Closed

6 tasks

ikevin127 reviewed Aug 20, 2024

View reviewed changes

ikevin127 mentioned this pull request Aug 20, 2024

[C+ Checklist Needs Completion] [$250] Contact method - Secondary contact method can not select Set as default #46350

Closed

6 tasks

Fix stuck navigation & Re-add Contact Method Details pages #15751

Fix stuck navigation & Re-add Contact Method Details pages #15751

Conversation

aldo-expensify commented Mar 8, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

MelvinBot commented Mar 8, 2023

aldo-expensify commented Mar 8, 2023

aldo-expensify commented Mar 8, 2023

aldo-expensify commented Mar 8, 2023 • edited Loading

aldo-expensify commented Mar 8, 2023 • edited Loading

parasharrajat commented Mar 8, 2023 • edited Loading

aldo-expensify commented Mar 8, 2023

aldo-expensify commented Mar 8, 2023

parasharrajat commented Mar 8, 2023

aldo-expensify commented Mar 9, 2023

aldo-expensify commented Mar 9, 2023

parasharrajat commented Mar 9, 2023 • edited Loading

Beamanator commented Mar 9, 2023

Beamanator commented Mar 9, 2023

aldo-expensify commented Mar 9, 2023

aldo-expensify commented Mar 9, 2023

aldo-expensify commented Mar 9, 2023

parasharrajat commented Mar 28, 2023 • edited Loading

parasharrajat commented Mar 28, 2023 • edited Loading

parasharrajat commented Mar 28, 2023

parasharrajat commented Mar 28, 2023

parasharrajat commented Mar 28, 2023 • edited Loading

Screenshots

🔲 iOS / native

🔲 iOS / Safari

🔲 MacOS / Desktop

🔲 MacOS / Chrome

🔲 Android / Chrome

🔲 Android / native

parasharrajat commented Mar 28, 2023 • edited Loading

For Notes

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

Reviewer Checklist

parasharrajat commented Mar 28, 2023

aldo-expensify commented Mar 29, 2023

aldo-expensify commented Mar 29, 2023

Beamanator commented Mar 29, 2023

neil-marcellini left a comment

Choose a reason for hiding this comment

OSBotify commented Mar 29, 2023

Beamanator commented Mar 29, 2023

OSBotify commented Mar 29, 2023

OSBotify commented Mar 31, 2023

sobitneupane commented Jun 13, 2023

fedirjh commented Jul 24, 2023

ikevin127 Aug 20, 2024

Choose a reason for hiding this comment

aldo-expensify commented Mar 8, 2023 •

edited

Loading

aldo-expensify commented Mar 8, 2023 •

edited

Loading

aldo-expensify commented Mar 8, 2023 •

edited

Loading

parasharrajat commented Mar 8, 2023 •

edited

Loading

parasharrajat commented Mar 9, 2023 •

edited

Loading

parasharrajat commented Mar 28, 2023 •

edited

Loading

parasharrajat commented Mar 28, 2023 •

edited

Loading

parasharrajat commented Mar 28, 2023 •

edited

Loading

parasharrajat commented Mar 28, 2023 •

edited

Loading

parasharrajat left a comment •

edited

Loading