Merge pull request #8570 from alphagov/feature-to-use-asset-ids

Feature to use asset IDs

app/controllers/admin/features_controller.rb

@@ -45,7 +45,11 @@ def build_feature
 
   def feature_params
     params.fetch(:feature, {}).permit(
-      :image, :image_cache, :alt_text, :document_id, :topical_event_id, :offsite_link_id
+      :alt_text,
+      :document_id,
+      :topical_event_id,
+      :offsite_link_id,
+      image_attributes: %i[file],
     )
   end
 

app/models/edition/lead_image.rb

@@ -34,8 +34,6 @@ def lead_image_caption
   end
 
   def lead_image_has_all_assets?
-    return true unless image_data.respond_to?(:all_asset_variants_uploaded?)
-
     image_data.all_asset_variants_uploaded?
   end
 

app/models/feature.rb

@@ -4,14 +4,13 @@ class Feature < ApplicationRecord
   belongs_to :offsite_link
   belongs_to :feature_list
 
-  has_one :image_new, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable
+  has_one :image, class_name: "FeaturedImageData", as: :featured_imageable, inverse_of: :featured_imageable
+  accepts_nested_attributes_for :image, reject_if: :all_blank
 
-  mount_uploader :image, FeaturedImageUploader, mount_on: :carrierwave_image
   validates :document, presence: true, unless: ->(feature) { feature.topical_event_id.present? || feature.offsite_link_id.present? }
   validates :started_at, presence: true
-  validates :image, presence: true, on: :create
+  validate :image_is_present
   validates :alt_text, length: { maximum: 255 }
-  validates_with ImageValidator, method: :image, size: [960, 640], if: :image_changed?
 
   before_validation :set_started_at!, on: :create
 
@@ -50,13 +49,20 @@ def locale
     feature_list ? feature_list.locale : :en
   end
 
+  def republish_to_publishing_api_async
+    republish_featurable_to_publishing_api
+    if document&.editions&.select { |edition| edition.is_a?(Speech) }.present?
+      Whitehall::PublishingApi.republish_document_async(document)
+    end
+  end
+
 private
 
   def set_started_at!
     self.started_at = Time.zone.now
   end
 
-  def image_changed?
-    changes["carrierwave_image"].present?
+  def image_is_present
+    errors.add(:"image.file", "can't be blank") if image.blank?
   end
 end

app/models/organisation.rb

@@ -242,6 +242,7 @@ def republish_dependent_documents
                   .includes(:images)
                   .where(images: { id: nil })
                   .map(&:document)
+                  .uniq(&:id)
     documents.each { |d| Whitehall::PublishingApi.republish_document_async(d) }
   end
 

app/models/person.rb

@@ -122,7 +122,7 @@ def publishing_api_presenter
   end
 
   def republish_dependent_documents
-    speeches.map { |speech| Whitehall::PublishingApi.republish_document_async(speech.document) }
+    speeches.uniq { |speech| speech.document.id }.map { |speech| Whitehall::PublishingApi.republish_document_async(speech.document) }
 
     historical_account&.republish_to_publishing_api_async
   end

app/models/worldwide_organisation.rb

@@ -144,6 +144,7 @@ def republish_dependent_documents
                   .includes(:images)
                   .where(images: { id: nil })
                   .map(&:document)
+                  .uniq(&:id)
     documents.each { |d| Whitehall::PublishingApi.republish_document_async(d) }
   end
 end

app/presenters/publishing_api/featured_documents_presenter.rb

@@ -1,15 +1,18 @@
 module PublishingApi
   module FeaturedDocumentsPresenter
     def featured_documents(featurable_item, document_limit)
-      featurable_item.feature_list_for_locale(I18n.locale).current.limit(document_limit).map do |feature|
-        if feature.document
-          featured_documents_editioned(feature)
-        elsif feature.topical_event
-          featured_documents_topical_event(feature)
-        elsif feature.offsite_link
-          featured_documents_offsite_link(feature)
+      featurable_item
+        .feature_list_for_locale(I18n.locale).current.limit(document_limit)
+        .select { |feature| feature.image.all_asset_variants_uploaded? }
+        .map do |feature|
+          if feature.document
+            featured_documents_editioned(feature)
+          elsif feature.topical_event
+            featured_documents_topical_event(feature)
+          elsif feature.offsite_link
+            featured_documents_offsite_link(feature)
+          end
         end
