From 9453db4410909e0b59865b0a41fcd93fa960b9cd Mon Sep 17 00:00:00 2001
From: Arturo Mejia <arturomejiamarmol@gmail.com>
Date: Thu, 10 Sep 2020 16:59:30 -0400
Subject: [PATCH] Close #8354: Do not restart FAILED downloads

---
 .../feature/downloads/DownloadMiddleware.kt   |  6 ++--
 .../downloads/DownloadMiddlewareTest.kt       | 29 +++++++++++++++++++
 docs/changelog.md                             |  4 ++-
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt
index e037a3d9bb3..80826b23994 100644
--- a/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt
+++ b/components/feature/downloads/src/main/java/mozilla/components/feature/downloads/DownloadMiddleware.kt
@@ -20,6 +20,7 @@ import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.state.content.DownloadState.Status.CANCELLED
 import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED
+import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED
 import mozilla.components.lib.state.Middleware
 import mozilla.components.lib.state.MiddlewareContext
 import mozilla.components.lib.state.Store
@@ -110,8 +111,9 @@ class DownloadMiddleware(
         }
     }
 
-    private fun sendDownloadIntent(download: DownloadState) {
-        if (download.status !in arrayOf(COMPLETED, CANCELLED)) {
+    @VisibleForTesting
+    internal fun sendDownloadIntent(download: DownloadState) {
+        if (download.status !in arrayOf(COMPLETED, CANCELLED, FAILED)) {
             val intent = Intent(applicationContext, downloadServiceClass)
             intent.putExtra(DownloadManager.EXTRA_DOWNLOAD_ID, download.id)
             startForegroundService(intent)
diff --git a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt
index 43a87f1ddff..bde65aada38 100644
--- a/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt
+++ b/components/feature/downloads/src/test/java/mozilla/components/feature/downloads/DownloadMiddlewareTest.kt
@@ -21,6 +21,8 @@ import mozilla.components.browser.state.state.BrowserState
 import mozilla.components.browser.state.state.content.DownloadState
 import mozilla.components.browser.state.state.content.DownloadState.Status.COMPLETED
 import mozilla.components.browser.state.state.content.DownloadState.Status.INITIATED
+import mozilla.components.browser.state.state.content.DownloadState.Status.FAILED
+import mozilla.components.browser.state.state.content.DownloadState.Status.CANCELLED
 import mozilla.components.browser.state.store.BrowserStore
 import mozilla.components.support.test.any
 import mozilla.components.support.test.argumentCaptor
@@ -38,6 +40,7 @@ import org.mockito.Mockito.verify
 import org.mockito.Mockito.times
 import org.mockito.Mockito.never
 import org.mockito.Mockito.spy
+import org.mockito.Mockito.reset
 
 @RunWith(AndroidJUnit4::class)
 class DownloadMiddlewareTest {
@@ -222,4 +225,30 @@ class DownloadMiddlewareTest {
 
         assertEquals(download, store.state.downloads.values.first())
     }
+
+    @Test
+    fun `sendDownloadIntent MUST call startForegroundService WHEN downloads are NOT COMPLETED, CANCELLED and FAILED`() = runBlockingTest {
+        val applicationContext: Context = mock()
+        val downloadMiddleware = spy(DownloadMiddleware(
+            applicationContext,
+            AbstractFetchDownloadService::class.java
+        ))
+
+        val ignoredStatus = listOf(COMPLETED, CANCELLED, FAILED)
+        ignoredStatus.forEach { status ->
+            val download = DownloadState("https://mozilla.org/download", status = status)
+            downloadMiddleware.sendDownloadIntent(download)
+            verify(downloadMiddleware, times(0)).startForegroundService(any())
+        }
+
+        reset(downloadMiddleware)
+
+        val allowedStatus = DownloadState.Status.values().filter { it !in ignoredStatus }
+
+        allowedStatus.forEachIndexed { index, status ->
+            val download = DownloadState("https://mozilla.org/download", status = status)
+            downloadMiddleware.sendDownloadIntent(download)
+            verify(downloadMiddleware, times(index + 1)).startForegroundService(any())
+        }
+    }
 }
diff --git a/docs/changelog.md b/docs/changelog.md
index 183ceae405b..29873c56cd2 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -15,6 +15,9 @@ permalink: /changelog/
 * **feature-recentlyclosed**
   * Added a new [RecentlyClosedTabsStorage] and a [RecentlyClosedMiddleware] to maintain a list of restorable recently closed tabs.
 
+* **feature-downloads**
+  * 🚒 Bug [issue #8354](https://github.com/mozilla-mobile/android-components/issues/8354) Do not restart FAILED downloads.
+
 # 57.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v56.0.0...v57.0.0)
@@ -66,7 +69,6 @@ permalink: /changelog/
   * 🚒 Bug [issue #8190](https://github.com/mozilla-mobile/android-components/issues/8190) ArithmeticException: divide by zero in Download notification.
   * 🚒 Bug [issue #8363](https://github.com/mozilla-mobile/android-components/issues/8363) IllegalStateException: Not allowed to start service Intent.
 
-
 * **ui-widgets**
   * 🆕 New VerticalSwipeRefreshLayout that comes to resolve many of the issues of the platform SwipeRefreshLayout and filters out other gestures than swipe down/up.