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.

Sign up for GitHub

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

Jump to bottom

Fix Display Vm option, when clicking on chart #3122

Merged
merged 2 commits into from
Dec 20, 2017
Merged

Fix Display Vm option, when clicking on chart #3122

merged 2 commits into from
Dec 20, 2017

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Dec 20, 2017 •
edited
Loading

Fix Display Vm option, when clicking on chart.

display_current_top functions was called with to parameters instead of one and logic used for evaluate result of RBAC check was inverted.

Links [Optional]

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

GIF

screencast from 2017-12-20 15-08-00

Steps for Testing/QA

  1. Drill any graph select: 'chart' > 'Top VMs on this day' .
  2. Now check for display> other/ vm_name

@PanSpagetka PanSpagetka force-pushed the display-vm-option-chart-fix branch from 2a0045b to d00ab83 Compare December 20, 2017 14:10
@PanSpagetka PanSpagetka force-pushed the display-vm-option-chart-fix branch from d00ab83 to c96714e Compare December 20, 2017 14:58
@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2017

Checked commits PanSpagetka/manageiq-ui-classic@0fe672c~...c96714e with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@himdel
Copy link
Contributor

himdel commented Dec 20, 2017 •
edited
Loading

LGTM, the extra param to display_current_top was introduced in #661 and looks like a clear typo (all the other functions had it).

And the record.present? condition was obviously wrong, agreed.

@himdel himdel added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 20, 2017
@himdel himdel self-assigned this Dec 20, 2017
@himdel himdel added the bug label Dec 20, 2017
@himdel himdel merged commit 82b0eed into ManageIQ:master Dec 20, 2017
@simaishi
Copy link
Contributor

simaishi commented Jan 3, 2018

Gaprindashvili backport details:

$ git log -1
commit 1647f128eb9d2630fef53bce4ec0a897fad8ff01
Author: Martin Hradil <[email protected]>
Date:   Wed Dec 20 18:16:20 2017 +0100

    Merge pull request #3122 from PanSpagetka/display-vm-option-chart-fix
    
    Fix Display Vm option, when clicking on chart
    (cherry picked from commit 82b0eedfef4ecc3a1e298ec942334369fc2ee48d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1530768

simaishi pushed a commit that referenced this pull request Jan 3, 2018
@himdel @simaishi
Merge pull request #3122 from PanSpagetka/display-vm-option-chart-fix
1647f12 
Fix Display Vm option, when clicking on chart
(cherry picked from commit 82b0eed)

https://bugzilla.redhat.com/show_bug.cgi?id=1530768
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@himdel himdel

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PanSpagetka @miq-bot @himdel @simaishi