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

CRM-19610 : Search preferences changes are not updated #10491

Merged
merged 3 commits into from
Jun 12, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jun 11, 2017

@seamuslee001
Copy link
Contributor Author

@jitendrapurohit i have made some changes to your PR and rebased it, I think the tests will pass now, Can you check to see if it makes sense to you?

@@ -352,7 +352,7 @@ protected function setDb($name, $value) {
}
$dao->find(TRUE);

if (isset($metadata['on_change'])) {
if (isset($metadata['on_change']) && !($value === 0 && ($dao->value === NULL || unserialize($dao->value) === 0))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @seamuslee001. It passes the test now but the main issue seems to be appearing after this change. As $value is supposed to be of type mixed in this function, === will always fail for '0' if passed as a string.

@seamuslee001
Copy link
Contributor Author

@jitendrapurohit i have altered the check to allow for possibility of '0' and locally tests are still passing, I note that your most recent change still fails the tests. I'm just not 100% sure if my logic is correct and don't know why mine would pass but yours wouldn't

Copy link
Contributor

@jitendrapurohit jitendrapurohit left a comment

Choose a reason for hiding this comment

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

Me neither, but i can confirm that this change fixes the original issue. So, i think this could be merged as it also passes the test build.

@seamuslee001
Copy link
Contributor Author

ping @eileenmcnaughton @totten

@eileenmcnaughton
Copy link
Contributor

This seems fine to me - note there is an open critical issue that would be probably fixed by this too. I think @nganivet logged it some time back?

@eileenmcnaughton eileenmcnaughton merged commit 91e4eaf into civicrm:master Jun 12, 2017
@seamuslee001 seamuslee001 deleted the CRM-19610 branch June 12, 2017 07:54
@eileenmcnaughton
Copy link
Contributor

Ah - linked JIRA issue is wrong?

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton nope correct i believe its the same as Jitendra's and #10453 should be merged as well

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.

4 participants