From ce2c63a5839f9011241580d683ad8e86fa784e7f Mon Sep 17 00:00:00 2001 From: MariaAga Date: Thu, 7 Nov 2024 14:07:12 +0000 Subject: [PATCH 1/2] Fixes #37987 - Filters dont inherit taxonomy on creation --- app/models/filter.rb | 12 +++---- ..._search_for_filters_with_override_false.rb | 10 ++++++ test/models/filter_test.rb | 11 ++++++- .../routes/FiltersForm/FiltersForm.js | 31 +++++++++++++++++-- .../routes/FiltersForm/NewFiltersFormPage.js | 4 +-- 5 files changed, 56 insertions(+), 12 deletions(-) create mode 100644 db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb diff --git a/app/models/filter.rb b/app/models/filter.rb index d96ae6e9175..dce04fd49db 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -168,24 +168,24 @@ def inherit_taxonomies! build_taxonomy_search end - private - def build_taxonomy_search - orgs = build_taxonomy_search_string('organization') - locs = build_taxonomy_search_string('location') + orgs = build_taxonomy_search_string_from_ids('organization') + locs = build_taxonomy_search_string_from_ids('location') taxonomies = [orgs, locs].reject { |t| t.blank? } self.taxonomy_search = taxonomies.join(' and ').presence end - def build_taxonomy_search_string(name) + def build_taxonomy_search_string_from_ids(name) return '' unless send("resource_taxable_by_#{name}?") - relation = send(name.pluralize).pluck(:id) + relation = send("#{name}_ids") return '' if relation.empty? parenthesize("#{name}_id ^ (#{relation.join(',')})") end + private + def nilify_empty_searches self.search = nil if search.empty? || unlimited == '1' self.taxonomy_search = nil if taxonomy_search.empty? diff --git a/db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb b/db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb new file mode 100644 index 00000000000..c71d396e3c3 --- /dev/null +++ b/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) + + filters.each do |filter| + filter.build_taxonomy_search + filter.update_column(:taxonomy_search, filter.taxonomy_search) + end + end +end diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index ade9b53f924..1db43826f43 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -245,8 +245,17 @@ class FilterTest < ActiveSupport::TestCase test 'saving nilifies empty taxonomy search' do f = FactoryBot.build(:filter, :resource_type => 'Domain') - f.role = FactoryBot.build(:role, :organizations => [FactoryBot.build(:organization)]) + f.role = FactoryBot.build(:role) f.save assert_nil f.taxonomy_search end + + test 'creating a filter inherits taxonomies from the role' do + f = FactoryBot.create(:filter, :resource_type => 'Domain') + organization_test = FactoryBot.create(:organization) + 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}))" + end end diff --git a/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js b/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js index 765b71a91a2..65c2fa598a4 100644 --- a/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js +++ b/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js @@ -1,4 +1,4 @@ -import React, { useState } from 'react'; +import React, { useState, useEffect, useMemo } from 'react'; import PropTypes from 'prop-types'; import { useDispatch } from 'react-redux'; import { @@ -39,6 +39,17 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { show_organizations: showOrgs = false, show_locations: showLocations = false, } = type; + + const isTaxonomySearch = useMemo( + () => chosenOrgs.length || chosenLocations.length, + [chosenOrgs.length, chosenLocations.length] + ); + + useEffect(() => { + if (isTaxonomySearch) { + setIsUnlimited(false); + } + }, [isTaxonomySearch]); const dispatch = useDispatch(); const [autocompleteQuery, setAutocompleteQuery] = useState(data.search || ''); const submit = async () => { @@ -46,7 +57,8 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { filter: { role_id: role, search: isUnlimited ? null : autocompleteQuery, - unlimited: isUnlimited, + unlimited: + isUnlimited || (!autocompleteQuery.length && !isTaxonomySearch), override: isOverride, permission_ids: chosenPermissions, organization_ids: chosenOrgs, @@ -163,8 +175,20 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { defaultLocations={data.locations} /> )} + {isGranular ? ( <> + {!!isTaxonomySearch && ( + <> + + + )} { > { setAutocompleteQuery(''); setIsUnlimited(checked); }} + isDisabled={isTaxonomySearch} aria-label="is unlimited" id="unlimited-check" name="unlimited" diff --git a/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js b/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js index e059164ba1c..500f952b218 100644 --- a/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js +++ b/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js @@ -9,7 +9,7 @@ import { FiltersForm } from './FiltersForm'; const NewFiltersFormPage = ({ location: { search }, history }) => { const { role_id: urlRoleId } = URI.parseQuery(search); const { - response: { name: roleName, id: roleId }, + response: { name: roleName, id: roleId, locations, organizations }, } = useAPI('get', `/api/v2/roles/${urlRoleId}`); const breadcrumbOptions = { breadcrumbItems: [ @@ -29,7 +29,7 @@ const NewFiltersFormPage = ({ location: { search }, history }) => { isNew roleName={roleName || ''} roleId={urlRoleId} - data={{}} + data={{ locations, organizations }} history={history} /> ) : ( From 30dd1f951efe76a4dd3a572230131b6bbcfc1e0a Mon Sep 17 00:00:00 2001 From: MariaAga Date: Wed, 29 Jan 2025 19:37:27 +0000 Subject: [PATCH 2/2] Refs #37987 - remove taxonimoes override from filter --- app/controllers/api/v2/filters_controller.rb | 3 - .../foreman/controller/parameters/filter.rb | 4 - app/controllers/filters_controller.rb | 15 --- app/controllers/roles_controller.rb | 9 +- app/models/filter.rb | 36 +----- app/models/role.rb | 6 +- app/views/api/v2/filters/main.json.rabl | 5 +- app/views/api/v2/filters/show.json.rabl | 4 - app/views/filters/index.html.erb | 4 - app/views/roles/_form.html.erb | 3 - config/initializers/f_foreman_permissions.rb | 4 +- config/routes.rb | 2 - ..._search_for_filters_with_override_false.rb | 10 -- ...emove_taxonomy_and_override_from_filter.rb | 22 ++++ .../api/v2/filters_controller_test.rb | 110 ----------------- .../api/v2/roles_controller_test.rb | 18 +-- test/controllers/filters_controller_test.rb | 19 --- test/controllers/roles_controller_test.rb | 17 +-- test/helpers/form_helper_test.rb | 4 +- test/models/filter_test.rb | 71 ++++------- test/models/role_test.rb | 38 ------ test/unit/authorizer_test.rb | 4 +- .../routes/FiltersForm/FiltersForm.js | 114 +++++++----------- .../routes/FiltersForm/FiltersForm.test.js | 9 -- .../routes/FiltersForm/NewFiltersFormPage.js | 2 +- .../routes/FiltersForm/Taxonomies.js | 49 -------- 26 files changed, 110 insertions(+), 472 deletions(-) delete mode 100644 db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb create mode 100644 db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb delete mode 100644 webpack/assets/javascripts/react_app/routes/FiltersForm/Taxonomies.js diff --git a/app/controllers/api/v2/filters_controller.rb b/app/controllers/api/v2/filters_controller.rb index 0dc73113c24..1f06348d426 100644 --- a/app/controllers/api/v2/filters_controller.rb +++ b/app/controllers/api/v2/filters_controller.rb @@ -25,10 +25,7 @@ def show param :filter, Hash, :action_aware => true, :required => true do param :role_id, String, :required => true param :search, String - param :override, :bool param :permission_ids, Array - param :organization_ids, Array - param :location_ids, Array end end diff --git a/app/controllers/concerns/foreman/controller/parameters/filter.rb b/app/controllers/concerns/foreman/controller/parameters/filter.rb index c3117bf1a8d..1ec1bb4767f 100644 --- a/app/controllers/concerns/foreman/controller/parameters/filter.rb +++ b/app/controllers/concerns/foreman/controller/parameters/filter.rb @@ -1,6 +1,5 @@ module Foreman::Controller::Parameters::Filter extend ActiveSupport::Concern - include Foreman::Controller::Parameters::Taxonomix class_methods do def filter_params_filter @@ -8,11 +7,8 @@ def filter_params_filter filter.permit :resource_type, :role_id, :role_name, :search, - :taxonomy_search, :unlimited, - :override, :permissions => [], :permission_ids => [], :permission_names => [] - add_taxonomix_params_filter(filter) end end end diff --git a/app/controllers/filters_controller.rb b/app/controllers/filters_controller.rb index 44040ca86a6..d05434a8a15 100644 --- a/app/controllers/filters_controller.rb +++ b/app/controllers/filters_controller.rb @@ -38,23 +38,8 @@ def destroy end end - def disable_overriding - @filter = resource_base.find(params[:id]) - @filter.disable_overriding! - process_success :success_msg => _('Filter overriding has been disabled') - end - private - def action_permission - case params[:action] - when 'disable_overriding' - 'edit' - else - super - end - end - def find_role @role = Role.find_by_id(role_id) end diff --git a/app/controllers/roles_controller.rb b/app/controllers/roles_controller.rb index 744cedea814..9dd53b3e1d3 100644 --- a/app/controllers/roles_controller.rb +++ b/app/controllers/roles_controller.rb @@ -18,7 +18,7 @@ class RolesController < ApplicationController include Foreman::Controller::AutoCompleteSearch include Foreman::Controller::Parameters::Role - before_action :find_resource, :only => [:clone, :edit, :update, :destroy, :disable_filters_overriding] + before_action :find_resource, :only => [:clone, :edit, :update, :destroy] def index params[:order] ||= 'name' @@ -69,19 +69,12 @@ def destroy end end - def disable_filters_overriding - @role.disable_filters_overriding - process_success :success_msg => _('Filters overriding has been disabled') - end - private def action_permission case params[:action] when 'clone' 'view' - when 'disable_filters_overriding' - 'edit' else super end diff --git a/app/models/filter.rb b/app/models/filter.rb index dce04fd49db..c39ddffafa3 100644 --- a/app/models/filter.rb +++ b/app/models/filter.rb @@ -1,7 +1,6 @@ class Filter < ApplicationRecord audited :associated_with => :role - include Taxonomix include Authorizable include TopbarCacheExpiry @@ -42,7 +41,6 @@ def ensure_taxonomies_not_escalated scoped_search :on => :id, :complete_enabled => false, :only_explicit => true, :validator => ScopedSearch::Validators::INTEGER scoped_search :on => :search, :complete_value => true - scoped_search :on => :override, :complete_value => { :true => true, :false => false } scoped_search :on => :limited, :complete_value => { :true => true, :false => false }, :ext_method => :search_by_limited, :only_explicit => true scoped_search :on => :unlimited, :complete_value => { :true => true, :false => false }, :ext_method => :search_by_unlimited, :only_explicit => true scoped_search :relation => :role, :on => :id, :rename => :role_id, :complete_enabled => false, :only_explicit => true, :validator => ScopedSearch::Validators::INTEGER @@ -50,7 +48,7 @@ def ensure_taxonomies_not_escalated scoped_search :relation => :permissions, :on => :resource_type, :rename => :resource scoped_search :relation => :permissions, :on => :name, :rename => :permission - before_validation :build_taxonomy_search, :nilify_empty_searches, :enforce_override_flag + before_validation :nilify_empty_searches before_save :enforce_inherited_taxonomies, :nilify_empty_searches validates :search, :presence => true, :unless => proc { |o| o.search.nil? } @@ -60,7 +58,7 @@ def ensure_taxonomies_not_escalated validate :role_not_locked before_destroy :role_not_locked - validate :same_resource_type_permissions, :not_empty_permissions, :allowed_taxonomies + validate :same_resource_type_permissions, :not_empty_permissions def self.allows_taxonomy_filtering?(_taxonomy) false @@ -153,18 +151,9 @@ def expire_topbar_cache role.usergroups.each { |g| g.expire_topbar_cache } end - def disable_overriding! - self.override = false - save! - end - def enforce_inherited_taxonomies - inherit_taxonomies! unless override? - end - - def inherit_taxonomies! - self.organization_ids = role.organization_ids if resource_taxable_by_organization? - self.location_ids = role.location_ids if resource_taxable_by_location? + @organization_ids = role.organization_ids if resource_taxable_by_organization? + @location_ids = role.location_ids if resource_taxable_by_location? build_taxonomy_search end @@ -178,7 +167,7 @@ def build_taxonomy_search def build_taxonomy_search_string_from_ids(name) return '' unless send("resource_taxable_by_#{name}?") - relation = send("#{name}_ids") + relation = name == "location" ? @location_ids : @organization_ids return '' if relation.empty? parenthesize("#{name}_id ^ (#{relation.join(',')})") @@ -218,21 +207,6 @@ def not_empty_permissions errors.add(:permissions, _('You must select at least one permission')) if permissions.blank? && filterings.blank? end - def allowed_taxonomies - if organization_ids.present? && !resource_taxable_by_organization? - errors.add(:organization_ids, _('You can\'t assign organizations to this resource')) - end - - if location_ids.present? && !resource_taxable_by_location? - errors.add(:location_ids, _('You can\'t assign locations to this resource')) - end - end - - def enforce_override_flag - self.override = false unless resource_taxable? - true - end - def role_not_locked errors.add(:role_id, _('is locked for user modifications.')) if role.locked? && !role.modify_locked errors.empty? diff --git a/app/models/role.rb b/app/models/role.rb index 842c751db3c..da0d56b4ced 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -216,10 +216,6 @@ def remove_permissions!(*args) find_for_permission_removal(args).map(&:destroy!) end - def disable_filters_overriding - filters.where(:override => true).map { |filter| filter.disable_overriding! } - end - def clone(role_params = {}) new_role = deep_clone(:except => [:name, :builtin, :origin], :include => [:locations, :organizations, { :filters => :permissions }]) @@ -286,7 +282,7 @@ def self.override_search_operator(operator) private def sync_inheriting_filters - filters.where(:override => false).find_each do |f| + filters.find_each do |f| unless f.save errors.add :base, N_('One or more of the associated filters are invalid which prevented the role to be saved') raise ActiveRecord::Rollback, N_("Unable to submit role: Problem with associated filter %s") % f.errors diff --git a/app/views/api/v2/filters/main.json.rabl b/app/views/api/v2/filters/main.json.rabl index b1af5a33bd7..cb19170ea9a 100644 --- a/app/views/api/v2/filters/main.json.rabl +++ b/app/views/api/v2/filters/main.json.rabl @@ -2,10 +2,13 @@ object @filter extends "api/v2/filters/base" -attributes :search, :resource_type_label, :unlimited?, :created_at, :updated_at, :override? +attributes :search, :resource_type_label, :unlimited?, :created_at, :updated_at child :role do extends "api/v2/roles/base" + node do |role| + partial("api/v2/taxonomies/children_nodes", :object => role) + end end child :permissions do diff --git a/app/views/api/v2/filters/show.json.rabl b/app/views/api/v2/filters/show.json.rabl index a9e2bbf991d..aea36bb4cc6 100644 --- a/app/views/api/v2/filters/show.json.rabl +++ b/app/views/api/v2/filters/show.json.rabl @@ -1,7 +1,3 @@ object @filter extends "api/v2/filters/main" - -node do |filter| - partial("api/v2/taxonomies/children_nodes", :object => filter) -end diff --git a/app/views/filters/index.html.erb b/app/views/filters/index.html.erb index f79b2f88352..67511d96f21 100644 --- a/app/views/filters/index.html.erb +++ b/app/views/filters/index.html.erb @@ -26,7 +26,6 @@ end %> <%= sort :resource, :as => s_("Filter|Resource"), :permitted => [:role_id] %> <%= s_("Filter|Permissions") %> <%= sort :search, :as => s_("Filter|Unlimited"), :permitted => [:role_id] %> - <%= sort :search, :as => s_("Filter|Override"), :permitted => [:role_id] %> <%= sort :search, :as => s_("Filter|Search"), :permitted => [:role_id] %> <% if @role && !@role.locked? %> <%= _('Actions') %> @@ -47,7 +46,6 @@ end %> <%= filter.permissions.map(&:name).join(', ') %> <%= checked_icon filter.unlimited? %> - <%= checked_icon filter.override? %> <%= content_tag('span', link_to_unless_locked(filter.search || _('N/A'), @role, hash_for_edit_filter_path(:id => filter, :role_id => @role). @@ -58,8 +56,6 @@ end %> <% buttons = [] %> <% buttons.push display_link_if_authorized(_("Edit"), hash_for_edit_filter_path(:id => filter, :role_id => @role). merge(:auth_object => filter, :authorizer => authorizer)) %> - <% buttons.push display_link_if_authorized(_("Disable overriding"), hash_for_disable_overriding_filter_path(:id => filter, :role_id => @role). - merge(:auth_object => filter, :authorizer => authorizer), :method => :patch) if filter.override? %> <% buttons.push display_delete_if_authorized(hash_for_filter_path(:id => filter, :role_id => @role). merge(:auth_object => filter, :authorizer => authorizer), :data => { :confirm => (_("Delete filter?")) } ) %> diff --git a/app/views/roles/_form.html.erb b/app/views/roles/_form.html.erb index d76e2261371..c37991e0780 100644 --- a/app/views/roles/_form.html.erb +++ b/app/views/roles/_form.html.erb @@ -28,7 +28,6 @@ <%= hidden_field_tag :original_role_id, @role.cloned_from_id if @cloned_role %> <% tax_help = N_("When the role's associated %{taxonomies} are changed,
the change will propagate to all inheriting filters. - Filters that are set to override
will remain untouched. Overriding of role filters can be easily disabled by
pressing the \"Disable overriding\" button. Note that not all filters support
%{taxonomies}, so these always remain global.") %> <% if show_location_tab? %> <% loc_help = _(tax_help) % { :taxonomies => _('locations') }%> @@ -52,8 +51,6 @@
<%= link_to_if_authorized(_("New filter"), hash_for_new_filters_path(:role_id => @role), { :class => 'btn btn-success pull-right'} ) %> - <%= link_to_if_authorized(_('Disable all filters overriding'), hash_for_disable_filters_overriding_role_path(:id => @role), - :method => :patch, :class => 'btn btn-default pull-right') %> <% end %> diff --git a/config/initializers/f_foreman_permissions.rb b/config/initializers/f_foreman_permissions.rb index f5bbda18c5b..2ab372bf07e 100644 --- a/config/initializers/f_foreman_permissions.rb +++ b/config/initializers/f_foreman_permissions.rb @@ -192,7 +192,7 @@ :'api/v2/filters' => [:index, :show]} map.permission :create_filters, {:filters => [:new, :create], :'api/v2/filters' => [:create]} - map.permission :edit_filters, {:filters => [:edit, :update, :disable_overriding], :permissions => [:index, :show_resource_types_with_translations], + map.permission :edit_filters, {:filters => [:edit, :update], :permissions => [:index, :show_resource_types_with_translations], :'api/v2/filters' => [:update], :'api/v2/permissions' => [:index, :show, :resource_types]} map.permission :destroy_filters, {:filters => [:destroy], @@ -453,7 +453,7 @@ :'api/v2/roles' => [:index, :show]} map.permission :create_roles, {:roles => [:new, :create, :clone], :'api/v2/roles' => [:create, :clone]} - map.permission :edit_roles, {:roles => [:edit, :update, :disable_filters_overriding], + map.permission :edit_roles, {:roles => [:edit, :update], :'api/v2/roles' => [:update]} map.permission :destroy_roles, {:roles => [:destroy], :'api/v2/roles' => [:destroy]} diff --git a/config/routes.rb b/config/routes.rb index e2723eee05b..178db4c95f3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -263,7 +263,6 @@ resources :roles, except: [:show] do member do get 'clone' - patch 'disable_filters_overriding' end collection do get 'auto_complete_search' @@ -272,7 +271,6 @@ resources :filters, except: [:show, :new, :edit] do member do - patch 'disable_overriding' get 'edit', to: 'react#index' end collection do diff --git a/db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb b/db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb deleted file mode 100644 index c71d396e3c3..00000000000 --- a/db/migrate/20241105143353_regenerate_taxonomy_search_for_filters_with_override_false.rb +++ /dev/null @@ -1,10 +0,0 @@ -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) - - filters.each do |filter| - filter.build_taxonomy_search - filter.update_column(:taxonomy_search, filter.taxonomy_search) - end - end -end diff --git a/db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb b/db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb new file mode 100644 index 00000000000..7099eb884fa --- /dev/null +++ b/db/migrate/20250109165511_remove_taxonomy_and_override_from_filter.rb @@ -0,0 +1,22 @@ +class RemoveTaxonomyAndOverrideFromFilter < ActiveRecord::Migration[7.0] + def up + # remove_column :filters, :override + # filters = Filter.where(role_id: Role.where(origin: nil).or(Role.where(builtin: 2))) + + # filters.each do |filter| + # filter.enforce_inherited_taxonomies + # filter.update_column(:taxonomy_search, filter.taxonomy_search) + # end + end + + def down + # add_column :filters, :taxonomy_search, :text + # add_column :filters, :override, :boolean, :default => false, :null => false + # 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 + # filter.update_column(:taxonomy_search, filter.taxonomy_search) + # end + end +end diff --git a/test/controllers/api/v2/filters_controller_test.rb b/test/controllers/api/v2/filters_controller_test.rb index 64317864c66..b965af51ee8 100644 --- a/test/controllers/api/v2/filters_controller_test.rb +++ b/test/controllers/api/v2/filters_controller_test.rb @@ -39,17 +39,6 @@ class Api::V2::FiltersControllerTest < ActionController::TestCase assert_equal show_response["permissions"].first["name"], "view_architectures" end - test "should create non-overridable filter" do - role = FactoryBot.create(:role, :name => 'New Role') - assert_difference('Filter.count') do - post :create, params: { :filter => { :role_id => role.id, :permission_ids => [permissions(:view_architectures).id] } } - end - assert_response :created - result = JSON.parse(@response.body) - assert_equal false, result['override?'] - assert_equal role.id, result['role']['id'] - end - test "should update filter" do valid_attrs = { :role_id => roles(:destroy_hosts).id, :permission_ids => [permissions(:create_hosts).id] } put :update, params: { :id => filters(:destroy_hosts_1).to_param, :filter => valid_attrs } @@ -63,103 +52,4 @@ class Api::V2::FiltersControllerTest < ActionController::TestCase end assert_response :success end - - context "with organizations and locations" do - before do - @org = taxonomies(:organization1) - @loc = taxonomies(:location1) - end - - test "should create overridable filter" do - filter_loc = taxonomies(:location2) - filter_org = taxonomies(:organization2) - role = FactoryBot.create(:role, :name => 'New Role', :locations => [@loc], :organizations => [@org]) - assert_difference('Filter.count') do - filter_attr = { - :role_id => role.id, - :permission_ids => [permissions(:view_domains).id], - :override => true, - :location_ids => [filter_loc.id], - :organization_ids => [filter_org.id], - } - post :create, params: { :filter => filter_attr } - end - assert_response :created - result = JSON.parse(@response.body) - assert_equal true, result['override?'] - assert_equal role.id, result['role']['id'] - assert_equal filter_org.id, result['organizations'][0]['id'] - assert_equal filter_loc.id, result['locations'][0]['id'] - end - - test "should disable filter override" do - role = FactoryBot.create(:role, :name => 'New Role', :locations => [@loc], :organizations => [@org]) - filter = FactoryBot.create(:filter, - :role_id => role.id, - :permission_ids => [permissions(:view_domains).id], - :override => true, - :locations => [taxonomies(:location2)], - :organizations => [taxonomies(:organization2)] - ) - put :update, params: { :id => filter.to_param, :filter => { :override => false } } - assert_response :success - filter.reload - assert_equal false, filter.override - assert_equal @org, filter.organizations.first - assert_equal @loc, filter.locations.first - end - - test "should create filter without override" do - role = FactoryBot.create(:role, :name => 'New Role', :location_ids => [@loc.id], :organization_ids => [@org.id]) - assert_difference('Filter.count') do - post :create, params: { :filter => { :role_id => role.id, :permission_ids => [permissions(:view_domains).id] } } - end - assert_response :created - result = JSON.parse(@response.body) - refute result['override?'] - assert_equal @org.id, result['organizations'][0]['id'] - assert_equal @loc.id, result['locations'][0]['id'] - end - - test "should not create overridable filter" do - role = FactoryBot.create(:role, :name => 'New Role') - assert_difference('Filter.count', 0) do - filter_attr = { - :role_id => role.id, - :permission_ids => [permissions(:view_architectures).id], - :override => true, - :location_ids => [@loc.id], - :organization_ids => [@org.id], - } - post :create, params: { :filter => filter_attr } - end - assert_response :unprocessable_entity - assert_match "You can't assign organizations to this resource", @response.body - assert_match "You can't assign locations to this resource", @response.body - end - end - - context "with organizations" do - before do - @org = FactoryBot.create(:organization) - end - - test "filter can override taxonomies" do - valid_attrs = { :role_id => roles(:destroy_hosts).id, :permission_ids => [permissions(:view_media).id], :organization_ids => [@org.id], :override => true } - assert_difference('Filter.count') do - post :create, params: { :filter => valid_attrs } - end - assert_response :created - assert assigns(:filter).organizations.include? @org - end - - test "taxonomies are ignored if override is not explicitly enabled" do - valid_attrs = { :role_id => roles(:destroy_hosts).id, :permission_ids => [permissions(:view_domains).id], :organization_ids => [@org.id] } - assert_difference('Filter.count') do - post :create, params: { :filter => valid_attrs } - end - assert_response :created - refute assigns(:filter).organizations.include? @org - end - end end diff --git a/test/controllers/api/v2/roles_controller_test.rb b/test/controllers/api/v2/roles_controller_test.rb index 2f0ff1d84d2..79f689fd8ee 100755 --- a/test/controllers/api/v2/roles_controller_test.rb +++ b/test/controllers/api/v2/roles_controller_test.rb @@ -208,23 +208,7 @@ class Api::V2::RolesControllerTest < ActionController::TestCase assert @org, updated_role.organizations.first assert @loc, updated_role.locations.first updated_filter = Filter.find_by :id => filter.id - assert_equal @org, updated_filter.organizations.first - assert_equal @loc, updated_filter.locations.first - end - - test "should not update overridable filter taxonomies on role taxonomies update" do - role_name = 'New Role' - role = FactoryBot.create(:role, :name => role_name) - filter = FactoryBot.create(:filter, :role_id => role.id, :permission_ids => [permissions(:view_domains).id], :override => true) - new_role_attrs = { :location_ids => [@loc.id], :organization_ids => [@org.id] } - put :update, params: { :id => role.id, :role => new_role_attrs } - assert_response :success - updated_role = Role.find_by :name => role_name - assert @org, updated_role.organizations.first - assert @loc, updated_role.locations.first - updated_filter = Filter.find_by :id => filter.id - assert_empty updated_filter.organizations - assert_empty updated_filter.locations + assert_equal "(organization_id ^ (#{@org.id})) and (location_id ^ (#{@loc.id}))", updated_filter.taxonomy_search end end diff --git a/test/controllers/filters_controller_test.rb b/test/controllers/filters_controller_test.rb index c0830fb3277..e859ae55efa 100644 --- a/test/controllers/filters_controller_test.rb +++ b/test/controllers/filters_controller_test.rb @@ -46,23 +46,4 @@ class FiltersControllerTest < ActionController::TestCase assert_select "table[data-table='inline']" end - - test 'should disable overriding and start inheriting taxonomies from roles' do - permission1 = FactoryBot.create(:permission, :domain, :name => 'permission1') - role = FactoryBot.build(:role, :permissions => []) - role.add_permissions! [permission1.name] - org1 = FactoryBot.create(:organization) - org2 = FactoryBot.create(:organization) - role.organizations = [org1] - role.filters.reload - filter_with_org = role.filters.detect(&:resource_taxable_by_organization?) - filter_with_org.update :organizations => [org1, org2], :override => true - - patch :disable_overriding, params: { :role_id => role.id, :id => filter_with_org.id }, session: set_session_user - - assert_response :redirect - filter_with_org.reload - assert_equal [org1], filter_with_org.organizations - refute filter_with_org.override - end end diff --git a/test/controllers/roles_controller_test.rb b/test/controllers/roles_controller_test.rb index 3b3ef45714f..d945fa1791e 100644 --- a/test/controllers/roles_controller_test.rb +++ b/test/controllers/roles_controller_test.rb @@ -71,24 +71,11 @@ class RolesControllerTest < ActionController::TestCase @role.organizations = [@org1] end - test 'should disable filter overriding' do - @role.filters.reload - @filter_with_org = @role.filters.detect { |f| f.resource_taxable_by_organization? } - @filter_with_org.update :organizations => [@org1, @org2], :override => true - - patch :disable_filters_overriding, params: { :id => @role.id }, session: set_session_user - @filter_with_org.reload - - assert_response :redirect - assert_equal [@org1], @filter_with_org.organizations - refute @filter_with_org.override? - end - test 'update syncs filters taxonomies if configuration changed' do put :update, params: { :id => @role.id, :role => { :organization_ids => ['', @org2.id.to_s, ''] } }, session: set_session_user assert_response :redirect filter = @role.filters.first - assert_equal [@org2], filter.organizations.all + assert_equal "(organization_id ^ (#{@org2.id}))", filter.taxonomy_search end test 'sets new taxonomies to filters after cloning properly' do @@ -99,7 +86,7 @@ class RolesControllerTest < ActionController::TestCase assert_response :redirect filter = Role.find_by_name('clonedrole').filters.first - assert_equal [@org2], filter.organizations.all + assert_equal "(organization_id ^ (#{@org2.id}))", filter.taxonomy_search end end diff --git a/test/helpers/form_helper_test.rb b/test/helpers/form_helper_test.rb index 14f9fcb8555..07d34af7c8e 100644 --- a/test/helpers/form_helper_test.rb +++ b/test/helpers/form_helper_test.rb @@ -101,8 +101,8 @@ class FormHelperTest < ActionView::TestCase test 'multiple_checkboxes produces right output for taxonomy relations' do user = FactoryBot.build_stubbed(:user, :organizations => [taxonomies(:organization1)]) - form_for Filter.new do |f| - assert_match(/input name="filter\[organization_ids\]\[\].*/, + form_for Role.new do |f| + assert_match(/input name="role\[organization_ids\]\[\].*/, multiple_checkboxes(f, :organizations, f.object, user.organizations)) end end diff --git a/test/models/filter_test.rb b/test/models/filter_test.rb index 1db43826f43..0f6439a7850 100644 --- a/test/models/filter_test.rb +++ b/test/models/filter_test.rb @@ -109,12 +109,15 @@ class FilterTest < ActiveSupport::TestCase test 'filter is not automatically scoped to any taxonomies' do original_org, Organization.current = Organization.current, @organization filter = Filter.new - assert_empty filter.organizations + assert_empty filter.taxonomy_search Organization.current = original_org end test "filter with organization set is always limited before validation" do - f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '1', :organization_ids => [@organization.id]) + f = FactoryBot.build(:filter, :search => '', :unlimited => '1') + f.role = FactoryBot.build(:role, :organizations => [@organization]) + f.save + f.enforce_inherited_taxonomies assert f.valid? assert f.limited? assert_include f.taxonomy_search, "(organization_id ^ (#{@organization.id}))" @@ -123,54 +126,48 @@ class FilterTest < ActiveSupport::TestCase end test "filter with location set is always limited before validation" do - f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '1', :location_ids => [@location.id]) + f = FactoryBot.build(:filter, :search => '', :unlimited => '1') + f.role = FactoryBot.build(:role, :locations => [@location]) + f.save + f.enforce_inherited_taxonomies assert f.valid? assert f.limited? assert_include f.taxonomy_search, "(location_id ^ (#{@location.id}))" end test "filter with location set is always limited before validation" do - f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '1', - :organization_ids => [@organization.id, @organization1.id], :location_ids => [@location.id]) + f = FactoryBot.build(:filter, :search => '', :unlimited => '1') + f.role = FactoryBot.build(:role, :locations => [@location], :organizations => [@organization, @organization1]) + f.save + f.enforce_inherited_taxonomies assert f.valid? assert f.limited? assert_equal "(organization_id ^ (#{@organization.id},#{@organization1.id})) and (location_id ^ (#{@location.id}))", f.taxonomy_search end test "removing all organizations and locations from filter nilify taxonomy search" do - f = FactoryBot.create(:filter, :search => '', :unlimited => '1', - :organization_ids => [@organization.id, @organization1.id], :location_ids => [@location.id]) - - f.update :organization_ids => [], :location_ids => [] + f = FactoryBot.create(:filter, :search => '', :unlimited => '1') + f.role = FactoryBot.build(:role, :locations => [@location], :organizations => [@organization, @organization1]) + f.save + f.enforce_inherited_taxonomies + f.role.update :organizations => [], :locations => [] + f.save + f.enforce_inherited_taxonomies assert f.valid? assert f.unlimited? assert_nil f.taxonomy_search end - - test "taxonomies can be assigned only if resource allows it" do - fb = FactoryBot.build_stubbed(:filter, :resource_type => 'Bookmark', :organization_ids => [@organization.id]) - fd = FactoryBot.build_stubbed(:filter, :resource_type => 'Domain', :organization_ids => [@organization.id]) - refute_valid fb - assert_valid fd - fb = FactoryBot.create(:filter, :resource_type => 'Bookmark') - fd = FactoryBot.create(:filter, :resource_type => 'Domain') - fb.location_ids = [@location.id] - refute_valid fb - fd.location_ids = [@location.id] - assert_valid fd - end end test "filter remains unlimited when no organization assigned" do - f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '1', :organization_ids => []) + f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '1') assert f.valid? assert f.unlimited? assert_empty f.taxonomy_search end - test "filter remains set to unlimited when no taxonomy assigned and has empty search" do - f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '0', :organization_ids => [], - :location_ids => []) + test "filter remains set to unlimited when empty search" do + f = FactoryBot.build_stubbed(:filter, :search => '', :unlimited => '0') assert f.valid? assert f.unlimited? assert_empty f.taxonomy_search @@ -215,32 +212,12 @@ class FilterTest < ActiveSupport::TestCase assert_valid f end - test 'disable overriding recalculates taxonomies' do - f = FactoryBot.build(:filter, :resource_type => 'Domain') - f.role = FactoryBot.build(:role, :organizations => [FactoryBot.build(:organization)]) - assert_empty f.organizations - f.disable_overriding! - refute f.override - assert_equal f.organizations, f.role.organizations - end - - test 'enforce_inherited_taxonomies respects override configuration' do - f = FactoryBot.build(:filter, :resource_type => 'Domain', :override => true) - f.role = FactoryBot.build(:role, :organizations => [FactoryBot.build(:organization)]) - f.save # we need ids - f.enforce_inherited_taxonomies - assert_empty f.organizations - f.override = false - f.enforce_inherited_taxonomies - assert_equal f.role.organizations, f.organizations - end - test 'enforce_inherited_taxonomies builds the taxonomy search string' do f = FactoryBot.build(:filter, :resource_type => 'Domain') f.role = FactoryBot.build(:role, :organizations => [FactoryBot.build(:organization)]) f.save # we need ids f.enforce_inherited_taxonomies - assert_equal "(organization_id ^ (#{f.organizations.first.id}))", f.taxonomy_search + assert_equal "(organization_id ^ (#{f.role.organizations.first.id}))", f.taxonomy_search end test 'saving nilifies empty taxonomy search' do diff --git a/test/models/role_test.rb b/test/models/role_test.rb index 98e0db99877..10b17d3d80c 100644 --- a/test/models/role_test.rb +++ b/test/models/role_test.rb @@ -371,21 +371,6 @@ def test_allow_to_modify_roles_when_bypass_locking end describe '#sync_inheriting_filters' do - it 'automatically propagates taxonomies to filters after save' do - @role.organizations = [@org1] - @role.save - @filter_with_org.reload - assert_equal [@org1], @filter_with_org.organizations - end - - it 'automatically propagates taxonomies only to inheriting filters' do - @filter_with_org.update_attribute :override, true - @role.organizations = [@org2] - @role.save - @filter_with_org.reload - assert_empty @filter_with_org.organizations - end - it 'should forced unlimited check if role or filter have no taxonomies' do @role.organizations = [@org1] @role.save @@ -413,33 +398,10 @@ def test_allow_to_modify_roles_when_bypass_locking @role.organizations = [@org1] @role.save @filter_without_org.reload - assert_empty @filter_without_org.organizations - assert_nil @filter_without_org.taxonomy_search - end - - it 'does not touch filters that do not support taxonomies even if they override' do - @filter_without_org.update_attribute :override, true - @role.organizations = [@org1] - @role.save - @filter_without_org.reload - assert_empty @filter_without_org.organizations assert_nil @filter_without_org.taxonomy_search end end - describe '#disable_filters_overriding' do - it 'disables overriding and inherits taxonomies' do - @filter_with_org.update_attribute :override, true - @role.organizations = [@org1] - as_admin do - @role.disable_filters_overriding - @filter_with_org.reload - assert_equal [@org1], @filter_with_org.organizations - refute @filter_with_org.override - end - end - end - describe '#search_by_permission' do setup do @domain_permission = FactoryBot.create(:permission, :domain, :name => 'view_test_domain') diff --git a/test/unit/authorizer_test.rb b/test/unit/authorizer_test.rb index 9e0b87b8bc1..83c729302e3 100644 --- a/test/unit/authorizer_test.rb +++ b/test/unit/authorizer_test.rb @@ -340,7 +340,7 @@ def setup test "#find_collection(Host::Base) works with taxonomies thanks to class name sanitization" do permission = Permission.find_by_name('view_hosts') - FactoryBot.create(:filter, :role => @role, :permissions => [permission], :unlimited => true, :organization_ids => [taxonomies(:organization1).id]) + FactoryBot.create(:filter, :role => @role, :permissions => [permission], :unlimited => true) auth = Authorizer.new(@user) assert_nothing_raised do @@ -353,7 +353,7 @@ def setup test 'allows filtering on associations that do not match association class' do permission = Permission.find_by_name('view_hosts') FactoryBot.create(:host, :debian, :with_facts) - FactoryBot.create(:filter, role: @role, permissions: [permission], search: 'os = Debian', organization_ids: [taxonomies(:organization1).id]) + FactoryBot.create(:filter, role: @role, permissions: [permission], search: 'os = Debian') authorizer = Authorizer.new(@user) # FactValue is referencing HostBase, but operatingsystem association is defined on Host::Managed # this could cause unknown association exception in Rails, we should avoid it diff --git a/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js b/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js index 65c2fa598a4..2f313342794 100644 --- a/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js +++ b/webpack/assets/javascripts/react_app/routes/FiltersForm/FiltersForm.js @@ -9,6 +9,7 @@ import { Popover, ActionGroup, Button, + TextInput, } from '@patternfly/react-core'; import { HelpIcon } from '@patternfly/react-icons'; import { translate as __ } from '../../common/I18n'; @@ -18,7 +19,6 @@ import { SelectPermissions } from './SelectPermissions'; import { SelectResourceType } from './SelectResourceType'; import { SelectRole } from './SelectRole'; import { EMPTY_RESOURCE_TYPE, SEARCH_ID } from './FiltersFormConstants'; -import { Taxonomies } from './Taxonomies'; import { APIActions } from '../../redux/API'; import { addToast } from '../../components/ToastsList'; @@ -27,18 +27,9 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { const [type, setType] = useState(EMPTY_RESOURCE_TYPE); const [chosenPermissions, setChosenPermissions] = useState([]); const [isUnlimited, setIsUnlimited] = useState(!!data['unlimited?']); - const [isOverride, setIsOverride] = useState(!!data['override?']); const [isGranular, setIsGranular] = useState(false); - const [chosenOrgs, setChosenOrgs] = useState( - data.organizations?.map(o => o.id) || [] - ); - const [chosenLocations, setChosenLocations] = useState( - data.locations?.map(l => l.id) || [] - ); - const { - show_organizations: showOrgs = false, - show_locations: showLocations = false, - } = type; + const chosenOrgs = data?.role?.organizations?.map(o => o.name) || []; + const chosenLocations = data?.role?.locations?.map(l => l.name) || []; const isTaxonomySearch = useMemo( () => chosenOrgs.length || chosenLocations.length, @@ -59,10 +50,7 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { search: isUnlimited ? null : autocompleteQuery, unlimited: isUnlimited || (!autocompleteQuery.length && !isTaxonomySearch), - override: isOverride, permission_ids: chosenPermissions, - organization_ids: chosenOrgs, - location_ids: chosenLocations, }, }; const apiOptions = { @@ -129,68 +117,41 @@ export const FiltersForm = ({ roleName, roleId, isNew, data, history }) => { defaultPermissions={data.permissions} setChosenPermissions={setChosenPermissions} /> - {(showOrgs || showLocations) && ( - - {__( - 'Filters inherit organizations and locations associated with the role by default. If override field is enabled, the filter can override the set of its organizations and locations. Later role changes will not affect such filter.After disabling the override field, the role organizations and locations apply again.' - )} - - } - > - - - } - > - { - setIsOverride(checked); - }} - aria-label="is override" - id="override-check" - name="override" - /> - - )} - {isOverride && ( - + - )} + + + + {isGranular ? ( <> - {!!isTaxonomySearch && ( - <> - - - )} { name="unlimited" /> + {!!isTaxonomySearch && ( + <> + + + )} { expect(screen.queryAllByText('access_dashboard')).toHaveLength(0); expect(screen.queryAllByText('view_hosts')).toHaveLength(1); expect(screen.queryAllByDisplayValue('os = CentOS')).toHaveLength(1); - expect(screen.queryAllByText('Override?')).toHaveLength(1); expect(screen.queryAllByText('Unlimited?')).toHaveLength(1); expect(screen.queryAllByText('0 of 7 items selected')).toHaveLength(1); // unselected permissions expect(screen.queryAllByText('0 of 2 items selected')).toHaveLength(1); // selected permissions @@ -231,7 +229,6 @@ describe('FiltersForm', () => { ); expect(screen.queryAllByText('access_dashboard')).toHaveLength(1); expect(screen.queryAllByText('view_hosts')).toHaveLength(0); - expect(screen.queryAllByText('Override?')).toHaveLength(0); expect(screen.queryAllByText('Unlimited?')).toHaveLength(0); act(() => { fireEvent.click(screen.getByLabelText('resource type toggle')); @@ -242,7 +239,6 @@ describe('FiltersForm', () => { expect(screen.queryAllByText('access_dashboard')).toHaveLength(0); expect(screen.queryAllByText('view_hosts')).toHaveLength(1); - expect(screen.queryAllByText('Override?')).toHaveLength(1); expect(screen.queryAllByText('Unlimited?')).toHaveLength(1); expect(screen.getByPlaceholderText('Search')).not.toBeDisabled(); act(() => { @@ -250,11 +246,6 @@ describe('FiltersForm', () => { }); expect(screen.getByPlaceholderText('Search')).toBeDisabled(); - expect(screen.queryAllByText('Organizations')).toHaveLength(0); - expect(screen.queryAllByText('Locations')).toHaveLength(0); - await act(async() => { - fireEvent.click(screen.getByLabelText('is override')); - }); expect(screen.queryAllByText('Organizations')).toHaveLength(1); expect(screen.queryAllByText('Locations')).toHaveLength(1); }); diff --git a/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js b/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js index 500f952b218..9c936889629 100644 --- a/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js +++ b/webpack/assets/javascripts/react_app/routes/FiltersForm/NewFiltersFormPage.js @@ -29,7 +29,7 @@ const NewFiltersFormPage = ({ location: { search }, history }) => { isNew roleName={roleName || ''} roleId={urlRoleId} - data={{ locations, organizations }} + data={{ role: { locations, organizations } }} history={history} /> ) : ( diff --git a/webpack/assets/javascripts/react_app/routes/FiltersForm/Taxonomies.js b/webpack/assets/javascripts/react_app/routes/FiltersForm/Taxonomies.js deleted file mode 100644 index b813242a894..00000000000 --- a/webpack/assets/javascripts/react_app/routes/FiltersForm/Taxonomies.js +++ /dev/null @@ -1,49 +0,0 @@ -import React from 'react'; -import PropTypes from 'prop-types'; -import { FormGroup } from '@patternfly/react-core'; -import { DualListWithIds } from '../../components/common/Pf4DualList/DualListWithIds'; -import { translate as __ } from '../../common/I18n'; - -export const Taxonomies = ({ - showOrgs, - showLocations, - setChosenOrgs, - setChosenLocations, - defaultOrgs, - defaultLocations, -}) => ( - <> - {showOrgs && ( - - - - )} - {showLocations && ( - - - - )} - -); -Taxonomies.propTypes = { - showOrgs: PropTypes.bool.isRequired, - showLocations: PropTypes.bool.isRequired, - setChosenOrgs: PropTypes.func.isRequired, - setChosenLocations: PropTypes.func.isRequired, - defaultOrgs: PropTypes.array, - defaultLocations: PropTypes.array, -}; -Taxonomies.defaultProps = { - defaultOrgs: [], - defaultLocations: [], -};