Skip to content

[WHIT-2724] Fix minor inconsistencies in block_content #10966

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

Merged
jamiestamp merged 1 commit into from
Jan 13, 2026
Merged

[WHIT-2724] Fix minor inconsistencies in block_content #10966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions app/models/concerns/edition/translatable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ class Trait < Edition::Traits::Trait
def process_associations_before_save(edition)
@edition.translations.each do |translation|
I18n.with_locale(translation.locale) do
edition.title = @edition.title
edition.summary = @edition.summary
edition.body = @edition.body
edition.block_content = @edition.block_content
edition.title = translation.title
edition.summary = translation.summary
edition.body = translation.body
Copy link
Copy Markdown
Contributor

@ChrisBAshton ChrisBAshton Jan 8, 2026

Choose a reason for hiding this comment

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

Can you explain a bit more about what is happening here, vs what was happening before, and how it ever worked before? I'm a bit cautious just because the original code is more than 13 years old and translations for legacy content types have been working fine this whole time 🤔

I assume this will also explain why the new body= method is needed.

Copy link
Copy Markdown
Contributor Author

@jamiestamp jamiestamp Jan 8, 2026

Choose a reason for hiding this comment

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

Of course! So I'll give a bit of an overview on what is happening here.

The code as it currently exists on main is using @edition to get translated values. This will work, and it will get any values that have been set specifically for each language we are iterating through, but the problem with doing it this way is that we also pull in any fallback values.

If we instead directly get the values from the translation records, we avoid picking up the fallback values that the globalize gem includes on Edition objects for translatable attributes.

Take these translations as an example:

English (primary)

{
  "body": "English body",
  "summary": "English summary"
}

Spanish

{
  "body": "Spanish body",
  "summary": nil
}

When using this version of the code:

edition.title = @edition.title
edition.summary = @edition.summary
edition.body = @edition.body
edition.block_content = @edition.block_content

Where Spanish does not have a value for summary, it will pick up the fallback value and assign that directly on the translation. So we end up with this in the Spanish translation:

Spanish

{
  "body": "Spanish body",
  "summary": "English summary"
}

Whereas with the update where we call the translation record we are skipping the fallback values and the translation records match the original.

Copy link
Copy Markdown
Contributor Author

@jamiestamp jamiestamp Jan 8, 2026

Choose a reason for hiding this comment

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

The body= method isn't strictly necessary, but it is in the spirit of what we are looking to achieve in the ticket and avoid any confusion. As Standard Editions don't use body, this prevents the value from being set in the DB and causing confusion until we can fully drop the field.

Copy link
Copy Markdown
Contributor

@ChrisBAshton ChrisBAshton Jan 8, 2026

Choose a reason for hiding this comment

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

Have just re-reviewed this with Alex. Thanks for the explanation - makes sense, if we've understood correctly!

From our understanding:

  1. This doesn't change the behaviour when creating new translations - the 'add translation' form will still pre-populate the primary locale values for each field, for the publisher to update.
  2. It's only when creating new editions that the lack of the primary locale fallback comes into effect. So any attribute that was nil on the original translation will remain nil on subsequent translations and not inherit the English value for it.

In practice, I don't foresee a situation where title/summary/body would ever be nil - so we presume that really this logic is only intended for the block_content attribute, but for consistency reasons (there's no reason not to), you've also applied it to the edition-level attributes. Does that sound about right?

Thanks for the body= explanation - good bit of defensive code there 🎉

We ought to test this pretty thoroughly on integration before merge, but looking good!

Copy link
Copy Markdown
Contributor Author

@jamiestamp jamiestamp Jan 8, 2026

Choose a reason for hiding this comment

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

Yep, your understanding on that is spot on!

And definitely worth testing on integration. I'll pop do not merge on here until that's done

edition.write_attribute(:block_content, translation.block_content)
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions app/models/standard_edition.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ def body
block_content["body"]
end

def body=(_)
nil
end

def can_set_previously_published?
type_instance.settings["backdating_enabled"]
end
Expand Down
252 changes: 252 additions & 0 deletions test/unit/app/models/standard_edition_translation_block_content_consistency_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
require "test_helper"

class StandardEditionTranslationBlockContentConsistencyTest < ActiveSupport::TestCase
def english_body_content = "English body content"
def translated_body_content(locale) = "Body content for #{locale}"

setup do
ConfigurableDocumentType.setup_test_types(
build_configurable_document_type(
"test", {
"schema" => {
"properties" => {
"body" => { "type" => "string", "format" => "govspeak", "title" => "Body" },
"image" => { "type" => "integer", "format" => "lead_image_select", "title" => "Lead image" },
},
},
"settings" => { "translations_enabled" => true },
}
),
)
end

test "creating a new draft from a published StandardEdition with translations should not duplicate old body field data" do
translated_edition = create_standard_edition(translate_for: %w[cy])

new_draft = translated_edition.create_draft(create(:writer))

english_translation = new_draft.translation_for(:en)
welsh_translation = new_draft.translation_for(:cy)

assert_nil english_translation[:body],
"English translation body column should be nil, but got: #{english_translation[:body]}"
assert_nil welsh_translation[:body],
"Welsh translation body column should be nil, but got: #{welsh_translation[:body]}"

assert_equal english_body_content, english_translation.block_content["body"],
"English body should be in block_content.body"
assert_equal translated_body_content("cy"), welsh_translation.block_content["body"],
"Welsh body should be in block_content.body"
end

test "editions without images should not have image key in block_content" do
translated_edition = create_standard_edition(translate_for: %w[cy])

assert_not_includes translated_edition.translation_for(:en).block_content.keys, "image",
"English translation should not have image key"
assert_not_includes translated_edition.translation_for(:cy).block_content.keys, "image",
"Welsh translation should not have image key"

new_draft = translated_edition.create_draft(create(:writer))

assert_not_includes new_draft.translation_for(:en).block_content.keys, "image",
"Draft English translation should not have image key"
assert_not_includes new_draft.translation_for(:cy).block_content.keys, "image",
"Draft Welsh translation should not have image key"
end

test "primary translation with image should not clone image to non-primary translations" do
standard_edition = create_standard_edition(with_image: true)
create_translated_edition(standard_edition, locale: "cy", with_image: false)

english_translation = standard_edition.translation_for(:en)
welsh_translation = standard_edition.translation_for(:cy)
image_id = english_translation.block_content["image"]

assert_equal english_body_content, english_translation.block_content["body"],
"English body should have correct content"
assert_equal translated_body_content("cy"), welsh_translation.block_content["body"],
"Welsh body should have correct content"
assert_not_includes welsh_translation.block_content.keys, "image",
"Welsh translation should not have image key"

new_draft = standard_edition.create_draft(create(:writer))

assert_equal image_id, new_draft.translation_for(:en).block_content["image"],
"Draft English translation should have image value"
assert_equal english_body_content, new_draft.translation_for(:en).block_content["body"],
"Draft English body should have correct content"

assert new_draft.translation_for(:cy).block_content["image"].blank?,
"Draft Welsh translation should not have a value but found Image ID: #{new_draft.translation_for(:cy).block_content['image']}"
assert_equal translated_body_content("cy"), new_draft.translation_for(:cy).block_content["body"],
"Draft Welsh body should have correct content"
end

test "translated edition with image should not clone to primary translation" do
standard_edition = create_standard_edition(with_image: false)
create_translated_edition(standard_edition, locale: "cy", with_image: true)

english_translation = standard_edition.translation_for(:en)
welsh_translation = standard_edition.translation_for(:cy)
image_id = welsh_translation.block_content["image"]

new_draft = standard_edition.create_draft(create(:writer))

assert english_translation.block_content["image"].blank?,
"Draft English translation should not have image value"
assert_equal english_body_content, new_draft.translation_for(:en).block_content["body"],
"Draft English body should have correct content"

assert_equal image_id, new_draft.translation_for(:cy).block_content["image"],
"Draft Welsh translation should have image value"
assert_equal translated_body_content("cy"), new_draft.translation_for(:cy).block_content["body"],
"Draft Welsh body should have correct content"
end

test "new draft editions with images get the translated image IDs from the parent" do
standard_edition = create_standard_edition(with_image: true)
create_translated_edition(standard_edition, locale: "cy", with_image: true)

english_image_id = standard_edition.translation_for(:en).block_content["image"]
welsh_image_id = standard_edition.translation_for(:cy).block_content["image"]

new_draft = standard_edition.create_draft(create(:writer))

assert_equal english_image_id, new_draft.translation_for(:en).block_content["image"],
"Draft English translation should have image value"
assert_equal english_body_content, new_draft.translation_for(:en).block_content["body"],
"Draft English body should have correct content"

assert_equal welsh_image_id, new_draft.translation_for(:cy).block_content["image"],
"Draft Welsh translation should have image value"
assert_equal translated_body_content("cy"), new_draft.translation_for(:cy).block_content["body"],
"Draft Welsh body should have correct content"
end

test "new draft with multiple translations each having images retains correct data per locale" do
standard_edition = create_standard_edition(with_image: true)
create_translated_edition(standard_edition, locale: "cy", with_image: true)
create_translated_edition(standard_edition, locale: "fr", with_image: true)

