diff --git a/app/models/html_attachment.rb b/app/models/html_attachment.rb index 0ac27312562..0194b94080b 100644 --- a/app/models/html_attachment.rb +++ b/app/models/html_attachment.rb @@ -7,7 +7,8 @@ class HtmlAttachment < Attachment autosave: true, inverse_of: :html_attachment - before_validation :clear_slug_if_non_english_locale + before_create :ensure_slug_is_valid + before_save :ensure_slug_is_valid validates :govspeak_content, presence: true @@ -61,9 +62,7 @@ def url(options = {}) end def should_generate_new_friendly_id? - return false unless sluggable_locale? - - slug.nil? || attachable.nil? || safely_resluggable? + slug.nil? || slug.blank? || attachable.nil? || safely_resluggable? end def deep_clone @@ -121,17 +120,11 @@ def public_url(options = {}) root + public_path(options) end - def sluggable_locale? - locale.blank? || (locale == "en") - end + # Sense-check that the title converted to ASCII contains more than just hyphens! + def ensure_slug_is_valid + return unless slug.present? && slug.count("A-Za-z") < 4 - def sluggable_string - sluggable_locale? ? title : nil - end - - def clear_slug_if_non_english_locale - if locale_changed? && !sluggable_locale? - self.slug = nil - end + # Force FriendlyId to regenerate the slug, but only if attachment isn't live already + self.slug = nil if safely_resluggable? end end diff --git a/test/unit/app/models/html_attachment_test.rb b/test/unit/app/models/html_attachment_test.rb index 8b49a2c9227..26ffbb53e80 100644 --- a/test/unit/app/models/html_attachment_test.rb +++ b/test/unit/app/models/html_attachment_test.rb @@ -46,18 +46,6 @@ class HtmlAttachmentTest < ActiveSupport::TestCase assert_equal expected, actual end - test "#url returns absolute path to the draft stack when previewing for non-english locale" do - edition = create(:draft_publication, :with_html_attachment) - attachment = edition.attachments.first - attachment.update!(locale: "fi") - - expected = "https://draft-origin.test.gov.uk/government/publications/" - expected += "#{edition.slug}/#{attachment.content_id}?preview=#{attachment.id}" - actual = attachment.url(preview: true, full_url: true) - - assert_equal expected, actual - end - test "#url returns absolute path to the live site when not previewing" do edition = create(:published_publication, :with_html_attachment) attachment = edition.attachments.first @@ -69,31 +57,12 @@ class HtmlAttachmentTest < ActiveSupport::TestCase assert_equal expected, actual end - test "#url returns absolute path to the live site when not previewing for non-english locale" do - edition = create(:published_publication, :with_html_attachment) - attachment = edition.attachments.first - attachment.update!(locale: "fi") - - expected = "https://www.test.gov.uk/government/publications/" - expected += "#{edition.slug}/#{attachment.content_id}" - actual = attachment.url(full_url: true) - - assert_equal expected, actual - end - test "#url returns relative path by default" do edition = create(:published_publication, :with_html_attachment) attachment = edition.attachments.first assert_equal "/government/publications/#{edition.slug}/#{attachment.slug}", attachment.url end - test "#url returns relative path by default for non-english locale" do - edition = create(:published_publication, :with_html_attachment) - attachment = edition.attachments.first - attachment.update!(locale: "fi") - assert_equal "/government/publications/#{edition.slug}/#{attachment.content_id}", attachment.url - end - test "#url works when an attachment with english locale has special characters in title" do edition = create(:published_publication, :with_html_attachment) attachment = edition.attachments.first @@ -111,7 +80,7 @@ class HtmlAttachmentTest < ActiveSupport::TestCase statistics = create(:published_national_statistics) attachment = statistics.attachments.last attachment.update!(locale: "fi") - assert_equal "/government/statistics/#{statistics.slug}/#{attachment.content_id}", attachment.url + assert_equal "/government/statistics/#{statistics.slug}/#{attachment.slug}", attachment.url end test "#url works with consultation outcomes" do @@ -124,7 +93,7 @@ class HtmlAttachmentTest < ActiveSupport::TestCase consultation = create(:consultation_with_outcome_html_attachment) attachment = consultation.outcome.attachments.first attachment.update!(locale: "fi") - assert_equal "/government/consultations/#{consultation.slug}/outcome/#{attachment.content_id}", attachment.url + assert_equal "/government/consultations/#{consultation.slug}/outcome/#{attachment.slug}", attachment.url end test "#url works with consultation public feedback" do @@ -137,7 +106,7 @@ class HtmlAttachmentTest < ActiveSupport::TestCase consultation = create(:consultation_with_public_feedback_html_attachment) attachment = consultation.public_feedback.attachments.first attachment.update!(locale: "fi") - assert_equal "/government/consultations/#{consultation.slug}/public-feedback/#{attachment.content_id}", attachment.url + assert_equal "/government/consultations/#{consultation.slug}/public-feedback/#{attachment.slug}", attachment.url end test "#url works with call for evidence outcomes" do @@ -232,7 +201,7 @@ class HtmlAttachmentTest < ActiveSupport::TestCase assert_equal "an-html-attachment", second_draft_attachment.slug end - test "slug is not created for non-english attachments" do + test "slug is not created for non-english attachments if conversion to non-ASCII isn't possible" do # Additional attachment to ensure the duplicate detection behaviour isn't triggered create(:html_attachment, locale: "fr") attachment = create(:html_attachment, locale: "ar", title: "المملكة المتحدة والمملكة العربية السعودية") @@ -257,11 +226,66 @@ class HtmlAttachmentTest < ActiveSupport::TestCase assert_equal expected_slug, attachment.to_param end - test "slug is cleared when changing from english to non-english" do - attachment = create(:html_attachment, locale: "en") + test "slug is created for non-english attachments where conversion to ASCII is possible" do + attachment = create(:html_attachment, locale: "cs", title: "toto je náhodný název") - attachment.update!(locale: "fr") - assert attachment.slug.blank? + expected_slug = "toto-je-nahodny-nazev" + assert_equal expected_slug, attachment.slug + assert_equal expected_slug, attachment.to_param + end + + test "slug requires minimum 3 azAZ characters" do + attachment = create(:html_attachment, locale: "cs", title: "tot") + assert_equal nil, attachment.slug + end + + test "slug reverts to nil, if document has never been published, if title is changed such that minimum 3 azAZ characters is not reached" do + attachment = build(:html_attachment, title: "an-html-attachment") + + create(:draft_publication, attachments: [attachment]) + + attachment.title = "foo" + attachment.save! + attachment.reload + + assert_equal nil, attachment.slug + end + + test "even if slug is considered too short NOW, slug on legacy old attachment is not updated when the title is changed if document is published" do + edition = create( + :published_publication, + attachments: [ + build(:html_attachment, title: "an-html-attachment"), + ], + ) + # Bypass validations and callbacks to manually set slug + attachment = edition.attachments.first + attachment.update_column(:slug, "foo") + assert_equal "foo", attachment.slug + + #  now check slug not overridden + draft = edition.create_draft(create(:writer)) + attachment = draft.attachments.first + attachment.title = "a-new-title" + attachment.save! + attachment.reload + assert_equal "foo", attachment.slug + end + + # This behaviour is built into the friendly_id gem + test "slug is made unique if there is an existing clash" do + edition = create( + :published_publication, + attachments: [ + build(:html_attachment, locale: "en", title: "food"), + build(:html_attachment, locale: "cs", title: "food"), + ], + ) + attachment_1 = edition.attachments.first + attachment_2 = edition.attachments.last + + assert_equal "food", attachment_1.slug + assert_equal "food--2", attachment_2.slug end test "#identifier falls back to content_id if no slug available" do