Proposal to fix longstanding name vs label problems for case roles

Also @kainuk - hmm wrong handle for Klaus?

added ~242 git:civicrm-core ~88 triaged labels

WIP https://github.com/demeritcowboy/civicrm-core/commit/c835aabe5122c4b06630b8ef86e4e602bc5b318f

There's nothing close to usable here yet. It's mostly just a reconnoiter. I don't get to use that word too often and wanted to type it.

changed title from Propsal to fix longstanding name vs label problems for case roles to Proposal to fix longstanding name vs label problems for case roles

Something to mull over as an alternate approach: https://github.com/colemanw/webform_civicrm/pull/237/files

i.e. don't actually try to fix the source but whenever a comparison is needed compare against "name OR label". There's probably some pros/cons which I'm too tired to think about right now.

FYI @pradeep https://civicrm.stackexchange.com/questions/31493/assigning-case-roles-via-webform

This isn't a bad approach, so long as it's looking for name first, then label only as a fallback. The only possible hangup is in the following situation:

Act 1: before any resolution to this issue

Someone sets up a case role called "Grandparent of" and hits Save before realizing that they should fill in the B-to-A label.
They go back and fix it, giving the relationship type the B-to-A label "Grandchild of". However, the name_b_a "Grandparent of" remains the same.
They store a new case type with the role "Grandchild of", which stores the label "Grandparent of" in the XML. (The convention being that the XML describes roles as relationship types from the non-client's perspective.)

Act 2: new version that looks for names first, then labels

This case type XML has listed "Grandparent of" in the XML. When evaluating the XML, CiviCRM first checks for relationship types where name_a_b or name_b_a match that.
The particular lines of code may be inconsistent in how they resolve this, but it may sometimes appear that "Grandparent of" is a symmetrical relationship. Sometimes that will display as "Grandchild of" and sometimes "Grandparent of", since the labels differ despite corresponding to identical names.

There are some places where a symmetrical relationship is defined operationally as the labels being equal, while at least one other place determines this using the names.

Thanks @andrewhunt good point. I have some more thoughts on the overall topic just want to organize them.

What I keep coming back to is that since name vs label has kept getting borked repeatedly over 10 years, and not just for case roles, the question is "WHY does this keep happening?" and the followup "What can be done to remove those causes and exterminate the problem?" or if not exterminate it, limit it.

Let's list some causes first, without thinking about the followup just yet. There are others but I'm only listing three because nobody likes lists that have more than three things.

In the beginning, there was only name. Except it meant label. Partly because the word "name" in non-tech speak is "a type of label". It's partly a consequence of the order in which they came to exist, but invites mixups.
When label was introduced there were existing functions that were being used to retrieve 'name', and it became a common practice to re-use them and just change the parameter 'name' to 'label'. With hindsight this leads to further confusing the two because you only see one at a time, especially if you've copy/pasted from somewhere that only uses one.
Name and Label are always initialized to the same thing, and so name is also a human-like string (because of its history in 1 above). This means that if there are problems they rarely come up during basic testing, and unit tests that might hit it are likely indirect and so end up also just doing a basic test where name and label are the same.

Ok now followup:

1 is really difficult to change now, but maybe worth a couple minutes thought. ..... Ok thought about it.

2 could be partly addressed by only letting people use a function that returns both name and label. Then it's clearer that there are two and you have to consciously pick the right one. Such a function already exists via the api if wrapped with a little wrapper. This would be doable going forward, and updating existing usage would also be doable. Deprecation warnings are doable.
Thinking out loud for the moment it would be neat if there was a tool (phpcs?) rule that noticed you're doing something iffy and then said something like "Hey, I see you've used 'label' here. Did you really mean 'name'?" Might be a difficult rule.

3 is doable by using something more computery for 'name', but at the moment that would break case roles, which is what started all this review. It wouldn't be necessary to update the existing names in an install, and existing misuse in xml files would still be dealt with by e.g. a new xml tag as described earlier.

If this comes across as scope-creep, my thinking is that for something that keeps coming up repeatedly for 10 years it's not inappropriate to think about it for a month or so.

What might be actual scope-creep is revisiting what the original purpose was (having business logic specific to certain dropdown choices) and whether it could be done differently (separate from any use for multilingual, or not). So I'll stop now.

Just for completeness, I wanted to note that we had a little one-year experiment in addressing the third point between PRs 12097 and 14216.

From my perspective, in a world where this case role label issue was resolved, it really shouldn't matter what the name is or how similar it is to the label. It's also a good attitude to have, since between people renaming (or, I suppose, re-labeling things) and localization, we really shouldn't be counting on labels for anything.

Just to play devil's advocate, there was a recent PR 14940 that suggests that for things like import, the user making the import file is rarely going to use (or even know) name and it's more likely they'll use label. I suppose that import is now subject to a similar edge case you described earlier, where if you relabelled something so that that label now matches a different option value's name, it would actually import it as the second one.

I do though believe name and label should be different and non-similar, if for nothing else than it makes problems visually obvious during testing. I think what I'm trying to get at is that the problems with name and label is a human problem, so I'm looking for ways to reduce that human factor.

mentioned in issue #329 (closed)

mentioned in commit 2d283a26

mentioned in commit 90b5313b

mentioned in commit 149ab585

mentioned in issue #1234 (closed)

Just an update on some progress. This patch is very very very (very) messy and I need change up some things, break it down, take some of the unneeded WIP out, and still have some things to test out, but so far in my testing it's making me hopeful. And I still want to tackle some of the above.

https://github.com/civicrm/civicrm-core/compare/master...demeritcowboy:say-my-name

(Or in patch form: https://github.com/civicrm/civicrm-core/compare/master...demeritcowboy:say-my-name.patch)

For systems using xml files it also needs this and I still have a little to do on it: https://github.com/civicrm/civicrm-core/compare/master...demeritcowboy:case-role-machineName

https://github.com/civicrm/civicrm-core/pull/15349

For reference, this table shows the CURRENT (not desired) objects that exist within the case type edit screens and how their member variables are related and how they map to the "real" entity they are based on.

(I didn't realize you could do tables in gitXXX until I saw @andrewhunt do it the other day. From now on every post of mine will be a table.)

Equivalent field from apiCalls.relTypes (=database table civicrm_relationship_type)

Object	label_a_b	label_b_a	id	Where used
relationshipTypeOptions (default direction)	id	text		roles form
relationshipTypeOptions (other direction¹)	text	id		roles form
caseRoles² (default direction)	displaylabel	name		roles form
caseRoles (other direction)	name	displaylabel		roles form
defaultRelationshipTypeOptions³ (default direction)		label	value (a_b)⁴	timeline activity assignee
defaultRelationshipTypeOptions (other direction)	label		value (b_a)	timeline activity assignee

Footnotes:

If unidirectional, i.e. for Spouse it is the same in both directions, so only the default direction is in the list, but for Case Coordinator both directions are in the list.
scope.caseType.definition.caseRoles, i.e. initially set to the result of an api call for CaseType.get. The displaylabel member is set shortly after.
caseRoles.name is a (little bit) fixed as the member variable name because it comes from xml which might be in external files.
This has a similar name to the above relationshipTypeOptions object but the word default here refers to when an activity on a case gets created later during actual case operations and a default assignee gets set, not default as in "the default for the above relationshipTypeOptions object".
The value member is the string combo of civicrm_relationship_type.id and direction, e.g. "2_a_b".

mentioned in commit fb3f5705

As promised here's another post that has a table. The new PR https://github.com/civicrm/civicrm-core/pull/15483 updates the table to look like so. This is better because:

The name things all match up to names.
The id things match up to id (mostly).
A redundant set of objects has been removed.

Also the PR also fixes the bugs.

Equivalent field from apiCalls.relTypes (=database table civicrm_relationship_type)

Object	name_a_b	label_a_b	name_b_a	label_b_a	id	Where used
relationshipTypeOptions (default direction)	xmlName			text³	id (a_b)⁴	roles form/ timeline activity assignee
relationshipTypeOptions (other direction¹)		text	xmlName		id (b_a)	roles form/ timeline activity assignee
caseRoles² (default direction)	name			displayLabel		roles form
caseRoles (other direction)		displayLabel	name			roles form

Footnotes:

If unidirectional, i.e. for Spouse it is the same in both directions, so only the default direction is in the list, but for Case Coordinator both directions are in the list.
scope.caseType.definition.caseRoles, i.e. initially set to the result of an api call for CaseType.get. The displayLabel member is set shortly after.
caseRoles.name is a (little bit) fixed as the member variable name because it comes from xml which might be in external files.
The use of text is a little bit mandated by the select2 dropdowns, which expect id and text.
The id member is the string combo of civicrm_relationship_type.id and direction, e.g. "2_a_b".

Since the system will still function as-is after the patch, I've ignored an upgrade script, but I can think of two scenarios where an upgrade script might prevent some future problems.

Example: You have something like this BEFORE the upgrade, where you created a new relationship type, fixed a spelling, and then added the role to a case type:

name_b_a	label_b_a	xml
Balana	Banana	`<name>Banana</name>`

So this doesn't require any action after the upgrade to continue to function, but two future actions might cause problems:

(a) You change the label again, so that it now reads Bananas, so that the xml now doesn't match any existing relationship type. The status check will catch this, but not until the next time you see the status check. If the upgrade script were to change Banana to Balana in the xml, then this would be avoided.

OR

(b) Visiting the case type edit screen to make some other changes, you'll see a blank row in the roles table. Again the upgrade script would avoid this.

The downside to a script is if you have a different scenario where you have one of the edge cases like two labels the same for different relationship types, the upgrade script might get it wrong and prevent you ever seeing the status check. But it could skip those and present a post-upgrade message. This is a pretty rare edge case to begin with.

Regarding people using xml files, I'd suggest there's a limit to the expectations/goals. Their current xml files will function as-is after upgrade, but then if they change a config item they need to change their xml file. And because it was broken before, that was what they had to do before anyway so it's not really a new expectation, but a different change to make. The patch itself doesn't break their existing setup.

mentioned in commit 86128f7a

Proposal to fix longstanding name vs label problems for case roles

Overview

Proposal

Child items ...

Activity