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#893 - fix misfiled case print/merge pdfs (alternate to PR 15609/15364) #15626

Merged
merged 1 commit into from
Oct 29, 2019

Conversation

demeritcowboy
Copy link
Contributor

Overview

This is the mostly the same as #15609 and the intent of #15364 but with some cleanup and a unit test.
@ray-wright @mattwire

Before

When doing a pdf document merge on case search results, if the order of the case ids as passed in from search results differed from the order that mysql naturally returns internally, the documents get filed incorrectly. See lab ticket and other PR for more detail.

After

Filed properly.

Technical Details

As discussed at #15364 (comment), this takes a step towards removing a static function with parameters and instead using the CRM_Core_Form_Task base class and overriding variables in the subclass. Also allowing for component-specific ordering where needed.

The copied function getContactIDsFromComponent() is exactly the same as the one from CRM_Core_DAO except for lines 244 and 250 where the $orderBy is used.

Comments

If you comment out the orderBy function in Case/Form/Task.php so that it functions the same as it was before without ordering and run the test, it shows how the contact ids didn't match up to case_ids.

@civibot
Copy link

civibot bot commented Oct 27, 2019

(Standard links)

@eileenmcnaughton
Copy link
Contributor

I'll let others weigh in / test but the code approach looks sensible & I don't see any red flags

@ray-wright
Copy link
Contributor

Just tested and this works - PDFs are filed on the proper case including the proper "with" contact. Thank you all for all your help working through this fix!

@demeritcowboy
Copy link
Contributor Author

Thanks @ray-wright for testing and for working out the issue/patch.

@seamuslee001
Copy link
Contributor

Merging based on the r-run of @ray-wright and @eileenmcnaughton 's code review

@seamuslee001 seamuslee001 merged commit 4aefa7b into civicrm:master Oct 29, 2019
@demeritcowboy demeritcowboy deleted the case-pdf-merge-order branch October 29, 2019 19:19
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