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

[Datatable] missing percentage column and wrong headers on export formatted csv #66883

Merged
merged 8 commits into from
May 27, 2020

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 18, 2020

Summary

Fixes #49149. Percentage column is missing in formatted csv.
Fixes #57096. Wrong formatted CSV when splitting tables.

Have tested it on Chrome, Safari and FF. Both raw and formatted CSVs are exported as expected.
Moreover the order of the header columns was different from the rows so the data were not displayed correctly in some cases (under the corresponding header).

Preview of formatted csv with percentages:
Screenshot 2020-05-20 at 10 54 12 AM

Preview of formatted csv when splitting tables:
Screenshot 2020-05-20 at 10 59 59 AM

@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch from 2105a0e to 0272e36 Compare May 18, 2020 11:48
@stratoula stratoula self-assigned this May 18, 2020
@stratoula stratoula added release_note:fix bug Fixes for quality problems that affect the customer experience Feature:Data Table Data table visualization feature v8.0.0 v7.9.0 labels May 18, 2020
@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch 2 times, most recently from 210da56 to 6d397db Compare May 19, 2020 10:54
@stratoula stratoula changed the title Datatable/missing percentage csv [Datatable] missing percentage column and wrong headers on export formatted csv May 19, 2020
@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch 3 times, most recently from f4f6835 to 5b2bf7a Compare May 19, 2020 14:28
@stratoula stratoula marked this pull request as ready for review May 20, 2020 08:01
@stratoula stratoula requested a review from a team May 20, 2020 08:01
@stratoula stratoula added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label May 20, 2020
@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

1 similar comment
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@kertal kertal self-requested a review May 25, 2020 06:26
@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch from 3312df7 to ac9e6ef Compare May 25, 2020 06:50
@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch from ac9e6ef to ac428ef Compare May 25, 2020 07:24
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Quick question while testing, could you take a look at the split table output, there seems to be a difference to current master, since columns 1 isn't displayed in master, I think it should only be part of the csv export?

Master:
Bildschirmfoto 2020-05-25 um 13 06 38

PR (Note the redundant first column):
Bildschirmfoto 2020-05-25 um 12 51 25
thx!

@stratoula
Copy link
Contributor Author

I will! Thanx @kertal I wasn't sure about it tbh ;)

@stratoula stratoula requested a review from kertal May 26, 2020 06:24
}
return escape(v);

if (!column) return;
Copy link
Member

Choose a reason for hiding this comment

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

Have a question to understand this part. The change in this implementation is, if there's no column available for a rows's property, don't add it to the column. This sounds valid, and an improvement. Just try to understand, how could that be the case? Are there row properties with no columns? Just checking to prevent some kind of regression here. Because then there is a nice potential to simplify the code here in a way like this

for (const col of columns) {
   const value = row[col.id];
   const formattedValue = formatted && col.formatter
      ? escape(col.formatter.convert(v))
      : escape(v);
   rowArray.push(formattedValue);
}

Copy link
Contributor Author

@stratoula stratoula May 26, 2020

Choose a reason for hiding this comment

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

Yes there is a case where the user has selected two buckets and has ticked the Show partial rows. On the table you can't see any change but when the CSV is downloaded an extra column is added but as there is no extra header, it is messed up. So the question here is which is the expected behavior? As far as I can't see any change on the table, I expect to see the same on the CSV, right? If not this is another bug that has to do with the partial rows

Copy link
Member

Choose a reason for hiding this comment

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

synced with @stratoula, and she showed me another issue in the current implementation, fixed by this PR, so here the example described in the recent comment,

Screenshot 2020-05-26 at 5 52 01 PM

Exporting this table led to an formatted CSV export with an extra column
"extension.keyword: Descending","geo.dest: Descending",Count
,624,122,CN
,624,96,IN
,624,51,US
,624,23,ID
,624,22,PK
gz,285,63,CN
gz,285,52,IN
gz,285,16,US
gz,285,9,NG
gz,285,7,ID

This is also fixed by this PR, and therefore it's safe to refactor the existing code in the suggested way

@stratoula stratoula force-pushed the datatable/missing-percentage-csv branch from ba9c1a8 to b9b0de8 Compare May 26, 2020 15:39
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM. 2 + 1 issues solved by a single PR 👏 . Tested locally with Safari, Firefox, Chrome. Works!

@stratoula stratoula requested a review from kertal May 27, 2020 09:36
@stratoula stratoula merged commit 123068c into elastic:master May 27, 2020
stratoula added a commit to stratoula/kibana that referenced this pull request May 27, 2020
…matted csv (elastic#66883)

* Fixes the bug with percentage column missing in formatted csv

* fix tests

* Fix: Wrong formatted CSV when splitting tables

* On split table the split row should be added as a column only on the exported csv

* eslint

* fix test

* Improve the loop on the columns and rows

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
stratoula added a commit that referenced this pull request May 27, 2020
…matted csv (#66883) (#67448)

* Fixes the bug with percentage column missing in formatted csv

* fix tests

* Fix: Wrong formatted CSV when splitting tables

* On split table the split row should be added as a column only on the exported csv

* eslint

* fix test

* Improve the loop on the columns and rows

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Data Table Data table visualization feature release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong formatted CSV when splitting tables Percentage column is missing in formatted csv
4 participants