Fix performance drop-down button in container view

[yaacov]

Description

compute -> containers -> containers -> [ chose one container ] -> monitoring -> utilization

The utilization button is not working.

a. fix the button.
b. fix the message form "VM" to "Container" when no data.

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

Screenshot

Before:

After:

[yaacov]

@miq-bot add_label bug, compute/containers

@simon3z @moolitayer @zeari @himdel @mzazrivec please review

@zakiva - thanks for the help :-)

[yaacov]

@himdel @AparnaKarve PTAL

[moolitayer]

@yaacov we want this button to be disabled if there is no data like in vms

has_perf_data? does not work for containers?

[yaacov]

@yaacov we want this button to be disabled if there is no data like in vms

We can not use the vms button ApplicationHelper::Button::VmPerf because it show the message:
"No C&U data has been collected for this VM" and container is not VM.

we want this button to be disabled if there is no data

In all other container objects, we show the button, and only after we get into the page we get the "no data message", this is the behaviour for container -> pods [1] and contianer -> nodes [2] . Container should have the same behaviour.

[1] https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/toolbar/container_group_center.rb#L21
[2] https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/application_helper/toolbar/container_node_center.rb#L21

[himdel]

If all this meant to fix is the message, can you just update the message?

Either copy VmPerf to ContainerPerf and update the message, or add a field to the definition which you can read in disabled?.

(But keep the message whole, even though you need 2 copies that way :).)

[yaacov]

If all this meant to fix is the message,

No, the main task of this PR is to:
a. prevent the graying out of the "Utilization" button in the drop down.
b. make this button work :-) and send us to the Utilization page.
c. the "VM" message is not mentioned in the BZ, but we should fix it as well.

This behaviour (not graying out) is consistent with the behaviour of this button in the providers, nodes and pods pages, and also with the behaviour of the "Timeline" button in providers.

We opened a new BZ about making all the buttons consistent [1] but this is not a priority.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1512276

[moolitayer]

We can not use the vms button ApplicationHelper::Button::VmPerf because it show the message:
"No C&U data has been collected for this VM" and container is not VM.

yeah that would require a different button. Anyway I looked at ems_container_center and there I see we use a different ApplicationHelper::Button::GenericFeatureButton. That makes me think we can use a new button for all of these mixing the generic feature behavior (can the user view C & U?) with disabling like done in the VM button(I see no reason to behave differently then VMs)

[yaacov]

That makes me think we can use a new button for all of these mixing the generic feature behavior (can the user view C & U?) with disabling like done in the VM button(I see no reason to behave differently then VMs)

This bug [1] is about enabling this button, like all the other buttons.

Fixing the behaviour of all the buttons to match the behaviour of vms is a good issue. We should discuss that, the BZ looks like a good place for that discussion.

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1509674

[yaacov]

that makes me think we can use a new button for all of these mixing the generic feature behavior (can the user view C & U?)

p.s.
@moolitayer we already have a button for that ( made with the help of @ohadlevy ) , we made it for this PR , but Einat said she want to have the button enabled, it's not a problem to implemnt, just not the scope of this PR.

[moolitayer]

After discussion, it seems disabling the button ATM would be detrimental for automation.

We will open a new issue/bug to change the behavior accordingly in all pages (hide/disable when needed)

[yaacov]

@himdel please review.

p.s.
The similar code codeclimate is unhappy about is true, but it's the template we use for buttons, I can't see how to solve it.

[himdel]

OK, so.. having a second class just for your container perf button is perfectly allright, if the logic and the message are indeed different.. feel free to do that.

But removing the disable logic altogether is wrong.

[miq-bot]

Checked commit yaacov@f1631b6 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

[yaacov]

@himdel updated to description, and added screen shots.

[himdel]

Tested in UI...

before:

POST /container/button/10000000000908?pressed=container_perf
No template found for ContainerController#button, rendering head :no_content

now:

GET /container/show/10000000000908?display=performance
(renders the screen)

[yaacov]

@miq-bot add_label gaprindashvili/yes

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1509674 target: 5.9.0

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit b558aa1748b8f4ddc74947bab11e359aefed2669
Author: Martin Hradil <[email protected]>
Date:   Mon Nov 13 17:37:47 2017 +0000

    Merge pull request #2653 from yaacov/fix-containers-performance-button
    
    Fix performance drop-down button in container view
    (cherry picked from commit 983bd1d690cd684d8e0f730f2e170fc2e3d69c7f)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1513597