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

$this->_selectedTables is not populated incase of boleen filters #14503

Merged
merged 1 commit into from
Jul 2, 2019

Conversation

pradpnayak
Copy link
Contributor

@pradpnayak pradpnayak commented Jun 12, 2019

Overview

When you filter on report for boolean fields with option No then $this->_selectedTables is not populated.

Before

When you filter on report for boolean fields with option No then $this->_selectedTables is not populated.

After

When you filter on report for boolean fields with option No then $this->_selectedTables is populated.

Technical Details

Boolean filters use 1 or 0 values and !empty on 0 will return false. Hence used CRM_Utils_System::isNull() to check if its null or empty

@civibot
Copy link

civibot bot commented Jun 12, 2019

(Standard links)

@civibot civibot bot added the master label Jun 12, 2019
@@ -4300,7 +4300,8 @@ public function selectedTables() {
}
if (array_key_exists('filters', $table)) {
foreach ($table['filters'] as $filterName => $filter) {
if (!empty($this->_params["{$filterName}_value"])
if ((isset($this->_params["{$filterName}_value"])
&& !CRM_Utils_System::isNull($this->_params["{$filterName}_value"]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pradpnayak or :

if (CRM_Utils_Rule::boolean($this->_params["{$filterName}_value"])) {
 ...
}

?

Copy link
Contributor Author

@pradpnayak pradpnayak Jun 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @monishdeb
Correct me if am wrong, CRM_Utils_Rule::boolean() checks for 0,1,Yes,No,True, False values but in my case the value can be any. I guess isNull would be better option.

Thoughts?

Pradeep

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm right. Well, I have no issue with this change.

Happy to merge 👍

@eileenmcnaughton
Copy link
Contributor

@pradpnayak @monishdeb I agree with this change - obviously there is a minor discussion around code style to resolve but the change makes sense

@monishdeb
Copy link
Member

I agree with this final patch and on quick run on local it safe to merge. Hence merging this PR

@monishdeb monishdeb merged commit b280082 into civicrm:master Jul 2, 2019
@pradpnayak pradpnayak deleted the ReportFix branch March 23, 2020 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants