Opened 8 years ago

Closed 3 years ago

Last modified 2 years ago

#9321 closed Bug (fixed)

When overriding the widget for a manytomany field default help_text stays

Reported by: mrcai Owned by: nobody
Component: Forms Version: 1.4
Severity: Normal Keywords: manytomanyfield, widget, related.py, admin
Cc: carlos.palol@…, domen@…, dguardiola@…, James Pic, Ivan Virabyan Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

When you override the widget for a manytomany field, the default helper_text "Hold down "Control", or "Command" on a Mac, to select more than one" remains. Which can be a little misleading.

In models.py I define a model Profile with:

favorite_genres = models.ManyToManyField(Genre, verbose_name='Favorite Genres',)

In forms.py I define a form ProfileEditForm(ModelForm):

class Meta:
  model = Profile

In my views.py I override the widget using:

ProfileEditForm.base_fields['favorite_genres'].widget = CheckboxSelectMultiple(
			choices=ProfileEditForm.base_fields['favorite_genres'].choices)

And it gets rendered as seen in the screen shot.

I guess the issue is with line 761 in django.db.models.fields.related

msg = ugettext_lazy('Hold down "Control", or "Command" on a Mac, to select more than one.')

I'm not really sure about how to go about patching this, so I'll just flag it if that's ok?

