From 502b427fc959e07f68af2438627701396b411f01 Mon Sep 17 00:00:00 2001 From: ChrisBAshton Date: Wed, 4 Dec 2024 13:52:37 +0000 Subject: [PATCH] Simplify `all_asset_variants_uploaded?` method Now that the logic defining `version :thumbnail, if: :pdf?` in `AttachmentUploader` has been removed, there is no attachment type that has anything other than an `:original` version. We therefore know that `required_variants` is always just `:original` and can simplify the method. I did consider renaming the method to simply `uploaded?` or `original_uploaded?`, but a method called `all_asset_variants_uploaded?` is defined in 8 different places, and called in almost a hundred places, so it's difficult to know which variation of the method is being called in which place (and therefore which method calls to rename). Safer to just leave it. With the method simplified, we can remove the test (since it would no longer work). --- app/models/attachment_data.rb | 3 +-- test/unit/app/models/attachment_data_test.rb | 10 ---------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/app/models/attachment_data.rb b/app/models/attachment_data.rb index d6af8bcfd53..18939956708 100644 --- a/app/models/attachment_data.rb +++ b/app/models/attachment_data.rb @@ -61,9 +61,8 @@ def indexable? def all_asset_variants_uploaded? asset_variants = assets.map(&:variant).map(&:to_sym) - required_variants = pdf? ? AttachmentUploader.versions.keys.push(:original) : [Asset.variants[:original].to_sym] - (required_variants - asset_variants).empty? + (%i[original] - asset_variants).empty? end def update_file_attributes diff --git a/test/unit/app/models/attachment_data_test.rb b/test/unit/app/models/attachment_data_test.rb index 921397b74eb..5f81e8b2a53 100644 --- a/test/unit/app/models/attachment_data_test.rb +++ b/test/unit/app/models/attachment_data_test.rb @@ -437,14 +437,4 @@ class AttachmentDataTest < ActiveSupport::TestCase assert_not attachment_data.all_asset_variants_uploaded? end - - # TODO: commented out to get the tests passing. - # We should consider rewriting the test using a temporary model that has an asset variant. - # - # test "#all_asset_variants_uploaded? returns false if some asset variants are missing" do - # attachment_data = build(:attachment_data_with_no_assets, content_type: AttachmentUploader::PDF_CONTENT_TYPE) - # attachment_data.assets << build(:asset) - - # assert_not attachment_data.all_asset_variants_uploaded? - # end end