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

dev/core#1410 Fix E-notice when doin a force case search with a prede… #15920

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

seamuslee001
Copy link
Contributor

…fined case subject field

Overview

This fixes an issue on the case search form when using a url like civicrm/case/search?reset=1&force=1&case_subject=test

Before

Notice message Warning: htmlspecialchars() expects parameter 1 to be string, array given in HTML_Common->_getAttrString() showed

After

No Notice and search completes correctly

ping @eileenmcnaughton @demeritcowboy

@civibot
Copy link

civibot bot commented Nov 21, 2019

(Standard links)

@civibot civibot bot added the master label Nov 21, 2019
*
* @see valid_date
*/
public function addRules() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was code left over after the conversion to date picker

$query->_qill[$grouping][] = CRM_Contact_BAO_Query::getQillValue('CRM_Case_DAO_Case', $name, $value, $op, 'Case Subject');
$query->_tables['civicrm_case'] = $query->_whereTables['civicrm_case'] = 1;
$query->_tables['civicrm_case_contact'] = $query->_whereTables['civicrm_case_contact'] = 1;
$query->handleWhereFromMetadata($fieldSpec, $name, $value, $op);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured made sense to try to standardise the thing even tho this wasn't strictly speaking the issue

@eileenmcnaughton
Copy link
Contributor

test this please

@eileenmcnaughton
Copy link
Contributor

Code looks good - @demeritcowboy do you want to test ?

@demeritcowboy
Copy link
Contributor

Sure, just I get an error about patch doesn't apply because of whitespace.

@seamuslee001 seamuslee001 changed the base branch from master to 5.20 November 21, 2019 21:26
@civibot civibot bot added 5.20 and removed master labels Nov 21, 2019
@seamuslee001
Copy link
Contributor Author

@demeritcowboy it should be against 5.20 rather than master just FYI

@demeritcowboy
Copy link
Contributor

I wonder if a better solution is just git rm CRM/Case/Form/Search.php...

Doing both a url search case_subject=test&force=1 and a form-based search with subject gives:
Error: Call to undefined method CRM_Contact_BAO_Query::handleWhereFromMetadata() in CRM_Case_BAO_Query::whereClauseSingle() (line 336 of .../CRM/Case/BAO/Query.php).

I'm also noticing something else that must have been introduced between 5.20 and master - I'll make a separate ticket.

@seamuslee001
Copy link
Contributor Author

@demeritcowboy maybe but there is still some special handling that is in that file so i don't think we can remove it. Given your error i'm thinking maybe i need to put this against master instead or change things

@eileenmcnaughton @demeritcowboy should this be a 5.21 or 5.20 thing i can go either way

@eileenmcnaughton
Copy link
Contributor

If this is about url support it's not a regression it's a new feature (out of NYSS interest) I think.

I'm all for rm -rf civicrm//Search/

@demeritcowboy
Copy link
Contributor

Yeah I was just joking (well, sort of). I forgot to put the smiley face.

@seamuslee001 seamuslee001 changed the base branch from 5.20 to master November 22, 2019 00:12
@civibot civibot bot added master and removed 5.20 labels Nov 22, 2019
@seamuslee001
Copy link
Contributor Author

ok @eileenmcnaughton @demeritcowboy i have put this back to master now i think its easier that way

@demeritcowboy
Copy link
Contributor

That seems good. Thanks.

@seamuslee001
Copy link
Contributor Author

@demeritcowboy are you going to do another r-run?

@demeritcowboy
Copy link
Contributor

Sorry, I wasn't clear: I did an r-run and it's good for subject. There are other things which are now in a separate lab ticket which aren't related to this PR.

@seamuslee001
Copy link
Contributor Author

@demeritcowboy just opened another PR #15924 to resolve your other issue

@demeritcowboy
Copy link
Contributor

Thanks!

@eileenmcnaughton
Copy link
Contributor

Looks like this has been tested & code looks good - MOP

@seamuslee001 seamuslee001 merged commit 7618d57 into civicrm:master Nov 22, 2019
@seamuslee001 seamuslee001 deleted the dev_core_1410 branch November 22, 2019 01:44
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.

3 participants