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-21776: Ensure from clause adds custom table when search query is … #11679

Merged
merged 1 commit into from
May 20, 2018

Conversation

jitendrapurohit
Copy link
Contributor

@jitendrapurohit jitendrapurohit commented Feb 16, 2018

…ordered by a custom field

Overview

Fix DB error displayed after performing any task on advanced search results sorted by custom field

Before

Performing any task on Advanced Search returns DB error when the search results are sorted by a custom field. To replicate - See JIRA issue description.

After

DB error is fixed. All tasks are performed correctly.

Technical Details

Custom table is not included as no return property is sent from the selector to improve the performance in #7566. The function prepareOrderBy() seems to be correctly adding specific table values to $this->_tables[]. This misses the assignment for custom tables which is added in this PR.

Comments

Added unit test.


@eileenmcnaughton
Copy link
Contributor

I could replicate this bug but this didn't work for me as it this was never true when the line was hit !array_key_exists($cfID, $this->_cfIDs)

I struggle with the logic of this - it seems like these lines in the select clause

    if ($cfID) {
      // add to cfIDs array if not present
      if (!array_key_exists($cfID, $this->_cfIDs)) {
        $this->_cfIDs[$cfID] = array();
      }
    }

should be adding those tables

@jitendrapurohit
Copy link
Contributor Author

@eileenmcnaughton Hmm, I get a DB error before replicating this issue now. When I did this PR, sorting on the custom field column worked correctly and the action task used to give me the error as mentioned in the JIRA issue description.

This seems to be another recent regression maybe due to changes done here. Your latest PR #11829 should be able to avoid that and I think it needs to be applied with this PR to verify the above comment.

I struggle with the logic of this - it seems like these lines in the select clause should be adding those tables

The selectClause() lines don't add the table as NO_RETURN_PROPERTIES is sent from the selector and prepareOrderBy() is meant to add all the required tables from the orderBy fields.

@eileenmcnaughton
Copy link
Contributor

@jitendrapurohit when I last looked at this I could replicate the bug but not the fix. I just attempted now & I did not replicate the bug - although the unit test did fail without the rest of the patch. Screen cast attached

cust_search

@jitendrapurohit
Copy link
Contributor Author

@eileenmcnaughton Performing any task after the search results are sorted will lead to an error. I just tried on dmaster and it replicates there too.

The screencast looks good. What missing is the user needs to select a task after ordering it by marriage date field.

@eileenmcnaughton
Copy link
Contributor

I confirm that I was able to replicate the bug taking the additional step described by @jitendrapurohit & that this fixed that error. Merging

@eileenmcnaughton eileenmcnaughton merged commit 00b6a86 into civicrm:master May 20, 2018
@jitendrapurohit jitendrapurohit deleted the CRM-21776 branch May 21, 2018 07:21
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