[Lens] Add validation on operation with no fields

[dej611]

Summary

Fixes #89032

This PR adds one more level of validation to the datasource, checking for inconsistencies between the data and the visualization states.

Note: datatable has an additional issue #98288 when cleaning up field information, hence it was not possible to show Rows and Columns filled with empty dimensions.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[elasticmachine]

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

[dej611]

@elasticmachine merge upstream

[flash1293]

It's still rendered as an empty dimension - should we make sure to also show an error in the dimension trigger similar to how calculations handle this? The original issue mentions this.

[dej611]

Yes, I thought about that. I guess we have to decide what to show: in the calculation case it's easier as the referenced column is gone, but the referencing one still holds the label.
In this case there's no information about the type of operation there, it's just visualization state.

We could have a placeholder for this case. What do you think?

[flash1293]

@dej611 Not sure what you have in mind as placeholder, but in this situation the visualization still thinks there's a column while the datasource removed it, right? We could detect this situation and render (warning triangle) Incomplete configuration in the dimension trigger.

[dej611]

Yes, I meant "what to write in the space" as placeholder. I'll add the proposed copy.

[dej611]

Updated the PR with some tiny refactoring + now showing the incomplete configuration message:

[wylieconlon]

I tested this and found a new error that this introduces:

1. Create 2 metrics
2. Delete the field of one metric
3. Drag the metric to change its order, and you get an error in

export function hasField(column: BaseIndexPatternColumn): column is FieldBasedIndexPatternColumn {
  return 'sourceField' in column;
}

[flash1293]

Good point @wylieconlon . This makes me think whether this is the right way of handling backspace for the field in the first place:

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

Line 198 in f667aa2

onDeleteColumn();

Should we just set the field property to empty string or something like this and treat that as a validation error on the datasource side? I think it would keep the rest of the system working and would be easily detectable in the validation logic? What do you think @dej611

[mbondyra]

@flash1293 would we use your proposal (setting a field to the empty string) when we switch between operations and the field is not compatible? We have two similar cases:

1. Setup the operation A. Remove the field. Close popover -> you get the empty dimension
2. Setup the operation A. Change to the operation B that doesn't accept the same field. Close popover. You get the last correct state. (operation A)

Maybe it's not for this task, it just we have a lot of inconsistency here and just wanted to mention that.

[flash1293]

@mbondyra IMHO it's nice the last valid state is kept in this case. That got me thinking - maybe we should just not do anything if the user tries to delete the field?

We can totally do this - it's as simple as removing the onDeleteColumn callback. I just tried it and the backspace keystroke is simply ignored. It has the additional advantage of being very simple to implement :)

[mbondyra]

Agreed, I also like this option better, let's just test it for fullReference operations too (if it's not worse than the bug we have here #97431)

[dej611]

@flash1293 backspace does not do anything, but whitespace still clears the field input.
The latter has no effect on the state, but if you close the dropdown it's still empty and "invalid". Closing the flyout will restore the previous valid state:

I think it's still acceptable as behaviour, just documenting it. If we all agree on that I can do on that direction.

[flash1293]

@dej611 Good point, I think that's fine

[dej611]

Ok, I'll create a separate PR as this one will need to be completely reverted 😅

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: c2f08fe
• Storybooks Preview

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	965.1KB	966.4KB	+1.3KB

History

• 💚 Build #122742 succeeded c2f08fe
• 💚 Build #122596 succeeded f380256
• 💚 Build #122333 succeeded 334ab45

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