[SIEM] [Cases] Delete, comment, and reducer updates #59616
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stephmilovic
added
WIP
|
Pinging @elastic/siem (Team:SIEM) |
stephmilovic
added
release_note:skip
stephmilovic
changed the title
This was referenced Mar 9, 2020
| @@ -7,11 +7,3 @@ | |||
| export const CASES_URL = `/api/cases`; | |||
| export const DEFAULT_TABLE_ACTIVE_PAGE = 1; | |||
| export const DEFAULT_TABLE_LIMIT = 5; | |||
| export const FETCH_FAILURE = 'FETCH_FAILURE'; | |||
There was a problem hiding this comment.
why do we switch from variables to inline strings?
There was a problem hiding this comment.
to make the Action type in the hooks better
There was a problem hiding this comment.
cannot we use ActionCreator from typescript-fsa as in the rest of the app?
There was a problem hiding this comment.
cnasikas
reviewed
Mar 10, 2020
cnasikas
reviewed
Mar 10, 2020
cnasikas
reviewed
Mar 10, 2020
cnasikas
reviewed
Mar 10, 2020
x-pack/legacy/plugins/siem/public/containers/case/use_delete_cases.tsx
Outdated
Show resolved
Hide resolved
cnasikas
reviewed
Mar 10, 2020
| @@ -16,6 +16,7 @@ export interface Comment { | |||
| export interface Case { | |||
| id: string; | |||
| comments: Comment[]; | |||
| commentIds: string[]; | |||
There was a problem hiding this comment.
Why we need the comment ids in a different array? Aren't included in comments array?
There was a problem hiding this comment.
@XavierM had reasons
There was a problem hiding this comment.
@cnasikas, you will only get the comments through the API if you set the query parameter includeComments but we still need to know how many comments are attached to a case to show it in the table. So to avoid a payload of data to just know the number of comment we are using commentIds.
There was a problem hiding this comment.
@XavierM Sorry to insist, but if this is the only use case why the API does not return the count of the comments? Do you think there will be another use case in the future where ids will be needed?
There was a problem hiding this comment.
I agree with you but since we do not have aggregation on the saved object I did that to avoid query every comment for every case. Might not be the most robust way of doing it. However, it will be faster. When we have aggregation in saved_object, we will be able to use that to just get the count like you said
cnasikas
reviewed
Mar 10, 2020
| @@ -130,3 +130,25 @@ export const patchComment = async ( | |||
| await throwIfNotOk(response.response); | |||
| return convertToCamelCase<CommentResponse, Comment>(decodeCommentResponse(response.body)); | |||
| }; | |||
|
|
|||
| export const deleteCases = async (caseIds: string[]): Promise<boolean> => { | |||
There was a problem hiding this comment.
What do you think about passing a signal (AbortController) so we can pass it to fetch?
There was a problem hiding this comment.
export const deleteCases = async ({caseIds: string[], signal: AbortSignal}): Promise<boolean> => {
const response = await KibanaServices.get().http.fetch<string>(`${CASES_URL}`, {
method: 'DELETE',
asResponse: true,
signal,
query: { ids: JSON.stringify(caseIds) },
});
await throwIfNotOk(response.response);
return response.body === 'true' ? true : false;
};
There was a problem hiding this comment.
related to this comment #59616 (comment)
cnasikas
reviewed
Mar 10, 2020
cnasikas
reviewed
Mar 10, 2020
| handleToggleModal: () => void; | ||
| } | ||
|
|
||
| export const useDeleteCases = (): UseDeleteCase => { |
There was a problem hiding this comment.
What do you think about creating an AbortController, pass it to the API calls and abort in the return function?
There was a problem hiding this comment.
const dispatchDeleteCases = useCallback(async (caseIds: string[]) => {
let cancel = false;
**const abortCtrl = new AbortController();**
try {
dispatch({ type: 'FETCH_INIT' });
await deleteCases(**{ caseIds, signal: abortCtrl.signal }**);
if (!cancel) {
dispatch({ type: 'FETCH_SUCCESS', payload: true });
}
} catch (error) {
if (!cancel) {
errorToToaster({
title: i18n.ERROR_TITLE,
error: error.body && error.body.message ? new Error(error.body.message) : error,
dispatchToaster,
});
dispatch({ type: 'FETCH_FAILURE' });
}
}
return () => {
cancel = true;
**abortCtrl.abort();**
};
}, []);
There was a problem hiding this comment.
@XavierM thoughts?
There was a problem hiding this comment.
That's correct, I notice that too but I was thinking that we can change it to a specific PR later on and create an issue to track it. Therefore we can move forward to this PR and staying true to just do deletions and showing comments. I am saying that because that's not the only places we will have to change it and try to get better to avoid big PR. does that make sense?
cnasikas
reviewed
Mar 10, 2020
cnasikas
reviewed
Mar 10, 2020
10 tasks
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "before each" hook for "should return 200 when copying to space with conflicts without overwriting"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "after each" hook for "should return 200 when copying to space with conflicts without overwriting"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "before each" hook for "should return 200 when copying to space with conflicts without overwriting"Standard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
3 tasks
stephmilovic
added a commit
to stephmilovic/kibana
that referenced
this pull request
Mar 11, 2020
stephmilovic
added a commit
that referenced
this pull request
Mar 12, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Disclaimer: new icons are not yet in Kibana.
magnetis used as a placeholderThis PR resolves the following All Cases issues from #59041:
commentCountcolumnIt also resolves the following Case View issues from #58526:
UI for delete commentnot in scopeAnd miscellaneously because I like to keep you on your toes...