Skip to content
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

association and disassociation of host groups and inventory host groups list. #6559

Conversation

jlmitch5
Copy link
Contributor

@jlmitch5 jlmitch5 commented Apr 3, 2020

link #5908 (this is the association/disassociation part)

  • hoisted up association modal and disassociation button to components (as they would be shared between inventories and host screens. there is nothing specific going on really, so I think this move makes sense
  • implemented these in both the all hosts host group and inventory host host groups list tabs.

--

Note there is a kind of strange thing where if your host is a member of a group as well a member of a child group of that group, and you try to disassociate with the root group, it will still show in the list (because it is still indirectly a member of that group through the child). I checked and this is the same as in the old ui. We could make this work by changing the list's response from /all_groups/ to /groups/ (the latter only shows groups you are directly in. But I'm not sure which we'd want.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@unlikelyzero unlikelyzero self-requested a review April 3, 2020 17:55
@jlmitch5 jlmitch5 force-pushed the newNewAssocDisassocHostGroupsList branch from f2ab381 to bf4de87 Compare April 3, 2020 18:32
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@unlikelyzero
Copy link

unlikelyzero commented Apr 3, 2020

@jlmitch5 is this visual issue specific to this modal? #6571
we should also try to fix #6577

@unlikelyzero
Copy link

note, this pr fixes #6549

@jlmitch5 jlmitch5 force-pushed the newNewAssocDisassocHostGroupsList branch from bf4de87 to 350a6d6 Compare April 6, 2020 14:15
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@marshmalien
Copy link
Member

Missing nested group breadcrumb. I think you can configure it in Inventories.jsx.
Screen Shot 2020-04-06 at 11 13 40 AM

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

Some small changes and a few questions seeking clarity.

return (
<Switch>
<Route
key="list"
path="/hosts/:id/groups"
render={() => {
return <HostGroupsList location={location} match={match} />;
return (
<HostGroupsList host={host} location={location} match={match} />
Copy link
Member

@AlexSCorey AlexSCorey Apr 6, 2020

Choose a reason for hiding this comment

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

We should probably no longer pass match and location in favor of the useLocation() and useParams, in the component that they are needed. Also our new pattern is to wrap the component in <Route> rather than use the render function.

@@ -15,27 +25,35 @@ const QS_CONFIG = getQSConfig('group', {
order_by: 'name',
});

function HostGroupsList({ i18n, location, match }) {
const [selected, setSelected] = useState([]);
function HostGroupsList({ i18n, location, match, host }) {
Copy link
Member

Choose a reason for hiding this comment

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

remove location and match from here in favor of useLocation() and useParams()

groups: results,
itemCount: count,
actions: actionsResponse.data.actions,
};
}, [hostId, location]), // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Member

Choose a reason for hiding this comment

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

It seems like we can also remove this eslint warning that is disabled

@@ -16,26 +26,33 @@ const QS_CONFIG = getQSConfig('group', {
});

function InventoryHostGroupsList({ i18n, location, match }) {
Copy link
Member

Choose a reason for hiding this comment

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

use useLocation() and useParams instead of passing location and match as props.

groups: results,
itemCount: count,
actions: actionsResponse.data.actions,
};
}, [hostId, location]), // eslint-disable-line react-hooks/exhaustive-deps
Copy link
Member

Choose a reason for hiding this comment

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

You can probably get rid of this es lint warning disable comment.

selected.length > 0 && selected.length === groups.length;
const fetchGroupsToAssociate = useCallback(
params => {
return InventoriesAPI.readGroups(
Copy link
Member

Choose a reason for hiding this comment

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

Why not use useRequest here?

};

const isAllSelected =
selected.length > 0 && selected.length === groups.length;
const fetchGroupsToAssociate = useCallback(
Copy link
Member

Choose a reason for hiding this comment

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

Why not use useRequest here?

Copy link
Member

Choose a reason for hiding this comment

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

That is because the request is not getting called in this component. On line 95, the function is getting "defined" for later use. fetchGroupsToAssociate is passed to AssociateModal as the fetchRequest prop. When the modal opens, fetchRequest is called within the useRequest hook to populate the modal list.

@AlexSCorey
Copy link
Member

Missing nested group breadcrumb. I think you can configure it in Inventories.jsx.
Screen Shot 2020-04-06 at 11 13 40 AM

@jlmitch5 This same issue is on the Facts tab. I'm happy to create an issue for it to be addressed in a separate PR, or address it in this 1. Let me know which you choose.

@jlmitch5 jlmitch5 force-pushed the newNewAssocDisassocHostGroupsList branch from 350a6d6 to f3f441b Compare April 6, 2020 17:55
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jlmitch5 jlmitch5 force-pushed the newNewAssocDisassocHostGroupsList branch from f3f441b to ce30594 Compare April 6, 2020 19:05
Copy link
Member

@marshmalien marshmalien left a comment

Choose a reason for hiding this comment

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

This looks ✨ Thanks for hooking this up.

Copy link
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

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

Good work on this!

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 5b3f5bf into ansible:devel Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants