Shorter sign up - remove Send Link by email screen #14025

ParaskP7 · 2021-02-10T10:26:15Z

Fixes #13971
Relates WordPressAuthenticator-iOS#574

This PR removes the below intermediate Sign Up screen:

Instead, an email will be now sent immediately after the user enters their email:

screen-20210210-120918.mp4

ℹ️ An additional paragraph was added on the confirmation screen stating the: Didn't mean to create a new account? Go back to re-enter your email address.

⚠️ Since the gotUnregisteredEmail(...) functionality is shared between the WordPress and WooCommerce apps, this was not removed altogether. Instead the implementing client just end up doing nothing as an implementation on the WordPress side.

To test:

Verify that the login and signup flows are working as expected, with no issue and with the above flow improvement.
Check tracking is as expected.

PR submission checklist:

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

This change effectively removes the intermediate 'Send link by email' screen. This screen is actually a dual purpose screen because it is also used for with social login (Google). As such, the screen is not removed per se, but actually becomes now a single purpose screen to be used only for the purpose of logging in with Google.

This extra message will let the user know they can go back if they didn't intend to sign up.

1) This screen no longer needs the extra 'newInstance(email)' method, which was only used within the deleted 'gotUnregisteredEmail(...)' method. 2) As such the 'mIsSocialSignup' and 'ARG_IS_SOCIAL_SIGNUP' logic is no longer needed and deleted as well. 3) The tracking of 'trackEmailSignupConfirmationViewed()' is also no needed based on the above changes. 4) Finally, the XML file can be updated to account only for the social logic (Google). 5) And as part of that last change, the 'signup_confirmation_magic_link_message' string becomes obsolete as well.

This 'gotUnregisteredEmail(...)' method is indeed now unused for the 'WordPress' app, but not for the 'WooCommerce' app. As such, this is added back as an interface method to be shared with both apps. The only difference is that for the 'WordPress' app it will not be doing anything as of now.

This 'gotUnregisteredEmail(...)' method is added back but it will not be doing anything as of now.

peril-wordpress-mobile · 2021-02-10T10:26:21Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-02-10T10:26:24Z

	Fails
🚫	Danger failed to run `/app/danger-0.el80ihdtlh.ts`.

	Messages
