Skip to content

fix(commoninjectionlib.class.php): normalize group assignment fields for assignable items only#631

Open
TarekRemo wants to merge 3 commits into
pluginsGLPI:mainfrom
TarekRemo:ticket_44708
Open

fix(commoninjectionlib.class.php): normalize group assignment fields for assignable items only#631
TarekRemo wants to merge 3 commits into
pluginsGLPI:mainfrom
TarekRemo:ticket_44708

Conversation

@TarekRemo

Copy link
Copy Markdown
Contributor

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.

Description

  • It fixes !44708
  • the block that normalizes group assignment fields (example : groups_id → _groups_id (array)) was triggered for any item with a search option pointing to glpi_groups. Some items (example: ITILCategory) store groups_id as a direct integer FK, not via Group_Item. For these items, the _groups_id array this block produced was silently ignored, leaving groups_id = 0.
  • Fix: Check that the item being injected has the AssignableItem trait to the guard condition. The rewrite now only happens for items like Computer that actually has this trait and expect group assignments through Group_Item relations.

@stonebuzz

Copy link
Copy Markdown
Contributor

I would appreciate having a unit test covering this part in order to definitively address the issue, as it has come up several times. This would also provide a good starting point for building our unit test coverage.Ideally, this should be validated with a real test involving the injection of a CSV file.

@TarekRemo

Copy link
Copy Markdown
Contributor Author

I would appreciate having a unit test covering this part in order to definitively address the issue, as it has come up several times. This would also provide a good starting point for building our unit test coverage.Ideally, this should be validated with a real test involving the injection of a CSV file.

I added unit tests. Didn't use a real CSV file but the tests simulate the starting point when a real CSV is injected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants