Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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

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.

Success message doesn't include host count (inconsistent with core bulk actions)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried changing org and location but it does not show count either

Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
module ForemanOpenscap
module Api
module V2
module HostsBulkActionsControllerExtensions
extend ActiveSupport::Concern

included do
before_action :find_editable_hosts, only: [:change_openscap_proxy]
end

def change_openscap_proxy
openscap_proxy_id = params[:openscap_proxy_id]

if openscap_proxy_id.blank?
return render json: {
error: {
message: _("No OpenSCAP Capsule selected."),
},
}, status: :unprocessable_entity
end

smart_proxy = begin
::SmartProxy.find(openscap_proxy_id)
rescue ActiveRecord::RecordNotFound
nil
end

if smart_proxy.nil?
return render json: {
error: {
message: _("OpenSCAP Capsule with id %s not found") % openscap_proxy_id,
},
}, status: :unprocessable_entity
end

unless smart_proxy.has_feature?('Openscap')
return render json: {
error: {
message: _("The selected capsule does not have the OpenSCAP feature enabled."),
},
}, status: :unprocessable_entity
end

failed_hosts = []
@hosts.each do |host|
host.openscap_proxy = smart_proxy
failed_hosts << host unless host.save
end

if failed_hosts.empty?
process_response(true, {
message: _("OpenSCAP Capsule set to %s") % smart_proxy.name,
})
else
render_error(:bulk_hosts_error, status: :unprocessable_entity,
locals: {
message: n_("Failed to assign OpenSCAP Capsule to %s host",
"Failed to assign OpenSCAP Capsule to %s hosts",
failed_hosts.count) % failed_hosts.count,
Comment on lines +57 to +59

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.

Could the message include also how many were successful/not done? (if I try 60, and see that 55 failed its unclear if the other 5 were skipped or successful)

failed_host_ids: failed_hosts.map(&:id),
})
end
end

private

def action_permission
case params[:action]
when 'change_openscap_proxy'
'edit'
else
super
end
end
end
end
end
end
2 changes: 2 additions & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@
end
end
end

match 'hosts/bulk/change_openscap_proxy', :to => 'hosts_bulk_actions#change_openscap_proxy', :via => [:put]
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion lib/foreman_openscap/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ class Engine < ::Rails::Engine
:resource_type => 'ForemanOpenscap::ScapContent'
permission :edit_hosts, { :hosts => %i[openscap_proxy_changed
select_multiple_openscap_proxy
update_multiple_openscap_proxy] },
update_multiple_openscap_proxy],
'api/v2/hosts_bulk_actions' => [:change_openscap_proxy] },
:resource_type => "Host"
permission :view_hosts, { 'api/v2/hosts' => [:policies_enc] }, :resource_type => 'Host'
permission :edit_hostgroups, { :hostgroups => [:openscap_proxy_changed] }, :resource_type => "Hostgroup"
Expand Down Expand Up @@ -195,6 +196,7 @@ class Engine < ::Rails::Engine
# Include concerns in this config.to_prepare block
config.to_prepare do
::Api::V2::HostsController.send(:include, ForemanOpenscap::Api::V2::HostsControllerExtensions)
::Api::V2::HostsBulkActionsController.send(:include, ForemanOpenscap::Api::V2::HostsBulkActionsControllerExtensions)
::Host::Managed.send(:include, ForemanOpenscap::OpenscapProxyExtensions)
::Host::Managed.send(:include, ForemanOpenscap::OpenscapProxyCoreExtensions)
::Host::Managed.send(:prepend, ForemanOpenscap::HostExtensions)
Expand Down
Comment thread
Lukshio marked this conversation as resolved.
Original file line number Diff line number Diff line change
@@ -0,0 +1,275 @@
import React, { useState, useEffect } from 'react';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import {
Modal,
Button,
Grid,
GridItem,
Form,
FormGroup,
Select,
Stack,
StackItem,
SelectOption,
SelectList,
MenuToggle,
} from '@patternfly/react-core';
import { foremanUrl } from 'foremanReact/common/helpers';
import { APIActions } from 'foremanReact/redux/API';
import { sprintf, translate as __ } from 'foremanReact/common/I18n';
import { STATUS } from 'foremanReact/constants';
import {
selectAPIStatus,
selectAPIResponse,
} from 'foremanReact/redux/API/APISelectors';
import {
BULK_CHANGE_OPENSCAP_PROXY_KEY,
HOSTS_API_PATH,
HOSTS_API_REQUEST_KEY,
OPENSCAP_PROXIES_KEY,
} from '../../../OpenscapRemediationWizard/constants';

const buildBulkRequestBody = ({
Comment thread
Lukshio marked this conversation as resolved.
Outdated
fetchBulkParams,
organizationId,
locationId,
...params
}) => ({
included: {
search: fetchBulkParams(),
},
...(organizationId != null ? { organization_id: organizationId } : {}),
...(locationId != null ? { location_id: locationId } : {}),
...params,
});

const fetchOpenscapProxies = () =>
APIActions.get({
key: OPENSCAP_PROXIES_KEY,
url: foremanUrl(
'/api/smart_proxies?search=feature%3DOpenscap&per_page=all'
),
});

const BulkChangeOpenscapProxyModal = ({
isOpen,
closeModal,
selectAllHostsMode,
selectedCount,
fetchBulkParams,
organizationId,
locationId,
}) => {
const dispatch = useDispatch();
const [proxyId, setProxyId] = useState('');
const [proxySelectOpen, setProxySelectOpen] = useState(false);
const [isSubmitting, setIsSubmitting] = useState(false);

useEffect(() => {
if (isOpen) {
dispatch(fetchOpenscapProxies());
} else {
setIsSubmitting(false);
}
}, [dispatch, isOpen]);

const proxies = useSelector(state =>
selectAPIResponse(state, OPENSCAP_PROXIES_KEY)
);
const proxyStatus = useSelector(state =>
selectAPIStatus(state, OPENSCAP_PROXIES_KEY)
);

const getProxyLabel = id => {
const proxy = proxies?.results?.find(
p => p.id.toString() === id.toString()
);
return proxy?.name || id;
};

const handleProxySelect = (_event, selection) => {
setProxyId(selection);
setProxySelectOpen(false);
};

const handleToggleClick = () => {
setProxySelectOpen(!proxySelectOpen);
};

const handleModalClose = () => {
setProxyId('');
setProxySelectOpen(false);
setIsSubmitting(false);
closeModal();
};

const handleSuccess = () => {
dispatch(
APIActions.get({
key: HOSTS_API_REQUEST_KEY,
url: foremanUrl(HOSTS_API_PATH),
})
);
Comment on lines +75 to +80

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 hoping to get this in to avoid bugs theforeman/foreman#11110

handleModalClose();
};

const handleError = () => {

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.

Would it be possible to re-use the error handling from theforeman/foreman#11045 ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@sbernhard Thanks for the feedback. It is actually solving different issues. In my case, I am using handleError for closing the modal and setting state. For handleSuccess I am using for refreshing the page and closing modal.

The foreman PR is solving parsing error/sucess messages from the server response and displaying Toast by using addToast. In my case, I am using directly success/errorToast built in APIActions so it's actually different codes for different purpose.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I also added my comments to the PR

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 starting to think we might need a generic handleSuccess as a lot of the bulk actions ignore the perPage param (and probably others) in the data reset - and we can either fix it 1, by 1, or send something similar to fetchBulkParams - like refreshData in the ForemanActionsBarContext

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think that at this moment we should finish the changes and schedule some further reactor in cooperation with UX team, unifying the design and code.

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.

Yeah good idea.

setIsSubmitting(false);
handleModalClose();
};

const handleConfirm = () => {
const requestBody = buildBulkRequestBody({
fetchBulkParams,
organizationId,
locationId,
openscap_proxy_id: proxyId,
});

setIsSubmitting(true);
dispatch(
APIActions.put({
key: BULK_CHANGE_OPENSCAP_PROXY_KEY,
url: foremanUrl('/api/v2/hosts/bulk/change_openscap_proxy'),
handleSuccess,
successToast: response => response.data.message,

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.

This doesnt parse the response correctly so the toast just says "success"

handleError,
errorToast: error =>
error?.response?.data?.error?.message || __('Error'),
params: requestBody,
})
);
};

const descriptionText = selectAllHostsMode ? (
<>

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.

FormattedMessage might be helpful here

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I was wondering what will be the best approach, but I decided to not use FormattedMessage so we don't rely on external libs.

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.

we use react-intl anyway for out translations

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Would you prefer to have it in FormattedMessage or I can leave it like this?

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 think you can leave it as is. was mostly sharing info

{__('Assign OpenSCAP capsule for ')}
<strong>{__('ALL selected hosts')}</strong>
<br />
{__(
'. This will change previous capsule assignments on the selected hosts.'
Comment thread
Lukshio marked this conversation as resolved.
Outdated
)}
</>
) : (
<>
{__('Assign OpenSCAP capsule for ')}
<strong>{sprintf(__('%s selected hosts.'), selectedCount)}</strong>
<br />
{__(
'This will change previous capsule assignments on the selected hosts.'
)}
</>
);

const modalActions = [
<Button
key="confirm"
ouiaId="bulk-change-openscap-capsule-modal-confirm-button"
variant="primary"
onClick={handleConfirm}
isDisabled={proxyId === '' || isSubmitting}
isLoading={isSubmitting}
spinnerAriaLabel={__('Loading')}
>
{__('Assign')}
</Button>,
<Button
key="cancel"
ouiaId="bulk-change-openscap-capsule-modal-cancel-button"
variant="link"
onClick={handleModalClose}
isDisabled={isSubmitting}
>
{__('Cancel')}
</Button>,
];

return (
<Modal
isOpen={isOpen}
onClose={handleModalClose}
onEscapePress={handleModalClose}
title={__('Assign OpenSCAP Capsule')}
width={650}
Comment thread
Lukshio marked this conversation as resolved.
Outdated
position="top"
actions={modalActions}
id="bulk-change-openscap-capsule-modal"
key="bulk-change-openscap-capsule-modal"
ouiaId="bulk-change-openscap-capsule-modal"
>
<Stack hasGutter>
<StackItem>{descriptionText}</StackItem>
{proxyStatus === STATUS.RESOLVED && proxies?.results?.length > 0 && (

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.

Could you add a spinner/skeleton until its resolved?

<StackItem>
<Grid>
<GridItem span={8}>
<Form>
<FormGroup label={__('Select OpenSCAP Capsule')}>
<Select
Comment thread
Lukshio marked this conversation as resolved.
Outdated
id="openscap-proxy-select"
isOpen={proxySelectOpen}
selected={proxyId}
onSelect={handleProxySelect}
onOpenChange={isSelectOpen =>
setProxySelectOpen(isSelectOpen)
}
ouiaId="bulk-change-openscap-capsule-select"
toggle={toggleRef => (
<MenuToggle
ref={toggleRef}
onClick={handleToggleClick}
isExpanded={proxySelectOpen}
style={{ width: '100%' }}
Comment thread
Lukshio marked this conversation as resolved.
Outdated
>
{proxyId
? getProxyLabel(proxyId)
: __('Select OpenSCAP Proxy')}
</MenuToggle>
)}
>
<SelectList>
{proxies.results.map(proxy => (
<SelectOption
key={proxy.id}
value={proxy.id.toString()}
>
{proxy.name}
</SelectOption>
))}
</SelectList>
</Select>
</FormGroup>
</Form>
</GridItem>
</Grid>
</StackItem>
)}
{proxyStatus === STATUS.RESOLVED &&
(!proxies?.results || proxies.results.length === 0) &&
__(
'No OpenSCAP Proxies available. Please configure a Smart Proxy with the OpenSCAP feature.'
)}
</Stack>
</Modal>
);
};

BulkChangeOpenscapProxyModal.propTypes = {
isOpen: PropTypes.bool,
closeModal: PropTypes.func,
fetchBulkParams: PropTypes.func.isRequired,
selectedCount: PropTypes.number.isRequired,
selectAllHostsMode: PropTypes.bool.isRequired,
organizationId: PropTypes.number,
locationId: PropTypes.number,
};

BulkChangeOpenscapProxyModal.defaultProps = {
isOpen: false,
closeModal: () => {},
organizationId: undefined,
locationId: undefined,
};

export default BulkChangeOpenscapProxyModal;
Loading
Loading