Convert RBAC Features Tree to use TreeBuilder (2) #137

hayesr · 2017-01-12T06:39:26Z

This is an alternative to #56 that makes more typical use of x_get_*_kids methods.
~~Heads-up: this way is 7x slower~~ Performance isn't bad using feature cache in MiqProductFeature

Still to figure out:

Sync keys with what controller is expecting so tree can be saved
Find an uncached way to send the editable state to nodes from builder
Solve a check-state bubble up problem with Menu::Sections

Addresses:
https://bugzilla.redhat.com/show_bug.cgi?id=1348623
https://bugzilla.redhat.com/show_bug.cgi?id=1411831
https://www.pivotaltracker.com/story/show/129518309

Related:
ManageIQ/manageiq#13577
ManageIQ/manageiq#13592

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

/cc @zimdel @skateman

ghost · 2017-01-12T09:18:36Z

Summoning @ZitaNemeckova and @himdel

skateman

Looks great, just a few minor comments ...

skateman · 2017-01-12T10:46:39Z

app/presenters/tree_builder_ops_rbac_menu.rb

+  end
+
+  def x_get_tree_section_kids(parent, count_only = false)
+    kids = parent.items.select do |item|


Isn't possible to write this as a one-liner with reject?

Shorter, but one line is awkward with the if kind_of?(Menu::Item).

skateman · 2017-01-12T10:46:56Z

app/presenters/tree_builder_ops_rbac_menu.rb

+    count_only_or_objects(count_only, kids)
+  end
+
+  # FIXME: This is inefficient, but done to use MiqProductFeature objects rather than hashes


skateman · 2017-01-12T10:48:07Z

app/presenters/tree_node/hash.rb

@@ -10,7 +10,8 @@ class Hash < Node

    set_attribute(:hide_checkbox) { @object.key?(:hideCheckbox) && @object[:hideCheckbox] ? true : nil }

-    set_attribute(:selected) { @object.key?(:select) && @object[:select] ? true : nil }
+    # set_attribute(:selected) { @object.key?(:select) && @object[:select] ? true : nil }
+    set_attribute(:selected) { @object[:select] if @object.key?(:select) }


This definitely needs to be tested against other trees using hash nodes & checkboxes ...

skateman · 2017-01-12T10:48:33Z

app/presenters/tree_node/menu/item.rb

+module TreeNode
+  module Menu
+    class Item < Node
+      set_attribute(:key) { "#{@options[:node_id_prefix]}__#{@object.feature}" }


Any particular reason for the __ instead of _?

Legacy. I still need to look into the keys that the controller expects to make updating work.

skateman · 2017-01-12T10:49:31Z

app/presenters/tree_node/menu/item.rb

+    class Item < Node
+      set_attribute(:key) { "#{@options[:node_id_prefix]}__#{@object.feature}" }
+
+      set_attribute(:title) { _(details[:name]) }


I think it's way too dynamic for gettext - ping @mzazrivec

This is a carry over of how it currently works: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/ops_controller/rbac_tree.rb#L120

Yes, using _(details[:name]) here is correct.

skateman · 2017-01-12T10:52:24Z

app/presenters/tree_node/menu/item.rb

+        self_selected? || select_by_kids_selected
+      end
+
+      set_attribute(:no_click, true)


Not sure if this is necessary, see https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/presenters/tree_builder.rb#L124

With this setting, clicking on a node will expand that part of the tree, without it clicking will try and load a page (I guess based on the key). Is there another setting I'm supposed to use for that?

Set :onclick => nil in set_locals_for_render 😉

That seems to prevent it from trying to load a page, but it highlights the node instead of expanding it.

skateman · 2017-01-12T10:52:55Z

app/presenters/tree_node/menu/item.rb

+
+      set_attribute(:no_click, true)
+
+      set_attribute(:hide_checkbox, false)


Isn't this default? I mean if there are checkboxes, they're displayed if not said othetwise.

himdel · 2017-01-12T12:51:14Z

app/presenters/tree_node/menu/section.rb

+  module Menu
+    class Section < Node
+      set_attribute(:key) do
+        "#{@options[:node_id_prefix]}___tab_#{@object.id}"


If you can add tab as a prefix for this kind of object, then it'd be better to have that, use full_ids and not override the key in any of the nodes..

(that should give you xx-10r2_tab-net)

I changed to full ids, but forms from users, groups, and roles all seem to go through the same method. Probably should be fixed in a subsequent PR.

hayesr · 2017-01-13T05:01:05Z

app/controllers/ops_controller/ops_rbac.rb

+    # Make sure tree_state doesn't hold on to old data between requests
+    #
+    unless @sb.fetch_path('trees', 'features_tree').nil?
+      @sb['trees'].delete('features_tree')


@himdel @skateman I'm guessing this isn't normal, but otherwise too much state is carried over between show and edit and back.

@hayesr Maybe something like TreeState.new(sandbox).remove_tree(name) in the constructor?

(mostly because we have the same thing here https://github.com/ManageIQ/manageiq-ui-classic/pull/15/files#diff-d10de31acbab60b307f4c0ea86b158c0R22 )

himdel · 2017-01-19T20:29:43Z

app/presenters/tree_node/menu/item.rb

+
+      set_attribute(:tooltip) { _(details[:description]) || _(details[:name]) }
+
+      set_attribute(:image, "100/feature_node")


missing .png

himdel · 2017-01-19T20:30:01Z

app/presenters/tree_node/menu/section.rb

+
+      set_attribute(:title) { _(@object.name) }
+
+      set_attribute(:image) { "100/feature_node" }


missing .png

— but it works 🤷‍♂️

Fixed.

Hehe, yeah, that's the magic of rails :-) which usually only breaks in production.. By making the first access to such an icon take about 2 minutes on a new appliance, became its not in the index and rails tries really hard to search for it :-)

himdel · 2017-01-19T20:30:39Z

app/presenters/tree_node/node.rb

@@ -129,7 +129,7 @@ def to_h
        :checkable    => checkable ? nil : false,
      }

-      node[:image] = if !image
+      node[:image] = if !respond_to?(:image) || !image


no longer needed, Node has a default image implementation

This shouldn't be a change from my code. How'd this get here?

skateman · 2017-01-23T12:16:32Z

@hayesr could you please fix the red rubocop issues?

hayesr · 2017-04-21T05:36:17Z

Removed the child-counting methods from node builders and it seems to work well with post_check. This might finally be ready to go when green 🤞 @skateman @himdel @martinpovolny

skateman · 2017-04-21T06:24:05Z

@hayesr please rebase to fix bower issues in travis & there are still things that weren't fixed...

hayesr · 2017-04-21T14:39:30Z

Restart CI

hayesr · 2017-04-21T16:10:43Z

Bower hates me.

hayesr · 2017-04-21T17:09:15Z

