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

Use timeline listnav only for dashboard controller #5218

Merged
merged 2 commits into from
Feb 2, 2019

Conversation

ZitaNemeckova
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova commented Feb 1, 2019

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

Introduced by #3827 so this PR can not reintroduce toolbar with Configuration,...

Steps to test:

  • make sure Cloud Intel -> Timeline still has a timeline tree in listnav
  • for Availability Zones, Containers, Container Groups, Container Nodes, Container Pods, Container Projects, Clusters/Deployment Roles, Hosts/Nodes, Cloud Providers, Infra Providers, Container Providers select one of items and go to its summary page and Monitoring -> Timelines -> see that listnav is the same as in summary page and toolbar has only Back button
  • all things in listnav redirects correctly after clicking

Before:
image

After:
image

@miq-bot add_label bug, hammer/yes

Having this issue resolved would save me tons of time.

@miq-bot
Copy link
Member

miq-bot commented Feb 1, 2019

Checked commits ZitaNemeckova/manageiq-ui-classic@990950f~...d7af2f9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
12 files checked, 0 offenses detected
Everything looks fine. 👍

@martinpovolny
Copy link
Member

martinpovolny commented Feb 2, 2019

This is an ugly place. We moved this pasta to a separate file and limited the branching a bit in the past. But what is really needed to limit regressions here in the future is to have the listnav logic split based on the controller. But for that we need a good understanding of what has to be rendered where (which is hard to get from the pasta) and far more tests... ...or we create even more regressions :-(

👍 for the new tests that your wrote 💯

It's funny that a miniature test coverage drop is reported by the CC here. I suspect that the coverage measurement has limited value.

@martinpovolny martinpovolny self-assigned this Feb 2, 2019
@martinpovolny martinpovolny merged commit bc27d17 into ManageIQ:master Feb 2, 2019
@martinpovolny martinpovolny added this to the Sprint 104 Ending Feb 04, 2019 milestone Feb 2, 2019
@ZitaNemeckova ZitaNemeckova deleted the add_timeline_listnav branch February 2, 2019 08:59
simaishi pushed a commit that referenced this pull request Mar 6, 2019
Use timeline listnav only for dashboard controller

(cherry picked from commit bc27d17)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686016
@simaishi
Copy link
Contributor

simaishi commented Mar 6, 2019

Hammer backport details:

$ git log -1
commit 91755d7d6f4feaf53e27d087901b17fb15338c02
Author: Martin Povolny <[email protected]>
Date:   Sat Feb 2 09:20:13 2019 +0100

    Merge pull request #5218 from ZitaNemeckova/add_timeline_listnav
    
    Use timeline listnav only for dashboard controller
    
    (cherry picked from commit bc27d17e8a464bad85815b0551b3f250464e7753)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686016

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.

4 participants