From c5a8afa59088ca765ee1c1045a03cfaf687f7ea0 Mon Sep 17 00:00:00 2001 From: eileen Date: Tue, 12 Jan 2021 20:55:15 +1300 Subject: [PATCH] [REF] Simplify activity import validation There are 2 types of validation on importing 1) the correct fields have been mapped 2) the specific row has all the required fields These generally live in different places - field mapping is validated on the MapField::formRule validation function - the specific row is validated in the (badly named) 'summary' function. However, the second function is also doing some validation of the mapping, checking that either activity_type_id or label is mapped & ditto activity_date_time. This duplicates the MapField rule and is hard to read, while making a larger cleanup difficult --- CRM/Activity/Import/Form/MapField.php | 4 +- CRM/Activity/Import/Parser/Activity.php | 107 +++++++++++++++--------- 2 files changed, 70 insertions(+), 41 deletions(-) diff --git a/CRM/Activity/Import/Form/MapField.php b/CRM/Activity/Import/Form/MapField.php index ae5a85ffac..8dcf6ee62a 100644 --- a/CRM/Activity/Import/Form/MapField.php +++ b/CRM/Activity/Import/Form/MapField.php @@ -265,7 +265,7 @@ class CRM_Activity_Import_Form_MapField extends CRM_Import_Form_MapField { $contactFieldsBelowWeightMessage = self::validateRequiredContactMatchFields('Individual', $importKeys); foreach ($requiredFields as $field => $title) { if (!in_array($field, $importKeys)) { - if ($field == 'target_contact_id') { + if ($field === 'target_contact_id') { if (!$contactFieldsBelowWeightMessage || in_array('external_identifier', $importKeys)) { continue; } @@ -275,7 +275,7 @@ class CRM_Activity_Import_Form_MapField extends CRM_Import_Form_MapField { . '
'; } } - elseif ($field == 'activity_type_id') { + elseif ($field === 'activity_type_id') { if (in_array('activity_label', $importKeys)) { continue; } diff --git a/CRM/Activity/Import/Parser/Activity.php b/CRM/Activity/Import/Parser/Activity.php index e38432cff8..822b2d3435 100644 --- a/CRM/Activity/Import/Parser/Activity.php +++ b/CRM/Activity/Import/Parser/Activity.php @@ -24,9 +24,6 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Activity_Import_Parser { protected $_mapperKeys; private $_contactIdIndex; - private $_activityTypeIndex; - private $_activityLabelIndex; - private $_activityDateIndex; /** * Array of successfully imported activity id's @@ -84,9 +81,6 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Activity_Import_Parser { // FIXME: we should do this in one place together with Form/MapField.php $this->_contactIdIndex = -1; - $this->_activityTypeIndex = -1; - $this->_activityLabelIndex = -1; - $this->_activityDateIndex = -1; $index = 0; foreach ($this->_mapperKeys as $key) { @@ -95,18 +89,6 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Activity_Import_Parser { case 'external_identifier': $this->_contactIdIndex = $index; break; - - case 'activity_label': - $this->_activityLabelIndex = $index; - break; - - case 'activity_type_id': - $this->_activityTypeIndex = $index; - break; - - case 'activity_date_time': - $this->_activityDateIndex = $index; - break; } $index++; } @@ -149,29 +131,15 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Activity_Import_Parser { public function summary(&$values) { $erroneousField = NULL; $this->setActiveFieldValues($values, $erroneousField); - $index = -1; - if ($this->_activityTypeIndex > -1 && $this->_activityLabelIndex > -1) { - array_unshift($values, ts('Please select either Activity Type ID OR Activity Type Label.')); - return CRM_Import_Parser::ERROR; - } - elseif ($this->_activityLabelIndex > -1) { - $index = $this->_activityLabelIndex; - } - elseif ($this->_activityTypeIndex > -1) { - $index = $this->_activityTypeIndex; - } - - if ($index < 0 or $this->_activityDateIndex < 0) { - $errorRequired = TRUE; - } - else { - $errorRequired = !CRM_Utils_Array::value($index, $values) || !CRM_Utils_Array::value($this->_activityDateIndex, $values); + try { + $this->validateActivityTypeIDAndLabel($values); + if (!$this->getFieldValue($values, 'activity_date_time')) { + throw new CRM_Core_Exception(ts('Missing required fields')); + } } - - if ($errorRequired) { - array_unshift($values, ts('Missing required fields')); - return CRM_Import_Parser::ERROR; + catch (CRM_Core_Exception $e) { + return $this->addError($values, [$e->getMessage()]); } $params = &$this->getActiveFieldParams(); @@ -436,4 +404,65 @@ class CRM_Activity_Import_Parser_Activity extends CRM_Activity_Import_Parser { return NULL; } + /** + * + * Get the value for the given field from the row of values. + * + * @param array $row + * @param string $fieldName + * + * @return null|string + */ + protected function getFieldValue(array $row, string $fieldName) { + if (!is_numeric($this->getFieldIndex($fieldName))) { + return NULL; + } + return $row[$this->getFieldIndex($fieldName)] ?? NULL; + } + + /** + * Get the index for the given field. + * + * @param string $fieldName + * + * @return false|int + */ + protected function getFieldIndex(string $fieldName) { + return array_search($fieldName, $this->_mapperKeys, TRUE); + + } + + /** + * Add an error to the values. + * + * @param array $values + * @param array $error + * + * @return int + */ + protected function addError(array &$values, array $error): int { + array_unshift($values, implode(';', $error)); + return CRM_Import_Parser::ERROR; + } + + /** + * Validate that the activity type id does not conflict with the label. + * + * @param array $values + * + * @return void + * @throws \CRM_Core_Exception + */ + protected function validateActivityTypeIDAndLabel(array $values): void { + $activityLabel = $this->getFieldValue($values, 'activity_label'); + $activityTypeID = $this->getFieldValue($values, 'activity_type_id'); + if ($activityLabel && $activityTypeID + && $activityLabel !== CRM_Core_PseudoConstant::getLabel('CRM_Activity_BAO_Activity', 'activity_type_id', $activityTypeID)) { + throw new CRM_Core_Exception(ts('Activity type label and Activity type ID are in conflict')); + } + if (!$activityLabel && !$activityTypeID) { + throw new CRM_Core_Exception(ts('Missing required fields: Activity type label or Activity type ID')); + } + } + } -- 2.25.1