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,7 @@
class ApplicationHelper::Button::DiagnosticsAuditLogs < ApplicationHelper::Button::Basic
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_audit_log')
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::DiagnosticsEvmLogs < ApplicationHelper::Button::Basic
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_evm_log')
end
end
7 changes: 7 additions & 0 deletions app/helpers/application_helper/button/diagnostics_logs.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::DiagnosticsLogs < ApplicationHelper::Button::CollectLogs
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_collect_logs')
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::DiagnosticsProductionLogs < ApplicationHelper::Button::Basic
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_production_log')
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::DiagnosticsSummary < ApplicationHelper::Button::Basic
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_summary')
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -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)
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 Why not just

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

end

def visible?
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
module ApplicationHelper::Button::Mixins::ActiveContextMixin
def active_tree?(tree)
if tree.kind_of?(Array)
tree.include?(@view_context.x_active_tree)
else
tree == @view_context.x_active_tree
end
end

def active_tab?(tab)
if tab.kind_of?(Array)
tab.include?(@view_context.active_tab)
else
tab == @view_context.active_tab
end
end
end
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
4 changes: 2 additions & 2 deletions app/helpers/application_helper/button/refresh_workers.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
class ApplicationHelper::Button::RefreshWorkers < ApplicationHelper::Button::Basic
needs :@record, :@sb
include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
@view_context.x_active_tree == :diagnostics_tree && @sb[:active_tab] == 'diagnostics_workers'
active_tree?(:diagnostics_tree) && active_tab?('diagnostics_workers')
end
end
7 changes: 7 additions & 0 deletions app/helpers/application_helper/button/reload_server_tree.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::ReloadServerTree < ApplicationHelper::Button::Basic
Copy link
Author

Choose a reason for hiding this comment

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

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

/cc @romanblanco

Copy link
Author

Choose a reason for hiding this comment

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

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

include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:diagnostics_tree) && active_tab?(%w(diagnostics_roles_servers diagnostics_servers_roles))
end
end
7 changes: 7 additions & 0 deletions app/helpers/application_helper/button/schedule_run_now.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::ScheduleRunNow < ApplicationHelper::Button::ButtonWithoutRbacCheck
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 Hm, i know I've suggested to use ButtonWithoutRbacCheck class, but now, when I'm trying the button, I'm getting back error message:

screenshot from 2017-02-23 15-18-33

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

include ApplicationHelper::Button::Mixins::ActiveContextMixin

def visible?
active_tree?(:settings_tree)
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
class ApplicationHelper::Button::ZoneLogDepotEdit < ApplicationHelper::Button::DiagnosticsLogs
def visible?
return false if active_tree?(:diagnostics_tree) &&
(active_tab?('diagnostics_roles_servers') || active_tab?('diagnostics_servers_roles'))
super
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ class ApplicationHelper::Toolbar::DiagnosticsServerCenter < ApplicationHelper::T
:refresh_server_summary,
'fa fa-repeat fa-lg',
N_('Reload Current Display'),
nil),
nil,
:klass => ApplicationHelper::Button::DiagnosticsSummary),
button(
:refresh_workers,
'fa fa-repeat fa-lg',
Expand All @@ -15,39 +16,45 @@ class ApplicationHelper::Toolbar::DiagnosticsServerCenter < ApplicationHelper::T
:refresh_audit_log,
'fa fa-repeat fa-lg',
N_('Reload the Audit Log Display'),
nil),
nil,
:klass => ApplicationHelper::Button::DiagnosticsAuditLogs),
button(
:fetch_audit_log,
'fa fa-download fa-lg',
N_('Download the Entire Audit Log File'),
nil,
:url => "/fetch_audit_log"),
:url => "/fetch_audit_log",
:klass => ApplicationHelper::Button::DiagnosticsAuditLogs),
button(
:refresh_log,
'fa fa-repeat fa-lg',
N_('Reload the EVM Log Display'),
nil),
nil,
:klass => ApplicationHelper::Button::DiagnosticsEvmLogs),
button(
:fetch_log,
'fa fa-download fa-lg',
N_('Download the Entire EVM Log File'),
nil,
:url => "/fetch_log"),
:url => "/fetch_log",
:klass => ApplicationHelper::Button::DiagnosticsEvmLogs),
button(
:refresh_production_log,
'fa fa-repeat fa-lg',
proc do
_('Reload the %{log_type} Log Display') % {:log_type => _(@sb[:rails_log])}
end,
nil),
nil,
:klass => ApplicationHelper::Button::DiagnosticsProductionLogs),
button(
:fetch_production_log,
'fa fa-download fa-lg',
proc do
_('Download the Entire %{log_type} Log File') % {:log_type => _(@sb[:rails_log])}
end,
nil,
:url => "/fetch_production_log"),
:url => "/fetch_production_log",
:klass => ApplicationHelper::Button::DiagnosticsProductionLogs),
])
button_group('ldap_domain_vmdb', [
select(
Expand All @@ -61,22 +68,23 @@ class ApplicationHelper::Toolbar::DiagnosticsServerCenter < ApplicationHelper::T
'fa fa-filter fa-lg',
N_('Collect the current logs from the selected Server'),
N_('Collect current logs'),
:klass => ApplicationHelper::Button::CollectLogs
:klass => ApplicationHelper::Button::DiagnosticsLogs
),
button(
:collect_logs,
'fa fa-filter fa-lg',
N_('Collect all logs from the selected Server'),
N_('Collect all logs'),
:klass => ApplicationHelper::Button::CollectLogs
:klass => ApplicationHelper::Button::DiagnosticsLogs
),
]
),
button(
:log_depot_edit,
'pficon pficon-edit fa-lg',
N_('Edit the Log Depot settings for the selected Server'),
N_('Edit')),
N_('Edit'),
:klass => ApplicationHelper::Button::DiagnosticsLogs),
select(
:restart_vmdb_choice,
'fa fa-cog fa-lg',
Expand All @@ -88,13 +96,15 @@ class ApplicationHelper::Toolbar::DiagnosticsServerCenter < ApplicationHelper::T
'pficon pficon-restart',
t = N_('Restart server'),
t,
:confirm => N_("Warning: Server will be restarted, do you want to continue?")),
:confirm => N_("Warning: Server will be restarted, do you want to continue?"),
:klass => ApplicationHelper::Button::DiagnosticsSummary),
button(
:restart_workers,
'pficon pficon-restart',
N_('Select a worker to restart'),
N_('Restart selected worker'),
:confirm => N_("Warning: Selected node will be restarted, do you want to continue?")),
:confirm => N_("Warning: Selected node will be restarted, do you want to continue?"),
:klass => ApplicationHelper::Button::RefreshWorkers),
]
),
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,21 +109,22 @@ class ApplicationHelper::Toolbar::DiagnosticsZoneCenter < ApplicationHelper::Too
'fa fa-filter fa-lg',
N_('Collect the current logs from the selected Zone'),
N_('Collect current logs'),
:klass => ApplicationHelper::Button::CollectLogs
:klass => ApplicationHelper::Button::ZoneLogDepotEdit
),
button(
:zone_collect_logs,
'fa fa-filter fa-lg',
N_('Collect all logs from the selected Zone'),
N_('Collect all logs'),
:klass => ApplicationHelper::Button::CollectLogs
:klass => ApplicationHelper::Button::ZoneLogDepotEdit
),
]
),
button(
:zone_log_depot_edit,
'pficon pficon-edit fa-lg',
N_('Edit the Log Depot settings for the selected Zone'),
N_('Edit')),
N_('Edit'),
:klass => ApplicationHelper::Button::ZoneLogDepotEdit),
])
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
Expand Up @@ -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),
]
),
])
Expand All @@ -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),
]
),
])
Expand Down
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
Expand Up @@ -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',
Expand All @@ -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),
]
),
])
Expand All @@ -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),
]
),
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ class ApplicationHelper::Toolbar::MiqScheduleCenter < ApplicationHelper::Toolbar
:schedule_run_now,
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.

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

'collect',
t = N_('Queue up this Schedule to run now'),
t),
t,
:klass => ApplicationHelper::Button::ScheduleRunNow),
]
),
])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ class ApplicationHelper::Toolbar::MiqSchedulesCenter < ApplicationHelper::Toolba
t,
:url_parms => "main_div",
:enabled => false,
:onwhen => "1+"),
:onwhen => "1+",
:klass => ApplicationHelper::Button::ScheduleRunNow),
]
),
])
Expand Down
9 changes: 6 additions & 3 deletions app/helpers/application_helper/toolbar/tenant_center.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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),
]
),
])
Expand All @@ -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),
]
),
])
Expand Down
Loading