Update Query.php #8238
Update Query.php #8238
Conversation
Let's sort it in alphabet order (sort_name asc) by default, for now it sorts by ID each time more than on search criteria is being selected (like part of the name and relationship type, etc.)
|
Can one of the admins verify this patch? |
|
jenkins, ok to test |
|
This function is called from LOTS of places so it's kinda a scary change to make. You probably want to start by logging a JIRA & fleshing out your reasoning & where you are experiencing a bad sort that this would fix https://issues.civicrm.org/jira/ |
|
NB - the jenkins warnings are because we follow drupal whitespace coding standards & jenkins tests against them with phpcs |
|
Jenkins, retest this please |
|
@yurg I'm going through a number of open PRs that may be considered for 4.7.8, and to be considered, this will need the tests to be passing. It looks like you've got some spaces at the end of a couple of lines (see the test results). If you can commit your fixes in the next couple of days and the tests pass, we'll be able to consider it for merging this month. |
|
Not sure I did it right, have used browser edit mode; besides Jenkins has pointed to a different line numbers; but at least I've tried. |
|
jenkins test this please |
|
test this please |
|
Hi @yurg as Release Manager this month, I'm trying to recruit people to help pare down the backlog of almost 100 PRs, some going back to last summer. I'm wondering if you would be able to help QA another PR if I got someone to QA this PR? |
|
Hi @JoeMurray , not really sure what exactly should I do but I'm ready to help. |
| $order = $orderBy = $limit = ''; | ||
| // let's sort it in alphabet order by default, for now it sorts by ID each time more than on search criteria is being selected | ||
| // (like part of the name and relationship type, etc.) | ||
| $order = " order by contact_a.sort_name asc"; |
There was a problem hiding this comment.
@yurg for consistency, I think order by should be uppercased.
|
Based on the above comments I'm closing this PR until we have a jira ticket with rationale, and unit tests. |
|
Agreed re: 1) strong need for unit tests on well-used function, and 2) need for JIRA issue explaining intent. |
|
Can one of the admins verify this patch? |
|
@yurg just guessing what is going here, but you are wanting to change the sort order in some part of CiviCRM so that contacts default to alphabetic. I'd suggest you discuss in dev channel on chat.civicrm.org or perhaps post a question to civicrm.stackexchange.com regarding what you are trying to achieve, and ask about the best way to do it. There is likely some code that is more specific to your interest that could be modified, or it might be that some of the Search Preferences under Admin menu may be enough, depending on what you want. |
|
@JoeMurray Exactly; alphabet sorting by default is more "human-friendly" than default sort by Contact ID; not sure why, but "Search preferences" tokens don't work, whatever combination of {title} {first_name}{last_name} I've tried. |
|
I wasn't thinking of tokens but the options under Admin > Custom Data and Screens > Search Preferences, Include Order By Clause and Include Alphabetical Pager |
Let's sort it in alphabet order (sort_name asc) by default, for now it sorts by ID each time more than one search criteria is being selected
(like part of the name and relationship type, etc.)