From daf88cc58ae28359f7cf7494b673b948db1c10ea Mon Sep 17 00:00:00 2001
From: James Hugman <james@hugman.tv>
Date: Wed, 3 May 2023 13:14:41 +0100
Subject: [PATCH] Bug 1830080 Add more careful handling of preview-collection
 in maybeFetchExperiments

(cherry picked from commit 22d398fe20df0b0d19fa3ca6de196ac74acb311c)
---
 .../mozilla/fenix/experiments/NimbusSetup.kt  | 20 +++---
 .../mozilla/fenix/nimbus/NimbusSystemTest.kt  | 61 +++++++++++++++++++
 2 files changed, 71 insertions(+), 10 deletions(-)

diff --git a/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt
index 5fd84dd8e6..db7d1c1591 100644
--- a/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt
+++ b/app/src/main/java/org/mozilla/fenix/experiments/NimbusSetup.kt
@@ -109,17 +109,17 @@ fun NimbusInterface.maybeFetchExperiments(
     feature: NimbusSystem = FxNimbusMessaging.features.nimbusSystem.value(),
     currentTimeMillis: Long = System.currentTimeMillis(),
 ) {
-    val minimumPeriodMinutes = if (!context.settings().nimbusUsePreview) {
-        feature.refreshIntervalForeground
+    if (context.settings().nimbusUsePreview) {
+        context.settings().nimbusLastFetchTime = 0L
+        fetchExperiments()
     } else {
-        0
-    }
-
-    val lastFetchTimeMillis = context.settings().nimbusLastFetchTime
-    val minimumPeriodMillis = minimumPeriodMinutes * Settings.ONE_MINUTE_MS
+        val minimumPeriodMinutes = feature.refreshIntervalForeground
+        val lastFetchTimeMillis = context.settings().nimbusLastFetchTime
+        val minimumPeriodMillis = minimumPeriodMinutes * Settings.ONE_MINUTE_MS
 
-    if (currentTimeMillis - lastFetchTimeMillis >= minimumPeriodMillis) {
-        context.settings().nimbusLastFetchTime = currentTimeMillis
-        fetchExperiments()
+        if (currentTimeMillis - lastFetchTimeMillis >= minimumPeriodMillis) {
+            context.settings().nimbusLastFetchTime = currentTimeMillis
+            fetchExperiments()
+        }
     }
 }
diff --git a/app/src/test/java/org/mozilla/fenix/nimbus/NimbusSystemTest.kt b/app/src/test/java/org/mozilla/fenix/nimbus/NimbusSystemTest.kt
index 3a0dbf2201..b3e2362b4d 100644
--- a/app/src/test/java/org/mozilla/fenix/nimbus/NimbusSystemTest.kt
+++ b/app/src/test/java/org/mozilla/fenix/nimbus/NimbusSystemTest.kt
@@ -100,4 +100,65 @@ class NimbusSystemTest {
         )
         assertTrue(nimbus.isFetching)
     }
+
+    @Test
+    fun `GIVEN a nimbus object calling maybeFetchExperiments WHEN using a preview collection THEN always call fetchExperiments`() {
+        var currentTime = 0L
+        fun assertFetchEveryTime() {
+            nimbus.maybeFetchExperiments(
+                context,
+                config,
+                currentTime,
+            )
+            assertTrue(nimbus.isFetching)
+            assertEquals(lastTimeSlot.captured, 0L)
+            nimbus.isFetching = false
+        }
+
+        // Using usePreview, we call fetch every time we call maybeFetch.
+        every { settings.nimbusUsePreview } returns true
+        currentTime = Settings.ONE_HOUR_MS
+        assertFetchEveryTime()
+
+        currentTime += Settings.ONE_MINUTE_MS
+        assertFetchEveryTime()
+
+        currentTime += Settings.ONE_MINUTE_MS
+        assertFetchEveryTime()
+
+        // Now turn preview collection off.
+        // We should fetch exactly once…
+        every { settings.nimbusUsePreview } returns false
+
+        currentTime += Settings.ONE_MINUTE_MS
+        nimbus.maybeFetchExperiments(
+            context,
+            config,
+            currentTime,
+        )
+        assertTrue(nimbus.isFetching)
+        assertEquals(lastTimeSlot.captured, currentTime)
+        nimbus.isFetching = false
+        every { settings.nimbusLastFetchTime } returns currentTime
+
+        // … and then back off. We show here that the next call to maybeFetch
+        // doesn't call fetch.
+        currentTime += Settings.ONE_MINUTE_MS
+        nimbus.maybeFetchExperiments(
+            context,
+            config,
+            currentTime,
+        )
+        assertFalse(nimbus.isFetching)
+
+        // Now wait, another hour, and we've reset the behaviour back to normal operation.
+        currentTime += Settings.ONE_HOUR_MS + Settings.ONE_MINUTE_MS
+        nimbus.maybeFetchExperiments(
+            context,
+            config,
+            currentTime,
+        )
+        assertTrue(nimbus.isFetching)
+        assertEquals(lastTimeSlot.captured, currentTime)
+    }
 }