Part 3: Sign In post authentication validate url and auto-login by AmandaRiu · Pull Request #1135 · woocommerce/woocommerce-android

AmandaRiu · 2019-06-08T05:53:21Z

This PR partially addresses #1091 by adding the logic to check the site entered during the login flow for the following after successful account authentication:

The site belongs to the account the user logged in with
The site has WooCommerce installed
If both of the above checks pass successfully, the login will complete and drop the user into the app without displaying the site selector.

Previous PRs for this feature:

Not included in this PR (coming later):

Tracks events
Create instructions support page and link the "View Instructions" button to this page.

Site not connected to Account

If the site the user entered during login does not match the account the user logged in with, display a message advising of such. The user is presented with an option to Try another account.

Note: the site url is listed in the message instead of the site name because that information is not available until the user is authenticated with an account attached to the site.

If the authenticated account has active stores, the user is also given the option to view connected stores.

There is also a help option Need help finding the connected email? that will display an instructional dialog. The Need more help option will open the support page where the user can then submit a ticket.

Landscape: Landscape-specific screens are also included. While I was in the code I also fixed the problem with this page not scrolling.

Not a Woo Store

If the site the user entered during login does belong to the account the user logged in with, but does not have WooCommerce installed, display a message advising of such. Just like before, the user is presented Try another account, and, if stores exist for the active account, an option to View connected stores will also be displayed.

Has Woo and matches login

If the site the user entered during login does belong to the account the user logged in with AND has WooCommerce installed, we skip displaying any sites and automatically treat that store as if the user manually selected it.

How this works

The url entered in the LoginSiteAddressFragment is stored in AppPrefs. I had to do this so it would be accessible even when the login process leaves the app as happens when a magic link is used for login. This pref key is get deleted upon successful store selection and verification.
The SitePickerActivity has been modified to handle all of the above states. Originally I was going to try to handle all of this and skip the site picker until the user clicked on View connected sites, but I didn't want to duplicate all that logic in the MainActivity.

Test Scenarios

I recommend testing all three of the above scenarios. Be sure to use other forms of login like magic link to verify functionality.
Test with accounts that have stores and accounts that do not have stores.
Test switching stores.
Test new landscape and portrait modes.

Update release notes:

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

This is needed so we can access this url no matter the login method a user uses.

Store name is not available for sites not connected to the authenticated account.

Setting the theme-level buttonstyle to a borderless button was causing conflicts with other button styles (such as the white button).

The continue button would disappear if you rotated the device.

This prevents some odd behavior where if the app is closed on the site picker screen it doesn't remember the login by url settings

peril-woocommerce · 2019-06-08T05:53:38Z

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

Generated by 🚫 dangerJS

The method is now handled by the SitePickerPresenter because that makes more sense.

…set()" This reverts commit 821a1be

This was due to the login url still being present in AppPrefs. Now we delete this value on successful login.

…ites

anitaa1990

I tested the following scenarios with a couple of ninja sites:

Entered username that does not match the test site and username is not connected with any stores
Entered username that does not match the test site but username is connected with other stores
Entered site without Jetpack installed
Entered site with Jetpack installed and correct username but no Woo installed
Entered site with correct username and Woo installed.

Everything works great! A couple of minor things I found:

After successfully logging in, when I click back from the Dashboard screen, the Login page is still visible.

I get this error message when entering an email address that is not associated with any WP.com account. I am not sure this error message is relevant since we no longer have the site address option in this page.

I am not sure if I missed any other scenarios but @nbradbury could I trouble you to review this as well, since you might have better insights on Login?

AmandaRiu · 2019-06-10T20:44:02Z

@anitaa1990 Thank you for the thorough review!

After successfully logging in, when I click back from the Dashboard screen, the Login page is still visible.

I was unable to reproduce this. Which method of login did you use? What device were you testing on? Maybe it's API/device specific.

I get this error message when entering an email address that is not associated with any WP.com account. I am not sure this error message is relevant since we no longer have the site address option in this page.

Very good catch! I have a fix in mind, but am waiting on approval from @kyleaparker This is the screen I have in mind as a replacement:

My proposed fix would remove the part of the error string that is no longer applicable and add a "Need help finding connected email?" button.

AmandaRiu · 2019-06-10T23:13:06Z

@anitaa1990 I just noticed that the login designs have a "help finding connected email" button on the email screen so I've gone ahead and added it to the app, as well as fixed the error message by removing the part that no longer applies. Ready for a second round:

anitaa1990 · 2019-06-11T07:10:04Z

@AmandaRiu, everything looks great!

After successfully logging in, when I click back from the Dashboard screen, the Login page is still visible.
I was unable to reproduce this. Which method of login did you use? What device were you testing on? Maybe it's API/device specific.

Regarding this issue, I was able to reproduce it in emulators 26, 27 and 28. I'm not sure if this is because of my specific email/userId during login.

In the above scenario, I enter the site address and redirected to the email screen. Once I select an email from the gmail list and enter the password, I am once again redirected to enter my WP username and password. FYI, the email address reallychumm1@gmail.com is connected with the WP username: murthyanitaa. And I also verified that this is happening in the develop branch as well. So this is not related to the changes in this PR.

Once I enter my WP credentials though, I am able to login successfully to the dashboard. When I click on back from the dashboard screen, I once again, see the Log In screen.

I shared my password for that email/username in Slack so you can reproduce the issue.

I can also confirm that, as you mentioned, I am not able to reproduce this using another account. So maybe this might be a one off, with just my userId? If that is the case, I am ok to merge these changes 👍 Let me know what you think

