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

Deregister hosts list frontend #1601

Merged
merged 5 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 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
37 changes: 35 additions & 2 deletions assets/js/components/HostsList.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import React from 'react';
import React, { useState } from 'react';

import { useSearchParams } from 'react-router-dom';
import { useSelector, useDispatch } from 'react-redux';
import { EOS_WARNING_OUTLINED } from 'eos-icons-react';

import Table from '@components/Table';
import DeregistrationModal from '@components/DeregistrationModal';
import HealthIcon from '@components/Health/HealthIcon';
import Tags from '@components/Tags';
import HostLink from '@components/HostLink';
Expand All @@ -16,8 +17,10 @@ import HealthSummary from '@components/HealthSummary/HealthSummary';
import { getCounters } from '@components/HealthSummary/summarySelection';
import ProviderLabel from '@components/ProviderLabel';
import Tooltip from '@components/Tooltip';
import CleanUpButton from '@components/CleanUpButton';

import { addTagToHost, removeTagFromHost, deregisterHost } from '@state/hosts';

import { addTagToHost, removeTagFromHost } from '@state/hosts';
import { post, del } from '@lib/network';
import { agentVersionWarning } from '@lib/agent';

Expand Down Expand Up @@ -50,6 +53,8 @@ function HostsList() {
);

const [searchParams, setSearchParams] = useSearchParams();
const [cleanUpModalOpen, setCleanUpModalOpen] = useState(false);
const [selectedHost, setSelectedHost] = useState(undefined);

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename the state property to hostToDeregister or something like that, selectedHost seems to generic

const dispatch = useDispatch();

Expand Down Expand Up @@ -177,6 +182,21 @@ function HostsList() {
/>
),
},
{
title: '',
key: 'deregisterable',
className: 'w-48',
render: (content, item) =>
content && (
<CleanUpButton
cleaning={item.deregistering}
onClick={() => {
setSelectedHost(item);
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this to a named function in the component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of creating functions for such trivial things (2 lines in fact), as it forces the user to scroll up and down in the code, to find out such a simple thing.
Anyway, for the sake of a request and thumbs up, i will apply the change hehe

Copy link
Contributor Author

@arbulu89 arbulu89 Jul 11, 2023

Choose a reason for hiding this comment

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

@CDimonaco This is how it looks like now: 3614c2e

Personally, I find it uglier (specially, because the functions are still inside the component). What do you think?

setCleanUpModalOpen(true);
}}
/>
),
},
],
};

Expand All @@ -199,13 +219,26 @@ function HostsList() {
id: host.id,
tags: (host.tags && host.tags.map((tag) => tag.value)) || [],
sap_systems: sapSystemList,
deregisterable: host.deregisterable,
deregistering: host.deregistering,
};
});

const counters = getCounters(data || []);
return (
<>
<PageHeader className="font-bold">Hosts</PageHeader>
<DeregistrationModal
hostname={selectedHost?.hostname}
isOpen={!!cleanUpModalOpen}
onCleanUp={() => {
setCleanUpModalOpen(false);
Copy link
Member

Choose a reason for hiding this comment

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

Can we extract this to a named function in the component?

dispatch(deregisterHost(selectedHost));
}}
onCancel={() => {
setCleanUpModalOpen(false);
}}
/>
<div className="bg-white rounded-lg shadow">
<HealthSummary {...counters} className="px-4 py-2" />
<Table
Expand Down
91 changes: 91 additions & 0 deletions assets/js/components/HostsList.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,97 @@ describe('HostsLists component', () => {
});
});

describe('deregistration', () => {
it('should show the clean up button when the host is deregisterable', () => {
const host1 = hostFactory.build({ deregisterable: true });
const host2 = hostFactory.build({ deregisterable: false });
const state = {
...defaultInitialState,
hostsList: {
hosts: [].concat(host1, host2),
},
};

const [StatefulHostsList] = withState(<HostsList />, state);

renderWithRouter(StatefulHostsList);
const table = screen.getByRole('table');
const cleanUpCell1 = table.querySelector(
'tr:nth-child(1) > td:nth-child(9)'
);
const cleanUpCell2 = table.querySelector(
'tr:nth-child(2) > td:nth-child(9)'
);
expect(cleanUpCell1).toHaveTextContent('Clean up');
expect(cleanUpCell2).not.toHaveTextContent('Clean up');
});

it('should show the host in deregistering state', () => {
const host = hostFactory.build({
deregisterable: true,
deregistering: true,
});
const state = {
...defaultInitialState,
hostsList: {
hosts: [host],
},
};

const [StatefulHostsList] = withState(<HostsList />, state);

renderWithRouter(StatefulHostsList);
expect(screen.getByLabelText('Loading')).toBeInTheDocument();
});

it('should request a deregistration when the clean up button in the modal is clicked', async () => {
const user = userEvent.setup();

const host = hostFactory.build({ deregisterable: true });
const state = {
...defaultInitialState,
hostsList: {
hosts: [host],
},
};

const [StatefulHostsList, store] = withState(<HostsList />, state);

renderWithRouter(StatefulHostsList);

const table = screen.getByRole('table');
const cleanUpButton = table.querySelector(
'tr:nth-child(1) > td:nth-child(9) > button'
);

await user.click(cleanUpButton);

expect(
screen.getByText(
`Clean up data discovered by agent on host ${host.hostname}`
)
).toBeInTheDocument();

const cleanUpModalButton = screen.getAllByRole('button', {
name: 'Clean up',
})[1];

await user.click(cleanUpModalButton);

const actions = store.getActions();
const expectedActions = [
{
type: 'DEREGISTER_HOST',
payload: expect.objectContaining({
jamie-suse marked this conversation as resolved.
Show resolved Hide resolved
id: host.id,
hostname: host.hostname,
}),
},
];
expect(actions).toEqual(expect.arrayContaining(expectedActions));
});
});

