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.

Sign up for GitHub

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

Chargeback Rates non explorer #7039

Merged
merged 18 commits into from
May 19, 2020
Merged

Chargeback Rates non explorer #7039

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
c083fc0
Converted chargeback rates explorer into non-explorer
h-kataria May 9, 2020
0fbdb08
Fixed an issue to show flash message when trying to edit default rate
h-kataria May 13, 2020
67c3abf
changed to render tree directly into show_list partial
h-kataria May 15, 2020
c59b978
set chargeback rates specific items in it's own tree
h-kataria May 15, 2020
c21e705
Addressed buttons related PR feedback
h-kataria May 15, 2020
23884bf
Simplified javascript_redirect calls
h-kataria May 15, 2020
67f8ef2
use `get_record_display_name` to get name/desciption of a record
h-kataria May 15, 2020
b85a562
Removed accidentally committed debugging statement
h-kataria May 15, 2020
ae4dbbf
Made tree building code generic
h-kataria May 16, 2020
1f3cfe3
An attempt to simplify setup and usage of `@gtl_buttons`
h-kataria May 16, 2020
0a4ced8
Moved code to determine toolbar file name to use `GenericToolbarMixin`
h-kataria May 18, 2020
750b140
Removed chargeback rates tree view as per GH discussion/comment
h-kataria May 18, 2020
1e81bda
Deleted Chargeback Rates tree builder files
h-kataria May 18, 2020
b98126b
Fixed spec tests
h-kataria May 18, 2020
7922c22
Deleted leftover tree view code
h-kataria May 19, 2020
f78f984
Removed redundant routes
h-kataria May 19, 2020
bf3fb4d
simplified routes and adjusted method names accordingly
h-kataria May 19, 2020
5e9422a
changed textfield to use `data-miq_focus` instead
h-kataria May 19, 2020
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
2 changes: 1 addition & 1 deletion app/controllers/application_controller/report_downloads.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ def set_summary_pdf_data
:page_layout => "portrait",
:page_size => "us-letter",
:run_date => run_time.strftime("%m/%d/%y %l:%m %p %z"),
:title => "#{klass} \"#{@record.name}\"".html_safe,
:title => "#{klass} \"#{@record.respond_to?(:name) ? @record.name : @record.description}\"".html_safe,
Copy link
Member

Choose a reason for hiding this comment

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

Can you please explain why is this extra complexity necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skateman because ChargebackRate record does not have name field. Previously we did not have download buttons on the Chargeback Rates list view because we did not render gtl view toolbar on the screen.

:quadicon => true
}

