Changing a civicase activity's label breaks the max_instances check

The variable activityTypeName is actually label. Of course.

Oh and look it sets it to label twice in the same function, in two completely independent ways:

https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/Form/Activity.php#L328-L330

https://github.com/civicrm/civicrm-core/blob/master/CRM/Activity/Form/Activity.php#L408

If I've traced this right I'm surprised this doesn't cause non-case problems too??

Thanks for reporting are you able to provide a PR?

Yes I'm just thinking over the approach. I also wanted to look if this creates permission problems for noncase activities too if it's checking against label instead of name.

mentioned in issue #1123 (closed)

My plan is to leave $this->_activityTypeName alone and just document that it is really label, and then in the places where it really should be name use a new variable $activityTypeInternalLookupName. A quick look through the code suggests that in many places activityTypeName is being used as a label (correctly), and so that's why I'm thinking to leave it alone despite the misnomer (or mislabler ?). Also, since activityTypeName is a form member and something that people might be referencing in form hooks, it makes me more hesitant to touch it.

You are right with your hesitation. @eileen do you have any thoughts on this?

I guess that makes sense - it's very long though - $this->activityType would be fairly common too & the doc block could clarify

Thanks - it is long, but I wonder if the docblock would then be confusing if adding activityType:

/**
 * $activityTypeName string
 *   This is actually label.
 * $activityType string
 *   This is name.
 */

Another idea is to add one called activityType but have it be an array, like so:

/**
 * $activityTypeName string
 *   This is actually label.
 * $activityType array
 *   ['name'] string The name
 *   ['label'] string The label
 */

@DaveD another option is to add a new function rather than a new variable - ie $this->getActivityTypeName();

Yes that makes sense to abstract it. Although I still have to call it something for use in the smarty template...hmm.

Something else to consider when making this change.

The name (not label) is being used in the Case Type Activity List, instead of the label. Clients have complained when they change the label for the activity type, it isn't reflected properly in case activity type list. The AJAX shows the different values coming out, but the interface is using the name value and not the label.

{"id":"2522","option_group_id":"2","label":"Family Capacity Building (Individual/Family Contact)","value":"78","name":"Individual/family session",`

I understand the name needs to stay the same for stability/compatibility/hooks/XLM files... However the label should be used consistently across the GUI, as it is confusing to the end user. The user really doesn't care what the name is, as long as the interface shows the label correctly.

mentioned in commit a33c01da

mentioned in commit 393e1801

mentioned in commit b85bb090

mentioned in commit 79eec22a

mentioned in commit 25318b0c

mentioned in commit 605c4d78

mentioned in issue #1470 (closed)

FYI - I got bitten by this bug as well (in a non-case environtment): when viewing an activity, the template adds line breaks if it's an inbound email. However, if the user changes the label for inbound email, it fails.

See stack exchange and also the PR that uses the activityTypeName and expects it to be a name, not a label.

This is still on my todo list, just haven't completely got back to it yet. Thanks for the nudge.

@jamie https://github.com/civicrm/civicrm-core/pull/16347

If you're able to test out the patch that would be great.

This looks good to met @DaveD - I wasn't able to test with data from the group that was affected (still on 5.13 and the initial commit - f692bf33 - has a load of conflicts). But the code looks solid and is quite simple and has a test.

I don't quite understand the jenkins error, but it doesn't seem to be related to your code or test.

Thanks @jamie. I think the test fail is related in that the added test is affecting some global form state that carries over between tests. I'll have to take a look.

mentioned in commit 9edd639e

mentioned in commit bfbbe0d9

Hi,

Thank you for raising an issue to help improve CiviCRM. As you may know, this issue has not had any activity for quite some time, so we have closed it.

We would like you to help us to determine if this issue should be re-opened:

If this issue was reporting a bug, can you attempt to reproduce it on a latest version of CiviCRM?
If this issue was proposing a new feature, can you verify if the feature proposal is still relevant? Did it get the concept-approved label? Have other people also shown interest? Could it be implemented as an extension?

If the answer to either question is yes, please feel free to comment or re-open the issue. Please also consider:

Is it something that you could help implement, either by sending a patch or hiring someone who can?
Would you be willing to fund the work, either through a CiviCRM partner, or the Core Team?

Thank you for your help and contributions to CiviCRM.

P.S. This is an automated message, see infra/gitlab issue 20. We understand that automatic responses are annoying, but given the number of open issues as the project evolves, we need a bit of help to triage and prioritise the most relevant issues.

closed

reopened

Hi,