Fixes #38024 - Add ordering by virtual columns #10381

ofedoren · 2024-11-20T15:30:10Z

This is an alternative for #10368, tries to make ordering by columns, which are not part of actual table, possible.

This PR enables it for roles model, making possible to click on locked column in roles index page and see the ordering, as well as doing so via API.

Added an entry for developer_docs saying how to use it within plugins.

Thanks, @adamruzicka, for the idea!

@MariaAga, what do you think?

P.S. The only issue I see is that we'd need to update https://github.com/theforeman/foreman-tasks/blob/master/app/controllers/foreman_tasks/api/tasks_controller.rb#L195 to be compatible. Could be done as part of removal deprecated stuff.

MariaAga · 2024-11-21T11:28:49Z

It seems to work as expected in ui and api, the code makes sense but I think we need a Ruby person to verify it, @adamruzicka ?

adamruzicka

Looks reasonable, left two comments. Could we also get a related patch in one of the plugins where its needed?

adamruzicka · 2024-11-21T14:42:15Z

app/controllers/application_controller.rb

+    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])


UI does not accept per_page being set to all?

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

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 🍪

Does this break per_page=all for api?

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.

ofedoren · 2024-11-29T15:36:41Z

I've updated the logic a bit: since most of the UI controllers use resource_base_search_and_page/resource_base_with_search, I updated this method, so the new wrap_for_virt_column_select method is not necessary, but rather an optional helper. Although, looking at the changes in theforeman/foreman_openscap#587, I don't think this wrapper will be used...

WDYT? Should we expose the wrapper or rather force usage of already defined loaders if ordering by virt columns desired?

ofedoren · 2025-07-31T13:11:24Z

@adamruzicka, @MariaAga, reviving this one: do we still want that?

Note: it also fixes broken ordering by locked on roles page.

Copilot

Pull Request Overview

This PR adds the ability to order by virtual columns in Foreman, specifically enabling ordering by the "locked" column in the roles model. This addresses issue #38024 by allowing users to sort roles by their locked status both in the UI and API.

Implements virtual column ordering support in both UI and API controllers
Adds a select_locked scope to the Role model to define the virtual "locked" column
Updates roles controller to use the new ordering functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
app/models/role.rb	Adds `select_locked` scope to define the virtual locked column
app/controllers/roles_controller.rb	Updates to use `resource_base_search_and_page` for virtual column support
app/controllers/application_controller.rb	Implements core virtual column ordering logic in `resource_base_with_search`
app/controllers/api/v2/base_controller.rb	Adds virtual column ordering support to API controllers
test/controllers/roles_controller_test.rb	Adds test for UI ordering by locked column
test/controllers/api/v2/roles_controller_test.rb	Adds comprehensive tests for API ordering by locked column
developer_docs/how_to_create_a_plugin.asciidoc	Documents how to implement virtual column ordering in plugins

Copilot · 2025-07-31T16:35:21Z

app/controllers/application_controller.rb

+    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


Using send with user-controlled input (select_method derived from params[:order]) could be a security risk. Consider using a whitelist approach or validating the method name against allowed virtual columns.

Suggested change

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

allowed_virtual_columns = %w[column1 column2 column3] # Replace with actual allowed column names

if virt_column.blank? || model_of_controller.columns_hash[virt_column] || !allowed_virtual_columns.include?(virt_column)

resource_base.search_for(params[:search], :order => params[:order])

else

select_method = "select_#{virt_column}"

Applied as 3776040.

app/controllers/api/v2/base_controller.rb

app/controllers/application_controller.rb

app/controllers/api/v2/base_controller.rb

app/controllers/application_controller.rb

adamruzicka · 2025-08-04T08:47:59Z

app/controllers/api/v2/base_controller.rb

+        virt_column = extract_virtual_column
+        select_method = "select_#{virt_column}"
+        scope = resource_scope(...)
+
+        if virt_column.blank? || resource_class.columns_hash[virt_column] || !scope.virtual_column_scopes.include?(select_method.to_sym)


We have this pattern with small differences in three different places, could we make it dryer?

Done. It indeed looks better now.

adamruzicka · 2025-09-30T07:59:11Z

/packit build

ofedoren · 2025-09-30T14:06:47Z

