Open TheoChevalier opened 2 years ago
Karl:
@register_snippet
class Signup(CTA):
...
translatable_fields = CTA.translatable_fields
class Meta(TranslatableMixin.Meta):
verbose_name = 'signup snippet'
Inheriting TranslatableMixin.Meta
is not required here because CTA is a concrete model (it's only required when inheriting from an abstract model like TranslatableMixin
itself)
The locale field gets added on to the first concrete model that inherits it, any models that inherit from CTA will use the locale field on CTA rather than create its own one
Pomax:
Ah. Let me take that off the meta and see if things work. And thank you for having a look
So I'm getting this, when running makemigrations
:
You are trying to add a non-nullable field 'locale' to cta without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
1) Provide a one-off default now (will be set on all existing rows with a null value for this column)
2) Quit, and let me add a default in models.py
I assume this is just the locale_id for English, which would be 1? or is this a much more complex value.
hm, no that would probably go super wrong, I guess this needs a migration function that copies the subclass locale and translation_key over into the now superclass?
Pomax:
So I have https://github.com/mozilla/foundation.mozilla.org/pull/7777 up, but that migration is pretty "not correct" - what would be the right way to make sure locales/translation keys don't break? E.g. if we have a localised Petition(TranslatableMixin, CTA)
, and now the translation data gets "moved" to CTA(TranslatableMixin)
, is that a matter of for cta in CTA.objects.all()
and then copying cta.locale = Petition.objects.get(pk=cta.pk).locale
?
(running that after the CTA model is updated, but before the subclass fields get removed?)
Pomax:
Right, so I have something that works up on https://github.com/mozilla/foundation.mozilla.org/pull/7777 but it definitely needs a review pass by "not just us" because I don't know the internals of wagtail-localize and I don't know if this is sufficient or whether it's going to blow up in our face once we deploy it...
Given Theo's testing of that PR, the conclusion so far is "it blows up".
@tbrlpld please can you reach out to one of the core team members for support on this one please?
@Pomax @TheoChevalier If I understand this issue here correctly, then CTA
was not translatable in the past but exists in the app? Is that correct?
We also want the sub-classes of CTA
(e.g. Signup
, Petition
, ...) to be translatable as well. They also exist already but were not translatable in the past. Correct?
Based on Karl's comments it should be enough to make CTA
translatable, because the sub-classes receive the locale through the multi-table inheritance. Because of that and the this Slack thread I wonder if the necessary steps to make a snippet with existing data translatable have been followed?
Oh. I guess from #7777 follows that the sub-classes have been translatable before. Did that work? Was that used? So the only functionality we want to add is making the plain CTA
objects translatable too?
Has @zerolab's comment on #7777 been considered?
@Pomax @TheoChevalier If I understand this issue here correctly, then
CTA
was not translatable in the past but exists in the app? Is that correct?We also want the sub-classes of
CTA
(e.g.Signup
,Petition
, ...) to be translatable as well. They also exist already but were not translatable in the past. Correct?
No, these were translatable, they just weren’t subclasses. Here’s an example of a campaign page in French with a translated petition snippet https://foundation.mozilla.org/cms/pages/4548/edit/
Things broke when we added Callpower snippets circa October/November when this issue was filed. There was no CTA
class before this.
Based on Karl's comments it should be enough to make
CTA
translatable, because the sub-classes receive the locale through the multi-table inheritance. Because of that and the this Slack thread I wonder if the necessary steps to make a snippet with existing data translatable have been followed?
I could be wrong but I think that’s what @Pomax tried in his first attempt to fix the issue?
Has @zerolab's https://github.com/mozilla/foundation.mozilla.org/pull/7777#issuecomment-1023355145 been considered?
Implementing the solution described in this comment should be the remaining work to fix the issue
@TheoChevalier Just to make sure that we are on the same page regarding the desired outcome here.
CTA
will be it's own snippet type. The admin will not show any of the other current "sub-classes" (e.g. Petition
in the "CTA" admin. The other snippets can be administered through their own admins. CTA
snippets are translatableCampaignPage
and BanneredCampaignPage
the cta
field snippet chooser will only present choices that are of the basic CTA
type. E.g. a instance of Petition
can not be selected anymore.Does that sound like what you would expect?
The separation of the inheritance hierarchy (as proposed by @zerolab) seems to be much more involved than I anticipated at first: https://medium.com/skilljar-engineering/moving-from-inheritance-to-composition-in-django-a268563089d7
@TheoChevalier Just to make sure that we are on the same page regarding the desired outcome here.
* Split the share CTA fields into a new mixin * This means `CTA` will be it's own snippet type. The admin will not show any of the other current "sub-classes" (e.g. `Petition` in the "CTA" admin. The other snippets can be administered through their own admins. * The `CTA` snippets are translatable * On the `CampaignPage` and `BanneredCampaignPage` the `cta` field snippet chooser will only present choices that are of the basic `CTA` type. E.g. a instance of `Petition` can not be selected anymore.
Does that sound like what you would expect?
I’m not sure I’m fully following, it’s quite confusing at the moment. Are you proposing to merge all fields from CallPower
, Signup
, and Petition
snippets into a single snippet?
Snippets would still be created by sub-classes, but when picking them, CTA becomes some sort of a generic view and you can pick any snippet created in any of the sub-classes?
Yea, this is all a bit confusing. I am not sure what the desired end state is, what has been tried and why certain approach were taken in the past.
Snippets would still be created by sub-classes, but when picking them, CTA becomes some sort of a generic view and you can pick any snippet created in any of the sub-classes?
What you are describing is the current state actually.
The sub-classes CallPower
, Signup
and Petition
are all derived from CTA
. They all share the fields on CTA
but each sub-class adds different additional fields. When an instance of one of the sub-classes is created, a corresponding CTA
is created that holds the shared fields. But, an instance of CTA
can also be created in it's own right, without creating a sub-class instance.
The snippet chooser on the pages points to CTA
. Because there is a CTA
instance for every instance of a sub-class you can find snippets from all sub-classes or direct CTA
instances. This can also be an issue, because in the UI it is not clear what the actual type of the snippet is.
Also, this current setup also creates the issue with the translation. The field on the page points at CTA
. CTA
itself is not translatable, only the sub-classes are. This creates the issue in Wagtail. We are trying to translate a not translatable model, the CTA
instance. But what is translatable are only the instance of the sub-classes.
So that's the current situation.
Now, the proposal by @zerolab was to remove the inheritance of CTA
by CallPower
, Signup
and Petition
. Instead of being actual sub-classes, we would just create an abstract mixin that defines all the shared fields. There would be no more actual inheritance. We could still create new CTA
class but as a separate class, not as a parent to the other ones. Each of these models would be completely separate. These classes would not be sub-classes anymore - so let's call them concrete classes.
This would require that the pages be changed. Instead of choosing the parent/abstract class, the field would have to point to one of the "sub-classes"/concrete classes. So we would either have to pick only one specific class that the page can link to, or we need to create multiple fields (one for each class) and then have validation logic that makes sure only one of them is set. (This could also be in a through model, but that is an implementation detail an does not really matter at the moment).
The main issue I see here is that it is apparently a really complex undertaking to break an existing inheritance chain in Django. That is what the article in my comment above describes.
Plus, we need to keep the translations.
Plus, we need to keep the connections to the pages. Or rather replace the connections to the CTA
with the connection to the correct "sub-class"/concrete class.
I guess this could be a bit easier if it was ok to break the connection from the pages to the CTA
and require them to be manually recreated, if that is acceptable? The translations of the individual snippets should remain. Only the connection from the page to the snippet would need to be re-created.
Also, would the implementation with the separate fields and choosers for the different concrete classes be acceptable (with validation logic preventing multiple being set)?
The complexity of breaking the inheritance chain would remain I am afraid.
@Pomax I am also interested in you take here.
@Pomax please could you book in a call to talk this through with Tibor? thanks!
We want to explore putting a through-model in between the "CTA picker" and the actual CTA type used, so that we get something like:
CampaignPage
CTAthroughModel (single object, allowing staff to pick any of the following:)
signup/petition/callpower/(etc?) each their own model
Through model and specific CTA classes would use the TranslatableMixin, and this way all fields in each CTA should be properly localizable, with Wagtail seeing the true objects, rather than only the "CTA super class".
(at least, this is our assumption at present and we'll want to test this)
In terms of steps we want to take:
CampaignPage
to "one of" Signup, Petition, and Callpower, with a save validation that enforces "only one of these"..cta
field of ForeignKey CTA
to a corresponding through model instance.CampaignPage
to use a chooser that lets folks pick one or more CTAs using the above through model.This will possibly also required:
cta()
definition in the mini_site_tags.py
file, used in the CTA templates.Per the history of the ticket, accepting and merging this PR (https://github.com/MozillaFoundation/foundation.mozilla.org/pull/7777) may close the issue. The PR was merged on April 20, 2022.
Hey @simont-cr, the PR you referenced was closed but not merged. This issue hasn't been solved yet
Theo has opened #12133 which is a duplicate of this one
CTA
instances that were created in other languages. I guess the most straightforward path is to simply disregard those as "translated content" and let editors handle how to port them to actual translations
Translating/Syncing translations for Banner pages crashes.
Example with this page: https://foundation.mozilla.org/cms/localize/update/122/
Wagtail is complaining about
wagtailpages.CTA
not being translatable, but after @Pomax ’s investigation, it seems if we do make it localizable, wagtail-localize is not happy about class inheritance.Note for @TheoChevalier to remember to sync changes from these pages once the issue is fixed:
Pages to translate:
┆Issue is synchronized with this Jira Story