-
Notifications
You must be signed in to change notification settings - Fork 114
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
[REST API] Initial support for WPCom account creation during Jetpack setup #14431
[REST API] Initial support for WPCom account creation during Jetpack setup #14431
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
59b291b
to
51510b2
Compare
51510b2
to
aa7a587
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing notes:
I was able to follow the steps in the test site and have a magic link sent to my account, with subject "Create WordPress.com account on your mobile device".
I checked log and also confirmed the "unknown_user" error:
⛔️ Error checking for passwordless account: endpointError(WordPressKit.WordPressComRestApiEndpointError(code: WordPressKit.WordPressComRestApiErrorCode, response: nil, apiErrorCode: Optional("unknown_user"), apiErrorMessage: Optional("User does not exist."), apiErrorData: nil, additionalUserInfo: Optional([:])))
I then copied the link from the email and pasted in Safari, and was given the steps to enter my site, install and connect Jetpack. Video below:
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2024-11-18.at.14.43.18.mp4
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?
// The user does not exist yet, trigger magic link flow for account creation | ||
await requestAuthenticationLink(email: email, forAccountCreation: true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not specifically related to functionality of this PR: I feel it might be valuable to add tracking for this case. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's already planned, I'll add some tracking before releasing the feature.
@@ -78,8 +87,16 @@ final class WPComEmailLoginViewModel: ObservableObject { | |||
} | |||
await startAuthentication(email: email, isPasswordlessAccount: passwordless) | |||
} catch { | |||
analytics.track(event: .JetpackSetup.loginFlow(step: .emailAddress, failure: error)) | |||
onError(error.localizedDescription) | |||
if allowAccountCreation, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about replacing the if-else with guard?
guard allowAccountCreation,
let apiError = error as? WordPressAPIError<WordPressComRestApiEndpointError>,
case .endpointError(let endpointError) = apiError,
endpointError.apiErrorCode == Constants.unknownUserErrorCode else {
analytics.track(event: .JetpackSetup.loginFlow(step: .emailAddress, failure: error))
onError(error.localizedDescription)
return
}
await requestAuthenticationLink(email: email, forAccountCreation: true)
I think guard
helps with early return pattern and communicates what conditions are required, and reduces nesting a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied in 99ad008
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in this PR imo is pretty straightforward and looks good to go. I left some code and testing comments to discuss.
Thanks @hafizrahman for the review.
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. Assuming it's the same case for you too, then the cause is here, we don't start the handling if the 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? |
# Conflicts: # Experiments/Experiments/DefaultFeatureFlagService.swift # Experiments/Experiments/FeatureFlag.swift
ae0c504
to
20a0bfc
Compare
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: Simulator.Screen.Recording.-.Hafiz.iPhone.15.-.2024-11-19.at.18.51.14.mp4So perhaps in my first test, it got closed automatically.
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. |
Thanks @hafizrahman, is there anything left before approving this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-approving because it should be good now, however since I see additional commits, let me know if there's anything new I should be checking/testing @hichamboushaba
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. |
I can't believe I forgot this step in the previous commit...
@hichamboushaba @hafizrahman I updated the pods to point to the latest WordPressAuthenticator commit on Notice that the Danger error about pointing to a branch is gone as a result. 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. |
4054f38
to
8f5e0d6
Compare
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:
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
Testing information
Screenshots
Simulator.Screen.Recording.-.iPhone.16.-.2024-11-15.at.09.45.51.mp4
RELEASE-NOTES.txt
if necessary.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: