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

Add User itemtype to Licenses linked items #18335

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

Conversation

RomainLvr
Copy link
Contributor

@RomainLvr RomainLvr commented Nov 18, 2024

Checklist before requesting a review

Please delete options that are not relevant.

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

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

Adding the relationship of Licenses and Contracts to Users

Linked with #18388

Screenshots (if appropriate):

Licenses :

image

Contracts :
image

@AdrienClairembault
Copy link
Contributor

The discussion in #13294 seems to to indicate that this feature was refused last time.

I've also found this on our gitlab: https://gitlab.teclib.com/editions/projects/features-request/-/issues/133

@cconard96
Copy link
Contributor

AFAIK this still hasn't been planned out and there is so much related to license management that people have been asking for for years that just isn't possible yet. It only really makes sense to me to do a complete rework.

@@ -462,7 +462,7 @@

$CFG_GLPI["cluster_types"] = ['Computer', 'NetworkEquipment'];

$CFG_GLPI['operatingsystem_types'] = ['Computer', 'Monitor', 'NetworkEquipment', 'Peripheral', 'Phone', 'Printer'];
$CFG_GLPI['operatingsystem_types'] = ['Computer', 'Monitor', 'NetworkEquipment', 'Peripheral', 'Phone', 'Printer', 'User'];
Copy link
Member

Choose a reason for hiding this comment

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

operatingsystem_types is supposed to be a list of assets itemtypes. All the codebase, either in GLPI and the plugins, is based on the assertion that it contains asset on which an OS can be installed. I think it is not a good idea to add the User class here.

@@ -269,7 +269,7 @@
'Peripheral', 'Phone', 'Printer', 'Project', 'Line',
'Software', 'SoftwareLicense', 'Certificate',
'DCRoom', 'Rack', 'Enclosure', 'Cluster', 'PDU', 'Appliance', 'Domain',
'DatabaseInstance',
'DatabaseInstance', 'User',
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to be out of the scope of the ticket !34567.

I do not know what could the side effects of adding this. This clearly requires an in-depth analysis to be sure that it would not have unexcepted side effect.
For instance, the fact that the entities_id of a user has nothing to do with the entity scope where it could be visible may have many side effects in the proposed dropdown values, the items lists content, ...

@orthagh
Copy link
Contributor

orthagh commented Nov 25, 2024

I don't remember at all the reason why #13294 was refused.
After checking, the important point is the summary tab and the countdown of used licenses and this seems to work correctly.

Maybe there are issues (I didn't check) with linking entities of users, which are managed differently than in other objects.
Also, if we start adding more objects to licenses, we need a dedicated CFG_GLPI constant.

As Curtis says, we still should rework licenses and at least decouple them from software itemtype, but regarding assignments to users, I don't see UX issues. As said above, there may exist technical problems.

@RomainLvr RomainLvr changed the title Add User itemtype to Licenses & Contracts linked items Add User itemtype to Licenses linked items Nov 26, 2024
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.

5 participants