CRM-21391 Refactor form search to use a base class, resolve issues with action … #11240
Conversation
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
2 times, most recently
from
1b46bb0 to
edb5dcd
Compare
|
@mattwire this is a difficult one to review/test. Can you give some more background on why these changes are needed, how to reproduce at least one of the bugs this fixes, and some suggested workflows to test with this PR. |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
edb5dcd to
a4f50cc
Compare
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
2 times, most recently
from
10519dc to
b773723
Compare
|
I think this is tricky to review because there are a lot of changes in it - on the other hand they seem to make sense. With things like this I normally split them up into a series of stand alone changes - e.g just fix up the constants, or do the minimum possible to institute the base class for the tasks in one PR & then next do the cleanup this facilitates |
|
Doesn't even need to be different PRs. One PR with a series of commits would help to break things down. |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
b773723 to
6726536
Compare
|
@colemanw @eileenmcnaughton Been requested to squash commits down in the past but perhaps I went too far with this one. I'll break it down into a few commits and re-push. |
mattwire
changed the title
|
@mattwire so my rule with commits is that if a change is complete in itself it gets it's own commit - e.g a function extraction. Where I ask people to squash is when they make subsequent fixes to an earlier commit - so the earlier commit is not correct without the later one. |
colemanw
added
the
needs-work-not-review-ready
|
Note for myself: Does this fix "Send Email" action missing from "Find Contacts"/"Find Memberships"? |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
2 times, most recently
from
6f2ab7d to
b8d20c9
Compare
|
Jenkins test this please |
|
@mattwire this is a pretty big PR to review - if you pulled out the first 2 commits into a separate PR I could review that - which I think would reduce the code involved quite a bit |
|
@eileenmcnaughton Ok breaking it down... |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
424befc to
f4fdff4
Compare
|
OK - I merged the original one - let's get this rebased & I'll see if we can get this merged. I have my head into the changes you are making now & agree it's an improvement |
|
Hmm - some of these changes will require more grepping than others - perhaps there is another easy PR to pull out to get a quick merge on & reduce bulk? |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
f4fdff4 to
87a2bdf
Compare
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
87a2bdf to
002c046
Compare
mattwire
changed the title
|
@eileenmcnaughton I've broken out the search class changes into another PR #11600. This PR now contains only changes to convert the remaining task classes to use CRM_Core_Task and changes in each class should be near identical. |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
2 times, most recently
from
634f8f0 to
ca93a01
Compare
CRM/Campaign/Task.php
Outdated
| @@ -143,8 +120,8 @@ public static function &permissionedTaskTitles($permission) { | |||
| public static function getTask($value) { | |||
| self::tasks(); | |||
| if (!$value || !CRM_Utils_Array::value($value, self::$_tasks)) { | |||
| // make the interview task by default | |||
| $value = 1; | |||
| // make the print task by default | |||
There was a problem hiding this comment.
@mattwire this comment is incorrect - otherwise changes in this commit look good.
Note the comment wouldn't be needed if the function were called 'getDefaultTask
There was a problem hiding this comment.
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
2 times, most recently
from
975e7e1 to
baf339a
Compare
|
@eileenmcnaughton Thanks for going through all of this... Do you want me to cherry-pick each commit into it's own PR like you've done for contribute/activity? |
|
@mattwire I've picked off contribution & activity but yeah - I'm kinda picking them off to be more manageable since this is quite a big PR. |
mattwire
force-pushed
the
CRM-21391_form_task_cleanup
branch
from
baf339a to
a0e367a
Compare
|
@eileenmcnaughton Just opened separate PRs for the remaining 7 task classes |
|
@mattwire these are almost all merged & I'm about to try mailing. I just spotted a regression caused by these - I have not logged an issue at this stage but am adding a screenshot to illustrate it. Note that the PR for this may need to be against the rc depending when it is raised vs the rc being cut
|
|
@eileenmcnaughton "Find Respondents To Reserve" is now fixed in #11808 |
|
I'm going to close this master task now. |
…list when changing the entity filter
Overview
Refactor all the component tasks so they are extend a new base class CRM_Core_Task.
Before
There were a number of issues with the "Advanced Search" when switching between component types where the task list would be populated with the wrong list of tasks, but the keys would trigger an action on the selected component and an unexpected action may occur.
Also there seemed to be a bug with saving group_type where mailing_list was always being checked.
After
This cleans up the code and uses shared code where possible.
Technical Details
Introduces a new base class CRM_Core_Task.
Comments
Further refactoring would be possible, also some tasks actually work in all contexts (eg. create/update smart group) and we could extend the lists to share these elements too.