Opened 11 years ago

Closed 11 years ago

Last modified 9 years ago

#21428 closed Bug (fixed)

Django 1.6 GenericRelation editable regression

Reported by: Josh Cartmell 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 (15)

comment:1 by Tim Graham, 11 years ago

Component: UncategorizedDatabase layer (models, ORM)
Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

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 by Anssi Kääriäinen, 11 years ago

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 by loic84, 11 years ago

Cc: loic@… added

That's a good compromise IMO.

comment:4 by Josh Cartmell, 11 years ago

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 by stephenmcd, 11 years ago

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

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

comment:6 by stephenmcd, 11 years ago

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 by stephenmcd, 11 years ago

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:

Version 0, edited 11 years ago by stephenmcd (next)

comment:10 by Anssi Kääriäinen, 11 years ago

Triage Stage: AcceptedReady 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.

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

Last edited 11 years ago by Anssi Kääriäinen (previous) (diff)

comment:11 by Josh Cartmell, 11 years ago

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 by Anssi Kääriäinen, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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 by Anssi Kääriäinen <akaariai@…>, 11 years ago

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

in reply to:  12 comment:17 by stephenmcd, 11 years ago

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