N°9553 - Refactor methods to be clearer on what it does

datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php

@@ -26,7 +26,6 @@ use Dict;
 use Exception;
 use MetaModel;
 use RunTimeEnvironment;
-use SecurityException;
 use SetupUtils;
 use utils;
 
@@ -84,11 +83,10 @@ class DataFeatureRemovalController extends Controller
 	{
 		$aParams = [];
 
-		try {
+		if (SetupUtils::IsSessionSetupTokenValid()) {
 			//from setup wizard/mtp
-			SetupUtils::CheckSetupToken();
 			SetupUtils::EraseSetupToken();
-		} catch (SecurityException $e) {
+		} else {
 			//from same module
 			$this->ValidateTransactionId();
 		}
@@ -102,8 +100,6 @@ class DataFeatureRemovalController extends Controller
 			'removed_extensions' => '[]',
 			'extensions_not_uninstallable' => '[]',
 			'copy_setup_files' => 1,
-			'return_button_label' => '',
-			'return_button_url' => '',
 		];
 
 		$aHiddenInputs = [];

datamodels/2.x/combodo-data-feature-removal/templates/AnalysisResult.html.twig

@@ -87,15 +87,11 @@
         {% endif %}
     {% endif %}
 
-    {% if aHiddenInputs.return_button_label != '' %}
-        {% UIButton ForAlternativeSecondaryAction { sLabel:aHiddenInputs.return_button_label, OnClickJsCode: 'window.location.href="' ~ aHiddenInputs.return_button_url ~ '"'} %}
-    {% else %}
-        {% UIForm Standard {} %}
-            {% UIInput ForHidden { sName:'transaction_id', sValue:sTransactionId} %}
-            {% UIInput ForHidden { sName:'operation', sValue:'Main'} %}
-            {% UIToolbar ForButton {} %}
-                {% UIButton ForPrimaryAction {sLabel:'UI:Button:BackToMain'|dict_s, sName:'btn_back', sId:'btn_back', bIsSubmit:true} %}
-            {% EndUIToolbar %}
-        {% EndUIForm %}
-    {% endif %}
+    {% UIForm Standard {} %}
+        {% UIInput ForHidden { sName:'transaction_id', sValue:sTransactionId} %}
+        {% UIInput ForHidden { sName:'operation', sValue:'Main'} %}
+        {% UIToolbar ForButton {} %}
+            {% UIButton ForPrimaryAction {sLabel:'UI:Button:BackToMain'|dict_s, sName:'btn_back', sId:'btn_back', bIsSubmit:true} %}
+        {% EndUIToolbar %}
+    {% EndUIForm %}
 {% EndUIPanel %}

datamodels/2.x/itop-config-mgmt/module.itop-config-mgmt.php

@@ -26,7 +26,6 @@ SetupWebPage::AddModule(
 		],
 		'data.struct' => [
 			'data/en_us.data.itop-brand.xml',
-			'data/en_us.data.itop-networkdevicetype.xml',
 			'data/en_us.data.itop-osfamily.xml',
 			'data/en_us.data.itop-osversion.xml',
 		],
@@ -102,6 +101,8 @@ if (!class_exists('ConfigMgmtInstaller')) {
 		 */
 		public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
 		{
+			// load localized data for NetworkDeviceType
+			static::LoadLocalizedDataOnCrossingVersion($oConfiguration,	$sPreviousVersion,	$sCurrentVersion,'3.3.0',__DIR__."/data/{{language_code}}.data.itop-networkdevicetype.xml"	);
 		}
 	}
 }

datamodels/2.x/itop-service-mgmt-provider/module.itop-service-mgmt-provider.php

@@ -88,13 +88,7 @@ if (!class_exists('ServiceMgmtProviderInstaller')) {
 		public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
 		{
 			// Load localized structural data: contract types
-			static::LoadLocalizedData(
-				$oConfiguration,
-				$sPreviousVersion,
-				$sCurrentVersion,
-				'3.3.0',
-				__DIR__."/data/{{language_code}}.data.itop-contracttype.xml"
-			);
+			static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contracttype.xml");
 		}
 	}
 }

datamodels/2.x/itop-service-mgmt/module.itop-service-mgmt.php

@@ -85,13 +85,7 @@ if (!class_exists('ServiceMgmtInstaller')) {
 		public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
 		{
 			// Load localized structural data: contact types and document types
-			static::LoadLocalizedData(
-				$oConfiguration,
-				$sPreviousVersion,
-				$sCurrentVersion,
-				'3.3.0',
-				__DIR__."/data/{{language_code}}.data.itop-contracttype.xml"
-			);
+			static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contracttype.xml");
 		}
 	}
 }

datamodels/2.x/itop-structure/module.itop-structure.php

@@ -100,20 +100,8 @@ if (!class_exists('StructureInstaller')) {
 		public static function AfterDatabaseCreation(Config $oConfiguration, $sPreviousVersion, $sCurrentVersion)
 		{
 			// Load localized structural data: contact types and document types
-			static::LoadLocalizedData(
-				$oConfiguration,
-				$sPreviousVersion,
-				$sCurrentVersion,
-				'3.3.0',
-				__DIR__."/data/{{language_code}}.data.itop-contacttype.xml"
-			);
-			static::LoadLocalizedData(
-				$oConfiguration,
-				$sPreviousVersion,
-				$sCurrentVersion,
-				'3.3.0',
-				__DIR__."/data/{{language_code}}.data.itop-documenttype.xml"
-			);
+			static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-contacttype.xml");
+			static::LoadLocalizedDataOnNewInstall($oConfiguration, $sPreviousVersion, __DIR__."/data/{{language_code}}.data.itop-documenttype.xml");
 
 			// Default language will be used for actions
 			// Note: There is a issue when upgrading, default language cannot be retrieved from the passed configuration, we have to read it from the disk

datamodels/2.x/itop-tickets/module.itop-tickets.php

@@ -61,6 +61,6 @@ class TicketsInstaller extends ModuleInstallerAPI
 			}
 		}
 		// Load localized structural data: predefined query phrases for notifications
-		static::LoadLocalizedData($oConfiguration, $sPreviousVersion, $sCurrentVersion, '3.0.0', __DIR__."/data/{{language_code}}.data.itop-tickets.xml");
+		static::LoadLocalizedDataOnCrossingVersion($oConfiguration, $sPreviousVersion, $sCurrentVersion, '3.0.0', __DIR__."/data/{{language_code}}.data.itop-tickets.xml");
 	}
 }

setup/moduleinstaller.class.inc.php

@@ -311,7 +311,7 @@ abstract class ModuleInstallerAPI
 
 	/**
 	 * @param \Config $oConfiguration
-	 * @param string $sPreviousVersion The previous version of the module (empty string will force the loading)
+	 * @param string $sPreviousVersion The previous version of the module (empty string in case of first install)
 	 * @param string $sCurrentVersion The current version of the module
 	 * @param string $sFirstLoadingVersion The first module version for which the data loading should be performed (e.g. '3.0.0')
 	 * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml')
@@ -321,74 +321,51 @@ abstract class ModuleInstallerAPI
 	 * @throws \CoreException
 	 * @throws \CoreUnexpectedValue
 	 */
-	public static function LoadLocalizedData(Config $oConfiguration, string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void
+	public static function LoadLocalizedDataOnCrossingVersion(Config $oConfiguration, ?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void
 	{
-		self::AssertLoadLocalizedDataParametersAreValid($sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sFilePattern);
+		self::AssertLoadLocalizedDataParametersAreValid($sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion);
 
-		// It's not very clear if it makes sense to test a particular version,
-		// as the loading mechanism checks object existence using reconc_keys
-		// and do not recreate them, nor update existing.
-		// Without test, new entries added to the data files, would be automatically loaded
+		// The loading is done only if
+		// - it's a first install of the module
+		// - or it's an upgrade of that module (PreviousVersion is less than the CurrentVersion), which means that we are really upgrading (and not reinstalling the same version or downgrading), and
+		//    - either the FirstLoadingVersion is between the PreviousVersion and the CurrentVersion
+		//    - or the FirstLoadingVersion is empty, forcing the loading on all upgrades,
 		if (($sPreviousVersion === '') ||
 			(version_compare($sPreviousVersion, $sCurrentVersion, '<')
-				&& version_compare($sPreviousVersion, $sFirstLoadingVersion, '<'))) {
+		&& version_compare($sPreviousVersion, $sFirstLoadingVersion, '<')
+		&& version_compare($sFirstLoadingVersion, $sCurrentVersion, '<='))) {
 
-			// Note: There is an issue when upgrading, default language cannot be retrieved from the passed configuration, we have to read it from the disk
-			if (utils::IsNullOrEmptyString($sPreviousVersion)) {
-				// Fresh install
-				$sDefaultLanguage = $oConfiguration->GetDefaultLanguage();
-			} else {
-				// Upgrade
-				$sDefaultLanguage = utils::GetConfig(true)->GetDefaultLanguage();
-			}
-
-			$sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern);
-			if ($sFileName !== '') {
-				self::XMLFileLoad($sFileName);
-			}
+			self::LoadLocalizedData($oConfiguration, $sFilePattern);
+		}
+	}
+	/**
+	* @param \Config $oConfiguration
+	* @param string $sPreviousVersion The previous version of the module (empty string in case of first install)
+	 * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml')
+	 *
+	 * @return void
+	*/
+	public static function LoadLocalizedDataOnNewInstall(Config $oConfiguration, ?string $sPreviousVersion, string $sFilePattern): void
+	{
+		if (utils::IsNullOrEmptyString($sPreviousVersion)) {
+			self::LoadLocalizedData($oConfiguration, $sFilePattern);
 		}
 	}
 
 	/**
-	 * @throws \CoreUnexpectedValue
-	 */
-	private static function AssertLoadLocalizedDataParametersAreValid(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sFilePattern): void
-	{
-		if (($sPreviousVersion !== '') && !self::IsValidLocalizedDataVersion($sPreviousVersion)) {
-			throw new CoreUnexpectedValue("LoadLocalizedData expects sPreviousVersion to be empty or match x.y[.z][-name], got '{$sPreviousVersion}'");
-		}
-
-		if (!self::IsValidLocalizedDataVersion($sCurrentVersion)) {
-			throw new CoreUnexpectedValue("LoadLocalizedData expects sCurrentVersion to match x.y[.z][-name], got '{$sCurrentVersion}'");
-		}
-
-		if (!self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) {
-			throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'");
-		}
-
-		if (utils::IsNullOrEmptyString($sFilePattern)) {
-			throw new CoreUnexpectedValue('LoadLocalizedData expects sFilePattern to be a non-empty string');
-		}
-
-		if (substr_count($sFilePattern, '{{language_code}}') !== 1) {
-			throw new CoreUnexpectedValue("LoadLocalizedData expects sFilePattern to contain the exact placeholder '{{language_code}}' exactly once");
-		}
-	}
-
-	private static function IsValidLocalizedDataVersion(string $sVersion): bool
-	{
-		return (preg_match('/^\d+\.\d+(?:\.\d+)?(?:-[A-Za-z0-9]+)?$/', $sVersion) === 1);
-	}
-
-	/**
-	 * @param array|string $sFileName
-	 * @param \XMLDataLoader $oDataLoader
+	 * @param \Config $oConfiguration
+	 * @param string $sFilePattern The pattern of the file to load, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml')
 	 *
 	 * @return void
 	 * @throws \Exception
 	 */
