N°6071 - Fix prefilled Tagset displayed (in transition or clone) but not saved (#751)

* N°6071 - Prefill Tagset in transition displayed but not saved

* Apply suggestions from code review

Co-authored-by: Molkobain <lajarige.guillaume@free.fr>

---------

Co-authored-by: Molkobain <lajarige.guillaume@free.fr>

application/cmdbabstract.class.inc.php

@@ -3983,6 +3983,27 @@ HTML;
 				$sTagSetJson = utils::ReadPostedParam("attr_{$sFormPrefix}{$sAttCode}", null, 'raw_data');
 				if ($sTagSetJson !== null) { // bulk modify, direct linked set not handled
 					$value = json_decode($sTagSetJson, true);
+					if ($this->IsNew()) {
+						if (is_array($value['orig_value'])) {
+							foreach ($value['orig_value'] as $val) {
+								if (!in_array($val, $value['removed'])) {
+									$value['added'][] = $val;
+								}
+							}
+						}
+					} else {
+						$aCurrentValues = $this->Get($sAttCode)->GetValues();
+						foreach ($value['orig_value'] as $val) {
+							if (!in_array($val, $aCurrentValues) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {
+								$value['added'][] = $val;
+							}
+						}
+						foreach ($aCurrentValues as $val) {
+							if (!in_array($val, $value['orig_value']) && !in_array($val, $value['removed']) && !in_array($val, $value['added'])) {
+								$value['removed'][] = $val;
+							}
+						}
+					}
 				}
 				break;
 

setup/runtimeenv.class.inc.php

@@ -24,6 +24,7 @@
  * @license     http://opensource.org/licenses/AGPL-3.0
  */
 
+use Combodo\iTop\Service\Session\SessionParameters;
 use Combodo\iTop\Setup\FeatureRemoval\SetupAudit;
 use Combodo\iTop\Setup\ModuleDependency\Module;
 use Combodo\iTop\Setup\ModuleDiscovery\ModuleFileReader;
@@ -351,27 +352,19 @@ class RunTimeEnvironment
 
 	/**
 	 * @param \Config $oConfig
-	 * @param string $sDataModelVersion
-	 * @param array $aSelectedModuleCodes
-	 * @param array $aSelectedExtensionCodes
-	 * @param string|null $sInstallComment
 	 *
 	 * @throws \CoreException
 	 * @throws \DictExceptionUnknownLanguage
 	 * @throws \MySQLException
 	 * @throws \Exception
 	 */
-	public function DoCreateConfig(Config $oConfig, string $sDataModelVersion, array $aSelectedModuleCodes, array $aSelectedExtensionCodes, ?string $sInstallComment = null, string $sSourceDesc = 'Setup')
+	public function DoCreateConfig(Config $oConfig, string $sSourceDesc = 'Setup')
 	{
 		$oConfig->Set('access_mode', ACCESS_FULL);
 
 		// Record which modules are installed...
 		$this->InitDataModel($oConfig, true);  // load data model and connect to the database
 
-		if (!$this->RecordInstallation($oConfig, $sDataModelVersion, $aSelectedModuleCodes, $aSelectedExtensionCodes, $sInstallComment)) {
-			throw new Exception('Failed to record the installation information');
-		}
-
 		$oConfig->UpdateIncludes('env-'.$this->sBuildEnv);
 		$sEnvironmentLabel = $this->GetFinalEnv().' (built on '.date('Y-m-d').')';
 		$oConfig->Set('app_env_label', $sEnvironmentLabel, $sSourceDesc);
@@ -649,9 +642,9 @@ class RunTimeEnvironment
 	public function RecordInstallation(Config $oConfig, $sDataModelVersion, $aSelectedModuleCodes, $aSelectedExtensionCodes, $sShortComment = null)
 	{
 		// Have it work fine even if the DB has been set in read-only mode for the users
-		$iPrevAccessMode = MetaModel::GetConfig()->Get('access_mode');
-		MetaModel::GetConfig()->Set('access_mode', ACCESS_FULL);
-		//$oConfig->Set('access_mode', ACCESS_FULL);
+		$iPrevAccessMode = $oConfig->Get('access_mode');
+		$oConfig->Set('access_mode', ACCESS_FULL);
+		$this->InitDataModel($oConfig, true);  // load data model and connect to the database
 
 		if (CMDBSource::DBName() == '') {
 			// In case this has not yet been done
@@ -755,6 +748,16 @@ class RunTimeEnvironment
 			}
 		}
 
+		$oParams = new SessionParameters(SetupUtils::SESSION_PARAMETERS_NAME);
+		if (class_exists('DesignerUpdate') && $oParams->GetParameter('return_application') === 'designer') {
+			// Now keep track of this update
+			$oLog = new DesignerUpdate();
+			$oLog->Set('revision_id', $oParams->GetParameter('revision_id', 0));
+			$oLog->Set('comment', $oParams->GetParameter('comment', ''));
+			$oLog->Set('compilation_date', time());
+			$oLog->DBInsert();
+		}
+
 		// Restore the previous access mode
 		MetaModel::GetConfig()->Set('access_mode', $iPrevAccessMode);
 

setup/sequencers/ApplicationInstallSequencer.php

@@ -48,6 +48,7 @@ class ApplicationInstallSequencer extends StepSequencer
 		'migrate-after' => 'Migrate data after database upgrade',
 		'after-db-create' => 'Load data after database create',
 		'load-data' => 'Loading data',
+		'record-installation' => 'Recording installation',
 		'create-config' => 'Creating the configuration File',
 		'commit' => 'Finalize',
 	];
@@ -59,6 +60,7 @@ class ApplicationInstallSequencer extends StepSequencer
 		'migrate-after' => 'Post-upgrade data migration completed',
 		'after-db-create' => 'Post-creation data loaded',
 		'load-data' => 'Data loaded',
+		'record-installation' => 'Installation recorded',
 		'create-config' => 'Configuration file created',
 	];
 
@@ -129,16 +131,22 @@ class ApplicationInstallSequencer extends StepSequencer
 					$this->oRunTimeEnvironment->DoLoadData($this->GetConfig(), $bSampleData, $aSelectedModules);
 					return $this->ComputeNextStep($sStep);
 
-				case 'create-config':
+				case 'record-installation':
 					$sDataModelVersion = $this->oParams->Get('datamodel_version', '0.0.0');
 					$aSelectedModuleCodes = $this->oParams->Get('selected_modules', []);
 					$aSelectedExtensionCodes = $this->oParams->Get('selected_extensions', []);
-					$this->oRunTimeEnvironment->DoCreateConfig(
+					$this->oRunTimeEnvironment->RecordInstallation(
 						$this->GetConfig(),
 						$sDataModelVersion,
 						$aSelectedModuleCodes,
 						$aSelectedExtensionCodes,
-						$sInstallComment,
+						$sInstallComment
+					);
+					return $this->ComputeNextStep($sStep);
+
+				case 'create-config':
+					$this->oRunTimeEnvironment->DoCreateConfig(
+						$this->GetConfig(),
 						$this->sSourceDesc
 					);
 					return $this->ComputeNextStep($sStep);
@@ -208,6 +216,7 @@ class ApplicationInstallSequencer extends StepSequencer
 		$aOthers = [
 			'after-db-create',
 			'load-data',
+			'record-installation',
 			'create-config',
 			'commit',
 		];

sources/Application/UI/Base/Component/Form/Form.php

@@ -25,7 +25,7 @@ class Form extends UIContentBlock
 	/** @var string */
 	protected $sAction;
 	/** @var string */
-	protected $sEncType = "application/x-www-form-urlencoded";
+	protected $sEncType = "multipart/form-data";
 
 	public function __construct(?string $sId = null)
 	{

tests/php-unit-tests/unitary-tests/setup/SetupUtilsTest.php

@@ -174,8 +174,9 @@ class SetupUtilsTest extends ItopTestCase
 		$this->RequireOnceItopFile('/setup/feature_removal/ModelReflectionSerializer.php');
 
 		$this->expectException(\CoreException::class);
-		$sDetails = sprintf('The current CLI PHP Version (%s) is lower than the minimum version required to run %s, which is (%s)', $sFoundVersion, ITOP_APPLICATION, SetupUtils::PHP_MIN_VERSION);
+		$sDetails = sprintf('The current CLI PHP Version (%s) is lower than the minimum version required to run %s, which is (%s). You may change the CLI PHP executable path by setting the "php_path" config parameter.', $sFoundVersion, ITOP_APPLICATION, SetupUtils::PHP_MIN_VERSION);
 
+		$this->expectException(\CoreException::class);
 		$this->expectExceptionMessage("Data consistency check failed: $sDetails");
 		$this->InvokeNonPublicStaticMethod(SetupUtils::class, 'CheckCliPhpVersionFromOutput', [ModelReflectionSerializer::ERROR_LABEL, 'sPHPExec', [$sOutput]]);
 	}

tests/php-unit-tests/unitary-tests/setup/sequencers/ApplicationInstallerSequencerTest.php

@@ -34,14 +34,14 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'db-schema',
 				'next-step-label' => 'Updating database schema',
 				'prev-step-success-message' => '',
-				'percentage-completed' => 16,
+				'percentage-completed' => 14,
 				'optional_steps' => [],
 			],
 			'next is log-parameters' => [
 				'next-step' => 'log-parameters',
 				'next-step-label' => 'Log parameters',
 				'prev-step-success-message' => '',
-				'percentage-completed' => 11,
+				'percentage-completed' => 10,
 				'optional_steps' => [
 					'log-parameters' => true,
 					'backup' => true,
@@ -52,7 +52,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'backup',
 				'next-step-label' => 'Performing a backup of the database',
 				'prev-step-success-message' => '',
-				'percentage-completed' => 12,
+				'percentage-completed' => 11,
 				'optional_steps' => [
 					'backup' => true,
 					'migrate-before' => true,
@@ -62,7 +62,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'migrate-before',
 				'next-step-label' => 'Migrate data before database upgrade',
 				'prev-step-success-message' => '',
-				'percentage-completed' => 14,
+				'percentage-completed' => 12,
 				'optional_steps' => [
 					'migrate-before' => true,
 				],
@@ -104,7 +104,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'next-step' => 'migrate-before',
 			'next-step-label' => 'Migrate data before database upgrade',
 			'prev-step-success-message' => 'Parameters logged',
-			'percentage-completed' => 22,
+			'percentage-completed' => 20,
 		];
 		$this->assertEquals($aExpected, $aRes);
 	}
@@ -116,7 +116,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'db-schema',
 				'next-step-label' => 'Updating database schema',
 				'prev-step-success-message' => 'Database backup completed',
-				'percentage-completed' => 28,
+				'percentage-completed' => 25,
 				'optional_steps' => [
 					'backup' => true,
 				],
@@ -125,7 +125,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'migrate-before',
 				'next-step-label' => 'Migrate data before database upgrade',
 				'prev-step-success-message' => 'Database backup completed',
-				'percentage-completed' => 25,
+				'percentage-completed' => 22,
 				'optional_steps' => [
 					'backup' => true,
 					'migrate-before' => true,
@@ -182,7 +182,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'next-step' => 'db-schema',
 			'next-step-label' => 'Updating database schema',
 			'prev-step-success-message' => 'Pre-upgrade data migration completed',
-			'percentage-completed' => 28,
+			'percentage-completed' => 25,
 		];
 		$this->assertEquals($aExpected, $aRes);
 	}
@@ -194,7 +194,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'migrate-after',
 				'next-step-label' => 'Migrate data after database upgrade',
 				'prev-step-success-message' => 'Database schema updated',
-				'percentage-completed' => 28,
+				'percentage-completed' => 25,
 				'optional_steps' => [
 					'migrate-after' => true,
 				],
@@ -203,7 +203,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 				'next-step' => 'after-db-create',
 				'next-step-label' => 'Load data after database create',
 				'prev-step-success-message' => 'Database schema updated',
-				'percentage-completed' => 33,
+				'percentage-completed' => 28,
 				'optional_steps' => [],
 			],
 		];
