Chargeback Rates non explorer #7039

h-kataria · 2020-05-11T20:00:59Z

Converted Chargeback Rates explorer to be a non-explorer screen, this will help with direct linking of Chargeback Rates list/show screen.
Added a new Tree View button to GTL views toolbar, to support switching between list/tree view on the Chargeback Rates screen
Added a new Type drop down on Add a new Chargeback Rate form, that defaults to Compute

Depends on #7016

Direct link to list view: http://localhost:3000/chargeback_rate/show_list
Direct link to summary screen: http://localhost:3000/chargeback_rate/show/7

h-kataria · 2020-05-11T20:07:56Z

@gtanzillo @Fryguy @chessbyte cc

chessbyte · 2020-05-11T21:27:49Z

/cc @gtanzillo

- Added a button in GTL view toolbar to add ability to toggle between list view and tree view. Tree has links to Chargeback Rate records, User can click on "Rates" link in breadcrumbs to go back to list view. - Updated all toolbar buttons to work in non-explorer modes Follow up PR for ManageIQ#7016 Fixes ManageIQ#6996

ManageIQ#6996

ManageIQ/manageiq-ui-classic#7039

skateman · 2020-05-15T06:27:20Z

app/controllers/application_controller/report_downloads.rb

@@ -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,


Can you please explain why is this extra complexity necessary?

@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.

skateman

This is a good start, but let's make some stuff nicer. I'd like to have things here as a pattern to how to convert from explorer to non-explorer.

Also I think it would need more people to review the complexity stuff. We want to simplify things, not add more if-else and constant in global places if possible.

skateman · 2020-05-15T06:32:27Z

app/controllers/chargeback_rate_controller.rb

-    @breadcrumbs = []
-    @explorer    = true
-    build_accordions_and_trees
+    send_action = CB_X_BUTTON_ALLOWED_ACTIONS[action]


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)

skateman · 2020-05-15T06:33:16Z

app/controllers/chargeback_rate_controller.rb


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


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

skateman · 2020-05-15T06:34:18Z

app/controllers/chargeback_rate_controller.rb


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


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

skateman · 2020-05-15T06:38:41Z

app/controllers/chargeback_rate_controller.rb

+      replace_gtl_main_div
+    else
+      render_flash unless performed?
+    end


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 😉

skateman · 2020-05-15T06:44:20Z

app/controllers/chargeback_rate_controller.rb

+    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'


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.

skateman · 2020-05-15T07:05:25Z

app/presenters/tree_builder_chargeback.rb

@@ -2,7 +2,7 @@ class TreeBuilderChargeback < TreeBuilder
  private

  def tree_init_options
-    {:open_all => true, :full_ids => true}
+    {:open_all => true, :full_ids => true, :click_url => "/chargeback_rate/tree_select/", :onclick => "miqOnClickGeneric"}


You're doing this in the wrong TreeBuilder as it could break stuff in the other subclasses. Can you please redefine it in the TreeBuilderChargebackRates and call super.merge? Also let's make it there a multiline and you can set full_ids to false (there). Maybe it would need a slight change at the tree_select but it should probably simplify stuff.

skateman · 2020-05-15T07:09:28Z

app/presenters/tree_builder_chargeback.rb

@@ -13,6 +13,7 @@ def x_get_tree_roots
      {
        :id   => type,
        :text => type,
+        :selectable => false,


Again, this breaks stuff in the other trees. Please redefine it TreeBuilderChargeBackRates and call super.map to set them non-selectable.

app/presenters/tree_node/chargeback_rate.rb

skateman · 2020-05-15T07:10:59Z

app/views/chargeback_rate/_tree.html.haml

@@ -0,0 +1 @@
+= render(:partial => "shared/tree", :locals => {:tree => @tree, :name => @tree.name})


This won't make @brumik happy 😆 not really this one, but that his PR is still not in 😕

skateman · 2020-05-15T07:11:52Z

app/views/chargeback_rate/show_list.html.haml

@@ -0,0 +1,3 @@
+#main_div
+  = render :partial => 'layouts/gtl'
+  = render :partial => 'tree' if @tree.present?


I don't really like this, but as long as we're not using the react component, why not. Maybe you could move the one-liner from the tree.html.haml directly here...

Added condition to only render gtl or tree

moved out chargeback rates tree specific code changes to `TreeBuilderChargeBackRates`

ZitaNemeckova

Tested in UI. LGTM 👍

app/controllers/chargeback_rate_controller.rb

app/helpers/application_helper/toolbar/chargeback_rates_center.rb

app/helpers/application_helper/toolbar_builder.rb

And changed to use toolbar module to load toolbars for chargeback rates list/show

app/controllers/chargeback_rate_controller.rb

config/routes.rb

app/helpers/application_helper/toolbar_chooser.rb

- Removed `db_list` that was accidentally added in this PR - Fixed routing spec tests. - Reverted changes made in toolbar_chooser, see ManageIQ#7039 (comment) for more info

skateman · 2020-05-19T15:17:47Z

config/routes.rb

        cb_rate_form_field_changed
-        cb_rate_show
        cb_rates_delete


Can we simplify this into just delete?

miq-bot · 2020-05-19T15:51:06Z

Checked commits h-kataria/manageiq-ui-classic@c083fc0~...bf3fb4d with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
25 files checked, 53 offenses detected

app/controllers/application_controller/report_downloads.rb

❗ - Line 214, Col 74 - Rails/OutputSafety - Tagging a string as html safe may be a security risk.

app/controllers/chargeback_rate_controller.rb

❗ - Line 118, Col 7 - Style/MultilineIfModifier - Favor a normal if-statement over a modifier clause in a multiline statement.
❗ - Line 128, Col 5 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
❗ - Line 31, Col 55 - Layout/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 337, Col 5 - Rails/Blank - Use params[:id].blank? instead of !params[:id].present?.
❗ - Line 63, Col 81 - Layout/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 63, Col 83 - Layout/SpaceAroundOperators - Operator => should be surrounded by a single space.
❗ - Line 97, Col 83 - Layout/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 97, Col 85 - Layout/SpaceAroundOperators - Operator => should be surrounded by a single space.

app/helpers/application_helper/toolbar/chargeback_rate_center.rb

❗ - Line 16, Col 73 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 24, Col 32 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 33, Col 75 - Layout/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 36, Col 3 - Layout/IndentFirstArrayElement - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/helpers/application_helper/toolbar_chooser.rb

❗ - Line 39, Col 7 - Layout/MultilineOperationIndentation - Align the operands of a condition in a elsif statement spanning multiple lines.
❗ - Line 40, Col 7 - Layout/MultilineOperationIndentation - Align the operands of a condition in a elsif statement spanning multiple lines.

app/presenters/menu/default_menu.rb

❗ - Line 51, Col 11 - Layout/IndentFirstArrayElement - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/views/chargeback_rate/_cb_rate_edit_table.html.haml

⚠️ - Line 1 - Avoid using instance variables in partials views

app/views/chargeback_rate/_cb_rate_show.html.haml

⚠️ - Line 16 - Avoid using instance variables in partials views

app/views/chargeback_rate/_edit.html.haml

app/views/chargeback_rate/_tier_first_row.haml

⚠️ - Line 3 - Avoid using instance variables in partials views

app/views/chargeback_rate/_tier_row.haml

⚠️ - Line 2 - Avoid using instance variables in partials views

app/views/chargeback_rate/edit.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.

app/views/chargeback_rate/show_list.html.haml

⚠️ - Line 1 - Layout/TrailingBlankLines: Final newline missing.
⚠️ - Line 1 - id attribute must be in lisp-case

spec/controllers/chargeback_rate_controller_spec.rb

❗ - Line 467, Col 66 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.
❗ - Line 477, Col 68 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.
❗ - Line 487, Col 66 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.
❗ - Line 497, Col 68 - Style/BracesAroundHashParameters - Redundant curly braces around a hash parameter.

spec/presenters/tree_builder_spec.rb

❗ - Line 34, Col 17 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

skateman · 2020-05-19T16:02:23Z

When I try to copy a chargeback rate and edit it's name, the new name doesn't get saved.

skateman · 2020-05-19T16:03:41Z

If I try to add a new chargeback rate, I am getting the following error:

`javascript_tag(javascript_focus('description'))` for some reason was not working well with `javascript_redirect`

skateman

Missed removal of these in ManageIQ#7039

Chargeback Rates non explorer (cherry picked from commit cdf3b48)

simaishi · 2020-05-21T19:56:45Z

Jansa backport details:

$ git log -1
commit 9b8376f5390a6b08ba87368262421a4dbc5e1b75
Author: Halász Dávid <[email protected]>
Date:   Tue May 19 19:18:47 2020 +0200

    Merge pull request #7039 from h-kataria/cb_rates_no_explorer

    Chargeback Rates non explorer

    (cherry picked from commit cdf3b4813979c66a821cf1afa0d847380d2e7f0f)

ManageIQ#7039 (comment). Reverted any gtl_view buttons related code that was introduced in this PR.

- Removed `db_list` that was accidentally added in this PR - Fixed routing spec tests. - Reverted changes made in toolbar_chooser, see ManageIQ#7039 (comment) for more info

Missed removal of these in ManageIQ#7039

h-kataria added enhancement wip jansa/yes? labels May 11, 2020

h-kataria requested a review from skateman May 11, 2020 20:01

h-kataria assigned skateman May 11, 2020

h-kataria force-pushed the cb_rates_no_explorer branch from 3970544 to 6059134 Compare May 11, 2020 20:01

h-kataria force-pushed the cb_rates_no_explorer branch 2 times, most recently from 5260a86 to c3381c0 Compare May 11, 2020 21:00

miq-bot added the unmergeable label May 12, 2020

h-kataria force-pushed the cb_rates_no_explorer branch from c3381c0 to 6c35fd4 Compare May 12, 2020 19:36

h-kataria force-pushed the cb_rates_no_explorer branch from 6c35fd4 to c083fc0 Compare May 12, 2020 19:38

miq-bot removed the unmergeable label May 12, 2020

gtanzillo mentioned this pull request May 12, 2020

Direct link support for Chargeback UI elements ManageIQ/manageiq#20162

Closed

12 tasks

h-kataria removed the wip label May 13, 2020

h-kataria changed the title ~~[WIP] - Chargeback Rates non explorer~~ Chargeback Rates non explorer May 13, 2020

Fixed an issue to show flash message when trying to edit default rate

0fbdb08

ManageIQ#6996

h-kataria added a commit to h-kataria/manageiq-cross_repo-tests that referenced this pull request May 13, 2020

Cross Repo tests for conversion of Chargeback Rates explorer

4e646ab

ManageIQ/manageiq-ui-classic#7039

This was referenced May 13, 2020

Cross Repo tests for conversion of Chargeback Rates explorer ManageIQ/manageiq-cross_repo-tests#123

Closed

Convert explorers to non-explorer style screens #6819

Open

skateman reviewed May 15, 2020

View reviewed changes

skateman suggested changes May 15, 2020

View reviewed changes

skateman requested review from mzazrivec, himdel and ZitaNemeckova May 15, 2020 07:14

skateman mentioned this pull request May 15, 2020

Non explorer chargeback assignments #7049

Merged

h-kataria added 2 commits May 15, 2020 12:59

changed to render tree directly into show_list partial

67c3abf

Added condition to only render gtl or tree

set chargeback rates specific items in it's own tree

c59b978

moved out chargeback rates tree specific code changes to `TreeBuilderChargeBackRates`

ZitaNemeckova reviewed May 19, 2020

View reviewed changes

skateman suggested changes May 19, 2020

View reviewed changes

Deleted leftover tree view code

7922c22

And changed to use toolbar module to load toolbars for chargeback rates list/show

skateman reviewed May 19, 2020

View reviewed changes

app/controllers/chargeback_rate_controller.rb Show resolved Hide resolved

skateman suggested changes May 19, 2020

View reviewed changes

config/routes.rb Outdated Show resolved Hide resolved

config/routes.rb Outdated Show resolved Hide resolved

config/routes.rb Outdated Show resolved Hide resolved

app/helpers/application_helper/toolbar_chooser.rb Outdated Show resolved Hide resolved

Removed redundant routes

f78f984

- Removed `db_list` that was accidentally added in this PR - Fixed routing spec tests. - Reverted changes made in toolbar_chooser, see ManageIQ#7039 (comment) for more info

skateman reviewed May 19, 2020

View reviewed changes

config/routes.rb Outdated

cb_rate_form_field_changed

cb_rate_show

cb_rates_delete

Copy link

Member

skateman May 19, 2020

Choose a reason for hiding this comment

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

Can we simplify this into just delete?

simplified routes and adjusted method names accordingly

bf3fb4d

changed textfield to use data-miq_focus instead

5e9422a

`javascript_tag(javascript_focus('description'))` for some reason was not working well with `javascript_redirect`

skateman added the overview/chargeback label May 19, 2020

skateman approved these changes May 19, 2020

View reviewed changes

skateman merged commit cdf3b48 into ManageIQ:master May 19, 2020

gtanzillo added jansa/yes and removed jansa/yes? labels May 19, 2020

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request May 20, 2020

Removed leftover settings for Chargeback Rates

bd96af1

Missed removal of these in ManageIQ#7039

h-kataria mentioned this pull request May 20, 2020

Removed leftover settings for Chargeback Rates #7062

Merged

simaishi pushed a commit that referenced this pull request May 21, 2020

Merge pull request #7039 from h-kataria/cb_rates_no_explorer

9b8376f

Chargeback Rates non explorer (cherry picked from commit cdf3b48)

simaishi added jansa/backported and removed jansa/yes labels May 21, 2020

kbrock pushed a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 8, 2020

Removed chargeback rates tree view as per GH discussion/comment

00a47b5

ManageIQ#7039 (comment). Reverted any gtl_view buttons related code that was introduced in this PR.

kbrock pushed a commit to kbrock/manageiq-ui-classic that referenced this pull request Aug 8, 2020

Removed leftover settings for Chargeback Rates

39518a8

Missed removal of these in ManageIQ#7039

h-kataria mentioned this pull request Aug 13, 2020

Remove Accordions from explorer style screens and link them into menu navigation #6996

Closed

17 tasks

gtanzillo mentioned this pull request Nov 3, 2020

Convert explorers to non-explorer style screens - completed in Jansa #7478

Closed

3 tasks

h-kataria deleted the cb_rates_no_explorer branch January 18, 2021 21:48

himdel mentioned this pull request Jan 28, 2021

Dead render partial #7609

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chargeback Rates non explorer #7039

Chargeback Rates non explorer #7039

h-kataria commented May 11, 2020 •

edited

Loading

h-kataria commented May 11, 2020

chessbyte commented May 11, 2020

skateman May 15, 2020

h-kataria May 15, 2020

skateman left a comment •

edited

Loading

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

skateman May 15, 2020

ZitaNemeckova left a comment

skateman May 19, 2020

miq-bot commented May 19, 2020

skateman commented May 19, 2020

skateman commented May 19, 2020

skateman left a comment

simaishi commented May 21, 2020


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

		@@ -0,0 +1 @@
		= render(:partial => "shared/tree", :locals => {:tree => @tree, :name => @tree.name})

Chargeback Rates non explorer #7039

Chargeback Rates non explorer #7039

Conversation

h-kataria commented May 11, 2020 • edited Loading

h-kataria commented May 11, 2020

chessbyte commented May 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skateman left a comment • 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

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

ZitaNemeckova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 19, 2020

skateman commented May 19, 2020

skateman commented May 19, 2020

skateman left a comment

Choose a reason for hiding this comment

simaishi commented May 21, 2020

h-kataria commented May 11, 2020 •

edited

Loading

skateman left a comment •

edited

Loading