CRM-21855 - Editing "A" side of relationship copies values to "B" side

[alexander0205]

When editing the "A" side of a relationship from 'civicrm/admin/reltype' the values entered in the "Relationship A to B" column are copied to the "Relationship B to A" column. The reverse is not true.
Go to civicrm/admin/reltype

• Edit anything in the "Relationship A to B" column
• Save and refresh the page
• The "Relationship B to A" column has also been updated

Overview

This change repair the bug from CRM-21855

Before

SilentCast)

Technical

The error is when we send
         CRM.api3 (info.entity, action, params, {error: null}) and this has a_b = values and b_a = null.

[civicrm-builder]

Can one of the admins verify this patch?

[alexander0205]

The error is when we send
         CRM.api3 (info.entity, action, params, {error: null}) and this has a_b = values and b_a = null.

[eileenmcnaughton]

@davejenx is this one you would review?

[eileenmcnaughton]

Or possible @guanhuan can assign someone as this feels like code you have an interest in

[mickadoo]

@alexander0205 I'm not sure this is the right approach. When I tried to reproduce this using the API I could by following these steps:

$result = civicrm_api3('RelationshipType', 'get', array(
  'id' => 10,
));

returns

        {
            "id": "10",
            "name_a_b": "Supervised by",
            "label_a_b": "Supervised by",
            "name_b_a": "Supervisor",
            "label_b_a": "Supervisor",
            "description": "Immediate workplace supervisor",
            "contact_type_a": "Individual",
            "contact_type_b": "Individual",
            "is_reserved": "0",
            "is_active": "1"
        }

Then update the "label_a_b"

civicrm_api3('RelationshipType', 'create', array(
  'id' => 10,
  'label_a_b' => "EDITED",
));

And the RelationshipType now becomes

{
            "id": "10",
            "name_a_b": "Supervised by",
            "label_a_b": "EDITED",
            "name_b_a": "Supervisor",
            "label_b_a": "EDITED",
            "description": "Immediate workplace supervisor",
            "contact_type_a": "Individual",
            "contact_type_b": "Individual",
            "is_reserved": "0",
            "is_active": "1"
}

Instead of fixing this client-side in what is sent by jquery.crmEditable.js I think it should be fixed in the PHP code that handles these requests. That would mean we don't just fix it for this specific case using the "Relationship Type" edit form, but for anywhere relationship types are edited.

Can you also update the PR description, the GIF isn't showing because of link formatting. It would also be nice to have a GIF showing the result after your changes.

