Skip to content

Commit 64600ab

Browse files
ccaillyccailly
authored
fix(forms): enhance form migration
- skip orphan error messages - improve default values handling
1 parent ca2a65b commit 64600ab

File tree

4 files changed

+139
-46
lines changed

4 files changed

+139
-46
lines changed

‎phpunit/functional/Glpi/Form/Migration/FormMigrationTest.php

+82-19
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,6 @@
8080
use Glpi\Form\QuestionType\QuestionTypesManager;
8181
use Glpi\Form\QuestionType\QuestionTypeUrgency;
8282
use Glpi\Form\Section;
83-
use Glpi\Message\MessageType;
8483
use Glpi\Migration\PluginMigrationResult;
8584
use Glpi\Tests\FormTesterTrait;
8685
use Location;
@@ -919,15 +918,11 @@ public function testFormMigrationWithOrphanSection(): void
919918
$this->setPrivateProperty($migration, 'result', $result);
920919
$this->assertTrue($this->callPrivateMethod($migration, 'processMigration'));
921920

922-
$errors = array_filter(
923-
$result->getMessages(),
924-
static fn (array $entry) => $entry['type'] === MessageType::Error
925-
);
926-
$this->assertCount(1, $errors);
927-
$this->assertEquals(
928-
current($errors)['message'],
929-
'Section "Orphan section" has no form. It will not be migrated.'
930-
);
921+
// Check that the section hasn't been migrated
922+
$section = new Section();
923+
$this->assertFalse($section->getFromDBByCrit([
924+
'name' => 'Orphan section',
925+
]));
931926
}
932927

933928
public function testFormMigrationWithOrphanQuestion(): void
@@ -950,15 +945,11 @@ public function testFormMigrationWithOrphanQuestion(): void
950945
$this->setPrivateProperty($migration, 'result', $result);
951946
$this->assertTrue($this->callPrivateMethod($migration, 'processMigration'));
952947

953-
$errors = array_filter(
954-
$result->getMessages(),
955-
static fn (array $entry) => $entry['type'] === MessageType::Error
956-
);
957-
$this->assertCount(1, $errors);
958-
$this->assertEquals(
959-
current($errors)['message'],
960-
'Question "Orphan question" has no section. It will not be migrated.'
961-
);
948+
// Check that the question hasn't been migrated
949+
$question = new Question();
950+
$this->assertFalse($question->getFromDBByCrit([
951+
'name' => 'Orphan question',
952+
]));
962953
}
963954

964955
public function testFormMigrationUpdateHorizontalRanks(): void
@@ -1165,6 +1156,78 @@ public function testFormMigrationTagConversion(string $rawContent, string $expec
11651156
);
11661157
}
11671158

1159+
public function testFormMigrationWithRadioQuestion(): void
1160+
{
1161+
/**
1162+
* @var \DBmysql $DB
1163+
*/
1164+
global $DB;
1165+
1166+
// Insert a new form
1167+
$this->assertTrue($DB->insert(
1168+
'glpi_plugin_formcreator_forms',
1169+
[
1170+
'name' => 'Test form migration for radio question',
1171+
]
1172+
));
1173+
1174+
// Insert a new section
1175+
$this->assertTrue($DB->insert(
1176+
'glpi_plugin_formcreator_sections',
1177+
[
1178+
'name' => 'Test form migration for radio question - Section',
1179+
'plugin_formcreator_forms_id' => $DB->insertId()
1180+
]
1181+
));
1182+
1183+
// Insert a new question with type "radio"
1184+
$section_id = $DB->insertId();
1185+
$this->assertTrue($DB->insert(
1186+
'glpi_plugin_formcreator_questions',
1187+
[
1188+
'plugin_formcreator_sections_id' => $section_id,
1189+
'name' => 'Test form migration for radio question - Question',
1190+
'fieldtype' => 'radios',
1191+
'default_values' => '1',
1192+
'values' => json_encode(['1', '2', '3']),
1193+
]
1194+
));
1195+
1196+
$migration = new FormMigration($DB, FormAccessControlManager::getInstance());
1197+
$this->setPrivateProperty($migration, 'result', new PluginMigrationResult());
1198+
$this->assertTrue($this->callPrivateMethod($migration, 'processMigration'));
1199+
1200+
// Check that the question has been migrated
1201+
/** @var Question $question */
1202+
$question = getItemByTypeName(Question::class, 'Test form migration for radio question - Question');
1203+
$this->assertInstanceOf(Question::class, $question);
1204+
1205+
/** @var QuestionTypeRadio $question_type */
1206+
$question_type = $question->getQuestionType();
1207+
$this->assertInstanceOf(QuestionTypeRadio::class, $question_type);
1208+
1209+
$this->assertEquals(
1210+
[
1211+
[
1212+
'uuid' => 0,
1213+
'value' => '1',
1214+
'checked' => true,
1215+
],
1216+
[
1217+
'uuid' => 1,
1218+
'value' => '2',
1219+
'checked' => false,
1220+
],
1221+
[
1222+
'uuid' => 2,
1223+
'value' => '3',
1224+
'checked' => false,
1225+
]
1226+
],
1227+
$question_type->getValues($question)
1228+
);
1229+
}
1230+
11681231
public static function provideFormMigrationConditionsForQuestions(): iterable
11691232
{
11701233
yield 'QuestionTypeShortText - Always visible' => [

‎src/Glpi/Form/Migration/FormMigration.php

+44-25
Original file line numberDiff line numberDiff line change
@@ -447,11 +447,10 @@ private function processMigrationOfSections(): void
447447
'PluginFormcreatorForm',
448448
$raw_section['plugin_formcreator_forms_id']
449449
)['items_id'] ?? 0;
450+
451+
// If the form ID is 0, it means the form does not exist
450452
if ($form_id === 0) {
451-
$this->result->addMessage(MessageType::Error, sprintf(
452-
'Section "%s" has no form. It will not be migrated.',
453-
$raw_section['name']
454-
));
453+
// No need to warn the user, as this section was not visible in formcreator anyway.
455454
continue;
456455
}
457456

@@ -509,11 +508,10 @@ private function processMigrationOfQuestions(): void
509508
'PluginFormcreatorSection',
510509
$raw_question['plugin_formcreator_sections_id']
511510
)['items_id'] ?? 0;
511+
512+
// If the section ID is 0, it means the section does not exist
512513
if ($section_id === 0) {
513-
$this->result->addMessage(MessageType::Error, sprintf(
514-
'Question "%s" has no section. It will not be migrated.',
515-
$raw_question['name']
516-
));
514+
// No need to warn the user, as this question was not visible in formcreator anyway.
517515
continue;
518516
}
519517

