Merge pull request #8316 from alphagov/push-content-with-non-legacy-e…

…ndpoints-to-pub-api-image-data

Push content with non legacy endpoints to publishing api - image data

app/components/admin/edition_images/uploaded_images_component.html.erb

@@ -21,6 +21,7 @@
   <% if lead_image %>
     <div class="govuk-grid-column-one-third">
       <img src="<%= lead_image[:url] %>" alt="<%= lead_image[:preview_alt_text] %>" class="app-view-edition-resource__preview">
+      <% unless lead_image[:all_image_asset_variants_uploaded] %><span class="govuk-tag govuk-tag--green">Processing</span><% end %>
     </div>
     <div class="govuk-grid-column-two-thirds">
       <p class="govuk-body"><strong>Caption: </strong><%= lead_image[:caption] %></p>
@@ -66,6 +67,7 @@
       <li class="govuk-grid-row">
         <div class="govuk-grid-column-one-third">
           <img src="<%= image[:url] %>" alt="<%= image[:preview_alt_text] %>" class="app-view-edition-resource__preview">
+          <% unless image[:all_image_asset_variants_uploaded] %><span class="govuk-tag govuk-tag--green">Processing</span><% end %>
         </div>
         <div class="govuk-grid-column-two-thirds">
           <p class="govuk-body"><strong>Caption: </strong><%= image[:caption] %></p>

app/components/admin/edition_images/uploaded_images_component.rb

@@ -49,6 +49,10 @@ def lead_image_guidance
 
 private
 
+  def all_image_asset_variants_uploaded?(image)
+    image.image_data.all_asset_variants_uploaded? if image.image_data.present?
+  end
+
   def image_to_hash(image, index)
     {
       url: image.url,
@@ -57,6 +61,7 @@ def image_to_hash(image, index)
       alt_text: image.alt_text.presence || "None",
       markdown: unique_names? ? "[Image: #{image.filename}]" : "!!#{index}",
       links: links_for_image(image),
+      all_image_asset_variants_uploaded: all_image_asset_variants_uploaded?(image),
     }
   end
 

app/controllers/admin/edition_workflow_controller.rb

@@ -19,7 +19,7 @@ class Admin::EditionWorkflowController < Admin::BaseController
 
   rescue_from ActiveRecord::RecordInvalid do
     redirect_to admin_edition_path(@edition),
-                alert: "Unable to #{action_name_as_human_interaction(params[:action])} because it is invalid (#{@edition.errors.full_messages.to_sentence}). Please edit it and try again."
+                alert: "Unable to #{action_name_as_human_interaction(params[:action])} because #{@edition.errors.full_messages.to_sentence.downcase}. Please edit it and try again."
   end
 
   rescue_from Transitions::InvalidTransition do

app/helpers/govspeak_helper.rb

@@ -48,7 +48,9 @@ def govspeak_html_attachment_to_html(html_attachment)
   end
 
   def prepare_images(images)
-    images.map do |image|
+    images
+      .select { |image| image.image_data&.all_asset_variants_uploaded? }
+      .map do |image|
       {
         id: image.image_data.carrierwave_image,
         image_data_id: image.image_data_id,
@@ -64,7 +66,7 @@ def prepare_images(images)
 
   def prepare_attachments(attachments, alternative_format_contact_email)
     attachments
-      .select { |attachment| !attachment.file? || attachment.attachment_data.all_asset_variants_uploaded? }
+      .select { |attachment| !attachment.file? || attachment.attachment_data&.all_asset_variants_uploaded? }
       .map do |attachment|
       attachment_component_params(attachment, alternative_format_contact_email:)
     end

app/models/edition/lead_image.rb

@@ -28,6 +28,12 @@ def lead_image_caption
     end
   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
+
 private
 
   def placeholder_image_url

app/models/edition/publishing.rb

@@ -57,7 +57,7 @@ def asset_manager_check_required?
   end
 
   def attachment_uploaded_to_asset_manager!
-    errors.add(:attachments, "must have finished uploading.") unless uploaded_to_asset_manager?
+    errors.add(:attachments, "must have finished uploading") unless uploaded_to_asset_manager?
   end
 
   def build_unpublishing(attributes = {})

app/models/image_data.rb

@@ -36,6 +36,17 @@ def bitmap?
     content_type !~ /svg/
   end
 
+  def all_asset_variants_uploaded?
+    if use_non_legacy_endpoints
+      asset_variants = assets.map(&:variant).map(&:to_sym).to_set
+      all_variants = ImageUploader.versions.keys.push(:original).to_set
+
+      return asset_variants == all_variants
+    end
+
+    true
+  end
+
 private
 
   Dimensions = Struct.new(:width, :height)

app/presenters/publishing_api/case_study_presenter.rb

@@ -54,7 +54,7 @@ def details
         first_public_at:,
         format_display_type: item.display_type_key,
       }
