prepare method in Manipulator (and FomField) doesn't get called (and also a minor bug in CheckboxSelectMultipleField)

[Le Roux]

I'm useing a CheckboxSelectMultipleField in a custom manipulator. (this
really needs documenting, by the way)

First off, the checkbox fields never get given the attribute
value="on". This is checked in prepare() to populate new_data. This is
easy to fix by just adding it into the render() method.

But.. prepare never gets called. I see Manipulator's prepare() method
calls prepare() on all the fields, but where is this supposed to get
called? I would have thought that calling it at the top of
get_validation_errors() would make sense.

Is this really a bug or should I call manipulator.prepare() inside my
view?

If it is a bug, then I'm happy to fix it and post a patch.. (didn't
want to add a ticket yet because I'm not sure about my understanding) I
can see how something like this could be missed, because that's the
only form field that actually uses prepare and since the formfield
generally doesn't work it probably means it is not widely used ;)

Le Roux

[Malcolm Tredinnick]

The prepare() methods are part of work-in-progress for some new
manipulator functionality. It is not a finished API. Ignore them for
now.

However, looking at CheckboxField.render(), I see

def render(self, data):
checked_html = ''
if data or (data is '' and self.checked_by_default):
checked_html = ' checked="checked"'
return '<input type="checkbox" id="%s" class="v%s" name="%s"%s />' % \
(self.get_id(), self.__class__.__name__,
self.field_name, checked_html)

So the "checked" status should be being set by "checked_html" being set
(either because we are rendering a True piece of data, or because it
should be checked by default). It looks like it should work to me.

You might want to investigate what "data" value is being passed to
render() here if you think there's a problem.

Regards,
Malcolm

Malcolm

[Malcolm Tredinnick]

On Thu, 2006-07-06 at 19:09 +1000, Malcolm Tredinnick wrote:
> On Thu, 2006-07-06 at 09:01 +0000, Le Roux wrote:
> > I'm useing a CheckboxSelectMultipleField in a custom manipulator. (this
> > really needs documenting, by the way)
> >
> > First off, the checkbox fields never get given the attribute
> > value="on". This is checked in prepare() to populate new_data. This is
> > easy to fix by just adding it into the render() method.

[...]

>
> However, looking at CheckboxField.render(), I see
>
> def render(self, data):
> checked_html = ''
> if data or (data is '' and self.checked_by_default):
> checked_html = ' checked="checked"'
> return '<input type="checkbox" id="%s" class="v%s" name="%s"%s />' % \
> (self.get_id(), self.__class__.__name__,
> self.field_name, checked_html)
>
> So the "checked" status should be being set by "checked_html" being set
> (either because we are rendering a True piece of data, or because it
> should be checked by default). It looks like it should work to me.

And then I re-read your message more carefully and you are correct: we
are only setting the "checked" attribute and not the (also required)
"value" attribute. It's a bug (although it may have flow-on effects,
too: are we prepared to handle "value" in the html2python code when the
form is submitted, for example?).

Regards,
Malcolm

[Le Roux]

I'm referring to CheckboxSelectMultipleField and your code snippet is
for CheckboxField...

I'm going to do an svn up just incase and think about it a bit more..
maybe I'm missing something. As far as I can tell the rendering works
fine. The value is just completely blank "on the other side" after
selecting things.

If this is all work in progress, I might just use some other field in
the meantime.

[Le Roux]

aah sorry.. my reply was to your first reply ;) I'll re-read everything
and then think again ;)

[Le Roux]

Looks like I fixed it now. Here's a patch:

django_src/django/forms/__init__.py:

--- __init__.py (revision 3278)
+++ __init__.py (working copy)
@@ -53,6 +53,7 @@

def get_validation_errors(self, new_data):
"Returns dictionary mapping field_names to error-message
lists"
+ self.prepare(new_data)
errors = {}
for field in self.fields:
errors.update(field.get_validation_errors(new_data))
@@ -636,7 +637,7 @@
if str(value) in str_data_list:
checked_html = ' checked="checked"'
field_name = '%s%s' % (self.field_name, value)
- output.append('<li><input type="checkbox" id="%s"
class="v%s" name="%s"%s /> <label for="%s">%s</label></li>' % \
+ output.append('<li><input type="checkbox" id="%s"
class="v%s" name="%s"%s value="on" /> <label for="%s">%s</label></li>'
% \
(self.get_id() + value , self.__class__.__name__,
field_name, checked_html,
self.get_id() + value, choice))
output.append('</ul>')

---- end patch ----

in my edit view, I have the following code to set the data:

category_ids = []
for category in newsitem.categories.all():
category_ids.append(category.id)
new_data['category_ids'] = category_ids

in my manipulator the field snippet is:
forms.CheckboxSelectMultipleField(field_name='category_ids',
choices=self.category_choices, ul_class='checkbox_list')

(choices is list of tuples that look like (category_id, name))

in my manipulator's save method I do:
category_ids = new_data.getlist('category_ids')
if category_ids:
categories =
returnValue.site.category_set.get_for_model(NewsItem).filter(id__in=category_ids)
newsitems.categories = list(categories)
else:
newsitems.categories = []

I hope that makes sense....

[Le Roux]

oops. that manipulator.save() bit is a bit buggy (I changed it after I
pasted to give returnValue a more meaningful name and then didn't
change all the occurrances) but it works here ;)

[Le Roux]

http://code.djangoproject.com/ticket/2300
http://code.djangoproject.com/attachment/ticket/2300/django_forms__init__.diff

[Malcolm Tredinnick]

