-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Exploratory View] Adds "Add to case" button #112463
Conversation
@elasticmachine merge upstream |
I think this looks good @shahzad31 - a great one to get in for the 7.16 GA, for sure! I'd be curious to see if @liciavale thinks we should create an "Actions" button that has open in lens and add to case with it given the upcoming actions of "share", "save" etc in future iterations. |
@elasticmachine merge upstream |
Pinging @elastic/uptime (Team:uptime) |
Yes, let's consider this when we add other actions. |
@elasticmachine merge upstream |
...s/observability/public/components/shared/exploratory_view/header/add_to_case_action.test.tsx
Outdated
Show resolved
Hide resolved
const { cases, http } = kServices; | ||
|
||
const getToastText = useCallback( | ||
(thaCase) => toMountPoint(<CaseToastText theCase={thaCase} basePath={http.basePath.get()} />), |
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 does toMountPoint
do?
Should thaCase
be theCase
? If not, what does tha mean?
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.
toMountPoint
is a util exposed by kibana when you need work with toasts etc and you need to mount react elements.
it('should return expected result', async function () { | ||
const { setData, core, findByText } = setupTestComponent(); | ||
|
||
expect(setData).toHaveBeenLastCalledWith({ |
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.
Is there any reason you can't use renderHook
to test this, instead of creating your own helper component?
To me it seems this is partially testing your helper component, since set data and when/how it's called isn't part of the actual hook. It seems like you're wanting to just test the result
object.
Using renderHook
you could pass the appropriate hook props and test that the result object
matches your expectation.
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.
i need contexts on top of hook render for this to work, render from rtl utility already has those context as wrapper in.
for example that hook uses useKibana
inside, if i wanted to use renderHook, i had to mock all those hooks separately.
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.
Not sure if this fits your use case, but you can also use the wrapper
option to wrap the hooks in providers. Could take one of the provider wrapper in our rtl helpers and wrap it with. But I see what you did here.
https://react-hooks-testing-library.com/usage/advanced-hooks
x-pack/plugins/observability/public/components/shared/exploratory_view/hooks/use_add_to_case.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/public/components/shared/exploratory_view/hooks/use_add_to_case.ts
Outdated
Show resolved
Hide resolved
[navigateToApp] | ||
); | ||
|
||
const onCaseClicked = useCallback( |
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.
You can define this as a async function rather than using .then
syntax.
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.
i actually wanted to add error handling here. i find erro handling in this pattern much more readable as compared to try/catch approach with await.
); | ||
}); | ||
} else { | ||
navigateToApp(appId, { |
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.
onCaseClick
should only be applied to a button if theCase
and lensAttributes
are truthy. Is this condition desired?
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.
yes this condition is actually for the cases modal, if there will be no theCase , clicking on add button in the case modal will take you to create case. it's weird but this is how that modal works.
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.
That's cool, i will probably extend rtl helpers for a use case like this in future
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.
let me know if this is blocking for this PR @dominiqueclarke
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.
After changing report type, Add To Case button no longer works
- Click Add To Case Button
- Exit Add To Case modal
- Change report type
- Click Add To Case button again
- Notice that Add To Case button no longer works
Add to case modal pops up when changing report config.
- Using Synthetics Monitoring -> KPI Over Time as an example, set a value for monitor name
- Click Add To Case Button
- Exit Add To Case modal
- Remove monitor name
- Add monitor name again
- Notice that the modal pops up again, even when you don't click the button.
It's a shame that you can open up the visualization in lens, but not in exploratory view. Assuming this is out of scope, but is there any way to add an additional option to open in exploratory view? EDIT: I'm realizing now that this is part of the AC for associated issue.
@dominiqueclarke i have fixed this issue |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Reporting Functional Tests With Deprecated Roles config.x-pack/test/reporting_functional/reporting_and_deprecated_security/security_roles_privileges·ts.Reporting Functional Tests with Deprecated Security configuration enabled Security with `reporting_user` built-in role Visualize Editor: Generate Screenshot does allow user with reporting_user roleStandard Out
Stack Trace
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @shahzad31 |
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 but one point in the AC is not implemented
There should be a link from cases back into exploratory view that loads the given chart in exploratory view
After discussing with Shahzad, this is not currently able to be implemented. This criteria should be moved to another ticket to keep track.
Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Shahzad <[email protected]>
Summary
Fix elastic/uptime#322
Added a "Add to case" button in exploratory view
Once user clicks it, it will show a modal
Once user selects the case, after loading
we can see the toast
In the toast if you click view case, it will leads you to the case and you can see the viz added in the comments