CRM-21849: Inline Relationship Type Edit #11853

mickadoo · 2018-03-21T17:30:18Z

Overview

To make it easier to edit relationship types this PR adds an icon to show a popup for inline editing of relationship types.

Before

It was not possible to edit relationship types inline. You had to go to the civicrm/admin/reltype screen

Calls to getoptions for Relationship with field 'relationship_type_id` would result in:

{

    "is_error": 1,
    "error_message": "The field 'relationship_type_id' has no associated option list."
}

After

The relationship type dropdown has an icon to allow inline editing.

The Relationship getoptions endpoint supports relationship_type_id

Technical Details

Spec functions for getoptions were ignored if the concrete spec function was not implemented. I wanted to modify the spec, but I didn't want to override the generic getoptions function. I modified the generic function to support custom spec functions.
Relationship type comparison in the Relationship BAO used strict comparison, even though one type was an int (according to doc-block) and the other (the BAO property) was a string, so I had to cast one to an int
When refreshing the options for relationship types we can't show all types. The original list is added here and uses some parameters to filter it. In order to get the same list, I needed to pass these to the getoptions call done from crm.optionEdit.js so I introduced option_context to the elements. If it exists it will be set as a property of the element (along with the other metadata, such as data-api-entity and data-api-field, and it will be passed along to the getoptions call.
The relationshipData was previously populated in the backend using CRM_Contact_BAO_Relationship::getContactRelationshipType but this means that it stays static. To allow dynamically recreating relationshipData (after editing etc.) the generation was switched to the frontend

CRM-21849: Add Link To Edit Relationship Types

seancolsen · 2018-03-27T02:18:11Z

(CiviCRM Review Template WORD-1.1)

(r-explain) Pass with flying colors!
(r-test) Pass
(r-code) Not evaluated
(r-doc) Pass
(r-maint) Not evaluated
(r-run) ❌ Needs work
(r-user) Pass
(r-tech) Not evaluated

I ran this locally to see what I could break. I found the following issues:

Can't select contact-type-limited relationships types when editing existing relationships
1. View Relationships tab for an individual contact with a "Household Member of" relationship.
2. Click "Edit" for the "Household Member of" relationship.
3. Expect "Relationship Type" field to be set to "Household Member of".
4. Observe "Relationship Type" field is empty and it's impossible to select "Household Member of".
Too many relationship type options after saving a relationship type
1. View Relationships tab for an individual.
2. Click "Add Relationship".
3. Click the "Edit Options" icon.
4. On the "Employee of" type, click "Edit".
5. Click "Save".
6. Click to choose a relationship type.
7. Expect types to be limited to valid relationships for individuals
8. Observe option "Employer of" which is not valid for individuals.
Recently enabled relationship type missing from options
1. View Relationships tab for a contact with an existing relationship of a type which is disabled.
2. Edit the relationship.
3. Click the "Edit Options" icon.
4. For the relationship type, click "More > Enable". Then click "Done".
5. (Note: At this point it would be nice if "Relationship Type" could be populated with )
6. Click to select a Relationship Type.
7. Expect the relationship type which you just enabled to be a option.
8. Observe that the relationship type is missing from the options.

mickadoo · 2018-03-27T15:14:22Z

@seanmadsen thanks very much for the comprehensive QA

Can't select contact-type-limited relationships types when editing existing relationships

Good catch. This seems to be related to the type casting I did. Unfortunately I found that unlike what the doc block says $contactId is often a string value. I would prefer to have reliable types, but this function is used in more than 20 places just in core and I can't be sure of the type of $contactId, so I changed it to do loose comparison instead.

Too many relationship type options after saving a relationship type

I couldn't reproduce this:

In my GIF you might point out that I didn't edit the "A" side of the relationship. This is because (for me anyway) there seems to be a bug where editing the "A" side also replaces the value on the "B" side. I have created an issue for this.

Recently enabled relationship type missing from options

Another good spot. I had a look at this and found that whether the options are refreshed or not depends on if the event returns data. In this commit I added some arbitrary data which will cause the options to be refreshed. As I mentioned in the commit message it is sort of a hack to get the desired functionality, but it could be useful data for reacting to the event.

One thing to note is that if you choose a disabled relationship type the option will be unselected. Even after enabling the relationship type in the popup it'll be unselected. This is because the rebuilding of the options just goes by the existing selected option, it doesn't do anything like fetching which option should be selected from the database. Basically if the option isn't selected already it doesn't know which one to select after.

seancolsen · 2018-03-27T18:27:00Z

Nice. Looking good, @mickadoo. I havn't re-tested, but I wanted to comment on the one you weren't able to reproduce... The steps I'm taking involve clicking on the "Edit" link, at the far right of the relationship type, which opens a separate form. Can you try that?

