FEATURE: Humanize file size error messages (#14398)

The file size error messages for max_image_size_kb and max_attachment_size_kb are shown to the user in the KB format, regardless of how large the limit is. Since we are going to support uploading much larger files soon, this KB-based limit soon becomes unfriendly to the end user. For example, if the max attachment size is set to 512000 KB, this is what the user sees: > Sorry, the file you are trying to upload is too big (maximum size is 512000KB) This makes the user do math. In almost all file explorers that a regular user would be familiar width, the file size is shown in a format based on the maximum increment (e.g. KB, MB, GB). This commit changes the behaviour to output a humanized file size instead of the raw KB. For the above example, it would now say: > Sorry, the file you are trying to upload is too big (maximum size is 512 MB) This humanization also handles decimals, e.g. 1536KB = 1.5 MB
discourse · Sep 21, 2021 · dba6a5e · dba6a5e
1 parent 3cda7ec
commit dba6a5e
Show file tree

Hide file tree

Showing 12 changed files with 78 additions and 10 deletions.
diff --git a/app/assets/javascripts/discourse/app/lib/uploads.js b/app/assets/javascripts/discourse/app/lib/uploads.js
@@ -328,7 +328,11 @@ function displayErrorByResponseStatus(status, body, fileName, siteSettings) {
     case 413:
       const type = uploadTypeFromFileName(fileName);
       const max_size_kb = siteSettings[`max_${type}_size_kb`];
-      bootbox.alert(I18n.t("post.errors.file_too_large", { max_size_kb }));
+      bootbox.alert(
+        I18n.t("post.errors.file_too_large_humanized", {
+          max_size: formatBytes(max_size_kb * 1024),
+        })
+      );
       return true;
 
     // the error message is provided by the server
@@ -354,3 +358,18 @@ export function bindFileInputChangeListener(element, fileCallbackFn) {
   element.addEventListener("change", changeListener);
   return changeListener;
 }
+
+export function formatBytes(bytes, decimals) {
+  if (bytes === 0) {
+    return "0 Bytes";
+  }
+  const kilobyte = 1024,
+    dm = decimals || 2,
+    sizes = ["Bytes", "KB", "MB", "GB", "TB", "PB", "EB", "ZB", "YB"],
+    incr = Math.floor(Math.log(bytes) / Math.log(kilobyte));
+  return (
+    parseFloat((bytes / Math.pow(kilobyte, incr)).toFixed(dm)) +
+    " " +
+    sizes[incr]
+  );
+}
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
@@ -219,7 +219,7 @@ def generate_presigned_put
 
     if file_size_too_big?(file_name, file_size)
       return render_json_error(
-        I18n.t("upload.attachments.too_large", max_size_kb: SiteSetting.max_attachment_size_kb),
+        I18n.t("upload.attachments.too_large_humanized", max_size: ActiveSupport::NumberHelper.number_to_human_size(SiteSetting.max_attachment_size_kb.kilobytes)),
         status: 422
       )
     end
@@ -296,7 +296,7 @@ def create_multipart
 
     if file_size_too_big?(file_name, file_size)
       return render_json_error(
-        I18n.t("upload.attachments.too_large", max_size_kb: SiteSetting.max_attachment_size_kb),
+        I18n.t("upload.attachments.too_large_humanized", max_size: ActiveSupport::NumberHelper.number_to_human_size(SiteSetting.max_attachment_size_kb.kilobytes)),
         status: 422
       )
     end

diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
@@ -3052,6 +3052,7 @@ en:
         edit: "Sorry, there was an error editing your post. Please try again."
         upload: "Sorry, there was an error uploading that file. Please try again."
         file_too_large: "Sorry, that file is too big (maximum size is %{max_size_kb}kb). Why not upload your large file to a cloud sharing service, then paste the link?"
+        file_too_large_humanized: "Sorry, that file is too big (maximum size is %{max_size}). Why not upload your large file to a cloud sharing service, then paste the link?"
         too_many_uploads: "Sorry, you can only upload one file at a time."
         too_many_dragged_and_dropped_files:
           one: "Sorry, you can only upload %{count} file at a time."

diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
@@ -4039,12 +4039,15 @@ en:
     cannot_promote_failure: "The upload cannot be completed, it may have already completed or previously failed."
     attachments:
       too_large: "Sorry, the file you are trying to upload is too big (maximum size is %{max_size_kb}KB)."
+      too_large_humanized: "Sorry, the file you are trying to upload is too big (maximum size is %{max_size})."
     images:
       too_large: "Sorry, the image you are trying to upload is too big (maximum size is %{max_size_kb}KB), please resize it and try again."
+      too_large_humanized: "Sorry, the image you are trying to upload is too big (maximum size is %{max_size}), please resize it and try again."
       larger_than_x_megapixels: "Sorry, the image you are trying to upload is too large (maximum dimension is %{max_image_megapixels}-megapixels), please resize it and try again."
       size_not_found: "Sorry, but we couldn't determine the size of the image. Maybe your image is corrupted?"
     placeholders:
       too_large: "(image larger than %{max_size_kb}KB)"
+      too_large_humanized: "(image larger than %{max_size})"
 
   avatar:
     missing: "Sorry, we can't find any avatar associated with that email address. Can you try uploading it again?"

diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb
@@ -142,7 +142,15 @@ def add_large_image_placeholder!(img)
     span = create_span_node("url", url)
     a.add_child(span)
     span.add_previous_sibling(create_icon_node("far-image"))
-    span.add_next_sibling(create_span_node("help", I18n.t("upload.placeholders.too_large", max_size_kb: SiteSetting.max_image_size_kb)))
+    span.add_next_sibling(
+      create_span_node(
+        "help",
+        I18n.t(
+          "upload.placeholders.too_large_humanized",
+          max_size: ActiveSupport::NumberHelper.number_to_human_size(SiteSetting.max_image_size_kb.kilobytes)
+        )
+      )
+    )
 
     # Only if the image is already linked
     if is_hyperlinked

diff --git a/lib/upload_creator.rb b/lib/upload_creator.rb
@@ -386,7 +386,9 @@ def is_still_too_big?
       @upload.errors.add(:base, I18n.t("upload.images.larger_than_x_megapixels", max_image_megapixels: SiteSetting.max_image_megapixels))
       true
     elsif max_image_size > 0 && filesize >= max_image_size
-      @upload.errors.add(:base, I18n.t("upload.images.too_large", max_size_kb: SiteSetting.max_image_size_kb))
+      @upload.errors.add(:base, I18n.t(
+        "upload.images.too_large_humanized", max_size: ActiveSupport::NumberHelper.number_to_human_size(max_image_size)
+      ))
       true
     else
       false

diff --git a/lib/validators/upload_validator.rb b/lib/validators/upload_validator.rb
@@ -138,7 +138,10 @@ def maximum_file_size(upload, type)
     max_size_bytes = max_size_kb.kilobytes
 
     if upload.filesize > max_size_bytes
-      message = I18n.t("upload.#{type}s.too_large", max_size_kb: max_size_kb)
+      message = I18n.t(
+        "upload.#{type}s.too_large_humanized",
+        max_size: ActiveSupport::NumberHelper.number_to_human_size(max_size_bytes)
+      )
       upload.errors.add(:filesize, message)
     end
   end

diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb
@@ -1120,6 +1120,7 @@
     end
 
     it "replaces large image placeholder" do
+      SiteSetting.max_image_size_kb = 4096
       url = 'https://image.com/my-avatar'
       image_url = 'https://image.com/avatar.png'
 
@@ -1134,6 +1135,7 @@
       cpp.post_process
 
       expect(cpp.doc.to_s).to match(/<div class="large-image-placeholder">/)
+      expect(cpp.doc.to_s).to include(I18n.t("upload.placeholders.too_large_humanized", max_size: "4 MB"))
     end
   end
 

diff --git a/spec/components/validators/upload_validator_spec.rb b/spec/components/validators/upload_validator_spec.rb
@@ -31,6 +31,21 @@
       expect(UploadCreator.new(csv_file, "#{filename}.zip", for_export: true).create_for(user.id)).to be_valid
     end
 
+    describe "size validation" do
+      it "does not allow images that are too large" do
+        SiteSetting.max_image_size_kb = 1536
+        upload = Fabricate.build(:upload,
+          user: Fabricate(:admin),
+          original_filename: "test.png",
+          filesize: 2097152
+        )
+        subject.validate(upload)
+        expect(upload.errors.full_messages.first).to eq(
+          "Filesize #{I18n.t("upload.images.too_large_humanized", max_size: "1.5 MB")}"
+        )
+      end
+    end
+
     describe 'when allow_staff_to_upload_any_file_in_pm is true' do
       it 'should allow uploads for pm' do
         upload = Fabricate.build(:upload,

diff --git a/spec/lib/upload_creator_spec.rb b/spec/lib/upload_creator_spec.rb
@@ -114,6 +114,21 @@
       end
     end
 
+    context "when image is too big" do
+      let(:filename) { 'logo.png' }
+      let(:file) { file_from_fixtures(filename) }
+
+      it "adds an error to the upload" do
+        SiteSetting.max_image_size_kb = 1
+        upload = UploadCreator.new(
+          file, filename, force_optimize: true
+        ).create_for(Discourse.system_user.id)
+        expect(upload.errors.full_messages.first).to eq(
+          "#{I18n.t("upload.images.too_large_humanized", max_size: "1 KB")}"
+        )
+      end
+    end
+
     describe 'pngquant' do
       let(:filename) { "pngquant.png" }
       let(:file) { file_from_fixtures(filename) }

diff --git a/spec/requests/uploads_controller_spec.rb b/spec/requests/uploads_controller_spec.rb
@@ -122,7 +122,7 @@
         expect(response.status).to eq(422)
         expect(Jobs::CreateAvatarThumbnails.jobs.size).to eq(0)
         errors = response.parsed_body["errors"]
-        expect(errors.first).to eq(I18n.t("upload.attachments.too_large", max_size_kb: 1))
+        expect(errors.first).to eq(I18n.t("upload.attachments.too_large_humanized", max_size: "1 KB"))
       end
 
       it 'ensures allow_uploaded_avatars is enabled when uploading an avatar' do
@@ -817,7 +817,7 @@ def upload_file(file, folder = "images")
       end
 
       it "returns 422 when the file is an attachment and it's too big" do
-        SiteSetting.max_attachment_size_kb = 1000
+        SiteSetting.max_attachment_size_kb = 1024
         post "/uploads/create-multipart.json", {
           params: {
             file_name: "test.zip",
@@ -826,7 +826,7 @@ def upload_file(file, folder = "images")
           }
         }
         expect(response.status).to eq(422)
-        expect(response.body).to include(I18n.t("upload.attachments.too_large", max_size_kb: SiteSetting.max_attachment_size_kb))
+        expect(response.body).to include(I18n.t("upload.attachments.too_large_humanized", max_size: "1 MB"))
       end
 
       def stub_create_multipart_request

diff --git a/spec/services/external_upload_manager_spec.rb b/spec/services/external_upload_manager_spec.rb
@@ -89,7 +89,7 @@
           SiteSetting.max_image_size_kb = 1
           upload = subject.promote_to_upload!
           expect(upload.errors.full_messages).to include(
-            "Filesize " + I18n.t("upload.images.too_large", max_size_kb: SiteSetting.max_image_size_kb)
+            "Filesize " + I18n.t("upload.images.too_large_humanized", max_size: ActiveSupport::NumberHelper.number_to_human_size(SiteSetting.max_image_size_kb.kilobytes))
           )
         end