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

Fixes #33078 - Add Traces bulk select & restart #9594

Merged
merged 32 commits into from Sep 10, 2021

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Aug 31, 2021

This adds bulk table row selection to the Traces tab, and allows you to restart traces via remote execution.

  • finish tests

Added a <SelectAllCheckbox> with the following features:

  • Select multiple traces across multiple pages
  • Select all, select page, select none options
  • Grammatically correct text re: how many items are selected
  • The Select all option is not implemented yet, since that will require Katello-wide changes. However, the checkbox is still accurately checked when all items are selected

select_options
one_selected
multi_page_select
all_selected

none_selected

@theforeman-bot
Copy link

Issues: #33078

const fetchItems = useCallback(
params =>
(hostId ? getHostTraces(hostId, params) : null),
[hostId],
);
const response = useSelector(state => selectAPIResponse(state, 'HOST_TRACES'));
const { results, ...meta } = response;
const onRestartApp = () => {
dispatch(resolveHostTraces(hostId, { trace_ids: [...selectedTraces] }, dispatch));
Copy link
Member Author

Choose a reason for hiding this comment

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

the spread syntax here in [...selectedTraces] converts it back to an array :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were doing the onSuccess in the component? Not passing the dispatch up to the API.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Andrewgdewar thanks, updated! turns out this was left over anyway, and not even doing anything 😄

Comment on lines 29 to 38
dispatch({
type: 'TOASTS_ADD',
payload: {
key: id,
message: {
type: 'success',
message: `Restarting ${pluralize(traceCount, 'trace')}.`,
link: {
children: 'View task',
href: `/foreman_tasks/tasks/${id}`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This took me like 2 hours to figure out.. I didn't know about webpack/scenes/Tasks/helpers.js 😭 I will update this

Copy link
Contributor

Choose a reason for hiding this comment

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

K, yeah I'd like to see the dispatch call made into it's own function, if only for clarity of purpose (can be local not exported).

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this same thing can also be accomplished in a single line

handleSuccess: response => renderTaskStartedToast(response.data),

(or close enough to the same thing, at least.) So I'm doing that instead.

@jeremylenz
Copy link
Member Author

To test:

  1. Get the host ID of your host
  2. In rails console,
100.times { |idx| Katello::HostTracer.create!(host_id: 12, application: "tracer_#{idx}", helper: "sudo systemctl restart blah", app_type: "daemon") }

To test end-to-end:

  1. Set up remote execution on your development environment
  2. Run a yum update on your client - you'll probably get some Traces
  3. Restart one or more traces (note: the task might hang, this is unrelated)
  4. Check journalctl -f on the client for some sshd activity
  5. Refresh the page and verify the traces are no longer listed

@jeremylenz
Copy link
Member Author

This PR uses JavaScript Sets to keep track of which traces are selected.

Advantages:

  1. The API for Sets is beautiful - you can do
const mySet = new Set([1,2,3])
mySet.add(3) // no change; 3 is already in the set
mySet.delete(2) // Set { 1, 3 }
mySet.size // 2
mySet.has(2) // false
mySet.has(1) // true
[...mySet] // [1, 3] - returns the Set as an array
mySet.clear() // Set { }
  1. When adding items, you don't have to worry about if they're already in the set
  2. When removing items, you don't have to iterate through the set, filter the set, or find the item to remove
  3. (according to MDN) mySet.has() is faster than myArray.includes() for a set / array of the same size

Disadvantages:

  1. Working with Sets requires that you mutate a value directly. Therefore, it doesn't work well with React useState. For this reason, I used useRef so that you would have a mutable value.
  2. However, changes to a Ref do not mean that props or state have changed, and therefore React will not automatically rerender when our Set changes. To get around this, I made the custom useSet hook which returns a forceRender() function that you must manually run every time you update the Set.

Mostly because of the above, I'm still ambivalent about whether this is a good approach. But I do really love being able to use the Set API for what it's meant for -- keeping track of a unique list of items and modifying that list easily.

@jeremylenz jeremylenz marked this pull request as ready for review September 2, 2021 21:47
@MariSvirik
Copy link

MariSvirik commented Sep 3, 2021

@jeremylenz
Hey, it looks great. I'm really glad we managed to do this.
I have a couple of comments:

select_options

  • No text is needed where there is nothing selected (even if it's no longer selected). It also saves space within a toolbar.

Screenshot 2021-09-03 at 11 09 44

  • Correct order in the list should be always hierarchical: 1. Select none (0) 2. Select page (nb.) 3. Select all (nb)
  • Menu items should always contain a number of selected items so a user is aware of how many they are going to select.

one_selected

  • Don't use 'traces' in the bulk selector even though it may be grammatically correct. No space to spare.
  • Keep partially checked checkbox blue

Screenshot 2021-09-03 at 11 09 51

  • Restart button should be secondary (blue outlined) not red. We almost never use red buttons anymore unless it's an extremely destructive action such as Delete my entire system which always comes with extra confirmation.
    For more information see PF4 guidelines
    Padding
    24px-bulk selector- 24px - search input - 16px - button ----- pagination (numbers) -8px- pagination (carets)-24 px.

@jeremylenz
Copy link
Member Author

jeremylenz commented Sep 3, 2021

Thanks for looking at it @MariSvirik :)

Menu items should always contain a number of selected items so a user is aware of how many they are going to select.

Not sure what is meant by this? Can you provide examples of correct wording for the menu items? "Select all / select page / select none" was directly from the Patternfly doc. update: I see what you mean now. Updating.

Keep partially checked checkbox blue

The partially checked state being non-blue was just the way the Patternfly React component came. I didn't change anything, maybe it's being overridden with our CSS or something.. I'll look into it.

Also, will update to address all your other comments.

>
<Thead>
<Tr>
<Th />
Copy link
Contributor

Choose a reason for hiding this comment

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

Empy row, but why?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's the checkbox column header!

Copy link
Member

Choose a reason for hiding this comment

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

I can't understand in the code how the select all is working..Would you need any updates to the rails traces controller or is it already supported?

Copy link
Member Author

Choose a reason for hiding this comment

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

No updates to the backend were needed. We just keep track on the front end of which trace IDs are selected, and render accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Backend updates will be needed to get 'Select all' functionality working, however. That is left for a future PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't understand in the code how the select all is working

To be clear, the 'Select all' checkbox is correctly rendering as checked, but the 'Select all' menu item doesn't work yet.

The checkbox rendering works just by comparing the number of selected items and the total from pagination metadata.

<DropdownItem key="select-all" component="button" isDisabled onClick={handleSelectAll}>
{__('Select all')}
</DropdownItem>,
<DropdownItem key="select-page" component="button" isDisabled={areAllRowsOnPageSelected} onClick={handleSelectPage}>
Copy link
Contributor

Choose a reason for hiding this comment

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

There was talk of removing items instead of disabling them when not relevant, not sure if @MariaAga had weighed in on that for the dropdown items?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're looking for @MariSvirik :)

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh... yes

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Works great. Tested with dummy traces on the UI only cause I don't know enough about the entire flow..Code LGTM..Good to go from my perspective..🎉 🎉
I'll let @Andrewgdewar turn the button green..

Copy link
Member

@jturel jturel left a comment

Choose a reason for hiding this comment

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

Code + functionality ACK!

Nice work @jeremylenz !

Copy link
Contributor

@Andrewgdewar Andrewgdewar left a comment

Choose a reason for hiding this comment

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

Looks good!

<DropdownItem key="select-all" component="button" isDisabled onClick={handleSelectAll}>
{__('Select all')}
</DropdownItem>,
<DropdownItem key="select-page" component="button" isDisabled={areAllRowsOnPageSelected} onClick={handleSelectPage}>
Copy link
Contributor

Choose a reason for hiding this comment

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

ahh... yes

const perPage = Number(metadata?.per_page ?? foremanPerPage);
const page = Number(metadata?.page ?? 1);
const total = Number(metadata?.subtotal ?? 0);
const { pageRowCount } = getPageStats({ total, page, perPage });
Copy link
Contributor

Choose a reason for hiding this comment

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

happy you pulled this out.

@jeremylenz jeremylenz merged commit 06b3f66 into Katello:master Sep 10, 2021
@jeremylenz
Copy link
Member Author

@MariSvirik I was finally able to spin up Firefox with this, and it confirmed something I suspected:
blue-partial-select

The blue partially-selected checkbox comes from how the browser styles it. In Firefox it looks like the above. The non-blue one is just the way Chrome does it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants