Fix Features Tree for "everything under" features

[hayesr]

Go to Configuration > Access Control > Roles, pick a role - the tree is on the right, in the role detail.

In #137 we set the select state of a node based on whether the role has the feature identifier for the corresponding part of the tree. Then we depend on the client side to check parents and children of that node. However, some features have no equivalent in the menu, so when the role has a feature like "Everything under Ansible" nothing would be selected.

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

This should not be backported.

@dclarizio @skateman @martinpovolny @himdel
Checking nodes with only Javascript was too good to be true. We have to do a little work on the server to cover cases like this.

@miq-bot add_label bug, fine/no, euwe/no

[himdel]

@hayesr what's the structure of the items in the case where the JS thing doesn't work? Trying to understand why that happens..

Sounds like "features not available in the menu" is something we need to show in the tree too..

[hayesr]

@himdel This tree is really a merger of MiqProductFeatures hierarchy and the main menu hierarchy. At the higher levels of the tree, we show Menu::Sections and Menu::Items instead of their corresponding features. In the features, you'll have something like this which is a shortcut (for storage in the Role) for every Ansible feature. That everything-feature corresponds to a Menu::Section which does not have a feature attribute to set server-side. In this PR I set the select state of the Menu::Item nodes and lean on the JS to select the Section node above them.

I suppose we could show an extra node under that, but it would look weird. Alternatively we could add a feature attribute to Menu::Section and litter default_menu.rb with a bunch more magic configuration strings. Or somehow send a separate "data" representation of this tree based on features only, then render it client-side to look like the menu+features version. This seems like a simpler solution.

[hayesr]

Original description and commit had the wrong BZ reference. Correct one is https://bugzilla.redhat.com/show_bug.cgi?id=1442796

[himdel]

Well, it still baffles me that we can hide features from a tree that's supposed to let you assign those features to users roles.. If I understand correctly, your solution would not allow anybody to add the "Everything under Ansible" privilege, because it does not appear in the tree, am I right?

[hayesr]

Not as far as I can tell. The edit-mode-tree sends all the features then they are compacted.

[hayesr]

Sometimes there is something like "All Accordions Under Instances".

But Menu::Sections seem to be special cases. Pretty sure the goal is to match the menu structure as closely as possible.

[hayesr]

Can we merge this @himdel @h-kataria ?

[himdel]

Well, the problem with not being able to assign certain features looks real, but could probably be addressed in a separate PR, this just fixes a bug, so I'd say yes...

.. except it doesn't work :). Create a new role, check just Automation > Ansible and Automation > Ansible Tower, Save.

I'd expect those 2 things to be checked, and the rest not. What happens is that the user has Everything.

Can you please fix that? :)

[himdel]

( This does however fix the situation where you pick an existing role which does not have Everything, and has nothing under Automation, check Automation > Ansible, save... In master, Ansible is not checked, with this PR, it is. 👍 )

[himdel]

So.. will you be addressing the new role problem in a separate PR?

Since I can verify that this fixes at least this one bug, I'm inclined to merge, unless both make sense here?

[hayesr]

@himdel I'm finally getting back to this, I do plan on fixing the problem you found.

[hayesr]

@himdel The problem seems to be with the form and only in the process of creating a new role. If you toggle Everything (top of the tree) on and off, then select items as you describe it works.

[miq-bot]

Checked commits hayesr/manageiq-ui-classic@67980fc~...b8c1008 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

[hayesr]

It seems from the controller code that the intended behavior for the role form is that everything is checked initially. In the tree builder the features ivar would end up empty for new records. This should be fixed now.

Ready for another look @himdel @h-kataria

[himdel]

Perfect, thanks @hayesr!

(The travis failure is the same as on master, will re-kick travis when it is fixed.)