Fixed correct setting of record to get counts of relationships

[h-kataria]

• Fixed to use CloudTenant record to get counts of relationships for Cloud Tenant dashboard, prior to changes in this PR, code was using a Provider record to get counts and was also passing in an incorrect controller to the CloudTenantDashboardService module.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1731735
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1487178

before

after

[himdel]

@h-kataria We should not be calling the CloudTenant @ems, right?

But also, looks like in EmsDashboardService, recent_records is using @ems.id to search for Vm|Template by ems_id. Which will now fail because @ems.id is actually a tenant id (as opposed to @ems_id).

(Also, format_data is using @ems.send, that probably works, but the @ems name is still misleading.)

---

Maybe the real problem is that CloudTenantDashboardService should not inherit from EmsDashboardService but only share a DashboardService or a mixin that doesn't assume the record is an ems?

Or can we just get rid of the @ems name and use @record, and move anything that needs to know the class of the record to the service that actually knows it?

[h-kataria]

@h-kataria We should not be calling the CloudTenant @ems, right?

But also, looks like in EmsDashboardService, recent_records is using @ems.id to search for Vm|Template by ems_id. Which will now fail because @ems.id is actually a tenant id (as opposed to @ems_id).

(Also, format_data is using @ems.send, that probably works, but the @ems name is still misleading.)

Maybe the real problem is that CloudTenantDashboardService should not inherit from EmsDashboardService but only share a DashboardService or a mixin that doesn't assume the record is an ems?

Or can we just get rid of the @ems name and use @record, and move anything that needs to know the class of the record to the service that actually knows it?

@himdel i agree with you, CloudTenant Dashboard should not be using the shared code with EmsDashboardService. Let me try to fix that, since these BZs need to be backported i was trying to keep changes to minimum.

[himdel]

Aah,
thanks :) Yeah, I think we should fix that, but if you want to backport a minimal change,
I think just changing @ems.id to @ems_id in recent_records may do the trick.

[h-kataria]

@himdel please re-review

[himdel]

I wonder if this is right after all.

I mean, we're showing a list of vms or images whcih have the same ems as the cloud tenant.
This is definitely better than before, where we would show vms and images of a random EMS (that happened to have the same db id as the cloud tenant).

But.. don't we have a ..more directl link between cloud tenant and instances/images?

[himdel]

... we do, @record.vms and @record.miq_templates.

So, what about adding a relation argument to recent_records and recent_resources and doing..

def recent_records(model, relation)
    db_table     = model.arel_table
    to_char_args = [db_table[:created_on], Arel::Nodes::SqlLiteral.new("'YYYY-MM-DD'")]
    group_by_sql = Arel::Nodes::NamedFunction.new("to_char", to_char_args)

   relation.where(db_table[:created_on].gt(30.days.ago.utc))
         .group(group_by_sql.to_sql)
         .count
end

def recent_images_data
  recent_resources(MiqTemplate, _('Recent Images'), _('Images'), @record.miq_templates)
end

def recent_instances_data
  recent_resources(ManageIQ::Providers::CloudManager::Vm, _('Recent Instances'), _('Instances'), @record.vms)
end

[himdel]

LGTM, thanks for splitting this! :)

Just that cloud_tenant.vms vs all vms under cloud_tenant.ext_management_system difference :)

[h-kataria]

LGTM, thanks for splitting this! :)

Just that cloud_tenant.vms vs all vms under cloud_tenant.ext_management_system difference :)

@himdel thx for the suggestion on passing the relation, implemented that, it works better now.

[miq-bot]

Checked commits h-kataria/manageiq-ui-classic@6440497~...1371c00 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 👍

[himdel]

LGTM, seeing consistent counts in the dashboard and the list screen :)

[simaishi]

Ivanchuk backport details:

$ git log -1
commit 482aa40349860fac6d8d903f8c0132beece02494
Author: Martin Hradil <[email protected]>
Date:   Fri Aug 16 15:27:01 2019 +0200

    Merge pull request #5995 from h-kataria/fixed_cloud_tenant_dashboard_view
    
    Fixed correct setting of record to get counts of relationships
    
    (cherry picked from commit 1492f6a42e723d5afce9bcd23a61ff02542a2b6e)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1731735
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1487178