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 ordering by VMs in NetworkManagers list #14092

Merged

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Feb 28, 2017

Fix ordering by VMs in NetworkManagers list

Fixes BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1383307

@Ladas
Copy link
Contributor Author

Ladas commented Feb 28, 2017

@kbrock so this is not very nice, but I think it's the best I can do, until the :through relation here starts to work #12884 . And I mean really work, we need it to do the join :-)

@Ladas
Copy link
Contributor Author

Ladas commented Feb 28, 2017

@miq-bot assign @kbrock

@Ladas Ladas force-pushed the fix_ordering_by_vms_in_network_managers_list branch from 5b86dff to 5c835d4 Compare February 28, 2017 11:42
@Ladas
Copy link
Contributor Author

Ladas commented Feb 28, 2017

before:
screenshot from 2017-02-28 12-45-01
screenshot from 2017-02-28 12-44-51

after:

screenshot from 2017-02-28 12-36-48
screenshot from 2017-02-28 12-36-35

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.

this looks good.

just the 1 comment and the code change

@@ -25,6 +25,21 @@ class << model_name
has_many :load_balancer_health_checks, :foreign_key => :ems_id, :dependent => :destroy
has_many :load_balancer_health_check_members, :through => :load_balancer_health_checks

# Generates ORDER BY ((SELECT COUNT(*) FROM "vms" WHERE "ext_management_systems"."parent_ems_id" = "vms"."ems_id"))
Copy link
Member

Choose a reason for hiding this comment

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

a. is it possible to define this higher up in the inheritance chain?

rewrite:

# Uses "ext_management_systems"."parent_ems_id" instead of "ext_management_systems"."id"
#
# ORDER BY ((
#   SELECT COUNT(*)
#   FROM "vms"
#   WHERE "ext_management_systems"."parent_ems_id" = "vms"."ems_id"
# ))

Copy link
Contributor Author

@Ladas Ladas Feb 28, 2017

Choose a reason for hiding this comment

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

hm I think it need to be here, it's specific to NetworkManager (or StorageManager). So any manager that has Vms through :parent_manager

Reformat the comment nicely
@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2017

Checked commits Ladas/manageiq@5c835d4~...e2c1e8a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 1 offense detected

app/models/manageiq/providers/network_manager.rb

@kbrock
Copy link
Member

kbrock commented Feb 28, 2017

kicking

@kbrock kbrock closed this Feb 28, 2017
@kbrock kbrock reopened this Feb 28, 2017
@Ladas Ladas closed this Mar 1, 2017
@Ladas Ladas reopened this Mar 1, 2017
@kbrock kbrock added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 1, 2017
@kbrock kbrock merged commit 7d9308b into ManageIQ:master Mar 1, 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.

3 participants