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

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

Merged
merged 4 commits into from
May 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions app/controllers/ops_controller/ops_rbac.rb
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def rbac_role_edit
case params[:button]
when 'cancel' then rbac_edit_cancel('role')
when 'save', 'add' then rbac_edit_save_or_add('role', 'miq_user_role')
when 'reset', nil then rbac_edit_reset(params[:typ], 'role', MiqUserRole) # Reset or first time in
when 'reset', nil then rbac_edit_reset(params[:typ], 'role', MiqUserRole) # Reset or form load
end
end

Expand Down Expand Up @@ -962,13 +962,12 @@ def rbac_group_right_tree(selected)
def rbac_role_get_details(id)
@edit = nil
@record = @role = MiqUserRole.find_by_id(from_cid(id))
@role_features = @role.feature_identifiers.sort
@features_tree = rbac_build_features_tree
@rbac_menu_tree = build_rbac_feature_tree
end

def rbac_build_features_tree
def build_rbac_feature_tree
@role = @sb[:typ] == "copy" ? @record.dup : @record if @role.nil? # if on edit screen use @record
TreeBuilder.convert_bs_tree(OpsController::RbacTree.build(@role, @role_features, !@edit.nil?)).to_json
TreeBuilderOpsRbacFeatures.new("features_tree", "features", @sb, true, :role => @role, :editable => @edit.present?)
end

# Set form variables for role edit
Expand Down Expand Up @@ -1198,7 +1197,7 @@ def rbac_role_set_form_vars
@edit[:current] = copy_hash(@edit[:new])

@role_features = @record.feature_identifiers.sort
@features_tree = rbac_build_features_tree
@rbac_menu_tree = build_rbac_feature_tree
end

# Get array of total set of features from the children of selected features
Expand Down
146 changes: 0 additions & 146 deletions app/controllers/ops_controller/rbac_tree.rb

This file was deleted.

10 changes: 10 additions & 0 deletions app/presenters/menu/item.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
module Menu
Item = Struct.new(:id, :name, :feature, :rbac_feature, :href, :type) do
extend ActiveModel::Naming

def self.base_class
Menu::Item
end

def self.base_model
model_name
end

Copy link
Member

Choose a reason for hiding this comment

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

I see that we have this piece of code multiple times, can you extract it to a mixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but maybe in a different PR? Changing these menu classes much more seems out of scope.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that creating a mixin with 6 lines is out of scope here, especially when you're introducing something new...

def initialize(an_id, a_name, features, rbac_feature, href, type = :default)
super
@parent = nil
Expand Down
12 changes: 7 additions & 5 deletions app/presenters/menu/manager.rb
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
module Menu
class Manager
include Enumerable
include Singleton

class << self
extend Forwardable

delegate %i(menu item_in_section? item section section_id_string_to_symbol each section_for_item_id) => :instance
delegate %i(menu item_in_section? item section section_id_string_to_symbol
section_for_item_id each map detect select) => :instance
end

def each
@menu.each { |section| yield section }
end

private
Expand Down Expand Up @@ -50,10 +56,6 @@ def item_in_section?(item_id, section_id)
@id_to_section[section_id].contains_item_id?(item_id)
end

def each
@menu.each { |section| yield section }
end

def initialize
load_default_items
load_custom_items
Expand Down
12 changes: 11 additions & 1 deletion app/presenters/menu/section.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
module Menu
Section = Struct.new(:id, :name, :icon, :items, :placement, :before, :type, :href) do
extend ActiveModel::Naming

def self.base_class
Menu::Section
end

def self.base_model
model_name
end
Copy link
Member

Choose a reason for hiding this comment

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

To the mixin 😉


def initialize(an_id, name, icon, *args)
super
self.items ||= []
Expand All @@ -17,7 +27,7 @@ def features
end

def features_recursive
Array(items).collect { |el| el.try(:feature) || el.try(:features) }.flatten.compact
Array(items).flat_map { |el| el.try(:feature) || el.try(:features) }.compact
Copy link
Member

Choose a reason for hiding this comment

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

👍 flat_map ❤️ a.k.a I learn something new every day 👍

Copy link
Member

@kbrock kbrock Apr 3, 2017

Choose a reason for hiding this comment

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

AND flat_map is a little quicker too.

ooh. This often gets rid of the compact (assuming features does not return [nil]):

Array(items).flat_map { |el| el.try(:feature) || el.try(:features) || [] }

end

def visible?
Expand Down
9 changes: 8 additions & 1 deletion app/presenters/tree_builder.rb
Original file line number Diff line number Diff line change
Expand Up @@ -261,9 +261,16 @@ def x_build_node(object, pid, options)
node = x_build_single_node(object, pid, options)

# Process the node's children
load_children = if object.kind_of?(Struct)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just use hash here, like every other tree?

We are in the process of getting rid of hash nodes, but we should be doing that systematically for all the trees... This looks too much like an exception for a signle tree...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So now you want me to go back to hashes for Menu::Item, Menu::Section, etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, that was definitely not my intention :)

However, all the trees dealing with Vm, etc. don't expect a hash here either, which leads me to think your code should not go through here. Any chance there's something else broken?

Copy link
Contributor

Choose a reason for hiding this comment

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

Withdrawing my objection here, I looked into it, it seems like that object[:load_children] that is already there should have had a object === Hash in front of it, since object can be any kind of AREL model.

So adding another branch for structs ..sounds about right :).

# Load children for Sections, don't for other Menu Structs.
object.kind_of?(Menu::Section)
else
object[:load_children]
end

node[:expand] = Array(@tree_state.x_tree(@name)[:open_nodes]).include?(node[:key]) || !!options[:open_all] || node[:expand]
if ancestry_kids ||
object[:load_children] ||
load_children ||
node[:expand] ||
@options[:lazy] == false

Expand Down
Loading