[Django] #21428: Django 1.6 GenericRelation editable regression
Django
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
joshcartme | Status: new
Type: | Version: 1.6
Uncategorized | Keywords: GenericRelation, ModelForm, Admin,
Component: | editable
Uncategorized | Has patch: 0
Severity: Normal | UI/UX: 0
Triage Stage: |
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/21428>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* severity: Normal => Release blocker
* needs_better_patch: => 0
* component: Uncategorized => Database layer (models, ORM)
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:1>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:2>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: loic@… (added)
Comment:
That's a good compromise IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:3>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:4>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by stephenmcd):
I've created a pull request for the other issue we came across:
https://github.com/django/django/pull/1913
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:5>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:6>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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:
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:7>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by stephenmcd):
(apologies for the formatting, first time trac user)
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:8>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Accepted
Keywords: GenericRelation, | Needs documentation: 0
ModelForm, Admin, editable | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by stephenmcd):
Here's the change nicely formatted:
https://gist.github.com/stephenmcd/7441463
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:9>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by akaariai):
* stage: Accepted => Ready for checkin
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:10>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:11>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:12>
Django
Reporter: joshcartme | Owner: nobody
Component: Database layer | Version: 1.6
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"0e079e4331a8be4dbd18d5e5776116330b0a5e61"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:13>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"1fd762c106ed0e6888fe66e7513718ca379385c0"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:14>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"326539f6a4936cda031b4a5f7caad788569f3329"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:15>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Anssi Kääriäinen <akaariai@…>):
In [changeset:"cbf8784d206819e83c43d75b422af51209c4b74c"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:16>
Django
Reporter: joshcartme | Owner: nobody
Type: Bug | Status: closed
Component: Database layer | Version: 1.6
(models, ORM) | Resolution: fixed
Severity: Release blocker | Triage Stage: Ready for
Keywords: GenericRelation, | checkin
ModelForm, Admin, editable | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by stephenmcd):
Replying to [comment:12 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
--
Ticket URL: <https://code.djangoproject.com/ticket/21428#comment:17>