Expand Down
2 changes: 1 addition & 1 deletion app/controllers/chargeback_assignment_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ def replace_right_cell(options = {})
presenter.show(:paging_div, :form_buttons_div).remove_paging
locals = {:record_id => @edit[:rec_id]}
if x_active_tree == :cb_rates_tree
locals[:action_url] = 'cb_rate_edit'
locals[:action_url] = 'edit'
else
locals.update(
:action_url => 'cb_assign_update',
Expand Down
246 changes: 68 additions & 178 deletions app/controllers/chargeback_rate_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ class ChargebackRateController < ApplicationController
after_action :cleanup_action
after_action :set_session_data

include Mixins::GenericListMixin
include Mixins::GenericSessionMixin
include Mixins::GenericFormMixin
include Mixins::GenericShowMixin
include Mixins::BreadcrumbsMixin

CB_X_BUTTON_ALLOWED_ACTIONS = {
Expand All @@ -14,64 +17,66 @@ class ChargebackRateController < ApplicationController
'chargeback_rates_new' => :cb_rate_edit
}.freeze

def x_button
generic_x_button(CB_X_BUTTON_ALLOWED_ACTIONS)
end

def x_show
@explorer = true
@record = identify_record(params[:id], ChargebackRate)
nodeid = x_build_node_id(@record)
params[:id] = "xx-#{@record.rate_type}_#{nodeid}"
params[:tree] = x_active_tree.to_s
tree_select
redirect_to :action => 'explorer' unless request.xml_http_request? # Ajax request means in explorer
end
def button
@edit = session[:edit] # Restore @edit for adv search box
@refresh_div = "main_div" # Default div for button.rjs to refresh
action = params[:pressed]

def tree_select
self.x_active_tree = :cb_rates_tree
self.x_node = params[:id]
get_node_info(x_node)
replace_right_cell if request.xml_http_request? # Ajax request means in explorer
end
unless CB_X_BUTTON_ALLOWED_ACTIONS.key?(action)
raise ActionController::RoutingError, _('invalid button action')
end

def explorer
@breadcrumbs = []
@explorer = true
build_accordions_and_trees
send_action = CB_X_BUTTON_ALLOWED_ACTIONS[action]
Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this to just BUTTON_ALLOWED_ACTIONS? The X_ prefix means explorer and I guess we want this as a pattern in other screens, so there's no reason to keep the CB_. Also maybe merge the test with the ˙sendand move it to theGenericButtonMixin`? Here we should keep something like:

BUTTON_ALLOWED_ACTIONS = {}.freeze
evaluate_button(action, BUTTON_ALLOWED_ACTIONS)

send(send_action)

@right_cell_text ||= _("All Chargeback Rates")
set_form_locals if @in_a_form
session[:changed] = false
return if performed?

render :layout => "application" unless request.xml_http_request?
if params[:pressed].ends_with?("_copy", "_edit", "_new") && @flash_array.nil?
Copy link
Member

Choose a reason for hiding this comment

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

You're testing params[:pressed] but above you allocated it as action

if @flash_array
Copy link
Member

Choose a reason for hiding this comment

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

One line above you test against the @flash_array˙ if it's nil` and I think this is dead code then...

show_list
replace_gtl_main_div
else
render :update do |page|
page << javascript_prologue
page.redirect_to :action => @refresh_partial, :id => @redirect_id, :pressed => params[:pressed]
end
end
elsif params[:pressed].ends_with?("_delete")
javascript_redirect(:action => 'show_list', :flash_msg => @flash_array[0][:message])
elsif @refresh_div == "main_div" && @lastaction == "show_list"
replace_gtl_main_div
else
render_flash unless performed?
end
Copy link
Member

Choose a reason for hiding this comment

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

In general this huge if-else thing isn't really clean, I think we should do something like on other non-explorer controllers. This is from CloudNetworkController:

  def button
    @edit = session[:edit] # Restore @edit for adv search box
    params[:display] = @display if %w[vms instances images].include?(@display)
    params[:page] = @current_page unless @current_page.nil? # Save current page for list refresh

    @refresh_div = "main_div"

    case params[:pressed]
    when 'cloud_network_delete'
      delete_networks
      javascript_redirect(previous_breadcrumb_url)
    when "cloud_network_edit"
      javascript_redirect(:action => "edit", :id => checked_item_id)
    when "cloud_network_new"
      javascript_redirect(:action => "new")
    else
      super
    end
  end

I would prefer something like this even if it goes against my suggestion above about evaluate_button. Consistency is more important 😉

end

def set_form_locals
@x_edit_buttons_locals = {:action_url => 'cb_rate_edit'}
def show_list
super
# only need these gtl type buttons on the screen
@gtl_buttons = %w[view_list view_tree]
cb_rates_build_tree if @gtl_type == 'tree'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to keep this a little bit cleaner, what about defining them similarly to the toolbar in some of the explorers and pulling it in and adding the logic into the GenericListMixin#show_list the same way as we do with @center_toolbar?

I need to dig a little more, because my fulltext search on @gtl_buttons doesn't find much, so I don't completely understand the dataflow yet, but you might come up with a better idea in the meantime.

end

# Show the main Schedules list view
def cb_rates_list
@gtl_type = "list"
@explorer = true
if params[:ppsetting] # User selected new per page value
@items_per_page = params[:ppsetting].to_i # Set the new per page value
@settings.store_path(:perpage, @gtl_type.to_sym, @items_per_page) # Set the per page setting for this gtl type
def tree_select
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, I'd like to get rid of this method, because it's tied to the explorer-trees. However, as long as we don't have @brumik's new react trees merged, it should be fine. Can you please add a TODO comment to clean this up after the new trees are in and do the redirect in the TreeBuilder directly instead?

id = parse_nodetype_and_id(params[:id]).last
render :update do |page|
page << javascript_prologue
page.redirect_to :action => 'show', :id => id
end
Copy link
Member

Choose a reason for hiding this comment

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

Maybe javascript_redirect, I'm asking and not suggesting because I'm not sure 🙈

@sortcol = session[:rates_sortcol].nil? ? 0 : session[:rates_sortcol].to_i
@sortdir = session[:rates_sortdir].nil? ? "ASC" : session[:rates_sortdir]

@view, @pages = get_view(ChargebackRate, :named_scope => [[:with_rate_type, x_node.split('-').last]]) # Get the records (into a view) and the paginator

@current_page = @pages[:current] unless @pages.nil? # save the current page number
session[:rates_sortcol] = @sortcol
session[:rates_sortdir] = @sortdir

update_gtl_div('cb_rates_list') if pagination_or_gtl_request? && @show_list
end

def cb_rate_edit
@_params[:id] ||= find_checked_items[0]
rate = new_rate_edit? ? ChargebackRate.new : ChargebackRate.find(params[:id])
if params[:pressed] == 'chargeback_rates_edit' && rate.default?
skateman marked this conversation as resolved.
Show resolved Hide resolved
add_flash(_("Default Chargeback Rate \"%{name}\" cannot be edited.") % {:name => rate.description}, :error)
return
end
@redirect_id = params[:id] if params[:id]
@refresh_partial = "edit"
end

def edit
assert_privileges(params[:pressed]) if params[:pressed]
case params[:button]
when "cancel"
Expand All @@ -80,13 +85,12 @@ def cb_rate_edit
else
add_flash(_("Add of new Chargeback Rate was cancelled by the user"))
end
get_node_info(x_node)
@edit = session[:edit] = nil # clean out the saved info
session[:changed] = false
replace_right_cell
javascript_redirect(:action => @lastaction, :id => params[:id], :flash_msg => @flash_array[0][:message])
Copy link
Member

Choose a reason for hiding this comment

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

If the @flash_array[0][:message] is really necessary, why not set the actual value here? Is there any other path that would render the message?

when "save", "add"
id = params[:button] == "save" ? params[:id] : "new"
return unless load_edit("cbrate_edit__#{id}", "replace_cell__chargeback_rate")
return unless load_edit("cbrate_edit__#{id}")

@rate = params[:button] == "add" ? ChargebackRate.new : ChargebackRate.find(params[:id])
Copy link
Member

Choose a reason for hiding this comment

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

Don't we have some kind of pattern of setting @record in non-explorer screens? I'm trying to find similar Model.new/Model.find on other screens, but no success 😕

if @edit[:new][:description].nil? || @edit[:new][:description] == ""
Expand Down Expand Up @@ -117,8 +121,7 @@ def cb_rate_edit
end
@edit = session[:edit] = nil # clean out the saved info
session[:changed] = @changed = false
get_node_info(x_node)
replace_right_cell(:replace_trees => [:cb_rates])
javascript_redirect(:action => @lastaction, :id => params[:id], :flash_msg => @flash_array[0][:message])
Copy link
Member

Choose a reason for hiding this comment

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

Same here...

else
@rate.errors.each do |field, msg|
add_flash("#{field.to_s.capitalize} #{msg}", :error)
Expand All @@ -134,32 +137,25 @@ def cb_rate_edit
@changed = session[:changed] = (@edit[:new] != @edit[:current])
javascript_flash
end

when "reset", nil # displaying edit from for actions: new, edit or copy
@in_a_form = true
@_params[:id] ||= find_checked_items[0]
session[:changed] = params[:pressed] == 'chargeback_rates_copy'

@rate = new_rate_edit? ? ChargebackRate.new : ChargebackRate.find(params[:id])
skateman marked this conversation as resolved.
Show resolved Hide resolved
@record = @rate

if params[:pressed] == 'chargeback_rates_edit' && @rate.default?
render_flash(_("Default Chargeback Rate \"%{name}\" cannot be edited.") % {:name => @rate.description}, :error)
return
end

cb_rate_set_form_vars

add_flash(_("All changes have been reset"), :warning) if params[:button] == "reset"

replace_right_cell
if params[:button] == "reset"
add_flash(_("All changes have been reset"), :warning)
render :update do |page|
page << javascript_prologue
page.replace("form_div", :partial => "edit")
page << "miqSparkle(false);"
Copy link
Member

Choose a reason for hiding this comment

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

Why aren't you using javascript_redirect here?

end
end
end
end

# AJAX driven routine to check for changes in ANY field on the form
def cb_rate_form_field_changed
return unless load_edit("cbrate_edit__#{params[:id]}", "replace_cell__chargeback")

return unless load_edit("cbrate_edit__#{params[:id]}")
cb_rate_get_form_vars
render :update do |page|
page << javascript_prologue
Expand All @@ -170,15 +166,6 @@ def cb_rate_form_field_changed
end
end

def cb_rate_show
@display = "main"
if @record.nil?
flash_to_session(_('Error: Record no longer exists in the database'), :error)
redirect_to(:action => 'cb_rates_list')
return
end
end

# Delete all selected or single displayed action(s)
def cb_rates_delete
assert_privileges("chargeback_rates_delete")
Expand All @@ -190,18 +177,13 @@ def cb_rates_delete
end
else # showing 1 rate, delete it
cb_rate = ChargebackRate.find_by(:id => params[:id])
self.x_node = x_node.split('_').first
if cb_rate.nil?
add_flash(_("Chargeback Rate no longer exists"), :error)
else
rates.push(params[:id])
end
end
process_cb_rates(rates, 'destroy') if rates.present?

cb_rates_list
@right_cell_text = _("%<typ>s Chargeback Rates") % {:typ => x_node.split('-').last}
replace_right_cell(:replace_trees => [:cb_rates])
end

# Add a new tier at the end
Expand Down Expand Up @@ -240,9 +222,7 @@ def cb_tier_remove

# Delete tier record
@edit[:new][:tiers][detail_index].delete_at(tier_to_remove_index.to_i)

@changed = session[:changed] = true

render :update do |page|
page << javascript_prologue
page.replace_html("chargeback_rate_edit_form", :partial => "cb_rate_edit_table")
Expand All @@ -256,43 +236,8 @@ def title

private ############################

def features
[
{
:role => "chargeback_rates",
:name => :cb_rates,
:title => _("Rates")
}
].map { |hsh| ApplicationController::Feature.new_with_hash(hsh) }
end

def get_node_info(node, show_list = true)
@show_list = show_list
node = valid_active_node(node)
if node == "root"
@record = nil
@right_cell_text = _("All Chargeback Rates")
elsif ["xx-Compute", "xx-Storage"].include?(node)
@record = nil
@right_cell_text = case node
when "xx-Compute" then _("Compute Chargeback Rates")
when "xx-Storage" then _("Storage Chargeback Rates")
end
cb_rates_list
else
@record = ChargebackRate.find(parse_nodetype_and_id(node).last)
@sb[:action] = nil
@right_cell_text = case @record.rate_type
when "Compute" then _("Compute Chargeback Rate \"%{name}\"") % {:name => @record.description}
when "Storage" then _("Storage Chargeback Rate \"%{name}\"") % {:name => @record.description}
end
cb_rate_show
end
{:view => @view, :pages => @pages}
end

def cb_rates_build_tree
TreeBuilderChargebackRates.new("cb_rates_tree", @sb)
@tree = TreeBuilderChargebackRates.new("tree", @sb)
end

# Common Schedule button handler routines
Expand All @@ -308,7 +253,7 @@ def cb_rate_set_form_vars
@edit[:new][:tiers] = []
@edit[:new][:num_tiers] = []
@edit[:new][:description] = @rate.description
@edit[:new][:rate_type] = @rate.rate_type || x_node.split('-').last
@edit[:new][:rate_type] = @rate.rate_type || "Compute"
@edit[:new][:details] = []

tiers = []
Expand Down Expand Up @@ -369,6 +314,7 @@ def cb_rate_set_form_vars
# Get variables from edit form
def cb_rate_get_form_vars
@edit[:new][:description] = params[:description] if params[:description]
@edit[:new][:rate_type] = params[:rate_type] if params[:rate_type]
if params[:currency]
@edit[:new][:currency] = params[:currency].to_i
rate_detail_currency = Currency.find(params[:currency])
Expand Down Expand Up @@ -421,64 +367,7 @@ def cb_rate_set_record_vars
end

def new_rate_edit?
params[:id] == 'new' || params[:pressed] == 'chargeback_rates_new'
end

def replace_right_cell(options = {})
replace_trees = Array(options[:replace_trees])
replace_trees = @replace_trees if @replace_trees # get_node_info might set this
@explorer = true
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_build_tree]) if replace_trees.include?(:cb_rates)