@@ -544,20 +542,34 @@ private function processMigrationOfQuestions(): void
544542
'uuid' => $raw_question['uuid']
545543
], fn ($value) => $value !== null);
546544

547-
$question = $this->importItem(
548-
Question::class,
549-
$data,
550-
[
551-
'uuid' => $raw_question['uuid'],
552-
]
553-
);
545+
try {
546+
$question = $this->importItem(
547+
Question::class,
548+
$data,
549+
[
550+
'uuid' => $raw_question['uuid'],
551+
]
552+
);
554553

555-
$this->mapItem(
556-
'PluginFormcreatorQuestion',
557-
$raw_question['id'],
558-
Question::class,
559-
$question->getID()
560-
);
554+
$this->mapItem(
555+
'PluginFormcreatorQuestion',
556+
$raw_question['id'],
557+
Question::class,
558+
$question->getID()
559+
);
560+
} catch (\Throwable $th) {
561+
$section = Section::getById($section_id) ?: null;
562+
$this->result->addMessage(
563+
MessageType::Error,
564+
sprintf(
565+
__('Error while importing question "%s" in section "%s" and form "%s": %s'),
566+
$raw_question['name'],
567+
$section?->getName(),
568+
$section?->getItem()?->getName(),
569+
$th->getMessage()
570+
)
571+
);
572+
}
561573

562574
$this->progress_indicator?->advance();
563575
}
@@ -915,8 +927,11 @@ private function processMigrationOfDestination(
915927
)['items_id'] ?? 0;
916928

917929
$form = new Form();
930+
931+
// If the form is invalid, skip this target
918932
if (!$form->getFromDB($form_id)) {
919-
throw new LogicException("Form with id {$raw_target['plugin_formcreator_forms_id']} not found");
933+
// No need to warn the user, as this destination was not visible in formcreator anyway.
934+
continue;
920935
}
921936

922937
$fields_config = [];
@@ -934,9 +949,10 @@ private function processMigrationOfDestination(
934949
$this->result->addMessage(
935950
MessageType::Error,
936951
sprintf(
937-
__('The "%s" destination field configuration of the form "%s" cannot be imported.'),
952+
__('The "%s" destination field configuration of the form "%s" cannot be imported : %s'),
938953
$configurable_field->getLabel(),
939-
$form->getName()
954+
$form->getName(),
955+
$th->getMessage()
940956
)
941957
);
942958
}
@@ -1018,8 +1034,11 @@ private function processMigrationOfITILActorsFields(
10181034
$raw_target_actor['items_id']
10191035
)['items_id'] ?? 0;
10201036

1037+
// The destination ID is 0 if the target was not migrated or not existing.
1038+
// In this case, we skip the actor
10211039
if ($target_id === 0) {
1022-
throw new LogicException("Destination for target id {$raw_target_actor['items_id']} not found");
1040+
// No need to warn the user, as this actor config was not visible in formcreator anyway.
1041+
continue;
10231042
}
10241043

10251044
$targets_actors[$target_id][$raw_target_actor['actor_role']][$raw_target_actor['actor_type']][] =

‎src/Glpi/Form/QuestionType/AbstractQuestionTypeSelectable.php

+8-1
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,15 @@ public function convertDefaultValue(array $rawData): array
188188
return [];
189189
}
190190

191+
/**
192+
* New default values format require an array of values.
193+
* The old system did not use an array if there was only one element.
194+
*/
191195
$default_values = json_decode($rawData['default_values']);
192-
if ($default_values === null && json_last_error() !== JSON_ERROR_NONE) {
196+
if (
197+
($default_values === null && json_last_error() !== JSON_ERROR_NONE)
198+
|| !is_array($default_values)
199+
) {
193200
$default_values = [$rawData['default_values']];
194201
}
195202

‎src/Glpi/Form/QuestionType/QuestionTypeUrgency.php

+5-1
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,11 @@ public function getConditionHandlers(
194194
#[Override]
195195
public function convertDefaultValue(array $rawData): ?int
196196
{
197-
return $rawData['default_values'] ?? null;
197+
if (!isset($rawData['default_values'])) {
198+
return null;
199+
}
200+
201+
return (int) $rawData['default_values'];
198202
}
199203

200204
#[Override]

0 commit comments

Comments
 (0)