Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
43 changes: 43 additions & 0 deletions app/controllers/api/v2/hosts_bulk_actions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,32 @@ def reassign_hostgroup
end
end

api :PUT, "/hosts/bulk/assign_taxonomy", N_("Assign organization/location")
param_group :bulk_host_ids
param :target_organization_id, :number, :desc => N_("ID of the organization to assign the hosts to")
param :target_location_id, :number, :desc => N_("ID of the location to assign the hosts to")
param :mismatch_setting_organization, :bool, N_("Fix organization on mismatch")
param :mismatch_setting_location, :bool, N_("Fix location on mismatch")
def assign_taxonomy
validate_taxonomy_settings
clear_current_taxonomy
find_editable_hosts

targets = Taxonomy.where(id: [params[:target_organization_id], params[:target_location_id]])
messages = targets.map do |tax|
tax_type = tax.type.downcase
BulkHostsManager.new(hosts: @hosts).assign_taxonomy(tax, params["mismatch_setting_#{tax_type}"])
tax_type == 'organization' ? _("Organization is set to %s.") % tax.name : _("Location is set to %s.") % tax.name
end

process_response(true, { :message => n_("Updated host: %{update}", "Updated hosts: %{update}",
@hosts.count) % { update: messages.join(" ") }})
rescue => e
render_error(:custom_error, :status => :unprocessable_entity, :locals => { :message => e.message})
ensure
restore_current_taxonomy
end

protected

def action_permission
Expand All @@ -99,6 +125,23 @@ def find_editable_hosts
find_bulk_hosts(:edit_hosts, params)
end

def clear_current_taxonomy
@context = Foreman::ThreadSession::Context.get
Foreman::ThreadSession::Context.set(user: @context[:user])
end

def restore_current_taxonomy
Foreman::ThreadSession::Context.set(**@context) if @context
end

def validate_taxonomy_settings
if [params[:mismatch_setting_organization], params[:mismatch_setting_location]].any? { |val| val == false }
raise _("Cannot update host(s) because of mismatch in settings.")
elsif [params[:mismatch_setting_organization], params[:mismatch_setting_location]].all? { |val| val.nil? }
raise _("At lease one of organization/location mismatch settings should be specified.")
end
end

def rebuild_config
all_fails = BulkHostsManager.new(hosts: @hosts).rebuild_configuration
failed_host_ids = all_fails.flat_map { |_key, values| values&.map(&:id) }
Expand Down
19 changes: 5 additions & 14 deletions app/controllers/concerns/foreman/controller/taxonomy_multiple.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,12 @@ def update_multiple_taxonomies(type)
end

taxonomy = Taxonomy.find_by_id(id)

if params[type][:optimistic_import] == 'yes'
@hosts.update_all("#{type}_id".to_sym => taxonomy.id)
# hosts location needs to be updated before import missing ids
taxonomy.import_missing_ids
else
if taxonomy.need_to_be_selected_ids.count == 0
@hosts.update_all("#{type}_id".to_sym => taxonomy.id)
else
error "Cannot update #{taxonomy.type} to #{taxonomy.name} because of mismatch in settings"
redirect_back_or_to helpers.current_hosts_path
return
end
begin
BulkHostsManager.new(hosts: @hosts).assign_taxonomy(taxonomy, params[type][:optimistic_import] == 'yes')
success "Updated hosts: Changed #{type.to_s.classify}"
rescue => e
error e.message
end
success "Updated hosts: Changed #{type.to_s.classify}"
redirect_back_or_to helpers.current_hosts_path
end
end
15 changes: 15 additions & 0 deletions app/services/bulk_hosts_manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,19 @@ def rebuild_configuration
end
all_fails
end

# @param [Boolean] optimistic_import
# either fix on mismatch or fail on mismatch
def assign_taxonomy(taxonomy, optimistic_import)
Comment thread
ekohl marked this conversation as resolved.
Outdated
tax_type = taxonomy.type.downcase
if optimistic_import
@hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
# hosts location needs to be updated before import missing ids
taxonomy.import_missing_ids
Comment on lines 47 to 49

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can consistently make it return the number of updated hosts:

Suggested change
@hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
# hosts location needs to be updated before import missing ids
taxonomy.import_missing_ids
updated = @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
# hosts location needs to be updated before import missing ids
taxonomy.import_missing_ids
updated

Then you can also update the documentation of this method with:

# @return [Integer]
#   The number of updated hosts

You can then use that in the bulk controller to accurately state how many hosts were updated.

However, that also presents another problem: you can't simply add the 2 numbers and they may have overlap. That again stresses why it would be better to do both updates in a single transaction and only update the hosts once. I'd expect it to be faster and also gives a more accurate summary to the user.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated = @hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)

Isn't this the same as @hosts.count?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Currently there is no problem to find out how many hosts are updated accurately. Not sure what the concern is here.

It is really hard to make the both updates in one transaction considering the two different mismatch settings and the support for both API versions. And I'm not sure if taxonomy.import_missing_ids works with updating both organization and location in one transaction or not.

If it really worth to make it one transaction and it is doable, maybe we can do it in a followup.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this the same as @hosts.count?

No, it's not. update_all returns the number of affected rows, not how many you sent in the first place.

So it is at least 0 (in case nothing was updated) or at most @hosts.count (in case all rows were updated).

Currently there is no problem to find out how many hosts are updated accurately. Not sure what the concern is here.

That's because you misunderstood the return value of update_all. There is a problem.

It is really hard to make the both updates in one transaction considering the two different mismatch settings and the support for both API versions. And I'm not sure if taxonomy.import_missing_ids works with updating both organization and location in one transaction or not.

The last point is an interesting challenge. But if you can't, then combining both into a single frontend dialog may be wrong to start with.

If it really worth to make it one transaction and it is doable, maybe we can do it in a followup.

I think it is about returning a correct number. If you don't want to show a correct number, then please don't show one at all (like the old UI).

@lfu lfu Jul 1, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will change the message to not display number of hosts.

The last point is an interesting challenge. But if you can't, then combining both into a single frontend dialog may be wrong to start with.

I want to make it right. Do you think it is reasonable to have 2 tasks for change organization and change location instead of 1 task here in this PR? @ekohl @jeremylenz

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think having separate modals for change org & change location would be fine, especially since it simplifies our implementation here. I can't imagine a lot of users wanting to do both at the same time, but if I am wrong, it's a rare-enough task that it's worth the tradeoff.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Today it's also separate, so it's the most straight forward migration for the user.

elsif taxonomy.need_to_be_selected_ids.empty?
@hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
else
raise "Cannot update #{taxonomy.type.downcase} to #{taxonomy.name} because of mismatch in settings"
end
end
end
2 changes: 1 addition & 1 deletion config/initializers/f_foreman_permissions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@
:"api/v2/hosts" => [:update, :disassociate, :forget_status],
:"api/v2/interfaces" => [:create, :update, :destroy],
:"api/v2/compute_resources" => [:associate],
:"api/v2/hosts_bulk_actions" => [:build, :reassign_hostgroup],
:"api/v2/hosts_bulk_actions" => [:build, :reassign_hostgroup, :assign_taxonomy],
}
map.permission :destroy_hosts, {:hosts => [:destroy, :multiple_actions, :reset_multiple, :multiple_destroy, :submit_multiple_destroy],
:"api/v2/hosts" => [:destroy],
Expand Down
1 change: 1 addition & 0 deletions config/routes/api/v2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
# new v2 routes that point to v2
scope "(:apiv)", :module => :v2, :defaults => {:apiv => 'v2'}, :apiv => /v2/, :constraints => ApiConstraints.new(:version => 2, :default => true) do
match 'hosts/bulk', :to => 'hosts_bulk_actions#bulk_destroy', :via => [:delete]
put 'hosts/bulk/assign_taxonomy', :to => 'hosts_bulk_actions#assign_taxonomy'
match 'hosts/bulk/build', :to => 'hosts_bulk_actions#build', :via => [:put]
match 'hosts/bulk/reassign_hostgroup', :to => 'hosts_bulk_actions#reassign_hostgroup', :via => [:put]

Expand Down
4 changes: 2 additions & 2 deletions test/controllers/hosts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -811,7 +811,7 @@ def test_unset_manage
:host_ids => Host.pluck('hosts.id'),
}, session: set_session_user
assert_redirected_to :controller => :hosts, :action => :index
assert flash[:error] == "Cannot update Location to Location 1 because of mismatch in settings"
assert_equal "Cannot update location to Location 1 because of mismatch in settings", flash[:error]
end
test "update multiple location does not update location of hosts if fails on pessimistic import" do
@request.env['HTTP_REFERER'] = current_hosts_path
Expand Down Expand Up @@ -870,7 +870,7 @@ def test_unset_manage
:host_ids => Host.pluck('hosts.id'),
}, session: set_session_user
assert_redirected_to :controller => :hosts, :action => :index
assert_equal "Cannot update Organization to Organization 1 because of mismatch in settings", flash[:error]
assert_equal "Cannot update organization to Organization 1 because of mismatch in settings", flash[:error]
end
test "update multiple organization does not update organization of hosts if fails on pessimistic import" do
@request.env['HTTP_REFERER'] = current_hosts_path
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,257 @@
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import {
Modal,
Button,
MenuToggle,
SelectOption,
TextContent,
Text,
} from '@patternfly/react-core';
import { addToast } from '../../../ToastsList/slice';
import { translate as __ } from '../../../../common/I18n';
import { STATUS } from '../../../../constants';
import {
selectAPIStatus,
selectAPIResponse,
} from '../../../../redux/API/APISelectors';
import {
BULK_ASSIGN_TAXONOMY_KEY,
bulkAssignTaxonomy,
fetchOrganizations,
fetchLocations,
ORGANIZATION_KEY,
LOCATION_KEY,
} from './actions';
import { foremanUrl } from '../../../../common/helpers';
import { APIActions } from '../../../../redux/API';
import {
HOSTS_API_PATH,
API_REQUEST_KEY,
} from '../../../../routes/Hosts/constants';
import TaxonomySelect from './TaxonomySelect';

const BulkAssignTaxonomyModal = ({
isOpen,
closeModal,
selectedCount,
fetchBulkParams,
}) => {
const dispatch = useDispatch();
const [organizationId, setOrganizationId] = useState('');
const [locationId, setLocationId] = useState('');
const [orgSelectOpen, setOrgSelectOpen] = useState(false);
const [locSelectOpen, setLocSelectOpen] = useState(false);
const [orgFixRadioChecked, setOrgFixRadioChecked] = useState(true);
const [locFixRadioChecked, setLocFixRadioChecked] = useState(true);
const organizations = useSelector(state =>
selectAPIResponse(state, ORGANIZATION_KEY)
);
const organizationStatus = useSelector(state =>
selectAPIStatus(state, ORGANIZATION_KEY)
);
const locations = useSelector(state =>
selectAPIResponse(state, LOCATION_KEY)
);
const locationStatus = useSelector(state =>
selectAPIStatus(state, LOCATION_KEY)
);
const hostUpdateStatus = useSelector(state =>
selectAPIStatus(state, BULK_ASSIGN_TAXONOMY_KEY)
);
const handleModalClose = () => {
setOrganizationId('');
setLocationId('');
setOrgFixRadioChecked(true);
setLocFixRadioChecked(true);
closeModal();
};

useEffect(() => {
dispatch(fetchOrganizations());
dispatch(fetchLocations());
}, [dispatch]);

const onOrgToggleClick = () => {
setOrgSelectOpen(!orgSelectOpen);
};
const toggleOrg = toggleRef => (
<MenuToggle
ref={toggleRef}
onClick={onOrgToggleClick}
isExpanded={orgSelectOpen}
style={{ width: '95%' }}
>
{getSelectedLabel(organizationId, organizations)}
</MenuToggle>
);

const handleOrgSelect = (event, selection) => {
setOrganizationId(selection);
setOrgSelectOpen(false);
};

const onLocToggleClick = () => {
setLocSelectOpen(!locSelectOpen);
};
const toggleLoc = toggleRef => (
<MenuToggle
ref={toggleRef}
onClick={onLocToggleClick}
isExpanded={locSelectOpen}
style={{ width: '95%' }}
>
{getSelectedLabel(locationId, locations)}
</MenuToggle>
);

const handleLocSelect = (event, selection) => {
setLocationId(selection);
setLocSelectOpen(false);
};

const getSelectedLabel = (id, taxonomy) =>
taxonomy.results.find(t => t.id === id)?.name;

const handleError = error => {
const {
response: {
data: {
error: { message },
},
},
} = error;
dispatch(addToast({ type: 'danger', message }));
handleModalClose();
};

const handleSuccess = response => {
dispatch(
addToast({
type: 'success',
message: response.data.message,
})
);
dispatch(
APIActions.get({
key: API_REQUEST_KEY,
url: foremanUrl(HOSTS_API_PATH),
})
);
handleModalClose();
};

const handleSave = () => {
const requestBody = {
included: {
search: fetchBulkParams(),
},
target_location_id: locationId,
target_organization_id: organizationId,
mismatch_setting_location: locFixRadioChecked,
mismatch_setting_organization: orgFixRadioChecked,
};

dispatch(bulkAssignTaxonomy(requestBody, handleSuccess, handleError));
};

const modalActions = [
<Button
key="add"
ouiaId="bulk-assign-taxonomy-modal-add-button"
variant="primary"
onClick={handleSave}
isDisabled={
hostUpdateStatus === STATUS.PENDING ||
(organizationId === '' && locationId === '')
}
isLoading={hostUpdateStatus === STATUS.PENDING}
>
{__('Save')}
</Button>,
<Button
key="cancel"
ouiaId="bulk-assign-taxonomy-modal-cancel-button"
variant="link"
onClick={handleModalClose}
>
{__('Cancel')}
</Button>,
];
return (
<Modal
isOpen={isOpen}
onClose={handleModalClose}
onEscapePress={handleModalClose}
title={__('Change organization/location')}
width="50%"
position="top"
actions={modalActions}
id="bulk-assign-taxonomy-modal"
key="bulk-assign-taxonomy-modal"
ouiaId="bulk-assign-taxonomy-modal"
>
<TextContent>
<Text ouiaId="bulk-assign-taxonomy-options">
{__(
'Select organization/location to add hosts to. This change may affect all your selected hosts.'
)}
</Text>
</TextContent>
{organizations && organizationStatus === STATUS.RESOLVED && (
<TaxonomySelect
headerText={__('Select organization')}
taxonomy="organization"
isOpen={orgSelectOpen}
selected={organizationId}
onSelect={handleOrgSelect}
onOpenChange={isSelectOpen => setOrgSelectOpen(isSelectOpen)}
toggle={toggleOrg}
radioChecked={orgFixRadioChecked}
setRadioChecked={setOrgFixRadioChecked}
>
{organizations.results?.map(org => (
<SelectOption key={org.id} value={org.id}>
{org.name}
</SelectOption>
))}
</TaxonomySelect>
)}
<hr />
{locations && locationStatus === STATUS.RESOLVED && (
<TaxonomySelect
headerText={__('Select location')}
taxonomy="location"
isOpen={locSelectOpen}
selected={locationId}
onSelect={handleLocSelect}
onOpenChange={isSelectOpen => setLocSelectOpen(isSelectOpen)}
toggle={toggleLoc}
radioChecked={locFixRadioChecked}
setRadioChecked={setLocFixRadioChecked}
>
{locations.results?.map(loc => (
<SelectOption key={loc.id} value={loc.id}>
{loc.name}
</SelectOption>
))}
</TaxonomySelect>
)}
</Modal>
);
};

BulkAssignTaxonomyModal.propTypes = {
isOpen: PropTypes.bool,
closeModal: PropTypes.func,
selectedCount: PropTypes.number.isRequired,
fetchBulkParams: PropTypes.func.isRequired,
};

BulkAssignTaxonomyModal.defaultProps = {
isOpen: false,
closeModal: () => {},
};

export default BulkAssignTaxonomyModal;
Loading
Loading