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.

Sign up for GitHub

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

Jump to bottom

[Endpoint] ERT-82 Alerts search bar #59702

Merged
merged 8 commits into from
Mar 12, 2020
Merged

[Endpoint] ERT-82 Alerts search bar #59702

merged 8 commits into from
Mar 12, 2020

Conversation

peluja1012
Copy link
Contributor

@peluja1012 peluja1012 commented Mar 9, 2020 •
edited
Loading

Summary

Integrates Stateful Search Bar (from kibana/src/plugins/data) into the Endpoint App's Alerts page. This PR also syncs the Search Bar's state with the browser url.

image

image

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@peluja1012 peluja1012 added WIP Work in progress Team:Endpoint Response Endpoint Response Team Feature:Endpoint Elastic Endpoint feature labels Mar 9, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-response (Team:Endpoint Response)

@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

@EricDavisX
Copy link
Contributor

Would love to talk about what tests you want to put in for this, what we think is warranted and useful. If you can help find and link file for the search bar component that we're re-using that would help to know what tests it already has so we can focus this pr to testing only what is needed and at what right level.

oatkiller
oatkiller reviewed Mar 11, 2020
View reviewed changes
x-pack/plugins/endpoint/server/routes/alerts/list/lib/index.ts
query: request.query.query,
query:
request.query.query !== undefined
? ((decode(request.query.query) as unknown) as Query)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does decode need any error handling or anything? Curious because this will have typescript trust that the value is a Query. What happens if the decode fails?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oatkiller In this case, the schema will have already validated the input, so we should be okay, no? Or do you think we should handle this differently? In any case, we should have API-level tests to check for a 400 and ensure there's no 500 when invalid input is provided. @peluja1012 <<<

oatkiller
oatkiller reviewed Mar 11, 2020
View reviewed changes
x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts Outdated
export const alertMiddlewareFactory: MiddlewareFactory<AlertListState> = (coreStart, depsStart) => {
async function fetchIndexPatterns(): Promise<IIndexPattern[]> {
const { indexPatterns } = depsStart.data;
// TODO: what's the best way to get the index pattern?
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

oatkiller
oatkiller reviewed Mar 11, 2020
View reviewed changes
x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/selectors.ts
uiQueryParams,
({ query }) => {
if (query !== undefined) {
return (decode(query) as unknown) as Query;
Copy link
Contributor

Choose a reason for hiding this comment

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

this unsafe cast makes me think we might need error handling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/elastic/endpoint-app-team/issues/265

oatkiller
oatkiller reviewed Mar 11, 2020
View reviewed changes
x-pack/plugins/endpoint/public/applications/endpoint/store/index.ts
@@ -48,37 +49,37 @@ export const substateMiddlewareFactory = <Substate>(
};
};

export const appStoreFactory: (
export const appStoreFactory: (middlewareDeps?: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a doc comment here explaining the behavior when no middleware deps are passed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@peluja1012 peluja1012 marked this pull request as ready for review March 12, 2020 13:01
@peluja1012 peluja1012 requested a review from a team as a code owner March 12, 2020 13:01
@peluja1012 peluja1012 added v7.7.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes and removed WIP Work in progress labels Mar 12, 2020
madirey
madirey reviewed Mar 12, 2020
View reviewed changes
x-pack/plugins/endpoint/common/schema/alert_index.ts
@@ -44,10 +43,10 @@ export const alertingIndexGetQuerySchema = schema.object(
schema.string({
validate(value) {
try {
fromKueryExpression(value);
decode(value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be decoded and then run through fromKueryExpression still? To check both cases? Or are we changing the format of query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@madirey We're changing the format of query to be of type Query, which has a query and a language (kql or lucene)

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@peluja1012 peluja1012 changed the title [Endpoint] Alerts search bar [Endpoint] ERT-82 Alerts search bar Mar 12, 2020
@peluja1012 peluja1012 merged commit cf5c801 into elastic:master Mar 12, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 13, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
b0010da 
* master:
  [Alerting] extend Alert Type with names/descriptions of action variables (elastic#59756)
  [Ingest] Fix data source creation and double system data source (elastic#60069)
  Add button to view full service map (elastic#59394)
  unskip tests for code coverage (elastic#59725)
  [Ingest] Add Fleet & EPM features (elastic#59376)
  [Logs UI] Show navigation bar while loading source configurati… (elastic#59997)
  [Endpoint] ERT-82 Alerts search bar (elastic#59702)
  Aggregate queue types being used by Beats (elastic#59850)
  skip flaky suite (elastic#59541)
  Convert Timeline to TypeScript (elastic#59966)
  Make context.core required argument to context providers (elastic#59996)
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59702 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 13, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59702 or prevent reminders by adding the backport:skip label.

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59702 or prevent reminders by adding the backport:skip label.

@peluja1012 peluja1012 mentioned this pull request Mar 16, 2020
Merged
peluja1012 added a commit that referenced this pull request Mar 16, 2020
@peluja1012
[Endpoint] ERT-82 Alerts search bar (#59702) (#60288)
541a9cf 
* Add SearchBar UI. Query, filter, and dateRange work

* Sync url with search bar, fix excluded filters on BE

* fix welcome title

* Use KibanaReactContext, fix indexPattern fetch

* Mock data plugin, api and ui tests pass

* Add view and functional tests

* use observables to handle filter updates

* address comments
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@madirey madirey madirey left review comments

@oatkiller oatkiller oatkiller approved these changes

@dplumlee dplumlee dplumlee approved these changes

@kqualters-elastic kqualters-elastic Awaiting requested review from kqualters-elastic

@paul-tavares paul-tavares Awaiting requested review from paul-tavares

Assignees
No one assigned
Labels
Feature:Endpoint Elastic Endpoint feature release_note:skip Skip the PR/issue when compiling release notes Team:Endpoint Response Endpoint Response Team v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@peluja1012 @elasticmachine @EricDavisX @kibanamachine @oatkiller @madirey @dplumlee