FIX / PDF export for assignable asset groups#63
Conversation
|
CHANGELOG please (always update it) |
stonebuzz
left a comment
stonebuzz
left a comment
There was a problem hiding this comment.
Better 🙂
However, we now have a significant amount of duplicated code. We should refactor and consolidate these methods into a single unified implementation.
|
@stonebuzz can I send it to the client ? |
| return implode(', ', array_filter(array_map( | ||
| static fn($group_id) => Toolbox::stripTags(Dropdown::getDropdownName('glpi_groups', $group_id)), | ||
| $group_ids, | ||
| ))); |
There was a problem hiding this comment.
There’s something I’m not fully grasping in this PR.
In GLPI, an object can be assigned to a group using a one-to-one relationship (i.e., the object contains a groups_id or groups_id_tech column, for example in glpi_itilcategories).
Objects can also be assigned to multiple groups through an intermediate relation table, glpi_groups_items (cf: glpi_computers).
These objects rely on that relation table when they use the AssignableItem trait.
Here (unless I’m mistaken), we are only handling the first case.
|
following @stonebuzz comment :
|
Rom1-B
left a comment
Rom1-B
left a comment
There was a problem hiding this comment.
There are no tests to validate the handling of the groups_id/groups_id_tech variables, whether they are an array or a scalar value. This plugin does not yet include a PHPUnit configuration. Could you please add it?
| if ($item instanceof AssignableItemInterface) { | ||
| // Many-to-many relation, stored in the glpi_groups_items pivot table | ||
| global $DB; | ||
| $it = $DB->request([ | ||
| 'SELECT' => 'groups_id', | ||
| 'FROM' => Group_Item::getTable(), | ||
| 'WHERE' => [ | ||
| 'itemtype' => $item::class, | ||
| 'items_id' => $item->getID(), | ||
| 'type' => $group_type, | ||
| ], | ||
| ]); | ||
| $group_ids = array_column(iterator_to_array($it), 'groups_id'); | ||
| } else { | ||
| // One-to-one relation, direct column on the item (e.g. glpi_itilcategories) | ||
| $group_ids = (array) ($item->fields[$field] ?? 0); | ||
| } |
There was a problem hiding this comment.
The AssignableItemInterface branch re-queries glpi_groups_items directly, but this is redundant: AssignableItem::post_getFromDB() (src/Glpi/Features/AssignableItem.php) already calls loadGroupFields() on every getFromDB(), which populates $item->fields['groups_id'] / ['groups_id_tech'] as arrays of group ids straight from glpi_groups_items. Every caller of getGroupName() here reaches it through PluginPdfCommon::addHeader() → $this->obj->getFromDB($ID), so that array is always already loaded by the time this runs.
The else branch's (array) ($item->fields[$field] ?? 0) already covers both shapes uniformly (an array cast is a no-op on an array, and wraps a scalar id in a single-element array), which is exactly the simplification you suggested and had applied in commit 1e1b4f5 before it was reverted by 8231424 ("handle many-to-many groups"). Dropping the instanceof branch removes an extra DB query per group field per item and directly resolves stonebuzz's "significant amount of duplicated code" comment, since it also brings this function in line with what AssignableItem already guarantees instead of adding a parallel lookup.
| $pdf->displayLine( | ||
| '<b><i>' . sprintf( | ||
| __('%1$s: %2$s'), | ||
| __('Group') . '</i></b>', | ||
| Dropdown::getDropdownName( | ||
| 'glpi_groups', | ||
| $computer->fields['groups_id'], | ||
| ), | ||
| self::getGroupName($computer), | ||
| ), | ||
| '<b><i>' . sprintf( | ||
| __('%1$s: %2$s'), | ||
| __('Group in charge of the hardware') . '</i></b>', | ||
| self::getGroupName($computer, Group_Item::GROUP_TYPE_TECH), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
This introduces a duplicate "Group in charge of the hardware" field on the Computer PDF. PluginPdfCommon::mainLine($pdf, $computer, 'group-model') (called at inc/computer.class.php:67, unchanged by this PR) already renders "Group in charge of the hardware" paired with "Model" via the group-model case in common.class.php:538-553. The line added here renders the same tech-group value again, paired with "Group", where the pre-PR code only paired "Group" with "UUID". The result is that the tech group appears twice in the generated PDF, and "UUID" is pushed into an extra half-empty row.
| ); | |
| $pdf->displayLine( | |
| '<b><i>' . sprintf( | |
| __('%1$s: %2$s'), | |
| __('Group') . '</i></b>', | |
| self::getGroupName($computer), | |
| ), | |
| '<b><i>' . sprintf( | |
| __('%1$s: %2$s'), | |
| __('UUID') . '</i></b>', | |
| $computer->fields['uuid'], | |
| ), | |
| ); |
3 participants
Description
groups_idandgroups_id_techwere always scalar values, while GLPI now loads these fields fromglpi_groups_itemsas arrays for assignable items. As a result, group-related fields could be empty or incorrectly rendered in generated PDFs.The classes updated are the ones impacted beacause they correspond to assets implementing the AssignableItem feature.
Screenshots :
Before fix :
After fix :