From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Wed, 12 Jan 2022 21:32:13 +0000 (+1300)
Subject: Rationalise relationship validation
X-Git-Url: https://vcs.fsf.org/?a=commitdiff_plain;h=dead2d663e4e8beacb7fc37362e0db31afed6009;p=civicrm-core.git

Rationalise relationship validation

The previous relationship validation was
1) throw a message is no valid employer relationship - this seems to be done
in a place that would make the site unusable I've centralised this check into a
cached function that gets the id & throws the exception & is used from
places previously fetching it (there is one that
didn't throw an exception previously but I think enough places did
that extending to that place would not break the site any more if it didn't exist

2) complicated look up to determine if the contact types match the relationship's
expectations. Since we already know the relationship's expectations we
can simplify this to just check contact a is an individual and b is an org which is
much more reasonable. It doesn;t seem like this would ever actually be false so
I added a deprecation to be a bit vocal if it is
---

diff --git a/CRM/Contact/BAO/Contact/Utils.php b/CRM/Contact/BAO/Contact/Utils.php
index 27e5446b28..74578de9bb 100644
--- a/CRM/Contact/BAO/Contact/Utils.php
+++ b/CRM/Contact/BAO/Contact/Utils.php
@@ -270,12 +270,13 @@ WHERE  id IN ( $idString )
         ])->execute()->first()['id'];
     }
 
-    // get the relationship type id of "Employee of"
-    $relTypeId = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', 'Employee of', 'id', 'name_a_b');
-    if (!$relTypeId) {
-      throw new CRM_Core_Exception(ts("You seem to have deleted the relationship type 'Employee of'"));
+    $relTypeId = CRM_Contact_BAO_RelationshipType::getEmployeeRelationshipTypeID();
+    if (!CRM_Contact_BAO_Contact::getContactType($contactID) || !CRM_Contact_BAO_Contact::getContactType($organization)) {
+      // There doesn't seem to be any reason to think this would ever be true but there
+      // was a previous more complicated check.
+      CRM_Core_Error::deprecatedWarning('attempting to create an employer with invalid contact types is deprecated');
+      return;
     }
-
     // create employee of relationship
     [$duplicate, $relationshipIds]
       = self::legacyCreateMultiple($relTypeId, $organization, $contactID);
@@ -340,10 +341,6 @@ WHERE  id IN ( $idString )
       'contact_id_b' => $organizationID,
       'relationship_type_id' => $relationshipTypeID,
     ];