describe('filtering', () => {
const cleanInitialState = {
hostsList: {
Expand Down
20 changes: 20 additions & 0 deletions assets/js/state/hosts.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,22 @@ export const hostsListSlice = createSlice({
return host;
});
},
setHostDeregistering: (state, action) => {
state.hosts = state.hosts.map((host) => {
if (host.id === action.payload.id) {
return { ...host, deregistering: true };
}
return host;
});
},
setHostNotDeregistering: (state, action) => {
Copy link
Member

Choose a reason for hiding this comment

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

I have a doubt about naming. I would have thought of something that talks about starting a deregistration and completing a deregistration.
No hard feelings tho 😄

state.hosts = state.hosts.map((host) => {
if (host.id === action.payload.id) {
return { ...host, deregistering: false };
}
return host;
});
},
startHostsLoading: (state) => {
state.loading = true;
},
Expand All @@ -95,13 +111,15 @@ export const CHECK_HOST_IS_DEREGISTERABLE = 'CHECK_HOST_IS_DEREGISTERABLE';
export const CANCEL_CHECK_HOST_IS_DEREGISTERABLE =
'CANCEL_CHECK_HOST_IS_DEREGISTERABLE';
export const HOST_DEREGISTERED = 'HOST_DEREGISTERED';
export const DEREGISTER_HOST = 'DEREGISTER_HOST';

export const checkHostIsDeregisterable = createAction(
CHECK_HOST_IS_DEREGISTERABLE
);
export const cancelCheckHostIsDeregisterable = createAction(
CANCEL_CHECK_HOST_IS_DEREGISTERABLE
);
export const deregisterHost = createAction(DEREGISTER_HOST);

export const {
setHosts,
Expand All @@ -113,6 +131,8 @@ export const {
setHeartbeatCritical,
setHostListDeregisterable,
setHostNotDeregisterable,
setHostDeregistering,
setHostNotDeregistering,
startHostsLoading,
stopHostsLoading,
removeHost,
Expand Down
30 changes: 30 additions & 0 deletions assets/js/state/hosts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import hostsReducer, {
removeHost,
setHostListDeregisterable,
setHostNotDeregisterable,
setHostDeregistering,
setHostNotDeregistering,
} from '@state/hosts';
import { hostFactory } from '@lib/test-utils/factories';

Expand Down Expand Up @@ -39,6 +41,34 @@ describe('Hosts reducer', () => {
expect(hostsReducer(initialState, action)).toEqual(expectedState);
});

it('should set host in deregistering state', () => {
const [host1, host2] = hostFactory.buildList(2);
const initialState = { hosts: [host1, host2] };

const action = setHostDeregistering(host1);

const expectedState = {
hosts: [{ ...host1, deregistering: true }, host2],
};

expect(hostsReducer(initialState, action)).toEqual(expectedState);
});

it('should remove deregistering state from host', () => {
const [host1, host2] = hostFactory.buildList(2);
const initialState = {
hosts: [host1, host2],
};

const action = setHostNotDeregistering(host1);

const expectedState = {
hosts: [{ ...host1, deregistering: false }, host2],
};

expect(hostsReducer(initialState, action)).toEqual(expectedState);
});

it('should remove host from state', () => {
const [host1, host2] = hostFactory.buildList(2);
const initialState = {
Expand Down
25 changes: 25 additions & 0 deletions assets/js/state/sagas/hosts.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,14 @@ import {
CHECK_HOST_IS_DEREGISTERABLE,
CANCEL_CHECK_HOST_IS_DEREGISTERABLE,
HOST_DEREGISTERED,
DEREGISTER_HOST,
removeHost,
setHostListDeregisterable,
setHostDeregistering,
setHostNotDeregistering,
} from '@state/hosts';

import { del } from '@lib/network';
import { notify } from '@state/actions/notifications';

export function* markDeregisterableHosts(hosts) {
Expand Down Expand Up @@ -43,10 +48,30 @@ export function* hostDeregistered({ payload }) {
);
}

export function* deregisterHost({ payload }) {
yield put(setHostDeregistering(payload));
try {
yield call(del, `/hosts/${payload.id}`);
} catch (error) {
yield put(
notify({
text: `Error deregistering host ${payload?.hostname}.`,
icon: '❌',
})
);
} finally {
yield put(setHostNotDeregistering(payload));
}
}

export function* watchHostDeregisterable() {
yield takeEvery(CHECK_HOST_IS_DEREGISTERABLE, checkHostDeregisterable);
}

export function* watchHostDeregistered() {
yield takeEvery(HOST_DEREGISTERED, hostDeregistered);
}

export function* watchDeregisterHost() {
yield takeEvery(DEREGISTER_HOST, deregisterHost);
}
55 changes: 55 additions & 0 deletions assets/js/state/sagas/hosts.test.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,39 @@
import MockAdapter from 'axios-mock-adapter';

import { recordSaga } from '@lib/test-utils';
import {
markDeregisterableHosts,
matchHost,
checkHostDeregisterable,
hostDeregistered,
deregisterHost,
} from '@state/sagas/hosts';

import {
cancelCheckHostIsDeregisterable,
setHostListDeregisterable,
removeHost,
setHostDeregistering,
setHostNotDeregistering,
} from '@state/hosts';

import { networkClient } from '@lib/network';
import { notify } from '@state/actions/notifications';
import { hostFactory } from '@lib/test-utils/factories';

const axiosMock = new MockAdapter(networkClient);

describe('Hosts sagas', () => {
beforeEach(() => {
axiosMock.reset();
jest.spyOn(console, 'error').mockImplementation(() => null);
CDimonaco marked this conversation as resolved.
Show resolved Hide resolved
});

afterEach(() => {
/* eslint-disable-next-line */
console.error.mockRestore();
});

it('should mark hosts as deregisterable', async () => {
const passingHost = hostFactory.build({ heartbeat: 'passing' });
const criticalHost = hostFactory.build({ heartbeat: 'critical' });
Expand Down Expand Up @@ -59,4 +80,38 @@ describe('Hosts sagas', () => {

expect(dispatched).toContainEqual(removeHost(payload));
});

it('should send host deregister request', async () => {
const host = hostFactory.build();

axiosMock.onDelete(`/hosts/${host.id}`).reply(204, {});

const dispatched = await recordSaga(deregisterHost, {
payload: host,
});

expect(dispatched).toEqual([
setHostDeregistering(host),
setHostNotDeregistering(host),
]);
});

it('should notify error on host deregistration request', async () => {
const host = hostFactory.build();

axiosMock.onDelete(`/hosts/${host.id}`).reply(404, {});

const dispatched = await recordSaga(deregisterHost, {
payload: host,
});

expect(dispatched).toEqual([
setHostDeregistering(host),
notify({
text: `Error deregistering host ${host.hostname}.`,
icon: '❌',
}),
setHostNotDeregistering(host),
]);
});
});
Loading
Loading