Fixes #37987 - Filters dont inherit taxonomy on creation #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

ofedoren · 2024-11-15T14:34:58Z

db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb

+    filters = Filter.where(role_id: Role.where(origin: nil).or(Role.where(builtin: 2))).where(override: false).where(taxonomy_search: nil)
+
+    filters.each do |filter|
+      filter.build_taxonomy_search_from_ids


Suggested change

filter.build_taxonomy_search_from_ids

filter.build_taxonomy_search

ofedoren · 2024-11-15T14:35:15Z

test/models/filter_test.rb

+    location_test = FactoryBot.create(:location)
+    f.role = FactoryBot.create(:role, :organizations => [organization_test], :locations => [location_test])
+    f.save
+    assert_equal f.taxonomy_search(), "(organization_id ^ (#{organization_test.id})) and (location_id ^ (#{location_test.id}))"


Suggested change

assert_equal f.taxonomy_search(), "(organization_id ^ (#{organization_test.id})) and (location_id ^ (#{location_test.id}))"

assert_equal f.taxonomy_search, "(organization_id ^ (#{organization_test.id})) and (location_id ^ (#{location_test.id}))"

ofedoren · 2024-11-15T14:41:37Z

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

@@ -29,7 +29,7 @@ const NewFiltersFormPage = ({ location: { search }, history }) => {
          isNew
          roleName={roleName || ''}
          roleId={urlRoleId}
-          data={{}}
+          data={{ locations, organizations }}


Does

data.organizations?.map(o => o.id) || []

mean it would always be [] since taxonomies weren't passed within data before your patch?..

This is the new page, in the edit page we have data={response}

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

thanks @ofedoren, updated

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

Draft until I finish "remove taxonimoes override from filter"

aneta-petrova · 2025-01-06T09:48:15Z

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

+
+      <FormGroup
+        label={__('Locations')}
+        helperText={__('Taxonomies are inherited from the role')}


Instead of "Taxonomies are inherited...", how about adding a line like that after Organizations and Locations each? So: "Organizations are inherited..." and "Locations are inherited ...". It's two lines instead of one but I'm concerned about the term taxonomies: will everyone know that it refers to orgs and locs?

Also, looking at the web UI, due to the layout of the page (indentation) and the formatting, it might look like "Taxonomies are inherited..." relates only to Locations.

Yeah, thats a better way, I'll change it

ofedoren

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

ofedoren · 2025-01-13T13:58:20Z

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

          :unlimited,
-          :override,
          :permissions => [], :permission_ids => [], :permission_names => []
        add_taxonomix_params_filter(filter)


Suggested change

add_taxonomix_params_filter(filter)

along with

include Foreman::Controller::Parameters::Taxonomix

ofedoren · 2025-01-13T14:19:38Z

app/models/filter.rb

@@ -1,7 +1,6 @@
 class Filter < ApplicationRecord
  audited :associated_with => :role

-  include Taxonomix


~~IIUC, we don't make filters non-taxonomable, but rather force them to be in line with the role they're assigned to disabling user override possibility.~~

~~If so, this should stay since this concern makes filter model taxonomable.~~

I think by disabling possibility to override taxonomies on filter level, there is no longer need to have taxonomy association, taxonomy_search can be created on the fly during before_save.

isnt that what before_save :enforce_inherited_taxonomies, do?

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:

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

ofedoren · 2025-01-14T20:05:47Z

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb

@@ -0,0 +1,23 @@
+class RemoveTaxonomyAndOverrideFromFilter < ActiveRecord::Migration[7.0]
+  def up
+    # remove_column :filters, :taxonomy_search


Do we want to remove the column even though we use it?

Probably not? we want to keep using it right?

ofedoren · 2025-01-14T20:42:31Z

db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb

+  def up
+    # remove_column :filters, :taxonomy_search
+    # remove_column :filters, :override
+    # filters = Filter.where(role_id: Role.where(origin: nil).or(Role.where(builtin: 2)))


Out of curiosity: if we drop overriding anyway and force all the filters use inherited taxonomies, shouldn't we update all the filters?

I think this selects only the editable filters

I think this selects only the editable filters

Aren't they all editable?..

Also, since filters were taxonomable and we're removing this, we should probably also clean related leftovers, such as TaxableTaxonomy.where(taxable_type: 'Filter').

Not the locked ones

def locked? return false if modify_locked || self.class.modify_locked return false unless respond_to? :origin origin.present? && builtin != BUILTIN_DEFAULT_ROLE end

But this is for role, not filter?

yes but filters are a part of one role, and if its locked it cant be edited, not the role and not its filters, so it cant have override: true
Or do we just want to run it on the default roles anyway?

My apologies, you're right. It a role is locked, the filters can't be overriden and we also don't allow to lock roles.

I guess we could then search for overriden filters before we remove the column to update their taxonomy search to be based on the assigned role. And after that we could remove the column and the related taxonomy records.

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

aneta-petrova mentioned this pull request Jan 6, 2025

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

Draft

9 tasks

Fixes #37987 - Filters dont inherit taxonomy on creation

ce2c63a

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

aneta-petrova reviewed Jan 13, 2025

View reviewed changes

ofedoren reviewed Jan 14, 2025

View reviewed changes

MariaAga force-pushed the roles-taxonomy-on-create branch 6 times, most recently from b310f1e to 4ea5855 Compare January 30, 2025 12:54

Refs #37987 - remove taxonimoes override from filter

30dd1f9

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37987 - Filters dont inherit taxonomy on creation #10370

Fixes #37987 - Filters dont inherit taxonomy on creation #10370

MariaAga commented Nov 6, 2024

ofedoren Nov 15, 2024

ofedoren Nov 15, 2024

ofedoren Nov 15, 2024

MariaAga Nov 15, 2024

MariaAga commented Nov 20, 2024

MariaAga commented Jan 3, 2025

aneta-petrova Jan 6, 2025

MariaAga Jan 6, 2025

ofedoren left a comment

ofedoren Jan 13, 2025

ofedoren Jan 13, 2025 •

edited

Loading

MariaAga Jan 20, 2025

aneta-petrova Jan 13, 2025

aneta-petrova Jan 13, 2025

MariaAga Jan 20, 2025

ofedoren left a comment •

edited

Loading

ofedoren Jan 14, 2025

MariaAga Jan 20, 2025

ofedoren Jan 14, 2025

MariaAga Jan 20, 2025

ofedoren Jan 24, 2025

MariaAga Jan 24, 2025

ofedoren Jan 24, 2025

MariaAga Jan 29, 2025

ofedoren Feb 4, 2025

	filter.build_taxonomy_search_from_ids
	filter.build_taxonomy_search

	assert_equal f.taxonomy_search(), "(organization_id ^ (#{organization_test.id})) and (location_id ^ (#{location_test.id}))"
	assert_equal f.taxonomy_search, "(organization_id ^ (#{organization_test.id})) and (location_id ^ (#{location_test.id}))"

	"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 dont inherit taxonomy on creation #10370

Are you sure you want to change the base?

Fixes #37987 - Filters dont inherit taxonomy on creation #10370

Conversation

MariaAga commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MariaAga commented Nov 20, 2024

MariaAga commented Jan 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren Jan 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofedoren Jan 13, 2025 •

edited

Loading

ofedoren left a comment •

edited

Loading