-      details_hash[:image] = if image_available? && image_required?
+      details_hash[:image] = if image_available? && image_required? && item.lead_image_has_all_assets?
                                image_details
                              else
                                { url: "", caption: nil, alt_text: "" }

app/presenters/publishing_api/news_article_presenter.rb

@@ -122,7 +122,7 @@ def initialize(news_article)
       end
 
       def call
-        return {} unless news_article.has_lead_image?
+        return {} unless news_article.has_lead_image? && news_article.lead_image_has_all_assets?
 
         {
           image: {

app/views/admin/attachments/_attachments.html.erb

@@ -7,7 +7,7 @@
     <li class="govuk-grid-row">
       <div class="govuk-grid-column-full">
         <p class="govuk-body">
-          <strong>Title:</strong> <%= attachment.title %> <% unless attachment.attachment_data.blank? || attachment.attachment_data.all_asset_variants_uploaded? %><span class="govuk-tag govuk-tag--green">Uploading</span><% end %>
+          <strong>Title:</strong> <%= attachment.title %> <% unless attachment.attachment_data.blank? || attachment.attachment_data.all_asset_variants_uploaded? %><span class="govuk-tag govuk-tag--green">Processing</span><% end %>
         </p>
         <p class="govuk-body">
           <strong>Type: </strong><%= attachment.is_a?(HtmlAttachment) ? attachment.readable_type : attachment.readable_type.capitalize %>

app/views/admin/editions/_standard_fields.html.erb

@@ -53,6 +53,7 @@
     <%= render "components/govspeak-editor", {
       label: {
         text: "Body" + "#{' (required)' if form.object.body_required?}",
+        hint_text: "If your attachment is still being processed, it will not be visible in preview.",
         heading_size: "l",
       },
       name: "edition[body]",

features/step_definitions/attachment_steps.rb

@@ -70,7 +70,7 @@
 end
 
 Then(/^the attachments should be in the following order:$/) do |attachment_list|
-  attachment_names = all("li p:first").map(&:text).map { |t| t.delete_prefix("Title:").chomp("Uploading").strip }
+  attachment_names = all("li p:first").map(&:text).map { |t| t.delete_prefix("Title:").chomp("Processing").strip }
 
   attachment_list.hashes.each_with_index do |attachment_info, index|
     attachment = Attachment.find_by(title: attachment_info[:title])

features/step_definitions/consultation_steps.rb

@@ -94,7 +94,7 @@
 end
 
 Then(/^the responses attachments should be in the following order:$/) do |attachment_list|
-  attachment_names = all("li p:first").map(&:text).map { |t| t.delete_prefix("Title:").chomp("Uploading").strip }
+  attachment_names = all("li p:first").map(&:text).map { |t| t.delete_prefix("Title:").chomp("Processing").strip }
 
   attachment_list.hashes.each_with_index do |attachment_info, index|
     attachment = Attachment.find_by(title: attachment_info[:title])

test/components/admin/edition_images/uploaded_images_component_test.rb

@@ -44,4 +44,20 @@ class Admin::EditionImages::UploadedImagesComponentTest < ViewComponent::TestCas
     assert_selector "input[value='!!1']"
     assert_selector "input[value='!!2']"
   end
+
+  test "shows \"Processing\" label where image assets (variants) are still uploading" do
+    lead_image_data_with_no_assets = build(:image_data, use_non_legacy_endpoints: true)
+    regular_image_data_with_no_assets = build(:image_data, use_non_legacy_endpoints: true)
+    regular_image_data_with_assets = build(:image_data_with_assets)
+    images = [
+      build_stubbed(:image, image_data: lead_image_data_with_no_assets),
+      build_stubbed(:image, image_data: regular_image_data_with_no_assets),
+      build_stubbed(:image, image_data: regular_image_data_with_assets),
+    ]
+    edition = build_stubbed(:draft_publication, images:)
+
+    render_inline(Admin::EditionImages::UploadedImagesComponent.new(edition:))
+
+    assert_text "Processing", count: 2
+  end
 end

test/factories/asset.rb

@@ -1,4 +1,7 @@
 FactoryBot.define do
   factory :asset do
+    variant { Asset.variants[:original] }
+    asset_manager_id { "asset_manager_id" }
+    filename { "filename" }
   end
 end

test/factories/images.rb

@@ -4,7 +4,7 @@
     image_data
   end
 
-  factory :image_with_asset, parent: :image do
+  factory :image_with_assets, parent: :image do
     after(:build) do |image|
       image.image_data = build(:image_data_with_assets)
     end

test/functional/admin/attachments_controller_test.rb

@@ -111,7 +111,7 @@ def self.supported_attachable_types
     assert_response :success
     assert_select "p.govuk-body", text: "Title: An HTML attachment"
     assert_select "p.govuk-body", text: "Title: An uploaded file attachment"
-    assert_select "p.govuk-body", text: "Title: An uploading file attachment Uploading"
+    assert_select "p.govuk-body", text: "Title: An uploading file attachment Processing"
     assert_select "p.govuk-body", text: "Title: An external attachment"
   end
 

test/functional/admin/edition_workflow_controller_test.rb

@@ -169,7 +169,19 @@ class Admin::EditionWorkflowControllerTest < ActionController::TestCase
     post :submit, params: { id: draft_edition, lock_version: draft_edition.lock_version }
 
     assert_redirected_to admin_publication_path(draft_edition)
-    assert_equal "Unable to submit this edition because it is invalid (Summary can't be blank). Please edit it and try again.", flash[:alert]
+    assert_equal "Unable to submit this edition because summary can't be blank. Please edit it and try again.", flash[:alert]
+  end
+
+  test "submission error should read as a sentence when there are multiple validation errors" do
+    draft_edition.summary = nil
+    draft_edition.title = nil
+    attachment = build(:file_attachment_with_no_assets)
+    draft_edition.attachments << attachment
+    draft_edition.save!(validate: false)
+    post :submit, params: { id: draft_edition, lock_version: draft_edition.lock_version }
+
+    assert_redirected_to admin_publication_path(draft_edition)
+    assert_equal "Unable to submit this edition because title can't be blank, summary can't be blank, and alternative format provider can't be blank. Please edit it and try again.", flash[:alert]
   end
 
   test "submit responds with 422 if missing a lock version" do

test/integration/image_deletion_integration_test.rb

@@ -47,7 +47,7 @@ class ImageDeletionIntegrationTest < ActionDispatch::IntegrationTest
 
       context "images have assets" do
         let(:managing_editor) { create(:managing_editor) }
-        let(:image) { build(:image_with_asset) }
+        let(:image) { build(:image_with_assets) }
         let(:first_asset_id) { image.image_data.assets.first.asset_manager_id }
         let(:edition) { create(:news_article) }
 

test/unit/app/helpers/govspeak_helper_test.rb

@@ -185,6 +185,19 @@ def edition_with_attachment(body:)
     refute_select_within_html html, ".gem-c-attachment"
   end
 
+  test "should ignore images with missing asset variants" do
+    embed_code = "[Image: minister-of-funk.960x640.jpg]"
+    body = "#Heading\n\n#{embed_code}\n\n##Subheading"
+    image = build(:image)
+    image.image_data.use_non_legacy_endpoints = true
+    image.image_data.assets = [build(:asset), build(:asset, variant: Asset.variants[:s960])]
+    image.image_data.save!
+    document = build(:published_news_article, images: [image], body:)
+
+    html = govspeak_edition_to_html(document)
+    refute_select_within_html html, ".image.embedded"
+  end
+
   test "should not convert documents with no block attachments" do
     text = "#Heading\n\n!@2"
     document = build(:published_detailed_guide, body: text)

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

@@ -40,4 +40,31 @@ class Edition::LeadImageTest < ActiveSupport::TestCase
     assert_equal "url", model.lead_image_url
     assert_equal "alt_text", model.lead_image_alt_text
   end
+
+  test "#lead_image_has_all_assets? returns false if the lead image (ImageData) has missing assets" do
+    image_with_missing_assets = build(:image)
+    image_with_missing_assets.image_data.use_non_legacy_endpoints = true
+    image_with_missing_assets.image_data.assets << [build(:asset)]
+
+    model = stub("Target", { images: [image_with_missing_assets] }).extend(Edition::LeadImage)
+
+    assert_not model.lead_image_has_all_assets?
+  end
+
+  test "#lead_image_has_all_assets? returns true if the lead image (ImageData) has all assets" do
+    image_with_missing_assets = build(:image_with_assets)
+
+    model = stub("Target", { images: [image_with_missing_assets] }).extend(Edition::LeadImage)
+
+    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
+    # e.g. DefaultNewsOrganisationImageData doesn't yet implement all_asset_variants_uploaded?
+    image = build(:default_news_organisation_image_data)
+    organisation = build(:organisation, default_news_image: image)
+    model = stub("Target", { images: [], lead_organisations: [], organisations: [organisation] }).extend(Edition::LeadImage)
+
+    assert model.lead_image_has_all_assets?
+  end
 end

test/unit/app/models/image_data_test.rb

@@ -79,6 +79,30 @@ class ImageDataTest < ActiveSupport::TestCase
     assert_equal 7, image_data.assets.count
   end
 
+  test "all_asset_variants_uploaded? returns true if use_non_legacy_endpoints is false" do
+    image_data = build(:image_data)
+    assert image_data.all_asset_variants_uploaded?
+  end
+
+  test "use_non_legacy_endpoints: true - all_asset_variants_uploaded? returns true if all assets present" do
+    image_data = build(:image_data_with_assets)
+
+    assert image_data.all_asset_variants_uploaded?
+  end
+
+  test "use_non_legacy_endpoints: true - all_asset_variants_uploaded? returns false if some assets are missing" do
+    image_data = build(:image_data, use_non_legacy_endpoints: true)
+    image_data.assets = [build(:asset), build(:asset, variant: Asset.variants[:s960])]
+
+    assert_not image_data.all_asset_variants_uploaded?
+  end
+
+  test "use_non_legacy_endpoints: true - all_asset_variants_uploaded? returns false if there are no assets" do
+    image_data = build(:image_data, use_non_legacy_endpoints: true)
+
+    assert_not image_data.all_asset_variants_uploaded?
+  end
+
   def build_example(file_name)
     file = File.open(Rails.root.join("test/fixtures/images", file_name))
     build(:image_data, file:)

test/unit/app/presenters/publishing_api/news_article_presenter_test.rb

@@ -237,6 +237,7 @@ class NewsArticleWithImage < TestCase
         high_resolution_lead_image_url: "/foo-large",
         lead_image_alt_text: "Bar",
         lead_image_caption: "Baz",
+        images: [build(:image)],
       )
     end
 

test/unit/lib/whitehall/asset_manager_storage_test.rb

@@ -187,7 +187,7 @@ def assets_protected?
     end
 
     test "should call deleteAssetWorker with asset manager id" do
-      model = create(:image_with_asset)
+      model = create(:image_with_assets)
 
       AssetManagerDeleteAssetWorker.expects(:perform_async).times(7).with(nil, regexp_matches(/asset_manager_id./))
 

test/unit/lib/whitehall/govspeak_renderer_test.rb

@@ -9,7 +9,7 @@ class Whitehall::GovspeakRendererTest < ActiveSupport::TestCase
   end
 
   test "interpolates images into rendered HTML when using !!number as a markdown" do
-    image_data = create(:image_data, id: 1)
+    image_data = create(:image_data_with_assets, id: 1)
     image = OpenStruct.new(alt_text: "My Alt", url: "http://example.com/image.jpg", image_data: ImageData.find(image_data.id))
     edition = build(:edition, body: "Some content with an image.\n\n!!1")
     edition.stubs(:images).returns([image])
@@ -19,7 +19,7 @@ class Whitehall::GovspeakRendererTest < ActiveSupport::TestCase
   end
 
   test "interpolates images into rendered HTML when using filename as a markdown" do
-    image_data = create(:image_data, id: 1)
+    image_data = create(:image_data_with_assets, id: 1)
     image = OpenStruct.new(alt_text: "My Alt", url: "http://example.com/image.jpg", image_data: ImageData.find(image_data.id))
     edition = build(:edition, body: "Some content with an image.\n\n[Image: minister-of-funk.960x640.jpg]")
     edition.stubs(:images).returns([image])