-	public static function XMLFileLoad(string $sFileName): void
+	protected static function LoadLocalizedData(Config $oConfiguration, string $sFilePattern): void
 	{
+		if (substr_count($sFilePattern, '{{language_code}}') !== 1) {
+			throw new CoreUnexpectedValue("LoadLocalizedData expects $sFilePattern to contain the exact placeholder '{{language_code}}' exactly once");
+		}
+		$sDefaultLanguage = $oConfiguration->GetDefaultLanguage();
+		$sFileName = self::GetLocalizedFileName($sDefaultLanguage, $sFilePattern);
 		if (!file_exists($sFileName)) {
 			throw new Exception("File $sFileName not found");
 		}
@@ -401,19 +378,39 @@ abstract class ModuleInstallerAPI
 		$oDataLoader->EndSession();
 	}
 
+	/**
+	 * @throws \CoreUnexpectedValue
+	 */
+	private static function AssertLoadLocalizedDataParametersAreValid(?string $sPreviousVersion, ?string $sCurrentVersion, string $sFirstLoadingVersion): void
+	{
+		if (($sPreviousVersion !== '') && !self::IsValidLocalizedDataVersion($sPreviousVersion)) {
+			throw new CoreUnexpectedValue("LoadLocalizedData expects sPreviousVersion to be empty or match x.y[.z][-name], got '{$sPreviousVersion}'");
+		}
+		if (!self::IsValidLocalizedDataVersion($sCurrentVersion)) {
+			throw new CoreUnexpectedValue("LoadLocalizedData expects sCurrentVersion to match x.y[.z][-name], got '{$sCurrentVersion}'");
+		}
+		if (($sFirstLoadingVersion !== '') && !self::IsValidLocalizedDataVersion($sFirstLoadingVersion)) {
+			throw new CoreUnexpectedValue("LoadLocalizedData expects sFirstLoadingVersion to match x.y[.z][-name], got '{$sFirstLoadingVersion}'");
+		}
+	}
+
+	private static function IsValidLocalizedDataVersion(string $sVersion): bool
+	{
+		return (preg_match('/^\d+\.\d+(?:\.\d+)?(?:-[A-Za-z0-9]+)?$/', $sVersion) === 1);
+	}
+
 	/**
 	 * @param string $sLanguage The language code to use for localization (e.g. 'EN US')
 	 * @param string $sFilePattern The full path+name of the file to localize, with {{language_code}} as placeholder for the language code (e.g. 'data.sample.{{language_code}}.xml')
 	 *
 	 * @return string The localized file name if found, or an empty string if not found
-	 * @throws \ConfigException
-	 * @throws \CoreException
 	 */
-	public static function GetLocalizedFileName($sLanguage, string $sFilePattern): string
+	private static function GetLocalizedFileName($sLanguage, string $sFilePattern): string
 	{
 		$sLang = str_replace(' ', '_', strtolower($sLanguage));
 		$sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern);
 		if (!file_exists($sFileName)) {
+			SetupLog::Debug("No data file found matching the pattern $sFilePattern and language_code $sLang. Trying with 'en_us' as fallback.");
 			$sLang = 'en_us';
 			$sFileName = str_replace('{{language_code}}', $sLang, $sFilePattern);
 		}

setup/setup.js

@@ -26,17 +26,17 @@ function WizardAsyncAction(sActionCode, oParams, OnErrorFunction)
 function WizardUpdateButtons()
 {
 	if (CanMoveForward()) {
-		$("#btn_next").removeClass('ibo-is-hidden');
+		$("#btn_next").prop('disabled', false);
 	}
 	else {
-		$("#btn_next").addClass('ibo-is-hidden');
+		$("#btn_next").prop('disabled', true);
 	}
 
 	if (CanMoveBackward()) {
-		$("#btn_back").removeClass('ibo-is-hidden');
+		$("#btn_back").prop('disabled', false);
 	}
 	else {
-		$("#btn_back").addClass('ibo-is-hidden');
+		$("#btn_back").prop('disabled', true);
 	}
 }
 

setup/wizardcontroller.class.inc.php

@@ -151,10 +151,10 @@ class WizardController
 		$sCurrentState = utils::ReadParam('_state', $this->sInitialState);
 		$oStep = $this->GetWizardStep($sCurrentStepClass, $sCurrentState);
 		if ($oStep->ValidateParams()) {
-			$aPossibleSteps = $oStep->GetPossibleSteps();
-			if ($oStep->CanMoveBackward()) {
+			if ($oStep->CanComeBack()) {
 				$this->PushStep(['class' => $sCurrentStepClass, 'state' => $sCurrentState]);
 			}
+			$aPossibleSteps = $oStep->GetPossibleSteps();
 			$oWizardState = $oStep->UpdateWizardStateAndGetNextStep(true); // true => moving forward
 			if (in_array($oWizardState->GetNextStep(), $aPossibleSteps)) {
 				$oNextStep = $this->GetWizardStep($oWizardState->GetNextStep(), $oWizardState->GetState());
@@ -237,12 +237,8 @@ HTML;
 
 		$oPage->add('<input type="hidden" name="_steps" value="'.utils::EscapeHtml(json_encode($this->aWizardSteps)).'"/>');
 		$oPage->add('<table style="width:100%;" class="ibo-setup--wizard--buttons-container"><tr>');
-		if (count($this->aWizardSteps) > 0) {
-			if ($oStep->CanMoveBackward()) {
-				$oPage->add('<td style="text-align: left"><button id="btn_back" class="ibo-button ibo-is-alternative ibo-is-neutral" type="submit" name="operation" value="back"><span class="ibo-button--label">Back</span></button></td>');
-			} else {
-				$oPage->add('<td style="text-align: left"><button id="btn_back" class="ibo-button ibo-is-alternative ibo-is-neutral ibo-is-hidden" type="submit" name="operation" value="back"><span class="ibo-button--label">Back</span></button></td>');
-			}
+		if ((count($this->aWizardSteps) > 0) && ($oStep->CanMoveBackward())) {
+			$oPage->add('<td style="text-align: left"><button id="btn_back" class="ibo-button ibo-is-alternative ibo-is-neutral" type="submit" name="operation" value="back"><span class="ibo-button--label">Back</span></button></td>');
 		}
 		if ($oStep->CanMoveForward()) {
 			$oPage->add('<td style="text-align:right;"><button id="btn_next" class="default ibo-button ibo-is-regular ibo-is-primary" type="submit" name="operation" value="next"><span class="ibo-button--label">'.utils::EscapeHtml($oStep->GetNextButtonLabel()).'</span></button></td>');

setup/wizardsteps/WizStepDataAudit.php

@@ -54,20 +54,16 @@ class WizStepDataAudit extends WizStepInstall
 		}
 	}
 
-	/**
-	 * Tells whether this step/state allows to go back or not
-	 * @return boolean True if the '<< Back' button should be displayed
-	 */
-	public function CanMoveBackward()
-	{
-		return false;
-	}
-
 	public function UpdateWizardStateAndGetNextStep($bMoveForward = true): WizardState
 	{
 		return new WizardState(WizStepSummary::class);
 	}
 
+	public function CanComeBack()
+	{
+		return false;
+	}
+
 	public function Display(SetupPage $oPage): void
 	{
 
@@ -112,8 +108,6 @@ JS);
 			'removed_extensions' => '[]',
 			'extensions_not_uninstallable' => '[]',
 			'copy_setup_files' => 1,
-			'return_button_label' => '',
-			'return_button_url' => '',
 		];
 		$aHiddenInputs = '';
 		foreach ($aParams as $sParamName => $defaultValue) {
@@ -129,42 +123,23 @@ INPUT;
 			<<<HTML
 <form id="data-feature-removal" class="ibo-setup--wizard ibo-is-hidden" method="post" action="$sApplicationUrl">
 	<input type="hidden" name="operation" value="AnalysisResult"/>
-	<input type="hidden" name="authent" value="$sUID"/>
+	<input type="hidden" name="setup_token" value="$sUID"/>
 	$aHiddenInputs
 </form>
 HTML
 		);
-
-		$sButtonLabel = $this->oWizard->GetParameter('return_button_label', '');
-		SetupLog::Info(__METHOD__.": return_button_label [$sButtonLabel]");
-		$sButtonUrl = $this->oWizard->GetParameter('return_button_url', '');
-		if ($sButtonLabel !== '') {
-			$oPage->add_ready_script(
-				<<<JS
-	$('.ibo-setup--wizard--buttons-container tr td:nth-child(1)').after('<td style="text-align:center;"><button id="return-button" class="ibo-button ibo-is-alternative ibo-is-neutral ibo-is-hidden" type="button" onclick="window.location.href=\'$sButtonUrl\'"><span class="ibo-button--label">$sButtonLabel</span></button></td>');
-JS
-			);
-		}
 	}
 
 	protected function AddProgressErrorScript($oPage, $aRes)
 	{
-		$oPage->add_ready_script(
-			<<<JS
-    if ($('#return-button').length > 0) {
-		$('#return-button').removeClass('ibo-is-hidden');
-    }
-JS
-		);
-
 		if (isset($aRes['error_code']) && $aRes['error_code'] === DataAuditSequencer::DATA_AUDIT_FAILED) {
 			$oPage->add_ready_script(
-				<<<JS
-	$('.ibo-setup--wizard--buttons-container tr td:nth-child(2)').after('<td style="text-align:center;"><button class="ibo-button ibo-is-alternative ibo-is-neutral" type="submit" name="operation" value="next" id="ignore_and_continue"><span class="ibo-button--label">Ignore and continue</span></button></td>');
+				<<<EOF
+	$('.ibo-setup--wizard--buttons-container tr td:nth-child(2)').before('<td style="text-align:center;"><button class="ibo-button ibo-is-alternative ibo-is-neutral" type="submit" name="operation" value="next" id="ignore_and_continue"><span class="ibo-button--label">Ignore and continue</span></button></td>');
 	$('#ignore_and_continue').on('click', function() {
 		return confirm("If you skip the cleanup you won't be able to run the process later. You'll have to migrate or delete unconsistent data manually.");
 	});
-	$('.ibo-setup--wizard--buttons-container tr td:nth-child(3)').after('<td style="text-align:center;"><span id="submit-wait" class="ibo-spinner ibo-is-inline ibo-is-hidden ibo-spinner ibo-block" data-role="ibo-spinner"><i class="ibo-spinner--icon fas fa-sync-alt fa-spin" aria-hidden="true"></i></span>&nbsp;<button id="goto-data-feature-removal" class="default ibo-button ibo-is-regular ibo-is-primary" type="button"><span class="ibo-button--label">Cleanup my data</span></button></td>');
+	$('.ibo-setup--wizard--buttons-container tr td:nth-child(2)').after('<td style="text-align:center;"><span id="submit-wait" class="ibo-spinner ibo-is-inline ibo-is-hidden ibo-spinner ibo-block" data-role="ibo-spinner"><i class="ibo-spinner--icon fas fa-sync-alt fa-spin" aria-hidden="true"></i></span>&nbsp;<button id="goto-data-feature-removal" class="default ibo-button ibo-is-regular ibo-is-primary" type="button"><span class="ibo-button--label">Cleanup my data</span></button></td>');
 	$('#goto-data-feature-removal').on("click", function() {
 		$('#goto-data-feature-removal').prop('disabled', true);
 		$('#submit-wait').removeClass("ibo-is-hidden");
@@ -173,7 +148,7 @@ JS
 	
 	$("#wiz_form").data("installation_status", "cleanup_needed");
 	$('#btn_next').hide();
-JS
+EOF
 			);
 		}
 
@@ -186,10 +161,6 @@ JS
 
 	public function JSCanMoveBackward()
 	{
-		if ($this->oWizard->GetParameter('return_button_label', '') !== '') {
-			return 'return false;';
-		}
-
 		return 'return  ["not started", "error", "cleanup_needed"].indexOf($("#wiz_form").data("installation_status")) !== -1;';
 	}
 }

setup/wizardsteps/WizStepDone.php

@@ -144,11 +144,6 @@ class WizStepDone extends WizardStep
 		return false;
 	}
 
-	public function JSCanMoveBackward()
-	{
-		return 'return false;';
-	}
-
 	/**
 	 * Tells whether this step of the wizard requires that the configuration file be writable
 	 * @return bool True if the wizard will possibly need to modify the configuration at some point

setup/wizardsteps/WizStepInstall.php

@@ -54,13 +54,6 @@ class WizStepInstall extends AbstractWizStepInstall
 		}
 	}
 
-	public function CanMoveBackward()
-	{
-		$sLabel = $this->oWizard->GetParameter('return_button_label', '');
-		SetupLog::Info(__METHOD__.": return_button_label [$sLabel]");
-		return $sLabel === '';
-	}
-
 	public function UpdateWizardStateAndGetNextStep($bMoveForward = true): WizardState
 	{
 		return new WizardState(WizStepDone::class);
@@ -116,20 +109,6 @@ JS);
 JS);
 	}
 
-	public function PostFormDisplay(SetupPage $oPage)
-	{
-		$sButtonLabel = $this->oWizard->GetParameter('return_button_label', '');
-		SetupLog::Info(__METHOD__.": return_button_label [$sButtonLabel]");
-		$sButtonUrl = $this->oWizard->GetParameter('return_button_url', '');
-		if ($sButtonLabel !== '') {
-			$oPage->add_ready_script(
-				<<<JS
-	$('.ibo-setup--wizard--buttons-container tr td:nth-child(1)').after('<td style="text-align:center;"><button id="return-button" class="ibo-button ibo-is-alternative ibo-is-neutral ibo-is-hidden" type="button" onclick="window.location.href=\'$sButtonUrl\'"><span class="ibo-button--label">$sButtonLabel</span></button></td>');
-JS
-			);
-		}
-	}
-
 	/**
 	 * @throws \Exception
 	 */
@@ -146,25 +125,27 @@ JS
 			// Tell the web page to move the progress bar and to launch the next step
 			$sMessage = addslashes(utils::EscapeHtml($aRes['next-step-label']));
 			$oPage->add_ready_script(
-				<<<JS
-				$("#wiz_form").data("installation_status", "running");
-				WizardUpdateButtons();
-				$('#setup_msg').html('$sMessage');
-				$('#progress').progression( {Current:{$aRes['percentage-completed']}, Maximum: 100} );
-				
-				//$("#percentage").html('{$aRes['percentage-completed']} % completed<br/>{$aRes['next-step-label']}');
-				ExecuteStep('{$aRes['next-step']}');
-JS
+				<<<EOF
+	$("#wiz_form").data("installation_status", "running");
+	WizardUpdateButtons();
+	$('#setup_msg').html('$sMessage');
+	$('#progress').progression( {Current:{$aRes['percentage-completed']}, Maximum: 100} );
+	
+	//$("#percentage").html('{$aRes['percentage-completed']} % completed<br/>{$aRes['next-step-label']}');
+	ExecuteStep('{$aRes['next-step']}');
+EOF
 			);
 			static::AddPrevStepSuccessMessage($oPage, $aRes['prev-step-success-message']);
 		} elseif ($aRes['status'] !== StepSequencer::ERROR) {
 			// Installation complete, move to the next step of the wizard
 			$oPage->add_ready_script(
-				<<<JS
-				$('#progress').progression( {Current:100, Maximum: 100} );
-				$("#wiz_form").data("installation_status", "completed");
-				$("#btn_next").trigger('click');
-JS
+				<<<EOF
+	$("#wiz_form").data("installation_status", "completed");
+	$('#progress').progression( {Current:100, Maximum: 100} );
+	WizardUpdateButtons();
+	$("#btn_next").off("click.install");
+	$("#btn_next").trigger('click');
+EOF
 			);
 			static::AddPrevStepSuccessMessage($oPage, $aRes['prev-step-success-message']);
 		} else {
@@ -172,12 +153,12 @@ JS
 			$sMessage = addslashes(utils::EscapeHtml($aRes['message']));
 			$sMessage = str_replace("\n", '<br>', $sMessage);
 			$oPage->add_ready_script(
-				<<<JS
-				$("#wiz_form").data("installation_status", "error");
-				$("#progress .progress").addClass('progress-error');
-				WizardUpdateButtons();
-				$('#setup_error').html('$sMessage').show();
-JS
+				<<<EOF
+	$("#wiz_form").data("installation_status", "error");
+	$("#progress .progress").addClass('progress-error');
+	WizardUpdateButtons();
+	$('#setup_error').html('$sMessage').show();
+EOF
 			);
 			$this->AddProgressErrorScript($oPage, $aRes);
 		}
@@ -185,13 +166,7 @@ JS
 
 	protected function AddProgressErrorScript($oPage, $aRes)
 	{
-		$oPage->add_ready_script(
-			<<<JS
-    if ($('#return-button').length > 0) {
-		$('#return-button').removeClass('ibo-is-hidden');
-    }
-JS
-		);
+
 	}
 
 	/**

setup/wizardsteps/WizStepLandingBeforeAudit.php

@@ -84,7 +84,7 @@ class WizStepLandingBeforeAudit extends WizStepModulesChoice
 		return 'Next';
 	}
 
-	public function CanMoveBackward()
+	public function CanComeBack()
 	{
 		return false;
 	}

setup/wizardsteps/WizStepSummary.php

@@ -57,14 +57,6 @@ class WizStepSummary extends AbstractWizStepInstall
 		}
 	}
 
-	public function CanMoveBackward()
-	{
-		$sLabel = $this->oWizard->GetParameter('return_button_label', '');
-		SetupLog::Info(__METHOD__.": return_button_label [$sLabel]");
-
-		return $sLabel === '';
-	}
-
 	public function UpdateWizardStateAndGetNextStep($bMoveForward = true): WizardState
 	{
 		$this->oWizard->SaveParameter('db_backup', false);
@@ -250,19 +242,6 @@ JS
 		);
 	}
 
-	public function PostFormDisplay(SetupPage $oPage)
-	{
-		$sButtonLabel = $this->oWizard->GetParameter('return_button_label', '');
-		$sButtonUrl = $this->oWizard->GetParameter('return_button_url', '');
-		if ($sButtonLabel !== '') {
-			$oPage->add_ready_script(
-				<<<JS
-	$('.ibo-setup--wizard--buttons-container tr td:nth-child(1)').after('<td style="text-align:center;"><button id="return-button" class="ibo-button ibo-is-alternative ibo-is-neutral" type="button" onclick="window.location.href=\'$sButtonUrl\'"><span class="ibo-button--label">$sButtonLabel</span></button></td>');
-JS
-			);
-		}
-	}
-
 	/**
 	 * Tells whether the "Next" button should be enabled interactively
 	 * @return string A piece of javascript code returning either true or false
@@ -278,11 +257,7 @@ JS
 	 */
 	public function JSCanMoveBackward()
 	{
-		if ($this->oWizard->GetParameter('return_button_label', '') === '') {
-			return 'return true;';
-		}
-
-		return 'return false;';
+		return 'return true;';
 	}
 
 }

