Toolbar refactoring miq server and region #11879

vecerek · 2016-10-12T15:25:06Z

@romanblanco @PanSpagetka @martinpovolny
Removed the appearances of unused buttons including from within the specs and created a button class for refresh_workers button for its use outside the toolbar_builder method.

Toolbar buttons id	Action taken or Toolbar buttons class created
refresh_workers	RefreshWorkers < Basic
log_download	deleted
refresh_logs	deleted
log_collect	deleted
log_reload	deleted
logdepot_edit	deleted
processmanager_restart	deleted

Links

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

romanblanco

@vecerek please remove the empty files:

app/helpers/application_helper/button/configured_system_provision.rb
spec/helpers/application_helper/buttons/configured_system_provision_spec.rb

martinpovolny · 2016-10-13T09:41:26Z

app/helpers/application_helper/button/configured_system_provision.rb

+  def visible?
+    @record.provisionable?
+  end
+end


pls, fix you editor setting so add the newline here

martinpovolny · 2016-10-13T09:41:38Z

spec/helpers/application_helper/buttons/configured_system_provision_spec.rb

+      it_behaves_like 'will not be skipped for this record'
+    end
+  end
+end


and here....

martinpovolny · 2016-10-13T09:42:03Z

spec/helpers/application_helper/toolbar_builder_spec.rb

@@ -759,8 +759,7 @@ def method_missing(sym, *args)
        stub_user(:features => :all)
      end

-      ["role_start", "role_suspend", "promote_server", "demote_server",
-       "log_download", "refresh_logs", "log_collect", "log_reload", "logdepot_edit", "processmanager_restart", "refresh_workers"].each do |id|
+      ["role_start", "role_suspend", "promote_server", "demote_server"].each do |id|


The usage with brackets is seen throughout the whole spec file. It may rather be changed globally using sed. Should it be done as a new branch for standardization?

martinpovolny · 2016-10-13T09:42:48Z

@romanblanco, @PanSpagetka : pls, review and test

romanblanco · 2016-10-17T13:00:37Z

@vecerek please rebase.

vecerek · 2016-10-18T11:03:43Z

@romanblanco @martinpovolny the tests that failed on Travis are passing locally

romanblanco · 2016-10-18T18:18:41Z

@vecerek Are you sure that your comment belongs to this PR? It seems to me that it belongs to #11982, doesn't it?

@martinpovolny code looks fine, but I can't find the button in UI. I'd expect it to be in:
Configuration (Ops) -> Diagnostics -> Select server -> Workers tab, but it doesn't seem so

vecerek · 2016-10-20T11:12:49Z

@romanblanco just deleted the misplaced comment, thanks

@martinpovolny @dclarizio could I ask for another restart of Travis, please? This time, the failed tests were different. However, they have passed locally, too.

romanblanco · 2016-10-20T11:30:36Z

@vecerek

@romanblanco just deleted the misplaced comment, thanks

No problem 👍 ☺️

About the converted button refresh_workers, do you know, how to get to the button in UI? I wasn't able to find it.

vecerek · 2016-10-20T12:39:13Z

@romanblanco I think I might know why it does not get displayed over there. The refresh_workers button gets hidden on miq server and region items in the diagnostic tree. The @record's class is being evaluated over here. What do you think?

romanblanco · 2016-10-20T12:46:57Z

@vecerek I've found another occurrence of refresh_workers button here. This might be also the reason why it's getting hidden.

Can you remove all existing occurrences, so we can make sure the behaviour is not influenced from more places?

miq-bot · 2016-10-20T14:46:15Z

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

vecerek · 2016-10-21T16:52:49Z

@romanblanco it is the reload button left to the configuration. We couldn't find it anywhere, since my local master branch was not up-to-date.

vecerek · 2016-10-21T18:53:44Z

@romanblanco I had to put back the refresh_workers into #build_toolbar_hide_button_ops. The missing refresh_workers array item had caused the method to return true and hide the button before the button's #visible? method would have got executed.

Those failing 12 examples on Travis are finally not passing locally. However they do fail on master, as well.

vecerek · 2016-10-24T17:50:26Z

@romanblanco
"Oops, Travis did it again
He played with my code, got lost in the game
Oh baby, baby..."

Tests do pass locally and I do have the latest changes applied from master. Checked twice 😄 What could have gone wrong?

jzigmund

Please edit the tests according to the simplified button class

jzigmund · 2016-10-26T17:44:04Z

app/helpers/application_helper/button/refresh_workers.rb

@@ -0,0 +1,23 @@
+class ApplicationHelper::Button::RefreshWorkers < ApplicationHelper::Button::Basic


I've found that sufficient for the refresh_workers button is

class ApplicationHelper::Button::RefreshWorkers < ApplicationHelper::Button::Basic needs :@record, :@sb def visible? @view_context.x_active_tree == :diagnostics_tree && @sb[:active_tab] == 'diagnostics_workers' end end

Thanks, you're right. I included your suggestion into my last commit.

…ples from toolbar_builder_spec

…end the #visible? method by additional cases and add new spec examples accordingly

… to make the button visible again

… miq_server or miq_region only if layout != ops and tailors the spec file accordingly

miq-bot · 2016-10-27T14:17:32Z

Checked commits vecerek/manageiq@e6515c0~...2797d55 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
5 files checked, 0 offenses detected
Everything looks good. 🏆

vecerek · 2016-11-02T09:52:53Z

@romanblanco Travis finally likes my PR! 😄 Could you do a review, pls?

jzigmund

Looks good to me. @martinpovolny please review or/and merge

romanblanco suggested changes Oct 13, 2016

View reviewed changes

martinpovolny reviewed Oct 13, 2016

View reviewed changes

