Add 'Clean up' button to Host Details page #1623

jamie-suse · 2023-07-12T12:30:31Z

CleanUpHostDetails.webm

Description

Adds the 'Clean up' button to the Host Details page. Allows the user to deregister a host.

How was this tested?

Added frontend unit tests to test the behaviour of the button.

arbulu89

Looks great!
Only 2 things.

Could you add a gif on how it works?
Change fireEvent by userEvent
We can merge after that XD

arbulu89 · 2023-07-12T12:32:37Z

assets/js/components/HostDetails/HostDetails.jsx

+
+  const dispatch = useDispatch();
+
+  const openDeregistrationModal = () => {


I think we can use directly the setCleanUpModalOpen(true) , right?
I know that you did to be consistent with the other.
Anyway, up to you, not a big deal hehe

arbulu89 · 2023-07-12T12:35:17Z

assets/js/components/HostDetails/HostDetails.test.jsx

@@ -1,5 +1,6 @@
 import React from 'react';
-import { screen } from '@testing-library/react';
+import { screen, fireEvent } from '@testing-library/react';


Use userEvent instead of fireEvent. It is the recommended library.
You will need to setup the user, etc
Check other test files with this approach

arbulu89 · 2023-07-12T12:36:04Z

assets/js/components/HostDetails/HostDetails.test.jsx

@@ -66,4 +67,115 @@ describe('HostDetails component', () => {
      )
    ).toBeInTheDocument();
  });
+
+  it("should not display 'Clean up' button when host is not deregisterable", () => {


double quotes? I guess the ci will complain about this hehe
I would remove the single quotes in any case from Clean up and put them in the test string

arbulu89 · 2023-07-12T12:40:00Z

assets/js/components/HostDetails/HostDetails.test.jsx

@@ -66,4 +67,115 @@ describe('HostDetails component', () => {
      )
    ).toBeInTheDocument();
  });
+
+  it("should not display 'Clean up' button when host is not deregisterable", () => {


We could maybe add a deregistration describe to group the tests about this topic.
Again, not strong opinions

arbulu89 · 2023-07-13T07:32:22Z

assets/js/components/HostDetails/HostDetails.jsx

+
+  const cleanUpHost = () => {
+    setCleanUpModalOpen(false);
+    dispatch(deregisterHost(host));


We should get the host as argument in the function.
If we use the global variable, we have the risk of having strange things.
For example, if the function was used when host is not loaded, things wouldn't work.

arbulu89

Okey!
Once we have the navigation sorted out the code looks good

arbulu89 · 2023-07-17T15:36:36Z

assets/js/components/HostDetails/HostDetails.jsx

+
+  const cleanUpHost = (selectedHost) => {
+    setCleanUpModalOpen(false);
+    dispatch(deregisterHost({ host: selectedHost, navigate }));


We careful with this!
If you change the payload format for the function, you need to change it every place that it is used, the host list view in this case.
I would suggest to go with deregisterHost({ ...selectedHost, navigate }), but if you prefer this way don't forget to change the host list view call as well

Ah yes this is true. I thought tests would catch this in HostsList.test.jsx, but it didn't. Is there a way to have tests detect this kind of change? 👀

Most probably not...

arbulu89 · 2023-07-18T08:10:45Z

assets/js/state/sagas/hosts.js

@@ -52,6 +52,7 @@ export function* deregisterHost({ payload }) {
  yield put(setHostDeregistering(payload));
  try {
    yield call(del, `/hosts/${payload.id}`);
+    payload.navigate?.('/hosts');


Never seen this format before.

Fancy Optional chaining with function calls.

Basically, if payload.navigate doesn't exist, then when we try to call payload.navigate(), we instead just returned undefined, acting like a NOOP.

If we didn't do this, then when calling payload.navigate(), we would get the error thrown: Uncaught TypeError: payload.navigate is not a function

We need to do this because we only want to navigate back to HostsList from HostDetails. It doesn't really make sense to do it from HostsList because we are already there and it is updated by websocket events. So as a result we don't pass in navigate to the payload to make this a NOOP.

dottorblaster · 2023-07-18T08:12:52Z

assets/js/state/sagas/hosts.js

@@ -52,6 +52,7 @@ export function* deregisterHost({ payload }) {
  yield put(setHostDeregistering(payload));
  try {
    yield call(del, `/hosts/${payload.id}`);
+    payload.navigate?.('/hosts');


Suggested change

payload.navigate?.('/hosts');

const { navigate } = payload;

navigate('/hosts');

But if payload.navigate doesn't exist and then we try to call navigate() we will get Uncaught TypeError: navigate is not a function.

With optional chaining if navigate doesn't exist, the call to navigate() just returns undefined, essentially a NOOP.

We need to do this because we only want to navigate back to HostsList from HostDetails. It doesn't really make sense to do it from HostsList because we are already there and it is updated by websocket events. So as a result we don't pass in navigate to the payload to make this a NOOP.

CDimonaco · 2023-07-18T09:09:10Z

assets/js/state/sagas/hosts.js

@@ -52,6 +52,7 @@ export function* deregisterHost({ payload }) {
  yield put(setHostDeregistering(payload));
  try {
    yield call(del, `/hosts/${payload.id}`);
+    payload.navigate?.('/hosts');


I agree to remove the optional chaining and unify the flows, we need to redirect always even on the host list, it's a no-op for the router, and we avoid to pass or not pass parameters, we don't have types and this cause only confusion imho.

Unify the things, pass the navigate always and remove the optional chaining

This reverts commit 2197e42.

Include a mocked `navigate()` function in action payload

jamie-suse · 2023-07-18T12:29:05Z

assets/js/state/sagas/hosts.test.js

@@ -84,15 +84,15 @@ describe('Hosts sagas', () => {
  it('should send host deregister request', async () => {
    const host = hostFactory.build();

+    const mockNavigate = (route) => route;


As we have chosen to always provide a function navigate() to the action payload for the deregisterHost() Saga, now in the test we need to provide a mocked navigate function. Is this function fine?

You could have just a () => {} but this is fine as well

CDimonaco

LGTM

assets/js/components/HostDetails/HostDetails.jsx

CDimonaco · 2023-07-18T14:00:17Z

assets/js/components/HostsList.jsx

@@ -66,7 +66,9 @@ function HostsList() {

  const cleanUpHost = (host) => {
    setCleanUpModalOpen(false);
-    dispatch(deregisterHost({ ...host, navigate }));
+
+    const { id, hostname } = host;


Pls destructure on the function argument level 👁️

Add 'Clean up' button to Host Details page

eb664ed

jamie-suse added enhancement New feature or request javascript Pull requests that update Javascript code labels Jul 12, 2023

jamie-suse requested review from rtorrero, CDimonaco, arbulu89 and jagabomb July 12, 2023 12:30

arbulu89 requested changes Jul 12, 2023

View reviewed changes

arbulu89 reviewed Jul 12, 2023

View reviewed changes

jamie-suse added 5 commits July 12, 2023 14:46

Change test descriptions

bd56859

Organise tests inside describe blocks

a720d46

Use userEvent instead of fireEvent

07a0950

Reify superfluous abstract function openDeregistrationModal()

2cfd98d

Use DEREGISTER_HOST constant instead of string literal

195ff87

arbulu89 reviewed Jul 13, 2023

View reviewed changes

arbulu89 requested changes Jul 13, 2023

View reviewed changes

jamie-suse added 2 commits July 17, 2023 14:50

Don't use global scope in cleanUpHost() callback

4049740

Navigate to Hosts List after deregistering from Host Details view

7a89b5a

arbulu89 requested changes Jul 17, 2023

View reviewed changes

Use deregisterHost() Saga consistently

6b4a209

arbulu89 approved these changes Jul 18, 2023

View reviewed changes

dottorblaster requested changes Jul 18, 2023

View reviewed changes

CDimonaco requested changes Jul 18, 2023

View reviewed changes

jamie-suse added 4 commits July 18, 2023 13:44

Always call navigate() in deregisterHost() Saga

974be7f

Update test

2197e42

Revert "Update test"

ede002a

This reverts commit 2197e42.

Update test for deregisterHost() Saga

e5580b8

Include a mocked `navigate()` function in action payload

jamie-suse commented Jul 18, 2023

View reviewed changes

jamie-suse requested review from dottorblaster and arbulu89 July 18, 2023 12:38

jamie-suse requested a review from CDimonaco July 18, 2023 12:38

dottorblaster approved these changes Jul 18, 2023

View reviewed changes

jamie-suse added 2 commits July 18, 2023 14:42

Change mockNavigate() to NOOP

c6fea89

Send minimal set of data in deregisterHost() Saga payload

0b00696

CDimonaco approved these changes Jul 18, 2023

View reviewed changes

Perform destructuring on argument of cleanUpHost()

b203e7a

CDimonaco approved these changes Jul 18, 2023

View reviewed changes

jamie-suse merged commit 4180201 into main Jul 18, 2023
16 checks passed

jamie-suse deleted the deregister-host-details branch July 18, 2023 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add 'Clean up' button to Host Details page #1623

Add 'Clean up' button to Host Details page #1623

jamie-suse commented Jul 12, 2023 •

edited

Loading

arbulu89 left a comment

arbulu89 Jul 12, 2023

arbulu89 Jul 12, 2023

arbulu89 Jul 12, 2023

arbulu89 Jul 12, 2023

CDimonaco Jul 12, 2023

arbulu89 Jul 13, 2023

arbulu89 left a comment

arbulu89 Jul 17, 2023

jamie-suse Jul 17, 2023

arbulu89 Jul 18, 2023

arbulu89 Jul 18, 2023

jamie-suse Jul 18, 2023

jamie-suse Jul 18, 2023

dottorblaster Jul 18, 2023

dottorblaster Jul 18, 2023

jamie-suse Jul 18, 2023

CDimonaco Jul 18, 2023

jamie-suse Jul 18, 2023

dottorblaster Jul 18, 2023 •

edited

Loading

jamie-suse Jul 18, 2023

CDimonaco left a comment

CDimonaco Jul 18, 2023


		const dispatch = useDispatch();

		const openDeregistrationModal = () => {

	payload.navigate?.('/hosts');
	const { navigate } = payload;
	navigate('/hosts');

Add 'Clean up' button to Host Details page #1623

Add 'Clean up' button to Host Details page #1623

Conversation

jamie-suse commented Jul 12, 2023 • edited Loading

Description

How was this tested?

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDimonaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamie-suse commented Jul 12, 2023 •

edited

Loading

dottorblaster Jul 18, 2023 •

edited

Loading