Prefer magic link to password under a configuration #662

jaclync · 2022-08-10T09:25:05Z

In WCiOS, we're experimenting with emphasizing the magic link in place of the password because the magic link has a much higher sign-in rate. In order not to break other apps like WPiOS, the changes are under a new configuration WordPressAuthenticatorConfiguration.isWPComMagicLinkPreferredToPassword that is default to false.

Design hHmVq03kKpvTDzV3uV4wqw-fi-14%3A2092

Notes on the code changes

Sorry about the slightly 500+ diffs 🙇🏻‍♀️ the main changes are on the new view controller MagicLinkRequestedViewController and its xib.

I decided to implement the new screen MagicLinkRequestedViewController with a lighter weight xib than a storyboard. I ran into some rabbit holes trying to use the shared button styles with NUXButton, which is initialized with a Storyboard. After some trials and errors, I was able to use NUXButtonViewController as a child view controller in a container view. Then I ran into a problem with the "Use password to sign in" CTA because it's using link style, and the WPAuthenticator doesn't have a shared style for it. After all, I implemented the primary CTA with NUXButtonViewController and the "Use password to sign in" CTA with a UIButton and a new extension applyLinkButtonStyle similar to WCiOS.

Notes on the UX

Right now, after requesting a magic link successfully from the password screen, it still navigates to the pre-existing magic link requested screen LoginMagicLinkViewController. I'm going to confirm with design later, because otherwise it feels like an infinite loop between the password and the new magic link requested screen.

Testing steps

Please refer to the testing steps in the WCiOS PR.

Example screenshots

light	dark

tablet:

…h password.

…credentials verify email flows.

selanthiraiyan

Great work introducing a coordinator. 🙇

It works great.

I left a few questions and non-blocking nits.

selanthiraiyan · 2022-08-11T03:48:42Z

WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift


    /// Show the Password entry view.
    ///
    func showPasswordView() {


Nit: Now that the method doesn't always show the password view, would it make sense to rename it and update the comments?

Question: This method is also being called from here. I haven't tested that flow. But, do we want the magic link behaviour there as well?

great question and suggestion! the SIWA flow that triggered the password view isn't easily testable, so I reverted the changes on this in 4b97bc5

selanthiraiyan · 2022-08-11T04:13:45Z

WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift

+                                              loginFields: loginFields,
+                                              tracker: tracker,
+                                              configuration: WordPressAuthenticator.shared.configuration)
+        passwordCoordinator = coordinator


❓ Why are we storing this in passwordCoordinator?

I tried removing this and noticed that we need this here

Instead of asking PasswordCoordinator to present the password view, can we pass the necessary properties to MagicLinkRequestedViewController and ask it present the screen?

We could even create a separate coordinator just for showing PasswordViewController and store it in MagicLinkRequestedViewController

I am sure that you considered the above and several other options, and I am curious to know your thoughts. 😄

yes exactly, the coordinator needs to be retained during the lifetime of its possible navigation flow. because there's logic about which view controller to show, I thought it's easier to reuse this flow by using a coordinator. we could reuse it in other cases where it makes sense to prefer a magic link to password.

