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

[Lens] Fix crash in transition from unique count to last value #88916

Merged
merged 7 commits into from
Jan 27, 2021
Merged

[Lens] Fix crash in transition from unique count to last value #88916

merged 7 commits into from
Jan 27, 2021

Conversation

wylieconlon
Copy link
Contributor

There are two behaviors to test. One is that the bug is fixed, the other is that we don't regress further.

Steps to reproduce the bug:

  1. Select unique count of a string field
  2. Switch to Last Value
  3. No crash

Also, test that there is no regression by looking at the error messages when you do:

  1. On the X axis, choose a Date Histogram
  2. On the Y axis, select "Moving average", but do not fully configure it
  3. Switch to "Average" from the incomplete state
  4. No error message is shown

Fixes #88890

Checklist

@wylieconlon wylieconlon added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Jan 20, 2021
@wylieconlon wylieconlon requested a review from a team January 20, 2021 22:34
@elasticmachine
Copy link
Contributor

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

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

@wylieconlon
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@elasticmachine merge upstream

@flash1293
Copy link
Contributor

@wylieconlon The functional test failure looks like an actual problem in the code, could you take a look?

flash1293
flash1293 reviewed Jan 27, 2021
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 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.

I tested the cases listed out here and it's working fine. I checked the other PR that introduced this change and I'm not sure why we added this in the first place - @dej611 do you know? It looks like we there is no case that would require it to validate incomplete columns using getErrorMessages. I don't remember whether we discussed this in the other PR.

@dej611 dej611 mentioned this pull request Jan 27, 2021
Merged
1 task
@dej611
Copy link
Contributor

dej611 commented Jan 27, 2021

I tested the cases listed out here and it's working fine. I checked the other PR that introduced this change and I'm not sure why we added this in the first place - @dej611 do you know? It looks like we there is no case that would require it to validate incomplete columns using getErrorMessages. I don't remember whether we discussed this in the other PR.

Yup. I've merged my other PR with this and found out the same thing. Passing the indexPattern over is enough to perform the right check. Left a comment on my side as well.

@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
lens 853.4KB 853.1KB -327.0B

History

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

@wylieconlon wylieconlon merged commit 445cb2e into elastic:master Jan 27, 2021
@wylieconlon wylieconlon deleted the lens/fix-transition branch January 27, 2021 23:44
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/basic-a11…
e814056 
…y-tests

* 'master' of github.com:elastic/kibana: (31 commits)
  [Discover] Add grid flyout jest test (elastic#89088)
  [Search Sessions] Improve session restoration back button (elastic#87635)
  [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274)
  [Security Solution] Init Osquery plugin (elastic#87109)
  [Fleet] Do not defined aliases inside datastream template (elastic#89512)
  skip flaky suite (elastic#86950)
  chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469)
  [build/docker] Add support for centos ARM builds (elastic#84831)
  Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488)
  [CI] Decrease number of Jest workers (elastic#89504)
  [Maps] remove maps_oss TS project (elastic#89502)
  Adds migration settings to Docker (elastic#89501)
  [Lens] Fix crash in transition from unique count to last value (elastic#88916)
  [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437)
  unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346)
  [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439)
  skip flaky suite (elastic#89478)
  skip flaky suite (elastic#89476)
  skip flaky suite (elastic#89477)
  skip flaky suite (elastic#89475)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jan 28, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/rollover-…
494a041 
…updates-and-timeline-cleanup

* 'master' of github.com:elastic/kibana: (44 commits)
  [Discover] Add grid flyout jest test (elastic#89088)
  [Search Sessions] Improve session restoration back button (elastic#87635)
  [TSVB] Remove vis_type_timeseries_enhanced plugin (elastic#89274)
  [Security Solution] Init Osquery plugin (elastic#87109)
  [Fleet] Do not defined aliases inside datastream template (elastic#89512)
  skip flaky suite (elastic#86950)
  chore(NA): bazel machinery installation on kbn bootstrap (elastic#89469)
  [build/docker] Add support for centos ARM builds (elastic#84831)
  Convert default_watch.json to a JS object in order to avoid TS complaints (elastic#89488)
  [CI] Decrease number of Jest workers (elastic#89504)
  [Maps] remove maps_oss TS project (elastic#89502)
  Adds migration settings to Docker (elastic#89501)
  [Lens] Fix crash in transition from unique count to last value (elastic#88916)
  [kbn-es] Always use bundled JDK when starting Elasticsearch (elastic#89437)
  unskip getting_started/shakespeare test elasticsearch 64016 (elastic#89346)
  [Maps] migrate maps, maps_file_upload, and maps_legacy_licensing to TS projects (elastic#89439)
  skip flaky suite (elastic#89478)
  skip flaky suite (elastic#89476)
  skip flaky suite (elastic#89477)
  skip flaky suite (elastic#89475)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/lib/absolute_timing_to_relative_timing.ts
wylieconlon pushed a commit to wylieconlon/kibana that referenced this pull request Jan 28, 2021
@kibanamachine @wylieconlon
[Lens] Fix crash in transition from unique count to last value (elast…
4e6dc28 
…ic#88916)

* [Lens] Fix transition from unique count to last value

* Fix test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@wylieconlon wylieconlon mentioned this pull request Jan 28, 2021
Merged
wylieconlon pushed a commit that referenced this pull request Jan 29, 2021
@kibanamachine
[Lens] Fix crash in transition from unique count to last value (#88916)…
a5232dc 
… (#89651)

* [Lens] Fix transition from unique count to last value

* Fix test

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

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

@flash1293 flash1293 flash1293 left review comments

@dej611 dej611 dej611 approved these changes

@mbondyra mbondyra Awaiting requested review from mbondyra

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

Successfully merging this pull request may close these issues.

[Lens] Lens editor crashes when switching to
5 participants
@wylieconlon @elasticmachine @flash1293 @dej611 @kibanamachine