Events page refactoring with entity data table usage #20831
Conversation
maxiadlovskii
changed the title
graylog2-web-interface/src/components/events/events/EventActions.tsx
Outdated
Show resolved
Hide resolved
…ns.tsx Co-authored-by: Zack King <[email protected]>
fix linter error
There was a problem hiding this comment.
I like this change a lot! So far I have not found a bug, but I found some problems with the implemented types.
Besides that I only added a bunch of suggestions for code improvements.
One general thing. Imo it is currently hard to understand that you can view further details by clicking on the title. Before you could click everywhere in the row, which made it at least a bit easier to understand.
Maybe we can also show a button in the action column like Details, which tollges the section.
I also think we should improve the styling when all details exist as columns and there the section is empty.
Thinking about it, maybe we can discuss always showing all details in the section, even when they exist as columns? I would like to compare both solutions. I can imagine it will be easier to understand and be beneficial to have a place where you can always find all information about an event, displayed in a way that is very easy to read.
| @@ -0,0 +1,5 @@ | |||
| type = "a" | |||
| message = "Implement commont entity data table with sorting and filtering for events page" | |||
There was a problem hiding this comment.
There is a typo in common.
| <>{isPermitted(permissions, | ||
| `eventdefinitions:edit:${eventDefinitionContext.id}`) | ||
| ? <Link to={Routes.ALERTS.DEFINITIONS.edit(eventDefinitionContext.id)}>{eventDefinitionContext.title}</Link> | ||
| : eventDefinitionContext.title} |
There was a problem hiding this comment.
Just a personal preference, but imo this is easier to read:
if (isPermitted(permissions, `eventdefinitions:edit:${eventDefinitionContext.id}`)) {
return (
<Link to={Routes.ALERTS.DEFINITIONS.edit(eventDefinitionContext.id)}>
{eventDefinitionContext.title}
</Link>
)
}
return <>{eventDefinitionContext.title}</>
| import useMetaDataContext from 'components/common/EntityDataTable/hooks/useMetaDataContext'; | ||
| import { Timestamp } from 'components/common'; | ||
|
|
||
| const EventDefinitionRenderer = ({ eventDefinitionId, permissions }: { eventDefinitionId: string, permissions: Immutable.List<string> }) => { |
There was a problem hiding this comment.
Isn't every component a renderer? what about EventDefinitionTitle?
There was a problem hiding this comment.
ah, it's because we called it columnRenderers. In similar places we call it EventDefinitionTitle or EventDefinitionCell
|
|
||
| const PriorityRenderer = ({ priority }: { priority: number }) => <PriorityName priority={priority} />; | ||
|
|
||
| const FieldsRenderer = ({ fields }: { fields: Record<string, string> }) => ( |
There was a problem hiding this comment.
I learned recently that you should only use a Record when it has fixed keys. Otherwise it is cleaner to use an index signature, like { [fieldName: string]: string }.
| renderCell: (_eventDefinitionId: string) => <EventDefinitionRenderer permissions={permissions} eventDefinitionId={_eventDefinitionId} />, | ||
| }, | ||
| priority: { | ||
| renderCell: (_priority: number) => <PriorityRenderer priority={_priority} />, |
There was a problem hiding this comment.
Here we could render PriorityName directly.
| <MetaDataProvider<EventsAdditionalData> meta={meta}> | ||
| <EventDetailsTable attributesList={attributesList} event={event} meta={meta} /> | ||
| <EventActions event={event} wrapper={ActionsWrapper} /> | ||
| </MetaDataProvider> |
There was a problem hiding this comment.
Do I understand it correctly that we are overwriting the meta data context, provided by the entity data table, here? So depending on where in the table you consume the context you get different data? Can't we just get rid of the context here and just provide the eventDefinition as a prop?
| const { entityActions, expandedSections } = useTableElements({ defaultLayout }); | ||
|
|
||
| return ( | ||
| <PaginatedEntityTable<Event> humanName="events" |
There was a problem hiding this comment.
Here we can define a type for meta.
| export const keyFn = (searchParams: SearchParams) => ['events', 'search', searchParams]; | ||
|
|
||
| export type EventsAdditionalData = { | ||
| context: {event_definitions?: EventDefinitionContext, streams?: EventDefinitionContext}, |
There was a problem hiding this comment.
If I am not mistaken this type is wrong. It reads like this is just one event definition while it is actually an object where the keys are the event definition ids and the values are the vent definitions.
Can you please check why this did not result in a TS error when using the context.
| }, | ||
| event_definition_id: { | ||
| renderCell: (_eventDefinitionId: string) => <EventDefinitionRenderer permissions={permissions} eventDefinitionId={_eventDefinitionId} />, | ||
| }, |
There was a problem hiding this comment.
Imo we can define a relating relative width here and make the column a bit smaller and maybe give it a slightly larger min width.
| </tbody> | ||
| </Table> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
I saw that we are rendering a placeholder for e.g. the remediation steps, when the value is empty is empty, but not for the additional fields. Can we unify this?
Also can you remove the padding for the additional fields value?
Description
This PR implements an events page on a general perspective with Entity Data Table usage
Now we can choose which columns to show. The rest is hidden under the expanded row
Event actions are in the action column now
We have the option to filtrate events by type and timestamp
And now it's possible to sort events by some of the fields
The changes also touch the default event details of the events widget
Motivation and Context
As the current implementation of the Events page uses the legacy code, we need to do refactoring first before implementing the new functionality. We need to use Entity Data Table on the page as we use it on other pages. This would allow us to use common approaches. We have to do refactoring for the general approach first and then reuse it on a security perspective as well.
fix: #20881
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
/jpd https://github.com/Graylog2/graylog-plugin-enterprise/pull/9020