Queue show notes downloading (#1390)

Automattic · Sep 21, 2023 · fa5a2e1 · fa5a2e1
1 parent 8bff7dd
commit fa5a2e1
Show file tree

Hide file tree

Showing 7 changed files with 74 additions and 31 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,10 +3,12 @@
 
 *   New Feature:
     *   Suggest episodes to play in Automotive
-        ([#1362](https://github.com/Automattic/pocket-casts-android/pull/1362)).
+        ([#1362](https://github.com/Automattic/pocket-casts-android/pull/1362))
 *   Bug Fixes:
     *   Avoid memory leak when opening Up Next queue
         ([#1397](https://github.com/Automattic/pocket-casts-android/pull/1397))
+    *   Improved the downloading of episode show notes
+        ([#1390](https://github.com/Automattic/pocket-casts-android/pull/1390))
 
 7.47
 -----

diff --git a/...rc/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt b/...rc/main/java/au/com/shiftyjelly/pocketcasts/podcasts/viewmodel/PodcastRatingsViewModel.kt
@@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.StateFlow
 import kotlinx.coroutines.flow.stateIn
 import kotlinx.coroutines.flow.update
 import kotlinx.coroutines.launch
+import retrofit2.HttpException
 import timber.log.Timber
 import java.io.IOException
 import javax.inject.Inject
@@ -62,7 +63,13 @@ class PodcastRatingsViewModel
             try {
                 ratingsManager.refreshPodcastRatings(uuid)
             } catch (e: Exception) {
-                Timber.e(e, "Failed to refresh podcast ratings")
+                val message = "Failed to refresh podcast ratings"
+                // don't report missing rating or network errors to Sentry
+                if (e is HttpException || e is IOException) {
+                    Timber.i(e, message)
+                } else {
+                    Timber.e(e, message)
+                }
             }
         }
     }

diff --git a/...src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt b/...src/main/java/au/com/shiftyjelly/pocketcasts/repositories/download/DownloadManagerImpl.kt
@@ -338,24 +338,13 @@ class DownloadManagerImpl @Inject constructor(
                 .addTag(episode.uuid)
                 .build()
 
-            val tasks = mutableListOf(downloadTask)
-            if (episode is PodcastEpisode) {
-                val cacheShowNotesData = Data.Builder()
-                    .putString(UpdateShowNotesTask.INPUT_PODCAST_UUID, episode.podcastUuid)
-                    .putString(UpdateShowNotesTask.INPUT_EPISODE_UUID, episode.uuid)
-                    .build()
-                val cacheShowNotesTask = OneTimeWorkRequestBuilder<UpdateShowNotesTask>()
-                    .setInputData(cacheShowNotesData)
-                    .addTag(episode.uuid)
-                    .build()
-                tasks.add(cacheShowNotesTask)
-            }
+            UpdateShowNotesTask.enqueue(episode, constraints, context)
 
             episodeManager.updateDownloadTaskId(episode, downloadTask.id.toString())
             WorkManager
                 .getInstance(context)
                 .beginWith(updateTask)
-                .then(tasks)
+                .then(downloadTask)
                 .enqueue()
         } catch (storageException: StorageException) {
             launch(downloadsCoroutineContext) {

diff --git a/...ain/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt b/...ain/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/DownloadEpisodeTask.kt
@@ -5,6 +5,7 @@ import android.media.MediaExtractor
 import android.media.MediaFormat
 import android.system.ErrnoException
 import android.system.OsConstants
+import android.util.Log
 import androidx.hilt.work.HiltWorker
 import androidx.work.Data
 import androidx.work.ForegroundInfo
@@ -125,13 +126,13 @@ class DownloadEpisodeTask @AssistedInject constructor(
             this.episode = runBlocking { episodeManager.findEpisodeByUuid(episodeUUID!!) } ?: return Result.failure()
 
             if (this.episode.downloadTaskId != id.toString()) {
-                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling.")
+                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Mismatched download task id for episode ${episode.title}. Cancelling. downloadTaskId: ${this.episode.downloadTaskId} id: $id.")
                 return Result.failure()
             }
 
             if (this.episode.isArchived) {
                 // In case the episode was archived again but the cancellation event hasn't come through yet
-                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Episode ${episode.title} is archived in download task. Cancelling.")
+                LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Episode ${episode.title} is archived in download task. Cancelling.")
                 return Result.success(Data.Builder().putBoolean(OUTPUT_CANCELLED, true).build())
             }
 
@@ -193,7 +194,7 @@ class DownloadEpisodeTask @AssistedInject constructor(
         runBlocking {
             val requirements = downloadManager.getRequirementsAndSetStatusAsync(episode)
             val message = e?.toString() ?: "No exception"
-            LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Download stopped, will retry with $requirements ${episode.title} ${episode.uuid} - $message")
+            LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Download stopped, will retry with $requirements ${episode.title} ${episode.uuid} - $message")
         }
     }
 
@@ -211,7 +212,7 @@ class DownloadEpisodeTask @AssistedInject constructor(
         val message = if (downloadMessage.isNullOrBlank()) "Download Failed" else downloadMessage
         episodeManager.updateDownloadErrorDetails(episode, message)
 
-        LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, e, "Download failed ${episode.title} ${episode.uuid} - $message")
+        LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, e, "Download failed ${episode.title} ${episode.uuid} - $message")
 
         return outputData
     }
@@ -510,7 +511,9 @@ class DownloadEpisodeTask @AssistedInject constructor(
             call?.cancel()
         }
         if (exception != null) {
-            LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, exception, "Download failed %s", this.episode.downloadUrl ?: "")
+            // don't report issues such as timeouts to Sentry
+            val logPriority = if (exception is IOException) Log.INFO else Log.ERROR
+            LogBuffer.addLog(logPriority, LogBuffer.TAG_BACKGROUND_TASKS, exception, "Download failed %s", this.episode.downloadUrl ?: "")
         }
 
         if (!emitter.isDisposed) {
@@ -575,13 +578,13 @@ class DownloadEpisodeTask @AssistedInject constructor(
             val message = if (statusCode == 404) ERROR_FILE_NOT_FOUND else String.format(
                 ERROR_DOWNLOAD_MESSAGE, if (responseReason.isBlank()) "" else "(error $statusCode $responseReason)"
             )
-            LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid response returned for episode download. ${response.code} $responseReason ${response.request.url}")
+            LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid response returned for episode download. ${response.code} $responseReason ${response.request.url}")
             return ResponseValidationResult(isValid = false, errorMessage = message)
         }
         // check the content type is valid
         response.header("Content-Type")?.let { contentType ->
             if (INVALID_CONTENT_TYPES.any { it == contentType }) {
-                LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid content type returned for episode download. $contentType ${response.request.url}")
+                LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "Invalid content type returned for episode download. $contentType ${response.request.url}")
                 return ResponseValidationResult(isValid = false, errorMessage = ERROR_FAILED_EPISODE)
             }
         }

diff --git a/...ain/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt b/...ain/java/au/com/shiftyjelly/pocketcasts/repositories/download/task/UpdateShowNotesTask.kt
@@ -1,11 +1,19 @@
 package au.com.shiftyjelly.pocketcasts.repositories.download.task
 
 import android.content.Context
+import android.os.SystemClock
+import android.util.Log
 import androidx.hilt.work.HiltWorker
+import androidx.work.Constraints
 import androidx.work.CoroutineWorker
+import androidx.work.Data
+import androidx.work.ExistingWorkPolicy
+import androidx.work.OneTimeWorkRequestBuilder
+import androidx.work.WorkManager
 import androidx.work.WorkerParameters
+import au.com.shiftyjelly.pocketcasts.models.entity.BaseEpisode
+import au.com.shiftyjelly.pocketcasts.models.entity.PodcastEpisode
 import au.com.shiftyjelly.pocketcasts.servers.ServerShowNotesManager
-import au.com.shiftyjelly.pocketcasts.utils.SentryHelper
 import au.com.shiftyjelly.pocketcasts.utils.log.LogBuffer
 import dagger.assisted.Assisted
 import dagger.assisted.AssistedInject
@@ -21,24 +29,46 @@ class UpdateShowNotesTask @AssistedInject constructor(
     private val showNotesManager: ServerShowNotesManager
 ) : CoroutineWorker(context, params) {
     companion object {
+        private const val TASK_NAME = "UpdateShowNotesTask"
         const val INPUT_PODCAST_UUID = "podcast_uuid"
-        const val INPUT_EPISODE_UUID = "episode_uuid"
+
+        fun enqueue(episode: BaseEpisode, constraints: Constraints = Constraints.NONE, context: Context) {
+            if (episode !is PodcastEpisode) {
+                return
+            }
+
+            LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "$TASK_NAME - enqueued ${episode.uuid}")
+            val cacheShowNotesData = Data.Builder()
+                .putString(INPUT_PODCAST_UUID, episode.podcastUuid)
+                .build()
+            val workRequest = OneTimeWorkRequestBuilder<UpdateShowNotesTask>()
+                .setInputData(cacheShowNotesData)
+                .addTag(episode.uuid)
+                .setConstraints(constraints)
+                .build()
+            WorkManager.getInstance(context).beginUniqueWork(TASK_NAME, ExistingWorkPolicy.APPEND_OR_REPLACE, workRequest).enqueue()
+        }
     }
 
     private val podcastUuid = inputData.getString(INPUT_PODCAST_UUID) ?: ""
-    private val episodeUuid = inputData.getString(INPUT_EPISODE_UUID) ?: ""
 
     override suspend fun doWork(): Result {
+        info("Worker started - podcast: $podcastUuid")
+        val startTime = SystemClock.elapsedRealtime()
         return try {
-            showNotesManager.downloadShowNotes(podcastUuid = podcastUuid, episodeUuid = episodeUuid)
+            showNotesManager.downloadToCacheShowNotes(podcastUuid = podcastUuid)
+            info("Worker completed - took ${SystemClock.elapsedRealtime() - startTime} ms")
             Result.success()
         } catch (e: Exception) {
-            LogBuffer.e(LogBuffer.TAG_BACKGROUND_TASKS, e, "Failed to update show notes")
-            if (e !is HttpException) {
-                SentryHelper.recordException(e)
-            }
+            info("Worker failed - took ${SystemClock.elapsedRealtime() - startTime} ms")
+            val logPriority = if (e is HttpException) Log.INFO else Log.ERROR
+            LogBuffer.addLog(logPriority, LogBuffer.TAG_BACKGROUND_TASKS, e, "Failed to update show notes")
             // Don't keep retrying if the download fails. The user can download the show notes when viewing them.
             Result.success()
         }
     }
+
+    private fun info(message: String) {
+        LogBuffer.i(LogBuffer.TAG_BACKGROUND_TASKS, "$TASK_NAME (Worker ID: $id) - $message")
+    }
 }
diff --git a/...es/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt b/...es/servers/src/main/java/au/com/shiftyjelly/pocketcasts/servers/ServerShowNotesManager.kt
@@ -86,4 +86,11 @@ class ServerShowNotesManager @Inject constructor(
         val response = podcastCacheServerManager.getShowNotes(podcastUuid = podcastUuid)
         return response.findEpisode(episodeUuid)?.showNotes
     }
+
+    suspend fun downloadToCacheShowNotes(podcastUuid: String) {
+        if (podcastUuid.isBlank()) {
+            return
+        }
+        podcastCacheServerManager.getShowNotes(podcastUuid = podcastUuid)
+    }
 }
diff --git a/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt b/modules/services/utils/src/main/java/au/com/shiftyjelly/pocketcasts/utils/log/LogBuffer.kt
@@ -3,6 +3,7 @@ package au.com.shiftyjelly.pocketcasts.utils.log
 import android.util.Log
 import au.com.shiftyjelly.pocketcasts.utils.FileUtil
 import timber.log.Timber
+import timber.log.Timber.Forest.tag
 import java.io.File
 import java.io.FileWriter
 import java.io.IOException
@@ -110,6 +111,10 @@ object LogBuffer {
         addLog(Log.INFO, tag, null, message, *args)
     }
 
+    fun i(tag: String, throwable: Throwable, message: String, vararg args: Any) {
+        addLog(Log.INFO, tag, throwable, message, *args)
+    }
+
     fun w(tag: String, message: String, vararg args: Any) {
         addLog(Log.WARN, tag, null, message, *args)
     }
@@ -127,7 +132,7 @@ object LogBuffer {
     }
 
     @Suppress("NAME_SHADOWING")
-    private fun addLog(priority: Int, tag: String, throwable: Throwable?, message: String?, vararg args: Any) {
+    fun addLog(priority: Int, tag: String, throwable: Throwable?, message: String?, vararg args: Any) {
         var message = message
         if (message != null && message.isEmpty()) {
             message = null