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

[Lens] Fix wrong error detection on transition to Top values operation #102384

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jun 16, 2021

Summary

Fixes #101758

The getErrorMessages method in the Top Values operation was returning an empty array in case of no errors rather than undefined, breaking the logic on the dimension editor detection for transition errors.
The regression was introduced within this release, so no need to report it externally.

This PR sets the correct result when the error array is empty.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dej611 dej611 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.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 16, 2021
@dej611 dej611 marked this pull request as ready for review June 17, 2021 08:24
@dej611 dej611 requested a review from a team June 17, 2021 08:24
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

Tested and this works fine, but I don't find the place in the code where it was causing issues before. Could you explain what's going on?

@dej611
Copy link
Contributor Author

dej611 commented Jun 17, 2021

getErrorMessage: (layer, columnId, indexPattern) =>
    [
      ...(getInvalidFieldMessage(
        layer.columns[columnId] as FieldBasedIndexPatternColumn,
        indexPattern
      ) || []),
      getDisallowedTermsMessage(layer, columnId, indexPattern) || '',
    ].filter(Boolean),

This snippet of code was returning an empty array.
In the dimension_editor the return value of that method was used to determine whether the field had an error or not:

Because of the truthiness of the empty array transitioning to Top Values was always showing an error in the UI.

@dej611
Copy link
Contributor Author

dej611 commented Jun 17, 2021

getErrorMessage: (layer, columnId, indexPattern) =>
    [
      ...(getInvalidFieldMessage(
        layer.columns[columnId] as FieldBasedIndexPatternColumn,
        indexPattern
      ) || []),
      getDisallowedTermsMessage(layer, columnId, indexPattern) || '',
    ].filter(Boolean),

This snippet of code was returning an empty array.
In the dimension_editor the return value of that method was used to determine whether the field had an error or not:

Because of the truthiness of the empty array transitioning to Top Values was always showing an error in the UI.

Sorry wrong ref there, same name but different implementation.
The getErrorMessage was called within the canTransition function here:

@flash1293
Copy link
Contributor

Got it, thanks. Could we adjust that place instead and do

!newDefinition.getErrorMessage?.(newLayer, columnId, indexPattern)?.length

In a bunch of other places we already treat empty array as no error, I think we should do the same here

@dej611
Copy link
Contributor Author

dej611 commented Jun 17, 2021

I've added a check for the array length as well.
Ideally the getErrorMessage should return undefined on errors, and all of them but date_histogram do now. The length check is meant to be a fallback.

If you think that should be the only fix let me know.

Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM, didn't test again

@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 1.5MB 1.5MB +175.0B

History

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

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 18, 2021
…ets-tab

* 'master' of github.com:elastic/kibana: (93 commits)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  [Maps] clean up feature editing name space to avoid conflicts with layer settings editing (elastic#102516)
  [canvas] Refactor Storybook from bespoke to standard configuration (elastic#101962)
  [Security Solution] adds wrapSequences method (RAC) (elastic#102106)
  [FTR] Stabilize SSLP functional tests (elastic#102553)
  [K8] Added `Inter` font files for new theme (elastic#102359)
  [Workplace Search] Convert Groups pages to new page template (elastic#102449)
  [DOC] Add experimental disclaimer to rollup jobs (elastic#95624)
  [Security Solution][Endpoint] Suppress some of the jest console.error noise created by endpoint list middelware (elastic#102535)
  [Fleet] Improve performance of Fleet setup (elastic#102219)
  [Alerting] Add event log entry when a rule starts executing (elastic#102001)
  [Fleet] Update docker image of registry used in integration tests (elastic#101911)
  [Asset Management] Osquery telemetry updates (elastic#100754)
  Converts saved object tagging to new management layout (elastic#102284)
  ...

# Conflicts:
#	x-pack/plugins/fleet/kibana.json
kibanamachine added a commit that referenced this pull request Jun 18, 2021
#102384) (#102611)

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 21, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (447 commits)
  skip flaky suite (elastic#102366)
  [Security Solution][Endpoint][Host Isolation] Isolation status badge from alert details (elastic#102274)
  Add email connector info for Elastic Cloud (elastic#91363)
  [Workplace Search] remove or replace xs props for text on source connect view (elastic#102663)
  Do not double register dashboard url generator (elastic#102599)
  [TSVB] Replaces EuiCodeEditor 👉 Monaco editor  (elastic#100684)
  [Discover] Update kibana.json adding owner and description (elastic#102292)
  [Exploratory View] Mobile experience (elastic#99565)
  chore(NA): moving @kbn/ui-shared-deps into bazel (elastic#101669)
  [TSVB] Index pattern select field disappear in Annotation tab (elastic#102314)
  [Security Solution][Endpoint][Host Isolation] Fixes bug where host isolation/unisolation works from alert details (elastic#102581)
  TSVB visualizations with no timefield do not render after upgrading from 7.12.1 to 7.13.0 (elastic#102494)
  [Logs UI] Add `event.original` fallback to message reconstruction rules (elastic#102236)
  [ML] Remove blank job definition as it is unused and out-of-sync with Elasticsearch (elastic#102506)
  [Lens] Fix wrong error detection on transition to Top values operation (elastic#102384)
  [ML] Anomaly detection job custom_settings improvements (elastic#102099)
  [Cases] Route: Get all alerts attach to a case (elastic#101878)
  Fixes wrong list exception type when creating endpoint event filters list (elastic#102522)
  remove search bar that's not working yet (elastic#102550)
  Migrated Ingest Node Pipeline Functional Tests to use test_user (elastic#102409)
  ...

# Conflicts:
#	x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Lens] Transitioning to Top Values from Intervals operation leads to a field error
4 participants