Ensure we only generate a deletion-request ping when toggling from on to off

glean-core/android/src/main/java/mozilla/telemetry/glean/Glean.kt

@@ -63,7 +63,7 @@ open class GleanInternalAPI internal constructor () {
     // This is the wrapped http uploading mechanism: provides base functionalities
     // for logging and delegates the actual upload to the implementation in
     // the `Configuration`.
-    internal val httpClient by lazy { BaseUploader(configuration.httpClient) }
+    internal lateinit var httpClient: BaseUploader
 
     private lateinit var applicationContext: Context
 
@@ -140,6 +140,7 @@ open class GleanInternalAPI internal constructor () {
         this.applicationContext = applicationContext
 
         this.configuration = configuration
+        this.httpClient = BaseUploader(configuration.httpClient)
         this.gleanDataDir = File(applicationContext.applicationInfo.dataDir, GLEAN_DATA_DIR)
 
         setUploadEnabled(uploadEnabled)

glean-core/android/src/test/java/mozilla/telemetry/glean/TestUtil.kt

@@ -208,11 +208,12 @@ internal fun waitForEnqueuedWorker(
  * is fixed, the contents of this can be amended to trigger WorkManager directly.
  *
  * @param context the application [Context] to get the [WorkManager] instance for
+ * @param workerTag the tag for the expected worker (default: `PingUploadWorker.PING_WORKER_TAG`)
  */
-internal fun triggerWorkManager(context: Context) {
+internal fun triggerWorkManager(context: Context, workerTag: String = PingUploadWorker.PING_WORKER_TAG) {
     // Check that the work is scheduled
-    val status = getWorkerStatus(context, PingUploadWorker.PING_WORKER_TAG)
-    Assert.assertTrue("A scheduled PingUploadWorker must exist",
+    val status = getWorkerStatus(context, workerTag)
+    Assert.assertTrue("A scheduled $workerTag must exist",
         status.isEnqueued)
 
     // Trigger WorkManager using TestDriver

glean-core/android/src/test/java/mozilla/telemetry/glean/scheduler/DeletionPingUploadWorkerTest.kt

@@ -5,17 +5,27 @@ import androidx.test.core.app.ApplicationProvider
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.work.testing.WorkManagerTestInitHelper
 import java.io.File
+import mozilla.components.support.test.any
 import mozilla.telemetry.glean.Dispatchers
 import mozilla.telemetry.glean.Glean
 import mozilla.telemetry.glean.GleanInternalAPI
 import mozilla.telemetry.glean.config.Configuration
 import mozilla.telemetry.glean.getWorkerStatus
+import mozilla.telemetry.glean.net.HeadersList
+import mozilla.telemetry.glean.net.PingUploader
+import mozilla.telemetry.glean.resetGlean
+import mozilla.telemetry.glean.triggerWorkManager
 import org.junit.After
+import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mockito.anyString
+import org.mockito.Mockito.spy
+import org.mockito.Mockito.times
+import org.mockito.Mockito.verify
 import org.robolectric.shadows.ShadowLog
 
 @RunWith(AndroidJUnit4::class)
@@ -63,4 +73,44 @@ class DeletionPingUploadWorkerTest {
 
         assertTrue(getWorkerStatus(context, DeletionPingUploadWorker.PING_WORKER_TAG).isEnqueued)
     }
+
+    /**
+     * A stub uploader class that does not upload anything, but we can spy on it.
+     */
+    private class TestUploader : PingUploader {
+        override fun upload(url: String, data: String, headers: HeadersList): Boolean {
+            return true
+        }
+    }
+
+    @Test
+    fun `deletion-request pings are only sent when toggled from on to off`() {
+        val httpClientSpy = spy(TestUploader())
+
+        // Use the test client in the Glean configuration
+        val config = Configuration(httpClient = httpClientSpy)
+        resetGlean(context, config)
+
+        // Get directory for pending deletion-request pings
+        val pendingDeletionRequestDir = File(Glean.getDataDir(), DeletionPingUploadWorker.DELETION_PING_DIR)
+
+        // Disabling upload generates a deletion ping
+        Glean.setUploadEnabled(false)
+        assertEquals(1, pendingDeletionRequestDir.listFiles()?.size)
+
+        // Trigger the upload manager and check that the upload was started
+        triggerWorkManager(context, DeletionPingUploadWorker.PING_WORKER_TAG)
+        verify(httpClientSpy, times(1)).upload(anyString(), anyString(), any())
+
+        // Re-setting upload to `false` should not generate an additional ping
+        // and no worker should be scheduled.
+        Glean.setUploadEnabled(false)
+
+        assertFalse(getWorkerStatus(context, DeletionPingUploadWorker.PING_WORKER_TAG).isEnqueued)
+        // Upload was definitely not triggered again
+        verify(httpClientSpy, times(1)).upload(anyString(), anyString(), any())
+
+        // No new file should have been written
+        assertEquals(0, pendingDeletionRequestDir.listFiles()?.size)
+    }
 }

glean-core/src/lib.rs

@@ -276,14 +276,14 @@ impl Glean {
     pub fn set_upload_enabled(&mut self, flag: bool) -> bool {
         log::info!("Upload enabled: {:?}", flag);
 
-        // When upload is disabled, submit a deletion-request ping
-        if !flag {
-            if let Err(err) = self.internal_pings.deletion_request.submit(self, None) {
-                log::error!("Failed to send deletion-request ping on optout: {}", err);
+        if self.upload_enabled != flag {
+            // When upload is disabled, submit a deletion-request ping
+            if !flag {
+                if let Err(err) = self.internal_pings.deletion_request.submit(self, None) {
+                    log::error!("Failed to submit deletion-request ping on optout: {}", err);
+                }
             }
-        }
 
-        if self.upload_enabled != flag {
             self.upload_enabled = flag;
             self.on_change_upload_enabled(flag);
             true

glean-core/tests/ping.rs

@@ -64,6 +64,24 @@ fn disabling_upload_clears_pending_pings() {
     assert_eq!(1, get_queued_pings(glean.get_data_path()).unwrap().len());
 }
 
+#[test]
+fn deletion_request_only_when_toggled_from_on_to_off() {
+    let (mut glean, _) = new_glean(None);
+
+    // Disabling upload generates a deletion ping
+    glean.set_upload_enabled(false);
+    assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len());
+
+    // Re-setting it to `false` should not generate an additional ping.
+    // As we didn't clear the pending ping, that's the only one that sticks around.
+    glean.set_upload_enabled(false);
+    assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len());
+
+    // Toggling back to true won't generate a ping either.
+    glean.set_upload_enabled(true);
+    assert_eq!(1, get_deletion_pings(glean.get_data_path()).unwrap().len());
+}
+
 #[test]
 fn empty_pings_with_flag_are_sent() {
     let (mut glean, _) = new_glean(None);