-      end
     end
 
   private
@@ -20,10 +23,7 @@ def featured_documents_editioned(feature)
       {
         title: edition.title,
         href: edition.public_path(locale: feature.feature_list.locale),
-        image: {
-          url: feature.image.url,
-          alt_text: feature.alt_text.presence || "",
-        },
+        image: get_image(feature),
         summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(edition.summary),
         public_updated_at: edition.public_timestamp,
         document_type: edition.display_type,
@@ -36,10 +36,7 @@ def featured_documents_topical_event(feature)
       {
         title: topical_event.name,
         href: topical_event.public_path(locale: feature.feature_list.locale),
-        image: {
-          url: feature.image.url,
-          alt_text: feature.alt_text,
-        },
+        image: get_image(feature),
         summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(topical_event.summary),
         public_updated_at: topical_event.start_date,
         document_type: nil, # We don't want a type for topical events
@@ -52,14 +49,20 @@ def featured_documents_offsite_link(feature)
       {
         title: offsite_link.title,
         href: offsite_link.url,
-        image: {
-          url: feature.image.url,
-          alt_text: feature.alt_text,
-        },
+        image: get_image(feature),
         summary: Whitehall::GovspeakRenderer.new.govspeak_to_html(offsite_link.summary),
         public_updated_at: offsite_link.date,
         document_type: offsite_link.display_type,
       }
     end
+
+    def get_image(feature)
+      {
+        url: feature.image.url,
+        medium_resolution_url: feature.image.url(:s465),
+        high_resolution_url: feature.image.url(:s712),
+        alt_text: feature.alt_text.presence || "",
+      }
+    end
   end
 end

app/presenters/publishing_api/speech_presenter.rb

@@ -139,8 +139,6 @@ def has_image?
     end
 
     def image_has_all_assets?
-      return true unless image.instance_of?(FeaturedImageData)
-
       image.all_asset_variants_uploaded?
     end
 

app/views/admin/features/new.html.erb

@@ -19,9 +19,9 @@
           heading_size: "l",
         },
         hint: "Select a 960px wide and 640px tall image to be shown when featuring",
-        name: "feature[image]",
-        id: "feature_image",
-        error_items: errors_for(@feature.errors, :image),
+        name: "feature[image_attributes][file]",
+        id: "feature_image_file",
+        error_items: errors_for(@feature.errors, :"image.file"),
       } %>
 
       <%= render "govuk_publishing_components/components/input", {

test/factories/featured_image_data.rb

@@ -4,13 +4,13 @@
     featured_imageable { build(:topical_event) }
 
     after(:build) do |featured_image_data|
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_original", variant: Asset.variants[:original], filename: "minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s960", variant: Asset.variants[:s960], filename: "s960_minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s712", variant: Asset.variants[:s712], filename: "s712_minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s630", variant: Asset.variants[:s630], filename: "s630_minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s465", variant: Asset.variants[:s465], filename: "s465_minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s300", variant: Asset.variants[:s300], filename: "s300_minister-of-funk.960x640.jpg")
-      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s216", variant: Asset.variants[:s216], filename: "s216_minister-of-funk.960x640.jpg")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_original", variant: Asset.variants[:original], filename: featured_image_data.filename)
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s960", variant: Asset.variants[:s960], filename: "s960_#{featured_image_data.filename}")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s712", variant: Asset.variants[:s712], filename: "s712_#{featured_image_data.filename}")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s630", variant: Asset.variants[:s630], filename: "s630_#{featured_image_data.filename}")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s465", variant: Asset.variants[:s465], filename: "s465_#{featured_image_data.filename}")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s300", variant: Asset.variants[:s300], filename: "s300_#{featured_image_data.filename}")
+      featured_image_data.assets << build(:asset, asset_manager_id: "asset_manager_id_s216", variant: Asset.variants[:s216], filename: "s216_#{featured_image_data.filename}")
     end
   end
 end

test/factories/features.rb

@@ -1,7 +1,7 @@
 FactoryBot.define do
   factory :feature do
     document
-    image { image_fixture_file }
+    image { build(:featured_image_data) }
 
     trait :with_topical_event_association do
       topical_event

test/functional/admin/features_controller_test.rb

