Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move addMenuProvider from onCreate to onViewCreated #142

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

ThomazFB
Copy link
Contributor

@ThomazFB ThomazFB commented Feb 20, 2024

Summary

Fix issue wordpress-mobile/WordPress-Android#20221 by adjusting the point where the menu provider added to the SignupConfirmationFragment view.

How to Test

  1. Import this PR to the WordPress or Woo app by using the 142-48e7fc1f82f011c246e23ac4b4a0c9195b3d7c21 version hash.
  2. Start the signup flow using an email account not registered on WordPress.com.
  3. Verify that during the Signup process, when landing in the view with the We’ll use this email address to create your new WordPress.com account (the SignupConfirmationFragment), the view loads as expected.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@thomashorta
Copy link
Contributor

Just a side note regarding the WordPress and Jetpack apps flow: it only opens the SignupConfirmationFragment if the user tries to do a Social Login (Login with Google) and the email is not yet registered on WordPress.com so the testing steps are slightly different for those apps:

  1. Import this PR to the WordPress app by using the 142-48e7fc1f82f011c246e23ac4b4a0c9195b3d7c21 version hash.
  2. Start the login/signup flow by tapping "Continue with WordPress.com".
  3. Use social login/signup by tapping "Continue with Google".
  4. Select an email that is not associated with a WordPress.com account.
  5. Verify that during the Signup process, when landing in the view with the "We’ll use this email address to create your new WordPress.com account" (the SignupConfirmationFragment), the view loads as expected.

@zwarm
Copy link
Contributor

zwarm commented Feb 21, 2024

Just a side note regarding the WordPress and Jetpack apps flow: it only opens the SignupConfirmationFragment if the user tries to do a Social Login (Login with Google) and the email is not yet registered on WordPress.com so the testing steps are slightly different for those apps:

  1. Import this PR to the WordPress app by using the 142-48e7fc1f82f011c246e23ac4b4a0c9195b3d7c21 version hash.
  2. Start the login/signup flow by tapping "Continue with WordPress.com".
  3. Use social login/signup by tapping "Continue with Google".
  4. Select an email that is not associated with a WordPress.com account.
  5. Verify that during the Signup process, when landing in the view with the "We’ll use this email address to create your new WordPress.com account" (the SignupConfirmationFragment), the view loads as expected.

I didn't think you can use Google Sign in with a debug build? @thomashorta how did you test this for Jetpack?

@thomashorta
Copy link
Contributor

thomashorta commented Feb 21, 2024

I didn't think you can use Google Sign in with a debug build? @thomashorta how did you test this for Jetpack?

Yeah, that's correct, I was just trying to test the steps that I wrote but I was only able to do them to confirm the crash exists using the release beta build. I wasn't able to do them to confirm the crash was fixed. 😞

I think that, for testing purposes, I will just modify the gotUnregisteredEmail (here) to call gotUnregisteredSocialAccount, so it will open the SignupConfirmationFragment when trying to use a new email.

I didn't test it yet though.

@zwarm
Copy link
Contributor

zwarm commented Feb 21, 2024

I think that, for testing purposes, I will just modify the gotUnregisteredEmail to call gotUnregisteredSocialAccount, so it will open the SignupConfirmationFragment when trying to use a new email.

I didn't test it yet though.

I started testing using the original steps, then saw the new ones. We can create a local release build for WP, but not JP (because it uses Google signing). I'll give that a go next

Copy link
Contributor

@zwarm zwarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomazFB
I have confirmed the fix has worked using the secondary set of testing instructions provided by @thomashorta . Using a locally built release version of WP I was able to (1) recreate the crash (2) verify this PR fixes the crash.

Let me know how I can assist further.

Copy link
Contributor

@thomashorta thomashorta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started testing using the original steps, then saw the new ones. We can create a local release build for WP, but not JP (because it uses Google signing). I'll give that a go next

Good point @zwarm. I was able to locally build a WordPress Vanilla Release build and confirm this PR fixes the crash and correctly shows the SignupConfirmationFragment when doing the Google Sign Up flow.

The code also looks good to me! Thanks @ThomazFB!

Copy link

@atorresveiga atorresveiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ThomazFB I tested the changes and on the Woo app, when trying to use an email that is not associated with a WordPress.com account, the app fails with a Toast message.

LGTM! :shipit:

@ThomazFB
Copy link
Contributor Author

Thank you, @thomashorta, @zwarm, and @atorresveiga, for the reviews! As we have the WP and Woo scenarios covered with the tests, we can move forward by merging this PR. I'll proceed with the new version release.

@ThomazFB ThomazFB merged commit 0e9b52c into trunk Feb 21, 2024
8 checks passed
@ThomazFB ThomazFB deleted the fix/signup-viewlifecycle-crash branch February 21, 2024 21:30
wzieba pushed a commit that referenced this pull request Oct 15, 2024
…e-crash

Move `addMenuProvider` from `onCreate` to `onViewCreated`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants