Opened 5 years ago

Closed 5 weeks ago

#15497 closed New feature (wontfix)

BooleanField should work for all PostgreSQL expressions

Reported by: lsaffre Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: julie@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using ExtJS as user interface for a Django application, one problem is that ExtJS Checkboxes submit a string "on" when they are checked. Django's BooleanField.to_python understands "t", "1", "true", but not "on". Here is a patch. I'd be glad to see this merged into Django.

Attachments (2)

extjs_checkboxes.diff (887 bytes) - added by lsaffre 5 years ago.
Patch against rev. 14995
15497.diff (2.8 KB) - added by jpichon 4 years ago.

Download all attachments as: .zip

Change History (14)

Changed 5 years ago by lsaffre

Patch against rev. 14995

comment:1 Changed 5 years ago by russellm

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by mlavin

This patch is against the models.BooleanField but it seems like this problem should be dealt with at the form level. I seems like this issue could be resolved by simply using a TypedChoiceField in the form or a custom subclass of forms.BooleanField. I can't see how using a particular JS framework would necessitate a change to the Django ORM.

comment:3 Changed 4 years ago by lsaffre

In my Lino project (http://code.google.com/p/lino/) I don't use Django forms.
Lino's server-side code that handles ExtJS form submits is here:
http://code.google.com/p/lino/source/browse/lino/ui/extjs/ext_ui.py#740

Or maybe you can imagine a customized import process for csv files. Wouldn't such a process also use the to_python method?

comment:4 Changed 4 years ago by mlavin

I'm going to go ahead and change my mind on this issue. I still don't agree with your use cases. I think that custom data cleaning can either by handled at the form level or by subclassing BooleanField if you want to use model validation. The special cases in the BooleanField.to_python should be motivated by the boolean types as defined by the supported database backends. However PostgreSQL 8.4 supports the following True values:

TRUE
't'
'true'
'y'
'yes'
'on'
'1'

and False values:

FALSE
'f'
'false'
'n'
'no'
'off'
'0'

so this perhaps this should be included.

comment:5 Changed 4 years ago by lsaffre

  • Summary changed from BooleanField should work for ExtJS Checkboxes to BooleanField should work for all PostgreSQL expressions

Aha, to_python is only for parsing SQL expressions, not as a general-purpose parser.
Yes that sounds good. Maybe change the sentence

"Converts the input value into the expected Python data type"
of the docstring into

"Converts the input value (an SQL expression) into the expected Python data type".

And I change the summary of this ticket to "BooleanField should work for all PostgreSQL expressions".

comment:6 Changed 4 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

Changed 4 years ago by jpichon

comment:7 Changed 4 years ago by jpichon

  • Cc julie@… added
  • Easy pickings unset
  • Needs tests unset
  • UI/UX unset

I attached a patch that includes Isaffre's diff, makes the same change for NullBooleanField and adds a couple of tests.

comment:8 Changed 4 years ago by mtredinnick

Whilst PostgreSQL supports various literals for true and false, it returns a much more constrained set, which is what Django needs to understand (and they are typically handled long before getting to this level). Most of the slightly odd "not really boolean" values that are currently accepted in model fields are historical legacy stuff and that isn't an excuse to add yet more. Anything populating a model field can reasonably be expected to normalise it's true/false input to Python True/False values, I feel.

As noted earlier, altering model fields to handle particular form (HTTP) submission raw data is the wrong level. If you're not using Django's forms, then your own form handling code should be doing various pieces of normalisation (or writing a custom model field to do it for you).

Will leave open for a second opinion, but I'm -1 on this.

comment:9 Changed 4 years ago by lsaffre

As reporter I also meanwhile think it's better to leave BooleanField
unchanged. I'd just change the docstring of Field.to_python to make
clear that it parses only SQL statements (but for all supported db backends).

comment:10 Changed 2 years ago by Kamu

  • Needs documentation set

It seems that the conclusion was to not add any more boolean values and update Field.to_python docs to state it is used for the parsing of SQL statements. If that is the case, then this requires a document patch. Otherwise this can be resolved as wontfix.

comment:11 Changed 5 weeks ago by thedrow

I think it's safe to close this as won't fix. It's pretty obvious that this should be done at the form level or view level.

comment:12 Changed 5 weeks ago by timgraham

  • Resolution set to wontfix
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.
Back to Top