woocommerce / woocommerce-ios

WooCommerce iOS app
https://www.woocommerce.com/mobile
GNU General Public License v2.0
258 stars 108 forks source link

Add SIWA support #2644

Closed jaclync closed 3 years ago

jaclync commented 3 years ago

Fixes #2643 Fixes #2654

There are design changes in this PR now that we're also using the prologue screen from WPAuthenticator. We're updating the design of the empty store screen per p91TBi-38l-p2#comment-1534, so the design changes in this PR are likely just for release 4.9. We originally wanted to target this for 4.8, but there are two new strings and we've passed the translation window for 4.8 so Aaron thought it'd be nicer to just target develop for 4.9.

Changes

Testing

Example screenshots

Prologue screen

\ light dark
develop Simulator Screen Shot - iPhone 8 Plus - 2020-08-12 at 18 38 18 Simulator Screen Shot - iPhone 8 Plus - 2020-08-12 at 18 38 09
PR branch Simulator Screen Shot - iPhone 11 Pro - 2020-08-18 at 09 08 27 Simulator Screen Shot - iPhone 11 Pro - 2020-08-18 at 09 08 20
PR branch - sign in sheet Simulator Screen Shot - iPhone 11 Pro - 2020-08-18 at 09 08 29 Simulator Screen Shot - iPhone 11 Pro - 2020-08-18 at 09 08 21

Empty stores screen

light dark
Simulator Screen Shot - iPhone 11 Pro - 2020-08-19 at 09 37 48 Simulator Screen Shot - iPhone 11 Pro - 2020-08-19 at 09 38 01

Update release notes:

rachelmcr commented 3 years ago

I tested and found one issue:

When I try to log in with an Apple ID that isn't connected to a WordPress.com account, and the email doesn't match a WordPress.com account email (so there's no related WordPress.com account at all — I'd need a new account) the login silently fails.

I can see that a new WordPress.com account was created behind the scenes, but I'm not logged in to the new account in the app — the spinner just disappears and I'm left on the prologue screen.

I don't see any informative errors in the console when that happens, only a couple stats-related errors:

⚠️ Could not convert WPAnalyticsStat with value: 45
⚠️ Could not convert WPAnalyticsStat with value: 443

Steps to reproduce:

  1. Ensure you are using an Apple ID with an email address that is not being used on a WordPress.com account.
  2. Ensure your Apple ID is not linked to a WordPress.com account.
  3. Ensure the WordPress app is not listed among the apps using your Apple ID in your Apple ID Password & Security settings (to ensure an entirely clean slate when you try to log in).
  4. Open the WooCommerce app.
  5. Select Log In.
  6. Select Continue with Apple.
  7. Go through the Apple ID screens to share your Apple ID details with the app.
  8. Notice the spinner that appears, and then disappears. Result: Silent failure.
  9. At this point you can try again to log in with Apple (steps 5-7) and notice that this time it logs you in to the new account, and you end up on the empty stores screen.
rachelmcr commented 3 years ago

Another small comment, just subjective feedback:

On the empty stores screen, the button says "Connect your app with Jetpack." I don't think of it as "my app" in this context. Since the text above the button says "Unable to find WooCommerce stores connected to your account" would it make sense to say "Connect your store with Jetpack" (as in "connect your store to your account")? Alternately, maybe it could say "Connect to the app with Jetpack"?

rachelmcr commented 3 years ago

I can also confirm that I tested the following scenarios with expected results:

peril-woocommerce[bot] commented 3 years ago

You can trigger an installable build for these changes by visiting CircleCI here.

peril-woocommerce[bot] commented 3 years ago

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

astralbodies commented 3 years ago

Nice catch, @rachelmcr! I've updated the PR to show the site picker for newly signed up users. It's a bit of a hack because the Authenticator normally synchronizes credentials after creating the wpcom account, but not for us in SIWA. Our sync code is not called after signup, so I force the call between signup and showing the site list picker.

jaclync commented 3 years ago

Thanks a lot for testing SIWA so thoroughly @rachelmcr! Great catch on the first-time SIWA scenario, I actually saw it once but couldn't reproduce it after I disconnected the app from my Apple ID (I was missing your steps 1 and 2 since a WP.com was already created).

I just tested Aaron's change and it worked and created a new WP.com account after I updated the email of the previous WP.com account associated with the Apple ID. I also tried deleting my WP.com account with the private Apple email but that didn't seem to work (SIWA still linked to that deleted WP.com account for me to enter the password). This is probably an edge case 😅