(My first ticket, hope I've done it right. I've put this in Database layer based on it being related.py that's going to need a patch)

Attachments (6)

manytomany.tiff (85.4 KB) - added by mrcai 8 years ago.
django_t9321_r15242_a.diff (2.2 KB) - added by Rob Speed 6 years ago.
Updated patch from #12359 by trey@…
9321+14402.ManyToManyField-help-text.diff (18.7 KB) - added by Julien Phalip 5 years ago.
9321.manytomanyfield-helptext.diff (13.3 KB) - added by Julien Phalip 5 years ago.
Using extra_help_text
hold-control-selectmultiple-fix.patch (1.5 KB) - added by marcdm@… 5 years ago.
_patch_help_text() function and calls for ChoiceField and subsclasses
hold-control-selectmultiple-fix.2.patch (1.5 KB) - added by marcdm 5 years ago.
patch described in comment 33

Download all attachments as: .zip

Change History (59)

Changed 8 years ago by mrcai

Attachment: manytomany.tiff added

comment:1 Changed 8 years ago by Julien Phalip

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

help_text is a field attribute and is independent from the widget. So, what you'd need to do is something like:

ProfileEditForm.base_fields['favorite_genres'].help_text = 'Some help in there'

comment:2 Changed 8 years ago by Malcolm Tredinnick

Component: Database layer (models, ORM)Forms
Triage Stage: UnreviewedAccepted

comment:3 Changed 8 years ago by Chris Beaven

Resolution: invalid
Status: closedreopened

Malcolm, did you mean to reopen this too?

comment:4 Changed 8 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

Apparently not. I missed Julien's comment. He's correct.

comment:5 Changed 8 years ago by Usip

Actually, julien is incorrect

if you check the file that mrcai reported, the ManyToManyField makes the assumption than django forms will use
SelectMultiple widget and concatenate the help_text of the model-field with the msg variable.

      msg = ugettext_lazy('Hold down "Control", or "Command" on a Mac, to select more than one.')

However, if you'd change the default widgets for MultipleChoiceFields (Model and plain) to CheckboxSelectMultiple, from for instance
your settings file.

  forms.MultipleChoiceField.widget = forms.widgets.CheckboxSelectMultiple
  forms.ModelMultipleChoiceField.widget = forms.widgets.CheckboxSelectMultiple

A forms field will now have concatenated the models help_text with the msg and you will end up
with a checkboxes and a misleading help_text (Hold down control....).

And as mrcai pointed out, the bug is in django.db.models.fields.related.py.

In my opinion this is a bug as the text should be appended by the widget and not by the db model field as it currently is. Please
verify your assumptions that it has nothing to do with the model field before closing this again, because it does.

comment:6 Changed 8 years ago by anonymous

Resolution: fixed
Status: closedreopened

comment:7 Changed 6 years ago by mathijsken

Needs documentation: set
Version: 1.01.2

I ran into this very issue just now and solved it by creating an UnhelpFullManyToManyField that overrides the init method (actually it just sets the help_text to an empty string)

Personally, I think the last two lines of the init should look something like:

if not self.help_text or self.help_text=="":

self.help_text = _('Hold down "Control", or "Command" on a Mac, to select more than one.')

This would change the behaviour though, and needs to be documented. It makes more sense to me, because this will act as a 'default' value that the user can still override if he wants to. He'll then loose the original help_text, but it's easy enough to copy/paste the string.

comment:8 Changed 6 years ago by gorghoa

I agree with Usip.

The help text should be attached to a widget ( view ) and not to the model. ( In my vision of the mvc/mvt )

It makes totally nonsense to me to attach it to the model. Moreover if we can change the widget...

regards.
Gorghoa

comment:9 Changed 6 years ago by Karen Tracey

Summary: When overriding the widget for a manytomany field default helper text staysWhen overriding the widget for a manytomany field default help_text stays

#12359 and #13858 also reported this; the first has a patch (but no test).

comment:10 Changed 6 years ago by bryce

Is there update for this issue? The workarounds are all pretty annoying

Changed 6 years ago by Rob Speed

Attachment: django_t9321_r15242_a.diff added

Updated patch from #12359 by trey@…

comment:11 Changed 6 years ago by Rob Speed

Component: FormsDatabase layer (models, ORM)
Has patch: set
Keywords: admin added

Attached an updated version of the patch from #12359 by trey@… which moves the assignment of the help_text from django.db.models.fields.related to django.contrib.admin.options.

I had begun making a patch to move the help_text assignment to django.forms.widgets.SelectMultiple, but in doing so I realized that this, too, is poor behavior. The help_text property is expected to be in direct control of the application; in no situation should it be modified by the framework.

comment:12 Changed 6 years ago by Julien Phalip

milestone: 1.4
Needs tests: set
Patch needs improvement: set
Triage Stage: AcceptedDesign decision needed

I'm marking as DDN since we still need to figure out the best approach for amending the help text based on which widget is used. I strongly feel that each widget should be able to control what help text gets displayed. I haven't given this enough thoughts yet, but perhaps it could be along the lines of:

# In django/forms/widgets.py
class Widget(object):
    ...
    def get_help_text(self, text):
        """ Hook to allow a widget to amend its help text """
        return text

class SelectMultiple(Select):
    ...
    def get_help_text(self, text):
        return string_concat(text, ' ', _('Hold down "Control", or "Command" on a Mac, to select more than one.'))

... and then call the get_help_text() method at the right time either when building or rendering the form.

See also #14402 for a tightly linked issue. I'd recommend solving both tickets at once.

I'd really like to see this fixed in 1.4.

Last edited 6 years ago by Julien Phalip (previous) (diff)

comment:13 Changed 6 years ago by Julien Phalip

Component: Database layer (models, ORM)Forms

Changed 5 years ago by Julien Phalip

comment:14 Changed 5 years ago by Julien Phalip

Easy pickings: unset
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Severity: Normal
Type: Bug

The attached patch solves two tickets at once, i.e. this one and #14402. The reason is that the bug fixes for those two tickets both rely on the introduction of the same new method Widget.alter_help_text. If this approach is validated then I'll break the patch in two, and #14402 can be checked in after this one.

The idea behing alter_help_text is to allow the widget to customize the form field's help text -- for example SelectMultiple would now be responsible for appending the 'Hold down "Control"' message instead of letting ManyToManyField do it. This new feature introduces a backwards incompatible change, although the upgrade path is really simple. See the doc in the patch for more info about that.

Let me know what you think!

comment:15 Changed 5 years ago by Jannis Leidel

Patch needs improvement: set

Yeah, I really think this callback API is odd, I don't like it at all, why not just have a extra_help_text class attribute in a widget that is appended if it exists?

comment:16 Changed 5 years ago by Julien Phalip

Well, this callback would allow the widget to operate more changes that just append some text, for example prepend some text or replace it altogether. A bit like the way the ModelAdmin.save_model() hook works. If you think that's a YAGNI and that we should only allow appending text, then a extra_help_text class attribute would work. Note that it would also be backwards compatible (which is cool, I think, if we want to get it right).

comment:17 Changed 5 years ago by Julien Phalip

Just following on your comment, I see your point about the oddness of that callback. It felt similarly odd to me when the ModelAdmin.save_model() and ModelAdmin.delete_model() hooks were introduced. I think the oddness is the price to pay for more flexibility and future-proofness. So I'm a bit ambivalent about this. I'll resubmit a patch using the extra_help_text class attribute so we can compare approaches.

comment:18 Changed 5 years ago by Chris Beaven

I also don't like the fact this is adding the default help text for all SelectMultiple widgets, as noted by the requirement to change current tests.

Changed 5 years ago by Julien Phalip

Using extra_help_text

comment:19 Changed 5 years ago by Julien Phalip

@SmileyChris: Yes, this is a manifestation of the backwards incompatible change of this patch. The "Hold down Control" message only makes sense when used with a <select> widget where multiple items can be selected, i.e. with SelectMultiple. So, had the behaviour been proper, then that message should have always been there in the tests, not just when used in the context of a ManyToManyField. I see this change more as way of re-establishing the correct behaviour. What do you think?

comment:20 Changed 5 years ago by Chris Beaven

I'd prefer to minimize the backwards compatibility of the patch. I don't really want people to have to hunt through and modify every instance of SelectMultiple they've used to continue to have their help_text unmolested.

comment:21 Changed 5 years ago by Chris Beaven

Er, minimize the backwards incompatibility...

Version 0, edited 5 years ago by Chris Beaven (next)

comment:22 Changed 5 years ago by Julien Phalip

On further reflection, one radical approach would be to remove the 'Hold down Control' extra message altogether, and let the developers directly add some extra information, if they see fit, using the traditional way via the model field's or form field's help_text attributes. The use of that extra message is actually quite debatable, and it has caused a lot of confusion for years from an implementation standpoint.

Anyways, just a thought.

Last edited 5 years ago by Julien Phalip (previous) (diff)

comment:23 Changed 5 years ago by Julien Phalip

OK, thank you all for your feedback. I'll try to summarize the problem and discussions so far. So, an ideal solution would:

  • stop ManyToManyField from directly appending the 'Hold down Control' message, since this has nothing to do with ORM-level business.
  • allow non-standard widgets and widgets inheriting from SelectMultiple to not append the 'Hold down Control' message when used in conjunction with a ManyToManyField.
  • be backwards compatible as it would potentially affect a lot of people:
    • When the default SelectMultiple widget, or any widget inheriting from SelectMultiple, is not used in conjunction with a ManyToManyField, then do not append the message.
    • When the default SelectMultiple is used in conjunction with a ManyToManyField, then do append the message.

A compromise needs to be found between doing things right and not annoying too many people with a potentially backwards incompatible change. The challenge is that, as much as ManyToManyField should not be allowed to guess which widget will be used, the SelectMultiple widget should also not trying to guess that it's used with a ManyToManyField. Maybe the compromise lies in the form field's realm, in particular ModelMultipleChoiceField.

Finally, a bit of forensic research reveals this behaviour was introduced at least at the time of the magic-removal branch merge: http://code.djangoproject.com/browser/django/trunk/django/db/models/fields/related.py?rev=2809#L580

comment:24 in reply to:  17 ; Changed 5 years ago by Jannis Leidel

Replying to julien:

Just following on your comment, I see your point about the oddness of that callback. It felt similarly odd to me when the ModelAdmin.save_model() and ModelAdmin.delete_model() hooks were introduced. I think the oddness is the price to pay for more flexibility and future-proofness. So I'm a bit ambivalent about this. I'll resubmit a patch using the extra_help_text class attribute so we can compare approaches.

Honestly this is not a good comparison at all, ModelAdmin is at a completely different abstraction layer; users are encouraged to write them while Widgets (especially the SelectMultiple) not so much.

comment:25 in reply to:  24 Changed 5 years ago by Julien Phalip

Replying to jezdez:

Honestly this is not a good comparison at all, ModelAdmin is at a completely different abstraction layer; users are encouraged to write them while Widgets (especially the SelectMultiple) not so much.

The comparison was just purely about the hook implementation paradigm, nothing more. I've actually had to write custom widgets on many occasions for the admin (in particular for the ManyToManyField), which is why I've always been keen to fix this ticket :-)

comment:26 Changed 5 years ago by Julien Phalip

UI/UX: set

comment:27 Changed 5 years ago by anonymous

#16370 reported this again.

comment:28 Changed 5 years ago by anonymous

I just ran into this and am having trouble finding a workaround. Can someone list the preferred workaround until the bug is resolved?

comment:29 Changed 5 years ago by Jacob

milestone: 1.4

Milestone 1.4 deleted

comment:30 Changed 5 years ago by carlos.palol@…

Cc: carlos.palol@… added

comment:31 Changed 5 years ago by anonymous

I solved this with:
class mymodelform(ModelForm):

class Meta:

pass

def init(self, *args, kwargs):

super(RegisterForm, self).init(*args, kwargs)

self.fieldsstaying?.help_text =

self.fieldsfamily_members?.help_text =

don't know if this is 'kosher' but it seems to work for me!

comment:32 Changed 5 years ago by dblado

and w/ formatting and me logged in

I solved this with:

class mymodelform(ModelForm):
    class Meta:
        pass
    def __init__(self, *args, **kwargs):
        super(RegisterForm, self).__init__(*args, **kwargs)
        self.fields['staying'].help_text = ''
        self.fields['family_members'].help_text = ''

don't know if this is 'kosher' but it seems to work for me!

Changed 5 years ago by marcdm@…

