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.

Sign up for GitHub

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

Ownership refactor #1578

Merged
merged 16 commits into from
Jul 20, 2017
Merged

Ownership refactor #1578

Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d116446
Change RBAC in ownership
PanSpagetka Jun 20, 2017
2d2f690
Refactor ownership methods
PanSpagetka Jun 21, 2017
13a5cc1
Extract button handling code into methods
PanSpagetka Jun 22, 2017
a6833a2
Fix setting ownership from non-explorer screen
PanSpagetka Jun 28, 2017
230aec9
Remove ownership_items variable in set_ownership
PanSpagetka Jun 28, 2017
1ba6504
Rename ownership_items to ownership_ids
PanSpagetka Jun 28, 2017
97a144d
Flip condition
PanSpagetka Jun 28, 2017
1b3a9a8
Set @ownershipitems in build_ownership_info method
PanSpagetka Jun 28, 2017
bf43761
Extract duplicated code for loading instance variables
PanSpagetka Jun 28, 2017
53de303
Dont use lonely operator
PanSpagetka Jul 3, 2017
689562d
Simplify recs assignment
PanSpagetka Jul 11, 2017
d4560da
Create filter_ownership_items method which follows RBAC pattern
PanSpagetka Jul 19, 2017
eef25a2
Use filter_ownership_items method
PanSpagetka Jul 19, 2017
0a95917
Add back check if record can be owned, which was removed in previous …
PanSpagetka Jul 19, 2017
903e62a
Extract assignment before conditions
PanSpagetka Jul 19, 2017
d6ac2d8
Replace find_by_id with find
PanSpagetka Jul 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
206 changes: 104 additions & 102 deletions app/controllers/mixins/actions/vm_actions/ownership.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ def ownership_form_fields
# Set Ownership selected db records
def set_ownership
assert_privileges(params[:pressed])
ownership_items = []
# check to see if coming from show_list or drilled into vms from another CI
controller = if request.parameters[:controller] == "vm" || ["all_vms", "vms", "instances", "images"].include?(params[:display])
"vm"
Expand All @@ -21,28 +20,22 @@ def set_ownership
end
@edit ||= {}
@edit[:controller] = controller
recs = []
if !session[:checked_items].nil? && @lastaction == "set_checked_items"
recs = session[:checked_items]
else
recs = find_checked_ids_with_rbac(get_class_from_controller_param(controller))
end
if recs.blank?
id = find_id_with_rbac(get_class_from_controller_param(params[:controller]), params[:id])
recs = [id.to_i]
end

recs = session[:checked_items] || checked_or_params

@edit[:object_ids] = recs
if recs.length < 1
if recs.empty?
add_flash(_("One or more %{model} must be selected to Set Ownership") % {
:model => Dictionary.gettext(db.to_s, :type => :model, :notfound => :titleize, :plural => true)}, :error)
@refresh_div = "flash_msg_div"
@refresh_partial = "layouts/flash_msg"
return
else
ownership_items = recs.collect(&:to_i)
end

if filter_ownership_items(get_class_from_controller_param(controller), ownership_items).empty?
@origin_ownership_items = recs.collect(&:to_i)
@ownershipitems = filter_ownership_items(get_class_from_controller_param(controller), recs)

if @ownershipitems.empty?
add_flash(_('None of the selected items allow ownership changes'), :error)
@refresh_div = "flash_msg_div"
@refresh_partial = "layouts/flash_msg"
Expand All @@ -51,11 +44,11 @@ def set_ownership

if @explorer
@sb[:explorer] = true
ownership(ownership_items)
ownership(@origin_ownership_items)
else
if role_allows?(:feature => "vm_ownership")
drop_breadcrumb(:name => _("Set Ownership"), :url => "/vm_common/ownership")
javascript_redirect :controller => controller, :action => 'ownership', :rec_ids => ownership_items, :escape => false # redirect to build the ownership screen
javascript_redirect :controller => controller, :action => 'ownership', :rec_ids => @origin_ownership_items, :escape => false # redirect to build the ownership screen
else
head :ok
end
Expand All @@ -81,12 +74,13 @@ def get_class_from_controller_param(controller)
end

