Apply suggestions from code review

Author: ccailly

css/includes/components/form/_form-editor.scss

@@ -394,7 +394,7 @@ input.value-selector {
     }
 }
 
-.visibility-dropdown-card {
+.visibility-dropdown-card, .validation-dropdown-card {
     width: 700px;
 }
 

src/Glpi/Controller/Form/Condition/EditorController.php

@@ -35,7 +35,6 @@
 namespace Glpi\Controller\Form\Condition;
 
 use Glpi\Controller\AbstractController;
-use Glpi\Exception\Http\NotFoundHttpException;
 use Glpi\Form\Condition\ConditionData;
 use Glpi\Form\Condition\EditorManager;
 use Glpi\Form\Condition\FormData;
@@ -95,7 +94,7 @@ public function validationEditor(Request $request): Response
         $question_uuid = $form_data->getSelectedItemUuid();
         $question_name = current(array_filter(
             $form_data->getQuestionsData(),
-            fn (QuestionData $question) => $question->getUuid() === $question_uuid
+            fn(QuestionData $question) => $question->getUuid() === $question_uuid
         ))->getName();
 
         // Retrieve the conditions data

src/Glpi/Form/AnswersHandler/AnswersHandler.php

@@ -111,7 +111,7 @@ public function validateAnswers(
 
         // Validate answers for each question if validation conditions are defined
         foreach ($questions_container->getQuestions() as $question) {
-            if ($question->getConfiguredValidationStrategy() === ValidationStrategy::ALWAYS_VALIDATED) {
+            if ($question->getConfiguredValidationStrategy() === ValidationStrategy::NO_VALIDATION) {
                 // Skip validation if the question is always validated
                 continue;
             }

src/Glpi/Form/Condition/ConditionableValidationTrait.php

@@ -74,6 +74,6 @@ public function getConfiguredValidationStrategy(): ValidationStrategy
         $field_name = $this->getValidationStrategyFieldName();
         $strategy_value = $this->fields[$field_name] ?? "";
         $strategy = ValidationStrategy::tryFrom($strategy_value);
-        return $strategy ?? ValidationStrategy::ALWAYS_VALIDATED;
+        return $strategy ?? ValidationStrategy::NO_VALIDATION;
     }
 }

src/Glpi/Form/Condition/Engine.php

@@ -174,7 +174,7 @@ private function computeItemValidation(Question $question): array
     {
         // Stop immediatly if the strategy result is forced.
         $strategy = $question->getConfiguredValidationStrategy();
-        if ($strategy == ValidationStrategy::ALWAYS_VALIDATED) {
+        if ($strategy == ValidationStrategy::NO_VALIDATION) {
             return [];
         }
 

src/Glpi/Form/Condition/ValidationStrategy.php

@@ -38,46 +38,46 @@
 
 enum ValidationStrategy: string implements StrategyInterface
 {
-    case ALWAYS_VALIDATED = 'always_validated';
-    case VALIDATED_IF = 'validated_if';
-    case NOT_VALIDATED = 'not_validated';
+    case NO_VALIDATION = 'no_validation';
+    case VALID_IF      = 'valid_if';
+    case INVALID_IF    = 'invalid_if';
 
     #[Override]
     public function getLabel(): string
     {
         return match ($this) {
-            self::ALWAYS_VALIDATED => __('Always validated'),
-            self::VALIDATED_IF     => __('Validated if...'),
-            self::NOT_VALIDATED    => __('Not validated if...'),
+            self::NO_VALIDATION => __('No validation'),
+            self::VALID_IF      => __('Valid if...'),
+            self::INVALID_IF    => __('Invalid if...'),
         };
     }
 
     #[Override]
     public function getIcon(): string
     {
         return match ($this) {
-            self::ALWAYS_VALIDATED => 'ti ti-filter',
-            self::VALIDATED_IF     => 'ti ti-filter-cog',
-            self::NOT_VALIDATED    => 'ti ti-filter-x',
+            self::NO_VALIDATION => 'ti ti-filter',
+            self::VALID_IF      => 'ti ti-filter-cog',
+            self::INVALID_IF    => 'ti ti-filter-x',
         };
     }
 
     #[Override]
     public function showEditor(): bool
     {
         return match ($this) {
-            self::ALWAYS_VALIDATED => false,
-            self::VALIDATED_IF     => true,
-            self::NOT_VALIDATED    => true,
+            self::NO_VALIDATION => false,
+            self::VALID_IF      => true,
+            self::INVALID_IF    => true,
         };
     }
 
     public function mustBeValidated(bool $conditions_result): bool
     {
         return match ($this) {
-            self::ALWAYS_VALIDATED => true,
-            self::VALIDATED_IF     => $conditions_result,
-            self::NOT_VALIDATED    => !$conditions_result,
+            self::NO_VALIDATION => true,
+            self::VALID_IF      => $conditions_result,
+            self::INVALID_IF    => !$conditions_result,
         };
     }
 }

src/Glpi/Form/Condition/ValueOperator.php

@@ -95,7 +95,7 @@ public function getErrorMessageForValidation(
         ValidationStrategy $validation_strategy,
         ConditionData $condition_data
     ): string {
-        if ($validation_strategy === ValidationStrategy::VALIDATED_IF) {
+        if ($validation_strategy === ValidationStrategy::VALID_IF) {
             return match ($this) {
                 self::GREATER_THAN           => sprintf(__("The value must be greater than %s"), $condition_data->getValue()),
                 self::GREATER_THAN_OR_EQUALS => sprintf(__("The value must be greater than or equal to %s"), $condition_data->getValue()),
@@ -106,7 +106,7 @@ public function getErrorMessageForValidation(
 
                 default => __("The value is not valid"),
             };
-        } elseif ($validation_strategy === ValidationStrategy::NOT_VALIDATED) {
+        } elseif ($validation_strategy === ValidationStrategy::INVALID_IF) {
             return match ($this) {
                 self::GREATER_THAN           => sprintf(__("The value must not be greater than %s"), $condition_data->getValue()),
                 self::GREATER_THAN_OR_EQUALS => sprintf(__("The value must not be greater than or equal to %s"), $condition_data->getValue()),

templates/pages/admin/form/conditional_validation_dropdown.html.twig

@@ -34,7 +34,7 @@
 
 {# Compute strategy, with a fallback to "Always visible" for new questions #}
 {% if item is null %}
-    {% set question_strategy = enum('Glpi\\Form\\Condition\\ValidationStrategy').ALWAYS_VALIDATED %}
+    {% set question_strategy = enum('Glpi\\Form\\Condition\\ValidationStrategy').NO_VALIDATION %}
 {% else %}
     {% set question_strategy = item.getConfiguredValidationStrategy() %}
 {% endif %}
@@ -44,7 +44,7 @@
         data-glpi-form-editor-validation-dropdown-container
         data-glpi-form-editor-question-extra-details
 
-        {% set hide = question_strategy == enum('Glpi\\Form\\Condition\\ValidationStrategy').ALWAYS_VALIDATED %}
+        {% set hide = question_strategy == enum('Glpi\\Form\\Condition\\ValidationStrategy').NO_VALIDATION %}
         class="btn-group ms-1 {{ hide ? 'd-none' : '' }}"
     >
         <button

templates/pages/admin/form/conditional_validation_editor.html.twig

@@ -30,21 +30,13 @@
  # ---------------------------------------------------------------------
  #}
 
-{% set last_condition_is_filled = false %}
-
-{% for condition in defined_conditions %}
-    {% set condition_is_filled = condition.getItemUuid() != '' %}
-    {% set last_condition_is_filled = loop.last and condition_is_filled %}
-    {% set value_operators = manager.getValueOperatorForValidationDropdownValues(
-        condition.getItemUuid()
-    ) %}
-
-    {# If no value operator is available, display a message #}
-    {% if value_operators is empty %}
-        <div class="alert alert-info mt-3">
-            {{ __('Conditional validation is not available for this question type.') }}
-        </div>
-    {% else %}
+{% set value_operators = manager.getValueOperatorForValidationDropdownValues(question_uuid) %}
+{% if value_operators is empty %}
+    <div class="alert alert-info mt-3">
+        {{ __('Conditional validation is not available for this question type.') }}
+    </div>
+{% else %}
+    {% for condition in defined_conditions %}
         <div
             class="row mt-3"
             data-glpi-conditions-editor-condition
@@ -152,10 +144,8 @@
                 </div>
             </div>
         </div>
-    {% endif %}
-{% endfor %}
+    {% endfor %}
 
-{% if last_condition_is_filled %}
     <button
         data-glpi-condition-editor-add-condition
         type="button"