Code

Opened 6 years ago

Closed 14 months ago

Last modified 4 weeks 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@…, jpic, 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 6 years ago.
django_t9321_r15242_a.diff (2.2 KB) - added by rspeed 3 years ago.
Updated patch from #12359 by trey@…
9321+14402.ManyToManyField-help-text.diff (18.7 KB) - added by julien 3 years ago.
9321.manytomanyfield-helptext.diff (13.3 KB) - added by julien 3 years ago.
Using extra_help_text
hold-control-selectmultiple-fix.patch (1.5 KB) - added by marcdm@… 2 years ago.
_patch_help_text() function and calls for ChoiceField and subsclasses
hold-control-selectmultiple-fix.2.patch (1.5 KB) - added by marcdm 2 years ago.
patch described in comment 33

Download all attachments as: .zip

Change History (59)

Changed 6 years ago by mrcai

comment:1 Changed 6 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 6 years ago by mtredinnick

  • Component changed from Database layer (models, ORM) to Forms
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 6 years ago by SmileyChris

  • Resolution invalid deleted
  • Status changed from closed to reopened

Malcolm, did you mean to reopen this too?

comment:4 Changed 6 years ago by mtredinnick

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

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

comment:5 Changed 5 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 5 years ago by anonymous

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:7 Changed 4 years ago by mathijsken

  • Needs documentation set
  • Version changed from 1.0 to 1.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 4 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 4 years ago by kmtracey

  • Summary changed from When overriding the widget for a manytomany field default helper text stays to When 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 4 years ago by bryce

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

Changed 3 years ago by rspeed

Updated patch from #12359 by trey@…

comment:11 Changed 3 years ago by rspeed

  • Component changed from Forms to Database layer (models, ORM)
  • Has patch set
  • Keywords related.py, admin added; related.py removed

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 3 years ago by julien

  • milestone set to 1.4
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Accepted to Design 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 3 years ago by julien (previous) (diff)

comment:13 Changed 3 years ago by julien

  • Component changed from Database layer (models, ORM) to Forms

Changed 3 years ago by julien

comment:14 Changed 3 years ago by julien

  • Easy pickings unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity set to Normal
  • Type set to 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 3 years ago by jezdez

  • 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 3 years ago by julien

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 follow-up: Changed 3 years ago by 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.

comment:18 Changed 3 years ago by SmileyChris

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 3 years ago by julien

Using extra_help_text

comment:19 Changed 3 years ago by julien

@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 3 years ago by SmileyChris

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 3 years ago by SmileyChris

Er, minimize the backwards incompatibility...

Version 0, edited 3 years ago by SmileyChris (next)

comment:22 Changed 3 years ago by julien

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 3 years ago by julien (previous) (diff)

comment:23 Changed 3 years ago by julien

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 ; follow-up: Changed 3 years ago by jezdez

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 3 years ago by julien

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 3 years ago by julien

  • UI/UX set

comment:27 Changed 3 years ago by anonymous

#16370 reported this again.

comment:28 Changed 3 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 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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

  • Cc carlos.palol@… added

comment:31 Changed 3 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 3 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 2 years ago by marcdm@…

_patch_help_text() function and calls for ChoiceField and subsclasses

comment:33 Changed 2 years ago by marcdm@…

  • Needs tests set
  • Resolution set to fixed
  • Status changed from reopened to closed
  • Version changed from 1.2 to 1.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 2 years ago by marcdm

patch described in comment 33

comment:34 Changed 2 years ago by russellm

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:35 Changed 2 years ago by domen@…

  • Cc domen@… added

What else is needed for this one to apply?

comment:36 Changed 2 years ago by quinode

  • Cc dguardiola@… added

comment:37 Changed 22 months ago by jpic

  • Cc jpic added

comment:38 Changed 21 months ago by ivan_virabyan

  • Cc ivan_virabyan added

comment:39 Changed 20 months ago by aaugustin

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

comment:40 Changed 18 months ago by dyve

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 16 months ago by aaugustin

  • Status changed from reopened to new

comment:42 Changed 16 months ago by gert

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 15 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

comment:45 Changed 15 months ago by dyve

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 15 months 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 14 months ago by ramiro

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 14 months ago by ramiro (previous) (diff)

comment:48 Changed 14 months ago by Ramiro Morales <cramm0@…>

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

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 14 months 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 4 months 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 4 months 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 4 weeks 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 4 weeks ago by timo

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.