_patch_help_text() function and calls for ChoiceField and subsclasses

comment:33 Changed 5 years ago by marcdm@…

Needs tests: set
Resolution: fixed
Status: reopenedclosed
Version: 1.21.4

Please accept my apologies for this patch if the format is wrong or something similar.

I think I've fixed this bug in a fairly simple way.

The thing that is at issue here is the addition of widget-related help-text by the Database field object (ManyToManyField) instead of the Form field object (MultipleChoiceField, ModelMultipleChoiceField, TypedChoiceField etc.)

The patch does the following :

  • remove the 2 lines that add the message in django.db.models.related.ManyToManyField.init
  • add a private method to django.forms.fields.ChoiceField field called _patch_help_text().
  • add calls to this method in the init of MultipleChoiceField and ModelMultipleChoiceField

The _patch_help_text() method, when called :

  • checks the widget on the form and updates the help text if the widget is a SelectMultiple.
  • uses type() to check self.widget so the message isn't printed for subclasses of SelectMultiple

What do u think? Sorry, I haven't read up on writing tests yet, so no tests. But, the goal was simple, show the message for SelectMultiple, suppress it all other times.

Changed 5 years ago by marcdm

patch described in comment 33

comment:34 Changed 5 years ago by Russell Keith-Magee

Resolution: fixed
Status: closedreopened

A ticket is closed when the patch is applied, not when the patch is provided. Reopening.

comment:35 Changed 4 years ago by domen@…

Cc: domen@… added

What else is needed for this one to apply?

comment:36 Changed 4 years ago by domguard

Cc: dguardiola@… added

comment:37 Changed 4 years ago by James Pic

Cc: James Pic added

comment:38 Changed 4 years ago by Ivan Virabyan

Cc: Ivan Virabyan added

comment:39 Changed 4 years ago by Aymeric Augustin