On Thu, 2006-07-06 at 10:31 +0000, Le Roux wrote:
> Looks like I fixed it now. Here's a patch:
>
> django_src/django/forms/__init__.py:
>
> --- __init__.py (revision 3278)
> +++ __init__.py (working copy)
> @@ -53,6 +53,7 @@
>
> def get_validation_errors(self, new_data):
> "Returns dictionary mapping field_names to error-message
> lists"
> + self.prepare(new_data)

No. The prepare() is not used anywhere yet. That's not an oversight.

> errors = {}
> for field in self.fields:
> errors.update(field.get_validation_errors(new_data))
> @@ -636,7 +637,7 @@
> if str(value) in str_data_list:
> checked_html = ' checked="checked"'
> field_name = '%s%s' % (self.field_name, value)

> - output.append('<li><input type="checkbox" id="%s"
> class="v%s" name="%s"%s /> <label for="%s">%s</label></li>' % \
> + output.append('<li><input type="checkbox" id="%s"
> class="v%s" name="%s"%s value="on" /> <label for="%s">%s</label></li>'
> % \

This changes sets the value to "on" unconditionally. Is that the right
value, I wonder? Probably doesn't matter much, providing you get
html2python() right (and if the validator inherited from
SelectMultipleField still works properly). [Just thinking out loud
here.]

> (self.get_id() + value , self.__class__.__name__,
> field_name, checked_html,
> self.get_id() + value, choice))
> output.append('</ul>')
>

It seems you might also need to create an html2python() method, since
the one inherited from SelectMultipleField is probably not completey
correct now. At a guess, if a checkbox is select, we should return True,
otherwise False. And html2python() is what is called to convert the
string data we receive from a form back into the appropriate Python
object.

Might be worth creating a ticket if you get this all working smoothly so
that we dont' lose the patch.

Regards,
Malcolm

[Le Roux Bodenstein]

Thanks. I don't know what to put inside html2python. The prepare
method actually sets the list to the values (the first value in the
tuple for choices).

See.. render sets the name of the checkbox to fieldname+value. The
value of the checkbox field is just On or not there.

look at prepare:

def prepare(self, new_data):
# new_data has "split" this field into several fields, so flatten it
# back into a single list.
data_list = []
for value, readable_value in self.choices:
if new_data.get('%s%s' % (self.field_name, value), '') == 'on':
data_list.append(value)
new_data.setlist(self.field_name, data_list)

(I didn't actually write that - someone else already did)

Here's the rendered html snippet (it might help)

<div>
<label>Categories:</label>
<ul class="checkbox_list">
<li><input type="checkbox" id="id_category_ids1"
class="vCheckboxSelectMultipleField" name="category_ids1" value="on"
/> <label for="id_category_ids1">Design</label></li>
<li><input type="checkbox" id="id_category_ids3"
class="vCheckboxSelectMultipleField" name="category_ids3"
checked="checked" value="on" /> <label
for="id_category_ids3">Development</label></li>
</ul>
</div>

the field name is category_ids, but the checkboxes are named
category_ids1 and category_ids3.

I suppose in html2python I can convert each of the values in the list
to integers, but the field really doesn't know enough about the data
to know what the original type of the field was. (I had to make sure
value and text were both strings in my manipulator's __init__ method
like this:

self.categories = categories
self.category_choices = []
for category in self.categories:
self.category_choices.append((str(category.id), category.name))

Suggestions and insight are welcome...

[Jorge Gajon]

Hi Le Roux,

> the field name is category_ids, but the checkboxes are named
> category_ids1 and category_ids3.

That is to be expected. Imagine you have two
CheckboxSelectMultipleFields in your manipulator, one named
'color_selection' and other named 'music_selection', which represent
foreign keys to Color and Music models.

Of course there would be a Color with id 1 and a Music with also an
id of 1. That's why the check boxes need to be uniquely identified by
prepending the model name:

<input type="checkbox" name="color_selection1" />
<input type="checkbox" name="color_selection2" />
... etc....
<input type="checkbox" name="music_selection1" />
<input type="checkbox" name="music_selection2" />

Otherwise the names would clash. That's why there is the prepare()
method, to bring back the selected values.

By calling prepare() on get_validation_errors() as you have shown in
your patch should make this transparent :)

>
> to know what the original type of the field was. (I had to make sure
> value and text were both strings in my manipulator's __init__ method

I guess this is how it is supposed to work, the value must be a
string. You can only output strings in html, there's not distinction
between a string and a int or something else, everything is a string
in html. The values for <input/> tags (of any kind) should be strings.

The __init__ method for the SelectField and RadioSelectField could be
changed to apply str() on each of the values of the passed in choices
tuples. But maybe that could be a little 'magical'.

> - output.append('<li><input type="checkbox" id="%s"
> class="v%s" name="%s"%s /> <label for="%s">%s</label></li>' % \
> + output.append('<li><input type="checkbox" id="%s"
> class="v%s" name="%s"%s value="on" /> <label for="%s">%s</label></li>'
> % \

That's a good catch, I have been using check boxes and didn't noticed
that they didn't have a value='on'. I guess firefox is supplying the
value 'on' by default, or maybe it is being set somewhere in the
Django machinery but I don't see where. But yeah, value="on" should be
there on the output.

Cheers!
Jorge

[Jorge Gajon]

>
> Thanks. I don't know what to put inside html2python. The prepare
> method actually sets the list to the values (the first value in the
> tuple for choices).

I don't think there's anything to change in html2python. As you said
the prepare only puts the selected check boxes in the self.data list.
Only those check boxes that were selected will be POSTed with the
value='on'.