AmandaRiu · 2019-06-12T21:18:51Z

And I also verified that this is happening in the develop branch as well. So this is not related to the changes in this PR.

If not directly related to this PR, lets go ahead and merge it 👍

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

	Fails
🚫	Danger failed to run `/app/danger-0.foi924krh4s.ts`.
🚫	Danger failed to run `/app/danger-0.7xaagnjidvr.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/signin-has-woo` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1135` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1135 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.foi924krh4s.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.7xaagnjidvr.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:07Z

	Fails
🚫	Danger failed to run `/app/danger-0.hrpkoej5fmf.ts`.
🚫	Danger failed to run `/app/danger-0.5zuue34sjio.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/signin-has-woo` `git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1135` Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1135 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.hrpkoej5fmf.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.5zuue34sjio.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 18 commits June 6, 2019 17:15

Temporarily store the login site address for use later

6a7ddd9

This is needed so we can access this url no matter the login method a user uses.

Clean up unnecessary imports and unused variables

2931773

Save login url input to AppPrefs for use during the login process

6af8148

Add support for auto-loading app using login url

7630897

Start MainActivity with a clean slate when logging out

56fc9aa

Add new strings for login milestone 1

b678131

Add "try another account" button for new login flow

4afa033

Process url login in the SitePicker

0dea0c8

Update strings per design request

39528fe

Hide "show connected stores" button if no stores are available

901de16

Use store name in error message if available

a0f95ce

Store name is not available for sites not connected to the authenticated account.

Remove default button style to fix issues with inheritance

5758095

Setting the theme-level buttonstyle to a borderless button was causing conflicts with other button styles (such as the white button).

Add "email help" button and dialog to login flow

54d485a

Add "need more help" listener to connected email dialog

cdb2523

ktlint error cleanup

7d594df

Fix onConfigurationChange bug

0a9ed67

The continue button would disappear if you rotated the device.

Add landscape versions of the site picker views

f6e3000

Keep login URL in preferences to retain site picker state on restart

1896923

This prevents some odd behavior where if the app is closed on the site picker screen it doesn't remember the login by url settings

AmandaRiu added the Login label Jun 8, 2019

AmandaRiu added this to the 2.1 milestone Jun 8, 2019

AmandaRiu added 5 commits June 7, 2019 23:57

Update the root of this view after all the changes made

e2175d8

Remove unused method. This gets cleared using the existing reset()

821a1be

Move method out of SelectedSite since it isn't directly related

8ca4fe7

The method is now handled by the SitePickerPresenter because that makes more sense.

Revert "Remove unused method. This gets cleared using the existing re…

c293352

…set()" This reverts commit 821a1be

Fix broken store switching functionality

44b26d3

This was due to the login url still being present in AppPrefs. Now we delete this value on successful login.

AmandaRiu requested review from anitaa1990 and nbradbury and removed request for nbradbury June 8, 2019 06:26

AmandaRiu marked this pull request as ready for review June 8, 2019 06:26

Keep toolbar visible when scrolling and prevent nested scrolling of s…

8a387dd

…ites

AmandaRiu added the [Status] Not Ready for Review label Jun 8, 2019

AmandaRiu added 2 commits June 8, 2019 01:29

Fix lint error

2fdc53d

Fix failing app settings test

53db6e8

AmandaRiu removed the [Status] Not Ready for Review label Jun 8, 2019

anitaa1990 self-assigned this Jun 10, 2019

anitaa1990 suggested changes Jun 10, 2019

View reviewed changes

Remove unnecessary null check

6e6779b

Remove part of login site error msg and add help button

75a19a7

AmandaRiu mentioned this pull request Jun 11, 2019

Part 4: Sign in Milestone 1 Tracks Events #1137

Merged

anitaa1990 approved these changes Jun 11, 2019

View reviewed changes

AmandaRiu removed the request for review from nbradbury June 13, 2019 02:36

anitaa1990 merged commit e88bae1 into feature/signin-milestone-1 Jun 13, 2019

anitaa1990 deleted the amanda/signin-has-woo branch June 13, 2019 02:38

AmandaRiu mentioned this pull request Jun 14, 2019

Feature: Sign In Milestone 1 #1150

Merged

1 task

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Part 3: Sign In post authentication validate url and auto-login#1135
anitaa1990 merged 28 commits intofeature/signin-milestone-1from
amanda/signin-has-woo

AmandaRiu commented Jun 8, 2019 •

edited

Loading

Uh oh!

peril-woocommerce bot commented Jun 8, 2019

Uh oh!

anitaa1990 left a comment

Uh oh!

AmandaRiu commented Jun 10, 2019

Uh oh!

AmandaRiu commented Jun 10, 2019 •

edited

Loading

Uh oh!

anitaa1990 commented Jun 11, 2019 •

edited

Loading

Uh oh!

AmandaRiu commented Jun 12, 2019

Uh oh!

peril-woocommerce bot commented May 6, 2021

Uh oh!

peril-woocommerce bot commented May 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

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

Uh oh!

Not included in this PR (coming later):

Site not connected to Account

Not a Woo Store

Has Woo and matches login

How this works

Test Scenarios

Uh oh!

peril-woocommerce bot commented Jun 8, 2019

Uh oh!

anitaa1990 left a comment

Choose a reason for hiding this comment

Uh oh!

AmandaRiu commented Jun 10, 2019

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

AmandaRiu commented Jun 12, 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!

3 participants

AmandaRiu commented Jun 8, 2019 •

edited

Loading

AmandaRiu commented Jun 10, 2019 •

edited

Loading

anitaa1990 commented Jun 11, 2019 •

edited

Loading