Skip to content

Issue #1099: Filtering on "Is present" and "Is blank" on boolean columns#2675

Merged
mshibuya merged 5 commits into
railsadminteam:masterfrom
HashNotAdam:hotfix/fix_presence_filtering_on_boolean_columns
Aug 22, 2016
Merged

Issue #1099: Filtering on "Is present" and "Is blank" on boolean columns#2675
mshibuya merged 5 commits into
railsadminteam:masterfrom
HashNotAdam:hotfix/fix_presence_filtering_on_boolean_columns

Conversation

@HashNotAdam
Copy link
Copy Markdown

Filtering a boolean field on "Is present" or "Is blank" produces SQL that
includes checking against an empty string. Since a boolean field can only be
true, false, or null, this causes errors in some databases.

Remove the empty string search for boolean field types.

@HashNotAdam HashNotAdam changed the title Hotfix/fix presence filtering on boolean columns Issue #1099: Filtering on "Is present" and "Is blank" on boolean columns Jul 27, 2016
@HashNotAdam
Copy link
Copy Markdown
Author

CI failing due to Rubocop issues addressed in PR 2679

Adam Rice added 3 commits July 29, 2016 15:52
Filtering a boolean field on "Is present" or "Is blank" produces SQL that
includes checking against an empty string. Since a boolean field can only be
true, false, or null, this causes errors in some databases.

This commit removes the empty string search for boolean field types.
@HashNotAdam HashNotAdam force-pushed the hotfix/fix_presence_filtering_on_boolean_columns branch from c658799 to 7178349 Compare July 29, 2016 05:53
@HashNotAdam HashNotAdam reopened this Jul 29, 2016
@mshibuya
Copy link
Copy Markdown
Member

Test needed!

@HashNotAdam
Copy link
Copy Markdown
Author

Come on! The original code isn't tested... how about you test the existing functionality and I'll test the micro-change in this PR and keep using my fork in the mean-time? :P

@mshibuya
Copy link
Copy Markdown
Member

The original code is tested here,
https://github.com/sferik/rails_admin/blob/master/spec/rails_admin/adapters/active_record_spec.rb#L188-L198
just adding a few examples is enough. This is important to make sure the code you wrote won't break in the future.

@HashNotAdam HashNotAdam force-pushed the hotfix/fix_presence_filtering_on_boolean_columns branch from 7178349 to 3e78509 Compare August 22, 2016 00:20
@HashNotAdam
Copy link
Copy Markdown
Author

I've updated those tests but that is both nasty and terribly smelly. The fact that changing the code didn't cause a test to break tells you it wasn't properly tested.

There is no testing for the class itself, just the output after running through a module and two classes. If you are lucky enough for a future change to break the test, there will still be some analysis to work out where the issue exists. Even having 2 classes nested inside that module with module methods and classes mixed together is horrible and it makes it harder to find what you are looking for.

@mshibuya mshibuya merged commit f919c03 into railsadminteam:master Aug 22, 2016
@mshibuya
Copy link
Copy Markdown
Member

I clearly state that horrible tests are better than no tests at all.
I know there's so many not-so-good things in the codebase, and since this is an open source software, it's everybody's job to improve them.

Thank you for your work, I really appreciate it 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants