Merge pull request #8479 from alphagov/revert-8473-upgrade-carrierwave

Revert "Upgrade carrierwave from 2.x to 3.x"
alphagov · Nov 9, 2023 · 8141b57 · 8141b57
2 parents 9d8b806 + cd861a5
commit 8141b57
Show file tree

Hide file tree

Showing 10 changed files with 27 additions and 26 deletions.
diff --git a/Gemfile b/Gemfile
@@ -7,7 +7,7 @@ gem "addressable"
 gem "babosa"
 gem "bootsnap", require: false
 gem "bootstrap-kaminari-views"
-gem "carrierwave"
+gem "carrierwave", "< 3" # pin at v2 to avoid breaking changes
 gem "carrierwave-i18n"
 gem "chronic"
 gem "dalli"

diff --git a/Gemfile.lock b/Gemfile.lock
@@ -112,12 +112,13 @@ GEM
       rack-test (>= 0.6.3)
       regexp_parser (>= 1.5, < 3.0)
       xpath (~> 3.2)
-    carrierwave (3.0.4)
-      activemodel (>= 6.0.0)
-      activesupport (>= 6.0.0)
+    carrierwave (2.2.4)
+      activemodel (>= 5.0.0)
+      activesupport (>= 5.0.0)
       addressable (~> 2.6)
       image_processing (~> 1.1)
       marcel (~> 1.0.0)
+      mini_mime (>= 0.1.3)
       ssrf_filter (~> 1.0)
     carrierwave-i18n (0.3.0)
     chronic (0.10.2)
@@ -831,7 +832,7 @@ GEM
       rexml
     ruby-progressbar (1.13.0)
     ruby-rc4 (0.1.5)
-    ruby-vips (2.2.0)
+    ruby-vips (2.1.4)
       ffi (~> 1.12)
     ruby2_keywords (0.0.5)
     rubyntlm (0.6.3)
@@ -886,7 +887,7 @@ GEM
       actionpack (>= 5.2)
       activesupport (>= 5.2)
       sprockets (>= 3.0.0)
-    ssrf_filter (1.1.2)
+    ssrf_filter (1.1.1)
     statsd-ruby (1.5.0)
     sync (0.5.0)
     sys-uname (1.2.3)
@@ -953,7 +954,7 @@ DEPENDENCIES
   binding_of_caller
   bootsnap
   bootstrap-kaminari-views
-  carrierwave
+  carrierwave (< 3)
   carrierwave-i18n
   chronic
   climate_control

diff --git a/app/controllers/admin/edition_images_controller.rb b/app/controllers/admin/edition_images_controller.rb
@@ -28,7 +28,6 @@ def create
     @new_image.build_image_data(image_params["image_data"])
 
     @new_image.image_data.validate_on_image = @new_image
-    @new_image.image_data.images << @new_image
 
     if @new_image.save
       @edition.update_lead_image if @edition.can_have_custom_lead_image?

diff --git a/app/models/image_data.rb b/app/models/image_data.rb
@@ -25,7 +25,11 @@ def filename
   end
 
   def auth_bypass_ids
-    images.map { |image| image.edition.auth_bypass_id }.uniq
+    images
+      .joins(:edition)
+      .where("editions.state in (?)", Edition::PRE_PUBLICATION_STATES)
+      .map { |e| e.edition.auth_bypass_id }
+      .uniq
   end
 
   def bitmap?

diff --git a/features/step_definitions/image_steps.rb b/features/step_definitions/image_steps.rb
@@ -9,7 +9,7 @@
 end
 
 Given("an organisation with a default news image exists") do
-  default_news_image = create(:featured_image_data)
+  default_news_image = build(:featured_image_data)
   @organisation = create(:organisation, default_news_image:)
 end
 

diff --git a/features/topical_event_featurings.feature b/features/topical_event_featurings.feature
@@ -1,5 +1,4 @@
 @design-system-only
-@disable-sidekiq-test-mode
 Feature:
   As an Editor.
   I want to be able to create and manage topical_event_featurings.

diff --git a/test/functional/admin/attachments_controller_test.rb b/test/functional/admin/attachments_controller_test.rb
@@ -170,10 +170,10 @@ def self.supported_attachable_types
 
   test "POST :create triggers a job to be queued to store the attachment in Asset Manager" do
     attachment = valid_file_attachment_params
+    variant = Asset.variants[:original]
     model_type = AttachmentData.to_s
 
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:original], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id]).once
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:thumbnail], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => variant, "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
 
     post :create, params: { edition_id: @edition.id, type: "file", attachment: }
   end
@@ -347,10 +347,10 @@ def self.supported_attachable_types
 
   test "PUT :update with a file triggers a job to be queued to store the attachment in Asset Manager" do
     attachment = create(:file_attachment, attachable: @edition)
