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

[ML] Fix decoding in the URL state #54915

Merged
merged 6 commits into from
Jan 16, 2020

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Jan 15, 2020

Summary

Fixes #54885. Use rison decoding only for _a and _g query parameters.
Besides contains some fixes for minor issues on Data Visualiser page.

Checklist

@darnautov darnautov added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.6.0 labels Jan 15, 2020
@darnautov darnautov requested review from a team as code owners January 15, 2020 14:57
@darnautov darnautov self-assigned this Jan 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

if (decodedParams.has(a)) {
urlState[a] = decode(parsedQueryString[a]) as Dictionary<any>;
} else {
urlState[a] = parsedQueryString[a];
Copy link
Member

@jgowdyelastic jgowdyelastic Jan 15, 2020

Choose a reason for hiding this comment

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

i don't know if items which aren't _a or _g are needed at all for the url state.
but it might also do no harm collecting them. @walterra would need to confirm

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment also setting the urlState expects all of url state so I suggest to keep it like that for now. I did it this way because useLocation is not mutable and I always needed the full most recent value for getting/setting search.

@darnautov Thanks for adding the test! Seems you forgot to commit to export getUrlState so the test fails on CI at the moment. Can you also add a test to check the other way around when setting the url state? I suspect we also need a check against decodedParams when setting the state here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed I forgot to include export to my initial commit, added in 40d9646.
test for setUrlState: 177baeb

@darnautov darnautov force-pushed the ML-54885-fix-url-state-decode branch from 2418175 to 40d9646 Compare January 16, 2020 10:15
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits and LGTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM. Great first use of @testing-library/react-hooks!

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡️

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@darnautov darnautov merged commit a922e1f into elastic:master Jan 16, 2020
@darnautov darnautov deleted the ML-54885-fix-url-state-decode branch January 16, 2020 18:34
darnautov added a commit to darnautov/kibana that referenced this pull request Jan 17, 2020
* [ML] condition for rison decode in getUrlState

* [ML] fix icon alignment

* [ML] use Set

* [ML] add export, fix typo

* [ML] setUrlState test

* [ML] fields stats width
darnautov added a commit to darnautov/kibana that referenced this pull request Jan 17, 2020
* [ML] condition for rison decode in getUrlState

* [ML] fix icon alignment

* [ML] use Set

* [ML] add export, fix typo

* [ML] setUrlState test

* [ML] fields stats width
darnautov added a commit that referenced this pull request Jan 17, 2020
* [ML] condition for rison decode in getUrlState

* [ML] fix icon alignment

* [ML] use Set

* [ML] add export, fix typo

* [ML] setUrlState test

* [ML] fields stats width
darnautov added a commit that referenced this pull request Jan 17, 2020
* [ML] condition for rison decode in getUrlState

* [ML] fix icon alignment

* [ML] use Set

* [ML] add export, fix typo

* [ML] setUrlState test

* [ML] fields stats width
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 17, 2020
…t-of-legacy

* 'master' of github.com:elastic/kibana: (142 commits)
  [Vis] Move Timelion Vis to vis_type_timelion (elastic#52069)
  Deprecate `chrome.navlinks.update` and add documentation (elastic#54893)
  [ML] Single Metric Viewer: Fix time bounds with custom strings. (elastic#55045)
  [Vis: Default editor] EUIficate and Reactify the sidebar (elastic#49864)
  [Mappings editor] Fix cannot set boolean value for "null_value" param (elastic#55015)
  [SIEM] Adds support for apm-* to the network map (elastic#54876)
  [Reporting] Define shims of legacy dependencies (elastic#54082)
  Resolver is overflow: hidden to prevent obscured elements from showing up (elastic#55076)
  Upgraded EUI to 18.2.1 (elastic#55090)
  [Maps] Support styles on agg fields with _of_ in name (elastic#54965)
  Remove xpack_main requirement, it's no longer in use (elastic#55060)
  Fix Snapshots Policies Alignment Issue in IE11 (elastic#54866)
  first rule cuts (elastic#54990)
  [DOCS] Adds geocentroid note to coordinate map (elastic#54389)
  [Canvas] Fixes the Copy Post Url link (elastic#54831)
  Fixes bugs with full screen filters (elastic#54792)
  [ML] Fix decoding in the URL state  (elastic#54915)
  Remove redundant `x-pack/typings`. (elastic#55042)
  [SIEM][Detection Engine] Adds critical missing status route to prepackaged rules
  Generate legacy vars when rendering all applications (elastic#54768)
  ...

# Conflicts:
#	x-pack/plugins/translations/translations/ja-JP.json
#	x-pack/plugins/translations/translations/zh-CN.json
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
* [ML] condition for rison decode in getUrlState

* [ML] fix icon alignment

* [ML] use Set

* [ML] add export, fix typo

* [ML] setUrlState test

* [ML] fields stats width
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:ml release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML] Index Data Visualizer url state error
7 participants