Skip to content

N°9010 fix flags when extension missing #800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Timmy38 wants to merge 2 commits into
base: feature/uninstallation
Choose a base branch
from
Open

N°9010 fix flags when extension missing #800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06b0cad
N°9010 Fix flags for missing extensions
Timmy38 Feb 3, 2026
c3cc073
N°9010 Add explanation
Timmy38 Feb 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions setup/wizardsteps.class.inc.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1937,14 +1937,15 @@ protected function GetStepInfo($idx = null)
public function ComputeChoiceFlags(array $aChoice, string $sChoiceId, array $aSelectedComponents, bool $bAllDisabled, bool $bDisableUninstallCheck, bool $bUpgradeMode)
{
$oITopExtension = $this->oExtensionsMap->GetFromExtensionCode($aChoice['extension_code']);
//If the extension is missing from disk, it won't exist in the ExtensionsMap, thus returning null
Copy link
Contributor

@odain-cbd odain-cbd Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whole method has become uggly and hard to understand functionally... even if most cases are covered by tests (which is nice).

please refactor it to have sth we can understand. for ex. use [if else] structure to spit things apart. on one side treat the exceptional case (extension missing on disk). then others...

Copy link
Contributor

@odain-cbd odain-cbd Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactor should not be so long as you have the cover :)

$bCanBeUninstalled = isset($aChoice['uninstallable']) ? $aChoice['uninstallable'] === true || $aChoice['uninstallable'] === 'yes' : $oITopExtension->CanBeUninstalled();
$bSelected = isset($aSelectedComponents[$sChoiceId]) && ($aSelectedComponents[$sChoiceId] == $sChoiceId);
$bMandatory = (isset($aChoice['mandatory']) && $aChoice['mandatory']) || $bUpgradeMode && $oITopExtension->bInstalled && !$bCanBeUninstalled && !$bDisableUninstallCheck;

$bMissingFromDisk = isset($aChoice['missing']) && $aChoice['missing'] === true;

$bMandatory = (isset($aChoice['mandatory']) && $aChoice['mandatory']) || $bUpgradeMode && !$bMissingFromDisk && $oITopExtension->bInstalled && !$bCanBeUninstalled && !$bDisableUninstallCheck;
Copy link
Contributor

@odain-cbd odain-cbd Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable $bMandatory has become sth much more complex than his name. maybe keep its logic simple (mandatory is mandatory). and the addional stuff can come after to treat each flag.

$bInstalled = $bMissingFromDisk || $oITopExtension->bInstalled;
$bDisabled = $bMandatory || $bAllDisabled || $bMissingFromDisk;
$bChecked = $bMandatory || $bSelected;
$bChecked = !$bMissingFromDisk && ($bMandatory || $bSelected);

if (isset($aChoice['sub_options'])) {
$aOptions = $aChoice['sub_options']['options'] ?? [];
Expand Down
63 changes: 63 additions & 0 deletions tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,69 @@ public function ProviderComputeChoiceFlags()
'checked' => true,
],
],
'A missing extension should be disabled and unchecked' => [
'aExtensionsOnDiskOrDb' => [
'itop-ext1' => [
'installed' => true,
],
],
'aWizardStepDefinition' => [
'extension_code' => 'itop-ext1',
'mandatory' => false,
'missing' => true,
'uninstallable' => true,
],
'bCurrentSelected' => false,
'aExpectedFlags' => [
'uninstallable' => true,
'missing' => true,
'installed' => true,
'disabled' => true,
'checked' => false,
],
],
'A missing extension should always be disabled and unchecked, even when mandatory' => [
'aExtensionsOnDiskOrDb' => [
'itop-ext1' => [
'installed' => true,
],
],
'aWizardStepDefinition' => [
'extension_code' => 'itop-ext1',
'mandatory' => true,
'missing' => true,
'uninstallable' => true,
],
'bCurrentSelected' => false,
'aExpectedFlags' => [
'uninstallable' => true,
'missing' => true,
'installed' => true,
'disabled' => true,
'checked' => false,
],
],
'A missing extension should always be disabled and unchecked, even when non-uninstallable' => [
'aExtensionsOnDiskOrDb' => [
'itop-ext1' => [
'installed' => true,
],
],
'aWizardStepDefinition' => [
'extension_code' => 'itop-ext1',
'mandatory' => true,
'missing' => true,
'uninstallable' => false,
],
'bCurrentSelected' => false,
'aExpectedFlags' => [
'uninstallable' => false,
'missing' => true,
'installed' => true,
'disabled' => true,
'checked' => false,
],
],
Comment on lines +70 to +132
Copy link

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases for missing extensions don't accurately simulate the production scenario. In production, when an extension is missing from disk, it won't exist in the ExtensionsMap's aExtensionsByCode array, so GetFromExtensionCode() returns null. However, in these tests, the extension is included in aExtensionsOnDiskOrDb, which means it WILL be present in the fake extension map and GetFromExtensionCode() will NOT return null. Consider updating the test setup to more accurately reflect the production scenario, perhaps by not including missing extensions in aExtensionsOnDiskOrDb, or by having a separate mechanism in the fake map to simulate extensions that are in the database but removed from disk.

Copilot uses AI. Check for mistakes.
'An installed but not selected extension should not be checked and be enabled' => [
'aExtensionsOnDiskOrDb' => [
'itop-ext1' => [
Expand Down