martinpovolny added refactoring ui labels Oct 13, 2016

martinpovolny assigned romanblanco and PanSpagetka Oct 13, 2016

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from 9547bbf to 3f23825 Compare October 13, 2016 10:08

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from 3f23825 to 840d91f Compare October 17, 2016 13:30

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from b35d5a7 to 8888534 Compare October 18, 2016 11:30

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch 2 times, most recently from bc81627 to cd42461 Compare October 21, 2016 18:39

vecerek changed the title ~~Toolbar refactoring miq server and region~~ [WIP] Toolbar refactoring miq server and region Oct 24, 2016

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from 59a5a62 to 1297896 Compare October 24, 2016 09:39

vecerek changed the title ~~[WIP] Toolbar refactoring miq server and region~~ Toolbar refactoring miq server and region Oct 24, 2016

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from 1297896 to f56ea33 Compare October 24, 2016 16:57

jzigmund suggested changes Oct 26, 2016

View reviewed changes

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from f56ea33 to 59b6d12 Compare October 26, 2016 18:31

Attila Vecerek added 14 commits October 27, 2016 16:12

Remove unused toolbar buttons (MIQ Server, MIQ Region)

e6515c0

Remove specs for unused toolbar buttons (MIQ Server, MIQ Region)

25651b0

Create class for refresh_workers and remove unnecessary code

d4567ff

Add spec for refresh_workers toolbar button

eef8583

Remove refresh_worker from toolbar_builder

968989e

Add missing final newline

5aea609

Delete empty files

012d005

Add needed record to refresh_workers button class and remove old exam…

287cede

…ples from toolbar_builder_spec

Fix Rubocop issues

9cdc58f

Remove extra occurence of refresh_workers in the toolbar_builder, ext…

92a8755

…end the #visible? method by additional cases and add new spec examples accordingly

Put back the removed occurence of refresh_workers in #toolbar_builder…

33f9781

… to make the button visible again

Fix code style offenses detected by Rubocop

a194d8e

Correct #visible? method so the button gets hidden when the record is…

0f79a50

… miq_server or miq_region only if layout != ops and tailors the spec file accordingly

Adopt the changes suggested by @jzigmund

2797d55

vecerek force-pushed the toolbar_refactoring__miq_server_and_region branch from 59b6d12 to 2797d55 Compare October 27, 2016 14:12

jzigmund approved these changes Nov 2, 2016

View reviewed changes

martinpovolny added the euwe/no label Nov 2, 2016

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

martinpovolny merged commit af903bf into ManageIQ:master Nov 2, 2016

vecerek deleted the toolbar_refactoring__miq_server_and_region branch November 2, 2016 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toolbar refactoring miq server and region #11879

Toolbar refactoring miq server and region #11879

vecerek commented Oct 12, 2016 •

edited

Loading

romanblanco left a comment

martinpovolny Oct 13, 2016

martinpovolny Oct 13, 2016 •

edited

Loading

martinpovolny Oct 13, 2016

vecerek Oct 13, 2016

martinpovolny commented Oct 13, 2016

romanblanco commented Oct 17, 2016

vecerek commented Oct 18, 2016

romanblanco commented Oct 18, 2016 •

edited

Loading

vecerek commented Oct 20, 2016 •

edited

Loading

romanblanco commented Oct 20, 2016 •

edited

Loading

vecerek commented Oct 20, 2016

romanblanco commented Oct 20, 2016

miq-bot commented Oct 20, 2016

vecerek commented Oct 21, 2016

vecerek commented Oct 21, 2016 •

edited

Loading

vecerek commented Oct 24, 2016

jzigmund left a comment

jzigmund Oct 26, 2016

vecerek Nov 2, 2016

miq-bot commented Oct 27, 2016

vecerek commented Nov 2, 2016

jzigmund left a comment

		@@ -0,0 +1,23 @@
		class ApplicationHelper::Button::RefreshWorkers < ApplicationHelper::Button::Basic

Toolbar refactoring miq server and region #11879

Toolbar refactoring miq server and region #11879

Conversation

vecerek commented Oct 12, 2016 • edited Loading

Links

romanblanco left a comment

Choose a reason for hiding this comment

martinpovolny Oct 13, 2016

Choose a reason for hiding this comment

martinpovolny Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

martinpovolny Oct 13, 2016

Choose a reason for hiding this comment

vecerek Oct 13, 2016

Choose a reason for hiding this comment

martinpovolny commented Oct 13, 2016

romanblanco commented Oct 17, 2016

vecerek commented Oct 18, 2016

romanblanco commented Oct 18, 2016 • edited Loading

vecerek commented Oct 20, 2016 • edited Loading

romanblanco commented Oct 20, 2016 • edited Loading

vecerek commented Oct 20, 2016

romanblanco commented Oct 20, 2016

miq-bot commented Oct 20, 2016

vecerek commented Oct 21, 2016

vecerek commented Oct 21, 2016 • edited Loading

vecerek commented Oct 24, 2016

jzigmund left a comment

Choose a reason for hiding this comment

jzigmund Oct 26, 2016

Choose a reason for hiding this comment

vecerek Nov 2, 2016

Choose a reason for hiding this comment

miq-bot commented Oct 27, 2016

vecerek commented Nov 2, 2016

jzigmund left a comment

Choose a reason for hiding this comment

vecerek commented Oct 12, 2016 •

edited

Loading

martinpovolny Oct 13, 2016 •

edited

Loading

romanblanco commented Oct 18, 2016 •

edited

Loading

vecerek commented Oct 20, 2016 •

edited

Loading

romanblanco commented Oct 20, 2016 •

edited

Loading

vecerek commented Oct 21, 2016 •

edited

Loading