[Cases] Convert remaining hooks to React Query #157254

cnasikas · 2023-05-10T11:19:59Z

Summary

This PR converts the remaining cases' hooks to React Query. It also adds the QueryProvider to the CasesProvider to be able to use the hooks when attaching data to a case from outside cases like ML.

Fixes: #134663

Testing

Verify you can add a comment successfully and the new comment markdown editor is empty after submission
Verify you can add attachments to a new case and an existing case
Add a file to a case. Verify that the entry in the user activity is being shown
Verify that you can push a case to an external service
Verify that you can push a case to an external service when creating a case
Verify that the loading spinner is being shown when you edit a comment in the actions of the comment
Verify that the loading spinner is being shown when you edit a title next to the title
Verify that the loading spinner is being shown when you edit a tag next to the tags section
Verify that the loading spinner is being shown when you edit severity next to the severity selectable
Verify that the status and "Sync alerts" is disabled when changing the status
Verify that the status and "Sync alerts" is disabled when changing the "Sync alerts"
Verify that the "edit pencil" is changed to a loading spinner when changing assignees
Verify you can update a comment
Verify you can update the fields of the case

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-05-10T11:20:02Z

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

elasticmachine · 2023-05-10T11:20:02Z

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

cnasikas · 2023-05-10T11:21:17Z

x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx

@@ -34,7 +34,6 @@ import { triggersActionsUiMock } from '@kbn/triggers-actions-ui-plugin/public/mo
 import { registerConnectorsToMockActionRegistry } from '../../common/mock/register_connectors';
 import { createStartServicesMock } from '../../common/lib/kibana/kibana_react.mock';
 import { waitForComponentToUpdate } from '../../common/test_utils';
-import { useCreateAttachments } from '../../containers/use_create_attachments';


not used anymore by the component.

cnasikas · 2023-05-10T15:06:27Z

@elasticmachine merge upstream

…eact_query_hooks

…fix'

js-jankisalvi

Awesome work!! Code looks much cleaner and easy to understand 🎉

I found small issue regarding loading spinner of severity.
i.e. when I update tag or assignee, I can see respective loading spinners and severity loading spinner as well.

loading_severity.mov

js-jankisalvi · 2023-05-11T08:14:58Z

x-pack/plugins/cases/public/components/user_actions/use_user_actions_handler.tsx