The title should also contain a reference to the ticket number. Commit messages should begin by referencing a ticket number. Have a look at other PRs (example #11887) to see what I'm talking about.

[eileenmcnaughton]

@mickadoo regarding ref to ticket number - I believe part of the move to gitlab is a loosening of that - @mlutfy can add more...

[guanhuan]

I am not sure this is correct either. From the UI, it looks like the A to B label is the required field and I believe the purpose that this is made this way is to prevent someone from creating a relationship type with no label.

If you try edit one side of label of an existing relationship type via the UI, it will not overwrite the other side of label.

I think the most correct behaviour here, according to my understanding of why the UI and API is decided the way they are, should actually be:

1. Make 'label_a_b' required by api
2. Prevent 'label_a_b' from being copied to 'label_b_a' if:
   a. 'label_b_a' param is inputted
   b. you are updating an existing relationship type (and 'label_b_a' is not empty - probably don't need this)

@eileenmcnaughton your thoughts?

[eileenmcnaughton]

@guanhuan I don't see myself as a UI specialist - I lean heavily on people like @agh1 @Stoob @petednz & @lcdservices when it comes to 'what is the correct UI behaviour'

[eileenmcnaughton]

( & on people like you too @guanhuan :-)

[eileenmcnaughton]

Acutally - looking at the screencast - it only makes sense to me to COPY the label (either way) when there is no existant label. I feel like the case to update is pretty weak - so I think @guanhuan I've just agreed with you!

[lcdservices]

I agree with @guanhuan's assessment too. However, it might be worth just making the label required in the UI for both directions (and fixing the api to require both fields). I'm not crazy about "silent copying" to fill a field that is in fact required. It seems more straightforward to me to simply communicate the fact that it's required in the UI.

[alexander0205]

Hello everyone, thank you for your comments, it is my first PR to support the CRM.
I agree with the comments of @guanhuan and I think it should be like this, but I did not want to touch the core in a robust way and the fields a_b and b_a are required and somehow you need to send the a_b and b_a side to have the correct functioning

This would be a patch to keep going.
If we create a new functionality for "civicrm_api3 ('RelationshipType', 'create')" we would have to modify the core in a robust way or make a 'CreateAloneColumn'.
I await your prompt response to solve this problem, I have taken this personal problem and I want to solve it correctly.
Thanks.

[seamuslee001]

Jenkins test this please

[eileenmcnaughton]

Regarding making the label compulsory in the UI (seems fine to me) we could also use js to prefill the second field off the first field (ie. copy from the non empty label to the empty one) to make it slightly more convenient to fill in

[mickadoo]

It could be a good idea to make both labels required on creation, but definitely not on update - I don't want to always have to provide the labels if I'm updating another field.

The API bug where label_a_b is always copied to label_b_a should be fixed regardless of what is changed in the UI.

[civicrm-builder]

Can one of the admins verify this patch?

[alexander0205]

@michaelmcandrew @civicrm-builder @guanhuan .
Change this file that contains this: if label B to A is blank, insert the value label A to B for it.
I did not want to make this change because, I do not like to play the core very much, but as the comments made me make this change.
Comment the code and I think it is a code that is too, because it has no rules to insert in case you have an empty field.
Test the software and do not generate any errors.
NEW PR FOR THIS TASK
#11925

[mickadoo]

Commenting code isn't a great idea, it leaves a lot of unused code lying around and makes files unnecessarily messy. We can recover any deleted code using git anyway.

Based on the comment I'd say you still have a little bit more to do in this PR:

• [] Ensure label_a_b and label_b_a are required for creation in both API and UI
• [] When updating an existing RelationshipType ensure the labels are not required

Also in the code you commented out below you changed something related to name_b_a. This might be outside of the scope of this PR

[eileenmcnaughton]

@mickadoo if we make the label required in the api & people are already using it, only supplying one, we could break their code :-( We should probably try to sanitise UI behaviour & maintain api behaviour

[mickadoo]

@eileenmcnaughton I'm fine with that. If it was me I would have just fixed the bug that I mentioned originally in the issue, and I'd probably just do that by changing this line:

if (!strlen(trim($strName = CRM_Utils_Array::value('label_b_a', $params)))) {
  $relationshipType->label_b_a = CRM_Utils_Array::value('label_a_b', $params);
}

to something like

$isEdit = !isset($params['id']);
$existingBALabel = NULL;
if ($isEdit) {
  $existingBALabel = CRM_Core_DAO::getFieldValue(
    CRM_Contact_DAO_RelationshipType::class,
    $params['id'],
    'label_b_a'
  );
}
$newBALabel = trim(CRM_Utils_Array::value('label_b_a', $params));

// If no new label_b_a in params and is empty in database use label_a_b as default
if (!$newBALabel && !$existingBALabel) {
  $relationshipType->label_b_a = CRM_Utils_Array::value('label_a_b', $params);
}

But I got the impression from the comments on this PR that more was required

[agh1]

I don't know what the state of this PR is, but since @eileenmcnaughton asked, I wanted to share my opinion that requiring the label B-to-A in the UI is inviting potential problems. The only way Civi has for knowing that a relationship is symmetrical is that the names are the same. For relationships created in the UI, the names are set by the first value given for the labels.

That means in a scenario where both labels are always required, it becomes really easy to make a new relationship called Friend of A-to-B and Freind of B-to-A. Worse, when a site admin notices the typo, that will just update the label, leaving name_a_b and name_b_a as the original, different values.

@eileenmcnaughton suggested a JS fix to autopopulate the B-A label from the A-B one, but I feel that it would be hard to make it autofill unless you explicitly tell it that the relationship should be symmetrical.

So that made me think, why don't we just have a checkbox in the UI called "Symmetrical"? It could do two things:

• greys out and fills the B-to-A label and contact type B fields when you check it
• sets the B-to-A label and name and the contact type B to equal their counterparts when the form is submitted

When editing an existing relationship, the checkbox would appear checked if all those values are equal. (In other words, there would be no corresponding database field for it because CiviCRM uses identical names--or label, in one case--as the way to indicate a relationship type is symmetrical.)

[colemanw]

I've created an alternate PR here: #11965

[colemanw]

@agh1 I agree with you about adding a checkbox for clarity in the form layer. It would have the added advantage of being able to de-check or re-check a box which could then make a symmetrical relationship non-symmetrical and vice-versa. The current UI is confusing in that regard.

Something for the "when I have extra time" todo list. Meanwhile the PR I just submitted solves the immediate bug.