Allow to disable an invalid relationship (eg. contact subtype was changed so no longer valid)

[mattwire]

Overview

If you use (and change) contact sub-types it is easy to end up with invalid relationships that you can't do anything with.
It should be possible to disable these relationships without having to modify them (eg. a contact who had "Staff" subtype is changed to "Ex-Staff" subtype).
The relationship is now invalid according to CiviCRM but it is still correct from a historical data perspective. This also affects the "update expired relationships" scheduled job which can get stuck if it finds and invalid relationship.

Before

Invalid relationships can't be disabled..

After

Invalid relationships can be disabled.

Technical Details

Explained above. It would be nice to have a separate API call for disable so we don't have to have so much logic in ::create. But that would require changes to various parts of the API/UI etc. so out of scope for now.

Comments

[civibot]

(Standard links)

• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[demeritcowboy]

Fail seems related:

CRM_Contact_BAO_RelationshipTest::testDisableInvalidRelationship
Exception: CRM_Core_Exception: "'2' is not a valid option for field relationship_type_id"
#0 /home/jenkins/bknix-dfl/build/build-2/web/sites/all/modules/civicrm/tests/phpunit/CRM/Contact/BAO/RelationshipTest.php(369): civicrm_api3("Relationship", "create", (Array:3))

[mattwire]

Hmm, passes locally!

[mattwire]

test this please

[demeritcowboy]

Hmm...

[demeritcowboy]

It's the same test fail.
I don't think it's a v3 vs v4 - when you run the test locally just the single test, it passes but by fluke, because there is a relationship_type_id 2 that happens to exist (Spouse), but when run as part of the whole file, the teardown() removes the stock relationships and they are never readded, so 2 no longer exists when it gets to this test.

Something is checking the relationship_type even when you're just trying to update is_active. Now, why it is checking "2" I haven't looked. Possibly this surfaces a different bug?

[mattwire]

Possibly this surfaces a different bug?

Yep, it's looking for related memberships for an unrelated relationship that doesn't exist in the test environment...
I've added a check so we don't call that code if not required - which should help a little bit with performance and future refactoring of that giant ball of code.

[mattwire]

Possibly this surfaces a different bug?

Or two...

[demeritcowboy]

Taking another look at this now but first noticing a non-blocking r-code comment:
The variable $disableRelationship can be undefined at line 63 and while php doesn't complain it just seems better to explicitly set it, and some fancy IDE's might nag about it.

-    if (!empty($params['id']) && array_key_exists('is_active', $params) && empty($params['is_active'])) {
-      $disableRelationship = TRUE;
-    }
+    $disableRelationship = !empty($params['id']) && array_key_exists('is_active', $params) && empty($params['is_active']);

[colemanw]

@mattwire can you make this change?
@demeritcowboy if you use the suggestion feature then it can be merged in with a click :)

[demeritcowboy]

I'll just merge and do a followup in a bit.