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 attributes to user role #23153

Closed
wants to merge 1 commit into from

Conversation

liu-samuel
Copy link
Contributor

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

Related:
ManageIQ/manageiq-ui-classic#9248

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

@Fryguy
Copy link
Member

Fryguy commented Aug 15, 2024

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

@liu-samuel
Copy link
Contributor Author

liu-samuel commented Aug 15, 2024

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
Copy link
Member

Fryguy commented Aug 15, 2024

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

@Fryguy
Copy link
Member

Fryguy commented Aug 15, 2024

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
Copy link
Contributor Author

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.

add attributes to user role
@miq-bot
Copy link
Member

miq-bot commented Oct 1, 2024

Checked commit liu-samuel@98235e1 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
Copy link
Member

kbrock commented Oct 15, 2024

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
Copy link
Contributor Author

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

@liu-samuel liu-samuel closed this Oct 15, 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.

4 participants