Part 1: Sign In Jetpack basic check and error messaging by AmandaRiu · Pull Request #1121 · woocommerce/woocommerce-android

AmandaRiu · 2019-06-04T02:05:42Z

This PR partially addresses #1091. To keep the PRs smaller, I'm breaking them down for review to be merged into a feature branch. This PR covers the following changes:

Removes any mention of Jetpack from the login prologue
Starts the login with the site address screen
Updates label text for LoginEmailFragment to use the site url
Updates error messaging in LoginSiteAddressFragment for all scenarios except the "not connected" scenario (coming in a later PR)
Uses SiteAction.FETCH_CONNECT_SITE_INFO to get information on the URL entered to determine if the site has WordPress installed, and if it has Jetpack installed and connected.
Adds the new "Jetpack Required" fragment (help buttons work, but other buttons just show toast message at this point).

Note: The code to check the URL for WordPress and Jetpack is a temporary measure for this round. If we decide to fully implement this login flow, we'll need to use the full XMLRPC discovery code and implement handlers for various scenarios such as HTTP Authentication required and self-signed SSL certifications.

Not a valid URL

Not a WordPress site

The initial designs required a check for WooCommerce on a site, but this is not possible without a major overhaul. Instead we check if WordPress is installed, and if not, we display an error:

Jetpack not connected

If jetpack is not installed and connected properly, the user will be directed to the "Jetpack Required" view:

WooCommerce site

If the site is a WordPress site and has Jetpack installed and connected, the user should be routed to login by email as normal. Other than the label changes, everything else should remain the same for the purpose of this PR:

cc: @kyleaparker for design review

Update release notes:

If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

The new sign in designs start at login by site.

Also converts the layouts to ConstraintLayout for optimal rendering and smoother management.

The new login flow for Woo has the login by site screen leading to this view so it doesn't make any sense having the option available as it would essentially be routing the user back to the previous screen.

This reverts commit d8a0f33. Turns out this part is not yet feasible so we'll skip it for now.

This new mode is for the Woo Android app.

peril-woocommerce · 2019-06-04T02:06:00Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

WooCommerce/src/main/java/com/woocommerce/android/LoginJetpackRequiredFragment.kt

WooCommerce/src/main/res/layout-land/fragment_login_prologue.xml

...n/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginSiteAddressFragment.java

anitaa1990

This looks great @AmandaRiu! I love how you migrated a lot of the UI to ConstraintLayout 😄

I tested the functionality and everything works good 👍 I am still trying to understand how the login lib works so not sure if I am the best person to review 😊

nbradbury · 2019-06-04T16:37:08Z

WooCommerce/src/main/java/com/woocommerce/android/LoginJetpackRequiredFragment.kt

@@ -0,0 +1,109 @@
+package com.woocommerce.android


Is there a reason this isn't in the ui/login folder?

I'm not sure what happened there. I manually moved it once, and then again in the latest branch I'm working on for the second PR. I've manually moved it again 🤞 a0b19dc

nbradbury · 2019-06-04T16:44:45Z

WooCommerce/src/main/java/com/woocommerce/android/LoginJetpackRequiredFragment.kt