@@ -256,7 +256,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'next-step' => 'after-db-create',
 			'next-step-label' => 'Load data after database create',
 			'prev-step-success-message' => 'Post-upgrade data migration completed',
-			'percentage-completed' => 42,
+			'percentage-completed' => 37,
 		];
 		$this->assertEquals($aExpected, $aRes);
 	}
@@ -284,7 +284,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'next-step' => 'load-data',
 			'next-step-label' => 'Loading data',
 			'prev-step-success-message' => 'Post-creation data loaded',
-			'percentage-completed' => 66,
+			'percentage-completed' => 60,
 		];
 		$this->assertEquals($aExpected, $aRes);
 	}
@@ -303,15 +303,39 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 		$aRes = $this->oSequencer->ExecuteStep('load-data');
 		$aExpected = [
 			'message' => '',
-			'next-step' => 'create-config',
-			'next-step-label' => 'Creating the configuration File',
+			'next-step' => 'record-installation',
+			'next-step-label' => 'Recording installation',
 			'prev-step-success-message' => 'Data loaded',
-			'percentage-completed' => 77,
+			'percentage-completed' => 70,
 			'status' => 1,
 		];
 		$this->assertEquals($aExpected, $aRes);
 	}
 
+	public function testRecordInstallation()
+	{
+		$aAdditionalParams = [
+			'datamodel_version'   => '6.6.6',
+			'selected_extensions' => ['c' => 'd'],
+			'selected_modules'    => ['a' => 'b'],
+			'sample_data'         => 1,
+		];
+		$this->GivenApplicationInstallSequencer($aAdditionalParams);
+
+		$this->oRunTimeEnvironment->expects($this->once())->method('RecordInstallation')
+			->with($this->oConfig, true, ['a' => 'b']);
+
+		$aRes = $this->oSequencer->ExecuteStep('record-installation');
+		$aExpected = [
+			'message'                   => '',
+			'next-step'                 => 'create-config',
+			'next-step-label' => 'Creating the configuration File',
+			'prev-step-success-message' => 'Installation recorded',
+			'percentage-completed'      => 80,
+			'status'                    => 1,
+		];
+		$this->assertEquals($aExpected, $aRes);
+	}
 	public function testCreateConfig()
 	{
 		$aAdditionalParams = [
@@ -322,7 +346,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 		];
 		$this->GivenApplicationInstallSequencer($aAdditionalParams);
 		$this->oRunTimeEnvironment->expects($this->once())->method('DoCreateConfig')
-			->with($this->oConfig, "6.6.6", ["a" => "b"], ["c" => "d"], null);
+			->with($this->oConfig, 'Setup');
 
 		$aRes = $this->oSequencer->ExecuteStep('create-config');
 		$aExpected = [
@@ -330,7 +354,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'next-step' => 'commit',
 			'next-step-label' => 'Finalize',
 			'prev-step-success-message' => 'Configuration file created',
-			'percentage-completed' => 88,
+			'percentage-completed' => 90,
 			'status' => 1,
 		];
 		$this->assertEquals($aExpected, $aRes);
@@ -433,6 +457,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'migrate-after',
 			'after-db-create',
 			'load-data',
+			'record-installation',
 			'create-config',
 			'commit',
 		];
