Comments on a seemingly "simple" PR

comments.md

My comments on 7388

- s = MiqSearch.find(@sb[:planning][:options][:filter_value])
- s.options ||= {}
- s.options[:userid] = session[:userid]
- s.options[:results_format] = :objects
-vms, attrs = s.search
+ vms = MiqSearch.find(@sb[:planning][:options][:filter_value]).filtered(nil, :userid => current_user)

1. Remove unused variable attrs

s = MiqSearch.find(@sb[:planning][:options][:filter_value])
s.options ||= {}
s.options[:userid] = session[:userid]
s.options[:results_format] = :objects
- vms, attrs = s.search
+ vms, = s.search

2. Use current_userid

module ApplicationController::CurrentUser
  def current_userid
    session[:userid]
  end
end

s = MiqSearch.find(@sb[:planning][:options][:filter_value])
s.options ||= {}
- s.options[:userid] = session[:userid]
+ s.options[:userid] = current_userid
s.options[:results_format] = :objects
vms, = s.search

3. condense call to search

class MiqSearch
  def search(opts = {})
    self.options ||= {}
    Rbac.search(options.merge(:class => db, :filter => filter).merge(opts))
  end
end

s = MiqSearch.find(@sb[:planning][:options][:filter_value])
-s.options ||= {}
-s.options[:userid] = current_userid
-s.options[:results_format] = :objects
+vms, = s.search(:userid => current_userid, :results_format => :objects)

4. pass :targets => class (no-op) to search

class MiqSearch
  def search(opts = {})
    self.options ||= {}
    Rbac.search(options.merge(:class => db, :filter => filter).merge(opts))
  end
end

class Rbac
  # simplified:
  def self.search(options = {})
    klass   = options[:class]
    klass   = klass.to_s.constantize unless klass.kind_of?(Class)
    targets = options[:targets]
    scope   = options.delete(:named_scope)
    if targets.nil?
      scope = apply_scope(klass, scope) # old path
    else # targets is a scope, class, or AASM class (VimPerformanceDaily in particular)
      targets = targets.to_s.constantize if targets.kind_of?(String) || targets.kind_of?(Symbol)
      targets = targets.all if targets < ActiveRecord::Base

      results_format ||= :objects
      scope = apply_scope(targets, scope) # new path (targets == klass.all)
    end
  end
end

For Rbac.search, passing in :target => class is basically a no-op.

s = MiqSearch.find(@sb[:planning][:options][:filter_value])
-vms, = s.search(:userid => current_userid, :results_format => :objects)
+vms, = s.search(:userid => current_userid, :result_format => :object, :target => s.db)

5. call filtered (indirectly Rbac.filtered) instead of search**

class Rbac
  def self.filtered(objects, options = {})
    Rbac.search(options.merge(:targets => objects, :results_format => :objects)).first
  end
end

class MiqSearch
  def search(opts = {})
    self.options ||= {}
    Rbac.search(options.merge(:class => db, :filter => filter).merge(opts))
  end

  def filtered(targets, opts = {})
    self.options ||= {}
    Rbac.filtered(targets, options.merge(:class => db, :filter => filter).merge(opts))
  end
end

s = MiqSearch.find(@sb[:planning][:options][:filter_value])
-vms, = s.search(:userid => current_userid, :result_format => :object, :target => s.db)
+vms, = s.filtered(s.db, :userid => current_userid)

*6. Fix filtered to better support :targets => nil or :targets => klass

class MiqSearch
-  def filtered(targets, opts = {})
+  def filtered(targets = nil, opts = {})
+    targets ||= db
    self.options ||= {}
    Rbac.filtered(targets, options.merge(:class => db, :filter => filter).merge(opts))
  end