Skip to content

Fixes #38805 - Remove options related to filter overrides #645

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
Oct 16, 2025
Merged

Fixes #38805 - Remove options related to filter overrides #645

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
74 changes: 8 additions & 66 deletions lib/hammer_cli_foreman/filter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ class ListCommand < HammerCLIForeman::ListCommand
field :id, _("Id")
field :resource_type, _("Resource type")
field :search, _("Search")
field :unlimited?, _("Unlimited?"), Fields::Boolean
field :override?, _("Override?"), Fields::Boolean
field :role, _("Role"), Fields::Reference
field :permissions, _("Permissions"), Fields::List
end
Expand All @@ -27,8 +25,15 @@ def extend_data(filter)

class InfoCommand < HammerCLIForeman::InfoCommand
output ListCommand.output_definition do
HammerCLIForeman::References.taxonomies(self)
Copy link
Member

@ofedoren ofedoren Oct 13, 2025

Choose a reason for hiding this comment

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

This information is still "there". WDYT about including it, but since it's on a different level, we could do it in two ways:

  1. Show at the same level to be "compatible" with the changes:
      from :role do
        collection :locations, _("Locations"), numbered: false, hide_blank: true do
          custom_field Fields::Reference, name_key: :title
        end
        collection :organizations, _("Organizations"), numbered: false, hide_blank: true do
          custom_field Fields::Reference, name_key: :title
        end
      end
  1. Include it at role level, but this would require some additional changes:
      field :role, _("Role"), Fields::Reference, details: [
        { key: :locations, structured_label: _('Locations'), except: :serialized },
        { key: :organizations, structured_label: _('Organizations'), except: :serialized }
      ], display_field_key: :name

and then in lib/hammer_cli_foreman/output/formatters.rb:36:

# ...
# The second check is for `base` adapter, since it's quite limited and this is a quick workaround instead of adding a better handler for nested structures (which I'm not sure we want for `base` though)
next if (detail[:id] && !show_ids) || (required_features & Array(detail[:except])).any?
# ...

Copy link
Contributor Author

@adamruzicka adamruzicka Oct 13, 2025

Choose a reason for hiding this comment

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

"same level" meaning showing it as if it was a attribute of the filter itself?

Copy link
Member

@ofedoren ofedoren Oct 13, 2025

Choose a reason for hiding this comment

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

Yeap. Maybe include in the label (inherited), but that wouldn't as "compatible" then :/

Copy link
Contributor Author

@adamruzicka adamruzicka Oct 14, 2025

Choose a reason for hiding this comment

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

Went with this

# hammer filter info --id 338
Id:                        338
Resource type:             Location
Search:                    none
Role:                      zGptuCUriW
Permissions:               view_locations, assign_locations
Created at:                2025/10/10 09:52:59
Updated at:                2025/10/10 09:52:59
Locations (inherited):
    DVAErp
Organizations (inherited):
    GxRhDkKoNeDD

HammerCLIForeman::References.timestamps(self)
from :role do
collection :locations, _("Locations"), numbered: false, hide_blank: true do
custom_field Fields::Reference, name_key: :title
end
collection :organizations, _("Organizations"), numbered: false, hide_blank: true do
custom_field Fields::Reference, name_key: :title
end
end
end

def extend_data(filter)
Expand All @@ -42,81 +47,18 @@ def extend_data(filter)
end


module TaxonomyCheck

def self.included(base)
def taxonomy_options?
opt_names = ['location_ids', 'organization_ids']
opt_names += resolver.searchables(:locations).map { |s| 'location_' + s.plural_name }
opt_names += resolver.searchables(:organizations).map { |s| 'organization_' + s.plural_name }
opt_names.any? { |opt| send(HammerCLI.option_accessor_name(opt)) }
end

def signal_override_usage_error
signal_usage_error _('Organizations and locations can be set only for overriding filters.')
end

base.extend_help do |base_h|
base_h.section(_('Overriding organizations and locations')) do |section_h|
override_condition = "--override=true"
org_opts = '--organization[s|-ids]'
loc_opts = '--location[s|-ids]'

section_h.text(_("Filters inherit organizations and locations from its role by default. This behavior can be changed by setting %{condition}.%{wsp}" +
"Therefore options %{org_opts} and %{loc_opts} are applicable only when the override flag is set.") % {
:wsp => "\n",
:org_opts => org_opts,
:loc_opts => loc_opts,
:condition => override_condition
})
end
end
end
end


class CreateCommand < HammerCLIForeman::CreateCommand
include TaxonomyCheck

success_message _("Permission filter for [%<resource_type_label>s] created.")
failure_message _("Could not create the permission filter")

def validate_options
signal_override_usage_error if !option_override && taxonomy_options?
end

build_options
end


class UpdateCommand < HammerCLIForeman::UpdateCommand
include TaxonomyCheck

success_message _("Permission filter for [%<resource_type_label>s] updated.")
failure_message _("Could not update the permission filter")

def request_params
params = super
unless override?
# Clear taxonomies in case the filter is switching override from true to false
params['filter']['location_ids'] = [] if !@filter || !@filter['locations'].empty?
params['filter']['organization_ids'] = [] if !@filter || !@filter['organizations'].empty?
end
params
end

def validate_options
signal_override_usage_error if !override? && taxonomy_options?
end

def override?
if option_override.nil?
filter['override?']
else
option_override
end
end

def filter
@filter ||= HammerCLIForeman.foreman_resource!(:filters).action(:show).call({ :id => get_identifier }, request_headers, request_options)
end
Expand Down
127 changes: 3 additions & 124 deletions test/functional/filter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,9 @@

