-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[SIEM] [Cases] Delete, comment, and reducer updates #59616
Changes from 10 commits
b9dd690
354d94b
27a08cc
20cfc98
cc4d506
f2abf92
7e98271
65d0a36
b7ce857
9b27a3b
48d49ff
4616822
5b66897
9965750
f625f5c
3688b32
ff6ff36
9dd9e8e
7f46690
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we switch from variables to inline strings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cannot we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
export const FETCH_INIT = 'FETCH_INIT'; | ||
export const FETCH_SUCCESS = 'FETCH_SUCCESS'; | ||
export const POST_NEW_CASE = 'POST_NEW_CASE'; | ||
export const POST_NEW_COMMENT = 'POST_NEW_COMMENT'; | ||
export const UPDATE_FILTER_OPTIONS = 'UPDATE_FILTER_OPTIONS'; | ||
export const UPDATE_TABLE_SELECTIONS = 'UPDATE_TABLE_SELECTIONS'; | ||
export const UPDATE_QUERY_PARAMS = 'UPDATE_QUERY_PARAMS'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ export interface Comment { | |
export interface Case { | ||
id: string; | ||
comments: Comment[]; | ||
commentIds: string[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we need the comment ids in a different array? Aren't included in comments array? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @XavierM had reasons There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cnasikas, you will only get the comments through the API if you set the query parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
createdAt: string; | ||
createdBy: ElasticUser; | ||
description: string; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
import { useCallback, useReducer } from 'react'; | ||
import { errorToToaster } from '../../components/ml/api/error_to_toaster'; | ||
import * as i18n from './translations'; | ||
import { useStateToaster } from '../../components/toasters'; | ||
import { deleteCases } from './api'; | ||
|
||
interface DeleteState { | ||
isDisplayConfirmDeleteModal: boolean; | ||
isDeleted: boolean; | ||
isLoading: boolean; | ||
isError: boolean; | ||
} | ||
type Action = | ||
| { type: 'DISPLAY_MODAL'; payload: boolean } | ||
| { type: 'FETCH_INIT' } | ||
| { type: 'FETCH_SUCCESS'; payload: boolean } | ||
| { type: 'FETCH_FAILURE' } | ||
| { type: 'RESET_IS_DELETED' }; | ||
|
||
const dataFetchReducer = (state: DeleteState, action: Action): DeleteState => { | ||
switch (action.type) { | ||
case 'DISPLAY_MODAL': | ||
return { | ||
...state, | ||
isDisplayConfirmDeleteModal: action.payload, | ||
}; | ||
case 'FETCH_INIT': | ||
return { | ||
...state, | ||
isLoading: true, | ||
isError: false, | ||
}; | ||
case 'FETCH_SUCCESS': | ||
return { | ||
...state, | ||
isLoading: false, | ||
isError: false, | ||
isDeleted: action.payload, | ||
}; | ||
case 'FETCH_FAILURE': | ||
return { | ||
...state, | ||
isLoading: false, | ||
isError: true, | ||
}; | ||
case 'RESET_IS_DELETED': | ||
return { | ||
...state, | ||
isDeleted: false, | ||
}; | ||
default: | ||
throw new Error(); | ||
stephmilovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}; | ||
interface UseDeleteCase extends DeleteState { | ||
dispatchResetIsDeleted: () => void; | ||
handleOnDeleteConfirm: (caseIds: string[]) => void; | ||
handleToggleModal: () => void; | ||
} | ||
|
||
export const useDeleteCases = (): UseDeleteCase => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. show me? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @XavierM thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
const [state, dispatch] = useReducer(dataFetchReducer, { | ||
isDisplayConfirmDeleteModal: false, | ||
isLoading: false, | ||
isError: false, | ||
isDeleted: false, | ||
}); | ||
const [, dispatchToaster] = useStateToaster(); | ||
|
||
const dispatchDeleteCases = useCallback(async (caseIds: string[]) => { | ||
let cancel = false; | ||
try { | ||
dispatch({ type: 'FETCH_INIT' }); | ||
await deleteCases(caseIds); | ||
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; | ||
}; | ||
}, []); | ||
|
||
const dispatchToggleDeleteModal = useCallback(() => { | ||
dispatch({ type: 'DISPLAY_MODAL', payload: !state.isDisplayConfirmDeleteModal }); | ||
}, [state.isDisplayConfirmDeleteModal]); | ||
|
||
const dispatchResetIsDeleted = useCallback(() => { | ||
dispatch({ type: 'RESET_IS_DELETED' }); | ||
}, [state.isDisplayConfirmDeleteModal]); | ||
|
||
const handleOnDeleteConfirm = useCallback( | ||
caseIds => { | ||
dispatchDeleteCases(caseIds); | ||
stephmilovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
dispatchToggleDeleteModal(); | ||
}, | ||
[state.isDisplayConfirmDeleteModal] | ||
); | ||
const handleToggleModal = useCallback(() => { | ||
dispatchToggleDeleteModal(); | ||
}, [state.isDisplayConfirmDeleteModal]); | ||
|
||
return { ...state, dispatchResetIsDeleted, handleOnDeleteConfirm, handleToggleModal }; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about passing a signal (AbortController) so we can pass it to
fetch
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related to this comment #59616 (comment)