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

Post-Signup Interstitial: Display interstitial screen #10976

Conversation

renanferrari
Copy link
Member

@renanferrari renanferrari commented Dec 17, 2019

Fixes #10969

For this PR, I have made the following changes:

  • Created a new preference called SHOULD_SHOW_POST_SIGNUP_INTERSTITIAL to control whether the screen has already been shown or not.
    • Created a new UndeletablePrefKey, since we want the screen to be shown only once per install.
    • It has a default value of true, which is changed to false when the interstitial is created.
  • Created a new method in the ActivityLauncher class to launch the interstitial activity.
    • It uses the TaskStackBuilder class and the FLAG_ACTIVITY_CLEAR_TOP to guarantee the interstitial activity will always be launched on top of WPMainActivity.
  • Updated LoginActivity and SignupEpilogueActivity with a logic to check for the need to launch the interstitial activity.
    • Since we need to check if the user has sites, a SiteStore instance is now injected into those classes.
    • LoginActivity also checks for the newly created preference.
    • Since this logic is pretty simple, I tried to avoid early abstractions.
  • Updated the UI tests for the signup flow, since it needed to account for the interstitial.
Signup Login
signup login

To test

1. Signup

  1. Clear app data.
  2. On the initial screen, tap Sign up for WordPress.com and Sign up with email.
  3. Enter an email address that is not associated with a WordPress.com account.
  4. Complete the signup flow.
  5. Confirm that the post-signup interstitial screen is shown.

2. Login (account without sites)

  1. Clear app data.
  2. On the initial screen, tap Log in.
  3. Enter an email address that is associated with an account that doesn't have any sites.
  4. Complete the login flow.
  5. Confirm that the login epilogue screen is not shown.
  6. Confirm that the post-signup interstitial screen is shown instead.

3. Login (account with sites)

  1. Clear app data.
  2. On the initial screen, tap Log in.
  3. Enter an email address that is associated with an account that has at least one site.
  4. Complete the login flow.
  5. Confirm that the post-signup interstitial screen is not shown.
  6. Confirm that the login epilogue screen is shown instead.

4. Once per app install

  1. Complete test 2 and sign out.
  2. Repeat test 2 without clearing the app data.
  3. Confirm that the post-signup interstitial screen is not shown.

5. Site creation and site connection flows

  1. Complete test 1 or 2.
  2. On the post-signup interstitial screen, tap either Create WordPress.com Site or Add Self-Hosted Site.
  3. Complete the flow.
  4. Confirm that the main screen is shown.

Notes

  • LoginEpilogueActivity seems to not only be shown at the end of the login flow itself, but also in other flows related to Jetpack and reauthentication.
    • Because of that, I initially thought of an edge case where I would need to make sure an already authenticated user with no sites wouldn't see the interstitial screen from one of these flows.
    • After a bit of investigation, this seems like it wouldn't be needed. The Jetpack flow seems to only happen when the user already has a site and the interstitial screen doesn't seem to be out of place for the reauthentication flow.
  • I’m assuming the only behavior I need to guarantee upon completion (successfully or not) of either the site creation or site connection flow is that the user will be redirected to WPMainActivity.
  • I haven't felt the need to implement a ViewModel for this screen yet. I might do that after we start tracking the screen events as we may need some unit tests then.

PR submission checklist:

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

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 17, 2019

Messages
📖

This PR contains changes in the subtree libs/mocks/. It is your responsibility to ensure these changes are merged back into wordpress-mobile/WordPressMocks. Follow these handy steps!
WARNING: Make sure your git version is 2.19.x or lower - there is currently a bug in later versions that will corrupt the subtree history!

  1. cd WordPress-Android
  2. git checkout issue/10969-display-interstitial-screen
  3. git subtree push --prefix=libs/mocks/ https://github.com/wordpress-mobile/WordPressMocks.git merge/WordPress-Android/10976
  4. Browse to https://github.com/wordpress-mobile/WordPressMocks/pull/new/merge/WordPress-Android/10976 and open a new PR.

Generated by 🚫 dangerJS

@maxme maxme self-requested a review December 19, 2019 08:43
@maxme maxme added the Signup label Dec 19, 2019
@maxme maxme added this to the 14.0 milestone Dec 19, 2019
Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

I added a few inline comments, and I want to add the following:

@renanferrari
Copy link
Member Author

renanferrari commented Dec 19, 2019

I wonder if we should update the button styling, current style looks a bit off to me. Not directly related to this PR, but we could update the code in here. cf. #10933 (comment)

The design spec for the button styling used to be like the one from the original blueprint, but it has been updated to the one you see implemented here. I think this change was made in preparation for dark mode, but I could be wrong. Maybe @SylvesterWilmott can give us a bit of context about this as well.

There is an "end screen" at the end of the sign in step. I think we should remove it and only have the new interstitial for users who don't have any wordpress.com sites. See how weird the UX looks currently: https://bia.is/s/bez8/signin-interstitial.webm

You're right, it does look weird. I think we can skip the LoginEpilogueActivity entirely for users that don't have any sites by calling the interstitial here.

@SylvesterWilmott
Copy link

The design spec for the button styling used to be like the one from the original blueprint, but it has been updated to the one you see implemented here. I think this change was made in preparation for dark mode, but I could be wrong. Maybe @SylvesterWilmott can give us a bit of context about this as well.

The pink colour is reserved for primary actions and should be used for one element on a screen. Since neither of the buttons in the interstitial is a primary action, both use the secondary style.

There is an "end screen" at the end of the sign in step. I think we should remove it and only have the new interstitial for users who don't have any wordpress.com sites. See how weird the UX looks currently: https://bia.is/s/bez8/signin-interstitial.webm

Agree

@renanferrari
Copy link
Member Author

renanferrari commented Dec 19, 2019

There is an "end screen" at the end of the sign in step. I think we should remove it and only have the new interstitial for users who don't have any wordpress.com sites. See how weird the UX looks currently: https://bia.is/s/bez8/signin-interstitial.webm

I have replaced LoginEpilogueActivity entirely for users that don't have any sites. Here's how it looks now:

Login (without sites) Login (with sites)
without sites with sites

Note: the empty login epilogue screen will still appear at the end of alternative login flows, like the one for deep links or reauthentication, for example. I personally don't think that's a problem, as the context is pretty different in these cases.

@maxme
Copy link
Contributor

maxme commented Dec 20, 2019

The pink colour is reserved for primary actions and should be used for one element on a screen. Since neither of the buttons in the interstitial is a primary action, both use the secondary style.

Perfect, thanks for confirming.

@maxme maxme self-requested a review December 20, 2019 09:45
Copy link
Contributor

@maxme maxme left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@maxme maxme merged commit acbf579 into wordpress-mobile:feature/10918-post-signup-interstitial Dec 20, 2019
@renanferrari
Copy link
Member Author

We had a bug with this implementation for login flows using Magic Link. It was later documented in this issue.

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.

3 participants