+    variant = Asset.variants[:original]
     model_type = attachment.attachment_data.class.to_s
 
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:original], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
-    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => Asset.variants[:thumbnail], "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
+    AssetManagerCreateAssetWorker.expects(:perform_async).with(anything, has_entries("assetable_id" => kind_of(Integer), "asset_variant" => variant, "assetable_type" => model_type), anything, @edition.class.to_s, @edition.id, [@edition.auth_bypass_id])
 
     put :update,
         params: {

diff --git a/test/unit/app/models/edition/images_test.rb b/test/unit/app/models/edition/images_test.rb
@@ -131,7 +131,7 @@ def can_have_custom_lead_image?; end
     edition_lead_image = EditionLeadImage.find_by!(edition_id: draft_edition.id)
 
     assert_not_equal image2.id, edition_lead_image.image_id
-    assert_equal image2.reload.image_data.images.last.id, edition_lead_image.image_id
+    assert_equal image2.image_data.images.last.id, edition_lead_image.image_id
   end
 
   test "captions for images can be changed between versions" do

diff --git a/test/unit/app/presenters/publishing_api/call_for_evidence_presenter_test.rb b/test/unit/app/presenters/publishing_api/call_for_evidence_presenter_test.rb
@@ -255,7 +255,7 @@ class OpenCallForEvidenceWithParticipationTest < TestCase
         call_for_evidence_response_form_data_attributes: response_form_data_attributes,
       )
 
-      @participation = build(
+      participation = create(
         :call_for_evidence_participation,
         call_for_evidence_response_form_attributes: response_form_attributes,
         email: "postmaster@example.com",
@@ -271,7 +271,7 @@ class OpenCallForEvidenceWithParticipationTest < TestCase
 
       self.call_for_evidence = create(
         :open_call_for_evidence,
-        call_for_evidence_participation: @participation,
+        call_for_evidence_participation: participation,
       )
     end
 
@@ -281,10 +281,9 @@ class OpenCallForEvidenceWithParticipationTest < TestCase
 
     test "ways to respond" do
       Plek.any_instance.stubs(:asset_root).returns("https://asset-host.com")
-      expected_id = @participation.call_for_evidence_response_form.call_for_evidence_response_form_data.id
-      filename = @participation.call_for_evidence_response_form.call_for_evidence_response_form_data.carrierwave_file
+      expected_id = CallForEvidenceResponseFormData.where(carrierwave_file: "two-pages.pdf").last.id
       expected_ways_to_respond = {
-        attachment_url: "https://asset-host.com/government/uploads/system/uploads/call_for_evidence_response_form_data/file/#{expected_id}/#{filename}",
+        attachment_url: "https://asset-host.com/government/uploads/system/uploads/call_for_evidence_response_form_data/file/#{expected_id}/two-pages.pdf",
         email: "postmaster@example.com",
         link_url: "http://www.example.com",
         postal_address: <<-ADDRESS.strip_heredoc.chop,

diff --git a/test/unit/app/presenters/publishing_api/consultation_presenter_test.rb b/test/unit/app/presenters/publishing_api/consultation_presenter_test.rb
@@ -256,7 +256,7 @@ class OpenConsultationWithParticipationTest < TestCase
         consultation_response_form_data_attributes: response_form_data_attributes,
       )
 
-      @participation = create(
+      participation = create(
         :consultation_participation,
         consultation_response_form_attributes: response_form_attributes,
         email: "postmaster@example.com",
@@ -272,7 +272,7 @@ class OpenConsultationWithParticipationTest < TestCase
 
       self.consultation = create(
         :open_consultation,
-        consultation_participation: @participation,
+        consultation_participation: participation,
       )
     end
 
@@ -282,10 +282,9 @@ class OpenConsultationWithParticipationTest < TestCase
 
     test "ways to respond" do
       Plek.any_instance.stubs(:asset_root).returns("https://asset-host.com")
-      expected_id = @participation.consultation_response_form.consultation_response_form_data.id
-      expected_filename = @participation.consultation_response_form.consultation_response_form_data.carrierwave_file
+      expected_id = ConsultationResponseFormData.where(carrierwave_file: "two-pages.pdf").last.id
       expected_ways_to_respond = {
-        attachment_url: "https://asset-host.com/government/uploads/system/uploads/consultation_response_form_data/file/#{expected_id}/#{expected_filename}",
+        attachment_url: "https://asset-host.com/government/uploads/system/uploads/consultation_response_form_data/file/#{expected_id}/two-pages.pdf",
         email: "postmaster@example.com",
         link_url: "http://www.example.com",
         postal_address: <<-ADDRESS.strip_heredoc.chop,