Toolbar button refactoring server role #11715

romanblanco · 2016-10-05T16:19:10Z

Toolbar buttons for ServerRole class moved from toolbar builder to separate button class

Toolbar buttons id	Toolbar buttons class
`zone_role_start`, `zone_role_suspend`	`RolePowerOptions`
`role_start`	`RoleStart < RolePowerOptions`
`role_suspend`	`RoleSuspend < RolePowerOptions`
`zone_promote_server`, `zone_demote_server`	`ServerLevelOptions`
`promote_server`	`ServerPromote < ServerLevelOptions`
`demote_server`	`ServerDemote < ServerLevelOptions`
`server_delete`	❌

Links

Parent issue: #6259
Related issue: #6554
Pivotal Tracker: https://www.pivotaltracker.com/story/show/126786645

romanblanco · 2016-10-05T16:22:14Z

@martinpovolny @PanSpagetka I'm not sure it it makes sense to have buttons like this in toolbar. I've checked if there is some way to make the buttons visible in JS, but couldn't find any.

So it doesn't seem to me as one of those exceptions, where the button is not visible, but can be enabled by JS.

EDIT: I might have been wrong, the buttons seems to be hidden by default, but there are some conditions for them in separate method for Ops. So far I'm marking this PR as WIP.

PanSpagetka · 2016-10-07T11:45:45Z

When you select some node in Diagnostic tree, like Zone and then Region, toolbar stays same for Region and depends on what was selected before.

Buttons in toolbar seems to be little bit weird, for example when you select Zone you can delete one server from this Zone, but this was same before this PR so I guess its right.

romanblanco · 2016-10-07T13:00:06Z

app/helpers/application_helper/button/role_power_options.rb

+  needs :@record
+
+  def visible?
+    @record.class == AssignedServerRole && @record.miq_server_started?


I've just discovered typo here, I'll fix it with other changes, please don't merge yet.

miq-bot · 2016-10-07T15:48:16Z

Checked commits romanblanco/manageiq@192bf58~...db69128 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
12 files checked, 20 offenses detected

app/helpers/application_helper/button/role_suspend.rb

❗ - Line 13, Col 24 - Style/IfInsideElse - Convert if nested inside else to elsif.
❕ - Line 15, Col 121 - Metrics/LineLength - Line is too long. [144/120]

app/helpers/application_helper/button/server_promote.rb

❕ - Line 9, Col 3 - Metrics/CyclomaticComplexity - Cyclomatic complexity for disabled? is too high. [7/6]

app/helpers/application_helper/toolbar/diagnostics_region_center.rb

❗ - Line 44, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 62, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 75, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 87, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❕ - Line 74, Col 121 - Metrics/LineLength - Line is too long. [126/120]
❕ - Line 86, Col 121 - Metrics/LineLength - Line is too long. [137/120]

app/helpers/application_helper/toolbar/diagnostics_zone_center.rb

❗ - Line 48, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 68, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 82, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 96, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❕ - Line 81, Col 121 - Metrics/LineLength - Line is too long. [126/120]
❕ - Line 95, Col 121 - Metrics/LineLength - Line is too long. [137/120]

spec/helpers/application_helper/buttons/role_start_spec.rb

❗ - Line 52, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 71, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 72, Col 11 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/helpers/application_helper/toolbar_builder_spec.rb

❗ - Line 762, Col 7 - Style/WordArray - Use %w or %W for an array of words.
❕ - Line 762, Col 121 - Metrics/LineLength - Line is too long. [142/120]

romanblanco · 2016-10-10T09:07:15Z

@PanSpagetka

When you select some node in Diagnostic tree, like Zone and then Region, toolbar stays same for Region and depends on what was selected before.

issues fixed. thanks! 👍

miq-bot · 2016-10-10T13:02:09Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

PanSpagetka · 2016-11-01T09:11:28Z

Looks good un UI

romanblanco · 2016-11-02T16:13:10Z

@martinpovolny PR is updated. Once Travis is green I believe this can be merged

…te class

…eparate class

miq-bot · 2016-11-02T16:17:31Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

`@sb` variable is not called directly in the button class, but needs to be set, because `x_active_tree` and `x_node` are wrappers for the `@sb`.

…er methods Buttons in 'diagnostics_servers_roles' and 'diagnostics_roles_servers' that were converted to separate classes were still getting hide, because in `toolbar_builder.rb` the method `build_toolbar_hide_button_ops` returns `true` for the buttons that are not listed in the case. By setting `false` and listing the buttons that are not converted yet in the case switch, the converted buttons are getting displayed correctly.

miq-bot added hacktoberfest technical debt ui labels Oct 5, 2016

romanblanco changed the title ~~Toolbar button refactoring server role~~ [WIP] Toolbar button refactoring server role Oct 5, 2016

miq-bot added the wip label Oct 5, 2016

romanblanco force-pushed the toolbar_button_refactoring_server_role branch 3 times, most recently from 45bb9a1 to 0bec807 Compare October 6, 2016 14:28

romanblanco changed the title ~~[WIP] Toolbar button refactoring server role~~ Toolbar button refactoring server role Oct 6, 2016

miq-bot removed the wip label Oct 6, 2016

chessbyte assigned martinpovolny Oct 6, 2016

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from 0bec807 to 804c4e9 Compare October 7, 2016 09:49

martinpovolny assigned jzigmund and PanSpagetka Oct 7, 2016

romanblanco commented Oct 7, 2016

View reviewed changes

miq-bot added the wip label Oct 7, 2016

romanblanco force-pushed the toolbar_button_refactoring_server_role branch 3 times, most recently from d22722d to db69128 Compare October 7, 2016 15:44

romanblanco force-pushed the toolbar_button_refactoring_server_role branch 2 times, most recently from 07b9e6f to 71721ca Compare October 7, 2016 16:42

miq-bot removed the wip label Oct 7, 2016

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from 74965c4 to de4d098 Compare October 10, 2016 09:06

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from de4d098 to a510904 Compare October 10, 2016 11:59

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from a510904 to d0ab20d Compare October 10, 2016 13:07

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from 70934b5 to 19cc41a Compare November 2, 2016 16:01

romanblanco added 8 commits November 2, 2016 17:14

Removed unused toolbar button server_delete

86cc443

Created generic class for role power options toolbar buttons

ebe3853

Toolbar button role_start moved to separate class

d23141c

Toolbar button role_suspend moved to separate class

2191096

Toolbar buttons zone_role_start and zone_role_suspend moved to separa…

b1e8bb3

…te class

Created generic class for server level options toolbar buttons

d1e9be4

Toolbar buttons zone_demote_server and zone_promote_server moved to s…

a330105

…eparate class

Toolbar button promote_server moved to separate class

e6e3c5e

romanblanco added 11 commits November 2, 2016 17:19

Toolbar button demote_server moved to separate class

9a4b04d

Removed specs for converted buttons

4b38078

Added spec for role_start toolbar button

dea8d40

Added spec for role_suspend toolbar button

4a07ca0

Added spec for server_demote toolbar button

c8ad064

Added spec for server_promote toolbar button

cd5b186

Updated generic classes, to check if the tree is correct

e3d44ee

Updated specs

397832d

Review response: Added missing instance variable

c22f642

`@sb` variable is not called directly in the button class, but needs to be set, because `x_active_tree` and `x_node` are wrappers for the `@sb`.

Use view context for ApplicationHelper methods

93627f2

romanblanco force-pushed the toolbar_button_refactoring_server_role branch from 19cc41a to eadc3fc Compare November 2, 2016 16:26

martinpovolny merged commit 716e611 into ManageIQ:master Nov 2, 2016

martinpovolny added the euwe/no label Nov 2, 2016

martinpovolny added this to the Sprint 49 Ending Nov 14, 2016 milestone Nov 2, 2016

romanblanco deleted the toolbar_button_refactoring_server_role branch November 2, 2016 17:27

romanblanco restored the toolbar_button_refactoring_server_role branch November 2, 2016 18:54

romanblanco deleted the toolbar_button_refactoring_server_role branch November 4, 2016 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar button refactoring server role #11715

Toolbar button refactoring server role #11715

romanblanco commented Oct 5, 2016 •

edited

Loading

romanblanco commented Oct 5, 2016 •

edited

Loading

PanSpagetka commented Oct 7, 2016 •

edited

Loading

romanblanco Oct 7, 2016

miq-bot commented Oct 7, 2016

romanblanco commented Oct 10, 2016

miq-bot commented Oct 10, 2016

PanSpagetka commented Nov 1, 2016

romanblanco commented Nov 2, 2016

miq-bot commented Nov 2, 2016

Toolbar button refactoring server role #11715

Toolbar button refactoring server role #11715

Conversation

romanblanco commented Oct 5, 2016 • edited Loading

Links

romanblanco commented Oct 5, 2016 • edited Loading

PanSpagetka commented Oct 7, 2016 • edited Loading

romanblanco Oct 7, 2016

Choose a reason for hiding this comment

miq-bot commented Oct 7, 2016

romanblanco commented Oct 10, 2016

miq-bot commented Oct 10, 2016

PanSpagetka commented Nov 1, 2016

romanblanco commented Nov 2, 2016

miq-bot commented Nov 2, 2016

romanblanco commented Oct 5, 2016 •

edited

Loading

romanblanco commented Oct 5, 2016 •

edited

Loading

PanSpagetka commented Oct 7, 2016 •

edited

Loading