search

ManageIQ / manageiq

ManageIQ Open-Source Management Platform
https://manageiq.org
Apache License 2.0
1.35k stars 900 forks source link

Add attributes to user role #23153

Closed liu-samuel closed 1 month ago

liu-samuel commented 3 months ago

Add attributes to user role to allow for storing of checked features and unique ids for each product feature

Related: https://github.com/ManageIQ/manageiq-ui-classic/pull/9248

@miq-bot add-label enhancement @miq-bot assign @Fryguy

Fryguy commented 3 months ago

I'm not sure I understand why this is needed? I assume unused user roles might have blank values?

liu-samuel commented 3 months ago

I'm not sure I understand why this is needed? I assume unused user roles might have blank values?

The way the checkbox tree requires that there's no duplicate values, so I had to put on an id for each node in the tree to render it. It also needs the exact id for each checked off box so it loads the correct information for the tree, so I have to save the exact id for any feature that's checked off and saved.

Fryguy commented 3 months ago

hmmm - I thought we already stored this information with unique ids though.

Fryguy commented 3 months ago

Is there a schema migration for these attributes, or do they already exist in the database? I think I need to understand how it's currently modeled to understand why this change is needed.

liu-samuel commented 3 months ago

Is there a schema migration for these attributes, or do they already exist in the database? I think I need to understand how it's currently modeled to understand why this change is needed.

When I went through it, it looked like it was stored by creating a new feature object, but this made it so I wasn't sure which specific node id it mapped to the features when I got it back in the React code

Yeah I had to make a migration, they didn't already exist.

miq-bot commented 2 months ago

Checked commit https://github.com/liu-samuel/manageiq/commit/98235e11f7ebbea44f7cae1925b2874c868f2067 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 1 file checked, 1 offense detected

app/models/miq_user_role.rb

kbrock commented 1 month ago

As a general rule, not a fan of serialized ids, but maybe this is a good aproach. Storing as a jsonb /hstore may alleviate some of the issue with this.

My main concern is having a field named _id and it not being a numeric. If we do go this route, lets at least name the column something that sounds like a hash and not an integer id.

liu-samuel commented 1 month ago

As a general rule, not a fan of serialized ids, but maybe this is a good aproach. Storing as a jsonb /hstore may alleviate some of the issue with this.

My main concern is having a field named _id and it not being a numeric. If we do go this route, lets at least name the column something that sounds like a hash and not an integer id.

I think we found a way to not need to store these ids so I'm actually going to close this PR