Fixes #39318 - Allow creating bookmarks for arf_reports controller#610
Conversation
The bookmark controller validator builds its list of valid controllers from database table names and permission resources. Since ArfReport uses STI (inheriting from ::Report and stored in the reports table), there is no foreman_openscap_arf_reports table. The previous extension only stripped the foreman_openscap_ prefix from table names, missing the foreman_openscap/ prefix that Permission.resources.map(&:tableize) produces for namespaced models like ForemanOpenscap::ArfReport. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes bookmark controller validation for the arf_reports controller by ensuring the validator recognizes controller names derived from namespaced permission resources (e.g. foreman_openscap/arf_reports) and maps them correctly to the un-namespaced controller names used by the UI (e.g. arf_reports).
Changes:
- Update the bookmark controller validator extension to strip both
foreman_openscap_andforeman_openscap/prefixes from entries produced by the core validator. - Add a unit test asserting that key Foreman OpenSCAP controllers (including
arf_reports) can be used to create valid bookmarks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/unit/concerns/bookmark_controller_validator_extensions_test.rb | Adds coverage to ensure bookmarks validate for key OpenSCAP controllers, including arf_reports. |
| app/validators/concerns/foreman_openscap/bookmark_controller_validator_extensions.rb | Extends the valid controller list by adding stripped-prefix variants for foreman_openscap_ and foreman_openscap/ entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| controllers + controllers | ||
| .select { |controller| controller.start_with?('foreman_openscap_', 'foreman_openscap/') } | ||
| .map { |controller| controller.sub(%r{^foreman_openscap[/_]}, '') } |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :/
There was a problem hiding this comment.
True, true, filed https://projects.theforeman.org/issues/39333 as we won't be able to solve all the problems here.
| require 'test_plugin_helper' | ||
|
|
||
| class BookmarkControllerValidatorExtensionsTest < ActiveSupport::TestCase | ||
| # All controllers including AutoCompleteSearch, except ComplianceDashboardController which is broken anyway |
There was a problem hiding this comment.
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
|
Robottelo approves SatelliteQE/robottelo#21539 (comment) |
|
Kinda ACK, just one thought: #610 (comment) Otherwise, if Katello changes land first (and AFAIU we do rely on them), this can be merged as well. |
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @adamruzicka and @pondrejk !
We'll deal with all the weirdness in https://projects.theforeman.org/issues/39333 later.
The bookmark controller validator builds its list of valid controllers from database table names and permission resources. Since
ArfReportuses STI (inheriting from::Reportand stored in the reports table), there is noforeman_openscap_arf_reportstable. The previous extension only stripped theforeman_openscap_prefix from table names, missing theforeman_openscap/prefix thatPermission.resources.map(&:tableize)produces for namespaced models likeForemanOpenscap::ArfReport.Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com