Django 1.6 GenericRelation editable regression

[Josh Cartmell]

Prior to Django 1.6 it was possible to mark a GenericRelation as editable and then add it to the admin fields/fieldsets of a model it was a field of.

The following is a simple Django project/app which demonstrates behavior that works under 1.5 but fails under 1.6. It includes a unittest that similarly works under 1.5 but fails under 1.6.
​https://bitbucket.org/joshcartme/16genericissue

I brought this up on the developers mailing list, here is the thread:
​https://groups.google.com/forum/#!topic/django-developers/Or7LmK17LIU

The instigation for this ticket is that Mezzanine makes use of GenericRelations in the way mentioned above. We have been trying to figure out how to bring Django 1.6 support to Mezzanine. Here is the Mezzanine ticket:
​https://github.com/stephenmcd/mezzanine/issues/745

From that Mezzanine ticket, here is a diff that reverts the changes that cause my simple test case to fail:
​https://gist.github.com/loic/7427160

[Tim Graham]

If you are able to write a test for Django's test suite itself instead of as a separate project, that will help us expedite this fix.

[Anssi Kääriäinen]

Editable GenericRelations weren't designed to work in modelforms, and I guess that is the reason why there is zero tests for this. To see that GenericRelations try to avoid to be editable, see GenericRelation.__init__ which explicitly overrides editable to False. But as Mezzanine relies on this and I don't see any way to overcome this in Mezzanine I think we should fix this in Django.

I have a proposed patch that just adds virtual fields to modelforms if they are editable. I wonder if this is enough to make Mezzanine work in 1.6?

Patch against 1.6 available from ​https://github.com/akaariai/django/compare/django:stable%2F1.6.x...ticket_21428.

[loic84]

That's a good compromise IMO.

[Josh Cartmell]

Thanks for the patch @akaariai, that does expose the fields in the admin, but Mezzanine is then running into some other problems after saving. I'll see if we can track down whether the remaining problems are with Mezzanine, or Django and post back here.

[stephenmcd]

I've created a pull request for the other issue we came across:

​https://github.com/django/django/pull/1913

[stephenmcd]

I've just run Mezzanine against akaariai's patch and it's much improved but still a minor issue

Previously in Django 1.5 the GenericRelation's save_form_data method would be called, but this no longer happens in 1.6

[stephenmcd]

The made this addition to akaari's patch using the same logic as elsewhere in the patch, which allows save_form_data to be called on the GenericRelation's form field:

django/forms/models.py

@@ -82 +82 @@
     # Wrap up the saving of m2m data as a function.
     def save_m2m():
         cleaned_data = form.cleaned_data
-        for f in opts.many_to_many:
+        for f in (opts.virtual_fields + opts.many_to_many):
+            if not getattr(f, 'editable', False):
+                continue
             if fields and f.name not in fields:
                 continue
             if exclude and f.name in exclude:

[Anssi Kääriäinen]

Pull request here: ​https://github.com/django/django/pull/1914. I'll mark it ready for checkin but leave it open for a day or two if somebody wants to review.

Please open separate ticket for the Admin issue.

EDIT: Removed request for test case - that already exists in the PR for the admin issue.

[Josh Cartmell]

Thanks @akaariai for the help so far, sorry that I dropped of the grid there for a few days.

Is the admin issue you want a separate ticket for the one addressed by this patch of yours:
​​https://github.com/akaariai/django/compare/django:stable%2F1.6.x...ticket_21428.

[Anssi Kääriäinen]

Admin issue is tracked in #21431 and a proposed fix is in ​https://github.com/akaariai/django/commit/c895dd39ae4e7687b57ff01ecfcc27caf70323ed. Could you verify the proposed fix works for Mezzanine?

[Anssi Kääriäinen <akaariai@…>]

In 0e079e4331a8be4dbd18d5e5776116330b0a5e61:

Fixed #21428 -- editable GenericRelation regression

The GenericRelation refactoring removed GenericRelations from
model._meta.many_to_many. This had the side effect of disallowing
editable GenericRelations in ModelForms. Editable GenericRelations
aren't officially supported, but if we don't fix this we don't offer any
upgrade path for those who used the ability to set editable=True
in GenericRelation subclass.

Thanks to Trac alias joshcartme for the report and stephencmd and Loic
for working on this issue.

[Anssi Kääriäinen <akaariai@…>]

In 1fd762c106ed0e6888fe66e7513718ca379385c0:

[1.6.x] Fixed #21428 -- editable GenericRelation regression

The GenericRelation refactoring removed GenericRelations from
model._meta.many_to_many. This had the side effect of disallowing
editable GenericRelations in ModelForms. Editable GenericRelations
aren't officially supported, but if we don't fix this we don't offer any
upgrade path for those who used the ability to set editable=True
in GenericRelation subclass.

Thanks to Trac alias joshcartme for the report and stephencmd and Loic
for working on this issue.

Backpatch of 0e079e4331a8be4dbd18d5e5776116330b0a5e61 from master.

[Anssi Kääriäinen <akaariai@…>]

In 326539f6a4936cda031b4a5f7caad788569f3329:

Fixed a regression caused by fix for #21428

On Python 3 sorting Fields mixed with GenericForeignKeys doesn't work
as GenericForeignKey isn't a subclass of django.db.models.fields.Field.

Refs #21428.

[Anssi Kääriäinen <akaariai@…>]

In cbf8784d206819e83c43d75b422af51209c4b74c:

[1.6.x] Fixed a regression caused by fix for #21428

On Python 3 sorting Fields mixed with GenericForeignKeys doesn't work
as GenericForeignKey isn't a subclass of django.db.models.fields.Field.

Refs #21428.

Backport of 326539f6a4 from master

[stephenmcd]

Replying to akaariai:

Admin issue is tracked in #21431 and a proposed fix is in ​https://github.com/akaariai/django/commit/c895dd39ae4e7687b57ff01ecfcc27caf70323ed. Could you verify the proposed fix works for Mezzanine?

I can confirm the merged fix resolves the issue in Mezzanine - thanks so much for your help!

Once #21431 is resolved we'll be able to release the next Mezzanine compatible with Django 1.6