[WHIT-2724] Fix minor inconsistencies in block_content

[jamiestamp]

This update makes values in new draft translations consistent with the original.

Changes:

• Directly assign the original block_content value to new translations instead of merging and using fallback values
• Disable assignment to the body attribute on new standard editions
• New tests to ensure translations on new drafts match the originals

[ChrisBAshton]

The code LGTM - good job 👍

But I do wonder quite how it worked before - have left a comment and hope you can fill in the gaps in my knowledge! 🙏

Also I think we can squash this all into one commit.

[ChrisBAshton]

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.

[jamiestamp]

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.

[jamiestamp]

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.

[ChrisBAshton]

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!

[jamiestamp]

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

[TonyGDS]

Tested on integration, LGTM