Merge pull request #8448 from alphagov/refactor-lead-image-validator

Refactor body_does_not_contain_lead_image validator

app/helpers/govspeak_helper.rb

@@ -52,7 +52,7 @@ def prepare_images(images)
       .select { |image| image.image_data&.all_asset_variants_uploaded? }
       .map do |image|
       {
-        id: image.image_data.carrierwave_image,
+        id: image.filename,
         image_data_id: image.image_data_id,
         edition_id: image.edition_id,
         alt_text: image.alt_text,

app/models/edition/custom_lead_image.rb

@@ -2,7 +2,7 @@ module Edition::CustomLeadImage
   extend ActiveSupport::Concern
 
   included do
-    validate :body_does_not_contain_lead_image_markdown
+    validate :body_does_not_contain_lead_image
   end
 
   def update_lead_image
@@ -33,21 +33,14 @@ def remove_lead_image
     edition_lead_image.destroy!
   end
 
-  def body_does_not_contain_lead_image_markdown
-    return if lead_image.blank?
+  def body_does_not_contain_lead_image
+    return if lead_image.blank? || images.none?
 
-    if body_contains_lead_image_filename_markdown? || body_contains_lead_image_index_markdown?
+    html = Whitehall::GovspeakRenderer.new.govspeak_edition_to_html(self)
+    doc = Nokogiri::HTML::DocumentFragment.parse(html)
+
+    if doc.css("img").any? { |img| img[:src] == lead_image.url }
       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

test/unit/app/helpers/govspeak_helper_test.rb

@@ -268,15 +268,15 @@ def edition_with_attachment(body:)
   test "embeds images using !!number syntax" do
     edition = build(:published_news_article, body: "!!1")
     image_data = create(:image_data, id: 1)
-    edition.stubs(:images).returns([OpenStruct.new(alt_text: "My Alt", url: "https://some.cdn.com/image.jpg", image_data: ImageData.find(image_data.id))])
+    edition.stubs(:images).returns([OpenStruct.new(alt_text: "My Alt", url: "https://some.cdn.com/image.jpg", filename: "image.jpg", image_data: ImageData.find(image_data.id))])
     html = govspeak_edition_to_html(edition)
     assert_select_within_html html, ".govspeak figure.image.embedded img[src='https://some.cdn.com/image.jpg']"
   end
 
   test "embeds images using [Image:] syntax" do
-    edition = build(:published_news_article, body: "[Image: minister-of-funk.960x640.jpg]")
+    edition = build(:published_news_article, body: "[Image: image.jpg]")
     image_data = create(:image_data, id: 1)
-    edition.stubs(:images).returns([OpenStruct.new(alt_text: "My Alt", url: "https://some.cdn.com/image.jpg", image_data: ImageData.find(image_data.id))])
+    edition.stubs(:images).returns([OpenStruct.new(alt_text: "My Alt", url: "https://some.cdn.com/image.jpg", filename: "image.jpg", image_data: ImageData.find(image_data.id))])
     html = govspeak_edition_to_html(edition)
     assert_select_within_html html, ".govspeak figure.image.embedded img[src='https://some.cdn.com/image.jpg']"
   end

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

@@ -3,11 +3,18 @@
 class Edition::CustomLeadImageTest < ActiveSupport::TestCase
   include ActionDispatch::TestProcess
 
+  def stubbed_image(name)
+    build(:image).tap do |image|
+      image.stubs(:filename).returns(name)
+      image.stubs(:url).returns("http://example.com/#{name}")
+    end
+  end
+
   def edition_with_custom_lead_image(options = {})
-    image1 = build(:image)
-    image2 = build(:image)
+    non_lead_image = stubbed_image("non_lead_image.jpg")
+    lead_image = stubbed_image("lead_image.jpg")
 
-    build(:draft_news_article, options.merge(images: [image1, image2], lead_image: image2))
+    build(:draft_news_article, options.merge(images: [non_lead_image, lead_image], lead_image:))
   end
 
   def body_text_valid(body)
@@ -19,13 +26,13 @@ def body_text_valid(body)
     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_not body_text_valid("foo\n[Image: lead_image.jpg]\nbar")
+    assert_not body_text_valid("foo\n[Image:lead_image.jpg]\nbar")
+    assert_not body_text_valid("foo\n[Image:     lead_image.jpg]\nbar")
+    assert_not body_text_valid("foo\n[Image:  \n  lead_image.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")
+    assert body_text_valid("foo\[Image: non_lead_image.jpg]\nbar")
   end
 
   test "#update_lead_image updates the lead_image association to the oldest image" do

test/unit/lib/whitehall/govspeak_renderer_test.rb

@@ -20,8 +20,8 @@ class Whitehall::GovspeakRendererTest < ActiveSupport::TestCase
 
   test "interpolates images into rendered HTML when using filename as a markdown" do
     image_data = create(:image_data, 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]")
+    image = OpenStruct.new(alt_text: "My Alt", url: "http://example.com/image.jpg", filename: "image.jpg", image_data: ImageData.find(image_data.id))
+    edition = build(:edition, body: "Some content with an image.\n\n[Image: image.jpg]")
     edition.stubs(:images).returns([image])
 
     assert_equivalent_html govspeak_with_image_html(image),