# FIXME
# if params[:action].ends_with?("_delete")
# page << "miqTreeActivateNodeSilently('#{x_active_tree.to_s}', '<%= x_node %>');"
# end
# presenter[:select_node] = x_node if params[:action].ends_with?("_delete")
presenter[:osf_node] = x_node

if c_tb.present?
presenter.reload_toolbars(:center => c_tb)
end
presenter.set_visibility(c_tb.present?, :toolbar)
presenter.update(:main_div, r[:partial => 'rates_tabs'])

if @record || @in_a_form ||
(@pages && (@items_per_page == ONE_MILLION || @pages[:items] == 0))
if %w[chargeback_rates_copy chargeback_rates_edit chargeback_rates_new].include?(@sb[:action])
presenter.hide(:toolbar)
# incase it was hidden for summary screen, and incase there were no records on show_list
presenter.show(:paging_div, :form_buttons_div).remove_paging
locals = {:record_id => @edit[:rec_id]}
locals[:action_url] = 'cb_rate_edit'
presenter.update(:form_buttons_div, r[:partial => 'layouts/x_edit_buttons', :locals => locals])
else
# Added so buttons can be turned off even tho div is not being displayed it still pops up Abandon changes box when trying to change a node on tree after saving a record
presenter.hide(:buttons_on).show(:toolbar).hide(:paging_div)
presenter.hide(:form_buttons_div) if params[:button]
end
else
presenter.hide(:form_buttons_div)
if x_node == "root"
presenter.hide(:toolbar).remove_paging
end
presenter.show(:paging_div)
end

presenter[:record_id] = determine_record_id_for_presenter

presenter[:clear_gtl_list_grid] = @gtl_type && @gtl_type != 'list'

presenter[:right_cell_text] = @right_cell_text
presenter[:lock_sidebar] = @in_a_form && @edit

presenter.update(:breadcrumbs, r[:partial => 'layouts/breadcrumbs'])

render :json => presenter.for_render
!params[:id].present? || params[:id] == 'new' || params[:pressed] == 'chargeback_rates_new'
skateman marked this conversation as resolved.
Show resolved Hide resolved
end

def display_detail_errors(detail, errors)
Expand Down Expand Up @@ -507,6 +396,7 @@ def breadcrumbs_options
:breadcrumbs => [
{:title => _("Overview")},
{:title => _("Chargeback")},
{:title => _("Rates"), :url => controller_url},
],
}
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/mixins/generic_show_mixin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def show_main
:url => "/#{controller_name}/show_list?page=#{@current_page}&refresh=y"},
true)

drop_breadcrumb(:name => _("%{name} (Summary)") % {:name => @record.name},
drop_breadcrumb(:name => _("%{name} (Summary)") % {:name => @record.respond_to?(:name) ? @record.name : @record.description},
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we have a helper for name/description. If not, we could create one. But there should be one I think.

:url => show_link(@record))
@showtype = "main"
end
Expand Down
Loading