-    if (!CRM_Contact_BAO_Relationship::checkRelationshipType($contactFields['contact_id_a'], $contactFields['contact_id_b'],
-      $contactFields['relationship_type_id'])) {
-      return [0, []];
-    }
 
     if (
       CRM_Contact_BAO_Relationship::checkDuplicateRelationship(
@@ -460,10 +457,7 @@ WHERE id={$contactId}; ";
       //2. delete related membership.
 
       //get the relationship type id of "Employee of"
-      $relTypeId = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', 'Employee of', 'id', 'name_a_b');
-      if (!$relTypeId) {
-        throw new CRM_Core_Exception(ts("You seem to have deleted the relationship type 'Employee of'"));
-      }
+      $relTypeId = CRM_Contact_BAO_RelationshipType::getEmployeeRelationshipTypeID();
       $relMembershipParams['relationship_type_id'] = $relTypeId . '_a_b';
       $relMembershipParams['contact_check'][$employerId] = 1;
 
diff --git a/CRM/Contact/BAO/Relationship.php b/CRM/Contact/BAO/Relationship.php
index 208e0198f8..ace3d020aa 100644
--- a/CRM/Contact/BAO/Relationship.php
+++ b/CRM/Contact/BAO/Relationship.php
@@ -603,12 +603,7 @@ class CRM_Contact_BAO_Relationship extends CRM_Contact_DAO_Relationship {
         $sharedContact->id = $relationship->contact_id_a;
         $sharedContact->find(TRUE);
 
-        // CRM-15881 UPDATES
-        // changed FROM "...relationship->relationship_type_id == 4..." TO "...relationship->relationship_type_id == 5..."
-        // As the system should be looking for type "employer of" (id 5) and not "sibling of" (id 4)
-        // As suggested by @davecivicrm, the employee relationship type id is fetched using the CRM_Core_DAO::getFieldValue() class and method, since these ids differ from system to system.
-        $employerRelTypeId = CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', 'Employee of', 'id', 'name_a_b');
-
+        $employerRelTypeId = CRM_Contact_BAO_RelationshipType::getEmployeeRelationshipTypeID();
         if ($relationship->relationship_type_id == $employerRelTypeId && $relationship->contact_id_b == $sharedContact->employer_id) {
           CRM_Contact_BAO_Contact_Utils::clearCurrentEmployer($relationship->contact_id_a);
         }
diff --git a/CRM/Contact/BAO/RelationshipType.php b/CRM/Contact/BAO/RelationshipType.php
index b7c5c75580..17e38025c0 100644
--- a/CRM/Contact/BAO/RelationshipType.php
+++ b/CRM/Contact/BAO/RelationshipType.php
@@ -9,7 +9,9 @@
  +--------------------------------------------------------------------+
  */
 
+use Civi\Api4\Relationship;
 use Civi\Api4\RelationshipType;
+use Civi\Core\Event\PreEvent;
 
 /**
  *
@@ -110,19 +112,21 @@ class CRM_Contact_BAO_RelationshipType extends CRM_Contact_DAO_RelationshipType
 
   /**
    * Callback for hook_civicrm_pre().
+   *
    * @param \Civi\Core\Event\PreEvent $event
-   * @throws CRM_Core_Exception
+   *
+   * @throws \API_Exception
+   * @throws \Civi\API\Exception\UnauthorizedException
    */
-  public static function self_hook_civicrm_pre(\Civi\Core\Event\PreEvent $event) {
+  public static function self_hook_civicrm_pre(PreEvent $event): void {
     if ($event->action === 'delete') {
       // need to delete all option value field before deleting group
-      \Civi\Api4\Relationship::delete(FALSE)
+      Relationship::delete(FALSE)
         ->addWhere('relationship_type_id', '=', $event->id)
         ->execute();
     }
   }
 
-
   /**
    * Get the id of the employee relationship, checking it is valid.
    *
diff --git a/CRM/Contribute/Form/Contribution/Confirm.php b/CRM/Contribute/Form/Contribution/Confirm.php
index 7f4e62aec0..f1a1536dc9 100644
--- a/CRM/Contribute/Form/Contribution/Confirm.php
+++ b/CRM/Contribute/Form/Contribution/Confirm.php
@@ -1324,7 +1324,7 @@ class CRM_Contribute_Form_Contribution_Confirm extends CRM_Contribute_Form_Contr
         \Civi\Api4\Relationship::create(FALSE)
           ->addValue('contact_id_a', $contactID)
           ->addValue('contact_id_b', $orgID)
-          ->addValue('relationship_type_id', CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_RelationshipType', 'Employee of', 'id', 'name_a_b'))
+          ->addValue('relationship_type_id', $relTypeId = CRM_Contact_BAO_RelationshipType::getEmployeeRelationshipTypeID())
           ->addValue('is_permission_a_b:name', 'View and update')
           ->execute();
       }
diff --git a/tests/phpunit/api/v3/ContactTest.php b/tests/phpunit/api/v3/ContactTest.php
index 040ef4ad32..3062f66b89 100644
--- a/tests/phpunit/api/v3/ContactTest.php
+++ b/tests/phpunit/api/v3/ContactTest.php
@@ -909,8 +909,6 @@ class api_v3_ContactTest extends CiviUnitTestCase {
    * Test creating a current employer through API.
    *
    * Check it will re-activate a de-activated employer
-   *
-   * @throws \CRM_Core_Exception
    */
   public function testContactCreateDuplicateCurrentEmployerEnables(): void {
     // Set up  - create employer relationship.