From a048e0548bb7f3257641415a09381460d699a878 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cailly?= Date: Tue, 15 Apr 2025 13:45:15 +0200 Subject: [PATCH 1/4] fix(forms): Refactoring migration logic to skip orphan elements, and improve default value handling for some question types --- .../Glpi/Form/Migration/FormMigrationTest.php | 72 +++++++++++++++++++ src/Glpi/Form/Migration/FormMigration.php | 69 +++++++++++------- .../AbstractQuestionTypeSelectable.php | 9 ++- .../Form/QuestionType/QuestionTypeUrgency.php | 2 +- 4 files changed, 125 insertions(+), 27 deletions(-) diff --git a/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php b/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php index ae0498811a5..181d44a0b7d 100644 --- a/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php +++ b/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php @@ -1165,6 +1165,78 @@ public function testFormMigrationTagConversion(string $rawContent, string $expec ); } + public function testFormMigrationWithRadioQuestion(): void + { + /** + * @var \DBmysql $DB + */ + global $DB; + + // Insert a new form + $this->assertTrue($DB->insert( + 'glpi_plugin_formcreator_forms', + [ + 'name' => 'Test form migration for radio question', + ] + )); + + // Insert a new section + $this->assertTrue($DB->insert( + 'glpi_plugin_formcreator_sections', + [ + 'name' => 'Test form migration for radio question - Section', + 'plugin_formcreator_forms_id' => $DB->insertId() + ] + )); + + // Insert a new question with type "radio" + $section_id = $DB->insertId(); + $this->assertTrue($DB->insert( + 'glpi_plugin_formcreator_questions', + [ + 'plugin_formcreator_sections_id' => $section_id, + 'name' => 'Test form migration for radio question - Question', + 'fieldtype' => 'radios', + 'default_values' => '1', + 'values' => json_encode(['1', '2', '3']), + ] + )); + + $migration = new FormMigration($DB, FormAccessControlManager::getInstance()); + $this->setPrivateProperty($migration, 'result', new PluginMigrationResult()); + $this->assertTrue($this->callPrivateMethod($migration, 'processMigration')); + + // Check that the question has been migrated + /** @var Question $question */ + $question = getItemByTypeName(Question::class, 'Test form migration for radio question - Question'); + $this->assertInstanceOf(Question::class, $question); + + /** @var QuestionTypeRadio $question_type */ + $question_type = $question->getQuestionType(); + $this->assertInstanceOf(QuestionTypeRadio::class, $question_type); + + $this->assertEquals( + [ + [ + 'uuid' => 0, + 'value' => '1', + 'checked' => true, + ], + [ + 'uuid' => 1, + 'value' => '2', + 'checked' => false, + ], + [ + 'uuid' => 2, + 'value' => '3', + 'checked' => false, + ] + ], + $question_type->getValues($question) + ); + } + public static function provideFormMigrationConditionsForQuestions(): iterable { yield 'QuestionTypeShortText - Always visible' => [ diff --git a/src/Glpi/Form/Migration/FormMigration.php b/src/Glpi/Form/Migration/FormMigration.php index 8025996a8ae..3bf313830a3 100644 --- a/src/Glpi/Form/Migration/FormMigration.php +++ b/src/Glpi/Form/Migration/FormMigration.php @@ -447,11 +447,10 @@ private function processMigrationOfSections(): void 'PluginFormcreatorForm', $raw_section['plugin_formcreator_forms_id'] )['items_id'] ?? 0; + + // If the form ID is 0, it means the form does not exist if ($form_id === 0) { - $this->result->addMessage(MessageType::Error, sprintf( - 'Section "%s" has no form. It will not be migrated.', - $raw_section['name'] - )); + // No need to warn the user, as this section was not visible in formcreator anyway. continue; } @@ -509,11 +508,10 @@ private function processMigrationOfQuestions(): void 'PluginFormcreatorSection', $raw_question['plugin_formcreator_sections_id'] )['items_id'] ?? 0; + + // If the section ID is 0, it means the section does not exist if ($section_id === 0) { - $this->result->addMessage(MessageType::Error, sprintf( - 'Question "%s" has no section. It will not be migrated.', - $raw_question['name'] - )); + // No need to warn the user, as this question was not visible in formcreator anyway. continue; } @@ -544,20 +542,34 @@ private function processMigrationOfQuestions(): void 'uuid' => $raw_question['uuid'] ], fn ($value) => $value !== null); - $question = $this->importItem( - Question::class, - $data, - [ - 'uuid' => $raw_question['uuid'], - ] - ); + try { + $question = $this->importItem( + Question::class, + $data, + [ + 'uuid' => $raw_question['uuid'], + ] + ); - $this->mapItem( - 'PluginFormcreatorQuestion', - $raw_question['id'], - Question::class, - $question->getID() - ); + $this->mapItem( + 'PluginFormcreatorQuestion', + $raw_question['id'], + Question::class, + $question->getID() + ); + } catch (\Throwable $th) { + $section = Section::getById($section_id); + $this->result->addMessage( + MessageType::Error, + sprintf( + 'Error while importing question "%s" in section "%s" and form "%s": %s', + $raw_question['name'], + $section?->getName(), + $section?->getItem()?->getName(), + $th->getMessage() + ) + ); + } $this->progress_indicator?->advance(); } @@ -915,8 +927,11 @@ private function processMigrationOfDestination( )['items_id'] ?? 0; $form = new Form(); + + // If the form is invalid, skip this target if (!$form->getFromDB($form_id)) { - throw new LogicException("Form with id {$raw_target['plugin_formcreator_forms_id']} not found"); + // No need to warn the user, as this destination was not visible in formcreator anyway. + continue; } $fields_config = []; @@ -934,9 +949,10 @@ private function processMigrationOfDestination( $this->result->addMessage( MessageType::Error, sprintf( - __('The "%s" destination field configuration of the form "%s" cannot be imported.'), + __('The "%s" destination field configuration of the form "%s" cannot be imported : %s'), $configurable_field->getLabel(), - $form->getName() + $form->getName(), + $th->getMessage() ) ); } @@ -1018,8 +1034,11 @@ private function processMigrationOfITILActorsFields( $raw_target_actor['items_id'] )['items_id'] ?? 0; + // The destination ID is 0 if the target was not migrated or not existing. + // In this case, we skip the actor if ($target_id === 0) { - throw new LogicException("Destination for target id {$raw_target_actor['items_id']} not found"); + // No need to warn the user, as this actor config was not visible in formcreator anyway. + continue; } $targets_actors[$target_id][$raw_target_actor['actor_role']][$raw_target_actor['actor_type']][] = diff --git a/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php b/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php index b5e022e5535..74f999c4f92 100644 --- a/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php +++ b/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php @@ -188,8 +188,15 @@ public function convertDefaultValue(array $rawData): array return []; } + /** + * New default values format require an array of values. + * The old system did not use an array if there was only one element. + */ $default_values = json_decode($rawData['default_values']); - if ($default_values === null && json_last_error() !== JSON_ERROR_NONE) { + if ( + ($default_values === null && json_last_error() !== JSON_ERROR_NONE) + || !empty($rawData['default_values']) && !is_array($default_values) + ) { $default_values = [$rawData['default_values']]; } diff --git a/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php b/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php index 0f602d23a7a..2eeb19dac4f 100644 --- a/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php +++ b/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php @@ -194,7 +194,7 @@ public function getConditionHandlers( #[Override] public function convertDefaultValue(array $rawData): ?int { - return $rawData['default_values'] ?? null; + return (int) $rawData['default_values'] ?? null; } #[Override] From 78c881617451d303cd968612885afbb930f34e11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cailly?= Date: Wed, 16 Apr 2025 13:34:46 +0200 Subject: [PATCH 2/4] fix: PHP lint --- src/Glpi/Form/Migration/FormMigration.php | 2 +- .../Form/QuestionType/AbstractQuestionTypeSelectable.php | 2 +- src/Glpi/Form/QuestionType/QuestionTypeUrgency.php | 6 +++++- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Glpi/Form/Migration/FormMigration.php b/src/Glpi/Form/Migration/FormMigration.php index 3bf313830a3..37d02edad6b 100644 --- a/src/Glpi/Form/Migration/FormMigration.php +++ b/src/Glpi/Form/Migration/FormMigration.php @@ -558,7 +558,7 @@ private function processMigrationOfQuestions(): void $question->getID() ); } catch (\Throwable $th) { - $section = Section::getById($section_id); + $section = Section::getById($section_id) ?: null; $this->result->addMessage( MessageType::Error, sprintf( diff --git a/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php b/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php index 74f999c4f92..4999dc850c9 100644 --- a/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php +++ b/src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php @@ -195,7 +195,7 @@ public function convertDefaultValue(array $rawData): array $default_values = json_decode($rawData['default_values']); if ( ($default_values === null && json_last_error() !== JSON_ERROR_NONE) - || !empty($rawData['default_values']) && !is_array($default_values) + || !is_array($default_values) ) { $default_values = [$rawData['default_values']]; } diff --git a/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php b/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php index 2eeb19dac4f..faf52068d0a 100644 --- a/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php +++ b/src/Glpi/Form/QuestionType/QuestionTypeUrgency.php @@ -194,7 +194,11 @@ public function getConditionHandlers( #[Override] public function convertDefaultValue(array $rawData): ?int { - return (int) $rawData['default_values'] ?? null; + if (!isset($rawData['default_values'])) { + return null; + } + + return (int) $rawData['default_values']; } #[Override] From 5180399edfe062d306f1a6d7c57deb19da8a5507 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Cailly?= Date: Thu, 17 Apr 2025 14:54:31 +0200 Subject: [PATCH 3/4] fix(tests): Update FormMigrationTest to check for un-migrated orphan sections and questions --- .../Glpi/Form/Migration/FormMigrationTest.php | 29 +++++++------------ 1 file changed, 10 insertions(+), 19 deletions(-) diff --git a/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php b/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php index 181d44a0b7d..6a100b3a375 100644 --- a/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php +++ b/phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php @@ -80,7 +80,6 @@ use Glpi\Form\QuestionType\QuestionTypesManager; use Glpi\Form\QuestionType\QuestionTypeUrgency; use Glpi\Form\Section; -use Glpi\Message\MessageType; use Glpi\Migration\PluginMigrationResult; use Glpi\Tests\FormTesterTrait; use Location; @@ -919,15 +918,11 @@ public function testFormMigrationWithOrphanSection(): void $this->setPrivateProperty($migration, 'result', $result); $this->assertTrue($this->callPrivateMethod($migration, 'processMigration')); - $errors = array_filter( - $result->getMessages(), - static fn (array $entry) => $entry['type'] === MessageType::Error - ); - $this->assertCount(1, $errors); - $this->assertEquals( - current($errors)['message'], - 'Section "Orphan section" has no form. It will not be migrated.' - ); + // Check that the section hasn't been migrated + $section = new Section(); + $this->assertFalse($section->getFromDBByCrit([ + 'name' => 'Orphan section', + ])); } public function testFormMigrationWithOrphanQuestion(): void @@ -950,15 +945,11 @@ public function testFormMigrationWithOrphanQuestion(): void $this->setPrivateProperty($migration, 'result', $result); $this->assertTrue($this->callPrivateMethod($migration, 'processMigration')); - $errors = array_filter( - $result->getMessages(), - static fn (array $entry) => $entry['type'] === MessageType::Error - ); - $this->assertCount(1, $errors); - $this->assertEquals( - current($errors)['message'], - 'Question "Orphan question" has no section. It will not be migrated.' - ); + // Check that the question hasn't been migrated + $question = new Question(); + $this->assertFalse($question->getFromDBByCrit([ + 'name' => 'Orphan question', + ])); } public function testFormMigrationUpdateHorizontalRanks(): void From d72b415e2ad8e4bff82d805fcb03fceaa31927f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=A9dric=20Anne?= Date: Tue, 29 Apr 2025 22:01:39 +0200 Subject: [PATCH 4/4] Update src/Glpi/Form/Migration/FormMigration.php --- src/Glpi/Form/Migration/FormMigration.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Glpi/Form/Migration/FormMigration.php b/src/Glpi/Form/Migration/FormMigration.php index 37d02edad6b..3f9d7289b04 100644 --- a/src/Glpi/Form/Migration/FormMigration.php +++ b/src/Glpi/Form/Migration/FormMigration.php @@ -562,7 +562,7 @@ private function processMigrationOfQuestions(): void $this->result->addMessage( MessageType::Error, sprintf( - 'Error while importing question "%s" in section "%s" and form "%s": %s', + __('Error while importing question "%s" in section "%s" and form "%s": %s'), $raw_question['name'], $section?->getName(), $section?->getItem()?->getName(),