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

Multiple groups for assets #16760

Merged

Conversation

cconard96
Copy link
Contributor

@cconard96 cconard96 commented Mar 12, 2024

Q A
Bug fix? no
New feature? yes
BC breaks? YES
Deprecations? no
Tests pass? NO
Fixed tickets !30462

For assets, the groups_id and groups_id_tech fields are changed to allow multiple selections.

@cconard96 cconard96 added the wip label Mar 12, 2024
@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch 2 times, most recently from f3f4538 to cf98205 Compare March 21, 2024 22:35
@cconard96
Copy link
Contributor Author

Currently broken and I am not making any progress to resolve:

  • Group search option for ReservationItem - DB schema is different between itemtypes that have one vs multiple groups, so the search option isn't the same for everything anymore.
  • Group/Group Tech search options for AllAssets - Same as above
  • Metacriteria Group search options for assets with multiple groups - Same as above

@cedric-anne
Copy link
Member

Currently broken and I am not making any progress to resolve:

  • Group search option for ReservationItem - DB schema is different between itemtypes that have one vs multiple groups, so the search option isn't the same for everything anymore.
  • Group/Group Tech search options for AllAssets - Same as above
  • Metacriteria Group search options for assets with multiple groups - Same as above

What are these itemtypes that have only one group?

@cconard96
Copy link
Contributor Author

What are these itemtypes that have only one group?

