woocommerce / woocommerce-ios

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

[REST API] Initial support for WPCom account creation during Jetpack setup #14431

Closed hichamboushaba closed 1 day ago

hichamboushaba commented 1 week ago

Closes: #14420 Closes: #14419

ℹ️ This is my first PR dealing with library updates, please let me know if I'm doing something wrong.

Description

This PR adds initial support for creating accounts during the Jetpack setup, it's based on the changes of:

⚠️ Please don't merge until both PRs above are merged and the Podfile is updated.

With the above changes, we can now follow the same approach as the Jetpack web UI, if we detect that an account doesn't exist yet, we start the signup using a magic link, and when the user taps on it, they will then be automatically authenticated and they can continue the Jetpack setup.

The changes are behind a feature flag until we update the UI of the magic link screen with more clarifications about the signup flow.

Steps to reproduce

  1. Sign in to a site using Application passwords.
  2. Start Jetpack setup from the dashboard or app settings.
  3. Enter an email that's not registered yet on WordPress.com

Testing information

Screenshots

https://github.com/user-attachments/assets/82bc025d-50ca-4026-a7ca-a804a7867994


Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

dangermattic commented 1 week ago
1 Warning
:warning: This PR is assigned to the milestone 21.2. This milestone is due in less than 2 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by :no_entry_sign: Danger

wpmobilebot commented 1 week ago

WooCommerce iOS📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS WooCommerce iOS
Build Numberpr14431-8f5e0d6
Version21.1
Bundle IDcom.automattic.alpha.woocommerce
Commit8f5e0d6e495ff30f585c5dfeeff870e8897c5cc6
App Center BuildWooCommerce - Prototype Builds #11698

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

hichamboushaba commented 3 days ago

Thanks @hafizrahman for the review.

Something that's different from your video, though, is that I'm asked to enter my site address again, while on your site it automatically jumps into the "Connecting Jetpack" screen. On my test site Jetpack was not installed yet, could this be the cause?

Nice find, this seems to be a bug with magic link handling for Application Passwords in general, the same issue happens also when signing in to an existing account. From my testing, this seems to happen only if the WooCommerce app was closed before opening Safari, if the app was kept open in the background, the issue doesn't occur, can you please confirm if this was your experience too?

Assuming it's the same case for you too, then the cause is here, we don't start the handling if the defaultSite property is not set yet, from my understanding on the iOS logic, this property is not set in iOS until we fetch the site info, which could take a while, so we fallback to the default magic link handling which is for WordPress.com signing, and which shows the site picker for your case.

Since this is an existing issue, and that we can consider an edge case (because generally the user will launch the email client from the app and won't use Safari for the magic link), I'll create a bug report, and then we can proceed with merging the PR, WDYT?

hafizrahman commented 3 days ago

From my testing, this seems to happen only if the WooCommerce app was closed before opening Safari, if the app was kept open in the background, the issue doesn't occur, can you please confirm if this was your experience too?

I believe I just pressed the home button, then went to Safari, then pasted there. But when I retried again just now, it is not happening again:

https://github.com/user-attachments/assets/ed26e0e0-25d4-4a58-b117-3317caa3af9b

So perhaps in my first test, it got closed automatically.

Since this is an existing issue, and that we can consider an edge case (because generally the user will launch the email client from the app and won't use Safari for the magic link),

Yeah, this is an edge case I agree, and even then it's not a broken path because there's still a way to complete. So it's OK to be handled separately.

hichamboushaba commented 3 days ago

Thanks @hafizrahman, is there anything left before approving this PR?

hichamboushaba commented 2 days ago

Thanks @hafizrahman actually there are no new changes, I just accidentally pushed some new commits, and after removing them, the PR was stuck in "Processing updates" state, hopefully pushing the new library changes will fix the issue before we merge this.

Screenshot 2024-11-20 at 09 46 56
mokagio commented 1 day ago

@hichamboushaba @hafizrahman I updated the pods to point to the latest WordPressAuthenticator commit on trunk.

Notice that the Danger error about pointing to a branch is gone as a result.

image

This should be good to merge now, at least as far as the pods setup is concerned. Let me know if you have any questions.