Toolbar refactoring: Ops #417

vecerek · 2017-02-17T14:51:51Z

Toolbar button ids	Toolbar button classes created or edited
schedule_run_now	ScheduleRunNow < Basic
rbac_tenant_add, rbac_project_add	TenantAdd < GenericFeatureButton
reload_server_tree	ReloadServerTree < Basic
refresh_server_summary, restart_server	DiagnosticsSummary < Basic
restart_workers	RefreshWorkers < Basic
collect_current_logs, collect_logs, log_depot_edit	DiagnosticsLogs < CollectLogs
refresh_log, fetch_log	DiagnosticsEvmLogs < Basic
refresh_audit_log, fetch_audit_log	DiagnosticsAuditLogs < Basic
refresh_production_log, fetch_production_log	DiagnosticsProductionLogs < Basic
db_refresh	DbRefsresh < Basic

Links

Parent issue: ManageIQ/manageiq#6259
Related issue: ManageIQ/manageiq#6554
Pending (not anymore): ManageIQ/manageiq#13982
Pivotal Tracker: https://www.pivotaltracker.com/story/show/133670283

miq-bot · 2017-02-20T11:20:17Z

@vecerek Cannot apply the following label because they are not recognized: pending_core

miq-bot · 2017-02-20T11:30:07Z

@vecerek Cannot apply the following label because they are not recognized: pending-core

miq-bot · 2017-02-20T11:30:08Z

@vecerek Cannot apply the following label because they are not recognized: pending-core

- Use Hash#fetch, since the feature might not always be specified. Later, Hash#dig could be used.

romanblanco · 2017-02-23T14:14:39Z

app/helpers/application_helper/toolbar/miq_schedule_center.rb

@@ -23,7 +23,8 @@ class ApplicationHelper::Toolbar::MiqScheduleCenter < ApplicationHelper::Toolbar
          :schedule_run_now,


'fa fa-play-circle-o fa-lg', missing here

romanblanco · 2017-02-23T14:20:47Z

app/helpers/application_helper/button/schedule_run_now.rb

@@ -0,0 +1,7 @@
+class ApplicationHelper::Button::ScheduleRunNow < ApplicationHelper::Button::ButtonWithoutRbacCheck


@vecerek Hm, i know I've suggested to use ButtonWithoutRbacCheck class, but now, when I'm trying the button, I'm getting back error message:

So, the buttons parent class probably should be Basic, as it was apparently disabled for me for a reason, and the only strange thing to me, is that the OOTB admin user does not have rights to use this feature.

Seems like other issue to me.

cc/ @dclarizio

romanblanco · 2017-02-23T15:23:22Z

app/helpers/application_helper/button/generic_feature_button.rb

@@ -3,7 +3,8 @@ class ApplicationHelper::Button::GenericFeatureButton < ApplicationHelper::Butto

  def initialize(view_context, view_binding, instance_data, props)
    super
-    @feature = props[:options][:feature]
+    # TODO: use #dig when ruby2.2 is no longer supported
+    @feature = props.fetch(:options, {}).fetch(:feature, nil)


@vecerek Why not just

@feature = props.fetch_path(:options, :feature)

romanblanco · 2017-02-23T16:00:44Z

app/helpers/application_helper/button/rbac_common_feature_button.rb

+  delegate :rbac_common_feature_for_buttons, :to => :@view_context
+
+  def role_allows_feature?
+    role_allows?(:feature => rbac_common_feature_for_buttons(@feature))


@vecerek I was pretty confused at this point, because you check availability of a @feature for a role at a lot of buttons, where you don't even define the feature to check (in toolbar).

After explanation, I've understood, that's why you've changed feature loading in previous commit af8a4e2, but still, it seems i bit wrong.

Isn't it better to use GenericFeatureButton based class only for the two buttons, that the rbac_common_feature_for_buttons method checks?

/cc @PanSpagetka

I agree that it look confusing when you have button named RbacCommonFeatureButton and you intentionally don't set any feature. Also I think, that it will hide button, which probably isn't right.

PanSpagetka · 2017-02-24T12:24:58Z

app/helpers/application_helper/toolbar/tenants_center.rb

@@ -13,7 +13,8 @@ class ApplicationHelper::Toolbar::TenantsCenter < ApplicationHelper::Toolbar::Ba
          N_('Edit the selected item'),
          :url_parms => "main_div",
          :enabled   => false,
-          :onwhen    => "1"),
+          :onwhen    => "1",
+          :klass     => ApplicationHelper::Button::TenantEdit),


Remove this class because you can use Basic button.

PanSpagetka · 2017-02-24T12:26:34Z

app/helpers/application_helper/button/tenant_edit.rb

  def disabled?
-    @record.source
+    @record.try!(:source)


There is no need to modify this class, because it is not needed to add this to summary toolbar.

…necessary assignement in tenants_center toolbar

When ButtonWithoutRbacCheck has been the parent, an error message has been being thrown which stated: 'The user is not authorized for this task or item.'; even though the user has had an admin role.

romanblanco · 2017-02-24T13:07:18Z

@vecerek Please complete the description, at least add the related issues

vecerek · 2017-02-24T13:52:50Z

app/helpers/application_helper/button/reload_server_tree.rb

@@ -0,0 +1,7 @@
+class ApplicationHelper::Button::ReloadServerTree < ApplicationHelper::Button::Basic


No button has been assigned to this class. The assignment needs to be added.

/cc @romanblanco

@romanblanco just pushed the changes, could you check it in UI, please?

romanblanco · 2017-02-24T14:07:34Z

app/helpers/application_helper/button/generic_feature_button.rb

@@ -4,7 +4,7 @@ class ApplicationHelper::Button::GenericFeatureButton < ApplicationHelper::Butto
  def initialize(view_context, view_binding, instance_data, props)
    super
    # TODO: use #dig when ruby2.2 is no longer supported
-    @feature = props.fetch(:options, {}).fetch(:feature, nil)
+    @feature = props.fetch_path(:options, :feature)


@vecerek Please remove this prevention. As we discussed, we'd like to have it in a way, that if you have a button that should check for a feature, you should always set the feature in a toolbar.

done, changes pushed

romanblanco · 2017-02-24T14:08:16Z

Once all the mentioned issues are fixed, it LGTM. Good job, @vecerek! 👍

vecerek · 2017-02-24T14:16:10Z

All issues fixed.

/cc @martinpovolny @mzazrivec

Also add the button property feature option in specs.

miq-bot · 2017-02-24T14:43:10Z

Checked commits vecerek/manageiq-ui-classic@2b51370~...0cd906f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
33 files checked, 17 offenses detected

app/helpers/application_helper/toolbar/diagnostics_region_center.rb

❗ - Line 8, Col 60 - Style/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.

app/helpers/application_helper/toolbar/diagnostics_server_center.rb

app/helpers/application_helper/toolbar/diagnostics_zone_center.rb

❗ - Line 129, Col 60 - Style/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 8, Col 60 - Style/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.

app/helpers/application_helper/toolbar/miq_schedule_center.rb

❗ - Line 27, Col 62 - Style/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.

app/helpers/application_helper/toolbar/miq_schedules_center.rb

❗ - Line 56, Col 66 - Style/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.

app/helpers/application_helper/toolbar/vmdb_table_center.rb

❗ - Line 8, Col 53 - Style/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.

app/helpers/application_helper/toolbar/vmdb_tables_center.rb

❗ - Line 8, Col 53 - Style/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.

miq-bot added the wip label Feb 17, 2017

vecerek force-pushed the toolbar_refactoring__ops branch 2 times, most recently from 2623819 to 33d903d Compare February 20, 2017 11:18

miq-bot added the pending core label Feb 20, 2017

vecerek force-pushed the toolbar_refactoring__ops branch 3 times, most recently from 9019e7c to 6700090 Compare February 23, 2017 11:22

romanblanco mentioned this pull request Feb 23, 2017

Toolbar refactoring: Simple buttons without Rbac check #175

Merged

miq-bot changed the title ~~[WIP] Toolbar refactoring: Ops~~ Toolbar refactoring: Ops Feb 23, 2017

miq-bot added refactoring and removed wip labels Feb 23, 2017

Attila Vecerek added 7 commits February 23, 2017 15:07

Create mixin with methods that determine the active tree and tab

2b51370

Refactor schedule_run_now button in Ops

9af4dce

Refactor buttons in ops' diagnostics tree

630cc05

Edit GenericFeatureButton's feature retrieval

af8a4e2

- Use Hash#fetch, since the feature might not always be specified. Later, Hash#dig could be used.

Refactor buttons in ops' rbac_tree

c4d4d2b

Refactor buttons in ops' vmdb_tree

b277753

Remove #hide_button_ops method

34ea18f

vecerek force-pushed the toolbar_refactoring__ops branch from 6700090 to 34ea18f Compare February 23, 2017 14:09

romanblanco reviewed Feb 23, 2017

View reviewed changes

Review response: Simplify rbac_tree button assignements

f38754d

PanSpagetka suggested changes Feb 24, 2017

View reviewed changes

Attila Vecerek added 2 commits February 24, 2017 13:55

Review response: Rollback changes done to TenantEdit class and its un…

5587124

…necessary assignement in tenants_center toolbar

Review response: Add icon to schedule_run_now button

b318fc7

Review response: Set ScheduleRunNow parent to Basic

e87b26f

When ButtonWithoutRbacCheck has been the parent, an error message has been being thrown which stated: 'The user is not authorized for this task or item.'; even though the user has had an admin role.

vecerek commented Feb 24, 2017

View reviewed changes

Assign reload_server_tree button to its class

164ce76

romanblanco reviewed Feb 24, 2017

View reviewed changes

Review response: Rollback GenericFeatureButton#initialize implementation

0cd906f

Also add the button property feature option in specs.

vecerek force-pushed the toolbar_refactoring__ops branch from 9cc0497 to 0cd906f Compare February 24, 2017 14:38

miq-bot added the toolbars label Feb 24, 2017

mzazrivec self-assigned this Feb 24, 2017

mzazrivec added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 24, 2017

mzazrivec merged commit ef884b3 into ManageIQ:master Feb 24, 2017

vecerek deleted the toolbar_refactoring__ops branch February 24, 2017 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar refactoring: Ops #417

Toolbar refactoring: Ops #417

vecerek commented Feb 17, 2017 •

edited

Loading

miq-bot commented Feb 20, 2017

miq-bot commented Feb 20, 2017

miq-bot commented Feb 20, 2017

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

PanSpagetka Feb 24, 2017

PanSpagetka Feb 24, 2017

PanSpagetka Feb 24, 2017

romanblanco commented Feb 24, 2017

vecerek Feb 24, 2017

vecerek Feb 24, 2017

romanblanco Feb 24, 2017

vecerek Feb 24, 2017

romanblanco commented Feb 24, 2017 •

edited

Loading

vecerek commented Feb 24, 2017

miq-bot commented Feb 24, 2017

		@@ -23,7 +23,8 @@ class ApplicationHelper::Toolbar::MiqScheduleCenter < ApplicationHelper::Toolbar
		:schedule_run_now,

		@@ -0,0 +1,7 @@
		class ApplicationHelper::Button::ScheduleRunNow < ApplicationHelper::Button::ButtonWithoutRbacCheck

		@@ -0,0 +1,7 @@
		class ApplicationHelper::Button::ReloadServerTree < ApplicationHelper::Button::Basic

Toolbar refactoring: Ops #417

Toolbar refactoring: Ops #417

Conversation

vecerek commented Feb 17, 2017 • edited Loading

Links

miq-bot commented Feb 20, 2017

miq-bot commented Feb 20, 2017

miq-bot commented Feb 20, 2017

romanblanco Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

romanblanco Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

romanblanco Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

romanblanco Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

PanSpagetka Feb 24, 2017

Choose a reason for hiding this comment

PanSpagetka Feb 24, 2017

Choose a reason for hiding this comment

PanSpagetka Feb 24, 2017

Choose a reason for hiding this comment

romanblanco commented Feb 24, 2017

vecerek Feb 24, 2017

Choose a reason for hiding this comment

vecerek Feb 24, 2017

Choose a reason for hiding this comment

romanblanco Feb 24, 2017

Choose a reason for hiding this comment

vecerek Feb 24, 2017

Choose a reason for hiding this comment

romanblanco commented Feb 24, 2017 • edited Loading

vecerek commented Feb 24, 2017

miq-bot commented Feb 24, 2017

vecerek commented Feb 17, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco Feb 23, 2017 •

edited

Loading

romanblanco commented Feb 24, 2017 •

edited

Loading