# Assign/unassign ownership to a set of objects
def ownership(ownership_items = [])
def ownership(ownership_ids = [])
@sb[:explorer] = true if @explorer
@in_a_form = @ownershipedit = true
drop_breadcrumb(:name => _("Set Ownership"), :url => "/vm_common/ownership")
ownership_items = params[:rec_ids] if params[:rec_ids]
build_ownership_info(ownership_items)
ownership_ids = params[:rec_ids] if params[:rec_ids]
@origin_ownership_items = ownership_ids
Copy link
Member

Choose a reason for hiding this comment

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

do we really need to introduce a new instance variable in refactored code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not new instance variable, I just change the location where it's set. It's used in ownership.html.haml.

Copy link
Member

Choose a reason for hiding this comment

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

I see 👍

build_ownership_info(ownership_ids)
return if @ownershipitems.empty?
build_targets_hash(@ownershipitems)
if @sb[:explorer]
Expand All @@ -96,48 +90,28 @@ def ownership(ownership_items = [])
end
end

def filter_ownership_items(klass, ownership_items)
@origin_ownership_items = ownership_items
@ownershipitems ||= begin
ownership_scope = klass.where(:id => ownership_items)
ownership_scope = ownership_scope.with_ownership if klass.respond_to?(:with_ownership)
Rbac.filtered(ownership_scope.order(:name))
end
end

def build_ownership_info(ownership_items)
def build_ownership_info(ownership_ids)
@edit ||= {}
klass = get_class_from_controller_param(params[:controller])
record = klass.find(ownership_items[0])
user = record.evm_owner if ownership_items.length == 1
@user = user ? user.id.to_s : nil
load_user_group_items(ownership_ids, klass)

@groups = {} # Create new entries hash (2nd pulldown)
# need to do this only if 1 vm is selected and miq_group has been set for it
group = record.miq_group if ownership_items.length == 1
@group = group ? group.id.to_s : nil
Rbac.filtered(MiqGroup.non_tenant_groups).each { |g| @groups[g.description] = g.id.to_s }

@user = @group = 'dont-change' if ownership_items.length > 1
@edit[:object_ids] = filter_ownership_items(klass, ownership_items)
@edit[:object_ids] = ownership_ids
@view = get_db_view(klass == VmOrTemplate ? Vm : klass) # Instantiate the MIQ Report view object
@view.table = MiqFilter.records2table(@ownershipitems, @view.cols + ['id'])
session[:edit] = @edit
end

# Build the ownership assignment screen
def build_ownership_hash(ownership_items)
def build_ownership_hash(ownership_ids)
klass = get_class_from_controller_param(params[:controller])
record = klass.find(ownership_items[0])
user = record.evm_owner if ownership_items.length == 1
@user = user ? user.id.to_s : ''
@ownershipitems ||= filter_ownership_items(klass, ownership_ids)
load_user_group_items(ownership_ids, klass)

@groups = {}
group = record.miq_group if ownership_items.length == 1
@group = group ? group.id.to_s : nil
Rbac.filtered(MiqGroup).each { |g| @groups[g.description] = g.id.to_s }
@user = @group = 'dont-change' if ownership_items.length > 1
@ownershipitems = Rbac.filtered(klass.where(:id => ownership_items).order(:name), :class => klass)
raise _('Invalid items passed') unless @ownershipitems.pluck(:id).to_set == ownership_items.map(&:to_i).to_set
raise _('Invalid items passed') unless @ownershipitems.pluck(:id).to_set == ownership_ids.map(&:to_i).to_set
{:user => @user,
:group => @group}
end
Expand All @@ -150,69 +124,97 @@ def valid_items_for(klass, param_ids)

def ownership_update
case params[:button]
when "cancel"
add_flash(_("Set Ownership was cancelled by the user"))
when "cancel" then ownership_handle_cancel_button
when "save" then ownership_handle_save_button
when "reset" then ownership_handle_reset_button
end
end

private

def load_user_group_items(ownership_ids, klass)
@ownershipitems ||= filter_ownership_items(klass, ownership_ids)
if ownership_ids.length > 1
@user = @group = 'dont-change'
else
record = @ownershipitems.first
@user = record.evm_owner.id.to_s if record.evm_owner
@group = record.miq_group.id.to_s if record.miq_group
end
end

