Display different copy in alerts when a flag is enabled in a newer version #633
Conversation
There was a problem hiding this comment.
LGTM! I left a few comments that could improve readability, but I'm only strong on the public private(set) thingy, otherwise it's solid and you can decide to address remaining comments or not. Will have a look at client PRs now. Thanks @SabrinaTardio!
| @@ -24,6 +24,7 @@ import Foundation | |||
| */ | |||
| public struct SyncFeatureFlags: OptionSet { | |||
| public let rawValue: Int | |||
| public var unavailableReason: PrivacyConfigurationFeatureDisabledReason? | |||
There was a problem hiding this comment.
This should be a public private(set) var, so that it can't be modified from outside.
There was a problem hiding this comment.
Yes! def should have done that
| @@ -68,20 +69,39 @@ public struct SyncFeatureFlags: OptionSet { | |||
| // MARK: - | |||
|
|
|||
| init(privacyConfig: PrivacyConfiguration) { | |||
| guard privacyConfig.isEnabled(featureKey: .sync) else { | |||
| var offFlag: SyncSubfeature? | |||
There was a problem hiding this comment.
Wouldn't calling it disabledSubfeature make it more clear?
| public struct SyncFeatureFlags: OptionSet { | ||
| public let rawValue: Int | ||
| public var unavailableReason: PrivacyConfigurationFeatureDisabledReason? |
There was a problem hiding this comment.
I like this approach a lot. I didn't think about it initially because I'm having this mindset that OptionSet is basically an enum so we shouldn't be adding other fields to it, besides the raw value. But since enums have associated values, why not add an extra field to an OptionSet struct if it's needed. It looks like an equivalent of an enum's associated value.
Good job! 👏
There was a problem hiding this comment.
I know what you mean lol… initially it didn’t come to me intuitively to do that… and then I thought… why not! we do it for enum too ahahah
| if let offFlag { | ||
| let state = privacyConfig.stateFor(offFlag) | ||
| switch state { | ||
| case .enabled: | ||
| break | ||
| case .disabled(let reason): | ||
| unavailableReason = reason | ||
| } |
There was a problem hiding this comment.
This was a bit confusing to me at the beginning and I had to follow the code closely to understand it better. Could we rewrite it as follows?
| if let offFlag { | |
| let state = privacyConfig.stateFor(offFlag) | |
| switch state { | |
| case .enabled: | |
| break | |
| case .disabled(let reason): | |
| unavailableReason = reason | |
| } | |
| if let offFlag, case .disabled(let reason) = privacyConfig.stateFor(offFlag) { | |
| unavailableReason = reason | |
| } |
There was a problem hiding this comment.
I personally find this less readable because is condensed but I’ll change it… cause it's probably just me 😄
Please review the release process for BrowserServicesKit here.
Required:
Task/Issue URL: https://app.asana.com/0/72649045549333/1206310394453442/f
iOS PR: duckduckgo/iOS#2374
macOS PR: duckduckgo/macos-browser#2114
What kind of version bump will this require?: Major/Minor/Patch
Description: Shows a different copy in alerts when a sync flag is enabled in a newer version and the user needs to update the app
Steps to test this PR:
<!—
Before submitting a PR, please ensure you have tested the combinations you expect the reviewer to test, then delete configurations you know do not need explicit testing.
Using a simulator where a physical device is unavailable is acceptable.
—>
OS Testing:
—
Internal references:
Software Engineering Expectations
Technical Design Template