[SIEM] Fix link on overview page #60348

XavierM · 2020-03-17T02:54:52Z

Summary

We were missing the query parameters on all links of the overview pages.

Checklist

Delete any items that are not applicable to this PR.

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
~~[ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~[ ] This was checked for keyboard-only and screenreader accessibility~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~

elasticmachine · 2020-03-17T02:54:54Z

Pinging @elastic/siem (Team:SIEM)

XavierM · 2020-03-17T03:01:04Z

x-pack/legacy/plugins/siem/public/components/url_state/helpers.ts

@@ -113,42 +108,13 @@ export const getTitle = (
  return navTabs[pageName] != null ? navTabs[pageName].name : '';
 };

-export const getCurrentLocation = (


FYI -> It was not used @stephmilovic told me and when I went in this files, I had to delete it ;)

XavierM · 2020-03-17T03:02:40Z

x-pack/legacy/plugins/siem/public/components/url_state/helpers.ts

 export const makeMapStateToProps = () => {
  const getInputsSelector = inputsSelectors.inputsSelector();
  const getGlobalQuerySelector = inputsSelectors.globalQuerySelector();
  const getGlobalFiltersQuerySelector = inputsSelectors.globalFiltersQuerySelector();
  const getGlobalSavedQuerySelector = inputsSelectors.globalSavedQuerySelector();
  const getTimelines = timelineSelectors.getTimelines();
-  const mapStateToProps = (state: State, { pageName, detailName }: UrlStateContainerPropTypes) => {
+  const mapStateToProps = (state: State) => {


pageName and detailName are not used in the function

andrew-goldstein · 2020-03-17T03:29:45Z

x-pack/legacy/plugins/siem/public/components/link_to/redirect_to_detection_engine.tsx

-export const getDetectionEngineAlertUrl = () =>
-  `${baseDetectionEngineUrl}/${DetectionEngineTab.alerts}`;
+export const getDetectionEngineUrl = (search?: string) =>
+  `${baseDetectionEngineUrl}${search != null ? `?${search}` : ''}`;


Consider creating a utility function for ${search != null ? ?${search} : ''}, because there are six instances of it in this PR, and likely additional instances in the future

I'm not sure if it's possible / valid to change the type signatures of the functions that take (search?: string) to something like (search: string | null), but an advantage of such a change is the type checker would force the caller to always handle this case by passing search or an explicit null when search should not be provided.

For example, making this parameter non-optional makes the type checker flag the following code in x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/index.tsx:

<DetectionEngineHeaderPage backOptions={{ href: getDetectionEngineUrl(), text: i18n.BACK_TO_DETECTION_ENGINE, }} title={i18n.PAGE_TITLE} >

If the above code really should be passing search, then the type checker just found a bug

If the above code is a valid case to not pass search, we would be forced to explicitly pass null, which feels OK to me

Making search non-optional also causes the type checker to flag getBreadcrumbs in x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/utils.ts because it uses a slight variation of the "search checking pattern", where in this case it's using search[0] instead:

${!isEmpty(search[0]) ? search[0] : ''}

That's why I did it like that because I just wanted to focus this PR on the overview page. Also, I remember that we made a decision that we did not want the breadcrumb to follow the url state for some reason. (maybe we can discuss it in one of our meetings)

but I mean you are correct, we should have a follow-up PRs to get all link through the app corrected and discussed breadcrumb.

It looks like all breadcrumbs have the URL state, so that resolved the question above. I will go over in another PR to make everything following the same pattern. so the breadcrumb's URL can be also simplified from this PR.

x-pack/legacy/plugins/siem/public/components/link_to/helpers.test.ts

andrew-goldstein

Thanks for fixing #60076 @XavierM! 🙏

Desk tested:

via the repro steps steps in the issue
the following buttons and links: View signals, View alerts, View events, View hosts, View network, View all timelines
using both single-click and "middle-click / open-in-new-tab"-style navigation
in the following browsers: Chrome 80.0.3987.132, Firefox 74.0, and Safari 13.0.5
LGTM 🚀 🔗

kibanamachine · 2020-03-17T14:45:41Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: 31e7961
Flaky suites:
- xpack-siemCypress

Test Failures

Kibana Pipeline / kibana-xpack-agent / sets the flyout button background to euiColorSuccess with a 10% alpha channel when the user starts dragging a host, but is not hovering over the flyout button.timeline flyout button sets the flyout button background to euiColorSuccess with a 10% alpha channel when the user starts dragging a host, but is not hovering over the flyout button

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has failed 2 times on tracked branches: https://github.com/elastic/kibana/issues/60369

Stack Trace

CypressError: Timed out retrying: expected '<button.euiButton.euiButton--primary.euiButton--iconRight.not-ready-to-drop>' to have CSS property 'background' with the value 'rgba(1, 125, 115, 0.1) none repeat scroll 0% 0% / auto padding-box border-box', but the value was 'rgb(255, 255, 255) none repeat scroll 0% 0% / auto padding-box border-box'
    at cypressErr (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:149934:9)
    at throwErr (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:149867:11)
    at Object.throwErrByPath (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:149915:3)
    at retry (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:144197:19)
    at onFailFn (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:132448:16)
    at tryCatcher (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:167415:23)
    at Promise._settlePromiseFromHandler (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:165351:31)
    at Promise._settlePromise (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:165408:18)
    at Promise._settlePromise0 (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:165453:10)
    at Promise._settlePromises (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:165528:18)
    at Async../node_modules/bluebird/js/release/async.js.Async._drainQueue (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:162140:16)
    at Async../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:162150:10)
    at Async.drainQueues (http://elastic:changeme@localhost:61121/__cypress/runner/cypress_runner.js:162024:14)

History

💔 Build #33697 failed 6828519
💚 Build #33683 succeeded 6f000ef

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

* Fix link on overview page * no needs of useMemo * clean up * review I * review II * review III

* master: [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358)

* Fix link on overview page * no needs of useMemo * clean up * review I * review II * review III

* master: (51 commits) do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) ...

* alerting/view-in-app: (53 commits) fixed typo handle optional alerting plugin do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) ...

Fix link on overview page

54cf075

XavierM added bug Fixes for quality problems that affect the customer experience Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v7.6.2 labels Mar 17, 2020

XavierM requested a review from andrew-goldstein March 17, 2020 02:54

XavierM self-assigned this Mar 17, 2020

no needs of useMemo

3c1d9cc

XavierM commented Mar 17, 2020

View reviewed changes

clean up

6f000ef

andrew-goldstein reviewed Mar 17, 2020

View reviewed changes

XavierM added 2 commits March 17, 2020 00:31

review I

1378b84

review II

6828519

andrew-goldstein reviewed Mar 17, 2020

View reviewed changes

x-pack/legacy/plugins/siem/public/components/link_to/helpers.test.ts Outdated Show resolved Hide resolved

andrew-goldstein approved these changes Mar 17, 2020

View reviewed changes

XavierM added 2 commits March 17, 2020 08:45

review III

68e8a97

Merge branch 'master' of github.com:elastic/kibana into overview-link

31e7961

XavierM merged commit 0f9f81c into elastic:master Mar 17, 2020

This was referenced Mar 17, 2020

[SIEM] Navigating from the Overview to the Detections page resets date selection in 7.6.1 #60076

Closed

[7.x] [SIEM] Fix link on overview page (#60348) #60381

Merged

XavierM added a commit to XavierM/kibana that referenced this pull request Mar 17, 2020

[SIEM] Fix link on overview page (elastic#60348)

87105f3

* Fix link on overview page * no needs of useMemo * clean up * review I * review II * review III

XavierM added a commit that referenced this pull request Mar 17, 2020

[SIEM] Fix link on overview page (#60348) (#60381)

df8e754

* Fix link on overview page * no needs of useMemo * clean up * review I * review II * review III

XavierM removed the v7.6.2 label Mar 17, 2020

andrew-goldstein mentioned this pull request Mar 24, 2020

[SIEM] Overview: Recent cases widget #60993

Merged

3 tasks

XavierM deleted the overview-link branch June 4, 2020 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Fix link on overview page #60348

[SIEM] Fix link on overview page #60348

XavierM commented Mar 17, 2020 •

edited

Loading

elasticmachine commented Mar 17, 2020

XavierM Mar 17, 2020

XavierM Mar 17, 2020

andrew-goldstein Mar 17, 2020

andrew-goldstein Mar 17, 2020

XavierM Mar 17, 2020

XavierM Mar 17, 2020

XavierM Mar 17, 2020

andrew-goldstein left a comment

kibanamachine commented Mar 17, 2020

Standard Out

Stack Trace

[SIEM] Fix link on overview page #60348

[SIEM] Fix link on overview page #60348

Conversation

XavierM commented Mar 17, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-goldstein left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 17, 2020

💛 Build succeeded, but was flaky

Standard Out

Stack Trace

History

XavierM commented Mar 17, 2020 •

edited

Loading