Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

HashNotAdam
Copy link

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
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
Member

Test needed!

@HashNotAdam
Copy link
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
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
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
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