Re: [Django] #2750: ManyToManyField ignores 'default' option
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by gcc):
* cc: chris+django@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:15>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: anubhav9042@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Someday/Maybe
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jarshwah):
https://github.com/django/django/pull/3108
I'm of the opinion that `default` for ManyToManyField doesn't make sense.
The default value of a field is used when a model is saved without a user
value. M2M fields can not have a value when being saved (values must be
added with the RelatedManager), so the default value would always be
applied if implemented.
I think the underlying problem of pre-selecting values in the admin for
m2m form fields is orthogonal to whether or not m2m model fields should
respect the default option.
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by jarshwah):
* has_patch: 0 => 1
* stage: Someday/Maybe => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by jarshwah):
* cc: josh.smeaton@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
There are failing tests and usage of this in Django's test suite that need
to be removed.
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
Comment:
While I don't think `default` makes sense for m2m models, it appears that
forms make use of the default option, so we can't simply check/warn and
document. This will involve some deeper investigation with regards to how
forms use the m2m default. I'll try to look into this a little more.
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by phildini):
Has there been any movement on this? When you say forms make use of the
default option, what do you mean?
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by François Granade):
Replying to [comment:22 Philip James]:
> Has there been any movement on this? When you say forms make use of the
default option, what do you mean?
what is meant here is:
the `default` option for a `ManyToManyField` is not used when saving the
object (when calling `<object>.save()`, or other): even if it is set (e.g.
to a callable returning an object list), the field will be set to an empty
list when saving.
However, this `default` option value is used in Django `Form` set up to
edit this object. In particular, in Django Admin, a the multivalue drop-
down will include for the field, that will contain all the objects
returned by the callable defined as the `default` value - and they will be
all selected. If the user saves the object without any edit (by pushing
the form's "save" button), then the field will indeed be set to all the
values.
The current behavior is a little misleading, I think (and I'm not the
only. one, from previous comments): I would expect the options set if the
model's fields to be used in the model's methods; if an option is only
used for forms, I would expect it to be set in the form, not in the model.
There may be design reasons for the current behavior though ?
In any case, this should probably be documented in the `ManyToManyField`
documentation ?
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:24>
Django
-------------------------------------+-------------------------------------
| verma
Type: Bug | Status: assigned
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Akshat verma
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Akshat
| verma
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 1
-------------------------------------+-------------------------------------
Changes (by Akshat verma):
* cc: Akshat verma (added)
* needs_tests: 0 => 1
* version: => 4.2
* easy: 0 => 1
* needs_docs: 0 => 1
* ui_ux: 0 => 1
Old description:
> It doesn't seem possible to use the 'default' option to pre-select values
> in the admin site for a ManyToManyField. I've tried a string value, a
> list of strings, and even (following a recommendation from someone on
> #django) passing a list of the actual objects. It would be nice to either
> have this functionality added or to update the docs to reflect the fact
> that 'default' has no effect on ManyToManyFields.
>
> -Basil
New description:
It doesn't seem possible to use the 'default' option to pre-select values
in the admin site for a ManyToManyField. I've tried a string value, a list
of strings, and even (following a recommendation from someone on #django)
passing a list of the actual objects. It would be nice to either have this
functionality added or to update the docs to reflect the fact that
'default' has no effect on ManyToManyFields.
-Basil
#you can override the init of the modelform like:
def __init__(self, *args, **kwargs):
if 'initial' not in kwargs:
kwargs['initial'] = {}
kwargs['initial'].update(product_template_admin_initial_values())
super(ProductTemplateAdminForm, self).__init__(*args, **kwargs)
#But that will not allow to use the request.
#If you depend on the request you can use the admin
get_changeform_initial_data
def get_changeform_initial_data(self, request):
return {'name': 'custom_initial_value'}
#You can call a method with the request as argument and return a
dictionary. the m2m default should be a list
--
Comment:
#you can override the init of the modelform like:
def __init__(self, *args, **kwargs):
if 'initial' not in kwargs:
kwargs['initial'] = {}
kwargs['initial'].update(product_template_admin_initial_values())
super(ProductTemplateAdminForm, self).__init__(*args, **kwargs)
#But that will not allow to use the request.
#If you depend on the request you can use the admin
get_changeform_initial_data
def get_changeform_initial_data(self, request):
return {'name': 'custom_initial_value'}
#You can call a method with the request as argument and return a
dictionary. the m2m default should be a list
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Akshat
| verma
Type: Bug | Status: assigned
Component: Database layer | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
Akshat, I deleted your comment to restore the ticket description. Please
don't change ticket descriptions. If you want to work on the ticket and
propose a patch, do this via GitHub PR.
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Akshat
| verma
Type: Bug | Status: assigned
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Akshat verma
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:28>
Django
-------------------------------------+-------------------------------------
| Feijen
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
There is a new [https://github.com/django/django/pull/18581 doc update PR]
that needs review, setting the right flags.
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Wim
| Feijen
Type: Bug | Status: assigned
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:26>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 1 => 0
* status: assigned => new
* version: => dev
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:27>
Django
-------------------------------------+-------------------------------------
| Peiris
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Nipun Peiris
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Nipun
| Peiris
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Update: I've implemented the fix by modifying `Field.formfield()` to check
`self.has_default() and not self.many_to_many` before applying defaults.
The regression test I wrote passes on my branch. However, there are some
existing test failures. I need to look into, they might be relying on the
old behavior. Will update once I've sorted those out.
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Nipun
| Peiris
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Nipun
| Peiris
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Nipun Peiris):
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Nipun
| Peiris
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Nipun Peiris):
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: bangcok_dangerus@… | Owner: Nipun
| Peiris
Type: Bug | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
I've marked this as patch needs improvement, but really I think it's a
case of a better way forward needs to be mapped out.
This is an old ticket, asking for something that now exists. Django has
supported setting a `default` on M2M's for the admin/forms since newforms.
- **2006**: Ticket opened asking for default on a M2M field to be picked
up by the Admin.
- **2008**: This gets supported in 1.0 (but you need raw ids). There was
never any documentation on how it works for M2M. This ticket was never
updated/closed.
- **2009**: Thomas wants to pass the queryset, but it doesn't work, gets
no response.
- **2011**: In 1.3 that starts working due to work on #13679 (which also
covered M2M as a tested side effect, still not documented). No update on
this ticket, it could definitely be closed at this point. **It's now not
clear what the ticket is actually for.**
- **2014**: Josh points out that default on M2M is confusing, it doesn't
work with save (and how could it), but the PR is closed as the feature is
in use by forms/admin.
- Since then there has been a PR with a short docs update that was
closed, and then the current PR that removed the admin/forms
functionality.
So I see three paths forward:
1. Close the ticket, it's done per the description.
1. Do a good docs update, explain how `default` on M2M is used for
Admin/Forms and doesn't make sense for Models. (My current preference)
1. Deprecate `default` support for M2M in Admin/Forms, and tell people
they have to use one of the following (or several, kept in sync).
People currently gets their Form, Admin and InlineAdmin working with a
single `default`:
{{{#!python
def current_site():
return Site.objects.filter(pk=settings.SITE_ID)
class Article(models.Model):
publish_on = models.ManyToManyField(Site, default=current_site)
}}}
Without it, they'd need it in each place:
{{{#!python
class ArticleForm(forms.ModelForm):
publish_on = forms.ModelMultipleChoiceField(
queryset=Site.objects.all(),
initial=current_site,
)
class ArticleAdmin(admin.ModelAdmin):
def get_changeform_initial_data(self, request):
initial = super().get_changeform_initial_data(request)
initial["publish_on"] = list(current_site().values_list("pk",
flat=True))
return initial
class ArticleInline(admin.TabularInline):
def get_formset(self, request, obj=None, **kwargs):
formset = super().get_formset(request, obj, **kwargs)
formset.form.base_fields["publish_on"].initial = current_site
return formset
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/2750#comment:33>