setup/wizardsteps/WizStepWelcome.php

@@ -18,6 +18,8 @@
  * You should have received a copy of the GNU Affero General Public License
  */
 
+use Combodo\iTop\Application\WebPage\WebPage;
+
 /**
  * First step of the iTop Installation Wizard: Welcome screen, requirements
  */
@@ -94,7 +96,7 @@ EOF
 			}
 		}
 		$sStyle = 'style="display:none;overflow:auto;"';
-		$sToggleButtons = '<button type="button" id="show_details" class="ibo-button ibo-is-secondary ibo-is-neutral" onclick="$(\'#details\').toggle(); $(this).toggle(); $(\'#hide_details\').toggle();"><span class="ibo-button--icon fa fa-caret-down"></span><span class="ibo-button--label">Show details</span></button><button type="button" id="hide_details" class="ibo-button ibo-is-secondary ibo-is-neutral" style="display:none;" onclick="$(\'#details\').toggle(); $(this).toggle(); $(\'#show_details\').toggle();"><span class="ibo-button--icon fa fa-caret-up"></span><span class="ibo-button--label">Hide details</span></button>';
+		$sToggleButtons = '<button type="button" id="show_details" class="ibo-button ibo-is-alternative ibo-is-neutral" onclick="$(\'#details\').toggle(); $(this).toggle(); $(\'#hide_details\').toggle();"><span class="ibo-button--icon fa fa-caret-down"></span><span class="ibo-button--label">Show details</span></button><button type="button" id="hide_details" class="ibo-button ibo-is-alternative ibo-is-neutral" style="display:none;" onclick="$(\'#details\').toggle(); $(this).toggle(); $(\'#show_details\').toggle();"><span class="ibo-button--icon fa fa-caret-up"></span><span class="ibo-button--label">Hide details</span></button>';
 		if (count($aErrors) > 0) {
 			$sStyle = 'overflow:auto;"';
 			$sTitle = count($aErrors).' Error(s), '.count($aWarnings).' Warning(s).';

setup/wizardsteps/WizardStep.php

@@ -151,6 +151,15 @@ abstract class WizardStep
 		return true;
 	}
 
+	/**
+	 * Tells whether the user will come back to this step/state if he click on "Back"
+	 * @return boolean True if the 'Back' button should display this step
+	 */
+	public function CanComeBack()
+	{
+		return true;
+	}
+
 	/**
 	 * Tells whether the "Next" button should be enabled interactively
 	 * @return string A piece of javascript code returning either true or false

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

@@ -285,61 +285,89 @@ SQL
 	}
 
 	/**
-	 * @covers \ModuleInstallerAPI::LoadLocalizedData
+	 * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion
+	 * @dataProvider LoadLocalizedData_RequiredLanguageProvider
 	 */
-	public function testLoadLocalizedData_LoadsOnFirstInstall(): void
+	public function testLoadLocalizedData_LoadRequiredLanguageOnFirstInstall(string $sRequiredLanguage, array $aAvailableLanguages, array $aExpectedCountByLanguage): void
 	{
 		// Given
-		[$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_FirstInstall_', 'fr_fr');
-		$this->GivenLocalizedDataFile($sTmpDir, "en_us", $sOrgName);
-		$this->GivenLocalizedDataFile($sTmpDir, "fr_fr", $sOrgName);
+		[$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_RequiredLanguage_', $sRequiredLanguage, $aAvailableLanguages);
+
 		// When no previous version, and current version higher than the first loading version
-		ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.3.0', '3.0.0', $sPattern);
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', '3.3.0', '3.0.0', $sPattern);
+
 		// Then data loaded
-		$this->AssertOrganizationCountByName($sOrgName, 'en_us', 0);
-		$this->AssertOrganizationCountByName($sOrgName, 'fr_fr', 1);
+		foreach ($aExpectedCountByLanguage as $sLanguage => $iExpectedCount) {
+			$this->AssertOrganizationCountByName($sOrgName, $sLanguage, $iExpectedCount);
+		}
+	}
+
+	public function LoadLocalizedData_RequiredLanguageProvider(): array
+	{
+		return [
+			'Required fr_fr and file exists' => [
+				'required language' => 'fr_fr',
+				'available languages' => ['en_us', 'fr_fr'],
+				'expected counts' => ['en_us' => 0, 'fr_fr' => 1],
+			],
+			'Required en_us and file exists' => [
+				'required language' => 'en_us',
+				'available languages' => ['en_us', 'fr_fr'],
+				'expected counts' => ['en_us' => 1, 'fr_fr' => 0],
+			],
+			'Required fr_fr but fallback to en_us' => [
+				'required language' => 'fr_fr',
+				'available languages' => ['en_us'],
+				'expected counts' => ['en_us' => 1, 'fr_fr' => 0],
+			],
+			'Required de_de and file exists' => [
+				'required language' => 'de_de',
+				'available languages' => ['en_us', 'fr_fr', 'de_de'],
+				'expected counts' => ['en_us' => 0, 'fr_fr' => 0, 'de_de' => 1],
+			],
+			'Required de_de but fallback to en_us' => [
+				'required language' => 'de_de',
+				'available languages' => ['en_us', 'fr_fr'],
+				'expected counts' => ['en_us' => 1, 'fr_fr' => 0, 'de_de' => 0],
+			],
+		];
 	}
 
 	/**
-	 * @covers \ModuleInstallerAPI::LoadLocalizedData
+	 * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion
+	 * @dataProvider LoadLocalizedData_VersionConditionNotMetProvider
 	 */
-	public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(): void
+	public function testLoadLocalizedData_DoesNotLoadWhenVersionConditionIsNotMet(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion): void
 	{
 		// Given
-		[$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us');
-		$this->GivenLocalizedDataFile($sTmpDir, "en_us", $sOrgName);
-
-		// When a previous version that is lower than the first loading version, but higher or equal to the current version
-		ModuleInstallerAPI::LoadLocalizedData($oConfig, '3.0.0', '3.1.0', '3.0.0', $sPattern);
+		[$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_NoLoad_', 'en_us', ['en_us']);
+		// When version gate conditions are not met
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern);
 		// Then no data loaded
 		$this->AssertOrganizationCountByName($sOrgName, 'en_us', 0);
 	}
 
-	/**
-	 * @covers \ModuleInstallerAPI::LoadLocalizedData
-	 */
-	public function testLoadLocalizedData_FallbacksToEnUsWhenLanguageFileIsMissing(): void
+	public function LoadLocalizedData_VersionConditionNotMetProvider(): array
 	{
-		[$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_Fallback_', 'fr_fr');
-		// Intentionally create ONLY en_us file
-		$this->GivenLocalizedDataFile($sTmpDir, 'en_us', $sOrgName);
-		// When loading localized data in fr_fr, but only en_us file exists
-		ModuleInstallerAPI::LoadLocalizedData($oConfig, '', '3.3.0', '3.0.0', $sPattern);
+		return [
+			'Equal versions (reinstall)' => ['3.1.0', '3.1.0', '3.0.0'],
+			'Downgrade attempt' => ['3.2.0', '3.1.0', '3.0.0'],
+			'Upgrade but first loading version already passed' => ['3.1.0', '3.2.0', '3.0.0'],
+			'Upgrade with boundary equality on first loading version' => ['3.0.0', '3.1.0', '3.0.0'],
+			'Upgrade but first loading version empty' => ['3.1.0', '3.2.0', ''],
 
-		$this->AssertOrganizationCountByName($sOrgName, 'fr_fr', 0);
-		$this->AssertOrganizationCountByName($sOrgName, 'en_us', 1);
+		];
 	}
 
 	/**
-	 * @covers \ModuleInstallerAPI::LoadLocalizedData
+	 * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion
 	 * @dataProvider LoadLocalizedData_ValidVersionFormatsProvider
 	 */
 	public function testLoadLocalizedData_AcceptsSupportedVersionFormats(string $sCurrentVersion, string $sFirstLoadingVersion): void
 	{
-		[$oConfig, $sOrgName, $sTmpDir, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_ValidVersion_', 'en_us');
-		$this->GivenLocalizedDataFile($sTmpDir, 'en_us', $sOrgName);
+		[$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_ValidVersion_', 'en_us', ['en_us']);
 
-		ModuleInstallerAPI::LoadLocalizedData($oConfig, '', $sCurrentVersion, $sFirstLoadingVersion, $sPattern);
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', $sCurrentVersion, $sFirstLoadingVersion, $sPattern);
 
 		$this->AssertOrganizationCountByName($sOrgName, 'en_us', 1);
 	}
@@ -348,11 +376,39 @@ SQL
 	{
 		return [
 			'Current version with suffix' => ['3.2-dev', '3.0.0'],
-			'Current version x.y.z' => ['1.2.4', '1.0'],
-			'Current version x.y.z-suffix' => ['2.3.3-beta', '2.0.0'],
+			'Current version x.y.z' => ['10.12.140-Tagada34', '1.0'],
+			'Current version x.y.z-suffix' => ['2.3.3-beta', '2.3.3-alpha'],
 			'Current version x.y.z-1' => ['1.2.4-1', '1.0.3-2'],
 		];
 	}
+	// Test when a file is loaded twice because of the version conditions, it doesn't create duplicates (idempotent loading)
+	public function testLoadLocalizedData_IdempotentLoading(): void
+	{
+		// Given
+		[$oConfig, $sOrgName, $sPattern] = $this->GivenLocalizedDataTestContext('XML_Load_Idempotent_', 'en_us', ['en_us']);
+
+		// When LoadLocalizedData is called twice with conditions that would load the file both times
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '', '3.1.0', '3.0.0', $sPattern);
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, '3.1.0', '3.2.0', '', $sPattern);
+
+		// Then no duplicate data loaded
+		$this->AssertOrganizationCountByName($sOrgName, 'en_us', 1);
+	}
+
+	/**
+	 * @covers \ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion
+	 * @dataProvider LoadLocalizedData_InvalidParametersProvider
+	 */
+	public function testLoadLocalizedData_ThrowsOnInvalidParameters(string $sPreviousVersion, string $sCurrentVersion, string $sFirstLoadingVersion, string $sPattern, string $sExpectedMessage): void
+	{
+		$oConfig = MetaModel::GetConfig();
+		$this->assertNotNull($oConfig);
+
+		$this->expectException(\CoreUnexpectedValue::class);
+		$this->expectExceptionMessage($sExpectedMessage);
+
+		ModuleInstallerAPI::LoadLocalizedDataOnCrossingVersion($oConfig, $sPreviousVersion, $sCurrentVersion, $sFirstLoadingVersion, $sPattern);
+	}
 
 	public function LoadLocalizedData_InvalidParametersProvider(): array
 	{
@@ -388,13 +444,6 @@ SQL
 				'pattern' => $sTmpDir.DIRECTORY_SEPARATOR.'data.{{LANGUAGE_CODE}}.xml',
 				'message' => "{{language_code}}",
 			],
-			'Parent directory does not exist' => [
-				'previous' => '',
-				'current' => '3.2.0',
-				'first' => '3.0.0',
-				'pattern' => $sTmpDir.DIRECTORY_SEPARATOR.'missing'.DIRECTORY_SEPARATOR.'data.{{language_code}}.xml',
-				'message' => 'parent directory',
-			],
 		];
 	}
 
@@ -403,7 +452,7 @@ SQL
 	 *
 	 * @return array{0: Config, 1: string, 2: string, 3: string, 4: string}
 	 */
-	private function GivenLocalizedDataTestContext(string $sOrgNamePrefix, string $sLanguage): array
+	private function GivenLocalizedDataTestContext(string $sOrgNamePrefix, string $sLanguage, array $aAvailableLanguages = []): array
 	{
 		$oConfig = MetaModel::GetConfig();
 		$oConfig->SetDefaultLanguage($sLanguage);
@@ -415,7 +464,11 @@ SQL
 		$this->aFileToClean[] = $sTmpDir;
 		$sPattern = $sTmpDir.DIRECTORY_SEPARATOR.'data.{{language_code}}.xml';
 
-		return [$oConfig, $sOrgName, $sTmpDir, $sPattern];
+		foreach ($aAvailableLanguages as $sAvailableLanguage) {
+			$this->GivenLocalizedDataFile($sTmpDir, $sAvailableLanguage, $sOrgName);
+		}
+
+		return [$oConfig, $sOrgName, $sPattern];
 	}
 
 	private function GivenLocalizedDataFile(string $sDir, string $sLang, string $sOrgName): string