Finally green! (Codeclimate doesn't like so many parameters in initialize, we should fix that later)

skateman · 2017-04-24T09:59:29Z

As I mentioned earlier there are still some unresolved issues:

a load_chidren? method that always returns true.
unnecessary attr_reader calls
a single-line single-used root_tooltip method

Please fix them, and we can merge this 👍

miq-bot · 2017-04-27T10:23:21Z

This pull request is not mergeable. Please rebase and repush.

kbrock · 2017-04-29T03:55:43Z

Codeclimate doesn't like so many parameters in initialize, we should fix that later

the operative word here is "later" aka someday/maybe/never

Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1348623 https://bugzilla.redhat.com/show_bug.cgi?id=1411831 https://www.pivotaltracker.com/story/show/129518309

miq-bot · 2017-05-01T23:48:55Z

Checked commits hayesr/manageiq-ui-classic@4f46dae~...a90e7a0 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
12 files checked, 2 offenses detected

app/presenters/menu/manager.rb

❗ - Line 9, Col 16 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

app/presenters/tree_builder_ops_rbac_features.rb

❗ - Line 35, Col 24 - Style/OptionalArguments - Optional arguments should appear at the end of the argument list.

hayesr · 2017-05-02T02:59:10Z

Ready for another look.

skateman

martinpovolny · 2017-05-02T11:36:11Z

🥇

hayesr force-pushed the convert_rbac_menu_tree_2 branch from 7856bfc to 2f476e2 Compare January 12, 2017 06:41

skateman reviewed Jan 12, 2017

View reviewed changes

himdel reviewed Jan 12, 2017

View reviewed changes

hayesr force-pushed the convert_rbac_menu_tree_2 branch 3 times, most recently from 2e993cf to 8419eb4 Compare January 13, 2017 04:52

hayesr commented Jan 13, 2017

View reviewed changes

hayesr changed the title ~~[WIP] Convert Menu Features Tree to use TreeBuilder (2)~~ [WIP] Convert RBAC Features Tree to use TreeBuilder (2) Jan 13, 2017

mzazrivec added the wip label Jan 13, 2017

hayesr force-pushed the convert_rbac_menu_tree_2 branch 3 times, most recently from 97c51bd to 87fbe37 Compare January 16, 2017 19:43

hayesr changed the title ~~[WIP] Convert RBAC Features Tree to use TreeBuilder (2)~~ Convert RBAC Features Tree to use TreeBuilder (2) Jan 16, 2017

hayesr closed this Jan 16, 2017

hayesr reopened this Jan 16, 2017

hayesr changed the title ~~Convert RBAC Features Tree to use TreeBuilder (2)~~ [WIP] Convert RBAC Features Tree to use TreeBuilder (2) Jan 18, 2017

hayesr force-pushed the convert_rbac_menu_tree_2 branch from 87fbe37 to c7ca1d5 Compare January 18, 2017 01:07

hayesr changed the title ~~[WIP] Convert RBAC Features Tree to use TreeBuilder (2)~~ Convert RBAC Features Tree to use TreeBuilder (2) Jan 18, 2017

hayesr closed this Jan 19, 2017

hayesr reopened this Jan 19, 2017

himdel reviewed Jan 19, 2017

View reviewed changes

This was referenced Jan 19, 2017

[EUWE] Fix RBAC Features Tree select behavior ManageIQ/manageiq#13577

Merged

[DARGA] Fix RBAC Features Tree select behavior ManageIQ/manageiq#13592

Merged

skateman approved these changes Jan 20, 2017

View reviewed changes

himdel removed the wip label Jan 20, 2017

mzazrivec assigned himdel Jan 23, 2017

hayesr closed this Apr 21, 2017

hayesr reopened this Apr 21, 2017

hayesr closed this Apr 21, 2017

hayesr reopened this Apr 21, 2017

hayesr closed this Apr 21, 2017

hayesr reopened this Apr 21, 2017

hayesr mentioned this pull request Apr 24, 2017

Not very well marked checkbox Tasks in the role EvmRole-administrator. ManageIQ/manageiq#14648

Closed

miq-bot added the unmergeable label Apr 27, 2017

hayesr added 2 commits May 1, 2017 16:06

Convert Menu Features Tree to use TreeBuilder

4f46dae

Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1348623 https://bugzilla.redhat.com/show_bug.cgi?id=1411831 https://www.pivotaltracker.com/story/show/129518309

Remove child-counting code from node builders

c495821

hayesr force-pushed the convert_rbac_menu_tree_2 branch from 6ad047c to c495821 Compare May 1, 2017 23:06

miq-bot removed the unmergeable label May 1, 2017

hayesr added 2 commits May 1, 2017 16:14

Bring view up-to-date with master

cdf889a

Address feedback

a90e7a0

skateman approved these changes May 2, 2017

View reviewed changes

martinpovolny merged commit a07cd16 into ManageIQ:master May 2, 2017

martinpovolny added this to the Sprint 60 Ending May 8, 2017 milestone May 2, 2017

hayesr mentioned this pull request May 2, 2017

Fix Features Tree for "everything under" features #1229

Merged

hayesr mentioned this pull request Jun 5, 2017

[FINE] Fix node selection in RBAC Tree #1301

Merged


		set_attribute(:no_click, true)

		set_attribute(:hide_checkbox, false)


		set_attribute(:tooltip) { _(details[:description]) \|\| _(details[:name]) }

		set_attribute(:image, "100/feature_node")


		set_attribute(:title) { _(@object.name) }

		set_attribute(:image) { "100/feature_node" }

Convert RBAC Features Tree to use TreeBuilder (2) #137

Convert RBAC Features Tree to use TreeBuilder (2) #137

Conversation

hayesr commented Jan 12, 2017 • edited Loading

ghost commented Jan 12, 2017

skateman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hayesr Jan 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel Jan 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman commented Jan 23, 2017

hayesr commented Apr 21, 2017

skateman commented Apr 21, 2017 • edited Loading

hayesr commented Apr 21, 2017

hayesr commented Apr 21, 2017

hayesr commented Apr 21, 2017

skateman commented Apr 24, 2017

miq-bot commented Apr 27, 2017

kbrock commented Apr 29, 2017

miq-bot commented May 1, 2017

hayesr commented May 2, 2017

skateman left a comment

Choose a reason for hiding this comment

martinpovolny commented May 2, 2017

hayesr commented Jan 12, 2017 •

edited

Loading

himdel Jan 12, 2017 •

edited

Loading

hayesr Jan 13, 2017 •

edited

Loading

himdel Jan 19, 2017 •

edited

Loading

skateman commented Apr 21, 2017 •

edited

Loading