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

[NP] Remove kbnUrl usage in discover/dashboard/visualize #60016

Merged
merged 7 commits into from
Mar 23, 2020
Merged

[NP] Remove kbnUrl usage in discover/dashboard/visualize #60016

merged 7 commits into from
Mar 23, 2020

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Mar 12, 2020 •
edited
Loading

Summary

Resolves #58999

Remove all references to KbnUrlProvider in dashboard/discover/visualize.
Cleans up unused angular services.

Replace kbnUrl.eval with template strings, embedded variables wrapped with encodeURIComponent for safety and BWC (as it was inside eval func).

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@flash1293 flash1293 mentioned this pull request Mar 13, 2020
Closed
69 tasks
@sulemanof sulemanof changed the title [NP] Remove kbnUrl usage in visualize [NP] Remove kbnUrl usage in discover/dashboard/visualize Mar 13, 2020
@flash1293 flash1293 mentioned this pull request Mar 16, 2020
Closed
8 tasks
@sulemanof sulemanof force-pushed the shim/remove_kbnurl_visualize branch from ee0e982 to 213c746 Compare March 19, 2020 07:22
@sulemanof sulemanof added Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0 labels Mar 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@sulemanof sulemanof marked this pull request as ready for review March 19, 2020 07:25
@sulemanof sulemanof requested a review from a team March 19, 2020 07:25
kertal
kertal reviewed Mar 20, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

🎉 goodbye kbnUrl! there's one thing that should be adapted. kbnUrl.eval is using encodeURIComponent for encoding and securing url params, this should also be done in this PR

kertal
kertal reviewed Mar 20, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the adaptions to use encodeURIComponent 👍 🔒
While testing again, I found an issue to investigate, seems in discover, the New button only resets Url, but state change is not triggered, same when I use the sidebar button, which should also trigger this change

image

@sulemanof
Fix reloading when base path
11cc68f
@sulemanof
Merge remote-tracking branch 'upstream/master' into shim/remove_kbnur…
f89b458 
…l_visualize

# Conflicts:
#	src/legacy/core_plugins/kibana/public/discover/get_inner_angular.ts
#	src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js
#	src/legacy/core_plugins/kibana/public/discover/np_ready/angular/doc_table/components/table_row.ts
@sulemanof
Copy link
Contributor Author

While testing again, I found an issue to investigate, seems in discover, the New button only resets Url, but state change is not triggered, same when I use the sidebar button, which should also trigger this change

Thanks, it a really good catch!
The redirection from the sidebar was not working on master also, so this fixes both cases by adding a history listener.

@kertal
Copy link
Member

kertal commented Mar 23, 2020

@elasticmachine merge upstream

@kertal
Copy link
Member

kertal commented Mar 23, 2020

While testing again, I found an issue to investigate, seems in discover, the New button only resets Url, but state change is not triggered, same when I use the sidebar button, which should also trigger this change

Thanks, it a really good catch!
The redirection from the sidebar was not working on master also, so this fixes both cases by adding a history listener.

thx! works!

kertal
kertal approved these changes Mar 23, 2020
View reviewed changes
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM 👍 , tested locally in Chrome

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@sulemanof sulemanof merged commit afd1179 into elastic:master Mar 23, 2020
@sulemanof sulemanof deleted the shim/remove_kbnurl_visualize branch March 23, 2020 07:57
@sulemanof sulemanof mentioned this pull request Mar 23, 2020
Merged
sulemanof added a commit to sulemanof/kibana that referenced this pull request Mar 23, 2020
@sulemanof @elasticmachine
[NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016)
7a28156 
* Remove kbnUrl usages from disciver/dashboard/visualize

* Remove kbnUrl usage in angular_config

* Wrap with encodeURIComponent

* Fix reloading when base path

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 23, 2020
@gmmorris
Merge branch 'master' into alerting/tls-warning
6230180 
* master: (39 commits)
  [APM]Create custom link from Trace summary (elastic#59648)
  [ML] Fixing app clean up (elastic#60853)
  [SIEM] Use ECS categorisation for Authentication widgets (elastic#60734)
  [NP] Remove kbnUrl usage in discover/dashboard/visualize (elastic#60016)
  Skip failing test
  [Uptime]Update fetch effect failed action handling (elastic#60742)
  [npm] upgrade elastic/maki (elastic#60829)
  [Uptime] Add Settings Page (elastic#53550)
  [APM] service maps: avoid unnecesary `useDeepObjectIdentity` (elastic#60836)
  [Index management] Re-enable index template tests (elastic#60780)
  Fixed UI/UX issues: alerts delete confirmation, combobox behaviors (elastic#60703)
  [SIEM] Fix patching of ML Rules (elastic#60830)
  [APM] Service Map - Separate overlapping edges by rotating nodes (elastic#60477)
  [Alerting] fix flaky test for index threshold grouping (elastic#60792)
  [SIEM][Detection Engine] Adds test scripts for machine learning feature
  Flatten child api response for resolver (elastic#60810)
  Change "url" to "urls" in APM agent instructions (elastic#60790)
  [DOCS] Updates API requests and examples (elastic#60695)
  [SIEM] [Cases] Create case from timeline (elastic#60711)
  [Lens] Resetting a layer generates new suggestions (elastic#60674)
  ...
sulemanof added a commit that referenced this pull request Mar 23, 2020
@sulemanof @elasticmachine
[NP] Remove kbnUrl usage in discover/dashboard/visualize (#60016) (#6…
cf85c74 
…0884)

* Remove kbnUrl usages from disciver/dashboard/visualize

* Remove kbnUrl usage in angular_config

* Wrap with encodeURIComponent

* Fix reloading when base path

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kertal kertal kertal approved these changes

@flash1293 flash1293 Awaiting requested review from flash1293

Assignees
No one assigned
Labels
backported Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove kbnUrl usage
4 participants
@sulemanof @elasticmachine @kertal @kibanamachine