deschler / django-modeltranslation

Translates Django models using a registration approach.
BSD 3-Clause "New" or "Revised" License
1.36k stars 259 forks source link

modeltranslation 0.19.3+ with Django CMS 4.1.x - CMS Plugin functionalities broken #748

Open aacimov opened 1 month ago

aacimov commented 1 month ago

As stated in Django CMS issue tracker: https://github.com/django-cms/django-cms/issues/7948

Django CMS 4.1.x with modeltranslation active - custom CMS plugin attributes allow_children, and child_classes do not work correctly.

Each plugin that has allow_children has the ability to add child plugins but they are locked (one is not able to add plugins to children even though the allow_children with child_classes is set on Column in this example).

On the other hand, plugins that do not have explicitly set the allow_children attribute still have the ability to add children which is wrong.

Image 1 - working example with modeltranslation 0.19.2 - Text plugin has the + sign disabled which is expected, and the Column plugins have the + sign enabled which is expected:

working 0 9 12

Image 2 - not working example with modeltranslation 0.19.3 and above - Text plugin has the + sign enabled which is not expected, and the Column plugins have the lock sign which is not expected since no other plugins can be added into it:

not_working 0 9 13

Project environment variables

aacimov commented 1 month ago

Just tested with django-cms 3.11.x - affected as well.

last-partizan commented 1 month ago

Hello.

Are you sure version 0.19.0 is working? because i there isn't much changes. But jump from 0.18 to 0.19 included some relatively big changes.

If you have time, please do additional testing using git version and git bisect to identify exact bad commit.

aacimov commented 1 month ago

@last-partizan yes, everything below 0.19.3 is working as expected. I will look into it over weekend probably and let you know.

benzkji commented 1 month ago

for me as well, it is django-modeltranslation<0.19.3 that works.

benzkji commented 1 month ago

0.9.13 introduces changes on the admin clasess, that is what the cms uses, it makes sense to me.

last-partizan commented 1 month ago

Yeah, there's some changes in admin, but they're mostly related to typing support.

If someone can provide repo with minimal reproduction, i can look into this.

MacLake commented 1 month ago

I can confirm it, 0.19.2 works, and 0.19.3 introduces the bug in both django-cms 3.11.6 and 4.1.2.

MacLake commented 4 weeks ago

So here is a minimal repo for reproducing the issue: I have added modeltranslation to django-cms-quickstart and published a fork here:

https://github.com/MacLake/django-cms-quickstart/tree/modeltranslation. Follow the instructions to get the project running.

First add a plugin that can contain other plugins, e. g. a Jumbotron, at top level. You can add a text plugin. to the Jumotron.

Then add a container, add a Jumbotron to the container. Now you can’t add a text or any other plugin to this Jumbotron, as it is locked. This is caused by modeltranslation>=0.19.3.

Then stop the containers, switch to the main branch (i.e. without modeltranslation) and run

docker compose build
docker compose up

The database content persists. Open the structure board Now the nested Jumbotron doesn’t have a lock icon, and you can add plugins to it.

benzkji commented 4 weeks ago

there is some background info in #745. The problem seems hidden and complicated.

benzkji commented 3 weeks ago

I think I found the problem. Dict access of a admin class that inherits from a TranslationAdmin is possible, and doesn't throw any error. As far as I know, that kind of access is/should never be possible, or only in special cases (fabric 1 did have some kind of "DictObject", for it's env...)? As Django itself relies on it, in it's template engine.

class MyTranslationAdmin(TranslationAdmin):
    my_prop = "123"

MyTranslationAdmin["my_prop"]  # returns that class itself!

Also, a PR that shows that: https://github.com/deschler/django-modeltranslation/pull/749 (test fails with 0.19.3+, runs ok below)

benzkji commented 1 day ago

755 resolves this problem for me.

last-partizan commented 8 hours ago

Thanks for testing, merged and released.