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

[RAM] Show Cases column on the alerts table #150963

Merged
merged 21 commits into from
Feb 21, 2023

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Feb 11, 2023

Summary

This PR adds the cases column to the alerts table. Assumptions:

  • I will not change the label of the case column. I will do it on another PR.
  • The column will not be part of the default columns. The user has to add it from the fields.
  • If there are multiple cases they will be shown as a comma-separated list. It will change later to include a "show more" button or similar to show only the latest case in the column.
  • Due to circular dependencies, Cases types and utility functions were not imported. I duplicate the code. I opened two issues ([Cases] Create package for case API types #151648, [Cases] Create package for common utility functions and constants #151649) to create packages that will contain the case types and utility functions.

Issue: #146864

Screenshot 2023-02-17 at 7 37 25 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

Release notes

Adds the Cases column to the alerts table. The column shows, for each alert, all cases the alert is attached to.

@cnasikas cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature Feature:Alerting/RulesManagement Issues related to the Rules Management UX labels Feb 11, 2023
@cnasikas cnasikas self-assigned this Feb 11, 2023
@cnasikas cnasikas mentioned this pull request Feb 11, 2023
8 tasks
@cnasikas cnasikas force-pushed the cases_alerts_table branch 2 times, most recently from 71edc38 to ed8c32d Compare February 15, 2023 18:37
@@ -34,7 +34,8 @@
"esUiShared",
"kibanaReact",
"kibanaUtils",
"actions"
"actions",
"cases"
Copy link
Member Author

@cnasikas cnasikas Feb 16, 2023

Choose a reason for hiding this comment

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

I added Cases in the required bundles to be able to import some constants. The bundle size of the triggers_actions_ui plugin increased by 200 bytes. I think it is acceptable. If you think otherwise let me know so I can hardcode the needed values.

Screenshot 2023-02-16 at 11 31 28 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

I see cases plugin has triggers_actions_ui as plugin dependency, could there be a cyclic dependency issue? Otherwise, it's ok for me

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 don't think there is an issue with bundles. Only with required plugins. I will verify with operations to be sure. If there is a problem I will duplicate the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcger You were right. CI was complaining about circular dependencies. I will create an issue to create a package so we can import the cases types from there. In this PR I duplicate the code.

* 2.0.
*/

export const triggersActionsUiQueriesKeys = {
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 followed the same pattern we do in Cases. Inspired by this article https://tkdodo.eu/blog/effective-react-query-keys#use-query-key-factories

import { act } from 'react-dom/test-utils';
import { AlertsFlyout } from './alerts_flyout';
import { AlertsField } from '../../../../types';
import { Alert, AlertsField } from '../../../../types';
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 created the following types

export type Alert = EcsFieldsResponse;
export type Alerts = Alert[];

and use them everywhere. I think is clearer.

data,
});

if (isSystemCell(_props.columnId)) {
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 created a factory for our internal cells. The factory will get the columnId and all required props and render the corresponding cell component.

if (isSystemCell(_props.columnId)) {
return (
<SystemCellFactory
alert={alert}
Copy link
Member Author

Choose a reason for hiding this comment

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

Passing the alert makes the code easier instead of passing the data


const AlertsTableState = (props: AlertsTableStateProps) => {
return (
<QueryClientProvider client={alertsTableQueryClient}>
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 alerts table is wrapped by the React Query provider.

@@ -240,6 +270,7 @@ const AlertsTableState = ({
const tableProps = useMemo(
() => ({
alertsTableConfiguration,
casesData: { cases: cases ?? new Map(), isLoading: isLoadingCases },
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 preferred to group all case props into one object. Let me know if you need me to do something else.


export const systemCells: SystemCellId[] = [ALERT_STATUS, ALERT_CASE_IDS];

const SystemCellFactoryComponent: React.FC<CellComponentProps> = (props) => {
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 created a factory for our internal cells. The factory will get the columnId and all required props and render the corresponding cell component.


const SystemCellFactoryComponent: React.FC<CellComponentProps> = (props) => {
const { columnId } = props;
const cellComponents: SystemCellComponentMap = useMemo(
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it is better to put it outside of the component.

},
{
enabled: caseIds.length > 0,
select: transformCases,
Copy link
Member Author

@cnasikas cnasikas Feb 16, 2023

Choose a reason for hiding this comment

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

The backend returns an array of cases. We transform it into a Map to avoid looping the cases array each time we need to find the case information.

Copy link
Contributor

@jcger jcger left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for those tests!

@@ -34,7 +34,8 @@
"esUiShared",
"kibanaReact",
"kibanaUtils",
"actions"
"actions",
"cases"
Copy link
Contributor

Choose a reason for hiding this comment

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

I see cases plugin has triggers_actions_ui as plugin dependency, could there be a cyclic dependency issue? Otherwise, it's ok for me

coreStart: TriggersAndActionsUiServices;
}

export const createAppMockRenderer = (): AppMockRenderer => {
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 moved it one level up because I need it in the section folder.

@cnasikas cnasikas marked this pull request as ready for review February 20, 2023 11:19
@cnasikas cnasikas requested a review from a team as a code owner February 20, 2023 11:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@cnasikas cnasikas requested a review from jcger February 20, 2023 11:23
@cnasikas cnasikas added the release_note:feature Makes this part of the condensed release notes label Feb 20, 2023
Copy link
Contributor

@jcger jcger left a comment

Choose a reason for hiding this comment

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

not done yet but I already wanted to let you know that I did some QA and saw that after adding an alert to a case, the table isn't updated and also even if clicking on refresh, it doesn't update. I have to hard reload to get the data

Screenshot.1.mov

.filter((theCase): theCase is Case => theCase != null);

if (validCases.length === 0) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen that if there is no linked case, the cell is just empty. I guess it's happening because of this return and I'm wondering if we should render '--' or even call the DefaultRenderer to keep consistency with other empty cell values

See difference between empty values of other cells compared to case

Screenshot

Copy link
Member Author

@cnasikas cnasikas Feb 20, 2023

Choose a reason for hiding this comment

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

You are right. I will call the DefaultRenderer to be consistent.

wrapper: appMockRender.AppWrapper,
});

act(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you really need to call act here? I tried without it I there is no warning or error message

Copy link
Member Author

@cnasikas cnasikas Feb 20, 2023

Choose a reason for hiding this comment

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

The examples in the documentation of the library use act when you call a function. In react testing library it does not need it but it seems to be needed in the react testing hook library.

Copy link
Contributor

@jcger jcger left a comment

Choose a reason for hiding this comment

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

nice!

@cnasikas cnasikas enabled auto-merge (squash) February 21, 2023 08:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
triggersActionsUi 520 540 +20

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
cases 71 75 +4
triggersActionsUi 530 531 +1
total +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 912.9KB 919.6KB +6.8KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
triggersActionsUi 49 50 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cases 130.2KB 130.4KB +163.0B
Unknown metric groups

API count

id before after diff
cases 87 92 +5
triggersActionsUi 559 560 +1
total +6

ESLint disabled in files

id before after diff
triggersActionsUi 5 6 +1

Total ESLint disabled count

id before after diff
triggersActionsUi 128 129 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @cnasikas

@cnasikas cnasikas merged commit 0a8c5f5 into elastic:main Feb 21, 2023
@cnasikas cnasikas deleted the cases_alerts_table branch February 21, 2023 10:07
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 21, 2023
@cnasikas
Copy link
Member Author

not done yet but I already wanted to let you know that I did some QA and saw that after adding an alert to a case, the table isn't updated and also even if clicking on refresh, it doesn't update. I have to hard reload to get the data

We decided offline that we will address it on another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting/RulesManagement Issues related to the Rules Management UX Feature:Cases Cases feature release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants