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

Saved visualization with search string confuse altering of search string #103396

Merged
merged 4 commits into from Jul 29, 2021
Merged

Saved visualization with search string confuse altering of search string #103396

merged 4 commits into from Jul 29, 2021

Conversation

VladLasitsa
Copy link
Contributor

@VladLasitsa VladLasitsa commented Jun 25, 2021 •
edited by stratoula

Closes: #101574
Closes #100920

Summary

Add possibility to sync query and filter from app state when we load page.

How to test it:

  • create a visualization
  • enter a search term (e.g. IP address field with an IP), apply time range (e.g. last 5 minutes)
  • save visualization
  • open this saved visualization
  • result is fine
  • alter search term, e.g. modify IP address
  • hit enter in search field
  • result looks fine
  • go with alternative 1 or 2

Alternative 1:

  • hit refresh button in browser / press F5
  • result shows nothing
  • inspect -> view request -> request: source of request shows that old AND new search terms are applied to the query

Aternative 2:

  • copy and paste URL in new tab
  • result shows nothing
  • inspect -> view request -> request: source of request shows that old AND new search terms are applied to the query

After fix: source of request shows only new search terms are applied to the query.

@VladLasitsa VladLasitsa added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:fix labels Jun 25, 2021
@VladLasitsa VladLasitsa self-assigned this Jun 25, 2021
@alexwizp
Copy link
Contributor

I see we already have some tests for this code in use_visualize_app_state.test.ts. Please cover your case by unit test

@VladLasitsa
Copy link
Contributor Author

I see we already have some tests for this code in use_visualize_app_state.test.ts. Please cover your case by unit test

Added test.

@alexwizp alexwizp requested a review from stratoula June 28, 2021 08:06
@stratoula stratoula added this to In progress in Legacy visualizations via automation Jun 28, 2021
@stratoula stratoula added v7.15.0 and removed v7.14.0 labels Jun 29, 2021
@stratoula
Copy link
Contributor

@VladLasitsa check if also this #100920 can be closed

@alexwizp alexwizp self-requested a review July 27, 2021 15:00
alexwizp
alexwizp approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@alexwizp alexwizp left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

stratoula
stratoula approved these changes Jul 29, 2021
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

LGTM, I tested it locally in Chrome and fixes all problematic cases

@alexwizp alexwizp marked this pull request as ready for review July 29, 2021 13:39
@alexwizp alexwizp requested a review from a team July 29, 2021 13:39
@elasticmachine
Copy link
Contributor

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

@alexwizp alexwizp added the auto-backport Deprecated: Automatically backport this PR after it's merged label Jul 29, 2021
@alexwizp alexwizp enabled auto-merge (squash) July 29, 2021 13:39
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
visualize 94.9KB 95.4KB +490.0B

History

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

cc @VladLasitsa

@alexwizp alexwizp merged commit 57042f0 into elastic:master Jul 29, 2021
Legacy visualizations automation moved this from In progress to Done Jul 29, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jul 29, 2021
@VladLasitsa @kibanamachine
Saved visualization with search string confuse altering of search str…
5737035 
…ing (elastic#103396)

* Add possibility to sync query and filter from state

* Fix unit-tests

* Fix name

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine mentioned this pull request Jul 29, 2021
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 29, 2021
@kibanamachine @VladLasitsa
Saved visualization with search string confuse altering of search str…
bdcaf2c 
…ing (#103396) (#107185)

* Add possibility to sync query and filter from state

* Fix unit-tests

* Fix name

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Uladzislau Lasitsa <Uladzislau_Lasitsa@epam.com>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
@VladLasitsa @kibanamachine @vadimkibana
Saved visualization with search string confuse altering of search str…
babb0ad 
…ing (elastic#103396)

* Add possibility to sync query and filter from state

* Fix unit-tests

* Fix name

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@KOTungseth KOTungseth added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Sep 1, 2021
@VladLasitsa VladLasitsa mentioned this pull request Oct 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@alexwizp alexwizp alexwizp approved these changes

Assignees

@VladLasitsa VladLasitsa

Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.15.0 v8.0.0
Projects
Legacy visualizations
Done
Milestone
No milestone
6 participants
@VladLasitsa @alexwizp @stratoula @elasticmachine @kibanamachine @KOTungseth