-
-
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#107: Add default assignee when creating cases #11998
dev/core#107: Add default assignee when creating cases #11998
Conversation
Can one of the admins verify this patch? |
1ecf8e3
to
299cf39
Compare
@reneolivo Idea looks good. Are you able to squash the commits down from 26 into functional changes? And is there any code that can be pulled into a separate PR to be reviewed/merged first as there is quite a lot here! |
Hello @mattwire squashing is possible of course. Splitting the PR can be a bit tricky though since we could merge an incomplete feature. In the original PR (as seen from the comments) we split the PRs like this:
We could try to split it that way too, but if only the first one gets approved then no default assignee would be chosen and we'd end up with an incomplete feature. What do you suggest? |
If it does not make sense to separate then that's fine. Squashing a bit should help with review. I just wondered if there was anything that could be picked out, eg bugfixes that are useful even without the feature. |
a9871a3
to
3bedb10
Compare
@mattwire I have sorted and squashed the commits into logical changes. There were one or two that fix something done in previous commits, but I had to leave them since they fix something that was done over several commit, otherwise I'd have to squash big chunks of changes and it would be harder to review. Please let me know if this is more manageable. |
$this->assertActivityAssignedToContactExists($this->assigneeContactId); | ||
} | ||
|
||
public function testCreateActivityWithDefaultContactByRelationButTheresNoRelationship() { |
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.
Can we rename this to testCreateActivityWithDefaultContactByRelationButThereIsNoRelationship
?
@reneolivo , I think this change would need the user documentation update: https://docs.civicrm.org/user/en/latest/case-management/set-up/ |
cea8b81
to
5b1a2f4
Compare
I'm +1 on this conceptually. I think it's a good feature for CiviCase and doesn't really affect anyone who doesn't need it. |
ang/crmCaseType/timelineTable.html
Outdated
@@ -55,12 +56,41 @@ | |||
<td> | |||
<select | |||
ui-jq="select2" | |||
ui-options="{dropdownAutoWidth : true}" | |||
ui-options="{ dropdownAutoWidth: true }" |
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 would suggest to leave these as it was. Because civicrm follows no space convention.
* | ||
* @param string $rev | ||
*/ | ||
public function upgrade_5_3_alpha1($rev) { |
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.
Should not it be 5_2_alpha1?
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.
It should be 5.3.alpha1 because that's the version in master.
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 just needed to move it to another file.
5885a1a
to
aa65268
Compare
@reneolivo please click the "details" button under the test failure indicator to see the code style issues that need fixing. |
@civicrm-builder retest this please |
1 similar comment
@civicrm-builder retest this please |
@colemanw can you please check again? I think everything is in order now. |
@reneolivo I'm testing this out, but merging this would be a lot nicer if the PR had one commit instead of 16. Could you rebase+squash it please? |
I tested this and it mostly works, but I found problems with the "By relationship" option.
Expect: 1 followup activity will be assigned to the friend, the other will be assigned to no-one. |
9a15b84
to
994084b
Compare
6d693d0
to
4f54a18
Compare
Hello @colemanw. I have rebased the changes with master and moved the upgrader to |
4f54a18
to
84f3a24
Compare
@civicrm-builder retest this please |
@colemanw we would ideally merge this before rc is cut... |
I'd like to merge this but there is one more merge conflict showing up in the upgrade file. @reneolivo are you able to fix this? If not I'll get to it when I can. |
This feature allows users to define default assignees for each activity in a case type. When creating a new case contacts are assigned to activities automatically by following one of these rules: * By relationship to case client * The user creating the case * A specific contact * None (default)
84f3a24
to
68098e7
Compare
@colemanw @eileenmcnaughton fixed the conflicts. |
Included in 5.4.0 Core PR: civicrm#11998
Included in 5.4.0 Core PR: civicrm#11998
Included in 5.4.0 Core PR: civicrm#11998
Included in 5.4.0 Core PR: civicrm#11998
This PR appears to be causing a fatal error on upgrade https://lab.civicrm.org/dev/core/issues/304 |
and still a problem on dmaster - imo either this should not be REQUIRED - or the documentation or some alert needs to show to explain to someone why they cannot Save a Case Type |
@reneolivo, I agree with @petednz -- it probably shouldn't be required (or, if it is required, then the UI needs to explain better.) A couple examples of why it shouldn't be required:
Aside: I haven't dug in fully yet, but there's a confounding issue -- there's an option-group for assignee policies, but it's only created on upgraded sites... and not on new/clean sites. (See screenshot above.) This could be preventing us from seeing the real/expected behavior. |
Apologies for the length of my last comment -- it was a bit misdirected under the assumption that the behavior was intended to work that way. After reading/testing more, I can confirm the problem only affects new builds and relates specifically to the option-group. #12842 is the fix for 5.5(stable). |
Hello @petednz and @totten the field is not required per se. It is only required when the user sets the default assignee by relationship. This is important because the backend expects the field to have a value assigned when the the default assignee is set by relationship. Also, it's important to note that for this case, the required field is a dropdown of relationship types, not assignees. If the workflow needs to be amended even after #12842 please let me know and I can change it. |
Overview
This PR allows admins to configure default assignees for cases. The default assignees can be selected as follows:
Before
Configuring the default assignee
Creating a new case
No contact assigned to activities.
After
Configuring the default assignee
Creating a new case
Technical Details
Inserting the default assignee options in the database
A new upgrader was added targeting version 5.0.1, a hypothetical version that branches off from 5.0.0, which is a new version in development. The upgrader reads as follows:
Displaying the default assignee options
The default assignee options are requested in the
apiCalls
for theCaseTypeCtrl
controller:The fields are displayed in the form by editing the timelineTable.html as this:
Default assignee for new cases
For new cases the default assignee is set to NONE:
Selecting the assignee
The default assignee is selected when creating the activity at the
CRM_Case_XMLProcessor_Process::createActivity
function since this one has most of the information we need to select the assignee:getDefaultAssigneeByRelationship
is the strategy for selecting an assignee by the relationship to the target contact:getDefaultAssigneeBySpecificContact
is the strategy when choosing a specific contact. It returns the ID only if the contact exists in case the contact were to be deleted at a later time:Comments
crmCaseType
controller.CRM_Case_XMLProcessor_Process
class was missing a test file so a new was created at:tests/phpunit/CRM/Case/XMLProcessor/ProcessTest.php
, but only tests related to the default assignee selections were added.CRM_Core_BAO_OptionValue::ensureOptionValueExists
had to be enhanced to return the option value id whether it finds the value or creates a new one.Gitlab Issue:
https://lab.civicrm.org/dev/core/issues/107