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

[Jetpack Setup] Use Magic Link for suspicious emails #13409

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Jan 28, 2025

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:

  • Trigger the Magic Link flow when detecting a suspicious email.
  • Update the Magic Link screen to allow not triggering the email automatically at start.
  • Update the Magic Link screen to show the "username" fallback button.

Steps to reproduce

  1. Use a non-Jetpack site (either Jetpack not installed, or Jetpack not connected)
  2. Sign in to the site.
  3. Open the app.
  4. Enable the API faker and add an endpoint following the below:
Endpoint type: WordPress.com
Endpoint path: /v1.1/users/{email}/auth_options (replace email with your WordPress.com email)
Response status code: 403
Response body: 
{
        "error": "email_login_not_allowed",
        "message": "Please log in using your WordPress.com username instead of your email address."
}
  1. Tap on the Jetpack benefits banner.
  2. Start the Jetpack setup.

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
  • I have considered if this change warrants release notes and have added them to 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:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@hichamboushaba hichamboushaba added type: enhancement A request for an enhancement. feature: login Related to any part of the log in or sign in flow, or authentication. feature: REST API Support connecting to sites using WordPress REST API. labels Jan 28, 2025
@dangermattic
Copy link
Collaborator

dangermattic commented Jan 28, 2025

1 Error
🚫 PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 28, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commit2f12442
Direct Downloadwoocommerce-wear-prototype-build-pr13409-2f12442.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 28, 2025

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

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commit2f12442
Direct Downloadwoocommerce-prototype-build-pr13409-2f12442.apk

Comment on lines +162 to +164
app:launchSingleTop="true"
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment"
app:popUpToInclusive="true" />
Copy link
Member Author

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.

Copy link
Contributor

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.

Comment on lines +168 to +170
app:launchSingleTop="true"
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment"
app:popUpToInclusive="true" />
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

@hichamboushaba hichamboushaba marked this pull request as ready for review January 28, 2025 10:25
@irfano irfano self-assigned this Jan 30, 2025
Copy link
Contributor

@irfano irfano left a 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. 👍🏻

Comment on lines +96 to +100
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
}
Copy link
Contributor

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,
Copy link
Contributor

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.

Comment on lines +162 to +164
app:launchSingleTop="true"
app:popUpTo="@id/jetpackActivationMagicLinkRequestFragment"
app:popUpToInclusive="true" />
Copy link
Contributor

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.

Comment on lines -173 to +188
WPCOM_EMAIL,
JETPACK_STATUS,
emailOrUsername = WPCOM_EMAIL,
jetpackStatus = JETPACK_STATUS,
magicLinkFallbackButton = MagicLinkFallbackButton.None,
requestAtStart = true,
Copy link
Contributor

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.

Comment on lines +72 to +77
verify(wpComLoginRepository, never()).requestMagicLink(
emailOrUsername = EMAIL,
flow = MagicLinkFlow.SiteCredentialsToWPCom,
source = MagicLinkSource.JetpackConnection,
isSignup = false
)
Copy link
Contributor

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.

Suggested change
verify(wpComLoginRepository, never()).requestMagicLink(
emailOrUsername = EMAIL,
flow = MagicLinkFlow.SiteCredentialsToWPCom,
source = MagicLinkSource.JetpackConnection,
isSignup = false
)
verify(wpComLoginRepository, never()).requestMagicLink(any(), any(), any(), any())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: login Related to any part of the log in or sign in flow, or authentication. feature: REST API Support connecting to sites using WordPress REST API. type: enhancement A request for an enhancement.
Projects
None yet
4 participants