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

Deduplicate tree building. #2059

Merged
merged 12 commits into from
Oct 10, 2017
Merged
2 changes: 1 addition & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2262,7 +2262,7 @@ def controller_for_common_methods
end

def reload_trees_by_presenter(presenter, trees)
trees.each_pair do |_, tree|
trees.each do |tree|
next unless tree.present?
presenter.reload_tree(tree.name, tree.locals_for_render[:bs_tree])
end
Expand Down
16 changes: 16 additions & 0 deletions app/controllers/application_controller/explorer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,22 @@
module ApplicationController::Explorer
extend ActiveSupport::Concern

def build_replaced_trees(replace_trees, valid_values)
return [] unless replace_trees
valid_values.find_all { |tree| replace_trees.include?(tree) }
.collect { |tree| try_build_tree(tree) }
.compact
end

def try_build_tree(tree_symbol)
method_name = "build_#{tree_symbol}_tree"
return unless respond_to?(method_name, true)
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little bit afraid that someone might try to remove the build_*_tree methods as they might seem unused after a grepping. Maybe it's over-engineering, but I'd rather keep a list of these methods here and check against. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well I prefer not. I'd like to unify it a bit further and then remove the build_*tree methods calling directly the tree builder.

There's no value and little information in the build_*tree methods

Copy link
Member

Choose a reason for hiding this comment

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

Good enough for me 👍

build_method = method(method_name)
# FIXME: This is temporary, we actually need to remove all the build_*_tree methods and
# use the Feature::build_tree instead.
build_method.arity == 1 ? build_method.call(tree_symbol) : build_method.call
end

# Historical tree item selected
def x_history
@hist = x_tree_history[params[:item].to_i] # Set instance var so we know hist button was pressed
Expand Down
17 changes: 1 addition & 16 deletions app/controllers/automation_manager_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -343,22 +343,7 @@ def default_node
end

def rebuild_trees(replace_trees)
trees = {}
if replace_trees
if replace_trees.include?(:automation_manager_providers)
trees[:automation_manager_providers] = build_automation_manager_tree(:automation_manager_providers,
:automation_manager_providers_tree)
end
if replace_trees.include?(:automation_manager_cs_filter)
trees[:automation_manager_cs_filter] = build_automation_manager_tree(:automation_manager_cs_filter,
:automation_manager_cs_filter_tree)
end
if replace_trees.include?(:configuration_scripts)
trees[:configuration_scripts] = build_automation_manager_tree(:configuration_scripts,
:configuration_scripts_tree)
end
end
trees
build_replaced_trees(replace_trees, %i(automation_manager_providers automation_manager_cs_filter configuration_scripts))
end

def leaf_record
Expand Down
14 changes: 5 additions & 9 deletions app/controllers/catalog_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1914,14 +1914,10 @@ def replace_right_cell(options = {})
replace_trees = @replace_trees if @replace_trees # get_node_info might set this
right_cell_text = @right_cell_text if @right_cell_text # get_node_info might set this too

trees = build_replaced_trees(replace_trees, %i(sandt svccat stcat ot))

type, _id = parse_nodetype_and_id(x_node)
trees = {}
if replace_trees
trees[:sandt] = build_st_tree if replace_trees.include?(:sandt)
trees[:svccat] = build_svccat_tree if replace_trees.include?(:svccat)
trees[:stcat] = build_stcat_tree if replace_trees.include?(:stcat)
trees[:ot] = build_orch_tmpl_tree if replace_trees.include?(:ot)
end

allowed_records = %w(MiqTemplate OrchestrationTemplate Service ServiceTemplate ServiceTemplateCatalog)
record_showing = (type && allowed_records.include?(TreeBuilder.get_model_for_prefix(type)) && !@view) ||
params[:action] == "x_show"
Expand Down Expand Up @@ -2092,7 +2088,7 @@ def need_container_template_locals?
end

# Build a Catalog Items explorer tree
def build_st_tree
def build_sandt_tree
TreeBuilderCatalogItems.new('sandt_tree', 'sandt', @sb)
end

Expand All @@ -2107,7 +2103,7 @@ def build_stcat_tree
end

# Build a Orchestration Templates explorer tree
def build_orch_tmpl_tree
def build_ot_tree
TreeBuilderOrchestrationTemplates.new('ot_tree', 'ot', @sb)
end

Expand Down
6 changes: 2 additions & 4 deletions app/controllers/chargeback_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -834,10 +834,8 @@ def replace_right_cell(options = {})
c_tb = build_toolbar(center_toolbar_filename)

# Build a presenter to render the JS
presenter = ExplorerPresenter.new(
:active_tree => x_active_tree,
)
reload_trees_by_presenter(presenter, :cb_rates => cb_rates_build_tree) if replace_trees.include?(:cb_rates)
presenter = ExplorerPresenter.new(:active_tree => x_active_tree)
reload_trees_by_presenter(presenter, [cb_rates_build_tree]) if replace_trees.include?(:cb_rates)

# FIXME
# if params[:action].ends_with?("_delete")
Expand Down
14 changes: 4 additions & 10 deletions app/controllers/miq_ae_customization_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,13 +237,7 @@ def replace_right_cell(options = {})
nodetype, replace_trees = options.values_at(:nodetype, :replace_trees)
# fixme, don't call all the time
build_ae_tree(:automate, :automate_tree) # Build Catalog Items tree
trees = {}
if replace_trees
trees[:ab] = ab_build_tree if replace_trees.include?(:ab)
trees[:old_dialogs] = old_dialogs_build_tree if replace_trees.include?(:old_dialogs)
trees[:dialogs] = dialog_build_tree if replace_trees.include?(:dialogs)
trees[:dialog_edit] = dialog_edit_build_tree if replace_trees.include?(:dialog_edit)
end
trees = build_replaced_trees(replace_trees, %i(ab old_dialogs dialogs dialog_edit))

@explorer = true
presenter = ExplorerPresenter.new(:active_tree => x_active_tree)
Expand Down Expand Up @@ -495,15 +489,15 @@ def setup_presenter_for_old_dialogs_tree(nodetype, presenter)
end
end

def old_dialogs_build_tree
def build_old_dialogs_tree
TreeBuilderProvisioningDialogs.new("old_dialogs_tree", "old_dialogs", @sb)
end

def dialog_build_tree
def build_dialogs_tree
TreeBuilderServiceDialogs.new("dialogs_tree", "dialogs", @sb)
end

def ab_build_tree
def build_ab_tree
TreeBuilderButtons.new("ab_tree", "ab", @sb)
end

Expand Down
12 changes: 6 additions & 6 deletions app/controllers/miq_ae_customization_controller/dialogs.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ def dialog_form_field_changed
dialog_get_form_vars

# dialog_edit_set_form_vars
dialog_edit_build_tree
build_dialog_edit_tree
dialog_sort_values if (params[:field_data_typ] || params[:field_sort_by] ||
params[:field_sort_order]) && @edit[:field_sort_by].to_s != "none"
@_params[:typ] = ""
Expand Down Expand Up @@ -296,7 +296,7 @@ def dialog_res_discard
self.x_node = "#{nodes[0]}_#{nodes[1]}_#{nodes[2]}"
end
end
dialog_edit_build_tree
build_dialog_edit_tree
@sb[:edit_typ] = nil
replace_right_cell(:nodetype => x_node, :replace_trees => [:dialog_edit])
end
Expand Down Expand Up @@ -418,7 +418,7 @@ def dialog_res_reorder

parent[name] = temp

dialog_edit_build_tree
build_dialog_edit_tree
render :update do |page|
page << javascript_prologue
session[:changed] = changed = (@edit[:new] != @edit[:current])
Expand Down Expand Up @@ -750,7 +750,7 @@ def generic_tree_node(key, text, image, tip = nil, options = {})
end

# Build a Dialogs tree
def dialog_edit_build_tree
def build_dialog_edit_tree
x_tree_init(:dialog_edit_tree, :dialog_edit, nil)
# building tab nodes under a dialog
tab_nodes = []
Expand Down Expand Up @@ -1135,7 +1135,7 @@ def dialog_edit_set_form_vars

@sb[:node_typ] = "element" if params[:action] != "dialog_form_field_changed"
end
dialog_edit_build_tree
build_dialog_edit_tree
session[:changed] = (@edit[:new] != @edit[:current])
session[:edit] = @edit
end
Expand Down Expand Up @@ -1282,7 +1282,7 @@ def dialog_set_form_vars
end

@edit[:current] = copy_hash(@edit[:new])
dialog_edit_build_tree
build_dialog_edit_tree
x_node_set("root", :dialog_edit_tree) # always set it to root for edit tree

session[:edit] = @edit
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/miq_policy_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ def replace_right_cell(options = {})
raise _("unknown tree in replace_trees: %{name}") % {name => name}
end
end
reload_trees_by_presenter(presenter, trees)
reload_trees_by_presenter(presenter, trees.values)

if params[:action].ends_with?('_delete') &&
!x_node.starts_with?('p') &&
Expand Down
8 changes: 1 addition & 7 deletions app/controllers/ops_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -767,13 +767,7 @@ def handle_bottom_cell(nodetype, presenter, locals)

def replace_explorer_trees(replace_trees, presenter)
# Build hash of trees to replace and optional new node to be selected
trees = {}
if replace_trees
trees[:settings] = settings_build_tree if replace_trees.include?(:settings)
trees[:rbac] = rbac_build_tree if replace_trees.include?(:rbac)
trees[:diagnostics] = diagnostics_build_tree if replace_trees.include?(:diagnostics)
trees[:vmdb] = db_build_tree if replace_trees.include?(:vmdb)
end
trees = build_replaced_trees(replace_trees, %i(settings rbac diagnostics vmdb))
reload_trees_by_presenter(presenter, trees)
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/ops_controller/db.rb
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def x_show
private #######################

# Build a VMDB tree for Database accordion
def db_build_tree
def build_vmdb_tree
TreeBuilderOpsVmdb.new("vmdb_tree", "vmdb", @sb)
end

Expand Down
6 changes: 3 additions & 3 deletions app/controllers/ops_controller/diagnostics.rb
Original file line number Diff line number Diff line change
Expand Up @@ -666,8 +666,8 @@ def refresh_screen
@explorer = true
if params[:pressed] == "delete_server" || params[:pressed] == "zone_delete_server"
@sb[:diag_selected_id] = nil
settings_build_tree
diagnostics_build_tree
build_settings_tree
build_diagnostics_tree
end
if x_node == "root"
parent = MiqRegion.my_region
Expand Down Expand Up @@ -893,7 +893,7 @@ def diagnostics_get_info
end
end

def diagnostics_build_tree
def build_diagnostics_tree
TreeBuilderOpsDiagnostics.new("diagnostics_tree", "diagnostics", @sb)
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/ops_controller/ops_rbac.rb
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ def process_tenants(tenants, task)
end

# Build the main Access Control tree
def rbac_build_tree
def build_rbac_tree
TreeBuilderOpsRbac.new("rbac_tree", "rbac", @sb)
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/ops_controller/settings/common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1192,7 +1192,7 @@ def settings_get_info(nodetype = x_node)
end

# Build the main Settings tree
def settings_build_tree
def build_settings_tree
TreeBuilderOpsSettings.new("settings_tree", "settings", @sb)
end

Expand Down
23 changes: 7 additions & 16 deletions app/controllers/provider_foreman_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,15 +217,12 @@ def features
end
end

def build_configuration_manager_tree(type, name)
tree = case name
when :configuration_manager_providers_tree
TreeBuilderConfigurationManager.new(name, type, @sb)
when :configuration_manager_cs_filter_tree
TreeBuilderConfigurationManagerConfiguredSystems.new(name, type, @sb)
end
instance_variable_set :"@#{name}", tree.tree_nodes
tree
def build_configuration_manager_providers_tree(_type)
TreeBuilderConfigurationManager.new(:configuration_manager_providers, :configuration_manager_providers_tree, @sb)
end

def build_configuration_manager_cs_filter_tree(_type)
TreeBuilderConfigurationManagerConfiguredSystems.new(:configuration_manager_cs_filter, :configuration_manager_cs_filter_tree, @sb)
end

def get_node_info(treenodeid, show_list = true)
Expand Down Expand Up @@ -334,13 +331,7 @@ def default_node
end

def rebuild_trees(replace_trees)
trees = {}
if replace_trees
trees[:configuration_manager_providers] = build_configuration_manager_tree(:configuration_manager_providers,
:configuration_manager_providers_tree) if replace_trees.include?(:configuration_manager_providers)
trees[:configuration_manager_cs_filter] = build_configuration_manager_tree(:configuration_manager_cs_filter, :configuration_manager_cs_filter_tree) if replace_trees.include?(:configuration_manager_cs_filter)
end
trees
build_replaced_trees(replace_trees, %i(configuration_manager_providers configuration_manager_cs_filter))
end

def leaf_record
Expand Down
12 changes: 2 additions & 10 deletions app/controllers/pxe_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,17 +102,9 @@ def replace_right_cell(options = {})

@explorer = true

trees = {}
if replace_trees
trees[:pxe_servers] = pxe_server_build_tree if replace_trees.include?(:pxe_servers)
trees[:pxe_image_types] = pxe_image_type_build_tree if replace_trees.include?(:pxe_image_types)
trees[:customization_templates] = customization_template_build_tree if replace_trees.include?(:customization_templates)
trees[:iso_datastores] = iso_datastore_build_tree if replace_trees.include?(:iso_datastores)
end
trees = build_replaced_trees(replace_trees, %i(pxe_servers pxe_image_types customization_templates iso_datastores))

presenter = ExplorerPresenter.new(
:active_tree => x_active_tree,
)
presenter = ExplorerPresenter.new(:active_tree => x_active_tree)

c_tb = build_toolbar(center_toolbar_filename) unless @in_a_form
h_tb = build_toolbar('x_history_tb')
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/pxe_controller/iso_datastores.rb
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ def process_iso_datastores(elements, task, display_name)
end

# Get information for an event
def iso_datastore_build_tree
def build_iso_datastores_tree
TreeBuilderIsoDatastores.new("iso_datastores_tree", "iso_datastores", @sb)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ def template_get_node_info(treenodeid)
end

# Get information for an event
def customization_template_build_tree
def build_customization_templates_tree
TreeBuilderPxeCustomizationTemplates.new("customization_templates_tree", "customization_templates", @sb)
end
end
2 changes: 1 addition & 1 deletion app/controllers/pxe_controller/pxe_image_types.rb
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ def process_pxe_image_type(pxes, task)
end

# Get information for an event
def pxe_image_type_build_tree
def build_pxe_image_types_tree
TreeBuilderPxeImageTypes.new("pxe_image_types_tree", "pxe_image_types", @sb)
end

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/pxe_controller/pxe_servers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ def process_pxes(pxes, task, display_name)
end

# Get information for an event
def pxe_server_build_tree
def build_pxe_servers_tree
TreeBuilderPxeServers.new("pxe_servers_tree", "pxe_servers", @sb)
end

Expand Down
24 changes: 6 additions & 18 deletions app/controllers/report_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -653,26 +653,14 @@ def replace_right_cell(options = {}) # :replace_trees key can be an array of tr
@sb[:active_tab] = params[:tab_id] ? params[:tab_id] : "report_info" if x_active_tree == :reports_tree &&
params[:action] != "reload" && !["miq_report_run", "saved_report_delete"].include?(params[:pressed]) # do not reset if reload saved reports buttons is pressed

trees = {}
rebuild = @in_a_form ? false : rebuild_trees

{
:reports => :build_reports_tree,
:schedules => :build_schedules_tree,
:savedreports => :build_savedreports_tree,
:db => :build_db_tree,
:widgets => :build_widgets_tree,
}.each do |tree, method|
next unless tree_exists?(tree.to_s + "_tree")

if replace_trees.include?(tree) || rebuild
trees[tree] = send(method)
end
rebuild = @in_a_form ? false : rebuild_trees
valid_trees = %i(reports schedules savedreports db widgets).find_all do |tree|
tree_exists?(tree.to_s + "_tree")
end
trees = build_replaced_trees(rebuild ? valid_trees : replace_trees, valid_trees)

presenter = ExplorerPresenter.new(:active_tree => x_active_tree)

presenter = ExplorerPresenter.new(
:active_tree => x_active_tree,
)
# Clicked on right cell record, open the tree enough to show the node, if not already showing
# Open the parent nodes of selected record, if not open
# Showing a report
Expand Down