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 Policy breadcrumbs when coming from GTL page #5814

Merged
merged 4 commits into from
Jul 26, 2019

Conversation

ZitaNemeckova
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova commented Jul 16, 2019

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

How to reproduce:
Go Compute -> Infrastructure -> VMs -> check two or more VMs -> Policy -> Edit Tags/Policy Simulation/Manage Policies -> see breadcrumbs

Before:
Screenshot 2019-07-16 at 13 09 08

After:
Screenshot 2019-07-16 at 13 02 31

There's no change when doing the same from a VM Summary page.

Following guides for breadcrumbs Tagging/Ownership/etc.

@miq-bot add_label bug, hammer/no

@rvsia please have a look, thanks :)

@rvsia
Copy link
Contributor

rvsia commented Jul 16, 2019

@ZitaNemeckova

I think you can solve this issue by checking if variable in the special_breadcrumb method has length == 1, if bigger then return nil. This check was there.. however it has gone missing probably during some refactoring... 😰

@ZitaNemeckova
Copy link
Contributor Author

@rvsia well, there still would be a VM name in case you check just one VM in GTL page. I think we don't want a VM name when coming from GTL page.

@rvsia
Copy link
Contributor

rvsia commented Jul 16, 2019

@ZitaNemeckova When I designed the breadcrumbs I was following this rule: show the Item's breadcrumb on tag page (etc.) if there is only one item which is being tagged. https://github.com/ManageIQ/manageiq-ui-classic/wiki/Breadcrumbs

@@ -74,7 +74,7 @@ def special_page_breadcrumb(variable)
elsif floating_ip_address?(variable.first)
title = variable.first[:address]
else
title = variable.first[:name]
title = variable.first[:name] if variable.count == 1 # show breadcrumb only for one item
Copy link
Contributor

Choose a reason for hiding this comment

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

The condition can be moved to line 70:

return unless variable && variable.count == 1 or
return if !variable || variable.count != 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Chose the second option to get rid of unless 😃

@ZitaNemeckova ZitaNemeckova force-pushed the fix_policy_bc branch 2 times, most recently from 54257fe to a61be78 Compare July 16, 2019 14:30
Copy link
Contributor

@rvsia rvsia left a comment

Choose a reason for hiding this comment

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

Great! Thanks! 🏆

@miq-bot
Copy link
Member

miq-bot commented Jul 16, 2019

Checked commits ZitaNemeckova/manageiq-ui-classic@bd815c1~...28b67ec with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@martinpovolny martinpovolny self-assigned this Jul 26, 2019
@martinpovolny martinpovolny added this to the Sprint 117 Ending Aug 5, 2019 milestone Jul 26, 2019
@martinpovolny martinpovolny merged commit 5e2db32 into ManageIQ:master Jul 26, 2019
simaishi pushed a commit that referenced this pull request Jul 26, 2019
Fix Policy breadcrumbs when coming from GTL page

(cherry picked from commit 5e2db32)

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

Ivanchuk backport details:

$ git log -1
commit 83a5289945bc0a51b34c0b6fdb21dbf278c9d4b0
Author: Martin Povolny <[email protected]>
Date:   Fri Jul 26 09:34:38 2019 +0200

    Merge pull request #5814 from ZitaNemeckova/fix_policy_bc
    
    Fix Policy breadcrumbs when coming from GTL page
    
    (cherry picked from commit 5e2db323ebed5eb888ec947cc4f6f3ff41458f9c)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1720714

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.

5 participants