Skip to content
Merged
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
38 changes: 38 additions & 0 deletions app/controllers/api/v2/hosts_bulk_actions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,34 @@ def disassociate
"Updated hosts: Disassociated from compute resource", @hosts.count)})
end

api :PUT, "/hosts/bulk/assign_organization", N_("Assign organization")
param_group :bulk_host_ids
param :id, :number, :required => true, :desc => N_("The organization ID to assign the hosts to")
param :mismatch_setting, :bool, :required => true, :desc => N_("Fix organization on mismatch")
def assign_organization
without_taxonomy do
find_editable_hosts
taxonomy = Organization.find(params[:id])
BulkHostsManager.new(hosts: @hosts).assign_taxonomy(taxonomy, params[:mismatch_setting])
message = _("Organization is set to %s") % taxonomy.name
process_response(true, { :message => n_("Updated host: #{message}", "Updated hosts: #{message}", @hosts.count)})
end
end

api :PUT, "/hosts/bulk/assign_location", N_("Assign location")
param_group :bulk_host_ids
param :id, :number, :required => true, :desc => N_("The location ID to assign the hosts to")
param :mismatch_setting, :bool, :required => true, :desc => N_("Fix location on mismatch")
def assign_location
without_taxonomy do
find_editable_hosts
taxonomy = Location.find(params[:id])
BulkHostsManager.new(hosts: @hosts).assign_taxonomy(taxonomy, params[:mismatch_setting])
message = _("Location is set to %s") % taxonomy.name
process_response(true, { :message => n_("Updated host: #{message}", "Updated hosts: #{message}", @hosts.count)})

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.

I have not seen a good argument why you use @hosts.count instead of letting assign_taxonomy return the number of affected rows. I started that discussion in #10538 (comment) and thought you'd use the separation to easily implement that.

Please make the change I suggested. Summarizing what I said earlier:

  • @hosts.count will always return the number of rows.
  • update_all returns the number of affected rows.
  • You can say: 0 <= affected <= number of rows. If you run update_all a second time the number of affected is 0.

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.

[17] pry(main)> Host.update_all(organization_id: 1)
=> 14
[18] pry(main)> Host.update_all(organization_id: 1)
=> 14
[19] pry(main)> Host.update_all(organization_id: 1)
=> 14
[20] pry(main)> Host.update_all(organization_id: 1)
=> 14

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.

Instead of showing how many hosts are updated in a popup, we could display how many hosts are selected in the modal itself. I prefer the modal approach.

I need the ForemanActionsBarContext.Provider change in #10560 to display the selected hosts in the modal. Any chance to merge that PR first?

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.

I'm still going to insist that you don't use @hosts.count but rather use the result of update_all. Making BulkHostsManager responsible for that is IMHO a cleaner abstraction and it saves a query. I don't understand why you resist that.

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.

@hosts.update_all(organization_id: 1) would always update all of them. That is @hosts.count if the command is successful.
Otherwise it would throw an error if it fails.
We don't need to do it as you suggested that is unnecessary.

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.

In other words, @hosts.count is the result of update_all here if it is successful.

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.

You can say: 0 <= affected <= number of rows. If you run update_all a second time the number of affected is 0.

This is not right. It is either @hosts.count or an error we would catch.

end
end

protected

def action_permission
Expand All @@ -115,6 +143,16 @@ def find_editable_hosts
find_bulk_hosts(:edit_hosts, params)
end

def without_taxonomy
context = Foreman::ThreadSession::Context.get
Foreman::ThreadSession::Context.set(user: context[:user])
yield
rescue => e
render_error(:custom_error, :status => :unprocessable_entity, :locals => { :message => e.message})
ensure
Foreman::ThreadSession::Context.set(**context) if context
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 @@ -51,4 +51,19 @@ def disassociate
host.disassociate!
end
end

# @param [Boolean] optimistic_import
# either fix on mismatch or fail on mismatch
def assign_taxonomy(taxonomy, optimistic_import)
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
elsif taxonomy.need_to_be_selected_ids.empty?
@hosts.update_all("#{tax_type}_id".to_sym => taxonomy.id)
else
raise _("Cannot update %{type} to %{name} because of mismatch in settings") % {type: taxonomy.type.downcase, name: taxonomy.name}
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, :change_owner, :disassociate],
:"api/v2/hosts_bulk_actions" => [:assign_organization, :assign_location, :build, :reassign_hostgroup, :change_owner, :disassociate],
}
map.permission :destroy_hosts, {:hosts => [:destroy, :multiple_actions, :reset_multiple, :multiple_destroy, :submit_multiple_destroy],
:"api/v2/hosts" => [:destroy],
Expand Down
2 changes: 2 additions & 0 deletions config/routes/api/v2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
# 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_organization', :to => 'hosts_bulk_actions#assign_organization'
put 'hosts/bulk/assign_location', :to => 'hosts_bulk_actions#assign_location'
match 'hosts/bulk/build', :to => 'hosts_bulk_actions#build', :via => [:put]
match 'hosts/bulk/change_owner', :to => 'hosts_bulk_actions#change_owner', :via => [:put]
put 'hosts/bulk/disassociate', :to => 'hosts_bulk_actions#disassociate'
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 @@ -812,7 +812,7 @@ def test_unset_manage
:host_ids => Host.pluck('hosts.id'),
}, session: set_session_user
assert_redirected_to new_hosts_index_page_path
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 @@ -871,7 +871,7 @@ def test_unset_manage
:host_ids => Host.pluck('hosts.id'),
}, session: set_session_user
assert_redirected_to new_hosts_index_page_path
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,8 @@
export const BULK_ASSIGN_ORGANIZATION_KEY = 'BULK_ASSIGN_ORGANIZATION';
export const BULK_ASSIGN_LOCATION_KEY = 'BULK_ASSIGN_LOCATION';
export const ORGANIZATION_KEY = 'ORGANIZATION';
export const LOCATION_KEY = 'LOCATION';
export const MODAL_TYPES = {
ORGANIZATION: 'ORGANIZATION',
LOCATION: 'LOCATION',
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import { FormattedMessage } from 'react-intl';
import {
Modal,
Button,
MenuToggle,
SelectOption,
TextContent,
Text,
TreeView,
} 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 {
bulkAssignOrganization,
bulkAssignLocation,
fetchOrganizations,
fetchLocations,
} from './actions';
import {
BULK_ASSIGN_ORGANIZATION_KEY,
BULK_ASSIGN_LOCATION_KEY,
ORGANIZATION_KEY,
LOCATION_KEY,
MODAL_TYPES,
} from './BulkAssignTaxonomyConstants';
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';

export const BulkAssignOrganizationModal = props => (
<BulkAssignTaxonomyModal modalType={MODAL_TYPES.ORGANIZATION} {...props} />
);
export const BulkAssignLocationModal = props => (
<BulkAssignTaxonomyModal modalType={MODAL_TYPES.LOCATION} {...props} />
);

const BulkAssignTaxonomyModal = ({
isOpen,
closeModal,
selectAllHostsMode,
selectedCount,
fetchBulkParams,
modalType,
}) => {
const org = modalType === MODAL_TYPES.ORGANIZATION;
const taxType = org ? 'organization' : 'location';
const dispatch = useDispatch();
const [taxId, setTaxId] = useState('');
const [selectOpen, setSelectOpen] = useState(false);
const [fixRadioChecked, setFixRadioChecked] = useState(true);
const taxResults = useSelector(state =>
org
? selectAPIResponse(state, ORGANIZATION_KEY)
: selectAPIResponse(state, LOCATION_KEY)
);
const status = useSelector(state =>
org
? selectAPIStatus(state, ORGANIZATION_KEY)
: selectAPIStatus(state, LOCATION_KEY)
);
const hostUpdateStatus = useSelector(state =>
org
? selectAPIStatus(state, BULK_ASSIGN_ORGANIZATION_KEY)
: selectAPIStatus(state, BULK_ASSIGN_LOCATION_KEY)
);
const handleModalClose = () => {
setTaxId('');
setFixRadioChecked(true);
closeModal();
};

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

const onToggleClick = () => {
setSelectOpen(!selectOpen);
};
const toggle = toggleRef => (
<MenuToggle
ref={toggleRef}
onClick={onToggleClick}
isExpanded={selectOpen}
style={{ width: '95%' }}
>
{getSelectedLabel(taxId, taxResults)}
</MenuToggle>
);

const handleSelect = (event, selection) => {
setTaxId(selection);
setSelectOpen(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(),
},
id: taxId,
mismatch_setting: fixRadioChecked,
};

org
? dispatch(
bulkAssignOrganization(requestBody, handleSuccess, handleError)
)
: dispatch(bulkAssignLocation(requestBody, handleSuccess, handleError));
};

const translatedTaxType = org ? __('organization') : __('location');
const modalText = (
<FormattedMessage
id={`bulk-assign-${taxType}-text-message`}
defaultMessage={__(
'Select {taxonomy} to add hosts to. This change may affect all your selected hosts.'
)}
values={{ taxonomy: translatedTaxType }}
/>
);

const modalActions = [
<Button
key="add"
ouiaId={`bulk-assign-${taxType}-modal-add-button`}
variant="primary"
onClick={handleSave}
isDisabled={hostUpdateStatus === STATUS.PENDING || taxId === ''}
isLoading={hostUpdateStatus === STATUS.PENDING}
>
{org ? __('Change organization') : __('Change location')}
</Button>,
<Button
key="cancel"
ouiaId={`bulk-assign-${taxType}-modal-cancel-button`}
variant="link"
onClick={handleModalClose}
>
{__('Cancel')}
</Button>,
];

const selectedTreeViewData = [
{
name: __('Selected hosts'),
id: 'selected-hosts-tree-view-title',
customBadgeContent: selectAllHostsMode ? 'All' : selectedCount,
},
];

return (
<Modal
isOpen={isOpen}
onClose={handleModalClose}
onEscapePress={handleModalClose}
title={org ? __('Change organization') : __('Change location')}
width="50%"
position="top"
actions={modalActions}
id={`bulk-assign-${taxType}-modal`}
key={`bulk-assign-${taxType}-modal`}
ouiaId={`bulk-assign-${taxType}-modal`}
>
<TextContent>
<Text ouiaId={`bulk-assign-${taxType}-text`}>{modalText}</Text>
</TextContent>
{taxResults && status === STATUS.RESOLVED && (
<TaxonomySelect
headerText={org ? __('Select organization') : __('Select location')}
taxonomy={taxType}
isOpen={selectOpen}
selected={taxId}
onSelect={handleSelect}
onOpenChange={isSelectOpen => setSelectOpen(isSelectOpen)}
toggle={toggle}
radioChecked={fixRadioChecked}
setRadioChecked={setFixRadioChecked}
>
{taxResults.results?.map(tax => (
<SelectOption key={tax.id} value={tax.id}>
{tax.name}
</SelectOption>
))}
</TaxonomySelect>
)}
<div style={{ width: '70%', maxHeight: '50%', marginLeft: '-1rem' }}>
<TreeView
data={selectedTreeViewData}
aria-label={__('Selected hosts')}
hasBadges
/>
</div>
</Modal>
);
};

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

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