Skip to content

Fixes #39318 - Allow creating bookmarks for arf_reports controller #610

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

Merged
ofedoren merged 1 commit into from
May 19, 2026
+20 −5
Merged

Fixes #39318 - Allow creating bookmarks for arf_reports controller #610

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 4 additions & 5 deletions app/validators/concerns/foreman_openscap/bookmark_controller_validator_extensions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,10 @@ module ForemanOpenscap
module BookmarkControllerValidatorExtensions
module ClassMethods
def valid_controllers_list
super + ActiveRecord::Base.connection
.tables
.map(&:to_s)
.select { |table| table.start_with? 'foreman_openscap_' }
.map { |table| table.sub('foreman_openscap_', '') }
controllers = super
controllers + controllers
.select { |controller| controller.start_with?('foreman_openscap_', 'foreman_openscap/') }
.map { |controller| controller.sub(%r{^foreman_openscap[/_]}, '') }
Comment on lines +6 to +8
Copy link
Copy Markdown
Member

@ofedoren ofedoren May 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I kinda agree with the copilot, but uniq won't remove the values that are with prefixes.

What I mean is: controllers will have those foreman_openscap[/_] prefixed values and then we add the new stripped values, which makes me wonder what is actually expected and why don't we have that already or at least why do we keep "unexpected" values (if they actually unexpected though). Have we considered a different approach? I mean, the bookmark component sends controller name based on auto_complete_controller_name, but the only place we override it is in templates_controller. Now I wonder if it's something forgotten or we actually want to have valid_controllers_list with super and prepend logic, which tends to have issues over time.

The patch is fine, it fixes the issue, it's just weird to have this way of validation which just confuses the hell out of me right now (especially with Katello's values)

Copy link
Copy Markdown
Contributor Author

@adamruzicka adamruzicka May 18, 2026

Choose a reason for hiding this comment

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

The validator by default looks at db table names and resources through their permissions and collects them in a single big list.

Db table names doesn't work here because arf reports live in the reports table.

Resources through permissions is sort of weird, in this case we're looking at ForemanOpenscap::ArfReport that gets turned by #tableize into foreman_openscap/arf_reports. The arf reports controller itself isn't namespaced, so its name is "just" arf_reports_controller.

An alternative would be namespacing the controller or un-namespacing arf reports.

(especially with Katello's values)

Katello seems to have their own autocomplete controller concern that works with api paths instead of controller names?

Copy link
Copy Markdown
Member

@ofedoren ofedoren May 18, 2026

Choose a reason for hiding this comment

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

The validator by default looks at db table names and resources through their permissions and collects them in a single big list.

Db table names doesn't work here because arf reports live in the reports table.

Resources through permissions is sort of weird, in this case we're looking at ForemanOpenscap::ArfReport that gets turned by #tableize into foreman_openscap/arf_reports. The arf reports controller itself isn't namespaced, so its name is "just" arf_reports_controller.

I get that, it's just we send controller's name from UI, but we "validate" the value though db table names and permissions? That's what I find weird.

Katello seems to have their own autocomplete controller concern that works with api paths instead of controller names?

I'm afraid I didn't check what's going on there, just from high level overview of the patches it gives me creeps how weird is the current state is.

Copy link
Copy Markdown
Contributor Author

@adamruzicka adamruzicka May 18, 2026

Choose a reason for hiding this comment

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

I get that, it's just we send controller's name from UI

Not exactly. We send the resource name in plural (arf_reports, hosts, foreman_tasks_tasks), so the table name/resource checking makes sense. Saving that value in a controller column is weird though.

Copy link
Copy Markdown
Member

@ofedoren ofedoren May 18, 2026

Choose a reason for hiding this comment

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

We send the resource name in plural

But with controller flavor? Why is controller even mentioned then (either in variables or error message)?

Those are mostly rhetorical. It's just seems we're fine with workarounds once again and it pains me a little.

table name/resource checking makes sense

Named as a controller, checked against table or resource. Although those three are not always the same :/

Copy link
Copy Markdown
Contributor Author

@adamruzicka adamruzicka May 19, 2026

Choose a reason for hiding this comment

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

True, true, filed https://projects.theforeman.org/issues/39333 as we won't be able to solve all the problems here.

end
end

Expand Down
16 changes: 16 additions & 0 deletions test/unit/concerns/bookmark_controller_validator_extensions_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
require 'test_plugin_helper'

class BookmarkControllerValidatorExtensionsTest < ActiveSupport::TestCase
# All controllers including AutoCompleteSearch, except ComplianceDashboardController which is broken anyway
Copy link
Copy Markdown
Contributor Author

@adamruzicka adamruzicka May 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The test comment ... says “All controllers including AutoCompleteSearch” but the list is only a subset

Well, not really?

# rg 'include Foreman::Controller::AutoCompleteSearch'
app/controllers/tailoring_files_controller.rb
2:  include Foreman::Controller::AutoCompleteSearch

app/controllers/policies_controller.rb
2:  include Foreman::Controller::AutoCompleteSearch

app/controllers/scap_contents_controller.rb
2:  include Foreman::Controller::AutoCompleteSearch

app/controllers/compliance_dashboard_controller.rb
2:  include Foreman::Controller::AutoCompleteSearch

app/controllers/arf_reports_controller.rb
2:  include Foreman::Controller::AutoCompleteSearch

[ArfReportsController, ScapContentsController,
PoliciesController, TailoringFilesController].each do |controller_class|
test "#{controller_class.controller_name} should be a valid bookmark controller" do
controller = controller_class.controller_name
bookmark = FactoryBot.build_stubbed(:bookmark, :name => "#{controller} bookmark",
:controller => controller,
:query => 'search query',
:public => true)
assert bookmark.valid?, "#{controller} should be a valid bookmark controller, errors: #{bookmark.errors.full_messages}"
end
end
end
Loading