@@ -454,6 +479,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'migrate-after',
 			'after-db-create',
 			'load-data',
+			'record-installation',
 			'create-config',
 			'commit',
 		];
@@ -494,6 +520,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'migrate-after' => true,
 			'after-db-create' => true,
 			'load-data' => true,
+			'record-installation' => true,
 			'create-config' => true,
 			'commit' => true,
 		];
@@ -512,6 +539,7 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 			'db-schema',
 			'after-db-create',
 			'load-data',
+			'record-installation',
 			'create-config',
 			'commit',
 		];
@@ -521,11 +549,12 @@ class ApplicationInstallerSequencerTest extends ItopTestCase
 	public function testGetStepAfterWithPercent()
 	{
 		$this->GivenApplicationInstallSequencer([], true);
-		$this->assertEquals(['log-parameters', 11], $this->oSequencer->GetStepAfterWithPercent(''));
-		$this->assertEquals(['migrate-after', 44], $this->oSequencer->GetStepAfterWithPercent('db-schema'));
-		$this->assertEquals(['load-data', 66], $this->oSequencer->GetStepAfterWithPercent('after-db-create'));
-		$this->assertEquals(['create-config', 77], $this->oSequencer->GetStepAfterWithPercent('load-data'));
-		$this->assertEquals(['commit', 88], $this->oSequencer->GetStepAfterWithPercent('create-config'));
+		$this->assertEquals(['log-parameters', 10], $this->oSequencer->GetStepAfterWithPercent(''));
+		$this->assertEquals(['migrate-after', 40], $this->oSequencer->GetStepAfterWithPercent('db-schema'));
+		$this->assertEquals(['load-data', 60], $this->oSequencer->GetStepAfterWithPercent('after-db-create'));
+		$this->assertEquals(['record-installation', 70], $this->oSequencer->GetStepAfterWithPercent('load-data'));
+		$this->assertEquals(['create-config', 80], $this->oSequencer->GetStepAfterWithPercent('record-installation'));
+		$this->assertEquals(['commit', 90], $this->oSequencer->GetStepAfterWithPercent('create-config'));
 		$this->assertEquals(['', 100], $this->oSequencer->GetStepAfterWithPercent('commit'));
 	}