Add `pref-key` to FML feature variables #5862

jhugman · 2023-10-11T12:51:04Z

This PR adds to the FML a pref key to feature variables. For example

features:
  my-feature:
    variables:
      is-enabled:
        type: Boolean
        default: false
        pref-key: "my-feature.enabled"

The NimbusBuilder now accepts a userDefaults or sharedPreferences which stores the prefs.

The generated code for myFeature.isEnabled now checks the prefs for a key my-feature.enabled.

This allows app code to toggle on or off features, via a pref rather than just experiment.

There are several restrictions for this feature:

only top level variables in a feature are supported— nesting gets tricky quickly, so we need to see if this is a feature that is actually used.
only Boolean, Int, String, and Text types are supported.

To detect if a user's preference has been set, a newisModified() method is provided. This returns true if any prefs in the feature have been set.

When isModified is true, exposure events are suppressed.

NB. It's not clear (without seeing usage) if this suppressing exposure events is enough before the prefs can be changed by a general population. Alternatives may be to unenroll from any experiments involving the feature. Until this is clear, we advise only exposing such user preferences in a secret settings context.

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2023-10-11T13:01:02Z

Codecov Report

Attention: Patch coverage is 95.74468% with 10 lines in your changes missing coverage. Please review.

Project coverage is 36.65%. Comparing base (8a74932) to head (426a907).
Report is 571 commits behind head on main.

Files with missing lines	Patch %	Lines
...port/nimbus-fml/src/intermediate_representation.rs	97.43%	4 Missing ⚠️
components/support/nimbus-fml/src/backends/mod.rs	0.00%	3 Missing ⚠️
...s/support/nimbus-fml/src/command_line/workflows.rs	81.81%	2 Missing ⚠️
...bus-fml/src/backends/kotlin/gen_structs/filters.rs	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5862      +/-   ##
==========================================
- Coverage   36.81%   36.65%   -0.16%     
==========================================
  Files         347      347              
  Lines       33447    33380      -67     
==========================================
- Hits        12313    12236      -77     
- Misses      21134    21144      +10

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jhugman

Self-review

jhugman · 2023-10-12T09:52:36Z

components/nimbus/ios/Nimbus/NimbusBuilder.swift

+    }
+
+    var userDefaults = UserDefaults.standard
+


This isn't been used.

This is now used!

jhugman · 2023-10-12T10:36:54Z

components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt

+            }
+         } ?: getter()
+      }
+   {%- endif %}


This is the heart of the patch for kt. It produces code like:

val myBoolean: Boolean get() { fun getter() = _variables.getBool("my-boolean") ?: _defaults.myBoolean return this._prefs?.let { if (it.contains("my-boolean-pref-key")) { it.getBoolean("my-boolean-pref-key", getter()) } else { null } } ?: getter() }

Commented:

val myBoolean: Boolean get() { fun getter() = _variables.getBool("my-boolean") ?: _defaults.myBoolean return this._prefs?.let { // We have to provide a default value, so we need to use contains // to detect if a value has been provided. It doesn't check the type, // so we have to assume that the app isn't overwriting the prefs // with the wrong type. if (it.contains("my-boolean-pref-key")) { // The prefs API needs us to provide a default value; in practice it will never // return the default because of the contains check. it.getBoolean("my-boolean-pref-key", getter()) } else { null } } ?: getter() }

Following self-review:

val myBoolean: Boolean get() = this._prefs?.let { if (it.contains("my-boolean-pref-key")) { try { it.getBoolean("my-boolean-pref-key", false) } catch (e: ClassCastException) { // This only gets to here if the app has written the // wrong type to this preference. null } } else { null } } ?: _variables.getBool("my-boolean") ?: _defaults.myBoolean

Honestly I really like the chained ?:

jhugman · 2023-10-12T11:17:43Z

components/support/nimbus-fml/src/backends/swift/templates/macros.swift

+            return {{ prop_swift }}
+        }
+        return {{ getter }}
+    }


Swift's user defaults API isn't as clever as Android's. It doesn't do the defaulting for us, and provides a catchall func object(forKey:)->Any?. This makes it much easier to generate!

Generated code:

public var myBoolean: Bool { if let prefs = self._prefs, let myBoolean = prefs.object(forKey: "my-boolean-pref-key") as? Bool { return myBoolean } return self._variables.getBool("my-boolean") ?? _defaults.myBoolean }

jhugman · 2023-10-12T11:19:26Z

components/support/nimbus-fml/src/defaults_merger.rs

-                doc: "".into(),
-                typ: TypeRef::String,
-            }],
+            props: vec![PropDef::new("button-color", TypeRef::String, json!("blue"))],


A significant number of lines changed in this PR is changing these PropDef { construction to a convenience test helper constructor.

jhugman · 2023-10-12T11:22:57Z

components/support/nimbus-fml/src/intermediate_representation.rs

+            }
+        }
+        Ok(())
+    }


This method is the beginning of a larger refactor to split out feature structure validation which needs to be done when the manifest is being written (e.g. is this a valid schema) from feature defaults validation (i.e. does this JSON conform to the schema).

Ah that's a great refactor

jhugman · 2023-10-12T11:26:06Z

components/nimbus/ios/Nimbus/FeatureHolder.swift

