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

Fix post-signup interstitial for login with magic link #11020

Conversation

renanferrari
Copy link
Member

@renanferrari renanferrari commented Dec 23, 2019

Fixes #11019

This PR adds a functionality for showing the post-signup interstitial after the login epilogue screen.

In #10976 we tried to completely replace the login epilogue screen for the interstitial screen at the end of the login flow when the user had no sites, to avoid showing an empty epilogue screen. The assumption was that we would have all relevant login flows covered by adding the following logic inside LoginActivity:

if (!mSiteStore.hasSite() && AppPrefs.shouldShowPostSignupInterstitial()) {
ActivityLauncher.showPostSignupInterstitial(this);
} else {
ActivityLauncher.showMainActivityAndLoginEpilogue(this, oldSitesIds, doLoginUpdate);
}
setResult(Activity.RESULT_OK);
finish();

However, when using a Magic Link, the login epilogue screen is launched from the main activity:

ActivityLauncher.showLoginEpilogue(this, true,
getIntent().getIntegerArrayListExtra(ARG_OLD_SITES_IDS));

This caused the interstitial screen not being shown at all for logins using Magic Link.

A naive approach to this problem would be to replicate the logic above that we used on LoginActivity to the main activity as well, but that wouldn't work as we wanted because, when launched from the main activity, the LoginEpilogueActivity receives an argument doLoginUpdate of true and is responsible to fetch the user account and settings (including sites) itself. This meant we couldn't perform a check to determine if the user had sites, which in turn meant we couldn't determine if it would be possible to replace the empty login epilogue screen for the interstitial screen like we do in the other cases.

After discussing a few options with @maxme, we've decided to go with the solution proposed here, where we'll try to replace the login epilogue screen for the interstitial where possible and let the login epilogue screen be followed by the interstitial where not.

So for example, at the end of the email/password login flow, an user with no sites would not see the login epilogue screen at all. Instead, they would only see the interstitial screen. As for the Magic Link flow, an user with no sites would still see the login epilogue screen, but it would be followed by the interstitial.

This is illustrated below:

Email/Password Flow Magic Link Flow
email/password flow magic link flow

To test

  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 using a magic link.
  5. On the login epilogue screen, tap Continue.
  6. Confirm that the post-signup interstitial screen is shown.

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.

@renanferrari renanferrari changed the title Implementing Post-Signup Interstitial for login with magic link Fix post-signup interstitial for login with magic link Dec 23, 2019
@maxme maxme self-requested a review December 23, 2019 16:37
@maxme maxme self-assigned this Dec 23, 2019
@maxme maxme added the Signup label Dec 23, 2019
@maxme maxme added this to the 14.0 milestone Dec 23, 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.

LGTM, :shipit:

@maxme maxme merged commit c3e6e60 into wordpress-mobile:feature/10918-post-signup-interstitial Dec 24, 2019
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.

None yet

2 participants