Conversation
|
| Filename | Overview |
|---|---|
| setup/setuputils.class.inc.php | Removes location-based hidden/mandatory treatment for environment extra modules, allowing some predeployed modules to be omitted interactively. |
| setup/wizardsteps/WizStepModulesChoice.php | Computes prior-page modules and forces duplicate extensions checked and disabled; also introduces an unused accumulator. |
| datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php | Computes package-provided modules and annotates duplicate extensions for the removal UI. |
| sources/Application/UI/Base/Layout/Extension/ExtensionDetailsUIBlockFactory.php | Adds the already-part-of-iTop badge and corresponding checked-disabled toggler state. |
| tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php | Updates existing flag expectations and covers an extension fully represented by earlier selected modules. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Analyze setup module directories] --> B[Build package choice pages]
B --> C[Collect modules selected on earlier pages]
C --> D{All extension modules already selected?}
D -->|Yes| E[Check and disable extension]
D -->|No| F[Apply normal extension flags]
E --> G[Show already part of iTop badge]
F --> H{Module is in environment extra directory?}
H -->|Installed remote extension| I[Protect from removal]
H -->|Not recorded as installed and unique| J[Currently remains selectable]
J --> K[Can be omitted from selected_modules]
Reviews (1): Last reviewed commit: "N°9906 Code clean up" | Re-trigger Greptile
There was a problem hiding this comment.
🟡 Changes recommended
Critical selection/removal issues and moderate correctness and coverage issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR detects extensions whose modules were already selected in setup, keeping them selected/disabled and labeling them in the removal UI.
Changes:
- Tracks prior wizard selections and adds
already_includedhandling. - Updates removal UI, templates, translations, tests, and flow documentation.
- Refactors setup module analysis and extension-map arguments.
File summaries
| File | Reviewed change / finding |
|---|---|
tests/php-unit-tests/unitary-tests/setup/WizStepModulesChoiceTest.php |
Adds coverage for updated choice flags. |
tests/php-unit-tests/unitary-tests/setup/ressources/compute_choice_flags.flowchart.fun |
Documents updated choice logic. |
sources/Application/UI/Base/Layout/Extension/ExtensionDetailsUIBlockFactory.php |
Adds badge and disabled toggler behavior. Critical: removed-extension handling can reset the selected state and submit off. |
setup/wizardsteps/WizStepModulesChoice.php |
Detects modules selected in earlier steps. Moderate: alternative choices can overwrite already_included flags. |
setup/setuputils.class.inc.php |
Adjusts installation module analysis. Critical: removed visibility handling may omit production modules and allow unintended removal. |
setup/extensionsmap.class.inc.php |
Makes extension-selection arguments optional. |
dictionaries/ui/layouts/extensions-details/zh_cn.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/tr.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/sk.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/ru.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/pt_br.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/pl.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/nl.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/ja.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/it.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/hu.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/fr.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/es_cr.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/en.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/en_gb.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/de.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/da.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
dictionaries/ui/layouts/extensions-details/cs.dictionary.itop.extensions-details.php |
Adds badge translation/fallback. |
datamodels/2.x/combodo-data-feature-removal/templates/Features.html.twig |
Uses extension metadata. |
datamodels/2.x/combodo-data-feature-removal/templates/AnalysisResult.html.twig |
Uses extension metadata. |
datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php |
Computes package-module overlap. Moderate: coverage is missing for overlap, empty lists, and SOURCE_WIZARD exclusion. |
Review details
Suppressed comments (2)
datamodels/2.x/combodo-data-feature-removal/src/Controller/DataFeatureRemovalController.php:403
- This new calculation controls whether an extension is rendered as a disabled, preselected item, but the existing controller test only covers
ConvertIntoSetupFormatand does not exercise it. Please add coverage for complete module overlap, partial overlap, empty module lists, and theSOURCE_WIZARDexclusion; otherwise an incorrect package-module set can silently make an extension impossible to remove.
public function IsIncludedInPackage(iTopExtension $oExtension): bool
setup/wizardsteps/WizStepModulesChoice.php:840
- For an alternative choice, this result is overwritten by
DisplayOptions(), which replaces bothdisabledandcheckedwith the group-level$bDisabledand$bSelectedvalues at lines 942-943. Consequently an extension whose modules were all selected in an earlier step can still render unchecked and enabled, so the new invariant is not enforced for alternatives; preserve thealready_includedflags when composing the radio-group state and define which already-included alternative wins if more than one matches.
} elseif ($bAlreadyIncluded) {
$bDisabled = true;
$bChecked = true;
- Files reviewed: 26/26 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
odain-cbd
left a comment
There was a problem hiding this comment.
could you pls add test covering the 2 new methods in DataFeatureController?
d7a81b6 to
8bdb7dd
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The cross-cutting setup, UI, data-removal, localization, and test changes require final human review.
Review details
- Files reviewed: 26/26 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
Do not merge this PR until the 3.3.0 is released