+/// App developers should use the generated concrete classes, which
+/// implement this interface.
+///
+public protocol FMLFeatureInterface: FMLObjectInterface {


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.

jeddai

Overall looks good, I'm loving the refactors and tests. I ran into issues trying to test it in Fenix and didn't want to hold up the review process any longer so I'm approving now. Since it's not a breaking change I'm not particularly concerned about merging it now to get builds going with it and making changes again later if we need to.

jeddai · 2023-10-12T16:30:18Z

components/nimbus/ios/Nimbus/FeatureHolder.swift

+    ///
+    /// 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


Does this need the public modifier?

Adding public gives:

❌ error: public modified cannot be used in protocols.

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

jeddai · 2023-10-12T21:28:25Z

components/support/nimbus-fml/src/intermediate_representation.rs

+            // There may be a chance that we can get Self::Option to work, but not at this time.
+            // This may be done by adding a branch to this match and adding a `preference_getter` to
+            // the `OptionalCodeType`.


That makes sense 👍

jeddai · 2023-10-12T21:28:51Z

components/support/nimbus-fml/src/intermediate_representation.rs

+            }
+        }
+        Ok(())
+    }


Ah that's a great refactor

jeddai · 2023-10-12T21:31:29Z

components/support/nimbus-fml/test/pref_overrides.kts

Just a thought — I'm not the biggest fan of having unnamed tests like this, at some point I think it would be nice to refactor the kts files into more standard junit tests

Please file an issue! I agree, it's getting unwieldy adding these to workflows.rs and a kts and swift file.

jeddai · 2023-10-12T21:43:26Z

components/support/nimbus-fml/src/backends/kotlin/templates/macros.kt

+            }
+         } ?: getter()
+      }
+   {%- endif %}


Honestly I really like the chained ?:

jhugman · 2023-10-13T23:56:03Z

components/nimbus/ios/Nimbus/FeatureHolder.swift

+}
+
+public extension FMLFeatureInterface {
+    func isModified() -> Bool {


Adding public gives:

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

jhugman requested a review from jeddai October 11, 2023 13:04

jhugman force-pushed the jhugman/exp-2822-add-pref-access-to-fml branch 3 times, most recently from 9643ee2 to cd9fd8e Compare October 12, 2023 12:18

jhugman commented Oct 12, 2023

View reviewed changes

jhugman force-pushed the jhugman/exp-2822-add-pref-access-to-fml branch 2 times, most recently from 26f4b36 to 1d53f2f Compare October 12, 2023 16:41

jhugman added 10 commits October 12, 2023 17:48

Refactor FieldBody to FeatureFieldBody to add pref-key

f97ae4f

Kotlin: Add SharedPreferences to helper files

1d9e3a1

Kotlin: Generating preference getting in templated code

13b1faf

Kotlin: Add integration tests for preference-overrides

e2edb95

Kotlin: add isModified to detect preference overrides

5c9ca0e

Swift: Add UserDefaults to helper files

badaf0a

Swift: Generating preference getting in templated code

cdbb5a6

Swift: Kotlin: Add integration tests for preference-overrides

dab7894

Swift: add isModified to detect preference overrides

67267c0

Add changelog

426a907

jhugman force-pushed the jhugman/exp-2822-add-pref-access-to-fml branch from 1d53f2f to 426a907 Compare October 12, 2023 16:50

jeddai approved these changes Oct 13, 2023

View reviewed changes

jhugman commented Oct 13, 2023

View reviewed changes

jhugman added this pull request to the merge queue Oct 14, 2023

Merged via the queue into main with commit e5a6dc1 Oct 14, 2023
16 checks passed

jhugman deleted the jhugman/exp-2822-add-pref-access-to-fml branch October 14, 2023 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `pref-key` to FML feature variables #5862

Add `pref-key` to FML feature variables #5862

jhugman commented Oct 11, 2023 •

edited

Loading

codecov-commenter commented Oct 11, 2023 •

edited

Loading

jhugman left a comment

jhugman Oct 12, 2023

jhugman Oct 12, 2023

jhugman Oct 12, 2023

jhugman Oct 12, 2023

jhugman Oct 12, 2023 •

edited

Loading

jeddai Oct 12, 2023

jhugman Oct 12, 2023 •

edited

Loading

jhugman Oct 12, 2023

jhugman Oct 12, 2023

jeddai Oct 12, 2023

jhugman Oct 12, 2023

jeddai left a comment

jeddai Oct 12, 2023

jhugman Oct 13, 2023

jeddai Oct 13, 2023

jeddai Oct 12, 2023

jeddai Oct 12, 2023

jeddai Oct 12, 2023

jhugman Oct 14, 2023

jeddai Oct 12, 2023

jhugman Oct 13, 2023

Add pref-key to FML feature variables #5862

Add pref-key to FML feature variables #5862

Conversation

jhugman commented Oct 11, 2023 • edited Loading

Pull Request checklist

codecov-commenter commented Oct 11, 2023 • edited Loading

Codecov Report

jhugman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhugman Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeddai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `pref-key` to FML feature variables #5862

Add `pref-key` to FML feature variables #5862

jhugman commented Oct 11, 2023 •

edited

Loading

codecov-commenter commented Oct 11, 2023 •

edited

Loading

jhugman Oct 12, 2023 •

edited

Loading

jhugman Oct 12, 2023 •

edited

Loading