Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolbar refactoring: Ops #417

Merged
merged 13 commits into from
Feb 24, 2017
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
class ApplicationHelper::Button::RbacCommonFeatureButton < ApplicationHelper::Button::GenericFeatureButton
delegate :rbac_common_feature_for_buttons, :to => :@view_context

def role_allows_feature?
role_allows?(:feature => rbac_common_feature_for_buttons(@feature))
Copy link
Member

@romanblanco romanblanco Feb 23, 2017

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

end

def visible?
true
end
end
9 changes: 2 additions & 7 deletions app/helpers/application_helper/button/tenant_add.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
class ApplicationHelper::Button::TenantAdd < ApplicationHelper::Button::GenericFeatureButton
class ApplicationHelper::Button::TenantAdd < ApplicationHelper::Button::RbacCommonFeatureButton
needs :@record
delegate :role_allows?, :rbac_common_feature_for_buttons, :to => :@view_context

def role_allows_feature?
role_allows?(:feature => rbac_common_feature_for_buttons(self[:child_id]))
end

def visible?
true
[email protected]?
end
end
6 changes: 2 additions & 4 deletions app/helpers/application_helper/button/tenant_edit.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
class ApplicationHelper::Button::TenantEdit < ApplicationHelper::Button::Basic
needs :@record

class ApplicationHelper::Button::TenantEdit < ApplicationHelper::Button::RbacCommonFeatureButton
def disabled?
@record.source
@record.try!(:source)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

end
end
9 changes: 6 additions & 3 deletions app/helpers/application_helper/toolbar/miq_group_center.rb
Original file line number Diff line number Diff line change
@@ -10,14 +10,16 @@ class ApplicationHelper::Toolbar::MiqGroupCenter < ApplicationHelper::Toolbar::B
:rbac_group_edit,
'pficon pficon-edit fa-lg',
t = N_('Edit this Group'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_group_delete,
'pficon pficon-delete fa-lg',
t = N_('Delete this Group'),
t,
:url_parms => "&refresh=y",
:confirm => N_("Are you sure you want to delete this Group?")),
:confirm => N_("Are you sure you want to delete this Group?"),
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -34,7 +36,8 @@ class ApplicationHelper::Toolbar::MiqGroupCenter < ApplicationHelper::Toolbar::B
t = proc do
_('Edit \'%{customer_name}\' Tags for this Group') % {:customer_name => @view_context.session[:customer_name]}
end,
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
15 changes: 10 additions & 5 deletions app/helpers/application_helper/toolbar/miq_groups_center.rb
Original file line number Diff line number Diff line change
@@ -10,15 +10,17 @@ class ApplicationHelper::Toolbar::MiqGroupsCenter < ApplicationHelper::Toolbar::
:rbac_group_add,
'pficon pficon-add-circle-o fa-lg',
t = N_('Add a new Group'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_group_edit,
'pficon pficon-edit fa-lg',
N_('Select a single Group to edit'),
N_('Edit the selected Group'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_group_delete,
'pficon pficon-delete fa-lg',
@@ -27,13 +29,15 @@ class ApplicationHelper::Toolbar::MiqGroupsCenter < ApplicationHelper::Toolbar::
:url_parms => "main_div",
:confirm => N_("Delete all selected Groups?"),
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
separator,
button(
:rbac_group_seq_edit,
'pficon pficon-edit fa-lg',
t = N_('Edit Sequence of User Groups for LDAP Look Up'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -55,7 +59,8 @@ class ApplicationHelper::Toolbar::MiqGroupsCenter < ApplicationHelper::Toolbar::
t,
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
9 changes: 6 additions & 3 deletions app/helpers/application_helper/toolbar/tenant_center.rb
Original file line number Diff line number Diff line change
@@ -32,14 +32,16 @@ class ApplicationHelper::Toolbar::TenantCenter < ApplicationHelper::Toolbar::Bas
:rbac_tenant_manage_quotas,
'pficon pficon-edit fa-lg',
t = N_('Manage Quotas'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_tenant_delete,
'pficon pficon-delete fa-lg',
t = N_('Delete this item'),
t,
:url_parms => "&refresh=y",
:confirm => N_("Are you sure you want to delete this item and all of it's children?")),
:confirm => N_("Are you sure you want to delete this item and all of it's children?"),
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -56,7 +58,8 @@ class ApplicationHelper::Toolbar::TenantCenter < ApplicationHelper::Toolbar::Bas
t = proc do
_('Edit \'%{customer_name}\' Tags for this Tenant') % {:customer_name => @view_context.session[:customer_name]}
end,
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
12 changes: 8 additions & 4 deletions app/helpers/application_helper/toolbar/tenants_center.rb
Original file line number Diff line number Diff line change
@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this class because you can use Basic button.

button(
:rbac_tenant_delete,
'pficon pficon-delete fa-lg',
@@ -22,15 +23,17 @@ class ApplicationHelper::Toolbar::TenantsCenter < ApplicationHelper::Toolbar::Ba
:url_parms => "main_div",
:confirm => N_("Delete all selected items and all of their children?"),
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_tenant_manage_quotas,
'pficon pficon-edit fa-lg',
N_('Select a single item to manage quotas'),
N_('Manage Quotas for the Selected Item'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -52,7 +55,8 @@ class ApplicationHelper::Toolbar::TenantsCenter < ApplicationHelper::Toolbar::Ba
t,
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
12 changes: 8 additions & 4 deletions app/helpers/application_helper/toolbar/user_center.rb
Original file line number Diff line number Diff line change
@@ -10,19 +10,22 @@ class ApplicationHelper::Toolbar::UserCenter < ApplicationHelper::Toolbar::Basic
:rbac_user_edit,
'pficon pficon-edit fa-lg',
t = N_('Edit this User'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_user_copy,
'fa fa-files-o fa-lg',
t = N_('Copy this User to a new User'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_user_delete,
'pficon pficon-delete fa-lg',
t = N_('Delete this User'),
t,
:url_parms => "&refresh=y",
:confirm => N_("Are you sure you want to delete this User?")),
:confirm => N_("Are you sure you want to delete this User?"),
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -39,7 +42,8 @@ class ApplicationHelper::Toolbar::UserCenter < ApplicationHelper::Toolbar::Basic
t = proc do
_('Edit \'%{customer_name}\' Tags for this User') % {:customer_name => @view_context.session[:customer_name]}
end,
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
9 changes: 6 additions & 3 deletions app/helpers/application_helper/toolbar/user_role_center.rb
Original file line number Diff line number Diff line change
@@ -10,19 +10,22 @@ class ApplicationHelper::Toolbar::UserRoleCenter < ApplicationHelper::Toolbar::B
:rbac_role_edit,
'pficon pficon-edit fa-lg',
t = N_('Edit this Role'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_role_copy,
'fa fa-files-o fa-lg',
t = N_('Copy this Role to a new Role'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_role_delete,
'pficon pficon-delete fa-lg',
t = N_('Delete this Role'),
t,
:url_parms => "&refresh=y",
:confirm => N_("Are you sure you want to delete this Role?")),
:confirm => N_("Are you sure you want to delete this Role?"),
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
12 changes: 8 additions & 4 deletions app/helpers/application_helper/toolbar/user_roles_center.rb
Original file line number Diff line number Diff line change
@@ -10,23 +10,26 @@ class ApplicationHelper::Toolbar::UserRolesCenter < ApplicationHelper::Toolbar::
:rbac_role_add,
'pficon pficon-add-circle-o fa-lg',
t = N_('Add a new Role'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_role_edit,
'pficon pficon-edit fa-lg',
N_('Select a single Role to edit'),
N_('Edit the selected Role'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_role_copy,
'fa fa-files-o fa-lg',
N_('Select a single Role to copy'),
N_('Copy the selected Role to a new Role'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_role_delete,
'pficon pficon-delete fa-lg',
@@ -35,7 +38,8 @@ class ApplicationHelper::Toolbar::UserRolesCenter < ApplicationHelper::Toolbar::
:url_parms => "main_div",
:confirm => N_("Delete all selected Roles?"),
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
15 changes: 10 additions & 5 deletions app/helpers/application_helper/toolbar/users_center.rb
Original file line number Diff line number Diff line change
@@ -10,23 +10,26 @@ class ApplicationHelper::Toolbar::UsersCenter < ApplicationHelper::Toolbar::Basi
:rbac_user_add,
'pficon pficon-add-circle-o fa-lg',
t = N_('Add a new User'),
t),
t,
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_user_edit,
'pficon pficon-edit fa-lg',
N_('Select a single User to edit'),
N_('Edit the selected User'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_user_copy,
'fa fa-files-o fa-lg',
N_('Select a single User to copy'),
N_('Copy the selected User to a new User'),
:url_parms => "main_div",
:enabled => false,
:onwhen => "1"),
:onwhen => "1",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
button(
:rbac_user_delete,
'pficon pficon-delete fa-lg',
@@ -35,7 +38,8 @@ class ApplicationHelper::Toolbar::UsersCenter < ApplicationHelper::Toolbar::Basi
:url_parms => "main_div",
:confirm => N_("Delete all selected Users?"),
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
@@ -57,7 +61,8 @@ class ApplicationHelper::Toolbar::UsersCenter < ApplicationHelper::Toolbar::Basi
t,
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::RbacCommonFeatureButton),
]
),
])
2 changes: 0 additions & 2 deletions app/helpers/application_helper/toolbar_builder.rb
Original file line number Diff line number Diff line change
@@ -371,8 +371,6 @@ def hide_button_ops(id)
when :diagnostics_tree
return false
when :rbac_tree
return true unless role_allows?(:feature => rbac_common_feature_for_buttons(id))
return true if %w(rbac_project_add rbac_tenant_add).include?(id) && @record.project?
return false
when :vmdb_tree
return !["db_connections", "db_details", "db_indexes", "db_settings"].include?(@sb[:active_tab])
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
describe ApplicationHelper::Button::RbacCommonFeatureButton do
let(:session) { Hash.new }
let(:view_context) { setup_view_context_with_sandbox({}) }
let(:button) { described_class.new(view_context, {}, {}, {:options => {:feature => feature}}) }

before { login_as FactoryGirl.create(:user, :features => 'rbac_tenant_add') }

describe '#role_allows_feature?' do
subject { button.role_allows_feature? }
%w(rbac_tenant_add rbac_project_add).each do |feature|
context 'when user role allows feature' do
let(:feature) { feature }
it { expect(subject).to be_truthy }
end
end
context 'when user role does not allow feature' do
let(:feature) { 'not_allowed_feature' }
it { expect(subject).to be_falsey }
end
end
end
36 changes: 10 additions & 26 deletions spec/helpers/application_helper/buttons/tenant_add_spec.rb
Original file line number Diff line number Diff line change
@@ -1,32 +1,16 @@
describe ApplicationHelper::Button::TenantAdd do
describe '#role_allows_feature?' do
let(:session) { {} }
before do
MiqProductFeature.seed
feature = MiqProductFeature.find_all_by_identifier("rbac_tenant_add")
@view_context = setup_view_context_with_sandbox({})
@button = tenant_add_button
let(:view_context) { setup_view_context_with_sandbox({}) }
let(:button) { described_class.new(view_context, {}, {'record' => record}, {}) }

role = FactoryGirl.create(:miq_user_role, :miq_product_features => feature)
group = FactoryGirl.create(:miq_group, :miq_user_role => role)
@user = FactoryGirl.create(:user, :miq_groups => [group])
describe '#visible?' do
subject { button.visible? }
context 'when record is a project' do
let(:record) { FactoryGirl.create(:tenant_project) }
it { expect(subject).to be_falsey }
end

def tenant_add_button
described_class.new(@view_context,
{},
{'record' => FactoryGirl.create(:tenant)},
{:options => {:feature => "rbac_project_add"}, :child_id => "rbac_project_add"})
end

it 'returns true for allowed features' do
login_as @user
expect(@button.role_allows_feature?).to be_truthy
end

it 'returns false for disallowed features' do
login_as FactoryGirl.create(:user, :role => 'EvmRole-user')
expect(@button.role_allows_feature?).to be_falsey
context 'when record is not a project' do
let(:record) { FactoryGirl.create(:tenant) }
it { expect(subject).to be_truthy }
end
end
end
28 changes: 0 additions & 28 deletions spec/helpers/application_helper/toolbar_builder_spec.rb
Original file line number Diff line number Diff line change
@@ -633,34 +633,6 @@ def setup_firefox_with_linux

end # end of disable button

describe "#hide_button_ops" do
subject { toolbar_builder.hide_button_ops(@id) }
before do
@record = FactoryGirl.create(:tenant, :parent => Tenant.seed)
feature = EvmSpecHelper.specific_product_features(%w(ops_rbac rbac_group_add rbac_tenant_add rbac_tenant_delete))
login_as FactoryGirl.create(:user, :features => feature)
@sb = {:active_tree => :rbac_tree}
end

%w(rbac_group_add rbac_project_add rbac_tenant_add rbac_tenant_delete).each do |id|
context "when with #{id} button should be visible" do
before { @id = id }
it "and record_id" do
expect(subject).to be_falsey
end
end
end

%w(rbac_group_edit rbac_role_edit).each do |id|
context "when with #{id} button should not be visible as user does not have access to these features" do
before { @id = id }
it "and record_id" do
expect(subject).to be_truthy
end
end
end
end

describe "#get_record_cls" do
subject { toolbar_builder.get_record_cls(record) }
context "when record not exist" do