eileenmcnaughton · 2018-03-28T10:46:37Z

@colemanw there is great review here by @seanmadsen but I think you also need to comment on the approach. Having 'contact_id' as a field for getoptions spec feels a bit odd having skimmed it - but I have not looked closely enough to have formed an opinion about it & the buildOptions structure is your brain child :-)

mickadoo · 2018-03-28T11:28:02Z

@seanmadsen Ah, I didn't realize that. I tried to reproduce and actually I managed to first, but I noticed when I cleared the browser cache the problem went away. It might be an issue where the JS files are being cached, but the HTML content is showing the gear icon

@eileenmcnaughton @colemanw A code review would be very helpful. I felt like I was going off the standard path here when I needed to make changes to how the spec functions are working, plus for other getoptions requests I don't usually see parameters to filter the available options. I just knew if I wanted to see the same options as in the original form after refreshing them I needed to pass the same criteria to the getoptions request.

seancolsen · 2018-03-28T22:08:29Z

Ah, caching! I see! Yes, I see what you mean. Ok...

I have reviewed this again after Michael's follow-up changes and can verify that he has addressed all my r-run concerns.

Here's where we're at now then:

(r-explain) Pass
(r-test) Pass
(r-code) Not evaluated
(r-doc) Pass
(r-maint) Not evaluated
(r-run) Pass
(r-user) Pass
(r-tech) Not evaluated

We still need someone to review those parts of the checklist that I skipped.

mickadoo · 2018-04-05T15:23:47Z

@colemanw if you're a bit busy maybe you can suggest someone else for a review?

eileenmcnaughton · 2018-04-05T22:34:14Z

Just as an aside I'm aware of another PR that is stalled really on needing core (& in particular Coleman to review). It's kind of similar in that there is a non-trivial amount of review effort. In that context I had a conversation about whether it would be possible to buy a 2 hour block of ct review time to get it prioritised over all the unfunded work (with no guarantee of merge in that time but at least definitely getting that time). It seems the blocker to such arrangements is often the 'sales process' - ie. the discussions around it take more time than is actually funded.

colemanw

I just tried r-run and ran into a bug. To reproduce:

Click the "relationships" tab for a contact.
Click "Add relationship"
Click the wrench and add a new relationship type.
Observe the "Contact(s)" field is still frozen an there is a js error in the console:

The problem is that there is a static list of relationship type metadata in the DOM which will not contain any data for the new relationship type. FWIW the variable is named relationshipData and is generated by CRM_Contact_Form_Relationship::getRelationshipTypeMetadata.

The solution is probably to add a new ajax callback path to fetch that info, and respond to the triggered event crmOptionsEdited to refetch it.

This property will be a string, but it is being compared to $contactID which should be an int.

This is used when some extra parameters are required when refreshing option lists after inline editing, i.e. relationship types are limited to the current contact type

Allows to modify spec even if generic getoptions function is used

Based on the docblock I expected contact ID to be an int, but this is not always the case.

The logic on whether to refresh the available options in CRM.popup depends on whether the crmPopupFormSuccess returned data. The data provided here is arbitrary but could be useful in the future for anyone reacting to the event

The formatting of relationship data is a FE related task and can be done in Javascript. This also allows reloading the data if some options change after editing relationship types.

deb1990 · 2018-04-16T13:33:00Z

templates/CRM/Contact/Form/Relationship.tpl

+          relationshipData = {},
+          contactTypes = {};
+
+        $('body').on('crmOptionsEdited', 'a.crm-option-edit-link', refreshRelationshipData);


Create a init function, it would help to organise the code, also place other initialisation code inside.

(function init () { $('body').on('crmOptionsEdited', 'a.crm-option-edit-link', refreshRelationshipData); refreshRelationshipData().done(...) $relationshipTypeSelect.change(function() { })()

deb1990 · 2018-04-16T13:35:54Z

templates/CRM/Contact/Form/Relationship.tpl

+         * Fetch contact types and reset relationship data
+         */
+        function refreshRelationshipData() {
+          var defer = $.Deferred();


No need to create defer object, the following will do the same thing

relationshipData = {}; return getContactTypes() .then(function() { return getRelationshipData(); }); });

deb1990 · 2018-04-16T13:42:41Z

templates/CRM/Contact/Form/Relationship.tpl

+         * Fetches the relationship data using latest relationship types
+         */
+        function getRelationshipData() {
+          var subtype,


These variables are used only inside $.each, so we can define them inside only. Overall this is how it should look like

function getRelationshipData() { var defer = $.Deferred(); if (!$.isEmptyObject(relationshipData)) { defer.resolve(relationshipData); } CRM.api3("RelationshipType", "get") .done(function (data) { $.each(data.values, function (key, relType) { // Please add a comment mentioning what is ["a", "b"] $.each(["a", "b"], function (index, suffix) { var subtype = relType["contact_subtype_" + suffix]; var type = subtype || relType["contact_type_" + suffix]; var label = getContactTypeLabel(type) || "Contact"; var placeholder = "- select " + label.toLowerCase() + " -"; relType["placeholder_" + suffix] = placeholder; }); relationshipData[relType["id"]] = relType; }); defer.resolve(relationshipData); }); return defer.promise(); }

nice, I like the early return!

colemanw · 2018-04-16T14:21:57Z

CRM/Contact/Form/Relationship.php

@@ -299,9 +299,6 @@ public function buildQuickForm() {
    // Select list
    $relationshipList = CRM_Contact_BAO_Relationship::getContactRelationshipType($this->_contactId, $this->_rtype, $this->_relationshipId);

-    // Metadata needed on clientside
-    $this->assign('relationshipData', self::getRelationshipTypeMetadata($relationshipList));


If the function getRelationshipTypeMetadata is no longer used then it can be removed. I agree this can all be done from the frontend.

It would be nice to clean it up to avoid having code that does the same thing in JS and PHP, but it's still used in one other place. My JS skills are very limited, but if you think it's best that we remove this function and provide some common service to fetch this data then I can give that a shot.

Sorry, didn't realize it was used elsewhere. I don't think that's a priority.

colemanw · 2018-04-16T14:26:19Z

templates/CRM/Contact/Form/Relationship.tpl

+          }
+
+          return defer.promise();
+        }


I would recommend not loading contact types via ajax because the two back-to-back ajax calls will be unnecessarily slow. I suggest pre-loading contactType data into the dom (via $form->assign()) to avoid the lag.

colemanw · 2018-04-16T18:22:09Z

templates/CRM/Contact/Form/Relationship.tpl

+            defer.resolve(relationshipData);
+          }
+
+          CRM.api3("RelationshipType", "get")


Need to set limit:0 to avoid the api3 default limit of 25.

My bad, we added an API wrapper to remove this by default in CiviHR and I'm too used to dealing with that

mickadoo · 2018-04-17T14:25:45Z

One test is failing, but seems to be the same on all PRs right now

colemanw · 2018-04-19T01:38:53Z

Ok I think this is good to merge. I've completed the review started by Sean and it's now passing on all levels.

mickadoo · 2018-04-19T09:17:48Z

Thanks for the in-depth review and QA @seanmadsen @colemanw and @deb1990 🙂

Included in CiviCRM 5.3.0 Core PR: civicrm#11853

Included in CiviCRM 5.2.0 Core PR: civicrm#11853

JKingsnorth · 2018-07-19T15:50:06Z

Please see https://lab.civicrm.org/dev/core/issues/268

colemanw · 2018-09-14T15:34:01Z

@mickadoo FYI the function _civicrm_api3_relationship_getoptions_spec has improper use of ts() - cannot use string concatenation within that function. I'm not even sure ts() is necessary in a field spec function since the strings are not user-facing.

eileenmcnaughton · 2019-04-26T12:35:14Z

This gitlab has been logged https://lab.civicrm.org/dev/core/issues/895 - which suggests a regression that possibly sorta comes from this PR - but with other factors

…e whole is_form thing halted my work because the keys are munged id + direction which is totally screwy. Probably would be best to ditch that as its only used for civicrm#11853

This adds basic pseudoconstant support for the field using name_a_b and label_a_b (it does not support other properties like name_b_a) For legacy purposes it preserves the work done in civicrm#11853 to return a list like [1_a_b => Child of, 1_b_a => Parent of] if the "is_form" flag is set. This was very nonstandard but is used to support in-place editing of the relationship type list on the relationship form.

civicrm-builder added the master label Mar 21, 2018

eileenmcnaughton added the has-test label Mar 21, 2018

mickadoo force-pushed the CRM-21849-inline-relationship-type-edit branch from 7a710f2 to 8494007 Compare March 27, 2018 15:08

colemanw requested changes Apr 12, 2018

View reviewed changes

mickadoo added 7 commits April 16, 2018 13:45

CRM-21849: Support fetching options for relationship type

89e4597

CRM-21849: Cast contact_id_a before comparison

e003e69

This property will be a string, but it is being compared to $contactID which should be an int.

CRM-21849: Allow adding option_context to elements

b3ee84c

This is used when some extra parameters are required when refreshing option lists after inline editing, i.e. relationship types are limited to the current contact type

CRM-21849: Allow spec modification for getoptions

3370a7b

Allows to modify spec even if generic getoptions function is used

CRM-21849: Add getoptions spec params for relationship_type_id

44d259b

CRM-21849: Change to loose comparison

b277f8d

Based on the docblock I expected contact ID to be an int, but this is not always the case.

CRM-21849: Include data in event after enable/disable

a2f1945

The logic on whether to refresh the available options in CRM.popup depends on whether the crmPopupFormSuccess returned data. The data provided here is arbitrary but could be useful in the future for anyone reacting to the event

mickadoo force-pushed the CRM-21849-inline-relationship-type-edit branch 2 times, most recently from 5ede39b to 5093350 Compare April 16, 2018 13:17

CRM-21849: Switch loading relationship data from backend to frontend

601cc89

The formatting of relationship data is a FE related task and can be done in Javascript. This also allows reloading the data if some options change after editing relationship types.

mickadoo force-pushed the CRM-21849-inline-relationship-type-edit branch from 5093350 to 601cc89 Compare April 16, 2018 13:24

deb1990 suggested changes Apr 16, 2018

View reviewed changes

colemanw reviewed Apr 16, 2018

View reviewed changes

mickadoo added 2 commits April 16, 2018 16:10

CRM-21849: Post-review cleanup

6fe0a7d

CRM-21849: Load contactTypes in PHP, not JS

8e383c2

colemanw reviewed Apr 16, 2018

View reviewed changes

deb1990 approved these changes Apr 17, 2018

View reviewed changes

CRM-21849: Remove limit on API call

265af90

colemanw approved these changes Apr 17, 2018

View reviewed changes

colemanw merged commit 8811fef into civicrm:master Apr 19, 2018

mickadoo mentioned this pull request Apr 19, 2018

CRM-21849: Inline Relationship Type Edit (4.7.27 backport) compucorp/civicrm-core#16

Merged

mickadoo added a commit to compucorp/civicrm-core that referenced this pull request Apr 19, 2018

PCHR-3465: Support inline relationship type editing

71c0d48

Included in CiviCRM 5.3.0 Core PR: civicrm#11853

mickadoo added a commit to compucorp/civicrm-core that referenced this pull request Apr 19, 2018

PCHR-3465: Support inline relationship type editing

75bb74a

Included in CiviCRM 5.2.0 Core PR: civicrm#11853

mickadoo deleted the CRM-21849-inline-relationship-type-edit branch April 30, 2018 08:11

colemanw mentioned this pull request Dec 26, 2020

Add pseudoconstant for Relationship.relationship_type_id #19278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRM-21849: Inline Relationship Type Edit #11853

CRM-21849: Inline Relationship Type Edit #11853

mickadoo commented Mar 21, 2018 •

edited

Loading

seancolsen commented Mar 27, 2018

mickadoo commented Mar 27, 2018

seancolsen commented Mar 27, 2018

eileenmcnaughton commented Mar 28, 2018

mickadoo commented Mar 28, 2018

seancolsen commented Mar 28, 2018

mickadoo commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

colemanw left a comment •

edited

Loading

deb1990 Apr 16, 2018

deb1990 Apr 16, 2018

deb1990 Apr 16, 2018

mickadoo Apr 16, 2018

colemanw Apr 16, 2018

mickadoo Apr 16, 2018

colemanw Apr 16, 2018

colemanw Apr 16, 2018

colemanw Apr 16, 2018

mickadoo Apr 17, 2018

mickadoo commented Apr 17, 2018

colemanw commented Apr 19, 2018

mickadoo commented Apr 19, 2018

JKingsnorth commented Jul 19, 2018

colemanw commented Sep 14, 2018

eileenmcnaughton commented Apr 26, 2019

CRM-21849: Inline Relationship Type Edit #11853

CRM-21849: Inline Relationship Type Edit #11853

Conversation

mickadoo commented Mar 21, 2018 • edited Loading

Overview

Before

After

Technical Details

seancolsen commented Mar 27, 2018

mickadoo commented Mar 27, 2018

seancolsen commented Mar 27, 2018

eileenmcnaughton commented Mar 28, 2018

mickadoo commented Mar 28, 2018

seancolsen commented Mar 28, 2018

mickadoo commented Apr 5, 2018

eileenmcnaughton commented Apr 5, 2018

colemanw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickadoo commented Apr 17, 2018

colemanw commented Apr 19, 2018

mickadoo commented Apr 19, 2018

JKingsnorth commented Jul 19, 2018

colemanw commented Sep 14, 2018

eileenmcnaughton commented Apr 26, 2019

mickadoo commented Mar 21, 2018 •

edited

Loading

colemanw left a comment •

edited

Loading