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

Fix user default entity injection #433

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

Conversation

Lainow
Copy link
Contributor

@Lainow Lainow commented Oct 16, 2024

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.
  • This change requires a documentation update.

Description

  • It fixes !34752
  • Here is a brief description of what this PR does

Fix default entity insertion for a user

@Lainow Lainow self-assigned this Oct 16, 2024
inc/userinjection.class.php Outdated Show resolved Hide resolved
@Lainow Lainow requested a review from Rom1-B October 17, 2024 09:47
@stonebuzz
Copy link
Contributor

waiting from customer feedback

@Lainow Lainow requested a review from stonebuzz October 17, 2024 12:47
Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

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

seems good

@stonebuzz
Copy link
Contributor

waiting from customer feedback

@Lainow Lainow requested a review from stonebuzz October 18, 2024 14:38
@Lainow
Copy link
Contributor Author

Lainow commented Oct 22, 2024

waiting from customer feedback

Validated by the customer

inc/commoninjectionlib.class.php Outdated Show resolved Hide resolved
@Lainow Lainow requested review from Rom1-B and trasher October 23, 2024 08:54
@trasher
Copy link
Contributor

trasher commented Oct 23, 2024

Hostely, I do not know this plugin, nor the way it works... But as far as I understand, you've replace completename with name; this sounds like an important change that mays broke the way it works until now.

Since there is no unit tests, I really do not know if it has an impact... But I guess we should be sure before changing that kind of things.

Copy link
Contributor

@stonebuzz stonebuzz left a comment

Choose a reason for hiding this comment

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

Can you get the customer to validate this new version?

@Lainow
Copy link
Contributor Author

Lainow commented Nov 4, 2024

Can you get the customer to validate this new version?

All good for him

inc/commoninjectionlib.class.php Outdated Show resolved Hide resolved
@Lainow Lainow requested a review from stonebuzz November 5, 2024 10:11
@stonebuzz
Copy link
Contributor

a more precise comment on the difficulty of going through the GLPI core (restriction of entities etc ...)

@Lainow Lainow requested a review from Rom1-B November 7, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants