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
Fix broken CSV export from data table. #34131
Conversation
Pinging @elastic/kibana-app |
@@ -30,21 +30,21 @@ import { fieldFormats } from '../../../registry/field_formats'; | |||
|
|||
const config = chrome.getUiSettingsClient(); | |||
|
|||
const defaultFormat = { convert: identity }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird thing I noticed when writing tests: the way we had defined the default formatter, the same object was being passed to every field that used the default format, which meant mutating convert
on one field would actually change all the default fields.
I wasn't sure how likely it would be for this to cause problems in the real world, but went ahead and changed it anyway just to be safe.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2a0c802
to
f81b5e8
Compare
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, tested on chrome linux
Fixes #33581
In 7.0 and later, the clicking to download a raw or formatted CSV from the data table vis failed. I think this may have been introduced in #28746, though not 100% sure.
This updates
agg_table
to handle tabified data, and also makes sure thatformatter.convert
is called for each value when exporting formatted CSV.We have tests that would have caught this, but they were using mocked data in the old format so they were still passing & giving a false positive. I've updated those as well & added a few new tests.