📖	This PR contains changes in the subtree `libs/login/`. It is your responsibility to ensure these changes are merged back into `wordpress-mobile/WordPress-Login-Flow-Android`. 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! `cd WordPress-Android` `git checkout issue/13971-send-email-after-email-entered` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/WordPress-Android/14025` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/WordPress-Android/14025 and open a new PR.

Error TypeError

Cannot read property 'added' of null
TypeError: Cannot read property 'added' of null
    at Object.exports.default (/app/danger-0.el80ihdtlh.ts:16:44)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

11|         warn("The PlayStoreStrings.po file must be updated any time changes are made to release notes");
12|     }
13| 
14|     // If changes were made to the strings, make sure they follow our guidelines
15|     const modifiedStrings = modifiedFiles.filter((path: string) => path.endsWith('values/strings.xml'))
----------------------------------------------^
16| 
17|     for (let file of modifiedStrings) {        
18|         const stringDiffs = await danger.git.diffForFile(file)
19|

Generated by 🚫 dangerJS

ParaskP7 · 2021-02-10T10:28:25Z

👋 @anitaa1990 !

Once more, I want to ping you on that change so that you are aware of it as well. After checking with Woo I understand that you are implementing this differently on your side, so I don't think you need to be doing much, but I figured it would be good if you are also aware of this.

peril-wordpress-mobile · 2021-02-10T10:33:15Z

You can test the changes on this Pull Request by downloading the APK here.

ParaskP7 · 2021-02-10T10:35:05Z

👋 @oguzkocer !

When this PR ends up being merged in develop, and since I am having issues pushing changes to the WordPress-Login-Flow-Android , should I be doing it manually once more, creating a manual PR to that repo in order to apply the Login related changes?

PS: Previous manual PR on WordPress-Login-Flow-Android.

oguzkocer · 2021-02-10T20:55:14Z

@ParaskP7 Yes, please go ahead with the manual update.

mattmiklic

Thanks for the ping; everything is looking as it should to me here.

renanferrari

Hey @ParaskP7, thanks for the changes!

I've tested the app and it seems to be working as expected. That said, it looks like this introduces some changes that could impact the Woo app, so unless you’ve already coordinated this with them, you should probably make some considerations, which I’ve detailed below.

libs/login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginEmailFragment.java

renanferrari · 2021-02-10T22:59:22Z

...login/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginAnalyticsListener.kt

@@ -65,7 +65,6 @@ interface LoginAnalyticsListener {
    fun trackPickEmailFromHint()
    fun trackShowEmailHints()
    fun emailFormScreenResumed()
-    fun trackEmailSignupConfirmationViewed()


This is a breaking change to the Woo app. It's not a big deal though, as long as you make sure to highlight it when pushing these changes to the library.

👋 @renanferrari !

Oh you're right, again nice catch! Woo implements this method but as a TODO("Not yet implemented"), which means that when they pull the changes from this library the compiler will complain. I'll revert that change on the library level and have the WordPress app implement it with a \\ Do nothing comment. Wdyt?

This is now done here: bbf2464

In this case I'm wondering if it's not better to actually remove this, since we are effectively removing the "signup confirmation" screen for email signups. And as none of the apps are going to use it, it seems right. What do you think?

👋 @renanferrari !

Do you mean if it's better to actually remove this, without the not, I just got a bit confused? 😅

If this is indeed what you meant, I was thinking along the same lines, but decided that it probably better to not have that removed since on the Woo side it is marked as TODO("Not yet implemented"). We now know that on the WordPress side this is a \\ Do nothing., but I am not sure about Woo.

Also Woo has an intermediate LoginNoWPcomAccountFoundFragment screen as well, which made me think that I shouldn't remove this logic from Woo just yet (it might be of used as some point, or planned).

However, if you really think that it is indeed better if we just remove this, and have Woo update their implementation by removing this as well, that is when they pull the latest changes, then I will confidently do so and I would agree. It is just that I choose the less risky, frictionless path on this instance.

👋 @renanferrari !

As per our discussion, I removed this tracking event, you can check this commit: 5252eda

👋 @anitaa1990 !

Could you also please verify that this is okay on your end?

Many thanks in advance! 🙏

This is done purely because otherwise the behavior for the 'Woo' app will change, which will end up breaking the 'Woo' flows. As such, this commit reverts that and instead updates the 'LoginActivity', which exists only on the 'WordPress' side and makes sure that the implementing 'gotUnregisteredEmail(...)' method calls the 'showSignupMagicLink(...)' instead, thus updating only the 'WordPress' login flow with the intended change.

This is done purely because otherwise this will break the 'Woo' app, which also using this interface method, even if that is currently just a 'TODO("Not yet implemented")'. As such, this commit reverts that and instead updates the implementing method on the 'WordPress' side with a '\\ Do nothing.' comment.

ParaskP7 · 2021-02-11T09:15:31Z

👋 @renanferrari !

So many thanks for the excellent review, especially when it comes to how this will affect Woo! 🙏

I've tested the app and it seems to be working as expected. That said, it looks like this introduces some changes that could impact the Woo app, so unless you’ve already coordinated this with them, you should probably make some considerations, which I’ve detailed below.

You're right! I have made some updates based on your comments, you could please take another look. 🙏

After some more thought, it has been decided to remove this and have 'Woo' update their app when pulling this login library change.

renanferrari

Thanks for the changes. Feel free to merge this once @anitaa1990 verifies everything is ok (I believe it should be).

Great job on this! LGTM

anitaa1990 · 2021-02-15T08:03:11Z

Hey @ParaskP7 👋 Thank you for the ping!

Is this screen removed only during the sign up process? If that is the case, we currently don't support sign ups in the Woo app so you should be good to go 👍

ParaskP7 · 2021-02-15T08:10:22Z

👋 @anitaa1990 !

Is this screen removed only during the sign up process?

Yes, you are right Anitaa, this screen is used during the sign up process.

If that is the case, we currently don't support sign ups in the Woo app so you should be good to go 👍

Perfect, thanks for the confirmation Anitaa, will now merge this PR! 🙏

ParaskP7 · 2021-02-15T09:20:47Z

PS: A manual PR was created (cherry-peeking the commit from this PR) within the WordPress-Login-Flow-Android repo to make sure that this change is replicated within that repo as well.

ParaskP7 added 10 commits February 10, 2021 11:23

Feature: Add extra message to the email confirmation screen

5d292f1

This extra message will let the user know they can go back if they didn't intend to sign up.

Feature: Add extra message to the email confirmation screen

10e917a

This extra message will let the user know they can go back if they didn't intend to sign up.

Refactor: Remove unused got unregistered email login interface method

8c29ab4

Refactor: Remove unused got unregistered email login impl. method

768f0ae

Refactor: Remove unused track email signup confirmation viewed method

6d919d7

Refactor: Remove unused signup confirmation magic link message string

1022d11

Refactor: Add back unused got unregistered email login impl. method

ec20572

This 'gotUnregisteredEmail(...)' method is added back but it will not be doing anything as of now.

ParaskP7 added [Type] Enhancement Login Design Needed A design solution is needed. labels Feb 10, 2021

ParaskP7 added this to the 16.8 milestone Feb 10, 2021

ParaskP7 requested review from renanferrari, mattmiklic, malinajirka and leandroalonso February 10, 2021 10:26

ParaskP7 self-assigned this Feb 10, 2021

mattmiklic approved these changes Feb 10, 2021

View reviewed changes

mattmiklic removed the Design Needed A design solution is needed. label Feb 10, 2021

renanferrari reviewed Feb 10, 2021

View reviewed changes

ParaskP7 added 2 commits February 11, 2021 11:01

ParaskP7 requested a review from renanferrari February 11, 2021 09:15

Refactor: Remove unused track email signup confirmation viewed method

5252eda

After some more thought, it has been decided to remove this and have 'Woo' update their app when pulling this login library change.

renanferrari approved these changes Feb 12, 2021

View reviewed changes

ParaskP7 merged commit 26adf0a into develop Feb 15, 2021

ParaskP7 deleted the issue/13971-send-email-after-email-entered branch February 15, 2021 08:24

ParaskP7 mentioned this pull request Feb 15, 2021

Shorter sign up - remove Send Link by email screen wordpress-mobile/WordPress-Login-Flow-Android#51

Merged

ParaskP7 mentioned this pull request Feb 15, 2021

Fix failing connected tests for sign up tests #14068

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shorter sign up - remove Send Link by email screen #14025

Shorter sign up - remove Send Link by email screen #14025

ParaskP7 commented Feb 10, 2021 •

edited

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited

ParaskP7 commented Feb 10, 2021

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited

ParaskP7 commented Feb 10, 2021

oguzkocer commented Feb 10, 2021

mattmiklic left a comment

renanferrari left a comment

renanferrari Feb 10, 2021

ParaskP7 Feb 11, 2021

ParaskP7 Feb 11, 2021

renanferrari Feb 11, 2021

ParaskP7 Feb 12, 2021

ParaskP7 Feb 12, 2021

ParaskP7 Feb 12, 2021

ParaskP7 commented Feb 11, 2021

renanferrari left a comment

anitaa1990 commented Feb 15, 2021

ParaskP7 commented Feb 15, 2021

ParaskP7 commented Feb 15, 2021

Shorter sign up - remove Send Link by email screen #14025

Shorter sign up - remove Send Link by email screen #14025

Conversation

ParaskP7 commented Feb 10, 2021 • edited

peril-wordpress-mobile bot commented Feb 10, 2021 • edited

peril-wordpress-mobile bot commented Feb 10, 2021 • edited

Error TypeError

Dangerfile

ParaskP7 commented Feb 10, 2021

peril-wordpress-mobile bot commented Feb 10, 2021 • edited

ParaskP7 commented Feb 10, 2021

oguzkocer commented Feb 10, 2021

mattmiklic left a comment

Choose a reason for hiding this comment

renanferrari left a comment

Choose a reason for hiding this comment

renanferrari Feb 10, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 11, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 11, 2021

Choose a reason for hiding this comment

renanferrari Feb 11, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 12, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 12, 2021

Choose a reason for hiding this comment

ParaskP7 Feb 12, 2021

Choose a reason for hiding this comment

ParaskP7 commented Feb 11, 2021

renanferrari left a comment

Choose a reason for hiding this comment

anitaa1990 commented Feb 15, 2021

ParaskP7 commented Feb 15, 2021

ParaskP7 commented Feb 15, 2021

ParaskP7 commented Feb 10, 2021 •

edited

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited

peril-wordpress-mobile bot commented Feb 10, 2021 •

edited