def ownership_handle_cancel_button
add_flash(_("Set Ownership was cancelled by the user"))
if @sb[:explorer]
@edit = @sb[:action] = nil
replace_right_cell
else
session[:flash_msgs] = @flash_array
javascript_redirect previous_breadcrumb_url
end
end

def ownership_handle_save_button
opts = {}
opts[:owner] = unless params[:user] == 'dont-change'
Copy link
Contributor

@himdel himdel Dec 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This breaks multi-target set ownership...

Before, owner or group only got assigned to if the user chose to clear the previously set owner/group.
Now, this is always nil, so it's impossible to change user without resetting groups, and vice versa.

Fixing in #5064

if params[:user].blank? # to clear previously set user
nil
elsif params[:user] != @user
User.find(params[:user])
end
end

opts[:group] = unless params[:group] == 'dont-change'
if params[:group].blank? # to clear previously set group
nil
elsif params[:group] != @group
MiqGroup.find(params[:group])
end
end


klass = get_class_from_controller_param(request.parameters[:controller])
param_ids = params[:objectIds].map(&:to_i)
raise _('Invalid items selected.') unless valid_items_for(klass, param_ids)

result = klass.set_ownership(param_ids, opts)
if result
object_types = object_types_for_flash_message(klass, params[:objectIds])

flash = _("Ownership saved for selected %{object_types}") % {:object_types => object_types}
add_flash(flash)
if @sb[:explorer]
@edit = @sb[:action] = nil
@sb[:action] = nil
replace_right_cell
else
session[:flash_msgs] = @flash_array
javascript_redirect previous_breadcrumb_url
end
when "save"
opts = {}
unless params[:user] == 'dont-change'
if params[:user].blank? # to clear previously set user
opts[:owner] = nil
elsif params[:user] != @user
opts[:owner] = User.find(params[:user])
end
end

unless params[:group] == 'dont-change'
if params[:group].blank? # to clear previously set group
opts[:group] = nil
elsif params[:group] != @group
opts[:group] = MiqGroup.find_by_id(params[:group])
end
end

klass = get_class_from_controller_param(request.parameters[:controller])
param_ids = params[:objectIds].map(&:to_i)
raise _('Invalid items selected.') unless valid_items_for(klass, param_ids)
else
result["missing_ids"].each { |msg| add_flash(msg, :error) } if result["missing_ids"]
result["error_updating"].each { |msg| add_flash(msg, :error) } if result["error_updating"]
javascript_flash
end
end

result = klass.set_ownership(param_ids, opts)
unless result == true
result["missing_ids"].each { |msg| add_flash(msg, :error) } if result["missing_ids"]
result["error_updating"].each { |msg| add_flash(msg, :error) } if result["error_updating"]
javascript_flash
else
object_types = object_types_for_flash_message(klass, params[:objectIds])

flash = _("Ownership saved for selected %{object_types}") % {:object_types => object_types}
add_flash(flash)
if @sb[:explorer]
@sb[:action] = nil
replace_right_cell
else
session[:flash_msgs] = @flash_array
javascript_redirect previous_breadcrumb_url
end
end
when "reset"
@in_a_form = true
if @edit[:explorer]
ownership
add_flash(_("All changes have been reset"), :warning)
request.parameters[:controller] == "service" ? replace_right_cell(:nodetype => "ownership") : replace_right_cell
else
javascript_redirect :action => 'ownership',
:flash_msg => _("All changes have been reset"),
:flash_warning => true,
:escape => true
end
def ownership_handle_reset_button
@in_a_form = true
if @edit[:explorer]
ownership
add_flash(_("All changes have been reset"), :warning)
request.parameters[:controller] == "service" ? replace_right_cell(:nodetype => "ownership") : replace_right_cell
else
javascript_redirect :action => 'ownership',
:flash_msg => _("All changes have been reset"),
:flash_warning => true,
:escape => true
end
end

def filter_ownership_items(klass, ownership_ids)
records = find_records_with_rbac(klass.order(:name), ownership_ids)
records.with_ownership if klass.respond_to?(:with_ownership)
end
end
end
end
Expand Down