N°6824 - Notification with current_contact placeholder trigger hundred of email sent (#562)

Co-authored-by: Molkobain <lajarige.guillaume@free.fr>
Co-authored-by: Thomas Casteleyn <thomas.casteleyn@super-visions.com>
Co-authored-by: Romain Quetiez <romain.quetiez@combodo.com>

core/metamodel.class.php

@@ -1241,7 +1241,7 @@ abstract class MetaModel
 			}
 			$sTable = self::DBGetTable($sClass);
 
-			// Could be completed later with all the classes that are using a given table 
+			// Could be completed later with all the classes that are using a given table
 			if (!array_key_exists($sTable, $aTables)) {
 				$aTables[$sTable] = array();
 			}
@@ -3522,7 +3522,7 @@ abstract class MetaModel
 		}
 
 		// Set the "host class" as soon as possible, since HierarchicalKeys use it for their 'target class' as well
-		// and this needs to be know early (for Init_IsKnowClass 19 lines below)		
+		// and this needs to be know early (for Init_IsKnowClass 19 lines below)
 		$oAtt->SetHostClass($sTargetClass);
 
 		// Some attributes could refer to a class
@@ -3564,7 +3564,7 @@ abstract class MetaModel
 
 		self::$m_aAttribDefs[$sTargetClass][$oAtt->GetCode()] = $oAtt;
 		self::$m_aAttribOrigins[$sTargetClass][$oAtt->GetCode()] = $sTargetClass;
-		// Note: it looks redundant to put targetclass there, but a mix occurs when inheritance is used		
+		// Note: it looks redundant to put targetclass there, but a mix occurs when inheritance is used
 	}
 
 	/**
@@ -3764,7 +3764,7 @@ abstract class MetaModel
 		self::$m_aStimuli[$sTargetClass][$oStimulus->GetCode()] = $oStimulus;
 
 		// I wanted to simplify the syntax of the declaration of objects in the biz model
-		// Therefore, the reference to the host class is set there 
+		// Therefore, the reference to the host class is set there
 		$oStimulus->SetHostClass($sTargetClass);
 	}
 
@@ -4219,40 +4219,77 @@ abstract class MetaModel
 		}
 		else
 		{
-			$aCurrentUser = array();
-			$aCurrentContact = array();
+			$aCurrentUser = [];
+			$aCurrentContact = [];
 			foreach ($aExpectedArgs as $expression)
 			{
 				$aName = explode('->', $expression->GetName());
 				if ($aName[0] == 'current_contact_id') {
 					$aPlaceholders['current_contact_id'] = UserRights::GetContactId();
-				}
-				if ($aName[0] == 'current_user') {
+				} else if ($aName[0] == 'current_user') {
 					array_push($aCurrentUser, $aName[1]);
-				}
-				if ($aName[0] == 'current_contact') {
+				} else if ($aName[0] == 'current_contact') {
 					array_push($aCurrentContact, $aName[1]);
 				}
 			}
 			if (count($aCurrentUser) > 0) {
-				$oUser = UserRights::GetUserObject();
-				$aPlaceholders['current_user->object()'] = $oUser;
-				foreach ($aCurrentUser as $sField) {
-					$aPlaceholders['current_user->'.$sField] = $oUser->Get($sField);
-				}
+				static::FillObjectPlaceholders($aPlaceholders, 'current_user', UserRights::GetUserObject(), $aCurrentUser);
 			}
 			if (count($aCurrentContact) > 0) {
-				$oPerson = UserRights::GetContactObject();
-				$aPlaceholders['current_contact->object()'] = $oPerson;
-				foreach ($aCurrentContact as $sField) {
-					$aPlaceholders['current_contact->'.$sField] = $oPerson->Get($sField);
-				}
+				static::FillObjectPlaceholders($aPlaceholders, 'current_contact', UserRights::GetContactObject(), $aCurrentContact);
 			}
 		}
 
 		return $aPlaceholders;
 	}
 
+	/**
+	 * @since 3.1.1 N°6824
+	 * @param array $aPlaceholders
+	 * @param string $sPlaceHolderPrefix
+	 * @param ?\DBObject $oObject
+	 * @param array $aCurrentUser
+	 *
+	 * @return void
+	 *
+	 */
+	private static function FillObjectPlaceholders(array &$aPlaceholders, string $sPlaceHolderPrefix, ?\DBObject $oObject, array $aCurrentUser) : void {
+		$sPlaceHolderKey = $sPlaceHolderPrefix."->object()";
+		if (is_null($oObject)){
+			$aContext = [
+				"current_user_id" => UserRights::GetUserId(),
+				"null object type" => $sPlaceHolderPrefix,
+				"fields" => $aCurrentUser,
+			];
+			IssueLog::Warning("Unresolved placeholders due to null object in current context", null,
+				$aContext);
+			$aPlaceholders[$sPlaceHolderKey] = \Dict::Format("Core:Placeholder:CannotBeResolved", $sPlaceHolderKey);
+			foreach ($aCurrentUser as $sField) {
+				$sPlaceHolderKey = $sPlaceHolderPrefix . "->$sField";
+				$aPlaceholders[$sPlaceHolderKey] = \Dict::Format("Core:Placeholder:CannotBeResolved", $sPlaceHolderKey);
+			}
+		} else {
+			$aPlaceholders[$sPlaceHolderKey] = $oObject;
+			foreach ($aCurrentUser as $sField) {
+				$sPlaceHolderKey = $sPlaceHolderPrefix . "->$sField";
+				if (false === MetaModel::IsValidAttCode(get_class($oObject), $sField)){
+					$aContext = [
+						"current_user_id" => UserRights::GetUserId(),
+						"obj_class" => get_class($oObject),
+						"placeholder" => $sPlaceHolderKey,
+						"invalid_field" => $sField,
+					];
+					IssueLog::Warning("Unresolved placeholder due to invalid attribute", null,
+						$aContext);
+					$aPlaceholders[$sPlaceHolderKey] = \Dict::Format("Core:Placeholder:CannotBeResolved", $sPlaceHolderKey);
+					continue;
+				}
+
+				$aPlaceholders[$sPlaceHolderKey] = $oObject->Get($sField);
+			}
+		}
+	}
+
 	/**
 	 * @param \DBSearch $oFilter
 	 *
@@ -6479,7 +6516,7 @@ abstract class MetaModel
 				$aCache['m_aExtensionClassNames'] = self::$m_aExtensionClassNames;
 				$aCache['m_Category2Class'] = self::$m_Category2Class;
 				$aCache['m_aRootClasses'] = self::$m_aRootClasses; // array of "classname" => "rootclass"
-				$aCache['m_aParentClasses'] = self::$m_aParentClasses; // array of ("classname" => array of "parentclass") 
+				$aCache['m_aParentClasses'] = self::$m_aParentClasses; // array of ("classname" => array of "parentclass")
 				$aCache['m_aChildClasses'] = self::$m_aChildClasses; // array of ("classname" => array of "childclass")
 				$aCache['m_aClassParams'] = self::$m_aClassParams; // array of ("classname" => array of class information)
 				$aCache['m_aAttribDefs'] = self::$m_aAttribDefs; // array of ("classname" => array of attributes)

dictionaries/en.dictionary.itop.core.php

@@ -49,6 +49,7 @@ Dict::Add('EN US', 'English', 'English', array(
 	'Core:AttributeTagSet' => 'List of tags',
 	'Core:AttributeTagSet+' => '',
 	'Core:AttributeSet:placeholder' => 'click to add',
+	'Core:Placeholder:CannotBeResolved' => '(%1$s : cannot be resolved)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromClass' => '%1$s (%2$s)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromOneChildClass' => '%1$s (%2$s from %3$s)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromSeveralChildClasses' => '%1$s (%2$s from child classes)',

dictionaries/fr.dictionary.itop.core.php

@@ -39,6 +39,7 @@ Dict::Add('FR FR', 'French', 'Français', array(
 	'Core:AttributeTagSet' => 'Liste d\'étiquettes',
 	'Core:AttributeTagSet+' => '',
 	'Core:AttributeSet:placeholder' => 'cliquer pour ajouter',
+	'Core:Placeholder:CannotBeResolved' => '(%1$s : non remplacé)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromClass' => '%1$s (%2$s)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromOneChildClass' => '%1$s (%2$s de la classe %3$s)',
 	'Core:AttributeClassAttCodeSet:ItemLabel:AttributeFromSeveralChildClasses' => '%1$s (%2$s d\'une sous-classe)',

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

@@ -0,0 +1,206 @@
+<?php
+
+namespace Combodo\iTop\Test\UnitTest\Core;
+
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use MetaModel;
+use PHPUnit\Framework\ExpectationFailedException;
+use UserRights;
+use VariableExpression;
+
+/**
+ * Class MetaModelMagicPlaceholderTest
+ * @since 3.1.1 N°6824
+ * @covers MetaModel::AddMagicPlaceholders()
+ * @package Combodo\iTop\Test\UnitTest\Core
+ */
+class MetaModelMagicPlaceholderTest extends ItopDataTestCase
+{
+	/**
+	 * Asserts that two array with DBObjects are equal (the important is to check the {class,id} couple
+	 *
+	 * @throws \SebastianBergmann\RecursionContext\InvalidArgumentException
+	 * @throws ExpectationFailedException
+	 */
+	public static function assertEqualsShallow($expected, $actual, string $message = ''): void
+	{
+		if (is_array($expected)) {
+			foreach ($expected as $key => $value) {
+				if ($value instanceof \DBObject) {
+					$expected[$key] = get_class($value).'::'.$value->GetKey();
+				}
+			}
+			foreach ($actual as $key => $value) {
+				if ($value instanceof \DBObject) {
+					$actual[$key] = get_class($value).'::'.$value->GetKey();
+				}
+			}
+		}
+		parent::assertEquals($expected, $actual, $message);
+	}
+
+	public function testAddMagicPlaceholdersWhenLoggedInUserHasAContact()
+	{
+		// Create data fixture => User + Person
+		$iNum = uniqid();
+		$sLogin = "AddMagicPlaceholders".$iNum;
+		$this->CreateTestOrganization();
+		$oPerson = $this->CreatePerson($iNum);
+		$sContactId = $oPerson->GetKey();
+		$oUser = $this->CreateUser($sLogin, 1, "Abcdef@12345678", $oPerson->GetKey());
+		UserRights::Login($sLogin);
+
+		// Test legacy behavior (no expected args)
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"]);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+				'current_contact_id' => $sContactId,
+				'current_user->object()' => $oUser,
+				'current_contact->object()' => $oPerson,
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders without second parameter (legacy) should add "curent_contact_id/current_user->object()/current_contact->object()"'
+		);
+
+		// With expected arguments explicitly given as "none"
+		$aExpectedArgs = [];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add only expected arguments'
+		);
+
+		// Test new behavior (with expected args)
+		$aExpectedArgs = [
+			new VariableExpression('current_user->login'),
+			new VariableExpression('current_user->not_existing_attribute'),
+			new VariableExpression('current_contact_id'),
+			new VariableExpression('current_contact->org_id'),
+			new VariableExpression('current_contact->not_existing_attribute'),
+		];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+				'current_contact_id' => $sContactId,
+				'current_user->object()' => $oUser,
+				'current_user->not_existing_attribute' => '(current_user->not_existing_attribute : cannot be resolved)',
+				'current_user->login' => $sLogin,
+				'current_contact->object()' => $oPerson,
+				'current_contact->org_id' => $oPerson->Get('org_id'),
+				'current_contact->not_existing_attribute' => '(current_contact->not_existing_attribute : cannot be resolved)',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add expected arguments and render them with an explicit error when the information could not be known'
+		);
+	}
+
+	public function testAddMagicPlaceholdersWhenLoggedInUserHasNoContact()
+	{
+		// Create data fixture => User without contact
+		$iNum = uniqid();
+		$sLogin = "AddMagicPlaceholders".$iNum;
+		$oUser = $this->CreateContactlessUser($sLogin, 1, "Abcdef@12345678");
+		UserRights::Login($sLogin);
+
+		// Test legacy behavior (no expected args)
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"]);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+				'current_contact_id' => 0,
+				'current_user->object()' => $oUser,
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders without second parameter (legacy) should add "current_contact_id=0/current_user->object()"'
+		);
+
+		// Test with expected arguments explicitly given as "none"
+		$aExpectedArgs = [];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add only expected arguments'
+		);
+
+		// Test with a few expected arguments, some of which being invalid attributes
+		$aExpectedArgs = [
+			new VariableExpression('current_user->login'),
+			new VariableExpression('current_user->not_existing_attribute'),
+			new VariableExpression('current_contact_id'),
+			new VariableExpression('current_contact->org_id'),
+			new VariableExpression('current_contact->not_existing_attribute'),
+		];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+				'current_contact_id' => 0,
+				'current_user->object()' => $oUser,
+				'current_user->not_existing_attribute' => '(current_user->not_existing_attribute : cannot be resolved)',
+				'current_user->login' => $sLogin,
+				'current_contact->object()' => '(current_contact->object() : cannot be resolved)',
+				'current_contact->org_id' => '(current_contact->org_id : cannot be resolved)',
+				'current_contact->not_existing_attribute' => '(current_contact->not_existing_attribute : cannot be resolved)',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add expected arguments and render them with an explicit error when the information could not be known'
+		);
+	}
+
+	public function testAddMagicPlaceholdersWhenThereIsNoLoggedInUser()
+	{
+		// Test legacy behavior (no expected args)
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"]);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+				'current_contact_id' => '',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders without second parameter (legacy) should add "curent_contact_id"'
+		);
+
+		// Test with expected arguments explicitly given as "none"
+		$aExpectedArgs = [];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add only expected arguments'
+		);
+
+		// Test with a few expected arguments, some of which being invalid attributes
+		$aExpectedArgs = [
+			new VariableExpression('current_user->login'),
+			new VariableExpression('current_user->not_existing_attribute'),
+			new VariableExpression('current_contact_id'),
+			new VariableExpression('current_contact->org_id'),
+			new VariableExpression('current_contact->not_existing_attribute'),
+		];
+		$aPlaceholders = MetaModel::AddMagicPlaceholders(['gabu' => "zomeu"], $aExpectedArgs);
+		$this->assertEqualsShallow(
+			[
+				'gabu' => 'zomeu',
+			    'current_contact_id' => '',
+				'current_user->object()' => '(current_user->object() : cannot be resolved)',
+			    'current_user->not_existing_attribute' => '(current_user->not_existing_attribute : cannot be resolved)',
+				'current_user->login' => '(current_user->login : cannot be resolved)',
+			    'current_contact->object()' => '(current_contact->object() : cannot be resolved)',
+				'current_contact->org_id' => '(current_contact->org_id : cannot be resolved)',
+			    'current_contact->not_existing_attribute' => '(current_contact->not_existing_attribute : cannot be resolved)',
+			],
+			$aPlaceholders,
+			'AddMagicPlaceholders should add expected arguments and render them with an explicit error when the information could not be known'
+		);
+	}
+}