describe 'filter' do
def api_expects_filter_info(options)
override = !!options[:override]
api_expects(:filters, :show, 'Get filter info').with_params(
id: '1'
).returns(@filter.merge('override?' => override))
).returns(@filter)
end

def taxonomy_usage_error(action, cmd)
Expand All @@ -27,15 +26,6 @@ def assert_update_success(result)
@cmd = %w[filter create]
end

it 'prints error when taxonomies are used for a not-overriding filter' do
params = ['--organization-ids=1,2', '--location-ids=3,4', '--override=false']

api_expects_no_call

result = run_cmd(@cmd + params)
assert_cmd(taxonomy_usage_error('create', @cmd), result)
end

it 'should create a filter' do
params = ['--role-id=1', '--permission-ids=[1]']

Expand Down Expand Up @@ -67,8 +57,8 @@ def assert_update_success(result)
api_expects(:filters, :index, 'List filters').returns(@filters)

output = IndexMatcher.new([
['ID', 'RESOURCE TYPE', 'SEARCH', 'UNLIMITED?', 'OVERRIDE?', 'ROLE', 'PERMISSIONS'],
['1', 'Architecture', 'none', 'yes', 'no', 'Manager', 'view_architectures']
['ID', 'RESOURCE TYPE', 'SEARCH', 'ROLE', 'PERMISSIONS'],
['1', 'Architecture', 'none', 'Manager', 'view_architectures']
])
expected_result = success_result(output)

Expand Down Expand Up @@ -100,115 +90,4 @@ def assert_update_success(result)
assert_cmd(success_result("Permission filter deleted.\n"), result)
end
end

describe 'update' do
before do
@cmd = %w[filter update]
@filter = {
'search' => nil,
'resource_type_label' => 'User',
'resource_type' => 'User',
'unlimited?' => false,
'created_at' => '2017-07-18 14:34:09 UTC',
'updated_at' => '2017-07-18 14:34:09 UTC',
'override?' => true,
'id' => 404,
'role' => {
'name' => 'Some Role',
'id' => 28,
'description' => "Description\nof the new\nrole",
'origin' => nil
},
'permissions' => [{
'name' => 'view_users',
'id' => 164,
'resource_type' => 'User'
}],
'locations' => [{
'id' => 28,
'name' => 'location74',
'title' => 'location74',
'description' => nil
}],
'organizations' => [{
'id' => 27,
'name' => 'organization74',
'title' => 'organization74',
'description' => nil
}]
}
end

it 'resets taxonomies when a filter is not-overriding' do
params = ['--id=1']

api_expects_filter_info(override: false)
api_expects(:filters, :update, 'Update the filter').with_params(
'filter' => {
'organization_ids' => [],
'location_ids' => []
}
).returns(@filter)

assert_update_success(run_cmd(@cmd + params))
end

it 'resets taxonomies when switching a filter to not-overriding' do
params = ['--id=1', '--override=false']

api_expects(:filters, :update, 'Update the filter').with_params(
'filter' => {
'organization_ids' => [],
'location_ids' => []
}
).returns(@filter)

assert_update_success(run_cmd(@cmd + params))
end

it 'can add taxonomies when a filter is overriding' do
params = ['--id=1', '--organization-ids=1,2', '--location-ids=3,4']

api_expects_filter_info(override: true)
api_expects(:filters, :update, 'Update the filter').with_params(
'filter' => {
'organization_ids' => %w[1 2],
'location_ids' => %w[3 4]
}
).returns(@filter)

assert_update_success(run_cmd(@cmd + params))
end

it 'can add taxonomies when switching a filter to overriding' do
params = ['--id=1', '--organization-ids=1,2', '--location-ids=3,4', '--override=true']

api_expects(:filters, :update, 'Update the filter').with_params(
'filter' => {
'organization_ids' => %w[1 2],
'location_ids' => %w[3 4]
}
).returns(@filter)

assert_update_success(run_cmd(@cmd + params))
end

it 'prints error when taxonomies are used on not-overriding' do
params = ['--id=1', '--organization-ids=1,2', '--location-ids=3,4']

api_expects_filter_info(override: false)

result = run_cmd(@cmd + params)
assert_cmd(taxonomy_usage_error('update', @cmd), result)
end

it 'prints error when taxonomies are used when switching a filter to not-overriding' do
params = ['--id=1', '--organization-ids=1,2', '--location-ids=3,4', '--override=false']

api_expects_no_call

result = run_cmd(@cmd + params)
assert_cmd(taxonomy_usage_error('update', @cmd), result)
end
end
end
10 changes: 0 additions & 10 deletions test/unit/filter_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,6 @@
describe "CreateCommand" do
let(:cmd) { HammerCLIForeman::Filter::CreateCommand.new("", ctx) }

before do
# FIXME: remove stubbing option_override once tests are switched to apidoc 1.14+
cmd.stubs(:option_override).returns(false)
end

describe "parameters" do
it_should_accept "role id and permission ids", ["--role-id=1", "--permission-ids=1,2"]
it_should_accept "role name and permission ids", ["--role=role", "--permission-ids=1,2"]
Expand All @@ -85,11 +80,6 @@
describe "UpdateCommand" do
let(:cmd) { HammerCLIForeman::Filter::UpdateCommand.new("", ctx) }

before do
# FIXME: remove stubbing option_override once tests are switched to apidoc 1.14+
cmd.stubs(:option_override).returns(false)
end

describe "parameters" do
it_should_accept "id, role id and permission ids", ["--id=1", "--role-id=1", "--permission-ids=1,2"]
it_should_accept "id, role name and permission ids", ["--id=1", "--role=role", "--permission-ids=1,2"]
Expand Down