Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support human readable slugs for non-English locales #9892

Merged
merged 2 commits into from
Feb 6, 2025
Merged

Support human readable slugs for non-English locales #9892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac898d0
Support human readable slugs for non-English locales
ChrisBAshton Feb 5, 2025
0609ff5
Only check `safely_resluggable?` before generating new ID
ChrisBAshton Feb 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions app/models/html_attachment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class HtmlAttachment < Attachment
autosave: true,
inverse_of: :html_attachment

before_validation :clear_slug_if_non_english_locale
before_save :ensure_slug_is_valid

validates :govspeak_content, presence: true

Expand Down Expand Up @@ -61,9 +61,7 @@ def url(options = {})
end

def should_generate_new_friendly_id?
return false unless sluggable_locale?

slug.nil? || attachable.nil? || safely_resluggable?
safely_resluggable?
end

def deep_clone
Expand Down Expand Up @@ -121,17 +119,12 @@ 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 slug to be `nil` - but only if attachment isn't live already.
# A `nil` slug will fall back to using the content_id as the slug.
self.slug = nil if safely_resluggable?
end
end
102 changes: 63 additions & 39 deletions test/unit/app/models/html_attachment_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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: "المملكة المتحدة والمملكة العربية السعودية")
Expand All @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I did spot in the docs:

Previous versions of FriendlyId appended a numeric sequence to make slugs unique, but this was removed to simplify using FriendlyId in concurrent code.

So I'd expect this test to need an update when we update finder_id. I did try updating finder_id locally but got Bundler attempted to update friendly_id but its version stayed the same and it wasn't immediately clear why.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting 🤔

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
Expand Down