Skip to content
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.

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

feat(forms): Implement form targets migration #19119

Open
wants to merge 2 commits into
base: 11.0/plugin-migration-framework
Choose a base branch
from

Conversation

ccailly
Copy link
Member

@ccailly ccailly commented Mar 3, 2025

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

Implement form targets migration

@cedric-anne cedric-anne force-pushed the 11.0/plugin-migration-framework branch from 5150667 to 7944881 Compare March 3, 2025 18:54
@ccailly ccailly force-pushed the feature/form-migration-targets branch from 6524987 to 5d9d37c Compare March 4, 2025 10:45
@ccailly ccailly self-assigned this Mar 4, 2025
@ccailly ccailly added the Forms label Mar 4, 2025
@ccailly ccailly added this to the 11.0.0 milestone Mar 4, 2025
@ccailly ccailly force-pushed the feature/form-migration-targets branch from 5d9d37c to aa5cb7c Compare March 5, 2025 09:40
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK for me, but I have 2 remarks.

  1. Having some code dedicated to form migrations directly inside the Glpi\Form namespace is probably not the best option. Indeed, the migration code is not supposed to change, and we will want to remove it easilly in a few years, when we will drop the support of GLPI 10.0 in out migration files. Moving all the related code in a Glpi\Migration\Formcreator namespace would be interesting.

  2. With your current logic, if a form is considered as outdated during the import operation, it will not be updated, to keep the GLPI version that is more recent, but you will still try to import its child items (questions, destinations, ...). In Command to create generic assets from genericobject plugin data #19033, I handled this way :

foreach ($raw_forms as $raw_form) {
    $reconciliation_criteria = [
        'name'                => $raw_form['name'],
        'entities_id'         => $raw_form['entities_id'],
        'forms_categories_id' => $this->getMappedItemTarget(
            'PluginFormcreatorCategory',
            $raw_form['plugin_formcreator_categories_id']
        )['items_id'] ?? 0,
    ];

    $existing_form = new Form();
    if (
        $existing_form->getFromDBByCrit($reconciliation_criteria)
        && \strtotime($input['date_mod']) < \strtotime($existing_form- >fields['date_mod'])
    ) {
        $this->result->markItemAsReused(Form::class, $existing_form->getID());
        $this->result->addMessage(
            MessageType::Debug,
            sprintf(
                __('%s "%s" (%d) is most recent on GLPI side, its update has been skipped.'),
                Form::getTypeName(1),
                $existing_form->getFriendlyName() ?: NOT_AVAILABLE,
                $existing_form->getID(),
            )
        );
        continue; // and ignore all child imports
    }

    // import the form and all its child items
}

This is more complex on your side, as you are handling child items in a separate loop, but it is probably better to skip child items import if the form is outdated.

Both improvement can be done in separate PRs, if you think their are not intoducing too much complexity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants