-
-
Notifications
You must be signed in to change notification settings - Fork 824
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#71 Add functionality to create PDF/Word docs from Activity searches #12012
Conversation
@totten - Would you mind taking at look at this as it uses the new token processor? FYI @guanhuan @mattwire @monishdeb re https://chat.civicrm.org/civicrm/pl/7ih15ybjd7yzm8184yc4b1jmxo |
@aydun Is there any work that could be done here to create abstract core tasks for PDF / PDFLetterCommon as I did when adding the new "Batch Update" functionality for cases in #11411 as that would help in the future to improve maintainability and possibly offer a route to migrating all PDF tasks across to the new token processor at some point in the future. |
return $html; | ||
} | ||
|
||
if (!empty($html)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this section could be shared with another class rather than duplication? - perhaps from getRows down could be renderHtmlFromRows or similar?
@eileenmcnaughton , @mattwire I've moved the rendering part to a new function in a new base class. CRM_Contact_Form_Task_PDFLetterCommon has functioned as a base class for all the *_PDFLetterCommon so I've changed that to use the new class. We can move truly common stuff to the new class and leave the Contact one with contact-specific code. |
Here's few suggestions after code review:
diff --git a/CRM/Activity/Form/Task/PDF.php b/CRM/Activity/Form/Task/PDF.php
new file mode 100644
index 00000000000..f1c4c5dd68a
--- /dev/null
+++ b/CRM/Activity/Form/Task/PDF.php
@@ -0,0 +1,79 @@
...
public function buildQuickForm() {
- CRM_Activity_Form_Task_PDFLetterCommon::buildQuickForm($this);
+ CRM_Contact_Form_Task_PDFLetterCommon::buildQuickForm($this);
} And it will no longer need us
public function listTokens($entity, $className) {
$tokens = self::createTokenProcessor()->listTokens($className);
if ($entity == 'contact') {
$tokens += CRM_Core_SelectValues::contactTokens();
}
...
return $tokens;
} and move to
|
Hmm - on first blush I think I like the use of inheritance more than I do moving things to a utils class. But I'm also pretty on the fence. Overall I think this looks like a good patch. Let's see @aydun thoughts... |
Thanks for your comments @monishdeb and @Eileen My thinking behind this structure is noted at https://github.com/aydun/civicrm-core/blob/51ed01419a9d4edb82d6d9ff9fdb9373bdc6376e/CRM/Core/Form/Task/PDFLetterCommon.php#L35-L40 - which is the 'alternative' at the end of @monishdeb's comments. I would see that a later move would refactor the common parts into It would be good to get some thoughts from @totten about his plan for migrating to the new token processing to ensure this patch is aligned with that thinking. In particular, https://github.com/aydun/civicrm-core/blob/51ed01419a9d4edb82d6d9ff9fdb9373bdc6376e/CRM/Activity/Tokens.php#L72-L73 seems like it could be improved. |
@monishdeb where are you at in your review - is this 'passable but with possible improvements that might be agreed in further discussions, but if that doesn't happen in the short-term we should merge it'? - if so then the merge-ready flag would indicate that - or is it still pending more testing? |
@eileenmcnaughton @aydun these are the specific changes I want to see in this PR and in my opinion I don't think we can treat this in a separate PR and/or issue (after we merge it) as because we are introducing some new files here and the following changes is appropriate to do it in this PR itself:
From the feature point of view, it working perfectly 👍 So if tagging PR with |
@monishdeb nope - if there are required changes it's not ready for merge-ready. Note that using traits are an option now if it fits |
The PR makes a few changes to the class hierarchy. That's reasonable, but (as a reader) I needed a way to keep track. It helped me to have a summary of the classes/functions at play, so I added that to the description. Most of the code in What's weird is that we call Overall, here are some of the responsibilities of
My reaction to reading that was: "I don't get it; this function does too many different things and lacks a unifying concept with clear pre-conditions/post-conditions." That's a little unfair -- I think there is a core concept, i.e. it's a "Template Selector/Editor Widget" (a mid-level UI building-block akin to a "Billing Block", "Address Block", "EntityRef", "Richtext Editor", or "Hierarchical Multiselect"), but... the design/implementation of the concept has... quirks.
Of course, I don't think anyone has commented on this... because really... it's not a functional problem. The approach here looks workable -- i.e. it might spend an extra 0.002s on unnecessary parsing, but that doesn't matter. There are a couple unused variables -- but you don't have to bend-over-backward with convoluted mapping logic. The oddity is visually small and doesn't pose any difficulty for the reader. And the oddity is pre-existing. In an ideal world, we'd probably refactor (I haven't formed an opinion on whether it's faithful application of |
@totten thanks for all your comments here. As you note One part that feels suspect is https://github.com/civicrm/civicrm-core/pull/12012/files#diff-bcf4e4fdbc2537981f5768c8b35b4164 |
@monishdeb can you recheck this now changes have been made to reflect input |
@monishdeb looks like this is just needing a final review from you |
Jenkins re test this please |
243487f
to
b1e240b
Compare
Lots of changes here... @monishdeb I've changed the class hierarchy so both @totten I refactored This includes the I've also changed |
CRM/Activity/Tokens.php
Outdated
} | ||
|
||
$activities = civicrm_api3('Activity', 'get', array( | ||
'id' => array('IN' => $activityIds), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might fail if processing a batch of 25+ messages. Probably needs 'options' => ['limit' => 0]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also -- the loop above (populating $activityIds
) makes perfect sense, but let me put it under a little extra scrutiny because this patch may become an example to inform similar patches to CRM/*/Tokens.php
. It'll probably be fairly common to extract a bunch of activityId
values (or contributionId
or caseId
or whatever), and that multiplies out to several 5 SLOC bits of boilerplate. Instead, I'd vote to skip the variable $activityIds
and have a helper function like:
$activities = civicrm_api3('Activity', 'get', array(
'id' => array('IN' => $e->getTokenProcessor()->getContextValues('activityId')),
'options' => ['limit' => 0]
);
(Aside: I've tried a few variations of CRM_Utils_Array::collect()
and array_map()
, but the magic bits made that more complicated. Adding getContextValues()
seemed easier.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I've added getContextValues()
. I still have $activityIds
since it is used later in ActivityContact.get
CRM/Activity/Tokens.php
Outdated
} | ||
else { | ||
$activity = (object) $prefetch['activity'][$row->context['activityId']]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely certain where to to put this comment; maybe this works...
There's a sort of elegance in saying that $activity
is the same thing whether it comes from the context['actionSearchResult']
or from $prefetch
. Intuitively, these both provide a copy of civicrm_activity.*
, so that feels well and good.
The problem is that they're not really interchangeable. For example:
actionSearchResult
includesactivity_type
(the label from the OV); however,$prefetch
comes from a defaultActivity.get
API call, so it doesn't.actionSearchResult
includescase_id
; however,$prefetch
comes from a defaultActivity.get
API call, so it doesn't.$prefetch
includes information about related contacts (given theactivityId
); however,prefetch()
short-circuits in the presence ofactionSearchResult
. So the contacts are consistently prefetched.- There's an expectation in our community that the schema for
Activity.get
API and forcivicrm_activity
are allowed to drift apart.actionSearchResult
implicitly tracks one while$prefetch
implicitly tracks the other.
IMHO, it would get confusing (over time) to maintain with two different functions for loading the data (alterActionScheduleQuery()
and prefetch()
).
Of course, alterActionScheduleQuery()
is in there for a reason -- reducing the number of queries by tapping into the main query. We don't want to issue a new SQL query for every combination of token+recipient. Batch-loading is better.
But I've got this nagging feeling that it would be better to significantly trim down alterActionScheduleQuery()
:
- Change: For
alterActionScheduleQuery()
, we don't need a blanketSELECT e.*
or any option-group joins. - Change: For
prefetch()
, get the list of activity IDs from a mix of (a)context['activityId']
and/or (b)context['actionSearchResult']->entity_id
(provided the mapping type is right). - Observe: It always loads activity data the same way.
- Observe:
prefetch()
only runs once for a given batch, so we're still batched-loading. - Observe: When running the scheduled-reminders, the main query can be a bit thinner (easier to read/debug).
- Observe: If we optimize
prefetch()
a bit more (only grab columns that are actually needed), then that optimization applies equally well to both use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your thoughtful comments.
I haven't touched alterActionScheduleQuery()
or looked at that process in much detail as it was pre-existing and only used for scheduled reminders. The concept of a base SQL query that is passed around for various subscribers to modify for their own needs seems clever, and probably performant, but easy to mess up.
As you point out the schemas for Activity.get
API and civicrm_activity
may diverge so tracking both could get confusing. Which one do you prefer for these purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aydun I think that in terms of Activity.get API vs civicrm_activity - I would probably lean towards the latter. It's one of our less clean apis with all the weird somewhat legacy handling of assignee & target. If we were there with api v4 I'd probably go the other way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton Thanks for the steer - I'll go that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton @totten I set off down the path of following civicrm_activity
rather than Activity.get
. Then I got to the comments on CRM_BAO_Activity::retrieve()
noting it should be deprecated and to use the API instead. if we go direct to the table or via the BAO we hit the same issue of bypassing ACL's, so I've stuck with the API, at least for now.
However, by explicitly specifying the return fields, the available tokens are the ones specified in __construct()
rather than whatever the API happens to return.
I've made the changes Tim suggested to trim down alterActionScheduleQuery()
and move the data gathering to prefetch()
so it is consistent between scheduled reminders and other uses.
Hmm any clues what those test failures are re |
@aydun this has gotten pretty big - if you can see any smaller extractions of code moves that are non functional it might be worth breaking off a couple of smaller prepatory PRs & we can get those merged |
Hi @eileenmcnaughton I've extracted 3 new PR's. If we can get those merged I can rebase the rest. @totten - you wanted to give This introduces eg |
FYI - this is now working on a production site. |
@eileenmcnaughton As you suggested, I have extracted the new base class into a separate PR. #13892 |
Rebased and ready for review |
@aydun I pulled out a few more changes - #13967 - we are doing some big formatting changes which would have made this bit go stale so I figured I could make the impact on this PR more positive by getting an extraction merged than by just forcing staleness I didn't do the bit about setting properties - TBH I wasn't clear that it was saving additional queries so I left it out for now |
@eileenmcnaughton Many thanks for extracting out those changes. I've rebased again to take account of that. I agree the properties don't save much for basic or custom tokens but it seems worthwhile for the special tokens, so for consistency I have kept that. |
Error looks unrelated. Jenkins test this please. |
(CiviCRM Review Template WORD-1.2)
|
@demeritcowboy you can get out of me talking you into a unit test today by testing this if you would like :-) It has had code review so just needs r-run |
jenkins test this please |
@aydun looks like we moved the style goalposts - @demeritcowboy you can assume for now it's passing since it did pass before |
@eileenmcnaughton I really just wanted to generate a test site. I can test locally just wanted to avoid any windows weirdness. |
Create CRM_Actvity_Form_Task_PDFLetterCommon extending CRM_Core_Form_Task_PDFLetterCommon CRM_Activity_Tokens: add source, target and assignee tokens Eg: {activity.target_N_display_name} - N can be substituted for a number to show the details of the nth activity target contact eg: {activity.target_1_display_name} is the display name of the first target For ease of use, contacts are numbered from 1. If the literal N is used or is replaced by 0, it is treated as 1. Slim down alterActionScheduleQuery() and move most data fetching to prefetch() Add tests for Activity PDF Letter
e2515e4
to
2dfc15f
Compare
|
@aydun I'm keen to get this merged - if there are any parts that are mentioned above that can be left out & put to a follow up I'd go for that |
I have opened #14662 in attempt to get everything except the part under discussion (activity contact tokens) merged #14662 Oh I fixed After selecting an activity and choosing the action from the actions dropdown I get Warning: count(): Parameter must be an array or an object that implements Countable in CRM_Core_Form_Task_PDFLetterCommon::buildQuickForm() (line 150 of blah\blah\CRM\Core\Form\Task\PDFLetterCommon.php). |
"Tokens for custom fields for the source don't seem to work" - they are excluded from my cut down version (punted) |
On thee 3rd r-run issue - I tested PDF. When I tried to test MS Word it came back with File Not Found. I don't think it's my windows laptop because MS Word works for the contact search print/merge function.
I think this means I have removed all the things that are not working properly from the version I've put up |
jenkins, test this please |
See https://lab.civicrm.org/mattwire/emailapi/tree/newtokenprocessor for a useful way to try it out |
Overview
Add a Print/Merge Document action to Activity searches
Before
Currently most other searches provide a Print/Merge action - except for Activities.
After
This adds a Print/Merge action to Activity search results
Technical Details
Activity tokens are defined but not evaluated in the legacy token system. They are functional in the new token system so this uses the new Civi\Token\TokenProcessor
This revises or updates classes to use the following class hierarchy:
CRM_Core_Form_Task_PDFLetterCommon
renderFromRows($rows, $msgPart, $formValues)
CRM_Contact_Form_Task_PDFLetterCommon
getLoggingOptions()
,,preProcess(&$form)
preProcessSingle(&$form, $cid)
,buildQuickForm(&$form)
,setDefaultValues()
,formRule(...)
,processMessageTemplate($formValues)
,postProcess(&$form)
,createActivities(...)
,formatMessage(...)
,getMimeType()
,getTokenCategories()
CRM_Activity_Form_Task_PDFLetterCommon
postProcess(&$form)
,postProcessActivities(&$form, $activityIds)
,createTokenProcessor()
,listTokens()
processMessageTemplate()
,renderFromRows()
HTML_Common
=>HTML_QuickForm
=>HTML_QuickForm_Page
=>CRM_Core_Form
CRM_Activity_Form_Task
preProcess()
,preProcessCommon(&$form)
,setContactIDs()
,addDefaultButtons(...)
CRM_Activity_Form_Task_PDF
,preProcess()
setDefaultValues()
,buildQuickForm()
,postProcess()
,listTokens()
preProcess()
Comments
Need to add some tests, but looking for feedback on the use of TokenProcessor first