+          commentUpdate: content,
+          version,
+        },
+        {


since we moved handling loadingCommentIds logic out of use_update_comment hook, Will we have to handle loadingCommentIds everywhere we use patchComment?
I am just wondering what will be the impact if someone uses patchComment somewhere else and doesn't add or remove loadingCommentIds.

That's an excellent question and observation. I think that the use_update_comment responsibility is to update a comment and that's it. The hooks provide the isLoading, isFetching, etc status variables which should be enough. It should not care if it is being used in the user activity. On the other hand, the useUserActionsHandler is responsible for providing which comment is loading at the moment as it is the one that puts all the pieces together. Does it make sense?

yeah, it makes sense. Currently it's action handler's responsibility to handle comments. And if it changes in future, we can also take care of loadingCommentIds accordingly.

js-jankisalvi · 2023-05-11T08:23:28Z

x-pack/plugins/cases/public/containers/use_create_attachments.tsx

-    () => () => {
-      isCancelledRef.current = true;
-      abortCtrlRef.current.abort();
+      return createAttachments(attachments, request.caseId, abortCtrlRef.signal);


code looks so clean with react query 😍

js-jankisalvi

Awesome work!! Code looks much cleaner and easy to understand 🎉

I found small issue regarding loading spinner of severity.
i.e. when I update tag or assignee, I can see respective loading spinners and severity loading spinner as well.

loading_severity.mov

js-jankisalvi · 2023-05-11T08:54:38Z

Verify you can add a comment successfully and the new comment markdown editor is empty after submission

I don't see same comment after submission anymore!! 😮 How did you fix this issue?

cnasikas · 2023-05-11T08:55:55Z

I found small issue regarding loading spinner of severity.
i.e. when I update tag or assignee, I can see respective loading spinners and severity loading spinner as well.

Thank you @js-jankisalvi! I can reproduce the issue on main also. If you can also reproduce it on main, can you open an issue so we can address it on another PR?

cnasikas · 2023-05-11T08:59:40Z

I don't see same comment after submission anymore!! 😮 How did you fix this issue?

Lol, I did not do anything. I don't know what is going on 🤷‍♀️. Maybe it needs more tries to reproduce it or our hook was buggy and somehow fixed it now.

js-jankisalvi · 2023-05-11T09:35:50Z

Thank you @js-jankisalvi! I can reproduce the issue on main also. If you can also reproduce it on main, can you open an issue so we can address it on another PR?

Yes, I can reproduce the behaviour in main as well. created issue: #157364

js-jankisalvi

verified all mentioned scenarios locally, looks good 👍 🚀

jonathan-buttner · 2023-05-11T15:04:44Z

Verify you can add a comment successfully and the new comment markdown editor is empty after submission ✅
Verify you can add attachments to a new case and an existing case ✅
Add a file to a case. Verify that the entry in the user activity is being shown ✅
Verify that you can push a case to an external service ✅
Verify that you can push a case to an external service when creating a case ✅
Verify that the loading spinner is being shown when you edit a comment in the actions of the comment ✅
Verify that the loading spinner is being shown when you edit a title next to the title ✅
Verify that the loading spinner is being shown when you edit a tag next to the tags section ✅
Verify that the loading spinner is being shown when you edit severity next to the severity selectable ✅
Verify that the status and "Sync alerts" is disabled when changing the status ✅
Verify that the status and "Sync alerts" is disabled when changing the "Sync alerts" ✅
Verify that the "edit pencil" is changed to a loading spinner when changing assignees ✅
Verify you can update a comment ✅
Verify you can update the fields of the case ✅

jonathan-buttner · 2023-05-11T13:45:13Z

x-pack/plugins/cases/public/components/add_comment/index.tsx

@@ -71,7 +71,7 @@ export const AddComment = React.memo(
      const editorRef = useRef<EuiMarkdownEditorRef>(null);
      const [focusOnContext, setFocusOnContext] = useState(false);
      const { permissions, owner, appId } = useCasesContext();
-      const { isLoading, createAttachments } = useCreateAttachments();
+      const { isLoading, mutate: createAttachments } = useCreateAttachments();


This might have been how it was before these changes, but it seems like other hooks like useUpdateComment leverages an onError. Should the calls of createAttachments do the same?

I think it depends on the scenario. Most of the calls to createAttachments call mutateAsync (which is of the form await createAttachments(...)) and are surrounded by a try/catch. In the scenario of adding a comment I think showing the error toaster (is being automatically by the hook) is enough. Do you have any scenarios in your mind where the onError is needed?

jonathan-buttner · 2023-05-11T14:08:56Z

x-pack/plugins/cases/public/containers/use_post_push_to_service.tsx

-    []
+    {
+      mutationKey: casesMutationsKeys.pushCase,
+      onSuccess: (_, { connector }) => {


Do we need to refresh anything once this succeeds?

The refreshing is done by the components that use the hook. But I agree, it should be done by the hook! I will change it.

jonathan-buttner · 2023-05-11T14:09:53Z

x-pack/plugins/cases/public/containers/use_create_attachments.tsx

    },
-    []
+    {


When this succeeds is the refresh of the comments done elsewhere so it gets displayed?

So far is being done outside of cases so navigating to cases will refresh the page. We may need to invalidate the caching keys for the case view page. I am not sure, what do you think?

cnasikas · 2023-05-12T08:05:03Z

x-pack/plugins/security_solution/cypress/e2e/cases/connectors.cy.ts

-      cy.get(TOASTER).should('have.text', "Created 'New connector'");
-      cy.get(TOASTER).should('have.text', 'Saved external connection settings');
-      cy.get(TOASTER).should('not.exist');


Once in a while, the test fails because the toaster disappears and Cypress can not find it. I removed it to reduce flakiness. We verify that the connector is selected properly which I think is enough.

MadameSheema · 2023-05-12T08:40:25Z

x-pack/plugins/security_solution/cypress/e2e/cases/connectors.cy.ts

@@ -86,6 +88,15 @@ describe('Cases connectors', () => {
    });
  });

+  after(() => {
+    cy.request({


NIT: Let's wrap this into a more readable method like deleteConnector and place it in x-pack/plugins/security_solution/cypress/tasks/common.ts :)

Done in c046e0a

semd

Security changes LGTM. Thanks!

semd · 2023-05-12T08:58:27Z

x-pack/plugins/security_solution/cypress/e2e/cases/connectors.cy.ts

@@ -94,13 +100,10 @@ describe('Cases connectors', () => {

    cy.wait('@createConnector').then(({ response }) => {
      cy.wrap(response?.statusCode).should('eql', 200);
+      connectorId = response?.body.id ?? '';


NIT: This connectorId does not seem to be used, can we remove it?

Correct. I forgot about it. Done 6655997

rylnd

D&R changes LGTM

rylnd · 2023-05-12T14:38:11Z

x-pack/plugins/security_solution/cypress/e2e/timelines/flyout_button.cy.ts

@@ -61,7 +61,8 @@ describe('timeline flyout button', () => {
    cy.get(TIMELINE_BOTTOM_BAR_TOGGLE_BUTTON).should('have.focus');
  });

-  it('the `(+)` button popover menu owns focus', () => {
+  // FLAKY: https://github.com/elastic/kibana/issues/153771


Thank you for the diligence here 👍

kibana-ci · 2023-05-12T15:29:42Z

💚 Build Succeeded

Buildkite Build
Commit: fff79e1

Metrics [docs]

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`	82	84	+2

Async chunks

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

id	before	after	diff
`cases`	376.7KB	374.5KB	-2.2KB

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
`cases`	31	30	-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`	166.6KB	166.2KB	-342.0B

Unknown metric groups

API count

id	before	after	diff
`cases`	99	101	+2

ESLint disabled line counts

id	before	after	diff
`cases`	57	55	-2
`enterpriseSearch`	19	21	+2
`securitySolution`	400	404	+4
total			+4

Total ESLint disabled count

id	before	after	diff
`cases`	75	73	-2
`enterpriseSearch`	20	22	+2
`securitySolution`	480	484	+4
total			+4

History

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

cc @cnasikas

## Summary This PR converts the remaining cases' hooks to React Query. It also adds the `QueryProvider` to the `CasesProvider` to be able to use the hooks when attaching data to a case from outside cases like ML. Fixes: #134663 ## Testing - Verify you can add a comment successfully and the new comment markdown editor is empty after submission - Verify you can add attachments to a new case and an existing case - Add a file to a case. Verify that the entry in the user activity is being shown - Verify that you can push a case to an external service - Verify that you can push a case to an external service when creating a case - Verify that the loading spinner is being shown when you edit a comment in the actions of the comment - Verify that the loading spinner is being shown when you edit a title next to the title - Verify that the loading spinner is being shown when you edit a tag next to the tags section - Verify that the loading spinner is being shown when you edit severity next to the severity selectable - Verify that the status and "Sync alerts" is disabled when changing the status - Verify that the status and "Sync alerts" is disabled when changing the "Sync alerts" - Verify that the "edit pencil" is changed to a loading spinner when changing assignees - Verify you can update a comment - Verify you can update the fields of the case ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cnasikas added 5 commits May 10, 2023 11:44

Convert useCreateAttachments

615a0fd

Convert usePostPushToService

5b96c98

Convert useUpdateCase

42ec0ab

Convert useUpdateComment

b41526b

Fix tests

7f5f203

cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.9.0 labels May 10, 2023

cnasikas self-assigned this May 10, 2023

cnasikas requested a review from a team as a code owner May 10, 2023 11:20

cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label May 10, 2023

cnasikas commented May 10, 2023

View reviewed changes

Fixes

81e4cee

cnasikas force-pushed the react_query_hooks branch from 7be43f8 to 81e4cee Compare May 10, 2023 11:29

cnasikas added 2 commits May 10, 2023 14:56

Add query client to the cases context

55375b3

Fix types

e10b9b7

cnasikas mentioned this pull request May 10, 2023

[Cases] Fix flakiness in all_cases_list.test.tsx #157285

Merged

2 tasks

kibanamachine and others added 4 commits May 10, 2023 11:06

Merge branch 'main' into react_query_hooks

362a6a1

Merge branch 'main' into react_query_hooks

03e46c3

Fix cypress tests

27b08a4

Merge branch 'react_query_hooks' of github.com:cnasikas/kibana into r…

b2f1105

…eact_query_hooks

cnasikas requested review from a team as code owners May 11, 2023 06:50

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

e5dc03a

…fix'

kibanamachine requested a review from a team as a code owner May 11, 2023 06:55

js-jankisalvi reviewed May 11, 2023

View reviewed changes

js-jankisalvi approved these changes May 11, 2023

View reviewed changes

Merge branch 'main' into react_query_hooks

e431f0d

jonathan-buttner approved these changes May 11, 2023

View reviewed changes

cnasikas added 3 commits May 12, 2023 09:31

Merge branch 'main' into react_query_hooks

b5efd8b

Refresh when pushing

4921e9f

Delete connector after the end of the test

41e16dc

cnasikas commented May 12, 2023

View reviewed changes

MadameSheema reviewed May 12, 2023

View reviewed changes

Use function from library

c046e0a

cnasikas requested a review from MadameSheema May 12, 2023 08:59

semd approved these changes May 12, 2023

View reviewed changes

Remove unused variable

6655997

cnasikas enabled auto-merge (squash) May 12, 2023 09:04

cnasikas added 2 commits May 12, 2023 16:56

Bring toaster variable back

49069b6

Skip flaky test

fff79e1

rylnd approved these changes May 12, 2023

View reviewed changes

cnasikas merged commit 0bbef23 into elastic:main May 12, 2023

cnasikas deleted the react_query_hooks branch May 12, 2023 15:29

kibanamachine added the backport:skip This commit does not require backporting label May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Convert remaining hooks to React Query #157254

[Cases] Convert remaining hooks to React Query #157254

cnasikas commented May 10, 2023 •

edited

Loading

elasticmachine commented May 10, 2023

elasticmachine commented May 10, 2023

cnasikas May 10, 2023

cnasikas commented May 10, 2023

js-jankisalvi left a comment

js-jankisalvi May 11, 2023

cnasikas May 11, 2023

js-jankisalvi May 11, 2023

js-jankisalvi May 11, 2023

js-jankisalvi left a comment

js-jankisalvi commented May 11, 2023

cnasikas commented May 11, 2023

cnasikas commented May 11, 2023

js-jankisalvi commented May 11, 2023

js-jankisalvi left a comment

jonathan-buttner commented May 11, 2023

jonathan-buttner May 11, 2023

cnasikas May 11, 2023

jonathan-buttner May 11, 2023

cnasikas May 11, 2023

jonathan-buttner May 11, 2023

cnasikas May 11, 2023

cnasikas May 12, 2023

MadameSheema May 12, 2023

cnasikas May 12, 2023

semd left a comment

semd May 12, 2023

cnasikas May 12, 2023 •

edited

Loading

rylnd left a comment

rylnd May 12, 2023

kibana-ci commented May 12, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

[Cases] Convert remaining hooks to React Query #157254

[Cases] Convert remaining hooks to React Query #157254

Conversation

cnasikas commented May 10, 2023 • edited Loading

Summary

Testing

Checklist

elasticmachine commented May 10, 2023

elasticmachine commented May 10, 2023

Choose a reason for hiding this comment

cnasikas commented May 10, 2023

js-jankisalvi 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

js-jankisalvi left a comment

Choose a reason for hiding this comment

js-jankisalvi commented May 11, 2023

cnasikas commented May 11, 2023

cnasikas commented May 11, 2023

js-jankisalvi commented May 11, 2023

js-jankisalvi left a comment

Choose a reason for hiding this comment

jonathan-buttner commented May 11, 2023

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

semd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas May 12, 2023 • edited Loading

Choose a reason for hiding this comment

rylnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented May 12, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas commented May 10, 2023 •

edited

Loading

cnasikas May 12, 2023 •

edited

Loading