Add rbac check to toolbar button for New Host Aggregates create new action #2980

lpichler · 2017-12-07T16:57:06Z

RBAC check to the toolbar button for New Host Aggregates for create new action:
Compute -> Cloud -> Host Aggregates
after

I am going to do similar thing in more screens, so please review and then I will use it in others screen.
What I am doing here:

added rbac check
add disabled message
added specs
added spec module to be able simply use restricted user

cc @skateman

@miq-bot add_label bug, rbac

@miq-bot assign @martinpovolny

Links

https://bugzilla.redhat.com/show_bug.cgi?id=1516229 (around 10 % done by this PR)

lpichler · 2017-12-07T16:57:42Z

@miq-bot add_label gaprindashvili/yes

martinpovolny · 2017-12-07T20:43:00Z

@lpichler : I see no problem with the approach.

skateman

The idea is good, everything else in the comments.

skateman · 2017-12-08T09:59:05Z

app/helpers/application_helper/button/new_host_aggregate.rb

  def disabled?
-    super || ManageIQ::Providers::CloudManager.all.none? { |ems| ems.supports?(:create_host_aggregate) }
+    self[:title] = _("No cloud providers support creating host aggregates.") unless supports_button_action?


No cloud provider supports creating host aggregates.

Also you should use @error_message instead of the self[:title] as in other buttons.

skateman · 2017-12-08T10:01:50Z

app/helpers/application_helper/button/new_host_aggregate.rb

+  def supports_button_action?
+    @supported_button_action ||= begin
+      filtered_providers = Rbac::Filterer.filtered(ManageIQ::Providers::CloudManager)
+      filtered_providers.empty? ? false : filtered_providers.all? { |ems| ems.supports?(:create_host_aggregate) }


Shouldn't this here any? instead of all?, I assume it's enough to have a single provider that supports the thing. Also the ternary operator seems unnecessary to me, is it gaining some extra performance?

skateman · 2017-12-08T10:03:00Z

spec/support/rbac_helper.rb

+        @group = FactoryGirl.create(:miq_group, :tenant => @tenant)
+        @user = FactoryGirl.create(:user, :miq_groups => [@group])
+        login_as
+        @user


Don't want this as login_as @user on a single line?

skateman · 2017-12-08T10:03:45Z

spec/support/rbac_helper.rb

+      end
+    end
+  end
+end


Do you REALLY need the instance variables in this module?

I'm pretty sure let can be used here, maybe with a single let!.

Forgot the keyword: https://relishapp.com/rspec/rspec-core/docs/example-groups/shared-context

skateman · 2017-12-08T10:04:20Z

spec/helpers/application_helper/buttons/new_host_aggregate_spec.rb


-      it_behaves_like 'a disabled button'
+      context 'when provider is not available due to RBAC rules' do


when is unnecessary, can be assumed from the context

but you can also go with due to RBAC rules as the context will be concatenated with the previous one

skateman · 2017-12-08T10:06:51Z

spec/helpers/application_helper/buttons/new_host_aggregate_spec.rb

+    setup_user_with_tenant
+  end
+
+  let!(:provider) { FactoryGirl.create(:ems_openstack, :tenant => @tenant) }


Please don't use the ! here, rather force the provider creation in a before block when necessary.
Also the @tenant seems scary, you should not have instance variables as you can use let...

skateman · 2017-12-08T10:07:28Z

spec/helpers/application_helper/buttons/new_host_aggregate_spec.rb


  describe '#disabled?' do
    subject { button[:title] }

    context 'no provider available' do
-      before { button.calculate_properties }
+      before do
+        provider.destroy


YAGNI if the provider has let only.

skateman · 2017-12-08T10:09:28Z

app/helpers/application_helper/button/new_host_aggregate.rb

  def disabled?
-    super || ManageIQ::Providers::CloudManager.all.none? { |ems| ems.supports?(:create_host_aggregate) }
+    self[:title] = _("No cloud providers support creating host aggregates.") unless supports_button_action?
+    super || !supports_button_action?


If you're using the @error_message, you can have super || @error_message.present? here.

skateman · 2017-12-08T10:10:04Z

spec/helpers/application_helper/buttons/new_host_aggregate_spec.rb

+          button.calculate_properties
+        end
+
+        it_behaves_like 'a disabled button', "No cloud providers support creating host aggregates."


This test is definitely invalid if you're testing against the same error message twice...it should have the error message you've set above.

mzazrivec · 2017-12-08T10:15:48Z

app/helpers/application_helper/button/new_host_aggregate.rb

+  def supports_button_action?
+    @supported_button_action ||= begin
+      filtered_providers = Rbac::Filterer.filtered(ManageIQ::Providers::CloudManager)
+      filtered_providers.empty? ? false : filtered_providers.all? { |ems| ems.supports?(:create_host_aggregate) }


Why is this filtered_providers.all? { |ems| ems.supports?(:create_host_aggregate) } ?

Shouldn't this be filtered_providers.any? { |ems| ems.supports?(:create_host_aggregate) } ?

lpichler · 2017-12-13T13:57:33Z

@skateman @mzazrivec thanks a lot for your feedback, I updated PR according to your suggestions. Please re-review.

skateman · 2017-12-13T18:23:24Z

app/helpers/application_helper/button/new_host_aggregate.rb

@@ -1,5 +1,13 @@
 class ApplicationHelper::Button::NewHostAggregate < ApplicationHelper::Button::ButtonNewDiscover
+  def supports_button_action?
+    @supported_button_action ||= begin


Why do you need this?

yes, it is not needed, I am not calling it twice now.

skateman · 2017-12-13T18:23:53Z

spec/support/rbac_helper.rb

@@ -0,0 +1,31 @@
+module Spec
+  module Support
+    module RBACHelper


You don't need the modules around, just put it into the folder where other shared examples live

skateman · 2017-12-14T16:37:00Z

@lpichler could you please be more specific in your last 2 commit messages? Or squash them with the previous one, thanks.

…essage for button

skateman

miq-bot · 2017-12-15T11:09:10Z

Checked commits lpichler/manageiq-ui-classic@171d5a2~...96cadc8 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

…_create_action Add rbac check to toolbar button for New Host Aggregates create new action (cherry picked from commit 31d234d)

simaishi · 2017-12-18T14:55:39Z

Gaprindashvili backport details:

$ git log -1
commit b30d04f54b03c011d39228e8a4328c0b0d0e6532
Author: Milan Zázrivec <[email protected]>
Date:   Fri Dec 15 13:02:36 2017 +0100

    Merge pull request #2980 from lpichler/add_rbac_checks_to_buttons_for_create_action
    
    Add rbac check to toolbar button for New Host Aggregates create new action
    (cherry picked from commit 31d234da1f0fa625fec67765162494bcda7c4abd)

miq-bot added bug rbac labels Dec 7, 2017

miq-bot assigned martinpovolny Dec 7, 2017

miq-bot added the gaprindashvili/yes label Dec 7, 2017

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch from b648a26 to ed39718 Compare December 7, 2017 17:17

skateman suggested changes Dec 8, 2017

View reviewed changes

mzazrivec reviewed Dec 8, 2017

View reviewed changes

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch from ed39718 to 23d66dd Compare December 13, 2017 13:56

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch from 23d66dd to 1855f58 Compare December 13, 2017 14:13

lpichler closed this Dec 13, 2017

lpichler reopened this Dec 13, 2017

skateman reviewed Dec 13, 2017

View reviewed changes

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch 2 times, most recently from 5540b69 to 8e5fa55 Compare December 14, 2017 16:29

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch 2 times, most recently from 96e0ac9 to 5843135 Compare December 14, 2017 16:59

consider RBAC in enablement of create new button

171d5a2

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch from 5843135 to ff6646d Compare December 15, 2017 11:00

RBAC check is moved to supports_button_action? method and add error m…

96cadc8

…essage for button

lpichler force-pushed the add_rbac_checks_to_buttons_for_create_action branch from ff6646d to 96cadc8 Compare December 15, 2017 11:01

skateman approved these changes Dec 15, 2017

View reviewed changes

mzazrivec added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 15, 2017

mzazrivec merged commit 31d234d into ManageIQ:master Dec 15, 2017

lpichler deleted the add_rbac_checks_to_buttons_for_create_action branch December 15, 2017 12:33

simaishi pushed a commit that referenced this pull request Dec 18, 2017

Merge pull request #2980 from lpichler/add_rbac_checks_to_buttons_for…

b30d04f

…_create_action Add rbac check to toolbar button for New Host Aggregates create new action (cherry picked from commit 31d234d)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rbac check to toolbar button for New Host Aggregates create new action #2980

Add rbac check to toolbar button for New Host Aggregates create new action #2980

lpichler commented Dec 7, 2017

lpichler commented Dec 7, 2017 •

edited

Loading

martinpovolny commented Dec 7, 2017

skateman left a comment

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

skateman Dec 8, 2017

mzazrivec Dec 8, 2017

lpichler commented Dec 13, 2017

skateman Dec 13, 2017

lpichler Dec 14, 2017

skateman Dec 13, 2017

lpichler Dec 14, 2017

skateman commented Dec 14, 2017

skateman left a comment

miq-bot commented Dec 15, 2017

simaishi commented Dec 18, 2017


		it_behaves_like 'a disabled button'
		context 'when provider is not available due to RBAC rules' do

Add rbac check to toolbar button for New Host Aggregates create new action #2980

Add rbac check to toolbar button for New Host Aggregates create new action #2980

Conversation

lpichler commented Dec 7, 2017

Links

lpichler commented Dec 7, 2017 • edited Loading

martinpovolny commented Dec 7, 2017

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

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

lpichler commented Dec 13, 2017

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

skateman commented Dec 14, 2017

skateman left a comment

Choose a reason for hiding this comment

miq-bot commented Dec 15, 2017

simaishi commented Dec 18, 2017

lpichler commented Dec 7, 2017 •

edited

Loading