app/controllers/concerns/application_shared.rb

    model = respond_to?(:model_of_controller) ? model_of_controller : resource_class
    return if virt_column.blank? || model.columns_hash[virt_column] || !base_scope.virtual_column_scopes.include?(select_method.to_sym)
-    base_scope.public_send(select_method).search_for(params[:search]).order(params[:order])
+    base_scope.public_send(select_method).search_for(params[:search]).reorder(params[:order])


This already considers unless params[:order].blank? since if it's empty the method returns earlier and the logic returns to the previous (current) state ignoring virtual column.

adamruzicka · 2025-10-02T07:54:27Z

Could you rebase please?

adamruzicka

Tested together with the foreman_openscap PR, works well

adamruzicka · 2025-10-02T12:52:31Z

Could you please squash the commits?

Refs #38024 - Check if select_virt_col is allowed Refs #38024 - Refactor the same logic Refs #38024 - Reorder in case default scope with order

ofedoren · 2025-10-02T12:58:35Z

Thanks, @adamruzicka, rebased / squashed. Although I'd like to see green tests just in case, but I don't think there will be related failures anyway 🤷

ofedoren · 2025-10-02T13:32:09Z

🍏

adamruzicka · 2025-10-02T14:05:51Z

Thank you @ofedoren !

pr-processor bot added the Not yet reviewed label Nov 20, 2024

github-actions bot added the Docs label Nov 20, 2024

adamruzicka reviewed Nov 21, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Nov 21, 2024

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from 56d9b7a to 373036f Compare November 22, 2024 11:51

ofedoren mentioned this pull request Nov 22, 2024

Fixes #38039 - Fix ordering by compliance_[failed|passed|othered] theforeman/foreman_openscap#587

Merged

1 task

ofedoren marked this pull request as draft November 28, 2024 16:38

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from 373036f to 0039511 Compare November 29, 2024 15:29

ofedoren marked this pull request as ready for review November 29, 2024 15:29

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from 0039511 to 4fea540 Compare December 2, 2024 14:12

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from 4fea540 to dfc8198 Compare January 17, 2025 14:33

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from dfc8198 to b0ae94f Compare July 31, 2025 13:03

ofedoren mentioned this pull request Jul 31, 2025

Fixes #38628 - Drop deprecated params for index tasks endpoint theforeman/foreman-tasks#775

Open

adamruzicka requested a review from Copilot July 31, 2025 16:34

Copilot AI reviewed Jul 31, 2025

View reviewed changes

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from b0ae94f to b548dc4 Compare August 1, 2025 13:06

adamruzicka reviewed Aug 4, 2025

View reviewed changes

ofedoren force-pushed the feat-38024-allow-virt-ordering branch 2 times, most recently from 3187f17 to c6ff412 Compare August 5, 2025 14:49

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from c6ff412 to a5c1ede Compare September 25, 2025 11:11

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from a5c1ede to 85e3145 Compare September 30, 2025 14:05

ofedoren commented Sep 30, 2025

View reviewed changes

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from 85e3145 to b8c69f6 Compare October 2, 2025 12:30

adamruzicka approved these changes Oct 2, 2025

View reviewed changes

Fixes #38024 - Add ordering by virtual columns

80dfd83

Refs #38024 - Check if select_virt_col is allowed Refs #38024 - Refactor the same logic Refs #38024 - Reorder in case default scope with order

ofedoren force-pushed the feat-38024-allow-virt-ordering branch from b8c69f6 to 80dfd83 Compare October 2, 2025 12:57

adamruzicka merged commit bf4c6af into theforeman:develop Oct 2, 2025
56 checks passed

Fixes #38024 - Add ordering by virtual columns #10381

Fixes #38024 - Add ordering by virtual columns #10381

Uh oh!

Conversation

ofedoren commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MariaAga commented Nov 21, 2024

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ofedoren commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofedoren commented Jul 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Sep 30, 2025

Uh oh!

ofedoren Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Oct 2, 2025

Uh oh!

adamruzicka left a comment

Choose a reason for hiding this comment

Uh oh!

adamruzicka commented Oct 2, 2025

Uh oh!

ofedoren commented Oct 2, 2025

Uh oh!

ofedoren commented Oct 2, 2025

Uh oh!

Uh oh!

adamruzicka commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ofedoren commented Nov 20, 2024 •

edited

Loading

ofedoren commented Nov 29, 2024 •

edited

Loading

ofedoren Sep 30, 2025 •

edited

Loading