+    ): View? {
+        setHasOptionsMenu(true)
+
+        // Inflate the layout for this fragment


I know this is nitpicky (Nickpicky?), but is this comment necessary?

ha! no that's a really good suggestion. Done in fe639d2

nbradbury · 2019-06-04T17:47:58Z

When I tap "Need more help" here:

It immediately asks me for my name and email address:

I think it should simply show the Help & Support activity, just as it does when the Help button is tapped in the toolbar.

nbradbury · 2019-06-04T18:01:05Z

@AmandaRiu I'm finished with my first round and it looks good despite my nitpicks. However, one potentially big problem I'm seeing is that after testing with a site that had Jetpack disabled, I then enabled Jetpack and tried again but continued to see the Jetpack required screen.

I'm assuming this is a caching issue, or something similar, but it could be a annoyance for users that enable Jetpack and still can't login.

AmandaRiu

@nbradbury Thank you for the quick review! I've addressed the comments inline and referenced code changes where needed.

I think it should simply show the Help & Support activity, just as it does when the Help button is tapped in the toolbar.

This is pre-existing functionality and I'd prefer not to tinker with it for this project. I'd be down for changing the flow, it would be pretty easy, but I'd definitely like to get @kyleaparker 's thumbs up on it, so we should just create an official ticket so it can be reviewed.

I'm assuming this is a caching issue, or something similar, but it could be a annoyance for users that enable Jetpack and still can't login.

Oh that's no good. I'll have to look into that as well. There must be something I can do there. I'll add it to my TODO list for the project. I wouldn't worry about it for this PR as this is focused more on the flow. Even the way we're checking for Jetpack is temporary.

Ready for another round!

nbradbury · 2019-06-04T21:34:00Z

@AmandaRiu I was about to merge this then thought I should test against a couple more sites. Both sites I tested with end up showing that Jetpack screen, even though both of them are wp.com sites running WooCommerce.

I know you mentioned "The code to check the URL for WordPress and Jetpack is a temporary measure for this round" - does that mean go ahead and merge even if it's not working?

Aside: I'm assuming this is why my self-hosted site kept showing that screen even after I enabled Jetpack, so the caching issue I mentioned is likely a red herring.

AmandaRiu · 2019-06-04T22:22:19Z

@nbradbury

I was about to merge this then thought I should test against a couple more sites. Both sites I tested with end up showing that Jetpack screen, even though both of them are wp.com sites running WooCommerce.

After some digging we figured out that Atomic sites have different values returned during the Connected Site check. This is an example response:

exists: true
hasJetpack: true
isJetpackActive: false
isJetpackConnected: false
isWordPress: true
isWordPressDotCom: true
jetpackVersion: "7.4"
skipRemoteInstall: false
urlAfterRedirects: "amandariu.com"

This is an atomic store and it seems like these values will always be false for an atomic store:

isJetpackActive: false
isJetpackConnected: false

I tested with a temp ninja site and walked through the results of the connect site check and verified that during each step of jetpack installation process, install, activate, connect...those values update as expected and can be relied on. I've updated the logic to only require hasJetpack = true if isWordPressDotCom = true in d347c49. This is now working as expected. Ready for testing!

nbradbury · 2019-06-04T22:25:06Z

Looks good!

kyleaparker · 2019-06-06T01:03:43Z

This is pre-existing functionality and I'd prefer not to tinker with it for this project. I'd be down for changing the flow, it would be pretty easy, but I'd definitely like to get @kyleaparker 's thumbs up on it, so we should just create an official ticket so it can be reviewed.

👍 ok to leave for the first milestone, will take a look and see if there are improvements we could make later on

peril-woocommerce · 2021-05-06T07:37:06Z

	Fails
🚫	Danger failed to run `/app/danger-0.kofk8nkoyi.ts`.
🚫	Danger failed to run `/app/danger-0.0x5ekwsc2bm.ts`.

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

	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 woocommerce-android` `git checkout amanda/sign-in-milestone-1` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1121` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1121 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.kofk8nkoyi.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|

Error TypeError

Cannot read property 'diff' of null
TypeError: Cannot read property 'diff' of null
    at checkCommitDiffs (/app/danger-0.0x5ekwsc2bm.ts:43:49)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

38|         if (git === undefined) {
39|             console.log("About to crash due to an error")
40|             console.log("File:", thisFile)
41|             console.log("Danger Object: ", danger)
42| 
---------------------------------------------------^
43|             if (danger !== undefined) {
44|                 console.log("Danger is no longer defined")
45|             }
46|             else {

Generated by 🚫 dangerJS

peril-woocommerce · 2021-05-06T07:37:06Z

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

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

	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 woocommerce-android` `git checkout amanda/sign-in-milestone-1` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1121` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1121 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.nm2mqckx6v.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|

Error TypeError

Cannot read property 'diff' of null
TypeError: Cannot read property 'diff' of null
    at checkCommitDiffs (/app/danger-0.rgcymzrcm7m.ts:43:49)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Dangerfile

38|         if (git === undefined) {
39|             console.log("About to crash due to an error")
40|             console.log("File:", thisFile)
41|             console.log("Danger Object: ", danger)
42| 
---------------------------------------------------^
43|             if (danger !== undefined) {
44|                 console.log("Danger is no longer defined")
45|             }
46|             else {

Generated by 🚫 dangerJS

AmandaRiu added 14 commits May 28, 2019 20:30

Route directly to login with site address when login is clicked

8482a00

The new sign in designs start at login by site.

Update site help image from wordpress-specific to more generic image

9db7ab3

Remove all mention of Jetpack

3e93b87

Also converts the layouts to ConstraintLayout for optimal rendering and smoother management.

Add a way to hide the option to login by site address

8f573af

The new login flow for Woo has the login by site screen leading to this view so it doesn't make any sense having the option available as it would essentially be routing the user back to the previous screen.

Route to the email login form after enter site address

0d757d2

Add new loginMode for woo and update related error strings

d8a0f33

Add debug property to populate site address during login

5698ef5

Revert "Add new loginMode for woo and update related error strings"

dd221b8

This reverts commit d8a0f33. Turns out this part is not yet feasible so we'll skip it for now.

Update error messages to match signin milestone 1

93f3e42

Add new string to handle non-WordPress site error

645531b

Add new WOO_LOGIN_MODE

3a7df62

This new mode is for the Woo Android app.

Add new logic to change the label to show the site logging into

9bda1b6

Add logic to get connected site info for woo login mode.

736a3c9

Add basic Jetpack Required fragment

3684f3b

AmandaRiu added the Login label Jun 4, 2019

AmandaRiu added this to the 2.1 milestone Jun 4, 2019

AmandaRiu changed the title ~~Amanda/sign in milestone 1~~ Part 1: Sign In Jetpack basic and error messaging Jun 4, 2019

AmandaRiu changed the title ~~Part 1: Sign In Jetpack basic and error messaging~~ Part 1: Sign In Jetpack basic check and error messaging Jun 4, 2019

Remove unnecessary TODO comment

906438a

wpmobilebot reviewed Jun 4, 2019

View reviewed changes

WooCommerce/src/main/java/com/woocommerce/android/LoginJetpackRequiredFragment.kt Outdated Show resolved Hide resolved

AmandaRiu added 2 commits June 3, 2019 20:25

Hide keyboard before displaying jetpack required screen

825a39d

Move parameter to new line to fix lint error

f839da8

AmandaRiu added the needs: design Requires design input/work from a designer. label Jun 4, 2019

AmandaRiu marked this pull request as ready for review June 4, 2019 02:33

AmandaRiu requested review from anitaa1990 and nbradbury June 4, 2019 02:33

anitaa1990 reviewed Jun 4, 2019

View reviewed changes

WooCommerce/src/main/res/layout-land/fragment_login_prologue.xml Show resolved Hide resolved

anitaa1990 reviewed Jun 4, 2019

View reviewed changes

...n/WordPressLoginFlow/src/main/java/org/wordpress/android/login/LoginSiteAddressFragment.java Show resolved Hide resolved

anitaa1990 reviewed Jun 4, 2019

View reviewed changes

nbradbury self-assigned this Jun 4, 2019

nbradbury reviewed Jun 4, 2019

View reviewed changes

AmandaRiu added 2 commits June 4, 2019 15:01

Move fragment to where it belongs in ui.login

a0b19dc

Remove silly comment for really obvious action

fe639d2

AmandaRiu commented Jun 4, 2019

View reviewed changes

AmandaRiu mentioned this pull request Jun 4, 2019

Part 2: What is Jetpack Dialog #1125

Merged

1 task

AmandaRiu added 2 commits June 4, 2019 15:48

Replace Chrome with more generic web browser verbiage

50af9bd

Special handling for atomic sites

d347c49

nbradbury merged commit 772781e into feature/signin-milestone-1 Jun 4, 2019

nbradbury deleted the amanda/sign-in-milestone-1 branch June 4, 2019 22:25

This was referenced Jun 8, 2019

Part 3: Sign In post authentication validate url and auto-login #1135

Merged

Part 4: Sign in Milestone 1 Tracks Events #1137

Merged

Feature: Sign In Milestone 1 #1150

Merged

designsimply added feature: login Related to any part of the log in or sign in flow, or authentication. and removed Login labels May 6, 2021

Conversation

AmandaRiu commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Not a valid URL

Not a WordPress site

Jetpack not connected

WooCommerce site

Uh oh!

peril-woocommerce bot commented Jun 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anitaa1990 left a comment

Choose a reason for hiding this comment

Uh oh!

nbradbury Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

AmandaRiu Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

nbradbury Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

AmandaRiu Jun 4, 2019

Choose a reason for hiding this comment

Uh oh!

nbradbury commented Jun 4, 2019

Uh oh!

nbradbury commented Jun 4, 2019

Uh oh!

AmandaRiu left a comment

Choose a reason for hiding this comment

Uh oh!

nbradbury commented Jun 4, 2019

Uh oh!

AmandaRiu commented Jun 4, 2019

Uh oh!

nbradbury commented Jun 4, 2019

Uh oh!

kyleaparker commented Jun 6, 2019

Uh oh!

peril-woocommerce bot commented May 6, 2021

Error TypeError

Dangerfile

Error TypeError

Dangerfile

Uh oh!

peril-woocommerce bot commented May 6, 2021

Error TypeError

Dangerfile

Error TypeError

Dangerfile

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AmandaRiu commented Jun 4, 2019 •

edited

Loading