Filtering timestamps fields by blank or present generates a query error #747

Merged
merged 1 commit into from Sep 28, 2011

Conversation

Projects
None yet
3 participants
Contributor

jbescoyez commented Sep 28, 2011

Filtering timestamps fields by blank or present generates a query error (on Postgres at least).

Postgres does not allow to compare a timestamp with "" (empty string).
This patch enables to filter timestamps with (not) null.

Filtering timestamps fields by blank or present generates a query err…
…or (on Postgres at least). They should be filtered by null.

Enable to filter null timestamps
Contributor

jbescoyez commented Sep 28, 2011

I would have loved to write a test. However I did not find where (and if) JS is tested in rails_admin.

Collaborator

bbenezech commented Sep 28, 2011

_blank and _present are pseudo-values that are aliased in main_controller when building the SQL request. I don't understand why you are modifying JS?

Contributor

jbescoyez commented Sep 28, 2011

Because _blank and _present are meaningful for let's say filtering strings but not timestamps. Actually, a timestamp field cannot be an empty string in Postgres (I do not know for other DBs). It means that every timestamp field that are #blank? are also #nil?. Hence, filtering by null would, IMHO, be more appropriate. Isn't it?

Collaborator

bbenezech commented Sep 28, 2011

Oh, yeah, sure. I didn't check the diff hard enough. _not_null and _null were already implemented, I guess.
Thanks for the patch.

bbenezech added a commit that referenced this pull request Sep 28, 2011

Merge pull request #747 from jbescoyez/fix-timestamp-presence-filter
Filtering timestamps fields by blank or present generates a query error

@bbenezech bbenezech merged commit dc8cbf0 into sferik:master Sep 28, 2011

msp commented Feb 23, 2012

Same deal on Postgres for acts_as_tree if you want to filter on parent rows i.e. those with a null, not empty, value. It falls into the belongs_to_assoc case which doesn't have those options. I verified it behaves as expected for my case but is it confusing to have blank & null options for all of these three?

    case 'string':
    case 'text':
    case 'belongs_to_association':
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment