-
Notifications
You must be signed in to change notification settings - Fork 136
[Jetpack Performance] Add local feature flag and enable app passwords for all endpoints #14544
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 Performance] Add local feature flag and enable app passwords for all endpoints #14544
Conversation
Generated by 🚫 Danger |
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
0e380bc to
2792941
Compare
| override fun isEnabledForDirectAccess(): Boolean = false | ||
| override suspend fun isEnabledForJetpackAccess(): Boolean = 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.
The wear app didn't support Application Passwords before #12124, and we are keeping the same behavior.
| * Returns true if the Application Passwords feature is enabled for Jetpack-mediated access. | ||
| * This applies when sites are accessed through Jetpack, even if they are self-hosted. | ||
| */ | ||
| suspend fun isEnabledForJetpackAccess(): Boolean |
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.
Having this a suspendable function will make using remote feature flags easier for us, given they require DB access.
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
ae981cd to
b43d54c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14544 +/- ##
============================================
+ Coverage 38.31% 38.32% +0.01%
+ Complexity 9645 9641 -4
============================================
Files 2051 2052 +1
Lines 114860 114813 -47
Branches 15240 15239 -1
============================================
- Hits 44011 44005 -6
+ Misses 66816 66776 -40
+ Partials 4033 4032 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| public boolean isUsingWpComRestApi() { | ||
| return isWPCom() || (isJetpackConnected() && getOrigin() == ORIGIN_WPCOM_REST); | ||
| return isWPCom() || getOrigin() == ORIGIN_WPCOM_REST; |
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.
When testing the app passwords feature with a Jetpack CP site, I found an issue, we always ignore the created password, and the cause was in this condition.
I checked the usages of this function, and I think we can safely remove the Jetpack connection condition here, as they apply also to Jetpack CP sites.
This gives the clients more freedom in enabling disabling features, which will allow us to control the feature using remote feature flag.
- Renames the functions for better clarity. - Make `isEnabledForJetpackAccess` suspendable to be ready for remote feature flag access
`isUsingWpComRestApi` was returning false for Jetpack CP sites, leading to always ignoring saved app passwords credentials, as for WPCom rest sites, `SiteModel#username` is always null.
06cffda to
8ea3d7f
Compare
| * The [SiteModel.ORIGIN_XMLRPC] support is kept for backward compatibility | ||
| */ | ||
| @Singleton | ||
| class WooNetwork @Inject constructor( |
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.
All the changes here are a copy of what we had before in WooExperimentalNetwork
|
@irfano since Jorge is AFK for two days, I'm removing his assignment for this PR. |
irfano
left a comment
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.
It works expected. I also tested the Wear app, it works there as well. I'm approving but not merging in case you'd like to make changes based on these notes:
- In WPAPIResponse, there's this comment: "This is used for tracking purposes, and used only with the Woo experimental networking mode." I believe this is no longer valid and can be removed.
- I believe the fetchAppPasswordsSupportIfNeeded() function is no longer needed and can be removed.
| "Request failed using Application Passwords for Jetpack Site,\n" + | ||
| "site: ${siteUrl},\n" + | ||
| "path: ${requestContext.path},\n" + | ||
| "method: ${requestContext.method},\n" + | ||
| "error: HTTP status code ${error.volleyError.networkResponse?.statusCode}, " + | ||
| "error message: ${error.errorCode?.ifEmpty { null } ?: error.message}", |
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.
Nice logging details, they’ll be helpful for debugging. 👍🏻
…asswords-toggle [Jetpack Performance] Add a preference toggle for enabling app passwords for Jetpack sites
# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/AppPrefs.kt
|
Thanks @irfano, both remarks are addressed, nice catch. |
Closes WOOMOB-1220
Description
This PR adds the following changes:
ApplicationClientIdto the DI, we now supply an implementation ofApplicationPasswordsConfiguration, this allows us to control whether the feature is enabled or not from the app module.WooExperimentalNetworklogic toWooNetwork, and update it to use the new property ofApplicationPasswordsConfiguration.Steps to reproduce
APP_PASSWORDS_FOR_JETPACK_SITESis enabled.APP_PASSWORDS_FOR_JETPACK_SITES.Testing information
The above steps should be enough.
The tests that have been performed
The above.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.