@@ -58,7 +58,9 @@ class Admin::FeaturesControllerTest < ActionController::TestCase
 
     params = {
       document_id: edition.document_id,
-      image: upload_fixture("images/960x640_gif.gif"),
+      image_attributes: {
+        file: upload_fixture("images/960x640_gif.gif"),
+      },
       alt_text: "some text",
     }
 
@@ -68,6 +70,7 @@ class Admin::FeaturesControllerTest < ActionController::TestCase
     post :create, params: { feature_list_id: feature_list.id, feature: params }
 
     assert_equal edition.document_id, Feature.last.document_id
+    assert_equal "960x640_gif.gif", Feature.last.image.filename
   end
 
   test "post :feature creates a feature and republishes the document & world location news when the featurable is an world location news" do
@@ -78,7 +81,9 @@ class Admin::FeaturesControllerTest < ActionController::TestCase
 
     params = {
       document_id: edition.document_id,
-      image: upload_fixture("images/960x640_gif.gif"),
+      image_attributes: {
+        file: upload_fixture("images/960x640_gif.gif"),
+      },
       alt_text: "some text",
     }
 
@@ -88,5 +93,6 @@ class Admin::FeaturesControllerTest < ActionController::TestCase
     post :create, params: { feature_list_id: feature_list.id, feature: params }
 
     assert_equal edition.document_id, Feature.last.document_id
+    assert_equal "960x640_gif.gif", Feature.last.image.filename
   end
 end

test/functional/admin/organisations_controller_test.rb

@@ -356,7 +356,9 @@ def example_organisation_attributes
     feature_list = organisation.load_or_create_feature_list("en")
     feature_list.features.create!(
       topical_event:,
-      image: image_fixture_file,
+      image_attributes: {
+        file: image_fixture_file,
+      },
       alt_text: "Image alternative text",
     )
 

test/unit/app/models/edition/lead_image_test.rb

@@ -57,12 +57,4 @@ class Edition::LeadImageTest < ActiveSupport::TestCase
 
     assert model.lead_image_has_all_assets?
   end
-
-  test "#lead_image_has_all_assets? returns true if the lead image data doesn't implement all_asset_variants_uploaded?" do
-    image = build(:featured_image_data)
-    organisation = build(:organisation, default_news_image: image)
-    model = stub("Target", { lead_image: nil, lead_organisations: [], organisations: [organisation] }).extend(Edition::LeadImage)
-
-    assert model.lead_image_has_all_assets?
-  end
 end

test/unit/app/models/feature_test.rb

@@ -9,18 +9,9 @@ class FeatureTest < ActiveSupport::TestCase
     assert_not build(:feature, image: nil).valid?
   end
 
-  test "valid without image on update" do
-    # This is to work around the fact that carrierwave will consider the image invalid
-    # because the file will have been moved to the 'clean' folder by the virus scanner
-    # and therefore not in the location that it expects
-    feature = create(:feature)
-    feature.image = nil
-    assert feature.valid?
-  end
-
   test "started_at set by default on creation" do
     feature = Feature.create!(
-      image: image_fixture_file,
+      image: build(:featured_image_data),
       feature_list: create(:feature_list),
       document: create(:document),
       alt_text: "foo",
@@ -42,28 +33,4 @@ class FeatureTest < ActiveSupport::TestCase
     assert feature.end!
     assert_equal Time.zone.now, feature.reload.ended_at
   end
-
-  test "rejects SVG logo uploads" do
-    svg_image = File.open(Rails.root.join("test/fixtures/images/test-svg.svg"))
-    feature = build(:feature, image: svg_image)
-
-    assert_not feature.valid?
-    assert_includes feature.errors.map(&:full_message), "Image You are not allowed to upload \"svg\" files, allowed types: jpg, jpeg, gif, png"
-  end
-
-  test "rejects non-image file uploads" do
-    non_image_file = File.open(Rails.root.join("test/fixtures/folders.zip"))
-    feature = build(:feature, image: non_image_file)
-
-    assert_not feature.valid?
-    assert_includes feature.errors.map(&:full_message), "Image You are not allowed to upload \"zip\" files, allowed types: jpg, jpeg, gif, png"
-  end
-
-  test "accepts valid image uploads" do
-    jpg_image = File.open(Rails.root.join("test/fixtures/big-cheese.960x640.jpg"))
-    feature = build(:feature, image: jpg_image)
-
-    assert feature
-    assert_empty feature.errors
-  end
 end