#19066 was a duplicate with a simple patch and tests.

comment:40 Changed 4 years ago by Dylan Verheul

I keep running into this issue. It's a really obnoxious test that certain does not belong in ManyToManyField. It belongs in widgets if anywhere. But why add this to every Many To Many? We don't show "Press Enter to submit" help texts when displaying forms?

This functionality assumes a HTML select widget, and nowadays much more different widget styles (scripted and otherwise) are available.

Basically, the text is a piece of hardcoded UI that should be completely removed instead of toned down. The only reason not to remove this is backward compatibility. If removing it is too big a step, it should be moved to SelectMultiple widget.

comment:41 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:42 Changed 4 years ago by Gert Steyn

1) As many people have stated (in various duplicate tickets) the help text should not be in the database layer

2) Also, overriding the help text from the model definition has no effect - so putting the help text in the database layer is breaking the model definition

This has been going on for 4 years now, without resolution, please allow me to propose a different approach:

Firstly it's a bug, there is no need to maintain any backwards compatibility. Why don't we delete it from the database layer and then opening a new ticket that states that the widget does not display the help text. That way at least just the widget is broken, currently the widget and the database layer is broken.

comment:44 Changed 3 years ago by Jacob

Triage Stage: Design decision neededAccepted

comment:45 Changed 3 years ago by Dylan Verheul

Adding my support to absolutely removing this from the database layer. I keep running into this. And sometimes it's just plain ridiculous (for example when using CheckboxSelectMultiple for a ManyToMany.

comment:46 Changed 3 years ago by anonymous

This is the single most annoying bug on django, if dyve won't fix it I will. For now I'll wait.

comment:47 Changed 3 years ago by Ramiro Morales

I've posted a proposal to the django-dev mailing list: https://groups.google.com/forum/?hl=en&fromgroups#!topic/django-developers/PqfHA3hvAxo

Feedback welcome.

Last edited 3 years ago by Ramiro Morales (previous) (diff)

comment:48 Changed 3 years ago by Ramiro Morales <cramm0@…>

Resolution: fixed
Status: newclosed

In 4ba1c2e785feecfa7a47aa5336a2b595f086a765:

Fixed #9321 -- Deprecated hard-coding of help text in model ManyToManyField fields.

This is backward incompatible for custom form field/widgets that rely
on the hard-coded 'Hold down "Control", or "Command" on a Mac, to select
more than one.' sentence.

Application that use standard model form fields and widgets aren't
affected but need to start handling these help texts by themselves
before Django 1.8.

For more details, see the related release notes and deprecation timeline
sections added with this commit.

comment:49 Changed 3 years ago by Ramiro Morales <cramm0@…>

In 8c2fd050f80f528cc1609c1a7f16901194834831:

Made fix for #9321 less buggy and more effective.

Don't try to be smart about building a good-looking help string
because it evaluates translations too early, simply use the same old
strategy as before. Thanks Donald Stufft for the report.

Also, actually fix the case reported by the OP by special-casing
CheckboxSelectMultiple.

Added tests.

Refs #9321.

comment:50 Changed 3 years ago by Tim Graham <timograham@…>

In e80de93af6a0a21a9063a55c4d6d20e3927243e9:

Removed hard-coded help_text for ManyToManyFields that use a SelectMultiple widget

Per deprecation timeline; refs #9321.

comment:51 Changed 3 years ago by Ramiro Morales <cramm0@…>

In 491419b5ffac3752a1c1804a763c017a2ed82e16:

Made m2m fields form help_text munging specific to admin widgets.

Refs #9321 and follow-up to e80de93af6a0a21a9063a55c4d6d20e3927243e9.

comment:52 Changed 2 years ago by anonymous

will this be implemented in 1.8? the ticket says 1.4 and the "press ctrl..."-help text is still appended to any custom text.

comment:53 Changed 2 years ago by Tim Graham

Please look at the commits above for details. The Version field on the ticket merely indicates what the current version was when the ticket was reported.

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