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

Fixes #38024 - Add ordering by virtual columns #10381

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions app/controllers/api/v2/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,20 @@ def render_error(error, options = { })
render options.merge(:template => "api/v2/errors/#{error}",
:layout => 'api/v2/layouts/error_layout')
end

def resource_scope_for_index(...)
virt_column = params[:order]&.split(' ')&.first
select_method = "select_#{virt_column}"
scope = resource_scope(...)

if virt_column.blank? || resource_class.columns_hash[virt_column] || !scope.respond_to?(select_method)
super(...)
else
ordered_scope = scope.send(select_method).search_for(params[:search]).order(params[:order])
return ordered_scope if paginate_options[:per_page] == 'all'
ordered_scope.paginate(**paginate_options)
end
end
end
end
end
18 changes: 17 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,13 @@ def remote_user_provided?
end

def resource_base_with_search
resource_base.search_for(params[:search], :order => params[:order])
virt_column = params[:order]&.split(' ')&.first
select_method = "select_#{virt_column}"
if virt_column.blank? || model_of_controller.columns_hash[virt_column] || !resource_base.respond_to?(select_method)
resource_base.search_for(params[:search], :order => params[:order])
else
resource_base.send(select_method).search_for(params[:search]).order(params[:order])
end
end

def resource_base_search_and_page(tables = [])
Expand Down Expand Up @@ -397,6 +403,16 @@ def parameter_filter_context
Foreman::ParameterFilter::Context.new(:ui, controller_name, params[:action])
end

def wrap_for_virt_column_select(base_scope)
virt_column = params[:order]&.split(' ')&.first
select_method = "select_#{virt_column}"
if virt_column.blank? || model_of_controller.columns_hash[virt_column] || !base_scope.respond_to?(select_method)
base_scope.search_for(params[:search], :order => params[:order]).paginate(:page => params[:page], :per_page => params[:per_page])
else
base_scope.send(select_method).search_for(params[:search]).order(params[:order]).paginate(:page => params[:page], :per_page => params[:per_page])
Copy link
Contributor

Choose a reason for hiding this comment

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

UI does not accept per_page being set to all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, thinking out loud, from the name I wouldn't really expect it to deal with pagination and so on, but that might be just me

Copy link
Member Author

Choose a reason for hiding this comment

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

UI does not accept per_page being set to all?

Nope, otherwise we'd have a button suggesting that.

Also, thinking out loud, from the name I wouldn't really expect it to deal with pagination and so on, but that might be just me

Naming is hard, any suggestion is more than welcome 🍪

Copy link
Member

Choose a reason for hiding this comment

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

Does this break per_page=all for api?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this break per_page=all for api?

wrap_for_virt_column_select method is only for UI controllers, thus no.

API controllers most (if not all) of the time use resource_scope_for_index method, which is overridden in this PR:
https://github.com/theforeman/foreman/pull/10381/files#diff-f97adc72a387ed049f1b789f93f9d5c592f481e9e37c6fb75eabfcc2f36d467eR172

This override should not affect per_page=all, since it's either uses super to use old behavior or explicitly deals with it in https://github.com/theforeman/foreman/pull/10381/files#diff-f97adc72a387ed049f1b789f93f9d5c592f481e9e37c6fb75eabfcc2f36d467eR181.

end
end

class << self
def parameter_filter_context
Foreman::ParameterFilter::Context.new(:ui, controller_name, nil)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class RolesController < ApplicationController

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])
@roles = resource_base_search_and_page
end

def new
Expand Down
1 change: 1 addition & 0 deletions app/models/role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class Role < ApplicationRecord
where("#{compare} builtin = 0")
}
scope :cloned, -> { where.not(:cloned_from_id => nil) }
scope :select_locked, -> { select("roles.*,(origin IS NOT NULL AND builtin <> #{BUILTIN_DEFAULT_ROLE}) as locked") }

validates_lengths_from_database
before_destroy :check_deletable
Expand Down
34 changes: 33 additions & 1 deletion developer_docs/how_to_create_a_plugin.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -1521,7 +1521,7 @@ module ForemanRemoteExecution
# We need to make sure, there's a AR relation we'd be searching on, because the class name can't be determined by the
# association name, it needs to be specified explicitly (as a string). Similarly for the foreign key.
has_one :execution_status_object, :class_name => 'HostStatus::ExecutionStatus', :foreign_key => 'host_id'

# Then we define the searching, the relation is the association name we define on a line above
# :rename key indicates the term user will use to search hosts for a given state, the convention is $feature_status
# :complete_value must be a hash with symbolized keys specifying all possible state values, otherwise the autocompletion
Expand Down Expand Up @@ -2391,6 +2391,38 @@ Foreman Webhooks plugin ships with an example "Remote Execution Host Job" templa

You can find all observable events by calling `Foreman::EventSubscribers.all_observable_events` in the Rails console.

=== How to order by a virtual column
_Requires Foreman 3.14 or higher, set `requires_foreman '>= 3.14'` in
engine.rb_

NOTE: The following assumes usage of:
* `resource_scope_for_index` in the API controller's index action.
* `resource_base_with_search` or `resource_base_search_and_page` in the UI controller's index action.

Starting Foreman 3.14, both UI and API controllers can order by virtual columns.
This is achieved by using the `select_<virtual_column>` scope in the model.
The scope should return the virtual column in the select statement.

[source, ruby]
....
# app/models/model.rb
scope :select_virtual_column, lambda {
select("model_table.*,(column,column2) as virtual_column")
}
....

NOTE: In case your UI controller doesn't use `resource_base_with_search` or `resource_base_search_and_page`,
you can use the `wrap_for_virt_column_select` method in the controller to order by virtual columns.

[source, ruby]
....
# app/controllers/plugin/controller.rb
def index
# Model can be already scoped by other means
@objects = wrap_for_virt_column_select(Model)
end
....

[[translating]]
== Translating

Expand Down
13 changes: 13 additions & 0 deletions test/controllers/api/v2/roles_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,19 @@ class Api::V2::RolesControllerTest < ActionController::TestCase
assert_equal Role.order(:name).pluck(:name), roles['results'].map { |r| r['name'] }
end

test "should order index by locked" do
unlocked_role = FactoryBot.create(:role, :name => "unlocked role", :origin => '', :builtin => 0)
get :index, params: { :order => "locked DESC" }
assert_response :success
roles = ActiveSupport::JSON.decode(@response.body)
assert_equal unlocked_role.id, roles['results'].first['id']

get :index, params: { :order => "locked ASC" }
assert_response :success
roles = ActiveSupport::JSON.decode(@response.body)
assert_not_equal unlocked_role.id, roles['results'].first['id']
end

test "should show individual record" do
get :show, params: { :id => roles(:manager).to_param }
assert_response :success
Expand Down
5 changes: 5 additions & 0 deletions test/controllers/roles_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ class RolesControllerTest < ActionController::TestCase
assert_not_nil Role.find_by_id(roles(:default_role).id)
end

test "should order index by locked" do
get :index, params: { order: "locked DESC" }, session: set_session_user
assert_response :success
end

context "with taxonomies" do
before do
@permission1 = FactoryBot.create(:permission, :domain, :name => 'permission1')
Expand Down
Loading