english_image_id = standard_edition.translation_for(:en).block_content["image"]
welsh_image_id = standard_edition.translation_for(:cy).block_content["image"]
french_image_id = standard_edition.translation_for(:fr).block_content["image"]

# Ensure all images are distinct
assert_not_equal english_image_id, welsh_image_id, "English and Welsh should have different images"
assert_not_equal english_image_id, french_image_id, "English and French should have different images"
assert_not_equal welsh_image_id, french_image_id, "Welsh and French should have different images"

new_draft = standard_edition.create_draft(create(:writer))

# Verify each locale retains its own image
assert_equal english_image_id, new_draft.translation_for(:en).block_content["image"],
"Draft English translation should have its original image"
assert_equal welsh_image_id, new_draft.translation_for(:cy).block_content["image"],
"Draft Welsh translation should have its original image"
assert_equal french_image_id, new_draft.translation_for(:fr).block_content["image"],
"Draft French translation should have its original image"

# Verify body content is also correct per locale
assert_equal english_body_content, new_draft.translation_for(:en).block_content["body"],
"Draft English body should have correct content"
assert_equal translated_body_content("cy"), new_draft.translation_for(:cy).block_content["body"],
"Draft Welsh body should have correct content"
assert_equal translated_body_content("fr"), new_draft.translation_for(:fr).block_content["body"],
"Draft French body should have correct content"
end

test "partial updates to block_content merge correctly within primary translation" do
draft_standard_edition = create_standard_edition(with_image: false, factory_name: :draft_standard_edition)

I18n.with_locale(:en) do
draft_standard_edition.block_content = { "image" => 123 }
draft_standard_edition.save!
end

draft_standard_edition.reload
primary_translation = draft_standard_edition.translation_for(:en)

assert_equal english_body_content, primary_translation.block_content["body"],
"Body should be preserved after partial update"
assert_equal 123, primary_translation.block_content["image"],
"Image should be added via partial update"
end

test "partial updates to block_content merge correctly within secondary translation" do
draft_standard_edition = create_standard_edition(with_image: false, factory_name: :draft_standard_edition)
create_translated_edition(draft_standard_edition, locale: "cy", with_image: false)

I18n.with_locale(:cy) do
draft_standard_edition.block_content = { "image" => 123 }
draft_standard_edition.save!
end

draft_standard_edition.reload
secondary_translation = draft_standard_edition.translation_for(:cy)

assert_equal translated_body_content(:cy), secondary_translation.block_content["body"],
"Body should be preserved after partial update"
assert_equal 123, secondary_translation.block_content["image"],
"Image should be added via partial update"
end

test "partial updates to block_content in translated locales do not affect primary translation" do
draft_standard_edition = create_standard_edition(with_image: false, factory_name: :draft_standard_edition)
create_translated_edition(draft_standard_edition, locale: "cy", with_image: false)

I18n.with_locale(:cy) do
draft_standard_edition.block_content = { "body" => "this is the updated translated version" }
draft_standard_edition.save!
end

draft_standard_edition.reload

primary_translation = draft_standard_edition.translation_for(:en)
assert_equal english_body_content, primary_translation.block_content["body"],
"Body should be preserved after partial update"
end

test "partial updates to block_content in primary locale do not affect secondary translations" do
draft_standard_edition = create_standard_edition(with_image: false, factory_name: :draft_standard_edition)
create_translated_edition(draft_standard_edition, locale: "cy", with_image: false)

I18n.with_locale(:en) do
draft_standard_edition.block_content = { "body" => "this is the updated english version" }
draft_standard_edition.save!
end

draft_standard_edition.reload

secondary_translation = draft_standard_edition.translation_for(:cy)
assert_equal translated_body_content(:cy), secondary_translation.block_content["body"],
"Body should be preserved after partial update"
end

private

def create_standard_edition(with_image: false, translate_for: [], factory_name: :published_standard_edition)
block_content = { "body" => english_body_content }
block_content["image"] = create(:image).image_data.id if with_image

standard_edition = create(
factory_name,
configurable_document_type: "test",
title: "English title",
summary: "English summary",
block_content:,
)

translate_for.each { |locale| create_translated_edition(standard_edition, locale:) }

standard_edition
end

def create_translated_edition(standard_edition, locale:, with_image: false)
block_content = { "body" => translated_body_content(locale) }
block_content["image"] = create(:image).image_data.id if with_image

I18n.with_locale(locale) { standard_edition.translations.create!(locale:, block_content:) }
end
end