Improve migration of conditions on checkboxes

Author: AdrienClairembault

src/Glpi/Form/Condition/ConditionHandler/MultipleChoiceFromValuesConditionHandler.php

@@ -79,8 +79,9 @@ public function applyValueOperator(
     }
 
     #[Override]
-    public function convertConditionValue(string $value): int
+    public function convertConditionValue(string $value): array
     {
-        return array_search($value, $this->values, true) ?: 0;
+        $value = array_search($value, $this->values, true) ?: 0;
+        return [$value];
     }
 }

src/Glpi/Form/Migration/FormMigration.php

@@ -68,6 +68,9 @@
 use Glpi\Form\Form;
 use Glpi\Form\FormTranslation;
 use Glpi\Form\Question;
+use Glpi\Form\QuestionType\QuestionTypeCheckbox;
+use Glpi\Form\QuestionType\QuestionTypeDropdown;
+use Glpi\Form\QuestionType\QuestionTypeDropdownExtraDataConfig;
 use Glpi\Form\QuestionType\QuestionTypeEmail;
 use Glpi\Form\QuestionType\QuestionTypeInterface;
 use Glpi\Form\QuestionType\QuestionTypeLongText;
@@ -189,16 +192,41 @@ private function getCreationStrategyFromLegacy(int $creation): CreationStrategy
      * @param QuestionTypeInterface $question_type The question type interface
      * @return ValueOperator|null The corresponding value operator or null if not found
      */
-    private function getValueOperatorFromLegacy(int $value_operator, mixed $value, QuestionTypeInterface $question_type): ?ValueOperator
-    {
+    private function getValueOperatorFromLegacy(
+        int $value_operator,
+        mixed $value,
+        QuestionTypeInterface $question_type,
+        ?JsonFieldInterface $question_config,
+    ): ?ValueOperator {
         // String condition handler support length comparison but with a different operator
         $has_string_condition_handler = in_array($question_type::class, [
             QuestionTypeShortText::class, QuestionTypeEmail::class, QuestionTypeLongText::class,
         ]);
 
+        $is_array_condition = $question_type instanceof QuestionTypeCheckbox
+            || (
+                $question_type instanceof QuestionTypeDropdown
+                && $question_config !== null
+                && $question_config instanceof QuestionTypeDropdownExtraDataConfig
+                && $question_config->isMultipleDropdown()
+            )
+        ;
+
         return match ($value_operator) {
-            1 => $value === "" ? ValueOperator::EMPTY : ValueOperator::EQUALS,
-            2 => $value === "" ? ValueOperator::NOT_EMPTY : ValueOperator::NOT_EQUALS,
+            1 => $value === ""
+                ? ValueOperator::EMPTY
+                : (
+                    $is_array_condition
+                    ? ValueOperator::CONTAINS
+                    : ValueOperator::EQUALS
+                ),
+            2 => $value === ""
+                ? ValueOperator::NOT_EMPTY
+                : (
+                    $is_array_condition
+                    ? ValueOperator::NOT_CONTAINS
+                    : ValueOperator::NOT_EQUALS
+                ),
             3 => $has_string_condition_handler ? ValueOperator::LENGTH_LESS_THAN : ValueOperator::LESS_THAN,
             4 => $has_string_condition_handler ? ValueOperator::LENGTH_GREATER_THAN : ValueOperator::GREATER_THAN,
             5 => $has_string_condition_handler ? ValueOperator::LENGTH_LESS_THAN_OR_EQUALS : ValueOperator::LESS_THAN_OR_EQUALS,
@@ -1415,7 +1443,8 @@ private function processMigrationOfVisibilityConditions(): void
             $value_operator = $this->getValueOperatorFromLegacy(
                 $raw_condition['show_condition'],
                 $value,
-                $question_type
+                $question_type,
+                $question->getExtraDataConfig()
             );
 
             if (isset($question->fields['extra_data'])) {

tests/functional/Glpi/Form/Migration/FormMigrationTest.php

@@ -2691,8 +2691,8 @@ public static function provideFormMigrationForSelectableQuestions(): iterable
             ],
             'expected_conditions' => [
                 [
-                    'value_operator' => ValueOperator::EQUALS,
-                    'value' => 3, // Index of 'Option 3' in ['Option 1', 'Option 2', 'Option 3']
+                    'value_operator' => ValueOperator::CONTAINS,
+                    'value' => [3], // Index of 'Option 3' in ['Option 1', 'Option 2', 'Option 3']
                     'logic_operator' => LogicOperator::AND,
                 ],
             ],
@@ -2746,13 +2746,13 @@ public static function provideFormMigrationForSelectableQuestions(): iterable
             ],
             'expected_conditions' => [
                 [
-                    'value_operator' => ValueOperator::EQUALS,
-                    'value' => 2, // Index of 'Option 2' in ['Option 1', 'Option 2', 'Option 3']
+                    'value_operator' => ValueOperator::CONTAINS,
+                    'value' => [2], // Index of 'Option 2' in ['Option 1', 'Option 2', 'Option 3']
                     'logic_operator' => LogicOperator::AND,
                 ],
                 [
-                    'value_operator' => ValueOperator::EQUALS,
-                    'value' => 3, // Index of 'Option 3' in ['Option 1', 'Option 2', 'Option 3']
+                    'value_operator' => ValueOperator::CONTAINS,
+                    'value' => [3], // Index of 'Option 3' in ['Option 1', 'Option 2', 'Option 3']
                     'logic_operator' => LogicOperator::OR,
                 ],
             ],
@@ -3634,6 +3634,63 @@ public function testNonVisiblePrivateFormMigration(): void
         );
     }
 
+    public function testCheckboxesConditionsAreMigratedAsContains(): void
+    {
+        /** @var \DBmysql $DB */
+        global $DB;
+
+        // Arrange: create a form with a condition on a checkbox question
+        $id = $this->createSimpleFormcreatorForm(
+            name: "Form with checkboxes",
+            questions: [
+                [
+                    'name'      => 'My checkbox question',
+                    'fieldtype' => 'checkboxes',
+                    'values'    => '["A","B","C","D"]',
+                ],
+                [
+                    'name'        => 'My other question',
+                    'fieldtype' => 'text',
+                    'show_rule'   => 2,
+                    '_conditions' => [
+                        [
+                            'plugin_formcreator_questions_id' => 'My checkbox question',
+                            'show_condition'                  => 1,
+                            'show_value'                      => "A",
+                            'show_logic'                      => 1,
+                        ],
+                    ],
+                ],
+            ],
+        );
+
+        // Act: import the form
+        $control_manager = FormAccessControlManager::getInstance();
+        $migration = new FormMigration(
+            db: $DB,
+            formAccessControlManager: $control_manager,
+            specificFormsIds: [$id],
+        );
+        $migration->execute();
+
+        // Assert: make sure the condition was expected as "Contains A", not
+        // "Equals "A".
+        // This might seem unintuitive but it match the behavior of formcreator
+        // for these questions.
+        $form = getItemByTypeName(Form::class, "Form with checkboxes");
+        $question_id = $this->getQuestionId($form, "My other question");
+        $question = Question::getById($question_id);
+        $condition = $question->getConfiguredConditionsData()[0];
+        $this->assertEquals(
+            ValueOperator::CONTAINS,
+            $condition->getValueOperator()
+        );
+        $this->assertEquals(
+            [1], // "A" will be assigned the first index, which is 1-based
+            $condition->getValue()
+        );
+    }
+
     protected function createSimpleFormcreatorForm(
         string $name,
         array $questions,