Merge pull request #8433 from alphagov/use-lead-image-association-thr…

…oughout-codebase

Use lead image association throughout codebase

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

@@ -18,18 +18,18 @@
 <% end %>
 
 <div class="govuk-grid-row">
-  <% if lead_image %>
+  <% if lead_image_hash %>
     <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 %>
+      <img src="<%= lead_image_hash[:url] %>" alt="<%= lead_image_hash[:preview_alt_text] %>" class="app-view-edition-resource__preview">
+      <% unless lead_image_hash[: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>
-      <p class="govuk-body"><strong>Alt text: </strong><%= lead_image[:alt_text] %></p>
+      <p class="govuk-body"><strong>Caption: </strong><%= lead_image_hash[:caption] %></p>
+      <p class="govuk-body"><strong>Alt text: </strong><%= lead_image_hash[:alt_text] %></p>
     </div>
   <% end %>
 
-  <% if lead_image || @edition.type == "CaseStudy" %>
+  <% if lead_image_hash || @edition.type == "CaseStudy" %>
     <%= form_with(url: update_image_display_option_admin_edition_path(@edition), method: :patch) do |form| %>
       <div class="app-view-edition-resource__actions govuk-grid-column-full govuk-button-group">
         <% if @edition.type == "CaseStudy" %>
@@ -41,8 +41,8 @@
           } %>
         <% end %>
 
-        <% if lead_image %>
-          <% lead_image[:links].each do |link| %><%= link %><% end %>
+        <% if lead_image_hash %>
+          <% lead_image_hash[:links].each do |link| %><%= link %><% end %>
         <% end %>
       </div>
     <% end %>

app/components/admin/edition_images/uploaded_images_component.rb

@@ -3,36 +3,37 @@
 class Admin::EditionImages::UploadedImagesComponent < ViewComponent::Base
   def initialize(edition:)
     @edition = edition
-    @lead_image = has_lead_image? ? @edition.images.first : nil
-    @document_images = @edition.images.drop(has_lead_image? ? 1 : 0)
-  end
-
-  def has_lead_image?
-    can_have_lead_image? && @edition.images.any?
   end
 
   def can_have_lead_image?
     @edition.can_have_custom_lead_image?
   end
 
   def lead_image
-    image_to_hash @lead_image, 0 if has_lead_image?
+    @lead_image ||= can_have_lead_image? ? @edition.lead_image : nil
+  end
+
+  def lead_image_hash
+    image_to_hash(lead_image, 0) if lead_image
   end
 
   def document_images
-    @document_images.map.with_index(1) { |image, index| image_to_hash image, index }
+    @document_images ||= (@edition.images - [lead_image].compact)
+                          .map.with_index(1) { |image, index| image_to_hash(image, index) }
   end
 
   def new_image_display_option
-    if @edition.image_display_option == "no_image"
-      return has_lead_image? ? "custom_image" : "organisation_image"
+    if @edition.image_display_option == "no_image" && @edition.images.present?
+      "custom_image"
+    elsif @edition.image_display_option == "no_image"
+      "organisation_image"
+    else
+      "no_image"
     end
-
-    "no_image"
   end
 
   def update_image_display_option_button_text
-    if has_lead_image?
+    if @edition.images.present?
       return "#{new_image_display_option == 'no_image' ? 'Hide' : 'Show'} lead image"
     end
 
