Skip to content

[Feature] Allow multiple "Insertion in the form" (dom) blocks per entity #941

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

JeremieMercier
Copy link

This PR provides the ability to define multiple "Insertion in the form" blocks for the same item based on its entity

  • Created a new method findContainers() (based on findContainer()) that returns all 'dom' containers for an item based on its entity (with parent entity handling via getAncestorsOf()).
  • Adapted hooks (pre_item_add, pre_item_update, post_item_add, post_item_update) to manage multiple containers using the _plugin_fields_data_multi array.
  • Updated the populateData() function to extract input values by stripping the prefix, ensuring that data is saved into the correct columns of the injection table.
  • Modified the container.form.php file to "clean" the form data (by removing the prefix) before calling updateFieldsValues(), thereby enabling the saving of domtab containers.

Please test these modifications and verify that everything works as expected for all object types supported by the Fields plugin. Note that my tests were successful, although I focused exclusively on 'Ticket' type objects for my use case.

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Issues : #744, #789, #834

@stonebuzz
Copy link
Contributor

Hi @JeremieMercier

I am questioning the plugin's ability to reconcile the correct container during API calls, an issue we have encountered in the past. For example, when updating a ticket and including the "fields" in the payload, could you verify this point?

@JeremieMercier
Copy link
Author

JeremieMercier commented Apr 9, 2025

Hi @stonebuzz,

I retrieve the block row ID like this:
image

Then, I update the field using the following method:
image

So I haven’t encountered any issue with the API — unless I misunderstood how you're updating the fields via the API?

@stonebuzz
Copy link
Contributor

It is also possible to update fields from the fields plugin during ticket update.

PUT {{URL_GLPI}} /Ticket/5

@JeremieMercier
Copy link
Author

JeremieMercier commented Apr 9, 2025

Yes, it works

Just include the block ID in the field name, like:
plugin_fields_15_bloconechamponefield

image
image

@JeremieMercier
Copy link
Author

Hello, do you have time to review the PR ?

@JeremieMercier
Copy link
Author

I applied the changes for PHPStan 🤞

@JeremieMercier
Copy link
Author

JeremieMercier commented Apr 17, 2025

I don't understand @trasher

image

With php 7.4 :

image

@JeremieMercier
Copy link
Author

A syntax error, it was slipped in after applying the suggestion 😅

@JeremieMercier
Copy link
Author

Are we good this time ? 😅🤞

@JeremieMercier
Copy link
Author

Hi, what's the next step ?

@stonebuzz
Copy link
Contributor

stonebuzz commented Apr 24, 2025

Hi @JeremieMercier

The feature you are proposing is substantial and introduces significant changes. To ensure its smooth integration and functionality, we must be extremely vigilant. This means we need to "lock down" our testing to guarantee that everything works as expected.

It is important to acknowledge that the plugin is already complex to maintain, especially with the new features introduced in GLPI. Given this complexity, we must approach any additional features with caution to ensure they do not further complicate maintenance and support.

To ensure the proper functioning of this PR, here is a concise list of unit tests that should be added:

Unit Tests for findContainers

  1. Retrieve Containers for Specific Itemtype and Entity

    • Verify that findContainers returns the correct containers for a given itemtype and entity.
  2. Retrieve Containers with Subtype

    • Verify that findContainers returns the correct containers when a subtype is specified.
  3. Retrieve Containers with Recursive Entity

    • Verify that findContainers considers recursive entities.

Unit Tests for Hooks

  1. preItemAdd with Multiple Containers

    • Verify that the preItemAdd hook correctly handles data for multiple containers.
  2. postItemAdd with Multiple Containers

    • Verify that the postItemAdd hook correctly updates field values for multiple containers.
  3. preItemUpdate with Multiple Containers

    • Verify that the preItemUpdate hook correctly handles data for multiple containers during item update.

Unit Tests for populateData

  1. populateData with Prefix

    • Verify that populateData correctly extracts field values by removing the prefix.
  2. populateData with Multiple Selection Fields

    • Verify that populateData correctly handles fields allowing multiple selections.

Unit Tests for showForTab

  1. showForTab with Multiple Containers

    • Verify that showForTab correctly displays containers for a given item.
  2. showForTab with Insufficient Rights

    • Verify that showForTab returns nothing if the user does not have sufficient rights on the containers.

Unit Tests for API

  1. Ticket input update

  2. Container update

Unit Tests for rights

@JeremieMercier
Copy link
Author

Hi @stonebuzz Thanks for the message

I fully understand the concerns and the importance of thorough testing.
I'm currently working on implementing the required tests and will make sure all the listed cases are properly covered.

While working on the tests already partially in place, I’ve been able to identify and fix some issues especially related to "dropdown" field handling and container restriction management.

I’ll keep updating here as I progress.

@akstis-typer
Copy link

akstis-typer commented May 23, 2025

Hi, @JeremieMercier.
What tool you are using to send HTTP requests?
I'm newby in WEB development so I'm not entirely shure what tool is that.
Thank you :)

Edit: I've done some research and found Postman, as a tool to send HTTP requests and creating API tests

@JeremieMercier
Copy link
Author

I’ve added all the requested unit tests (except the API ones):

  • findContainers : basic cases, sub-types, entity recursion
  • Hooks: preItem, postItemAdd, preItemUpdate with multiple containers
  • populateData: prefix handling and multi-selection fields
  • showForTab: displaying multiple containers and checking rights

Please note I’m not a testing expert, so the tests may not follow every usual best practice. Feel free to share any feedback so I can improve.

The API tests still need to be written by whoever is willing to tackle them.

@JeremieMercier JeremieMercier force-pushed the feature/multiple-dom-blocks branch from 9090d78 to 536a7cf Compare May 25, 2025 21:48
    Created a new method findContainers() (based on findContainer()) that returns all 'dom' containers for an item based on its entity (with parent entity handling via getAncestorsOf()).

    Adapted hooks (pre_item_add, pre_item_update, post_item_add, post_item_update) to manage multiple containers using the _plugin_fields_data_multi array.

    Updated the populateData() function to extract input values by stripping the prefix, ensuring that data is saved into the correct columns of the injection table.

    Modified the container.form.php file to "clean" the form data (by removing the prefix) before calling updateFieldsValues(), thereby enabling the saving of domtab containers.

This PR provides the ability to define multiple "Insertion in the form" blocks for the same item based on its entity by leveraging the new findContainers() method and adapting the save process.
…oving the prefix) before calling updateFieldsValues(), thereby enabling the saving of domtab containers.
- Added missing @var annotation for global $DB
- Replaced empty() on $entityIds with count()
- Removed redundant is_array() check on $entityRestriction
- Simplified isset() condition by removing unnecessary !== null check
Note: PHPStan still reports a type error on DB::request() (expects array<string>|string),
but this issue already exists elsewhere in the file and was not introduced by this change.
update: showForTab method
fix: dropdown field
• New phpunit.xml and bootstrap for the Fields plugin
• PluginFieldsContainerTest covers:
  - Creating / reading containers
  - Filtering containers by entity, subtype, recursion
  - Hooks: preItem, postItemAdd, preItemUpdate
  - populateData (single + multi-dropdown)
  - showForTab rendering and rights checks
@JeremieMercier JeremieMercier force-pushed the feature/multiple-dom-blocks branch from 051fcd8 to cf0f71f Compare May 29, 2025 16:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants