N°7216 import improves error handling missing or null data

core/bulkchange.class.inc.php

@@ -519,7 +519,7 @@ protected function IsNullExternalKeySpec($aRowData, $sAttCode)
 		foreach ($this->m_aExtKeys[$sAttCode] as $sForeignAttCode => $iCol)
 		{
 			// The foreign attribute is one of our reconciliation key
-			if (strlen($aRowData[$iCol]) > 0)
+			if (isset($aRowData[$iCol]) && strlen($aRowData[$iCol]) > 0)
 			{
 				return false;
 			}
@@ -1181,6 +1181,9 @@ public function Process(CMDBChange $oChange = null)
 					foreach($this->m_aData as $iRow => $aRowData)
 					{
 						$sFormat = $sDateTimeFormat;
+						if(!isset($this->m_aData[$iRow][$iCol])){
+							continue;
+						}
 						$sValue = $this->m_aData[$iRow][$iCol];
 						if (!empty($sValue))
 						{
@@ -1233,11 +1236,19 @@ public function Process(CMDBChange $oChange = null)
 		$iPreviousTimeLimit = ini_get('max_execution_time');
 		$iLoopTimeLimit = MetaModel::GetConfig()->Get('max_execution_time_per_loop');
 
+		$iNBFields = count($this->m_aAttList) +  count($this->m_aExtKeys);
+
 		// Avoid too many events
 		cmdbAbstractObject::SetEventDBLinksChangedBlocked(true);
 		try {
 			foreach ($this->m_aData as $iRow => $aRowData) {
 				set_time_limit(intval($iLoopTimeLimit));
+				// Stop if not the good number of cols in $aRowData
+				if(count($aRowData) != $iNBFields){
+					$aResult[$iRow]["__STATUS__"] = new RowStatus_Issue(Dict::Format('UI:CSVReport-Row-Issue-NbField',count($aRowData),$iNBFields) );
+					continue;
+				}
+
 				if (isset($aResult[$iRow]["__STATUS__"])) {
 					// An issue at the earlier steps - skip the rest
 					continue;
@@ -1348,7 +1359,11 @@ public function Process(CMDBChange $oChange = null)
 			{
 				if (!array_key_exists($iCol, $aResult[$iRow]))
 				{
-					$aResult[$iRow][$iCol] = new CellStatus_Void($aRowData[$iCol]);
+					if(isset($aRowData[$iCol])) {
+						$aResult[$iRow][$iCol] = new CellStatus_Void($aRowData[$iCol]);
+					} else {
+						$aResult[$iRow][$iCol] = new CellStatus_Issue('', null, Dict::S('UI:CSVReport-Value-Issue-NoValue'));
+					}
 				}
 			}
 			foreach($this->m_aExtKeys as $sAttCode => $aForeignAtts)
@@ -1362,7 +1377,11 @@ public function Process(CMDBChange $oChange = null)
 					if (!array_key_exists($iCol, $aResult[$iRow]))
 					{
 						// The foreign attribute is one of our reconciliation key
-						$aResult[$iRow][$iCol] = new CellStatus_Void($aRowData[$iCol]);
+						if(isset($aRowData[$iCol])) {
+							$aResult[$iRow][$iCol] = new CellStatus_Void($aRowData[$iCol]);
+						} else {
+							$aResult[$iRow][$iCol] = new CellStatus_Issue('', null, 'UI:CSVReport-Value-Issue-NoValue');
+						}
 					}
 				}
 			}

dictionaries/en.dictionary.itop.ui.php

@@ -719,6 +719,8 @@
 	'UI:CSVReport-Row-Issue-Reconciliation'     => 'failed to reconcile',
 	'UI:CSVReport-Row-Issue-Ambiguous'          => 'ambiguous reconciliation',
 	'UI:CSVReport-Row-Issue-Internal'           => 'Internal error: %1$s, %2$s',
+	'UI:CSVReport-Value-Issue-NoValue'          => 'No value',
+	'UI:CSVReport-Row-Issue-NbField'            => 'Not the expected number of columns (found: %1$s, expected: %2$s)',
 
 	'UI:CSVReport-Icon-Unchanged'         => 'Unchanged',
 	'UI:CSVReport-Icon-Modified'          => 'Modified',

dictionaries/fr.dictionary.itop.ui.php

@@ -4,7 +4,7 @@
  *
  * @copyright Copyright (C) 2010-2024 Combodo SAS
  * @license    https://opensource.org/licenses/AGPL-3.0
- * 
+ *
  */
 /**
  *
@@ -678,6 +678,8 @@
 	'UI:CSVReport-Row-Issue-ExpectedDateFormat' => 'Expected format: %1$s~~',
 	'UI:CSVReport-Row-Issue-Inconsistent' => 'Incohérence entre attributs: %1$s',
 	'UI:CSVReport-Row-Issue-Internal' => 'Erreur interne: %1$s, %2$s',
+	'UI:CSVReport-Value-Issue-NoValue' => 'Pas de valeur',
+	'UI:CSVReport-Row-Issue-NbField' => 'Le nombre de colonnes ne correspond pas à ce qui est attendu (constaté : %1$s, attendu : %2$s)',
 	'UI:CSVReport-Row-Issue-MissingExtKey' => 'Ne peut pas être créé car il manque des clés externes : %1$s',
 	'UI:CSVReport-Row-Issue-Reconciliation' => 'Echec de réconciliation',
 	'UI:CSVReport-Row-Unchanged' => 'inchangé',

pages/ajax.csvimport.php

@@ -345,7 +345,7 @@ function GetMappingForField($sClassName, $sFieldName, $iFieldIndex, $bAdvancedMo
 				$oPanel->AddSubBlock($oTable);
 
 				$oPage->AddSubBlock($oPanel);
-				if (empty($sInitSearchField)) {
+				if (empty($sInitSearchField) || empty($aInitFieldMapping)) {
 					// Propose a reconciliation scheme
 					//
 					$aReconciliationKeys = MetaModel::GetReconcKeys($sClassName);

tests/php-unit-tests/unitary-tests/core/BulkChangeTest.php

@@ -221,30 +221,36 @@ public function bulkChangeWithoutInitDataProvider() {
 	 * @param $aExtKeys
 	 * @param $aReconcilKeys
 	 */
-	public function testBulkChangeWithExistingData($aInitData, $aCsvData, $aAttributes, $aExtKeys, $aReconcilKeys, $aResult, $aResultHTML= null) {
+	public function testBulkChangeWithExistingData($aInitData, $aCsvData, $aAttributes, $aExtKeys, $aReconcilKeys, $aResult, $aResultHTML= null ) {
 		//change value during the test
 		$db_core_transactions_enabled=MetaModel::GetConfig()->Get('db_core_transactions_enabled');
 		MetaModel::GetConfig()->Set('db_core_transactions_enabled',false);
 
+
 		if (is_array($aInitData) && sizeof($aInitData) != 0) {
 			/** @var Server $oServer */
 			$oServer = $this->createObject('Server', array(
-				'name' => $aInitData[1],
-				'status' => $aInitData[2],
-				'org_id' => $aInitData[0],
+				'name'          => $aInitData[1],
+				'status'        => $aInitData[2],
+				'org_id'        => $aInitData[0],
 				'purchase_date' => $aInitData[3],
 			));
-			$aCsvData[0][2]=$oServer->GetKey();
-			$aResult[2]=$oServer->GetKey();
-			if ($aResult["id"]==="{Id of the server created by the test}") {
-				$aResult["id"]=$oServer->GetKey();
-				if ($aResultHTML!==null){
-					$aResultHTML[2]=$oServer->GetKey();
-					$aResultHTML["id"]=$oServer->GetKey();
+			if(array_key_exists('id', $aAttributes)) {
+				$iColumnForId = $aAttributes['id'];
+				$aCsvData[0][$iColumnForId] = $oServer->GetKey();
+				$aResult[$iColumnForId] = $oServer->GetKey();
+				if ($aResult["id"] === "{Id of the server created by the test}") {
+					$aResult["id"] = $oServer->GetKey();
+					if ($aResultHTML !== null) {
+						$aResultHTML[$iColumnForId] = $oServer->GetKey();
+						$aResultHTML["id"] = $oServer->GetKey();
+					}
 				}
 			}
 			$this->debug("oServer->GetKey():".$oServer->GetKey());
 		}
+
+
 		$oBulk = new BulkChange(
 			"Server",
 			$aCsvData,
@@ -269,7 +275,7 @@ public function testBulkChangeWithExistingData($aInitData, $aCsvData, $aAttribut
 						$this->debug('GetCLIValue:'.$oCell->GetCLIValue());
 						$this->debug("aResult:".$aResult[$i]);
 						$this->assertEquals( $aResult[$i], $oCell->GetCLIValue(), "failure on " . get_class($oCell) . ' cell type for cell number ' . $i );
-						if (null !== $aResultHTML) {
+						if (null !== $aResultHTML && array_key_exists($i, $aResultHTML)) {
 							$this->assertEquals($aResultHTML[$i], $oCell->GetHTMLValue(), "failure on " . get_class($oCell) . ' cell type for cell number ' . $i);
 						}
 					} else if ($i === "__ERRORS__") {
@@ -293,37 +299,39 @@ public function bulkChangeWithExistingDataProvider() {
 						["1", "Server1", "production", ""],
 					"csvData" =>
 						[[">Demo", "Server1"]],
-						 "attributes"=>
-							 ["name" => 1],
-						 "extKeys"=>
-							 ["org_id" => ["name" => 0]],
-						 "reconcilKeys"=>
-							 ["name"],
-						 "expectedResult"=>
-							 [
-								 0 => ">Demo",
-								 "org_id" => "n/a",
-								 1 => "Server1",
-								"id" => "Invalid value for attribute",
-								 "__STATUS__" => "Issue: ambiguous reconciliation",
-								 "__ERRORS__" => "Allowed 'status' value(s): stock,implementation,production,obsolete",
-							 ],
+					"attributes"=>
+						 ["name" => 1],
+					"extKeys"=>
+						 ["org_id" => ["name" => 0]],
+					"reconcilKeys"=>
+						 ["name"],
+					"expectedResult"=>
+						[
+							0 => ">Demo",
+							"org_id" => "n/a",
+							1 => "Server1",
+							"id" => "Invalid value for attribute",
+							"name" => "Invalid value for attribute",
+							"__STATUS__" => "Issue: ambiguous reconciliation",
+							"__ERRORS__" => "Object not found",
+						],
 					"expectedResultHTML"=>
 						[
 							0 => ">Demo",
 							"org_id" => "n/a",
 							1 => "Server1",
 							"id" => "Invalid value for attribute",
+							"name" => "Invalid value for attribute",
 							"__STATUS__" => "Issue: ambiguous reconciliation",
-							"__ERRORS__" => "Allowed 'status' value(s): stock,implementation,production,obsolete",
+							"__ERRORS__" => "Object not found",
 						],
 			],
 			"Case 6 - 1 : Unexpected value (update)" => [
 			    "initData"=>
 					["1", ">ServerTest", "production", ""],
-			     "csvData"=>
+			    "csvData"=>
 					[["Demo", ">ServerTest", "key - will be automatically overwritten by test", ">BadValue", ""]],
-			     "attributes"=>
+			    "attributes"=>
 					["name" => 1, "id" => 2, "status" => 3, "purchase_date" => 4],
 				"extKeys"=>
 					["org_id" => ["name" => 0]],
@@ -353,7 +361,6 @@ public function bulkChangeWithExistingDataProvider() {
 					    "__STATUS__" => "Issue: Unexpected attribute value(s)",
 					    "__ERRORS__" => "Allowed 'status' value(s): stock,implementation,production,obsolete",
 				    ],
-
 			],
 			"Case 6 - 2 : Unexpected value (update)" => [
 				"initData"=>
@@ -395,9 +402,9 @@ public function bulkChangeWithExistingDataProvider() {
 				"initData"=>
 					[],
 				"csvData"=>
-					[["Demo", ">ServerTest", "<svg onclick\"alert(1)\">", ""]],
+					[["Demo", ">ServerTest", "<svg onclick\"alert(1)\">", 1]],
 				"attributes"=>
-					["name" => 1, "status" => 2, "purchase_date" => 3],
+					["name" => 1, "status" => 2, 'id'=> 3],
 				"extKeys"=>
 					["org_id" => ["name" => 0]],
 				"reconcilKeys"=>
@@ -409,7 +416,7 @@ public function bulkChangeWithExistingDataProvider() {
 						"org_id" => "3",
 						1 => "\">ServerTest\"",
 						2 => '\'<svg onclick"alert(1)">\' is an invalid value',
-						3 => "",
+						3 => "1",
 						"__STATUS__" => "Issue: Unexpected attribute value(s)",
 						"__ERRORS__" => "Allowed 'status' value(s): stock,implementation,production,obsolete",
 					],
@@ -420,7 +427,7 @@ public function bulkChangeWithExistingDataProvider() {
 						"org_id" => "3",
 						1 => "&gt;ServerTest",
 						2 => "'&lt;svg onclick&quot;alert(1)&quot;&gt;' is an invalid value",
-						3 => "",
+						3 => "1",
 						"__STATUS__" => "Issue: Unexpected attribute value(s)",
 						"__ERRORS__" => "Allowed 'status' value(s): stock,implementation,production,obsolete",
 					],
@@ -579,11 +586,52 @@ public function bulkChangeWithExistingDataProvider() {
 					[						"id" => "{Id of the server created by the test}",
 					                         0 => "Demo", "org_id" => "n/a", 1 => ">ServerTest", 2 => "1", 3 => "production", 4 => "'2020-20-03' is an invalid value", "id" => 1, "__STATUS__" => "Issue: wrong date format"],
 			],
+			"Case 11 : Missing AttributeDateTime cell should issue an error" => [
+				"initData"=>
+					["1", "ServerTest", "production", "2020-02-01"],
+				"csvData"=>
+					[["1"]],
+				"attributes"=>
+					["id" => 0,"purchase_date" => 1],
+				"extKeys"=>
+					[],
+				"reconcilKeys"=>
+					["id"],
+				"expectedResult"=>
+					[  1 => "'' is an invalid value", "id" => 1, "__STATUS__" => 'Issue: Not the expected number of columns (found: 1, expected: 2)'],
+			],
+			"Case 12 : Missing AttributeEnum cell should issue an error" => [
+				"initData"=>
+					["1", "ServerTest", "production", "2020-02-01"],
+				"csvData"=>
+					[[ "1"]], // missing status
+				"attributes"=>
+					[ "id" => 0, "status" => 1],
+				"extKeys"=>
+					[],
+				"reconcilKeys"=>
+					["id"],
+				"expectedResult"=>
+					[  1 => "'' is an invalid value", "id" => 1, "__STATUS__" => 'Issue: Not the expected number of columns (found: 1, expected: 2)'],
+				],
+			"Case 13 : Missing AttributeExternalKey cell should issue an error" => [
+				"initData"=>
+					["1", "ServerTest", "production", "2020-02-01"],
+				"csvData"=>
+					[[ "1"]], // missing org_id
+				"attributes"=>
+					[ "id" => 0],
+				"extKeys"=>
+					["org_id" => ["name" => 1]],
+				"reconcilKeys"=>
+					["id"],
+				"expectedResult"=>
+					[  1 => "'' is an invalid value", "id" => 1, "__STATUS__" => 'Issue: Not the expected number of columns (found: 1, expected: 2)'],
+			],
 		];
 	}
 
 
-
 	/**
 	 * test $oBulk->Process with new server and new organization datas
 	 *
@@ -617,7 +665,7 @@ public function testBulkChangeWithExistingDataAndSpecificOrg($aInitData, $aCsvDa
 				'org_id' => $oOrganisation->GetKey(),
 				'purchase_date' => $aInitData["serverPurchaseDate"],
 			));
-			$aCsvData[0][2]=$oServer->GetKey();
+			$aCsvData[0][2] = $oServer->GetKey();
 			$aResult[2]=$oServer->GetKey();
 			if ($aResult["id"]==="{Id of the server created by the test}") {
 				$aResult["id"]=$oServer->GetKey();
@@ -647,7 +695,7 @@ public function testBulkChangeWithExistingDataAndSpecificOrg($aInitData, $aCsvDa
 					$this->debug('GetCLIValue:'.$oCell->GetCLIValue());
 					$this->debug("aResult:".$aResult[$i]);
 					$this->assertEquals($aResult[$i], $oCell->GetCLIValue(), "$i cell is incorrect");
-					if (null !== $aResultHTML) {
+					if (null !== $aResultHTML && array_key_exists($i, $aResultHTML)) {
 						$this->assertEquals($aResultHTML[$i], $oCell->GetHTMLValue());
 					}
 				} elseif ($i === "__STATUS__") {
@@ -674,7 +722,7 @@ public function bulkChangeWithExistingDataAndSpecificOrgProvider() {
 				"csvData" =>
 					[["Demo", ">Server1"]],
 				"attributes"=>
-					["name" => 1],
+					["name" => 1,"id" => 2],
 				"extKeys"=>
 					["org_id" => ["name" => 0]],
 				"reconcilKeys"=>

tests/php-unit-tests/unitary-tests/datamodels/2.x/itop-config/BulkChangeExtKeyTest.php

@@ -183,11 +183,11 @@ public function testExternalFieldIssueImportFail_AllObjectsVisibleByCurrentUser_
 			null,
 			null,
 			null,
+			null,
 			'Found 2 matches'
 		);
 	}
 
-
 	/**
 	 * @dataProvider ReconciliationKeyProvider
 	 */
@@ -209,6 +209,7 @@ public function testExternalFieldIssueImportFail_AllObjectsVisibleByCurrentUser_
 			null,
 			$bIsRackReconKey,
 			$aCsvData,
+			['description'=>3],
 			$aExtKeys,
 			$sSearchLinkUrl
 		);
@@ -224,7 +225,7 @@ private function GetUid(){
 	}
 
 	public function performBulkChangeTest($sExpectedDisplayableValue, $sExpectedDescription, $oOrg, $bIsRackReconKey,
-		$aAdditionalCsvData=null, $aExtKeys=null, $sSearchLinkUrl=null, $sError="Object not found") {
+		$aAdditionalCsvData=null, $aAdditionalAttributes=null, $aExtKeys=null, $sSearchLinkUrl=null, $sError="Object not found") {
 		if ($sSearchLinkUrl===null){
 			$sSearchLinkUrl = 'UI.php?operation=search&filter='.rawurlencode('%5B%22SELECT+%60Rack%60+FROM+Rack+AS+%60Rack%60+WHERE+%28%60Rack%60.%60name%60+%3D+%3Aname%29%22%2C%7B%22name%22%3A%22UnexistingRack%22%7D%2C%5B%5D%5D');
 		}
@@ -247,6 +248,9 @@ public function performBulkChangeTest($sExpectedDisplayableValue, $sExpectedDesc
 			}
 		}
 		$aAttributes = ["name" => 2];
+		if ($aAdditionalAttributes !== null){
+			$aAttributes = array_merge($aAttributes, $aAdditionalAttributes);
+		}
 		if ($aExtKeys == null){
 			$aExtKeys = ["org_id" => ["name" => 0], "rack_id" => ["name" => 1]];
 		}