-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display SUMA updates in Host Details page #2430
Conversation
19cee66
to
752dc59
Compare
752dc59
to
a7be359
Compare
relevantPatches={numRelevantPatches} | ||
upgradablePackages={numUpgradablePackages} | ||
softwareUpdatesLoading={softwareUpdatesLoading} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we wanna add a tooltip text as a prop here? I really don't know, and I really don't wanna block this PR just for this nit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Do you think it's worth testing this tooltip behaviour from HostDetails.test.jsx
, as it's already being tested in AvailableSoftwareUpdates.test.jsx
... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think we can iterate over that, for example in the future we can have a selector to determine tooltip's text
@@ -66,6 +78,7 @@ function HostDetailsPage() { | |||
getExportersStatus(); | |||
refreshCatalog(); | |||
dispatch(updateLastExecution(hostID)); | |||
dispatch(fetchSoftwareUpdates(hostID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to be like the following
dispatch(fetchSoftwareUpdates(hostID)); | |
dispatch(fetchSoftwareUpdates({hostId: hostID})) |
unless we change that action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm just had a look through our codebase and we use a mix of hostID
and hostId
🤔 I guess hostID
is more popular though, so I'll change the action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
up to you, but the thing is we need to pass an object, not the plain id (whatever is called).
unless we do transformation in the action
export const createAction = createAction(
FETCH_SOFTWARE_UPDATES,
(hostId) => ({ payload: { hostId } })
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see what you mean. I thought I was using the action-creator in @state/softwareUpdates
. Instead, I was calling the Saga directly 😳
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aaand yes!
Description
This change mounts the
AvailableSoftwareUpdates
component in the Host Details page, allowing users to see the available updates and patches for a host, as instructed by SUMA.How was this tested?
Added React component tests to validate that the Available Software Updates component is correctly being displayed in the Host Details page.