Fixes #37980 - Remove roles locked sort

[MariaAga]

In roles,
sorting by locked gives "unknown attribute 'locked' for Class." or, before foreman 3.5 - "Invalid search query: the field 'locked' in the order statement is not valid field for search"

[adamruzicka]

Wouldn't it be better to fix it instead of removing it, if possible?

[MariaAga]

In theory yes, but Im not sure its possible since it isnt a real field and the search works with:

scoped_search :on => :locked, :ext_method => :search_by_locked, 
...
  def self.search_by_locked(key, operator, value)
    role_condition = "origin IS NOT NULL AND builtin <> #{BUILTIN_DEFAULT_ROLE}"
    if value == 'false'
      role_condition = "NOT (#{role_condition})"
    end
    {:conditions => role_condition}
  end

And I couldnt find anywhere that sorts by something with "ext_method" and works, the other places just dont sort by it, or when theyre broken nothing happens. In roles if you try to sort by locked the page crashes.
Also if no one notices (broken at least since 3.3) I'm not sure its used.

[ofedoren]

theyre broken nothing happens

We have the same "unsortable" fields in openscap plugin:

I don't know if they ever worked though.

Wouldn't it be better to fix it instead of removing it, if possible?

It fully depends on scoped_search library now. I've tried to come up with something, but due to lack of experience with the library it looked too complicated :/ As of now the library expects a field to be a real column to be able to sort/order.

As of now I see 3 possible fixes:

• Remove unsupported orderings (Maria's PR)
• Make scoped_search take any field for ordering (something like ext_method, but to use it for order)
• Finally migrate to React pages, where we could do client based sorting

[adamruzicka]

A fourth option would be to provide a calculated field with select and do the sorting outside of scoped search.

# role.rb
...
  scope :with_locked, -> { select("roles.*,(origin IS NOT NULL AND builtin <> #{BUILTIN_DEFAULT_ROLE}) as locked") }

  default_scope do
    with_locked
  end
...

# roles_controller.rb
...
  def index
    params[:order] ||= 'name'
    @roles = Role.authorized(:view_roles).search_for(params[:search]).order(params[:order]).paginate(:page => params[:page], :per_page => params[:per_page])
  end
...

It would mean going over all the things which are currently unsearchable and do these things for them, which might make sense for some (like duration for tasks), but it feels like too much trouble to be able to sort roles on a boolean field.

With that being said, we have technical means to fix the unsortable columns if we choose to, but in this particular case I'd be fine with dropping it.

[ofedoren]

@adamruzicka, I'm not so confident about changing the default scope, but how about making it "general"? Something like:

# In application_controller.rb
  def wrap_with_virt_column_order(base_scope)
    virt_column = params[:order]&.split(' ')&.first
    if virt_column.blank? || model_of_controller.columns_hash[virt_column]
      base_scope.search_for(params[:search], :order => params[:order]).paginate(:page => params[:page], :per_page => params[:per_page])
    else
      base_scope.send("with_#{virt_column}").search_for(params[:search]).order(params[:order]).paginate(:page => params[:page], :per_page => params[:per_page])
    end
  end

And then for models with virtual columns, such as Role:

# In role.rb
  scope :with_locked, -> { select("roles.*,(origin IS NOT NULL AND builtin <> #{BUILTIN_DEFAULT_ROLE}) as locked") }

# in roles_controller.rb
  def index
    params[:order] ||= 'name'
    @roles = wrap_with_virt_column_order(Role.authorized(:view_roles))
  end

I've check foreman_openscap plugin, there it could be:

# In ArfReport.rb
  scope :with_compliance_failed, lambda { select("reports.*,(#{report_status_column} >> #{bit_mask 'failed'}) as compliance_failed") }

# In arf_reports_controller.rb
  def index
    @arf_reports = wrap_with_virt_column_order(resource_base.includes(:policy, :openscap_proxy, :host => %i[policies last_report_object host_statuses]))
  end

[adamruzicka]

That sounds reasonable. The only change I'd make is that in application_controller.rb, I'd turn the condition to virt_column.blank? || model_of_controller.columns_hash[virt_column] || !base_scope.respond_to?("with_#{virt_column}") so that it doesn't break in new ways until we introduce all the with_* methods.