-
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
chore(rca): notes management without investigation store #190623
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
I expect this to be a fairly light-weight look up in most cases? If so, every 10s would be great. |
|
||
const deleteInvestigationNoteParamsSchema = t.type({ | ||
path: t.type({ | ||
id: t.string, |
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.
maybe rename to investigationId
}, | ||
{ | ||
onSuccess: (response, {}) => { | ||
toasts.addSuccess('Note deleted'); |
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.
we need i18n here
refetchIntervalInBackground: true, | ||
onError: (error: Error) => { | ||
toasts.addError(error, { | ||
title: 'Something went wrong while fetching investigation notes', |
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.
i18n here
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.
LGTM! Thanks for the pair review, you brought me up to speed!
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Resolves ?
Summary
This PR removes the usage of the original investigation hook & store to manage the notes. It also uses the real user of the request to populate the createdBy field of the investigation and note.
We refresh the list of notes on creation/deletion (technically, react should not re-render the existing notes, but need to double check), and refresh automatically every
6010 seconds (should we do it more often)?Finally, user can only delete their own notes at the moment.
Testing
Screenshot