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

Add pref-key to FML feature variables #5862

Merged
merged 10 commits into from
Oct 14, 2023
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@
- This adds methods to get the feature_ids and descriptions from a loaded manifest.
- Added a `channels` subcommmand to the command line ([#5844](https://github.com/mozilla/application-services/pull/5844)).
- This prints the channels for the given manifest.
- Added `pref-key` to feature variables schema definition ([#5862](https://github.com/mozilla/application-services/pull/5862)).
- This allows developers to override remote and default values of top-level variables with preferences.
- Requires setting `userDefaults` and `sharedPreferences` in the call to `NimbusBuilder`.

### 🦊 What's Changed 🦊

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package org.mozilla.experiments.nimbus

import android.content.Context
import android.content.SharedPreferences

/**
* Small interface to get the feature variables out of the Nimbus SDK.
Expand All @@ -15,6 +16,9 @@ interface FeaturesInterface {

val context: Context

val prefs: SharedPreferences?
get() = null

/**
* Get the variables needed to configure the feature given by `featureId`.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
package org.mozilla.experiments.nimbus

import android.content.Context
import android.content.SharedPreferences
import android.content.pm.PackageInfo
import android.content.pm.PackageManager
import android.net.Uri
Expand Down Expand Up @@ -58,6 +59,7 @@ data class NimbusServerSettings(
@Suppress("LargeClass", "LongParameterList")
open class Nimbus(
override val context: Context,
override val prefs: SharedPreferences? = null,
appInfo: NimbusAppInfo,
coenrollingFeatureIds: List<String>,
server: NimbusServerSettings?,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package org.mozilla.experiments.nimbus

import android.content.Context
import android.content.SharedPreferences
import android.net.Uri
import androidx.annotation.RawRes
import kotlinx.coroutines.runBlocking
Expand Down Expand Up @@ -83,6 +84,11 @@ abstract class AbstractNimbusBuilder<T : NimbusInterface>(val context: Context)
*/
var featureManifest: FeatureManifestInterface<*>? = null

/**
* The shared preferences used to configure the app.
*/
var sharedPreferences: SharedPreferences? = null

/**
* Build a [Nimbus] singleton for the given [NimbusAppInfo]. Instances built with this method
* have been initialized, and are ready for use by the app.
Expand Down Expand Up @@ -210,6 +216,7 @@ class DefaultNimbusBuilder(context: Context) : AbstractNimbusBuilder<NimbusInter
Nimbus(
context,
appInfo = appInfo,
prefs = sharedPreferences,
coenrollingFeatureIds = getCoenrollingFeatureIds(),
server = serverSettings,
deviceInfo = createDeviceInfo(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

package org.mozilla.experiments.nimbus.internal

import android.content.SharedPreferences
import org.json.JSONObject
import org.mozilla.experiments.nimbus.FeaturesInterface
import org.mozilla.experiments.nimbus.HardcodedNimbusFeatures
Expand All @@ -22,7 +23,7 @@ import java.util.concurrent.locks.ReentrantLock
class FeatureHolder<T : FMLFeatureInterface>(
private var getSdk: () -> FeaturesInterface?,
private val featureId: String,
private var create: (Variables) -> T,
private var create: (Variables, SharedPreferences?) -> T,
) {
private val lock = ReentrantLock()

Expand All @@ -48,7 +49,8 @@ class FeatureHolder<T : FMLFeatureInterface>(
val variables = getSdk()?.getVariables(featureId, false) ?: run {
NullVariables.instance
}
create(variables).also { value ->
val prefs = getSdk()?.prefs
create(variables, prefs).also { value ->
cachedValue = value
}
}
Expand All @@ -59,7 +61,9 @@ class FeatureHolder<T : FMLFeatureInterface>(
* their behavior because of it.
*/
fun recordExposure() {
getSdk()?.recordExposureEvent(featureId)
if (!value().isModified()) {
getSdk()?.recordExposureEvent(featureId)
}
}

/**
Expand All @@ -72,7 +76,9 @@ class FeatureHolder<T : FMLFeatureInterface>(
* [recordExposure] instead.
*/
fun recordExperimentExposure(slug: String) {
getSdk()?.recordExposureEvent(featureId, slug)
if (!value().isModified()) {
getSdk()?.recordExposureEvent(featureId, slug)
}
}

/**
Expand Down Expand Up @@ -108,7 +114,7 @@ class FeatureHolder<T : FMLFeatureInterface>(
*
* This is most likely useful during testing and other generated code.
*/
fun withInitializer(create: (Variables) -> T) {
fun withInitializer(create: (Variables, SharedPreferences?) -> T) {
lock.runBlock {
this.create = create
this.cachedValue = null
Expand Down Expand Up @@ -170,8 +176,14 @@ interface FMLObjectInterface {
*
* App developers should use the generated concrete classes, which
* implement this interface.
*
* This interface is really only here to allow bridging between Kotlin
* and other languages.
*/
interface FMLFeatureInterface : FMLObjectInterface
interface FMLFeatureInterface : FMLObjectInterface {
/**
* A test if the feature configuration has been modified somehow, invalidating any experiment
* that uses it.
*
* This may be `true` if a `pref-key` has been set in the feature manifest and the user has
* set that preference.
*/
fun isModified(): Boolean = false
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,16 @@ class TestNimbusBuilder(context: Context) : AbstractNimbusBuilder<NimbusInterfac
appInfo: NimbusAppInfo,
serverSettings: NimbusServerSettings?,
): NimbusInterface =
Nimbus(context, appInfo, listOf(), serverSettings, NimbusDeviceInfo("en-US"), null, NimbusDelegate.default())
Nimbus(
context = context,
prefs = sharedPreferences,
appInfo = appInfo,
coenrollingFeatureIds = listOf(),
server = serverSettings,
deviceInfo = NimbusDeviceInfo("en-US"),
observer = null,
delegate = NimbusDelegate.default(),
)

override fun newNimbusDisabled(): NimbusInterface =
uninitialized()
Expand Down
43 changes: 36 additions & 7 deletions components/nimbus/ios/Nimbus/FeatureHolder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,18 @@ public typealias GetSdk = () -> FeaturesInterface?
///
/// There are methods useful for testing, and more advanced uses: these all start with `with`.
///
public class FeatureHolder<T> {
public class FeatureHolder<T: FMLFeatureInterface> {
private let lock = NSLock()
private var cachedValue: T?

private var getSdk: GetSdk
private let featureId: String

private var create: (Variables) -> T
private var create: (Variables, UserDefaults?) -> T

public init(_ getSdk: @escaping () -> FeaturesInterface?,
featureId: String,
with create: @escaping (Variables) -> T)
with create: @escaping (Variables, UserDefaults?) -> T)
{
self.getSdk = getSdk
self.featureId = featureId
Expand All @@ -43,18 +43,22 @@ public class FeatureHolder<T> {
return v
}
var variables: Variables = NilVariables.instance
var defaults: UserDefaults?
if let sdk = getSdk() {
variables = sdk.getVariables(featureId: featureId, sendExposureEvent: false)
defaults = sdk.userDefaults
}
let v = create(variables)
let v = create(variables, defaults)
cachedValue = v
return v
}

/// Send an exposure event for this feature. This should be done when the user is shown the feature, and may change
/// their behavior because of it.
public func recordExposure() {
getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: nil)
if !value().isModified() {
getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: nil)
}
}

/// Send an exposure event for this feature, in the given experiment.
Expand All @@ -67,7 +71,9 @@ public class FeatureHolder<T> {
///
/// - Parameter slug the experiment identifier, likely derived from the {value}.
public func recordExperimentExposure(slug: String) {
getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: slug)
if !value().isModified() {
getSdk()?.recordExposureEvent(featureId: featureId, experimentSlug: slug)
}
}

/// Send a malformed feature event for this feature.
Expand Down Expand Up @@ -121,10 +127,33 @@ public class FeatureHolder<T> {
/// This changes the mapping between a `Variables` and the feature configuration object.
///
/// This is most likely useful during testing and other generated code.
public func with(initializer: @escaping (Variables) -> T) {
public func with(initializer: @escaping (Variables, UserDefaults?) -> T) {
lock.lock()
defer { self.lock.unlock() }
cachedValue = nil
create = initializer
}
}

/// A bare-bones interface for the FML generated objects.
public protocol FMLObjectInterface {}

/// A bare-bones interface for the FML generated features.
///
/// App developers should use the generated concrete classes, which
/// implement this interface.
///
public protocol FMLFeatureInterface: FMLObjectInterface {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The FMLFeatureInterface/FMLFeatureObject was introduced in Kotlin when we did the toJSONObject work.

This is the Swift version, now we need it to implement isModified().

It still does not do the JSON object creation.

/// A test if the feature configuration has been modified somehow, invalidating any experiment
/// that uses it.
///
/// This may be `true` if a `pref-key` has been set in the feature manifest and the user has
/// set that preference.
func isModified() -> Bool
Copy link
Member

Choose a reason for hiding this comment

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

Does this need the public modifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding public gives:

❌ error: public modified cannot be used in protocols.

Copy link
Member

Choose a reason for hiding this comment

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

Cool I just wasn't sure — iirc we had a few prs in recent history just adding privacy modifiers

}

public extension FMLFeatureInterface {
func isModified() -> Bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding public gives:

⚠️ public modifier is redundant for instance method declared in a public extension.

return false
}
}
8 changes: 8 additions & 0 deletions components/nimbus/ios/Nimbus/FeatureInterface.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import Foundation
///
/// This is intended to be standalone to allow for testing the Nimbus FML.
public protocol FeaturesInterface: AnyObject {
var userDefaults: UserDefaults? { get }

/// Get the variables needed to configure the feature given by `featureId`.
///
/// - Parameters:
Expand Down Expand Up @@ -56,3 +58,9 @@ public protocol FeaturesInterface: AnyObject {
/// is malformed, providing more detail to experiment owners of where to look for the problem.
func recordMalformedConfiguration(featureId: String, with partId: String)
}

public extension FeaturesInterface {
var userDefaults: UserDefaults? {
nil
}
}
8 changes: 8 additions & 0 deletions components/nimbus/ios/Nimbus/Nimbus.swift
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import Foundation
import Glean

public class Nimbus: NimbusInterface {
private let _userDefaults: UserDefaults?

private let nimbusClient: NimbusClientProtocol

private let resourceBundles: [Bundle]
Expand All @@ -28,11 +30,13 @@ public class Nimbus: NimbusInterface {

init(nimbusClient: NimbusClientProtocol,
resourceBundles: [Bundle],
userDefaults: UserDefaults?,
errorReporter: @escaping NimbusErrorReporter)
{
self.errorReporter = errorReporter
self.nimbusClient = nimbusClient
self.resourceBundles = resourceBundles
_userDefaults = userDefaults
NilVariables.instance.set(bundles: resourceBundles)
}
}
Expand Down Expand Up @@ -102,6 +106,10 @@ extension Nimbus: NimbusEventStore {
}

extension Nimbus: FeaturesInterface {
public var userDefaults: UserDefaults? {
_userDefaults
}

public func recordExposureEvent(featureId: String, experimentSlug: String? = nil) {
if let experimentSlug = experimentSlug {
recordExposureFromExperiment(featureId: featureId, experimentSlug: experimentSlug)
Expand Down
14 changes: 13 additions & 1 deletion components/nimbus/ios/Nimbus/NimbusBuilder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ public class NimbusBuilder {
var resourceBundles: [Bundle] = [.main]

/**
* The object generated from the `nimbus.fml.yaml` file and the nimbus-gradle-plugin.
* The object generated from the `nimbus.fml.yaml` file.
*/
@discardableResult
public func with(featureManifest: FeatureManifestInterface) -> NimbusBuilder {
Expand All @@ -142,6 +142,17 @@ public class NimbusBuilder {

var featureManifest: FeatureManifestInterface?

/**
* Main user defaults for the app.
*/
@discardableResult
public func with(userDefaults: UserDefaults) -> NimbusBuilder {
self.userDefaults = userDefaults
return self
}

var userDefaults = UserDefaults.standard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't been used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now used!

/**
* The command line arguments for the app. This is useful for QA, and can be safely left in the app in production.
*/
Expand Down Expand Up @@ -240,6 +251,7 @@ public class NimbusBuilder {
coenrollingFeatureIds: getCoenrollingFeatureIds(),
dbPath: dbFilePath,
resourceBundles: resourceBundles,
userDefaults: userDefaults,
errorReporter: errorReporter)
}

Expand Down
8 changes: 7 additions & 1 deletion components/nimbus/ios/Nimbus/NimbusCreate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ public extension Nimbus {
dbPath: String,
resourceBundles: [Bundle] = [Bundle.main],
enabled: Bool = true,
userDefaults: UserDefaults? = nil,
errorReporter: @escaping NimbusErrorReporter = defaultErrorReporter
) throws -> NimbusInterface {
guard enabled else {
Expand Down Expand Up @@ -66,7 +67,12 @@ public extension Nimbus {
)
)

return Nimbus(nimbusClient: nimbusClient, resourceBundles: resourceBundles, errorReporter: errorReporter)
return Nimbus(
nimbusClient: nimbusClient,
resourceBundles: resourceBundles,
userDefaults: userDefaults,
errorReporter: errorReporter
)
}

static func buildExperimentContext(
Expand Down
17 changes: 17 additions & 0 deletions components/support/nimbus-fml/fixtures/android/runtime/Context.kt
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,20 @@ object Resources {

fun getResourceName(resId: Int) = "res:$resId"
}

@Suppress("PACKAGE_OR_CLASSIFIER_REDECLARATION")
class SharedPreferences {
// Minimal interface used by generated code.
fun contains(key: String): Boolean = map.containsKey(key)
fun getBoolean(key: String, def: Boolean): Boolean = (map[key] as? Boolean) ?: def
fun getString(key: String, def: String): String = (map[key] as? String) ?: def
fun getInt(key: String, def: Int): Int = (map[key] as? Int) ?: def

// For testing
val map = mutableMapOf<String, Any>()
fun put(key: String, value: Boolean) = map.put(key, value)
fun put(key: String, value: String) = map.put(key, value)
fun put(key: String, value: Int) = map.put(key, value)
fun clear() = map.clear()
fun remove(key: String) = map.remove(key)
}
Loading