@@ -56,7 +57,7 @@ def all_image_asset_variants_uploaded?(image)
   def image_to_hash(image, index)
     {
       url: image.url,
-      preview_alt_text: index.zero? ? "Lead image" : "Image #{index}",
+      preview_alt_text: lead_image == image ? "Lead image" : "Image #{index}",
       caption: image.caption.presence || "None",
       alt_text: image.alt_text.presence || "None",
       markdown: unique_names? ? "[Image: #{image.filename}]" : "!!#{index}",

app/models/edition/custom_lead_image.rb

@@ -2,16 +2,7 @@ module Edition::CustomLeadImage
   extend ActiveSupport::Concern
 
   included do
-    validates :body,
-              format: {
-                without: /^!!1([^\d]|$)/,
-                message: "cannot have a reference to the first image in the text",
-                multiline: true,
-              }
-  end
-
-  def image_disallowed_in_body_text?(index)
-    index == 1
+    validate :body_does_not_contain_lead_image_markdown
   end
 
   def update_lead_image
@@ -37,4 +28,22 @@ def remove_lead_image
 
     edition_lead_image.destroy!
   end
+
+  def body_does_not_contain_lead_image_markdown
+    return if lead_image.blank?
+
+    if body_contains_lead_image_filename_markdown? || body_contains_lead_image_index_markdown?
+      errors.add(:body, "cannot have a reference to the lead image in the text")
+    end
+  end
+
+  def body_contains_lead_image_filename_markdown?
+    body.match?(/\[Image:\s*#{Regexp.escape(lead_image.filename)}\s*\]/)
+  end
+
+  def body_contains_lead_image_index_markdown?
+    lead_image_index = images.find_index(lead_image)
+
+    body.match?(/^!!#{lead_image_index + 1}([^\d]|$)/)
+  end
 end

app/models/edition/lead_image.rb

@@ -19,16 +19,16 @@ def high_resolution_lead_image_url
   end
 
   def lead_image_alt_text
-    if images.first.try(:alt_text)
-      images.first.alt_text.squish
+    if lead_image.try(:alt_text)
+      lead_image.alt_text.squish
     else
       ""
     end
   end
 
   def lead_image_caption
-    if images.first
-      caption = images.first.caption && images.first.caption.strip
+    if lead_image
+      caption = lead_image.caption && lead_image.caption.strip
       caption.presence
     end
   end
@@ -58,8 +58,8 @@ def image_url
   end
 
   def image_data
-    if images.first
-      images.first.image_data
+    if lead_image
+      lead_image.image_data
     elsif lead_organisations.any? && lead_organisations.first.default_news_image
       lead_organisations.first.default_news_image
     elsif organisations.any? && organisations.first.default_news_image

app/presenters/publishing_api/case_study_presenter.rb

@@ -81,7 +81,7 @@ def image_details
     end
 
     def image_available?
-      item.images.any? || emphasised_organisation_default_image_available?
+      item.lead_image.present? || emphasised_organisation_default_image_available?
     end
 
     def image_required?

app/views/admin/editions/show/_main.html.erb

@@ -190,7 +190,7 @@
           ],
           rows: @edition.images.map do | image |
             [
-              { text: sanitize("#{image.filename} #{tag.span("Lead image", class: 'govuk-tag app-view-summary__tag') if @edition.can_have_custom_lead_image? && image == @edition.images.first}") },
+              { text: sanitize("#{image.filename} #{tag.span("Lead image", class: 'govuk-tag app-view-summary__tag') if @edition.can_have_custom_lead_image? && image == @edition.lead_image}") },
             ]
           end,
         } %>

test/components/admin/edition_images/uploaded_images_component_test.rb

@@ -3,14 +3,37 @@
 require "test_helper"
 
 class Admin::EditionImages::UploadedImagesComponentTest < ViewComponent::TestCase
-  test "lead image rendered for case study" do
+  test "lead image rendered for case study with default hidden field that toggles organisation image off" do
     images = [build_stubbed(:image), build_stubbed(:image)]
-    edition = build_stubbed(:draft_case_study, images:)
+    edition = build_stubbed(:draft_case_study, images:, lead_image: images.first)
     render_inline(Admin::EditionImages::UploadedImagesComponent.new(edition:))
 
     assert_selector "img", count: 2
     assert_selector "img[alt='Lead image']"
     assert_selector "img[alt='Image 1']"
+    assert_selector "input[type='hidden'][name='edition[image_display_option]'][value='no_image']", visible: :hidden
+  end
+
+  test "has the correct hidden field for toggling 'image_display_option' when it's 'no_image' and no images have been to the case study" do
+    edition = build_stubbed(:draft_case_study, image_display_option: "no_image")
+    render_inline(Admin::EditionImages::UploadedImagesComponent.new(edition:))
+
+    assert_selector "input[type='hidden'][name='edition[image_display_option]'][value='organisation_image']", visible: :hidden
+  end
+
+  test "has the correct hidden field for toggling 'image_display_option' when it's 'organisation_image' and no images have been to the case study" do
+    edition = build_stubbed(:draft_case_study, image_display_option: "organisation_image")
+    render_inline(Admin::EditionImages::UploadedImagesComponent.new(edition:))
+
+    assert_selector "input[type='hidden'][name='edition[image_display_option]'][value='no_image']", visible: :hidden
+  end
+
+  test "has the correct hidden field for toggling 'image_display_option' when it's 'no_image' and an image has been to the case study" do
+    image = build_stubbed(:image)
+    edition = build_stubbed(:draft_case_study, image_display_option: "no_image", images: [image])
+    render_inline(Admin::EditionImages::UploadedImagesComponent.new(edition:))
+
+    assert_selector "input[type='hidden'][name='edition[image_display_option]'][value='custom_image']", visible: :hidden
   end
 
   test "lead image not rendered for publication" do

test/functional/admin/case_studies_controller_test.rb

@@ -20,7 +20,8 @@ class Admin::CaseStudiesControllerTest < ActionController::TestCase
 
   test "PATCH :update_image_display_option updates the image_display option and handles updating an editions lead image" do
     edition = create(:draft_case_study, image_display_option: "custom_image")
-    create(:edition_lead_image, edition:)
+    image = create(:image, edition:)
+    create(:edition_lead_image, edition:, image:)
 
     patch :update_image_display_option, params: { id: edition.id, edition: { image_display_option: "no_image" } }
 

test/functional/admin/edition_images_controller_test.rb

@@ -45,7 +45,7 @@ class Admin::EditionImagesControllerTest < ActionDispatch::IntegrationTest
     file = upload_fixture("images/960x640_jpeg.jpg")
     post admin_edition_images_path(edition), params: { image: { image_data: { file: } } }
 
-    assert_equal "960x640_jpeg.jpg", edition.lead_image.filename
+    assert_equal "960x640_jpeg.jpg", edition.reload.lead_image.filename
   end
 
   test "#create shows the cropping page if image is too large" do

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

@@ -4,59 +4,44 @@ class Edition::CustomLeadImageTest < ActiveSupport::TestCase
   include ActionDispatch::TestProcess
 
   def edition_with_custom_lead_image(options = {})
-    build(:draft_news_article, options)
-  end
-
-  test "reports that the first image is not available for adding inline" do
-    assert edition_with_custom_lead_image.image_disallowed_in_body_text?(1)
-  end
+    image1 = build(:image)
+    image2 = build(:image)
 
-  test "reports other images are not disallowed" do
-    assert_not edition_with_custom_lead_image.image_disallowed_in_body_text?(2)
+    build(:draft_news_article, options.merge(images: [image1, image2], lead_image: image2))
   end
 
   def body_text_valid(body)
     edition_with_custom_lead_image(body:).valid?
   end
 
-  test "validates that the first image is not included in the body text" do
-    assert_not body_text_valid("foo\n!!1\nbar")
-    assert_not body_text_valid("foo\n!!1 \nbar")
-    assert_not body_text_valid("foo\n!!1")
-    assert_not body_text_valid("foo\n!!1s\nbar")
-    assert body_text_valid("foo\n!!10\nbar")
-    assert body_text_valid("foo\nfoo bar !!1\nbar")
+  test "validates that the lead image is not included in the body text" do
+    assert_not body_text_valid("foo\n!!2\nbar")
+    assert_not body_text_valid("foo\n!!2 \nbar")
+    assert_not body_text_valid("foo\n!!2")
+    assert_not body_text_valid("foo\n!!2s\nbar")
+    assert_not body_text_valid("foo\[Image: minister-of-funk.960x640.jpg]\nbar")
+    assert_not body_text_valid("foo\[Image:minister-of-funk.960x640.jpg]\nbar")
+    assert_not body_text_valid("foo\[Image:     minister-of-funk.960x640.jpg]\nbar")
+    assert_not body_text_valid("foo\[Image:  \n  minister-of-funk.960x640.jpg]\nbar")
+    assert body_text_valid("foo\n!!20\nbar")
+    assert body_text_valid("foo\nfoo bar !!2\nbar")
+    assert body_text_valid("foo\[Image: anotherfilename.jpg]\nbar")
   end
 
   test "#update_lead_image updates the lead_image association to the oldest image" do
-    image1 = build_stubbed(:image)
-    image2 = build_stubbed(:image)
-    edition = build_stubbed(:news_article, images: [image1, image2])
-
-    edition.stubs(:lead_image).returns(nil)
-    edition.images.stubs(:order).with(:created_at, :id).returns([image1, image2])
-    edition_lead_image = mock
-
-    edition
-    .expects(:build_edition_lead_image)
-    .with(image: image1)
-    .once
-    .returns(edition_lead_image)
-
-    edition_lead_image
-    .expects(:save!)
-    .once
-    .returns(true)
+    image1 = build(:image)
+    image2 = build(:image)
+    edition = create(:news_article, images: [image1, image2])
 
     edition.update_lead_image
+
+    assert_equal image1, edition.reload.lead_image
   end
 
   test "#update_lead_image deletes the associated edition_lead_image if image_display_option is 'no_image'" do
     edition_lead_image = build(:edition_lead_image)
     edition = build(:news_article, image_display_option: "no_image", edition_lead_image:)
 
-    edition.stubs(:images).returns([edition_lead_image.image])
-
     edition_lead_image
     .expects(:destroy!)
     .once
@@ -71,7 +56,6 @@ def body_text_valid(body)
   test "#update_lead_image deletes the associated edition_lead_image if image_display_option is 'organisation_image'" do
     edition_lead_image = build(:edition_lead_image)
     edition = build(:news_article, image_display_option: "organisation_image", edition_lead_image:)
-    edition.stubs(:images).returns([edition_lead_image.image])
 
     edition_lead_image
     .expects(:destroy!)
@@ -85,9 +69,8 @@ def body_text_valid(body)
   end
 
   test "#update_lead_image returns nil if lead_image is present" do
-    edition = build(:news_article)
     image = build(:image)
-    edition.stubs(:lead_image).returns(image)
+    edition = build(:news_article, images: [image], lead_image: image)
 
     edition
     .expects(:build_edition_lead_image)