update for PR feedback

mozilla · Jun 27, 2023 · a20e265 · a20e265
1 parent 4b622f3
commit a20e265
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 155 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,11 +6,12 @@
 - Android: The JVM compatibility target is now version 17 ([#5651](https://github.com/mozilla/application-services/pull/5651))
   - _NOTE: This is technically a breaking change, but all existing downstream projects have already made the necessary changes._
 
-## Nimbus ⛅️🔬🔭
+## Nimbus SDK ⛅️🔬🔭
 
 ### 🦊 What's Changed 🦊
 
-- Add `enrollments` value to `TargetingAttributes` (Nimbus only) — it is a set of strings containing all enrollments, past and present ([#5685](https://github.com/mozilla/application-services/pull/5685)).
+- Add `enrollments` value to `TargetingAttributes` — it is a set of strings containing all enrollments, past and present ([#5685](https://github.com/mozilla/application-services/pull/5685)).
+  - _Note: This change only applies to stateful uses of the Nimbus SDK, e.g. mobile_
 
 ## Nimbus FML ⛅️🔬🔭🔧
 

diff --git a/components/nimbus/src/nimbus_client.rs b/components/nimbus/src/nimbus_client.rs
@@ -311,12 +311,15 @@ impl NimbusClient {
         let mut is_enrolled_set = HashSet::<String>::new();
         let mut all_enrolled_set = HashSet::<String>::new();
         for ee in prev_enrollments {
-            if let EnrollmentStatus::Enrolled { .. } = ee.status {
-                is_enrolled_set.insert(ee.slug.clone());
-                all_enrolled_set.insert(ee.slug.clone());
-            }
-            if let EnrollmentStatus::WasEnrolled { .. } = ee.status {
-                all_enrolled_set.insert(ee.slug.clone());
+            match ee.status {
+                EnrollmentStatus::Enrolled { .. } => {
+                    is_enrolled_set.insert(ee.slug.clone());
+                    all_enrolled_set.insert(ee.slug.clone());
+                }
+                EnrollmentStatus::WasEnrolled { .. } => {
+                    all_enrolled_set.insert(ee.slug.clone());
+                }
+                _ => {}
             }
         }
 

diff --git a/components/nimbus/src/tests/stateful/test_nimbus.rs b/components/nimbus/src/tests/stateful/test_nimbus.rs
@@ -972,39 +972,11 @@ fn test_fetch_enabled() -> Result<()> {
     Ok(())
 }
 
-#[test]
-fn test_active_enrollment_in_targeting() -> Result<()> {
-    let mock_client_id = "client-1".to_string();
-
-    let temp_dir = tempfile::tempdir()?;
-
-    let app_context = AppContext {
-        app_name: "fenix".to_string(),
-        app_id: "org.mozilla.fenix".to_string(),
-        channel: "nightly".to_string(),
-        ..Default::default()
-    };
-    let mut client = NimbusClient::new(
-        app_context.clone(),
-        temp_dir.path(),
-        None,
-        AvailableRandomizationUnits {
-            client_id: Some(mock_client_id),
-            ..AvailableRandomizationUnits::default()
-        },
-    )?;
-    let targeting_attributes = TargetingAttributes {
-        app_context,
-        ..Default::default()
-    };
-    client.with_targeting_attributes(targeting_attributes);
-    client.initialize()?;
-
-    // Apply an initial experiment
-    let experiment_json = serde_json::to_string(&json!({
+fn generate_experiment(slug: &str, targeting: &str) -> Result<String> {
+    Ok(serde_json::to_string(&json!({
         "data": [{
             "schemaVersion": "1.0.0",
-            "slug": "test-1",
+            "slug": slug,
             "endDate": null,
             "featureIds": ["some-feature-1"],
             "branches": [
@@ -1029,7 +1001,7 @@ fn test_active_enrollment_in_targeting() -> Result<()> {
                 "namespace": "secure-gold",
                 "randomizationUnit": "nimbus_id"
             },
-            "targeting": "true",
+            "targeting": targeting,
             "userFacingName": "test experiment",
             "referenceBranch": "control",
             "isEnrollmentPaused": false,
@@ -1038,7 +1010,39 @@ fn test_active_enrollment_in_targeting() -> Result<()> {
             "id": "secure-copper",
             "last_modified": 1_602_197_324_372i64,
         }
-    ]}))?;
+    ]}))?)
+}
+
+#[test]
+fn test_active_enrollment_in_targeting() -> Result<()> {
+    let mock_client_id = "client-1".to_string();
+
+    let temp_dir = tempfile::tempdir()?;
+
+    let app_context = AppContext {
+        app_name: "fenix".to_string(),
+        app_id: "org.mozilla.fenix".to_string(),
+        channel: "nightly".to_string(),
+        ..Default::default()
+    };
+    let mut client = NimbusClient::new(
+        app_context.clone(),
+        temp_dir.path(),
+        None,
+        AvailableRandomizationUnits {
+            client_id: Some(mock_client_id),
+            ..AvailableRandomizationUnits::default()
+        },
+    )?;
+    let targeting_attributes = TargetingAttributes {
+        app_context,
+        ..Default::default()
+    };
+    client.with_targeting_attributes(targeting_attributes);
+    client.initialize()?;
+
+    // Apply an initial experiment
+    let experiment_json = generate_experiment("test-1", "true")?;
     client.set_experiments_locally(experiment_json)?;
     client.apply_pending_experiments()?;
 
@@ -1049,44 +1053,7 @@ fn test_active_enrollment_in_targeting() -> Result<()> {
     assert!(targeting_helper.eval_jexl("'test-1' in active_experiments".to_string())?);
 
     // Apply experiment that targets the above experiment is in enrollments
-    let experiment_json = serde_json::to_string(&json!({
-        "data": [{
-            "schemaVersion": "1.0.0",
-            "slug": "test-2",
-            "endDate": null,
-            "featureIds": ["some-feature-1"],
-            "branches": [
-                {
-                "slug": "control",
-                "ratio": 1
-                },
-                {
-                "slug": "treatment",
-                "ratio": 1
-                }
-            ],
-            "channel": "nightly",
-            "probeSets": [],
-            "startDate": null,
-            "appName": "fenix",
-            "appId": "org.mozilla.fenix",
-            "bucketConfig": {
-                "count": 10000,
-                "start": 0,
-                "total": 10000,
-                "namespace": "secure-gold",
-                "randomizationUnit": "nimbus_id"
-            },
-            "targeting": "'test-1' in enrollments",
-            "userFacingName": "test experiment",
-            "referenceBranch": "control",
-            "isEnrollmentPaused": false,
-            "proposedEnrollment": 7,
-            "userFacingDescription": "This is a test experiment for testing purposes.",
-            "id": "secure-copper",
-            "last_modified": 1_602_197_324_372i64,
-        }
-    ]}))?;
+    let experiment_json = generate_experiment("test-2", "'test-1' in enrollments")?;
     client.set_experiments_locally(experiment_json)?;
     client.apply_pending_experiments()?;
 
@@ -1131,44 +1098,7 @@ fn test_previous_enrollment_in_targeting() -> Result<()> {
     client.initialize()?;
 
     // Apply an initial experiment
-    let experiment_json = serde_json::to_string(&json!({
-        "data": [{
-            "schemaVersion": "1.0.0",
-            "slug": "test-1",
-            "endDate": null,
-            "featureIds": ["some-feature-1"],
-            "branches": [
-                {
-                "slug": "control",
-                "ratio": 1
-                },
-                {
-                "slug": "treatment",
-                "ratio": 1
-                }
-            ],
-            "channel": "nightly",
-            "probeSets": [],
-            "startDate": null,
-            "appName": "fenix",
-            "appId": "org.mozilla.fenix",
-            "bucketConfig": {
-                "count": 10000,
-                "start": 0,
-                "total": 10000,
-                "namespace": "secure-gold",
-                "randomizationUnit": "nimbus_id"
-            },
-            "targeting": "true",
-            "userFacingName": "test experiment",
-            "referenceBranch": "control",
-            "isEnrollmentPaused": false,
-            "proposedEnrollment": 7,
-            "userFacingDescription": "This is a test experiment for testing purposes.",
-            "id": "secure-copper",
-            "last_modified": 1_602_197_324_372i64,
-        }
-    ]}))?;
+    let experiment_json = generate_experiment("test-1", "true")?;
     client.set_experiments_locally(experiment_json)?;
     client.apply_pending_experiments()?;
 
@@ -1191,44 +1121,7 @@ fn test_previous_enrollment_in_targeting() -> Result<()> {
     assert!(targeting_helper.eval_jexl("'test-1' in enrollments".into())?);
 
     // Apply experiment that targets the above experiment is in enrollments
-    let experiment_json = serde_json::to_string(&json!({
-        "data": [{
-            "schemaVersion": "1.0.0",
-            "slug": "test-2",
-            "endDate": null,
-            "featureIds": ["some-feature-1"],
-            "branches": [
-                {
-                "slug": "control",
-                "ratio": 1
-                },
-                {
-                "slug": "treatment",
-                "ratio": 1
-                }
-            ],
-            "channel": "nightly",
-            "probeSets": [],
-            "startDate": null,
-            "appName": "fenix",
-            "appId": "org.mozilla.fenix",
-            "bucketConfig": {
-                "count": 10000,
-                "start": 0,
-                "total": 10000,
-                "namespace": "secure-gold",
-                "randomizationUnit": "nimbus_id"
-            },
-            "targeting": "'test-1' in enrollments",
-            "userFacingName": "test experiment",
-            "referenceBranch": "control",
-            "isEnrollmentPaused": false,
-            "proposedEnrollment": 7,
-            "userFacingDescription": "This is a test experiment for testing purposes.",
-            "id": "secure-copper",
-            "last_modified": 1_602_197_324_372i64,
-        }
-    ]}))?;
+    let experiment_json = generate_experiment("test-2", "'test-1' in enrollments")?;
     client.set_experiments_locally(experiment_json)?;
     client.apply_pending_experiments()?;