From 16b8d4b607294cf465c8ad742d11c9cc4d12c232 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Tue, 15 Feb 2022 16:53:43 -0600
Subject: [PATCH] uploads: consider uploads as failed when all assets fail.

Make the "completed" status for an upload mean "at least one file in the
upload successfully completed". The "error" status means "all files in
the upload failed".

This means that when an upload has multiple assets and some succeed and
some fail, the whole upload is considered completed. This can happen
when uploading multiple files and some files are over the size limit,
for example. The upload is considered failed only if all files in the
upload fail.

This fixes an issue where, if uploading a single file and that file
failed because it was over the size limit, then the upload wouldn't be
marked as failed.
---
 .../src/javascripts/file_upload_component.js       | 14 +++++++-------
 app/models/upload_media_asset.rb                   |  6 +++++-
 test/functional/uploads_controller_test.rb         |  4 +---
 3 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/app/javascript/src/javascripts/file_upload_component.js b/app/javascript/src/javascripts/file_upload_component.js
index ee4545e7c..89f293db1 100644
--- a/app/javascript/src/javascripts/file_upload_component.js
+++ b/app/javascript/src/javascripts/file_upload_component.js
@@ -101,7 +101,13 @@ export default class FileUploadComponent {
       upload = await $.get(`/uploads/${upload.id}.json`);
     }
 
-    if (upload.media_asset_count > 0) {
+    if (upload.status === "error") {
+      this.$dropzone.removeClass("success");
+      this.$component.find("progress").addClass("hidden");
+      this.$component.find("input").removeAttr("disabled");
+
+      Utility.error(`Upload failed: ${upload.error}.`);
+    } else {
       let params = new URLSearchParams(window.location.search);
       let isBookmarklet = params.has("url");
       params.delete("url");
@@ -115,12 +121,6 @@ export default class FileUploadComponent {
       } else {
         window.location.assign(url);
       }
-    } else if (upload.status === "error") {
-      this.$dropzone.removeClass("success");
-      this.$component.find("progress").addClass("hidden");
-      this.$component.find("input").removeAttr("disabled");
-
-      Utility.error(`Upload failed: ${upload.error}.`);
     }
   }
 
diff --git a/app/models/upload_media_asset.rb b/app/models/upload_media_asset.rb
index 7eec97a3f..0b3e8c659 100644
--- a/app/models/upload_media_asset.rb
+++ b/app/models/upload_media_asset.rb
@@ -68,7 +68,11 @@ class UploadMediaAsset < ApplicationRecord
 
   def update_upload_status
     upload.with_lock do
-      upload.update!(status: "completed") if upload.upload_media_assets.all?(&:finished?)
+      if upload.upload_media_assets.all?(&:failed?)
+        upload.update!(status: "error", error: upload.upload_media_assets.map(&:error).join("; "))
+      elsif upload.upload_media_assets.all?(&:finished?)
+        upload.update!(status: "completed")
+      end
     end
   end
 
diff --git a/test/functional/uploads_controller_test.rb b/test/functional/uploads_controller_test.rb
index c8ea90fa2..6d26746b7 100644
--- a/test/functional/uploads_controller_test.rb
+++ b/test/functional/uploads_controller_test.rb
@@ -211,9 +211,7 @@ class UploadsControllerTest < ActionDispatch::IntegrationTest
 
       context "for a video longer than the video length limit" do
         should "fail for a regular user" do
-          @source = "https://cdn.donmai.us/original/63/cb/63cb09f2526ef3ac14f11c011516ad9b.webm"
-          post_auth uploads_path(format: :json), @user, params: { upload: { source: @source }}
-          perform_enqueued_jobs
+          create_upload!("https://cdn.donmai.us/original/63/cb/63cb09f2526ef3ac14f11c011516ad9b.webm", user: @user)
 
           assert_response 201
           assert_match("Duration must be less than", Upload.last.error)