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

[Security Solution] [Timeline] delete notes #154834

Merged
merged 19 commits into from Apr 25, 2023

Conversation

kqualters-elastic
Copy link
Contributor

@kqualters-elastic kqualters-elastic commented Apr 12, 2023

Summary

This pr is a part of work to harden the UX around investigation guides and user/outside of kibana generated content, currently in the form of markdown: #153377 This pr allows users to delete notes associated with a timeline, as invalid or incorrect content can currently be persisted in a timeline, with no way to remove it except to manually remove a hidden saved object that defines the note. An unused interface was removed as part of this, which seemed to indicate adding the ability to delete notes was planned at some point, but never implemented.
delete_notes

Checklist

Delete any items that are not applicable to this PR.

@kqualters-elastic kqualters-elastic added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team labels Apr 12, 2023
@kqualters-elastic kqualters-elastic marked this pull request as ready for review April 24, 2023 19:27
@kqualters-elastic kqualters-elastic requested review from a team as code owners April 24, 2023 19:27
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@kqualters-elastic kqualters-elastic added the release_note:feature Makes this part of the condensed release notes label Apr 25, 2023
Copy link
Contributor

@dplumlee dplumlee left a comment

Choose a reason for hiding this comment

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

Alerts area files lgtm


try {
const frameworkRequest = await buildFrameworkRequest(context, security, request);
const noteId = request.body?.noteId ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought, if there is no noteId, could we just early return or just throw an error that the page has to be refreshed before the note can be deleted or something?


const { isFetching } = useDeleteNote(noteToDelete);
const disableDelete = useMemo(() => {
return isFetching || noteId == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

So noteId should never be missing in the query then right?

[addError]
);

return useQuery(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you want to use useMutation instead of useQuery? Not sure if you'd get weird caching things using useQuery instead of useMutation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya thank you, updated

Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes, works great! LGTM 💪🏾

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #1 / Detection rules, bulk edit Timeline templates Apply timeline template to custom rules

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 9.1MB 9.1MB +7.5KB
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 397 400 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 477 480 +3
total +5

History

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

@kqualters-elastic kqualters-elastic merged commit d50e828 into elastic:main Apr 25, 2023
20 checks passed
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Apr 25, 2023
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 release_note:feature Makes this part of the condensed release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Investigations Security Solution Investigations Team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants