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

Fix for problem with miq_group (find_by_description) #15813

Closed
wants to merge 1 commit into from

Conversation

arnikasky
Copy link
Contributor

Hi, developers
It's my fix for issue #15700
Faced with issue, when had problem with database and first record which was taken from database, was wring.

Best Regards,
Hanna Novikava

@miq-bot
Copy link
Member

miq-bot commented Aug 15, 2017

Checked commit arnikasky@e809fa5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 3 offenses detected

app/models/miq_group.rb

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Good eye,

Please move the fix up a few lines.
Or just ping me and I can introduce the change.

(I offer to you so you can get credit)
Also, you probably want to make these changes on a branch instead of your master - but that is just to make your life easier. all the same for the merger

@@ -64,7 +64,7 @@ def self.seed
roles = MiqUserRole.where("name like 'EvmRole-%'").index_by(&:name)

role_map.each_with_index do |(group_name, role_name), index|
group = groups[group_name] || new(:description => group_name)
group = MiqGroup.where(description: group_name, tenant_id: Tenant.root_tenant).first || new(:description => group_name)
Copy link
Member

Choose a reason for hiding this comment

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

The introduction of groups was to avoid the N+1. This adds it back.

Introduce this change 3 lines up creating groups.

groups = where(:group_type => SYSTEM_GROUP, tenant_id: Tenant.root_tenant).includes(:entitlement).index_by(&:description)

@kbrock
Copy link
Member

kbrock commented Aug 22, 2017

Closing in lieu of #15876

@kbrock kbrock closed this Aug 22, 2017
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