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

Feature: Sign In Milestone 1 #1150

Merged
merged 78 commits into from
Jun 14, 2019
Merged

Feature: Sign In Milestone 1 #1150

merged 78 commits into from
Jun 14, 2019

Conversation

AmandaRiu
Copy link
Contributor

This PR fixes #1091 by implementing the latest designs for Sign in milestone 1. This feature PR includes the following approved PRs:

New build config variable for debug builds

  • wp.debug.wpcom_website_url: This will pre-populate the site address for debug builds

Major Design Flows

Aside from the normal url scrubbing, below are the major flows associated with this project.

  • WordPress Required -> Site at URL does not have WordPress installed
  • Jetpack Required -> Site at URL does not have Jetpack
  • WooCommerce Required -> Set at URL does not have WooCommerce installed
  • Account Mismatch -> Site at URL does not belong to the authenticated account
  • Auto-Login Success -> Site at URL is a Jetpack-connected WordPress site with the WooCommerce plugin installed and activated AND it's connected to the authenticated account.

WordPress Required

Jetpack Required

WooCommerce Required

Account Mismatch

Auto-Login Success!

Design Deviations

Check for WooCommerce

The designs required checking for WordPress, Jetpack, and Woo when the user submits the url for login, but this would've required a huge amount of code so for this iteration, so we decided to keep it simple since this new login flow is a test and we may or may not keep it. So the temporary workaround is to use an existing FluxC endpoint: FetchConnectedSiteInfo. This will tell us if the site exists, has WP, and has Jetpack connected, but with some caveats to be aware of:

  • Sites requiring HTTP Auth will not work
  • Self-signed certificates will not work
    If in the future we decide to keep this flow, a more robust solution will need to be developed to ensure all scenarios are accounted for.

Checking if the site has WooCommerce installed is not available until after the account is authenticated so that part of the flow has been moved to post-authentication.

Offline error messaging

The original designs called for an error message to be displayed below the text input if the device was not connected, but since WCAndroid uses a shared library I wanted to make sure it was okay with the folks over at WPAndroid. At the end it was decided to leave it as-is. So if the device is not connected, then the user will just see a toast message.

Other features

  • landscape versions of each view have also been implemented where appropriate.
  • View connected stores button is only visible if the authenticated account has connected woo stores.

To Test

Each of the PRs that make this feature were independently tested, but here's an overview of things that should be verified for alpha/beta:

  • Each of the main stated flows using various login options (magic link, google (if release build), password).
  • Landscape modes
  • Clicking through each of the help dialog options
  • Switching sites (since SitePickerActivity was so heavily modified)
  • Invalid URL (empty, spaces...etc)
  • No active site at url
  • Tracks events

Update release notes:

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

AmandaRiu and others added 30 commits May 28, 2019 20:30
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.
The new asset has a transparent background so I can set the background
to stretch as needed while preserving aspect ratio of this image.
Part 1: Sign In Jetpack basic check and error messaging
@peril-woocommerce
Copy link

peril-woocommerce bot commented Jun 14, 2019

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

Generated by 🚫 dangerJS

@AmandaRiu AmandaRiu added Login type: enhancement A request for an enhancement. labels Jun 14, 2019
@AmandaRiu AmandaRiu added this to the 2.1 milestone Jun 14, 2019
Copy link
Contributor

@anitaa1990 anitaa1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great @AmandaRiu! I'm not sure if @nbradbury would like to take a look as well before we merge so leaving this open for now. Really nice work 😄

@nbradbury nbradbury self-assigned this Jun 14, 2019
@nbradbury
Copy link
Contributor

@AmandaRiu This all works great! One thing I think we should address separately, though, is the login epilogue in this flow. On a fast connection the site verification happens very quickly, so the epilogue appears very briefly and then the main activity appears. :shipit:

@nbradbury nbradbury merged commit 36f2070 into develop Jun 14, 2019
@nbradbury nbradbury deleted the feature/signin-milestone-1 branch June 14, 2019 11:26
@AmandaRiu
Copy link
Contributor Author

@AmandaRiu This all works great! One thing I think we should address separately, though, is the login epilogue in this flow. On a fast connection the site verification happens very quickly, so the epilogue appears very briefly and then the main activity appears. :shipit:

@nbradbury yeah I struggled with that one and it still bothers me. The same type of "flashing" happens currently when you do a magic link login and it's a symptom of using SitePickerActivity for all these login scenarios. Since we were in a time crunch I felt it was okay to leave it for now. Eventually we may want to implement fragments for that activity so we can have a "loading" fragment, "site picker", "no woo stores"...etc. This way the appropriate view will always be visible. You'd still get "flashing" if the view is only up for a heart beat, but at least it'd be a view that makes sense.

@peril-woocommerce
Copy link

Fails
🚫

Danger failed to run /app/danger-0.3ze6ynjouol.ts.

🚫

Danger failed to run /app/danger-0.ekw3iue3wd7.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!

  1. cd woocommerce-android
  2. git checkout feature/signin-milestone-1
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1150
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1150 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.3ze6ynjouol.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.ekw3iue3wd7.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
Copy link

Fails
🚫

Danger failed to run /app/danger-0.3hiv7sxlm3v.ts.

🚫

Danger failed to run /app/danger-0.swdx4fuz6w.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!

  1. cd woocommerce-android
  2. git checkout feature/signin-milestone-1
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/1150
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/1150 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.3hiv7sxlm3v.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.swdx4fuz6w.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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: login Related to any part of the log in or sign in flow, or authentication. type: enhancement A request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign In Milestone 1
4 participants