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

[Login] Allow login for WPCOM suspended sites #14257

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

Conversation

selanthiraiyan
Copy link
Contributor

@selanthiraiyan selanthiraiyan commented Oct 30, 2024

Closes: #14196
Closes: #14255
Closes: #14256

Description

⚠️ Don't merge until wordpress-mobile/WordPressAuthenticator-iOS#858 is merged.

Adds code to allow login for WPCOM suspended sites. This logic is added behind a configuration flag in the authenticator library.

The enableSiteCredentialsLoginForWPCOMSuspendedSites configuration here will be enabled later once the rest of the tasks are completed.

Steps to reproduce

Prerequisites

  • Create a JN site with Woo and Jetpack
  • Connect Jetpack from wp-admin page
  • Follow the steps to mark it as suspended in WPCOM. Internal - p1729584367490299/1729584240.840689-slack-C03L1NF1EA3
  • Reach out to me or @hichamboushaba if you need help suspending a site.

Login into a suspended site

  • Build and run the app
  • Logout if needed
  • Login into the app using the JN site
  • Validate that you see the "Invalid URL" error
  • Stop the app
  • Set true for the enableSiteCredentialsLoginForWPCOMSuspendedSites configuration here
  • Run the app
  • Login into the app using the JN site
  • You should be able to login into the app using .org site credentials
  • Validate that you don't see the Jetpack banner in the dashboard.

WPCOM credentials login

  • Follow the steps and mark the site as not suspended. (Remove flags) Internal - p1729584367490299/1729584240.840689-slack-C03L1NF1EA3
  • Logout from the app.
  • Try logging in using the same JN site.
  • You should be navigated to WPCOM login flow and validate that WPCOM login works.

JN site .org credentials login

  • Create a JN site with Woo and no Jetpack
  • Logout from the app
  • Login into the app using .org site credentials
  • Validate that you see the Jetpack banner in the dashboard

Testing information

I tested the following scenarios.

  • Suspended site's .org site credentials login works when enableSiteCredentialsLoginForWPCOMSuspendedSites configuration is set as true. The Jetpack banner is hidden.
  • Normal site credentials login works as before. The Jetpack banner is displayed for eligible sites.
  • Normal WPCOM credentials login works as before.

Screenshots

Suspended site

enableSiteCredentialsLoginForWPCOMSuspendedSites false enableSiteCredentialsLoginForWPCOMSuspendedSites true
Simulator Screenshot - iPhone 15 Pro Max - 2024-10-30 at 13 45 21 Simulator Screenshot - iPhone 15 Pro Max - 2024-10-30 at 13 52 15

Dashboard - Jetpack banner of site without Jetpack when enableSiteCredentialsLoginForWPCOMSuspendedSites true

Normal JN site (without Jetpack) Suspended JN site
image image

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

  • 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 all devices (phone/tablet) and no regressions are added.

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Oct 30, 2024

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 Numberpr14257-bcf1634
Version20.9
Bundle IDcom.automattic.alpha.woocommerce
Commitbcf1634
App Center BuildWooCommerce - Prototype Builds #11367
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@selanthiraiyan selanthiraiyan marked this pull request as ready for review October 30, 2024 09:48
@selanthiraiyan selanthiraiyan added the feature: login Related to any part of the log in or sign in flow, or authentication. label Oct 30, 2024
@hichamboushaba hichamboushaba self-assigned this Oct 31, 2024
Copy link
Member

@hichamboushaba hichamboushaba left a comment

Choose a reason for hiding this comment

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

Nice work @selanthiraiyan, the logic works well.

There are just two points that needs addressing before approving the PR, the unit tests are failing, and the "Install jetpack" button is still shown in the app settings.

Comment on lines +69 to +71
// Whether the site is suspended on WordPress.com and can't be connected using Jetpack
//
case wpcomSiteSuspended
Copy link
Member

Choose a reason for hiding this comment

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

Just a question for my learning, do we have any guidelines on where to use UserDefaults vs something like GeneralAppSettings in the app? in the custom fields project, I used GeneralAppSettings, and it needed a lot of boilerplate to my liking, so I'm not sure if I should've used UserDefaults instead 😅

@@ -60,6 +60,8 @@ final class DashboardViewModel: ObservableObject {

@Published private(set) var jetpackBannerVisibleFromAppSettings = false

@Published private(set) var isSiteEligibleToInstallJetpack = true
Copy link
Member

Choose a reason for hiding this comment

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

We need a similar logic for the app settings to hide the "Install Jetpack" button.

@selanthiraiyan selanthiraiyan modified the milestones: 21.1, 21.2 Nov 7, 2024
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.
Projects
None yet
3 participants