Also thanks for the feedback on the empty stores screen! "Connect your store with Jetpack" does sound more clear, I updated the copy and screenshots in the PR description.

@woocommerce/ios-developers would any of you take a look at the WPAuthenticator PR first so I can wrap up there and focus on the PR here?

shiki commented 3 years ago

Thank you for working on this, Jaclyn! 🙏

I reviewed https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/373 and that looks good to me. I assume we will be reviewing this draft PR later, right? I noticed that there are some design issues in dark mode that we'd probably need to fix.

Tested:

⚠️ PSA: I ran the UI tests and they expectedly failed. 🙂

jaclync commented 3 years ago

@shiki nice catch on the failing UI test, I fixed it here: https://github.com/woocommerce/woocommerce-ios/pull/2644/commits/2cbb52006cf9637ce6a6fbb6eae4fed3cb732c76 (passed locally and on CI now!)

and yes, the PR will be updated to be a formal PR when the design changes are finalized with G's input.

jaclync commented 3 years ago

Hey @Garance91540! Here are some screenshots with the color constants that you can find in AuthenticationManager and our semantic colors for further lookup (dark/light mode):

prologue sign in method sheet
prologue prologue-sheet
jaclync commented 3 years ago

Looks like the status bar isn't showing properly in light mode. I thought this was a pre-existing condition but based on your PR screenshots, it's introduced by this PR.

great catch @astralbodies! it's fixed in https://github.com/woocommerce/woocommerce-ios/pull/2644/commits/2fd21ce6f755a0655fc5091b6a528d008546ef8a by passing statusBarStyle as .default (auto adjustment based on the view color) since the default value in WPiOS is set to light style :)

Garance91540 commented 3 years ago

Hi @jaclync, I'll P2 official updates tomorrow, but in the meantime tagged you in the Figma file with the updated designs.

Let me know if it's possible to:

  1. update the button colors with the ones designed
  2. update the "jetpack needed" screen
jaclync commented 3 years ago

@woocommerce/ios-developers @Garance91540 the PR is finally ready for review now! I've update the PR to target develop and we'd like to have it for 4.9 (code freeze next Monday). I also updated the PR description to include the latest screenshots of the affected screens. After this PR is wrapped up, I will start working on the updated design for the empty store screen in p91TBi-38l-p2#comment-1534.

jaclync commented 3 years ago

I didn't notice the black background behind the apple logo on dark mode. https://d.pr/i/DvSWGJ Is that something we can't change? If not for the next iterations I'll share designs with all secondary CTAs black on these screens

@Garance91540 I took a look at how the Apple icon was setup, and like you said it is using different icons based on dark/light mode in the authenticator library because it's assuming the secondary button has a dark background in dark mode like in WPiOS. Updating the design to have a dark background for secondary CTAs would be great, and we can try removing the black/white background for the assets (apple-white.pdf and apple-black.pdf)

https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/blob/0b3af0ff19b60e304e02cc82b058d4a57b18e01a/WordPressAuthenticator/Extensions/UIImage+Assets.swift#L45-L47

jaclync commented 3 years ago

I ran into this when testing with no Apple account signed in and when I had to verify my Apple ID email.

@shiki Great point on the generic error! An easy improvement could be to update the copy of the error message:

it doesn't look the prettiest, but it's a small change to make. feel free to suggest other copy too! (cc @Garance91540) if it looks fine to you all, I'll make a PR to WPAuthenticator and update this PR or in a separate PR.

also, since SIWA is only iOS 13+ I added a check to enable SIWA only for iOS 13+ in this commit https://github.com/woocommerce/woocommerce-ios/pull/2644/commits/881d91467f9dc46e4927df2cb46f4f2baf5e1e0f and the screenshots for iOS 13/12:

iOS 13 iOS 12
Simulator Screen Shot - iPhone 11 Pro - 2020-08-20 at 11 19 27 Simulator Screen Shot - iPhone Xs - 2020-08-20 at 11 22 10
astralbodies commented 3 years ago

Item of note: I did notice a weird glitch on the SIWA Apple logo, and filed it in the Authenticator repo. https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/issues/392

jaclync commented 3 years ago

Thanks a lot for testing @astralbodies @rachelmcr @shiki and reviewing @shiki @Garance91540 ! I'm going to merge the PR soon, and make a PR in the authenticator library about the SIWA error message (it might not make code freeze by Monday, so possibly targeting the next release)