Fixes #37987 - Filters remove override, unlimted, and taxonomies #10370

MariaAga · 2024-11-06T15:48:53Z

To test:

Create a role
Assign an organization and a location to the role
Create a filter under the role with host permissions
before the pr: no taxonomy_search in the DB, and the filter is marked as unlimited

Added a migration to regenerate the taxonomy_search field of all filters which have override set to false to make them honor the organizations and locations set on the role.

Added a ui block on the unlimited checkbox if there are taxonomies in place.

Fixed "build_taxonomy_search" as it relayed on locations, but on creation there are only location_ids. Before this pr the workaround would be to edit and submit the filter (with no changes) since on edit locations are already populated

app/models/filter.rb

db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb

test/models/filter_test.rb

webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js

MariaAga · 2024-11-20T11:56:42Z

thanks @ofedoren, updated

MariaAga · 2025-01-03T17:45:47Z

Draft until I finish "remove taxonimoes override from filter"

webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js

ofedoren

There are some comments, you can ignore until the final review (pasting them just so I don't lose them)

app/controllers/concerns/foreman/controller/parameters/filter.rb

app/models/filter.rb

aneta-petrova · 2025-01-13T17:22:48Z

webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js

+                ouiaId="taxonomy-search-alert"
+                variant="info"
+                title={__(
+                  "The filter is scoped to the selected organizations and locations, therefore can't be unlimited"


Not sure if helpful or just nitpicking, but I actually had trouble parsing what the alert meant. Do you think this would be better?

Suggested change

"The filter is scoped to the selected organizations and locations, therefore can't be unlimited"

"You cannot use unlimited filtering because the filter is scoped to the organizations and locations inherited from the role."

I propose s/selected/inherited because 1. I think you don't really "select" orgs/locs anymore and 2. "inherited" is used in the descriptions on lines 122 and 135, I think consistency would help users connect the dots more quickly.

Would it be better to move the info alert below the (greyed-out) unlimited checkbox?

Yeah, for consistency should we change the warning to be a field small warning or an alert?
The 2 options shown here:

@aneta-petrova ?

Sorry, I have a hard time deciding which of the two options would be better. What do you think?

I guess what I feel most strongly about here is the wording -- as suggested above.

ofedoren

Thanks, @MariaAga, LGTM, just few cents inline (consider previous comments as well). Small rubocop issue as well https://github.com/theforeman/foreman/actions/runs/12709621398/job/35429016546?pr=10370

UPD: Reminder for me to remove related stuff from hammer: https://github.com/theforeman/hammer-cli-foreman/blob/master/lib/hammer_cli_foreman/filter.rb#L13

app/models/role.rb

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb

ofedoren

@MariaAga, can this be undrafted after the migration is uncommented or you're planning to do more changes?

Also, while checking something, I've found we have another, but rather small, PF5 related issue here: https://github.com/theforeman/foreman/blob/develop/webpack/assets/javascripts/react_app/components/common/Pf4DualList/DualListWithIds.js#L42

it should be

const onListChange = (_e, newAvailableOptions, newChosenOptions) => {

now

UPD: just noticed, this needs a rebase.

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb

app/models/filter.rb

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb

MariaAga · 2025-04-14T11:23:59Z

Thanks for the reviews, I rebased and addressed the comments

can this be undrafted after the migration is uncommented or you're planning to do more changes?

Yes, migration is just scary

ofedoren

Thanks, @MariaAga, but this seems to have less changes then before, for example all changes from e.g. #10370 (review) are gone. Is that intentional? At very least leaving enforce_override_flag seems against the idea of the PR.

ofedoren · 2025-04-17T13:35:24Z

db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb

@@ -0,0 +1,10 @@
+class RegenerateTaxonomySearchForFiltersWithOverrideFalse < ActiveRecord::Migration[6.1]
+  def change
+    filters = Filter.where(role_id: Role.where(origin: nil).or(Role.where(builtin: 2))).where(override: false).where(taxonomy_search: nil)


where(taxonomy_search: nil)

Was it in the previous iteration of commit? I thought we wanted to explicitly override any overridden searches, this will only add instead thus leaving overridden searches, which seems wrong...

ofedoren

Thanks, @MariaAga and @adamruzicka, I'm afraid I have nothing to add, but one nitpick that can be ignored :)

UPD: I've created a ticket for hammer part: https://projects.theforeman.org/issues/38805. I'll probably pick this myself.

UPD2: Ah... Since we're touching API (and hammer), I'm afraid we might need to touch FAM as well :/ Not that sure though.

app/models/filter.rb

adamruzicka · 2025-10-07T08:12:16Z

UPD2: Ah... Since we're touching API (and hammer), I'm afraid we might need to touch FAM as well :/ Not that sure though.

I already looked. Unless I'm misreading it, the functionality that is being removed was never implemented there so we shouldn't need to do anything.

adamruzicka · 2025-10-16T14:17:10Z

Thank you @MariaAga & @ofedoren !

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

theforeman#10370 removed the method `limited?`, so it shold be replaced here.

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

theforeman#10370 removed the method `limited?`, so it shold be replaced here.

theforeman#10370 removed the method `limited?`, so it should be replaced here.

#10370 removed the method `limited?`, so it should be replaced here.

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

github-actions bot added the UI label Nov 6, 2024

MariaAga force-pushed the roles-taxonomy-on-create branch from baf4134 to f46f2a5 Compare November 6, 2024 16:26

adamruzicka reviewed Nov 7, 2024

View reviewed changes

app/models/filter.rb Outdated Show resolved Hide resolved

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

MariaAga force-pushed the roles-taxonomy-on-create branch 2 times, most recently from 319787a to e2698dc Compare November 7, 2024 14:20

ofedoren reviewed Nov 15, 2024

View reviewed changes

MariaAga force-pushed the roles-taxonomy-on-create branch from e2698dc to 1579997 Compare November 20, 2024 11:56

MariaAga marked this pull request as draft January 3, 2025 17:45

aneta-petrova reviewed Jan 6, 2025

View reviewed changes

webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js Outdated Show resolved Hide resolved

aneta-petrova mentioned this pull request Jan 6, 2025

Remove references to the Override checkbox for filters theforeman/foreman-documentation#3548

Merged

3 tasks

MariaAga force-pushed the roles-taxonomy-on-create branch from 69d381a to c054e59 Compare January 10, 2025 12:35

ofedoren reviewed Jan 13, 2025

View reviewed changes

app/controllers/concerns/foreman/controller/parameters/filter.rb Outdated Show resolved Hide resolved

app/models/filter.rb Show resolved Hide resolved

aneta-petrova reviewed Jan 13, 2025

View reviewed changes

ofedoren reviewed Jan 14, 2025

View reviewed changes

app/models/role.rb Show resolved Hide resolved

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb Outdated Show resolved Hide resolved

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb Outdated Show resolved Hide resolved

MariaAga force-pushed the roles-taxonomy-on-create branch 7 times, most recently from 4ea5855 to 30dd1f9 Compare January 30, 2025 13:40

ofedoren reviewed Apr 4, 2025

View reviewed changes

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb Outdated Show resolved Hide resolved

adamruzicka reviewed Apr 7, 2025

View reviewed changes

MariaAga force-pushed the roles-taxonomy-on-create branch from 30dd1f9 to d4b9075 Compare April 14, 2025 11:22

ofedoren reviewed Apr 17, 2025

View reviewed changes

pr-processor bot added the Waiting on contributor label Oct 3, 2025

MariaAga force-pushed the roles-taxonomy-on-create branch from 5102aff to c6930c8 Compare October 6, 2025 09:07

pr-processor bot added Needs re-review and removed Waiting on contributor Needs re-review labels Oct 6, 2025

MariaAga force-pushed the roles-taxonomy-on-create branch from c6930c8 to a7591dd Compare October 6, 2025 13:35

ofedoren previously approved these changes Oct 6, 2025

View reviewed changes

app/models/filter.rb Outdated Show resolved Hide resolved

Fixes #37987 - Filters remove override, unlimted, and taxonomies

3e5be59

MariaAga dismissed ofedoren’s stale review via 3e5be59 October 6, 2025 15:22

MariaAga force-pushed the roles-taxonomy-on-create branch from a7591dd to 3e5be59 Compare October 6, 2025 15:22

ofedoren approved these changes Oct 14, 2025

View reviewed changes

adamruzicka approved these changes Oct 16, 2025

View reviewed changes

adamruzicka merged commit 8a7e1b1 into theforeman:develop Oct 16, 2025
65 checks passed

adamlazik1 added a commit to adamlazik1/foreman-tasks that referenced this pull request Oct 23, 2025

Fix failing tests

68d61f1

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

adamlazik1 mentioned this pull request Oct 23, 2025

Fix failing tests theforeman/foreman-tasks#784

Merged

adamlazik1 added a commit to adamlazik1/foreman-tasks that referenced this pull request Oct 23, 2025

Fix failing tests

fc83b85

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

adamlazik1 added a commit to adamlazik1/foreman-tasks that referenced this pull request Oct 24, 2025

Fix failing tests

4e4a78b

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

adamlazik1 added a commit to adamlazik1/foreman that referenced this pull request Oct 24, 2025

Replace undefined method

fe54d6a

theforeman#10370 removed the method `limited?`, so it shold be replaced here.

adamlazik1 mentioned this pull request Oct 24, 2025

Refs #37987 - Replace undefined method #10738

Merged

adamruzicka pushed a commit to theforeman/foreman-tasks that referenced this pull request Oct 24, 2025

Fix failing tests

e85eed5

theforeman/foreman#10370 made changes to filters which must be addressed here for tests to pass.

adamlazik1 added a commit to adamlazik1/foreman that referenced this pull request Oct 24, 2025

Refs #37987 - Replace undefined method

6bb0a9f

theforeman#10370 removed the method `limited?`, so it shold be replaced here.

adamlazik1 added a commit to adamlazik1/foreman that referenced this pull request Oct 24, 2025

Refs #37987 - Replace undefined method

24e9d1d

theforeman#10370 removed the method `limited?`, so it should be replaced here.

stejskalleos pushed a commit that referenced this pull request Nov 7, 2025

Refs #37987 - Replace undefined method

d59fd78

#10370 removed the method `limited?`, so it should be replaced here.

Gauravtalreja1 mentioned this pull request Nov 21, 2025

Fix corrupted_roles check: remove reference to missing 'override' column theforeman/foreman_maintain#1064

Merged

	"The filter is scoped to the selected organizations and locations, therefore can't be unlimited"
	"You cannot use unlimited filtering because the filter is scoped to the organizations and locations inherited from the role."

Fixes #37987 - Filters remove override, unlimted, and taxonomies #10370

Fixes #37987 - Filters remove override, unlimted, and taxonomies #10370

Uh oh!

Conversation

MariaAga commented Nov 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MariaAga commented Nov 20, 2024

Uh oh!

MariaAga commented Jan 3, 2025

Uh oh!

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aneta-petrova Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

aneta-petrova Jan 13, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga Jan 20, 2025

Choose a reason for hiding this comment

Uh oh!

MariaAga Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

aneta-petrova Feb 12, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

ofedoren left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MariaAga commented Apr 14, 2025

Uh oh!

ofedoren left a comment

Choose a reason for hiding this comment

Uh oh!

ofedoren Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

ofedoren left a comment • 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!

Uh oh!

adamruzicka commented Oct 7, 2025

Uh oh!

Uh oh!

adamruzicka commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ofedoren left a comment •

edited

Loading

ofedoren left a comment •

edited

Loading

ofedoren left a comment •

edited

Loading