Fixes #39532 - All hosts - add action - bulk add capsule#616
Conversation
MariaAga
left a comment
MariaAga
left a comment
There was a problem hiding this comment.
initial review:
Capsule is for downstream, and gets translated from "Proxy" please only use proxy here.
Going to do some tests now, and then a more in depth review once it gets updated
There was a problem hiding this comment.
Success message doesn't include host count (inconsistent with core bulk actions)
There was a problem hiding this comment.
I tried changing org and location but it does not show count either
pr-processor
Bot
added
Waiting on contributor
Needs re-review
and removed
Not yet reviewed
labels
|
Please have a look at this, too: #615 |
| handleModalClose(); | ||
| }; | ||
|
|
||
| const handleError = () => { |
There was a problem hiding this comment.
Would it be possible to re-use the error handling from theforeman/foreman#11045 ?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
I also added my comments to the PR
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
MariaAga
left a comment
MariaAga
left a comment
There was a problem hiding this comment.
(midway review as I cant comment on the discussion without clicking "submit review")
Capsule is still mentioned in the code, PR title, and commits.
Also casing should be always OpenSCAP Proxy (and not OpenSCAP proxy)
| > | ||
| <Stack hasGutter> | ||
| <StackItem>{descriptionText}</StackItem> | ||
| {proxyStatus === STATUS.RESOLVED && proxies?.results?.length > 0 && ( |
There was a problem hiding this comment.
Could you add a spinner/skeleton until its resolved?
| handleModalClose(); | ||
| }; | ||
|
|
||
| const handleError = () => { |
There was a problem hiding this comment.
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
| key: BULK_CHANGE_OPENSCAP_PROXY_KEY, | ||
| url: foremanUrl('/api/v2/hosts/bulk/change_openscap_proxy'), | ||
| handleSuccess, | ||
| successToast: response => response.data.message, |
There was a problem hiding this comment.
This doesnt parse the response correctly so the toast just says "success"
There was a problem hiding this comment.
It is parsed correctly, but it usually exceeds this rule: https://github.com/theforeman/foreman/blob/7170b302c65d0a318179151f41fb7c699fe01629/webpack/assets/javascripts/react_app/components/ToastsList/helpers.js#L23
thinking of that 60 chars are really low limit to our messages
There was a problem hiding this comment.
You can wrap it in a <div> so it will be sent inside the alert body
addToast({
type: 'success',
message: <div>Ex amet voluptate laborum sint ea ut in commodo. Nulla voluptate velit qui est cupidatat ea et commodo ipsum proident adipisicing occaecat. Consequat quis commodo minim nulla ipsum aute ipsum.</div>
})There was a problem hiding this comment.
I tried to avoid using extra addToast when successToast can be used, but I guess that there is no other option. Then it probably can be aligned with theforeman/foreman#11045 or maybe it should be part of the bigger bulk actions refactor. Maybe we could edit addToast to convert title to text once it reach the 60 chars limitation, wdym @MariaAga
There was a problem hiding this comment.
Maybe we could edit addToast to convert title to text once it reach the 60 chars limitation
That would be a good feature
There was a problem hiding this comment.
Feature introduced here, in favor of that I am bringing back old handler theforeman/foreman#11114
| handleModalClose(); | ||
| }; | ||
|
|
||
| const handleError = () => { |
| dispatch( | ||
| APIActions.get({ | ||
| key: HOSTS_API_REQUEST_KEY, | ||
| url: foremanUrl(HOSTS_API_PATH), | ||
| }) | ||
| ); |
There was a problem hiding this comment.
I'm hoping to get this in to avoid bugs theforeman/foreman#11110
There was a problem hiding this comment.
Let's wait until it gets merged
There was a problem hiding this comment.
@ofedoren Could you take a look at the ruby here?
Should this be under api? then its missing docs, or should it be under controllers
3 participants
Users can assign an OpenSCAP Proxy to a host. This can be done individually for a host via the host edit form, but also in bulk via the legacy host overview UI (see screenshot). The new UI currently doesn't support this.
Option can be found on AllHosts page -> select hosts -> kebab menu -> Change association -> OpenSCAP Capsule