dev/core#530 Make a_b relationships available as case roles

[alifrumin]

Overview

This change makes it so users can select b_a relationships AND a_b relationships when creating case roles, before this change only b_a relationships were available when creating case roles.

for more information see: https://lab.civicrm.org/dev/core/issues/530

Before

Before this change users could only create case roles for CiviCRM Cases that were one direction ( b -> a). See screen shot below that shows the "New Case Type" Ui with the options for "add role" open, only the label_b_a's are shown as one can see by checking against the Relationship Type table next to it.

There were several issues with case roles labels that originated from some confusion regarding relationship directions (originally documented in #9975) including:

1. when creating a case type, you can only select B-to-A labels

then after creating a case you see the B-to-A "Parent of" label

after picking the contact, the label becomes the A-to-B "Child of"

2. Similarly, when viewing a case, the Roles drop-down, only shows the labels in the B to A direction, but when one assigns the case role in the B to A direction the label displayed is in the A to B direction.
3. When editing a Case Activity in the "Send a copy" the role is correct but the label is wrong.
4. Currently, all relationships are showing the B contact, regardless of who the client is
   yet they all have the B-to-A label, regardless of who the client is for example:

In this example Rebekah is the client, and the case has her as the parent of Kathleen and the child of Carylon. In the send a copy box, she's shown as parent of Carylon and her relationship w/ Kathleen is displayed as her being parent of herself.

5. when reassigning a case role that is B-to-A (where the client is on the B side of the relationship), the list of available contacts is the contact type of the B side since households and organizations are frequently on the B side of a relationship, this makes it difficult-to-impossible to manage a case where the client is a household or organization

After

With this change, when creating a case type users can choose which direction of a relationship type to use, they are no longer required to use b-> a, a<-b relationships are also available.

Additionally, this change fixes resolves the bugs described in the Before section by properly displaying the labels as they are defined in the case type.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[jitendrapurohit]

@alifrumin The test failures look related - https://test.civicrm.org/job/CiviCRM-Core-PR/25405/

[alifrumin]

Thanks @jitendrapurohit, I will look into these

[demeritcowboy]

Willing to review this. One little note so far: If you use the Print Report button on Manage Case you get duplicate rows in the Roles section. I don't see this on dmaster.demo.civicrm.org.

[alifrumin]

Thanks @demeritcowboy I will wait until you have finished your review and then debug the Print Report button on Manage Case.

[demeritcowboy]

Overall it seems to do as described and you've obviously put a lot of work into it!

• General standards
  • ([r-explain] PASS Great explanation.
  • ([r-user]ISSUE: People might be confused as to which direction they SHOULD use. I expect for the first while people who are used to not having a choice will sometimes pick the wrong direction unthinkingly when using the Add Role button. But I don't think anything here needs to change just maybe communicated somehow.
  • ([r-doc]PASS
  • ([r-run]ISSUE:
    • On case dashboard, case counts are wrong for both My Cases and All Cases. I don't see it on dmaster.demo, and I was also able to reproduce the problem on a local install with the patch applied. There's a warning notice about Search::formRule() but I don't think that warning is related to this PR and is a separate problem.
    • Case summary report gives this warning, and I don't see the warning on dmaster.demo.
      Notice: Undefined index: label_b_a in CRM_Report_Form_Case_Summary->where() (line 311 of /home/jenkins/bknix-dfl/build/core-13916-8q6bw/sites/all/modules/civicrm/CRM/Report/Form/Case/Summary.php).
    • Print Report link on Manage Case gives duplicate output in the Roles section.
• Developer standards
  • ([r-tech]COMMENTS: Not sure what to say about the removal of REL_TYPE_CNAME from CRM_Case_XMLProcessor. Two thoughts:
    • By removing it, and especially by removing the comment, it further entrenches the misuse of label vs name which is scattered a bit through civicase.
    • It's not just being removed, but replaced with label_a_b instead of its previous value of label_b_a. It makes me wonder what glitches this might introduce.
    • Having said that, REL_TYPE_CNAME is broken currently so does it really matter?
  • ([r-code]PASS
  • ([r-maint]Undecided I admit I am struggling to understand any of the karma tests for CiviCase. I see a configuration array was added, but it doesn't look like a new test.
  • ([r-test]PASS The currently listed conflicts appear to be recent formatting changes made to many files, not real conflicts.

I should file these below as separate issues since they don't seem related to the PR and I see them on dmaster.demo, but wanted to record them here for the moment in case you or someone else comes across them while testing and might think they are related.

• On case dashboard: Strict warning: Declaration of CRM_Case_Form_Search::formRule() should be compatible with CRM_Core_Form_Search::formRule($fields, $files, $form) in require_once() (line 37 of /home/jenkins/bknix-dfl/build/core-13916-8q6bw/sites/all/modules/civicrm/CRM/Case/Form/Search.php).
• Deleting a case role doesn't add the Remove Case Role activity to the case activities.
• Case Summary report gives duplicate results when there's more than one role assigned on a case.

[ltaliano]

I tested it and it fixes the bugs described in #9975 and also the those described on the pr it self and the related git lab issue: https://lab.civicrm.org/dev/core/issues/530.

[alifrumin]

Thank you @demeritcowboy, I will take a look at these notes.

[alifrumin]

I think I fixed the merge conflicts and the issues @demeritcowboy pointed out under r-run but Jenkins seems to be having difficulty when I go to check the test build I see:

[alifrumin]

Jenkins retest this please

[alifrumin]

@mlutfy, have you seen this Jenkins error before: https://test.civicrm.org/job/CiviCRM-Core-PR/25692/checkstyleResult/error/ any suggestions welcome.

[seamuslee001]

@alifrumin you need to rebase this as the CI pipeline cannot apply it cleanly ontop of master

[alifrumin]

Thank you @seamuslee001 I will try that.

[demeritcowboy]

@alifrumin I had a ton of trouble the other day and git rebase just kept giving me conflicts. This article kept me from going homicidal: https://stackoverflow.com/a/6103022/8332458

[demeritcowboy]

@alifrumin Sorry I know how much work this is. I'm noticing that with the current default roles defined for case types they are backwards after this PR, so maybe the default config would also need to be changed? For example if you create a new case with the defaults, one of the preassigned roles is "Benefits Specialist". So now when you assign this role to someone, you incorrectly get it assigned as the Client who is the Benefits Specialist of the other person. So the default would need to be changed to "Benefits Specialist is".

Even if you don't manually assign anything, the case manager role itself is backwards right after creation. The client ends up being the Homeless Services Coordinator of the logged in user. So again that default config would need to be changed.

And then this brings up an upgrade question for people who already have set configs.

I'm not trying to block this - honest! I think bidirectional would be a good improvement if it can be done.

[alifrumin]

Hey @demeritcowboy, I appreciate your feedback. that's an interesting point. I will noodle on this.

[agh1]

@demeritcowboy so @alifrumin and I spent a lot of time thinking through this question about default roles. Basically, the situation absent this PR is that relationship types are displayed willy-nilly from different perspectives, and the only thing maintaining any order is the occasionally-enforced assumption that the client is always on the A side of a case role relationship.

In dropping that assumption and straightening out the display of case roles, the convention has been that case roles should be described in the UI from the perspective of the client. The problem that you noticed is that the default XML-defined case types describe the roles from the perspective of the non-client. In further testing, it's clear that the same goes for case types that are created in the UI under the current situation.

Consequently, we have to face that the case types have the reverse convention that case roles are described in the case type definition from the perspective of the non-client. It really isn't feasible to go around and change everyone's case types during upgrade, especially since some are in XML files.

Instead, we've just acquiesced to the fact that a case type is really saying "there's a role with relationship type 123 where the client is on the B side". In the UI for a case, we say the client is "student of" the teacher; in the XML case type definition, for legacy reasons, we say the case type has a "teacher of" the client.

The most recent changes Alice committed have made it such that the A-B label in the XML corresponds to the B-A label everywhere else. The translation is done as closely to saving or retrieving the XML as possible.

[demeritcowboy]

Well you two are persistent for sure. Unless you have more to add I think the next step would be to update those 2 style fixes to see if any actual tests fail?

[agh1]

Test failures are both good signs. The first is this assertion:

civicrm-core/tests/phpunit/CRM/Case/BAO/CaseTypeForkTest.php

Line 42 in a1e9759

$this->assertEquals($relTypeID . '_b_a', $xmlProcessor->getCaseManagerRoleId('ForkableCaseType'));

CRM_Case_XMLProcessor_Process::getCaseManagerRoleId is behaving normally and should show the relationship from the client's perspective. Switch _b_a to _a_b.

The second failure is looking at a list of case role options:

civicrm-core/tests/karma/unit/crmCaseTypeSpec.js

Lines 353 to 363 in a1e9759

	result.push({
	label: relType.label_b_a,
	value: relType.id + '_b_a'
	});
	if (!isBidirectionalRelationship) {
	result.push({
	label: relType.label_a_b,
	value: relType.id + '_a_b'
	});
	}

The relationship type options are intentionally swapped. Update the directions to match those here:

civicrm-core/ang/crmCaseType.js

Lines 288 to 298 in a1e9759

	result.push({
	label: relType.label_b_a,
	value: relType.id + '_a_b'
	});
	if (!isBidirectionalRelationship) {
	result.push({
	label: relType.label_a_b,
	value: relType.id + '_b_a'
	});
	}

[alifrumin]

Ah, @demeritcowboy I am able to trigger the code by:

1. creating a Case Type "Apples" with a couple roles including "Apple Corer for".
2. going to the relationship type page and deleting the relationship type "Apple Corer for"
3. Editing the Case type "Apples" at this point the Apple Corer Role line is blank
   
4. Saving the Case Type -- this will trigger this code to be called and a new relationship to be created to match the xml, you can see the new relationship type by editing the case type again (see "apple Corer is is" role in the screenshot below), and in the Relationship Types list.
   

[demeritcowboy]

@alifrumin Ok thanks I see. So for these odd situations it seems unlikely anybody is relying on it to be Individual, and you can just define the relationship type first or change it after.

For reference I was also able to trigger it by "converting" an xml file to managed, i.e. the xml file is for a case type that isn't defined yet in the db, and it has a role that isn't defined in the db as a relationship type. Then

1. Add the case type in the admin screens with the same name as the file and just press save.
2. Now choose revert from the More dropdown.
3. Now go in to edit the case type. The row for the new role will be blank, but just press save.
4. Now go in to edit the case type again. Everything is fine and the relationship type now exists too.

I would expect an api call to CaseType.create with a similar xml definition would also trigger this, which again seems fine.

So I am done!

[eileenmcnaughton]

@colemanw @mattwire @jitendrapurohit @lcdservices this seems to have had a lot of thought & review & appears to be considered mergeable by @demeritcowboy after careful thought. Is someone able to take a final overview on this

[demeritcowboy]

@alifrumin Do you want to edit the title or the overview to indicate parts of it require #14216 ?

[alifrumin]

@demeritcowboy, I do not think anything in the pr requires #14216, it makes things cleaner and is a nice to have not a need to have AND its been merged.

[eileenmcnaughton]

I merged that PR - but as I noted on it

dug a little & still think this was done on a 'this seems like better practice' rather than a real reason so I'm happy to merge this reversal.

• if dev/core#530 Make a_b relationships available as case roles #13916 relies on labels always matching names that could be a problem since a) the data model doesn't require it & b) any types saved 'recently' will be different

[alifrumin]

@eileenmcnaughton has anyone expressed interest in doing a second review or is @demeritcowboy review sufficient?

[eileenmcnaughton]

@mattwire @colemanw is one of you able to evaluate this - it has had a lot of review but I'm not familiar with case code

[colemanw]

This is a good & valuable change but is tricky to review because of the complexity and the number of workflows affected. I'll need to carve out some time to test it properly. It also needs rebasing.

[colemanw]

The "labels instead of names" thing was a design mistake that's haunted us for way too long. Every time a piece of CiviCase gets refactored this problem gets punted as "out of scope" for the issue at hand. I understand that, but jeeze this is a frustrating problem!

[agh1]

At least now there's an explicit ticket for it, even though I'm not a fan of the solution proposed in #14001.

[demeritcowboy]

@agh1 @colemanw That PR wasn't intended as a proposed solution for name vs. label - its intention seems to be to just allow changing a relationship label in the UI which happens to be blocked because of name vs label. In https://lab.civicrm.org/dev/core/issues/774#note_14566 I had started on a WIP but put it on hold since it is a bit of work and has a blocker or two.

[alifrumin]

@colemanw Thank you!!

[alifrumin]

Rebased @colemanw

[mlutfy]

I did a few tests on a blank/demo site. Works as expected. I various variants of case roles / direction.

I also tested the newer civicase extension:

• if the relationships were added before enabling the extension, it does not display the relationships (none at all, there is a JS error)
• if the relationships are created normally, after enabling the extension, it's OK.

So I would say that this is not a blocker, since it affects people who 1) are using this patch and created relationships in the other direction, and 2) migrate from CiviCase-classic to CiviCase-v5.

I will open an issue on the civicase-v5 github repo.

[mlutfy]

Thank you @alifrumin, @demeritcowboy, @agh1, @colemanw !