Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#21428 closed Bug (fixed)

Django 1.6 GenericRelation editable regression

Reported by: joshcartme Owned by: nobody
Component: Database layer (models, ORM) Version: 1.6
Severity: Release blocker Keywords: GenericRelation, ModelForm, Admin, editable
Cc: loic@… Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (17)

comment:1 Changed 16 months ago by timo

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

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.

comment:2 Changed 16 months ago by akaariai

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.

comment:3 Changed 16 months ago by loic84

  • Cc loic@… added

That's a good compromise IMO.

comment:4 Changed 16 months ago by joshcartme

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.

comment:5 Changed 16 months ago by stephenmcd

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

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

comment:6 Changed 16 months ago by 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

comment:7 Changed 16 months ago by 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:

--- a/django/forms/models.py
+++ b/django/forms/models.py
@@ -82,7 +82,9 @@ def save_instance(form, instance, fields=None, fail_message='saved',

# 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:

comment:8 Changed 16 months ago by stephenmcd

(apologies for the formatting, first time trac user)

comment:9 Changed 16 months ago by stephenmcd

Here's the change nicely formatted:

https://gist.github.com/stephenmcd/7441463

comment:10 Changed 16 months ago by akaariai

  • Triage Stage changed from Accepted to Ready for checkin

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. Also, a test case or project would be very welcome.

Version 0, edited 16 months ago by akaariai (next)

comment:11 Changed 16 months ago by joshcartme

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.

comment:12 follow-up: Changed 16 months ago by 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?

comment:13 Changed 16 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

comment:14 Changed 16 months ago by 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.

comment:15 Changed 16 months ago by 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.

comment:16 Changed 16 months ago by 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

comment:17 in reply to: ↑ 12 Changed 16 months ago by 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

Note: See TracTickets for help on using tickets.
Back to Top