selanthiraiyan · 2022-08-11T04:15:24Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+    }
+
+    override var sourceTag: WordPressSupportSourceTag {
+        .loginMagicLink


Nit:
This being a new screen should we consider adding sourceTag?
This might be helpful when we work on help center experiment. 😅 Internal ref - pe5sF9-n8-p2

sure, added in 31ef4e4

selanthiraiyan · 2022-08-11T04:17:06Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+
+        tracker.set(flow: .loginWithMagicLink)
+
+        if isMovingToParent {


Nit: Kindly consider using isBeingPresentedInAnyWay.

good catch, updated in f40cc75

selanthiraiyan · 2022-08-11T04:21:36Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.xib

@@ -0,0 +1,148 @@
+<?xml version="1.0" encoding="UTF-8"?>


IMHO, it might be helpful to fix these constraints to make changes to this file in future.

This is really tricky, these IB errors are confusing because these 3 views are in a UIStackView and their Y position or height is supposed to be dynamic. The actual layout was all fine from my testing on various font sizes in different simulators, the scroll view is scrollable on larger font sizes. I ended up adding minimum placeholder constraints to be removed at build time in 51f90cd, please let me know if you have better suggestions on this Auto Layout challenge 🙇🏻‍♀️

selanthiraiyan · 2022-08-11T04:24:03Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+    @IBOutlet private weak var emailLabel: UILabel!
+    @IBOutlet private weak var cannotFindEmailLabel: UILabel!
+    @IBOutlet weak var buttonContainerView: UIView!
+    @IBOutlet weak var loginWithPasswordButton: UIButton!


Nit: We could make buttonContainerView and loginWithPasswordButton private as well.

Updated in 6ff631a

selanthiraiyan · 2022-08-11T04:25:09Z

WordPressAuthenticator/Unified Auth/View Related/Password/PasswordCoordinator.swift

+    }
+
+    func start() async {
+        if configuration.isWPComMagicLinkPreferredToPassword {


Super nit: What do you think about using guard?

Since this is an experiment, I don't think the magic link preferred flow is the assumed path. It's also a configuration that WPiOS isn't adopting yet.

…acking.

…uesting a magic link.

…WPComLogin`.

…om views in `UIStackView` with dynamic height.

jaclync

Thank you for the review and for catching a few things @selanthiraiyan! I responded to your questions in woocommerce/woocommerce-ios#7449 (review) to consolidate all the responses in the same PR 🙂 See if you'd like to take another look or I'll merge it early Friday, I tested a few more times.

In the Login with site address flow

After the following step,

Enter a valid non-A8C WP.com email and tap "Continue" --> after a little bit (requesting magic link), a new magic link requested screen should be shown with two CTAs at the bottom

the below event is tracked,

Tracked unified_login_interaction, properties: [AnyHashable("click"): "request_magic_link", AnyHashable("step"): "start", AnyHashable("flow"): "login_site_address", AnyHashable("source"): "default"]

Would it be alright to track a click property even though the user didn't manually request magic link?

Thanks for catching this logging mistake, this click event shouldn't have been tracked. I removed this in 2cb71a6

❓ Just curious
Do you have a sequence of analytics events planned to measure the success of this iteration? (To compare with other iterations and etc)

According to the metric for this experiment/iteration in pe5sF9-iy-p2, we want to measure the % of users that enter the magic link login flow and end up successfully logging in goes up.

In this PR, I added a new event to track this step in f40cc75:

🔵 Tracked unified_login_step, properties: [AnyHashable("source"): "default", AnyHashable("flow"): "login_magic_link", AnyHashable("step"): "magic_link_auto_requested"]

so we can measure the successful sign-in rate in a funnel with this new step.

jaclync · 2022-08-11T09:13:24Z

WordPressAuthenticator/Unified Auth/View Related/Password/PasswordCoordinator.swift

+    }
+
+    func start() async {
+        if configuration.isWPComMagicLinkPreferredToPassword {


Since this is an experiment, I don't think the magic link preferred flow is the assumed path. It's also a configuration that WPiOS isn't adopting yet.

jaclync · 2022-08-11T09:17:55Z

WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift

+                                              loginFields: loginFields,
+                                              tracker: tracker,
+                                              configuration: WordPressAuthenticator.shared.configuration)
+        passwordCoordinator = coordinator


yes exactly, the coordinator needs to be retained during the lifetime of its possible navigation flow. because there's logic about which view controller to show, I thought it's easier to reuse this flow by using a coordinator. we could reuse it in other cases where it makes sense to prefer a magic link to password.

jaclync · 2022-08-11T13:33:13Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.xib

@@ -0,0 +1,148 @@
+<?xml version="1.0" encoding="UTF-8"?>


This is really tricky, these IB errors are confusing because these 3 views are in a UIStackView and their Y position or height is supposed to be dynamic. The actual layout was all fine from my testing on various font sizes in different simulators, the scroll view is scrollable on larger font sizes. I ended up adding minimum placeholder constraints to be removed at build time in 51f90cd, please let me know if you have better suggestions on this Auto Layout challenge 🙇🏻‍♀️

jaclync · 2022-08-11T13:33:53Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+    @IBOutlet private weak var emailLabel: UILabel!
+    @IBOutlet private weak var cannotFindEmailLabel: UILabel!
+    @IBOutlet weak var buttonContainerView: UIView!
+    @IBOutlet weak var loginWithPasswordButton: UIButton!


Updated in 6ff631a

jaclync · 2022-08-11T13:34:59Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+    }
+
+    override var sourceTag: WordPressSupportSourceTag {
+        .loginMagicLink


sure, added in 31ef4e4

jaclync · 2022-08-11T13:38:26Z

WordPressAuthenticator/Unified Auth/View Related/Get Started/GetStartedViewController.swift


    /// Show the Password entry view.
    ///
    func showPasswordView() {


great question and suggestion! the SIWA flow that triggered the password view isn't easily testable, so I reverted the changes on this in 4b97bc5

jaclync · 2022-08-11T13:39:13Z

WordPressAuthenticator/Unified Auth/View Related/Login/MagicLinkRequestedViewController.swift

+
+        tracker.set(flow: .loginWithMagicLink)
+
+        if isMovingToParent {


good catch, updated in f40cc75

selanthiraiyan · 2022-08-12T02:54:07Z

Thanks for making the changes Jaclyn. This looks great now. 🚀

jaclync added 7 commits August 10, 2022 16:56

Add a configuration to prefer magic link to password.

f8da05a

Add email icon PDF asset.

7c67052

Create a new magic link requested screen with an action to log in wit…

5fe61e6

…h password.

Use PasswordCoordinator for navigation for the password flow.

2d5d00c

Bump pod version.

5c420a2

Revert changes on auto requesting magic link in Google auth and site …

02e6716

…credentials verify email flows.

Update magic link requested title to use an existing title3 font.

74830ef

jaclync mentioned this pull request Aug 10, 2022

Prefer magic link to password under a feature flag woocommerce/woocommerce-ios#7449

Merged

3 tasks

jaclync requested review from itsmeichigo and selanthiraiyan August 11, 2022 00:31

selanthiraiyan self-assigned this Aug 11, 2022

selanthiraiyan approved these changes Aug 11, 2022

View reviewed changes

jaclync added 9 commits August 11, 2022 17:03

MagicLinkRequestedViewController: use a new source tag for support tr…

31ef4e4

…acking.

MagicLinkRequestedViewController: track with a new step.

f40cc75

Remove tracking requestMagicLink click event when automatically req…

2cb71a6

…uesting a magic link.

Merge branch 'trunk' into wcios/7437-magic-link-auto-sent

1be7afd

Bump pod version after merging trunk.

ba6e77c

Keep the same password only flow for `AppleAuthenticatorDelegate.show…

4b97bc5

…WPComLogin`.

Update IBOutlets to private.

6ff631a

Update view frames in the xib.

eff81a2

Add placeholder Auto Layout constraints only to suppress IB issues fr…

51f90cd

…om views in `UIStackView` with dynamic height.

jaclync commented Aug 11, 2022

View reviewed changes

jaclync merged commit b20a8de into trunk Aug 12, 2022

jaclync deleted the wcios/7437-magic-link-auto-sent branch August 12, 2022 02:59

Prefer magic link to password under a configuration #662

Prefer magic link to password under a configuration #662

Uh oh!

Conversation

jaclync commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Design hHmVq03kKpvTDzV3uV4wqw-fi-14%3A2092

Notes on the code changes

Notes on the UX

Testing steps

Example screenshots

Uh oh!

selanthiraiyan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaclync left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selanthiraiyan commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jaclync commented Aug 10, 2022 •

edited

Loading