-
Notifications
You must be signed in to change notification settings - Fork 130
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
[Jetpack Setup] Use Magic Link for suspicious emails #13409
base: trunk
Are you sure you want to change the base?
[Jetpack Setup] Use Magic Link for suspicious emails #13409
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
app:launchSingleTop="true" | ||
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment" | ||
app:popUpToInclusive="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.
This combination or parameters allows us to have only a single instance of the password fragment when used in this flow, as the popUpToInclusive will make sure the MagicLinkRequestFragment
is removed from backstack, to make PasswordFragment
as the top one, and then it will be replaced.
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.
TIL about these popUpTo
and popUpToInclusive
.
app:launchSingleTop="true" | ||
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment" | ||
app:popUpToInclusive="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.
Same as above.
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.
Good job on adding this feature to a complex flow, @hichamboushaba !
I’ve added some comments and minor suggestions, but none of them are blockers, so I’m approving. I tested it and didn’t find any issues. 👍🏻
val fallbackButtonText = when (viewState.magicLinkFallbackButton) { | ||
MagicLinkFallbackButton.Password -> R.string.enter_your_password_instead | ||
MagicLinkFallbackButton.UsernameAndPassword -> R.string.login_use_wpcom_username_instead | ||
MagicLinkFallbackButton.None -> null | ||
} |
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.
A minor suggestion: We can move this logic to the view model. So, the ViewState
can only have fallbackButtonText
.
@@ -162,6 +168,7 @@ private fun JetpackActivationWPComScreenPreview() { | |||
WooThemeWithBackground { | |||
JetpackActivationWPComEmailScreen( | |||
viewState = JetpackActivationWPComEmailViewModel.ViewState( | |||
usernameOnly = false, |
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.
Please correct me if I'm mistaken. We were using this screen with a text field labeled "email or username". Now, we've added usernameOnly
argument, allowing the same screen to be used with only a "username" text field. Is that correct?
I'm considering whether we should rename the class from JetpackActivationWPComEmailScreen
to JetpackActivationEmailOrUsernameScreen
. I found it challenging to understand what was happening here, and I believe this change could make things clearer.
app:launchSingleTop="true" | ||
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment" | ||
app:popUpToInclusive="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.
TIL about these popUpTo
and popUpToInclusive
.
WPCOM_EMAIL, | ||
JETPACK_STATUS, | ||
emailOrUsername = WPCOM_EMAIL, | ||
jetpackStatus = JETPACK_STATUS, | ||
magicLinkFallbackButton = MagicLinkFallbackButton.None, | ||
requestAtStart = 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.
Is given email is WPcom passwordless user, when onContinueClick clicked, then trigger ShowPasswordScreen()
incorrect? This test asserts that the magic link screen is shown, not the password screen.
verify(wpComLoginRepository, never()).requestMagicLink( | ||
emailOrUsername = EMAIL, | ||
flow = MagicLinkFlow.SiteCredentialsToWPCom, | ||
source = MagicLinkSource.JetpackConnection, | ||
isSignup = false | ||
) |
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.
Since we are checking never
condition, I suggest having general parameters here. Because, in the future, we might change SiteCredentialsToWPCom
to something else in the code. Then this test case may give false positive.
verify(wpComLoginRepository, never()).requestMagicLink( | |
emailOrUsername = EMAIL, | |
flow = MagicLinkFlow.SiteCredentialsToWPCom, | |
source = MagicLinkSource.JetpackConnection, | |
isSignup = false | |
) | |
verify(wpComLoginRepository, never()).requestMagicLink(any(), any(), any(), any()) |
Closes: #13403
Closes: #13408
Description
This PR updates the WPCom login that takes part of the Jetpack Setup to use Magic Link by default for suspicious emails, the main changes are:
Steps to reproduce
Testing information
TC1: Use Magic Link for the login, and confirm you can continue.
TC2: Use the "username" button on the Magic Link screen, then use your username/password.
The tests that have been performed
^
Images/gif
Screen_recording_20250128_110632.mov
RELEASE-NOTES.txt
if necessary. Use the "[Internal]" label for non-user-facing changes.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: