[Lens] Fix wrong error detection on transition to Top values operation by dej611 · Pull Request #102384 · elastic/kibana

dej611 · 2021-06-16T17:26:20Z

Summary

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.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2021-06-17T08:24:31Z

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

flash1293

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 · 2021-06-17T13:43:22Z

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:

kibana/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

Line 333 in 0cfd04c

const fieldErrorMessage =

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

dej611 · 2021-06-17T13:55:50Z

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:

kibana/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

Line 333 in 0cfd04c

const fieldErrorMessage =

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:

kibana/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

Line 175 in 0cfd04c

compatibleWithCurrentField: canTransition({

flash1293 · 2021-06-17T13:58:27Z

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 · 2021-06-17T14:33:07Z

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.

flash1293

LGTM, didn't test again

kibanamachine · 2021-06-17T16:34:49Z

💚 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

💚 Build #131785 succeeded 1b9f7e7

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

elastic#102384)

kibanamachine · 2021-06-18T08:48:22Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

…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

#102384) (#102611) Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

…-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

🐛 Fix transition to Top values from other operations

1b9f7e7

dej611 marked this pull request as ready for review June 17, 2021 08:24

dej611 requested a review from a team June 17, 2021 08:24

flash1293 reviewed Jun 17, 2021

View reviewed changes

🐛 Check also array length for errors

6b5add5

flash1293 approved these changes Jun 17, 2021

View reviewed changes

dej611 merged commit ae99824 into elastic:master Jun 18, 2021

dej611 deleted the fix/101758 branch June 18, 2021 08:45

kibanamachine mentioned this pull request Jun 18, 2021

[7.x] [Lens] Fix wrong error detection on transition to Top values operation (#102384) #102611

Merged

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 18, 2021

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

1dfe6ea

elastic#102384)

kibanamachine added a commit that referenced this pull request Jun 18, 2021

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

0e04be0

#102384) (#102611) Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>

flash1293 mentioned this pull request Jun 21, 2021

[Lens] "Top Values" allows setting missing values switch for numeric fields #102640

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[Lens] Fix wrong error detection on transition to Top values operation#102384
dej611 merged 2 commits intoelastic:masterfrom
dej611:fix/101758

dej611 commented Jun 16, 2021

Uh oh!

elasticmachine commented Jun 17, 2021

Uh oh!

flash1293 left a comment

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

flash1293 commented Jun 17, 2021

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

flash1293 left a comment

Uh oh!

kibanamachine commented Jun 17, 2021

Uh oh!

kibanamachine commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dej611 commented Jun 16, 2021

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Jun 17, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

flash1293 commented Jun 17, 2021

Uh oh!

dej611 commented Jun 17, 2021

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jun 17, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Jun 18, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants