[Data Table] Remove extra column in split mode #83193

sulemanof · 2020-11-11T17:26:05Z

Summary

Fixes #73814

Initial issue:
when using Split table by column mode, the "splitting column" is repeated inside each row of all tables

The regression was caused by this pr: #54127

Checked in kibana 7.5.1 - the issue is not reproducible there:

After the fix:

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)
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 · 2020-11-12T07:26:03Z

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

alexwizp · 2020-11-12T09:01:42Z

src/plugins/vis_type_table/public/legacy/agg_table/agg_table.js


          $scope.formattedColumns = table.columns
            .map(function (col, i) {
              const isBucket = buckets.find((bucket) => bucket.accessor === i);
-              const isSplitColumn = splitColumn


👍 I didn't find any cases why this code needed.

alexwizp

LGTM! Tested locally. cvs export works correctly

stratoula · 2020-11-12T14:44:26Z

@sulemanof my only concern is that now the export in CSV doesn't have this column. So while when I split table (rows mode) the exported CSV has the the "splitting column", when I export it from columns mode it doesn't have it.

Rows mode

Columns mode

alexwizp · 2020-11-12T15:08:28Z

@stratoula Hm... I tested that case. If it's true I should remove my approval.

sulemanof · 2020-11-12T15:46:13Z

@sulemanof my only concern is that now the export in CSV doesn't have this column. So while when I split table (rows mode) the exported CSV has the the "splitting column", when I export it from columns mode it doesn't have it.

Rows mode

Columns mode

The main question is: do we need this column in exported csv if the actual table doesn't have it?
I also want to highlight that this column is presented in a non-formatted exported file

stratoula · 2020-11-12T15:52:32Z

This is a good question 🙂 From one point of view, it is good that is exported as it contains the splitting column info, from the other hand it is not displayed in the table so we could omit it. But my main point is that if it is not exported on column mode, shouldn't be exported on row mode. Can you check how it worked on 7.5.1?

sulemanof · 2020-11-17T09:27:55Z

This is a good question 🙂 From one point of view, it is good that is exported as it contains the splitting column info, from the other hand it is not displayed in the table so we could omit it. But my main point is that if it is not exported on column mode, shouldn't be exported on row mode. Can you check how it worked on 7.5.1?

The problem is in 7.5.1 the formatted export was not working at all, and the fix #54127 introduced the bug of extra column appeared 😄

The same issue is reproducible in earlier versions, I checked in 7.2.1, 7.0.1
You could see the problem in code: the columns is an array without the split column, but rows still have values of the split column:

The working feature was found in 6.8.1 & 6.5.1 : the split column does not exist neither in formatted export nor in raw!

6.5.1 - formatted export :

6.8.1 - raw export :

FMPOV, the formatted export should not contain the split column in any case, since it is not presented in the table.
But then I found the issue #57096, which describes the opposite 🤷‍♂️
@markov00 do you have anything to add here?

sulemanof · 2020-11-17T09:28:19Z

@elasticmachine merge upstream

markov00 · 2020-11-17T14:38:16Z

FMPOV, the formatted export should not contain the split column in any case, since it is not presented in the table.
But then I found the issue #57096, which describes the opposite 🤷‍♂️
@markov00 do you have anything to add here?

Hey @sulemanof I think the linked issue doesn't actually indicate if we need that additional column or not, it just points out that there was a missing header on the exported table.

…olumn

…into fix/table_split_column

sulemanof · 2020-11-19T12:17:24Z

The PR updated.
Currently a table exports only visible data, as it was in kibana 6.x

kibanamachine · 2020-11-19T13:54:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 51fc8e8

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
`visTypeTable`	67.1KB	66.6KB	-512.0B

History

💚 Build #88246 succeeded fe76033
💚 Build #87395 succeeded 2eb8589

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

stratoula

LGTM! I tested it locally, the splitted column/row is not exported. Thank you @sulemanof ❤️

* Fix extra column in split table * Update table exports Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: skip "Dashboards linked by a drilldown are both copied to a space" (elastic#83824) [alerts] adds action group and date to mustache template variables for actions (elastic#83195) skip flaky suite (elastic#79389) [DOCS] Reallocates limitations to point-of-use (elastic#79582) [Enterprise Search] Engine overview layout stub (elastic#83756) Disable exporting/importing of templates. Optimize pitch images a bit (elastic#83098) [DOCS] Consolidates plugins (elastic#83712) [ML] Space management UI (elastic#83320) test just part of the message to avoid updates (elastic#83703) [Data Table] Remove extra column in split mode (elastic#83193) Improve snapshot error messages (elastic#83785) skip flaky suite (elastic#83773) skip flaky suite (elastic#83771) skip flaky suite (elastic#65278) skip flaky suite (elastic#83793) [Task Manager] Ensures retries are inferred from the schedule of recurring tasks (elastic#83682) [index patterns] improve index pattern cache (elastic#83368) [Fleet] Rename ingestManager plugin ID fleet (elastic#83200) fixed pagination in connectors list (elastic#83638)

* Fix extra column in split table * Update table exports Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Fix extra column in split table * Update table exports Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Fix extra column in split table

2eb8589

sulemanof requested review from majagrubic, stratoula and alexwizp November 11, 2020 17:35

sulemanof added Feature:Data Table Data table visualization feature regression release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.1 v7.11 v8.0.0 labels Nov 11, 2020

sulemanof marked this pull request as ready for review November 12, 2020 07:26

sulemanof requested a review from a team November 12, 2020 07:26

alexwizp reviewed Nov 12, 2020

View reviewed changes

alexwizp approved these changes Nov 12, 2020

View reviewed changes

spalger added v7.11.0 and removed v7.11 labels Nov 12, 2020

Merge branch 'master' into fix/table_split_column

fe76033

sulemanof added 3 commits November 19, 2020 12:45

Merge remote-tracking branch 'upstream/master' into fix/table_split_c…

e1ab7e1

…olumn

Update table exports

4ad1283

Merge branch 'fix/table_split_column' of github.com:sulemanof/kibana …

51fc8e8

…into fix/table_split_column

stratoula approved these changes Nov 19, 2020

View reviewed changes

alexwizp approved these changes Nov 19, 2020

View reviewed changes

sulemanof merged commit b263145 into elastic:master Nov 19, 2020

sulemanof deleted the fix/table_split_column branch November 19, 2020 15:02

sulemanof mentioned this pull request Nov 19, 2020

[7.x] [Data Table] Remove extra column in split mode (#83193) #83807

Merged

sulemanof removed the v7.10.1 label Nov 19, 2020

sulemanof mentioned this pull request Nov 30, 2020

[Data table] Reactify & EUIficate the visualization #70801

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data Table] Remove extra column in split mode #83193

[Data Table] Remove extra column in split mode #83193

sulemanof commented Nov 11, 2020 •

edited

Loading

elasticmachine commented Nov 12, 2020

alexwizp Nov 12, 2020

alexwizp left a comment

stratoula commented Nov 12, 2020

alexwizp commented Nov 12, 2020

sulemanof commented Nov 12, 2020

stratoula commented Nov 12, 2020

sulemanof commented Nov 17, 2020

sulemanof commented Nov 17, 2020

markov00 commented Nov 17, 2020

sulemanof commented Nov 19, 2020

kibanamachine commented Nov 19, 2020

stratoula left a comment

[Data Table] Remove extra column in split mode #83193

[Data Table] Remove extra column in split mode #83193

Conversation

sulemanof commented Nov 11, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Nov 12, 2020

alexwizp Nov 12, 2020

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

stratoula commented Nov 12, 2020

alexwizp commented Nov 12, 2020

sulemanof commented Nov 12, 2020

stratoula commented Nov 12, 2020

sulemanof commented Nov 17, 2020

sulemanof commented Nov 17, 2020

markov00 commented Nov 17, 2020

sulemanof commented Nov 19, 2020

kibanamachine commented Nov 19, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

History

stratoula left a comment

Choose a reason for hiding this comment

sulemanof commented Nov 11, 2020 •

edited

Loading