Repository navigation
Conversation
|
Hello @tsmr, Thank you! |
…injection into InjectGlpiAsset
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
Co-authored-by: Stanislas <skita@teclib.com>
Co-authored-by: Stanislas <skita@teclib.com>
Co-authored-by: Stanislas <skita@teclib.com>
Co-authored-by: Stanislas <skita@teclib.com>
|
Hi all! Thank you for this great PR! We tested it and it seems to work well:
We're happy it works now, and of course don't pretend that it is be perfect, however we have some feedback, in order of importance:
Thank you! |
RomainLvr
left a comment
There was a problem hiding this comment.
Re Mirkk's feedback (point 3, custom dropdowns): custom fields use search option ids >= 45000 (CustomFieldDefinition::getSearchOptionID()), but PluginDatainjectionCommonInjectionLib::addToSearchOptions() only flags ids < 1000 as injectable, so they're dropped before the blacklist logic even runs. Even after lifting that, every custom field shares the same linkfield (custom_fields, since values are stored in one JSON column), and addToSearchOptions() keeps only one option per linkfield (dedup added for issue #121) - so only one custom field could ever be importable at a time, and writing a resolved value back would need to target custom_<system_name> rather than custom_fields directly (see Asset::handleCustomFieldsUpdate()). That's bigger than a one-line fix, flagging it so it's tracked rather than silently missing.
| unset($tab[4]); | ||
| unset($tab[40]); |
There was a problem hiding this comment.
Removing these two lines drops the Type (4) and Model (40) dropdown options entirely, so they can never be imported. They come from Asset::rawSearchOptions() and behave like any other dropdown option already supported here (e.g. Manufacturer, 23).
| unset($tab[4]); | |
| unset($tab[40]); |
There was a problem hiding this comment.
As say into the PR : Warning this development don't add type & model injection of GlpiAsset ? So you can continue the development if you want
Co-authored-by: Romain Lecouvreur <102067890+RomainLvr@users.noreply.github.com>
Herafia
left a comment
There was a problem hiding this comment.
Hello @tsmr,
Thanks for these latest updates.
I've included a few small suggestions.
If you'd like, you can also check the “Allow changes by maintainers” box on this PR. That would allow us to push small fixes directly to your branch and speed up the merge.
Thanks!
Co-authored-by: Laura <35998899+Herafia@users.noreply.github.com>
I can't :/ My branch is on our organization's server. But I can give you temporary write access to my branch |
Checklist before requesting a review
Please delete options that are not relevant.
Description
Permit to Inject GlpiAsset (and add Injectable Capacity)
Warning this development don't add type & model injection of GlpiAsset