-
Notifications
You must be signed in to change notification settings - Fork 121
[Application Passwords] Add experimental feature toggle in app settings #16059
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
[Application Passwords] Add experimental feature toggle in app settings #16059
Conversation
Generated by 🚫 Danger |
|
|
cdb0d88 to
9194cd4
Compare
itsmeichigo
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.
This works great 👍 I left some non-blocking comments and questions.
RELEASE-NOTES.txt
Outdated
| 23.2 | ||
| ----- | ||
|
|
||
| - [*] Adds a toggle for Application Passwords feature in Experimental Features settings screen [https://github.com/woocommerce/woocommerce-ios/pull/16059] |
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.
Nit: since the feature is not enabled yet, this will not be available to users in version 23.2. Let's remove this.
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.
Reverted in 9026d48
| Section( | ||
| footer: VStack( | ||
| alignment: .leading, | ||
| spacing: Layout.spacing |
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.
Super nit: is this spacing redundant? There seems to be only one item in the VStack in both cases.
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. Previously I attempted a solution with a separate Link element. The redundant spacing remained. I'm gonna remove VStack at all.
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.
Done in 4c6321a
| DispatchQueue.main.async { | ||
| self.checkApplicationPasswordExperimentFeatureState() | ||
| } |
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.
Nit: Should this be simplified:
| DispatchQueue.main.async { | |
| self.checkApplicationPasswordExperimentFeatureState() | |
| } | |
| Task { @MainActor in | |
| await checkApplicationPasswordExperimentFeatureState() | |
| } |
Then make checkApplicationPasswordExperimentFeatureState async for better testability if we need to test it.
| .betaFeatureEnabledPublisher( | ||
| .applicationPasswords | ||
| ) | ||
| .dropFirst() |
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.
❓ Why is it necessary to drop the initial value?
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.
I added dropFirst because eventually the generalAppSettings.betaFeatureEnabledPublisher automatically emits the current toggle state value upon subscription. Without the dropFirst it would also trigger the checkApplicationPasswordExperimentFeatureState() here. But the initial checkApplicationPasswordExperimentFeatureState call happens in AuthenticatedState.init.
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.
Open to improvement suggestions.
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.
I think dropFirst should be used with caution to avoid missing data. Since removeDuplicates is already in place, it should be sufficient for avoiding triggering checkApplicationPasswordExperimentFeatureState when unnecessary.
| case hiddenStoreIDs | ||
|
|
||
| // Application passwords experiment remote FF cached value | ||
| case applicationPasswordsExperimentRemoteFFValue |
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.
Another suggestion: should we clear this flag in SessionManager.reset?
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.
Done in a085407
…re-toggle # Conflicts: # WooCommerce/Classes/System/SessionManager.swift

Part of WOOMOB-1189
Description
Implements an experimental feature toggle for "Application Passwords Experiment" feature
isApplicationPasswordsSwitchEnabledproperty toGeneralAppSettingsAppSettingsactions to access the corresponding settingBetaFeature.applicationPasswordsto be displayed in experimental features as a toggle.settings_beta_features_application_passwords_toggledanalytics event triggered by the feature toggle.ApplicationPasswordsExperimentAvailabilityCheckerdraft as an async wrapper around upcoming remote FF.ApplicationPasswordsExperimentStatehandles that.RequestAuthenticatorin runtime.Testing steps
ApplicationPasswordsExperimentAvailabilityCheckerset themockResultValuetofalse. Make sure the setting entry disappears in "Experimental Features". Make sure JP requests are made as usual despite the prior toggle state.Screenshots
RELEASE-NOTES.txtif necessary.