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

Unified Login: Not a WordPress Site error flow #3331

Merged
merged 11 commits into from
Dec 17, 2020

Conversation

AmandaRiu
Copy link
Contributor

Closes #2942. This PR adds the new "Not a WordPress site" error screen to the login by store address flow. This screen will only be show when the user enters a URL for a site that does not have WordPress installed.

Before After
not-store-before screenshot-1608068020199

Tracks events

No new tracks events added, only new Step and Flow properties. Below are the events associated with this new view:

  • Not a WordPress site error screen displayed. Set the active step to NOT_WORDPRESS_SITE:
    Tracked: unified_login_step, Properties: {"source":"default","flow":"login_site_address","step":"not_wordpress_site"}

  • On the new screen: User clicked on the "Help" icon:
    Tracked: unified_login_interaction, Properties: {"source":"default","flow":"login_site_address","step":"not_wordpress_site","click":"show_help"}

  • On the new screen: User clicked "Try another store":
    Tracked: unified_login_interaction, Properties: {"source":"default","flow":"login_site_address","step":"not_wordpress_site","click":"try_another_store"}

  • On the new screen: User clicked "Try another account":
    Tracked: unified_login_interaction, Properties: {"source":"default","flow":"login_site_address","step":"not_wordpress_site","click":"try_another_account"}

Testing

Do a fresh install of the app or clear app data, and start the login process:

  1. Click Enter your store address. The site address form is displayed.
  2. Enter google.com in the store address field and click Continue. The "Not a WordPress site" error screen is displayed.
  3. Click the Help icon and verify the help screen is displayed. Verify the appropriate tracks event is fired off.
  4. Click back to return to the error screen.
  5. Click Try another store. Verify the site address form is displayed and the appropriate tracks event is fired off.
  6. Click Continue to re-display the "Not a WordPress site" error screen.
  7. Click Log in with another account. Verify the app is directed back to the Prologue/Welcome screen and the appropriate tracks event is fired off.

Update release notes:

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Dec 15, 2020

Fails
🚫

Danger failed to run /app/danger-0.fkkgablba9.ts.

🚫

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

  1. cd woocommerce-android
  2. git checkout issue/2942-ul-not-wordpress-site
  3. git subtree push --prefix=libs/login/ https://github.com/wordpress-mobile/WordPress-Login-Flow-Android.git merge/woocommerce-android/3331
  4. Browse to https://github.com/wordpress-mobile/WordPress-Login-Flow-Android/pull/new/merge/woocommerce-android/3331 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.fkkgablba9.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.548p0tq94k7.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 AmandaRiu added Login category: tracks Related to analytics, including Tracks Events. labels Dec 15, 2020
@AmandaRiu AmandaRiu added this to the 5.7 milestone Dec 15, 2020
@AmandaRiu AmandaRiu marked this pull request as ready for review December 15, 2020 21:45
@AmandaRiu AmandaRiu requested review from Garance91540 and a team December 15, 2020 21:46
@peril-woocommerce
Copy link

peril-woocommerce bot commented Dec 15, 2020

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

@anitaa1990 anitaa1990 self-assigned this Dec 16, 2020
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.

The code changes look good @AmandaRiu! I just found one minor issue when testing.
The toolbar title is not displayed when the generic error screen is displayed. It's not a huge bblocker so if you feel it's fine to fix that in another PR, we can merge 👍

Screenshot 2020-12-16 at 6 23 59 PM

@AliSoftware AliSoftware modified the milestones: 5.7, 5.8 Dec 16, 2020
@AliSoftware
Copy link
Contributor

Moved to milestone 5.8 as it was not ready by the time I had to do the 5.7 code freeze.
(We can discuss re-targeting the PR to release/5.7 if you feel it's really worth including it in 5.7)

Garance91540
Garance91540 previously approved these changes Dec 16, 2020
Copy link

@Garance91540 Garance91540 left a comment

Choose a reason for hiding this comment

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

👏

@AmandaRiu
Copy link
Contributor Author

Oh thank you for f inding that Anitaa! I've updated to show the title, and I've also fixed the alignment of the image to better match the design.

Before After
before after

Ready for another round!

@AmandaRiu AmandaRiu added the needs: design Requires design input/work from a designer. label Dec 16, 2020
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.

LGTM :shipit:

Just asking here in case it's missed, do you think we need to update the Login library and also make sure the listener added here is updated in the WordPress app as well?

@anitaa1990 anitaa1990 merged commit 39ebf46 into develop Dec 17, 2020
@anitaa1990 anitaa1990 deleted the issue/2942-ul-not-wordpress-site branch December 17, 2020 06:38
@designsimply 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: tracks Related to analytics, including Tracks Events. feature: login Related to any part of the log in or sign in flow, or authentication. needs: design Requires design input/work from a designer.
Projects
No open projects
Unified Login
  
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Unified Login: Not a WordPress site screen
5 participants