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

Allow dashboards for different groups to have the same name #19491

Merged
merged 3 commits into from
Nov 14, 2019

Conversation

yrudman
Copy link
Contributor

@yrudman yrudman commented Nov 11, 2019

Fixes #18924

This PR should go together with Data Migration PR ManageIQ/manageiq-schema#432, which will copy owner_id to group_id

@miq-bot miq-bot added the wip label Nov 11, 2019
@yrudman yrudman force-pushed the dashboard-name-uniqueness-for-group branch 2 times, most recently from 360fa5b to 208f611 Compare November 11, 2019 19:01
@yrudman yrudman force-pushed the dashboard-name-uniqueness-for-group branch 2 times, most recently from 6c8c7ee to a7bb025 Compare November 11, 2019 21:05
@yrudman yrudman changed the title [WIP] allow to have the same dashboard name for different group Allow dashboard for different group to have the same name Nov 11, 2019
@yrudman yrudman changed the title Allow dashboard for different group to have the same name Allow dashboards for different groups to have the same name Nov 11, 2019
@miq-bot miq-bot removed the wip label Nov 11, 2019
userid = user.try(:userid)
group_id = user.try(:current_group_id)
# a unique record is defined by name, group_id and userid
where(:name => name, :group_id => group_id, :userid => userid)
if userid.present?
where(:name => name, :group_id => group_id, :userid => userid)
Copy link
Member

Choose a reason for hiding this comment

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

You can also basically remove the group_id variable now, and just use user.current_group_id, since the group_id isn;t needed elsewhere, and you don't need the try because you've checked userid is present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@Fryguy
Copy link
Member

Fryguy commented Nov 12, 2019

I expected to see a change to a validator in this code, but there isn't one.

I'm also curious if the data migration and most of this code is even needed if we can get away with group_id || owner_id. I'm sure I'm overlooking something but that's the feeling I get from reading it.

@yrudman
Copy link
Contributor Author

yrudman commented Nov 13, 2019

I expected to see a change to a validator in this code, but there isn't one.

No need to change validation in lib/extentions/ar_miq_sert.rb:

validates_uniqueness_of :name,
                        :scope => [:set_type, :userid, :group_id],
                        :if    => proc { |c| c.class.in_my_region.exists?(:name => c.name) }

the same logic still, but instead of group_id been nil for dashboards assigned to group (and preventing different group to have the same name) it will be <group_id>

.
.

I'm also curious if the data migration and most of this code is even needed if we can get away with group_id || owner_id.

  • first thought is: why to use group_id if owner_id already holds that data, BUT acts_as_miq_set is used in 8 models. It would be a massive refactoring to drop group_id column from miq_sets table and alter validation above - very intrusive changes for this small improvement

  • without data migration duplicated dashboards for group (the same name) can be created, since old dashboard will have group_id nil and new ones will have <group_id>

@yrudman yrudman force-pushed the dashboard-name-uniqueness-for-group branch from a7bb025 to 6ca85cb Compare November 13, 2019 15:40
@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2019

Checked commits yrudman/manageiq@c691cd3~...6ca85cb with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

yrudman added a commit to yrudman/manageiq-schema that referenced this pull request Nov 13, 2019
yrudman added a commit to yrudman/manageiq-schema that referenced this pull request Nov 13, 2019
yrudman added a commit to yrudman/manageiq-schema that referenced this pull request Nov 13, 2019
@Fryguy Fryguy merged commit dd27112 into ManageIQ:master Nov 14, 2019
@Fryguy Fryguy added this to the Sprint 125 Ending Nov 25, 2019 milestone Nov 14, 2019
@Fryguy Fryguy self-assigned this Nov 14, 2019
@Fryguy Fryguy added the bug label Nov 14, 2019
@yrudman yrudman deleted the dashboard-name-uniqueness-for-group branch November 14, 2019 14:57
jrafanie added a commit to jrafanie/manageiq-ui-classic that referenced this pull request Nov 14, 2019
We previously expected a duplicate dashboard name to raise an error and it no
longer is a problem as of ManageIQ/manageiq#19491
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard names should only be unique within a group
3 participants