Skip to content

fix(forms): Refactoring migration logic to skip orphan elements messages, and improve default value handling for some question types #19474

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
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(forms): Refactoring migration logic to skip orphan elements messages, and improve default value handling for some question types #19474

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
8affe98
fix(forms): Refactoring migration logic to skip orphan elements, and …
ccailly Apr 15, 2025
5a15bd6
fix: PHP lint
ccailly Apr 16, 2025
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
72 changes: 72 additions & 0 deletions phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1029,4 +1029,76 @@ public function testFormMigrationUpdateHorizontalRanks(): void
$this->assertEquals($index, $block->fields['horizontal_rank']);
}
}

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)
);
}
}
69 changes: 44 additions & 25 deletions src/Glpi/Form/Migration/FormMigration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,11 +366,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;
}

Expand Down Expand Up @@ -428,11 +427,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;
}

Expand Down Expand Up @@ -463,20 +461,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) ?: null;
$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();
}
Expand Down Expand Up @@ -834,8 +846,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 = [];
Expand All @@ -853,9 +868,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()
)
);
}
Expand Down Expand Up @@ -925,8 +941,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']][] =
Expand Down
9 changes: 8 additions & 1 deletion src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
|| !is_array($default_values)
) {
$default_values = [$rawData['default_values']];
}

Expand Down
6 changes: 5 additions & 1 deletion src/Glpi/Form/QuestionType/QuestionTypeUrgency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,11 @@ public function getConditionHandler(
#[Override]
public function convertDefaultValue(array $rawData): ?int
{
return $rawData['default_values'] ?? null;
if (!isset($rawData['default_values'])) {
return null;
}

return (int) $rawData['default_values'];
}

#[Override]
Expand Down
Loading