Rack, Certificate, Appliance, Item_DeviceSimcard, Line, SoftwareLicense, Unmanaged at least based on the related arrays in inc/define.php. Then, whatever plugins add to these arrays (I don't know which plugins the client is using currently).

@cedric-anne
Copy link
Member

What are these itemtypes that have only one group?

Rack, Certificate, Appliance, Item_DeviceSimcard, Line, SoftwareLicense, Unmanaged at least based on the related arrays in inc/define.php. Then, whatever plugins add to these arrays (I don't know which plugins the client is using currently).

IMHO, All items defined in the linkgroup_types and linkgroup_tech_types config entries should be linkable to multiple groups. All the class your are listing here may probably declared in these config entries, if they are not yet.

@cconard96
Copy link
Contributor Author

In addition to my previous comment, these itemtypes have groups_id or groups_id_tech columns and may be relevant to issues with this PR or issues in the future:

  • Cluster
  • DatabaseInstance
  • DomainRecord
  • Domain
  • Enclosure
  • PassiveDCEquipment
  • PDU

@cconard96
Copy link
Contributor Author

IMHO, All items defined in the linkgroup_types and linkgroup_tech_types config entries should be linkable to multiple groups. All the class your are listing here may probably declared in these config entries, if they are not yet.

That means expanding potentially both the new Read/Update Assigned rights and multiple-groups feature beyond assets which is beyond the scope of the developments that was agreed to.

@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch 2 times, most recently from 1e5bfe1 to 61eef4c Compare April 8, 2024 15:00
@cconard96 cconard96 removed the wip label Apr 11, 2024
@cconard96 cconard96 marked this pull request as ready for review April 11, 2024 11:11
@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch from 61eef4c to 6abb8d4 Compare April 11, 2024 22:51
Copy link
Member

@cedric-anne cedric-anne left a comment

Choose a reason for hiding this comment

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

I still think it would be better to apply this change to all the classes that are present in the linkgroup_types and the linkgroup_tech_types configurations.

  1. It would make this behaviour consistant accross all assets.
  2. It would probably give the ability to validate that the trait methods are correctly executed by doing some tests that loops on all these configured types.

@orthagh @trasher , your opinion on this?

install/mysql/glpi-empty.sql Outdated Show resolved Hide resolved
install/mysql/glpi-empty.sql Outdated Show resolved Hide resolved
src/Features/AssignableAsset.php Outdated Show resolved Hide resolved
install/mysql/glpi-empty.sql Outdated Show resolved Hide resolved
src/Features/AssignableAsset.php Outdated Show resolved Hide resolved
src/Search/Provider/SQLProvider.php Outdated Show resolved Hide resolved
src/Search/Provider/SQLProvider.php Outdated Show resolved Hide resolved
templates/generic_show_form.html.twig Outdated Show resolved Hide resolved
templates/generic_show_form.html.twig Outdated Show resolved Hide resolved
inc/relation.constant.php Outdated Show resolved Hide resolved
@orthagh
Copy link
Contributor

orthagh commented Apr 15, 2024

I still think it would be better to apply this change to all the classes that are present in the linkgroup_types and the linkgroup_tech_types configurations.

I agree.
As discussed with @cedric-anne in Mattermost, the initial selection must be due to the assets profile page.
For the moment, I think we should extend the fields changes to a larger selection (good example: Rack)
The whole "Management" tab of profiles could be adapted as "Assets" tab.
For the lines where the split of right is not relevant (infocom ?), maybe we can disable the relative checkbox.

For things where a profile change is complex, still make the multiple fields. We can manage the rest afterward probably.
Everything considered as an asset should behave the same with these fields.
Plugins are another matter, let's leave it aside for the moment.

That means expanding potentially both the new Read/Update Assigned rights and multiple-groups feature beyond assets which is beyond the scope of the developments that was agreed to.

Do this represent a big work @cconard96 ?
I agree Wuerth has not ordered this, but we will need to maintain the dev after.

@cconard96
Copy link
Contributor Author

I agree. As discussed with @cedric-anne in Mattermost, the initial selection must be due to the assets profile page. For the moment, I think we should extend the fields changes to a larger selection (good example: Rack) The whole "Management" tab of profiles could be adapted as "Assets" tab.

Some things on the Management tab will need to remain. Suppliers, contacts and infocom aren't assets. I'm not sure contracts are either. Would that mean changing which menu the types are under too so that it matches?

For the lines where the split of right is not relevant (infocom ?), maybe we can disable the relative checkbox.

I don't know what this is referring to.

For things where a profile change is complex, still make the multiple fields. We can manage the rest afterward probably. Everything considered as an asset should behave the same with these fields. Plugins are another matter, let's leave it aside for the moment.

That means expanding potentially both the new Read/Update Assigned rights and multiple-groups feature beyond assets which is beyond the scope of the developments that was agreed to.

Do this represent a big work @cconard96 ? I agree Wuerth has not ordered this, but we will need to maintain the dev after.

Depends on what we decide is an asset, I think.

@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch 2 times, most recently from f81c788 to 49e7e5f Compare April 15, 2024 18:50
src/CommonDBTM.php Outdated Show resolved Hide resolved
src/CommonDBTM.php Outdated Show resolved Hide resolved
@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch from e211ddf to 0a3c32c Compare April 16, 2024 16:44
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from c272f00 to 713118d Compare April 17, 2024 12:39
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch 2 times, most recently from a4633aa to 0f05b7c Compare April 18, 2024 06:42
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from 9336ea7 to 47567ad Compare April 24, 2024 14:22
src/Group.php Outdated Show resolved Hide resolved
src/User.php Outdated Show resolved Hide resolved
@cconard96 cconard96 force-pushed the feature/multiple_asset_groups branch from 3d44435 to 2705877 Compare April 30, 2024 14:55
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from 2705877 to b4fd04a Compare May 13, 2024 14:42
@cedric-anne
Copy link
Member

I just rebased to include modifications made in #17021. Maybe some tests will be broken.

@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from b4fd04a to 0fc09a1 Compare May 22, 2024 15:11
@cedric-anne
Copy link
Member

I put the PR back to draft, waiting for the customer feedback.

@cedric-anne cedric-anne marked this pull request as draft May 28, 2024 12:27
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from 05f835d to 34c6e7e Compare July 31, 2024 07:42
@cedric-anne cedric-anne added this to the 11.0.0 milestone Jul 31, 2024
@cedric-anne cedric-anne marked this pull request as ready for review July 31, 2024 07:42
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from 34c6e7e to d12a512 Compare July 31, 2024 08:24
@cedric-anne cedric-anne force-pushed the feature/multiple_asset_groups branch from d12a512 to 2ea2a74 Compare July 31, 2024 09:06
@cedric-anne
Copy link
Member

The customer has validated the feature.

I just rebased the branch to fix conflicts.

@cedric-anne cedric-anne merged commit 6edfb3d into glpi-project:main Jul 31, 2024
9 checks passed
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.

4 participants