Fix crash on downloading an empty dataset as CSV #929

Skn0tt · 2019-09-13T12:31:49Z

When downloading an empty dataset as CSV, there is a crash because you cannot .trim() an array.

This PR adds a fix and a regression test.

coveralls · 2019-09-16T01:40:11Z

Coverage increased (+0.5%) to 77.835% when pulling be5b934 on Skn0tt:master into 2d37c38 on gregnb:master.

Skn0tt · 2019-09-28T11:39:27Z

If somebody needs this fix before it's reviewed, they can use the published PR package: https://github.com/Skn0tt/mui-datatables/packages/28551

gabrielliwerant

Hey @Skn0tt, thanks for working on this. A couple of things.

I'd like fixes to be as parsimonious as possible. Refactoring, if necessary and desirable, should be handled separately so we don't have to worry as much about regressions and fixes at the same time. Looks like all that's really needed is to add something like

if (!data.length) return '';

before this line https://github.com/gregnb/mui-datatables/blob/master/src/utils.js#L49. Not sure that anything else is required here for the time being.

The testing is also greatly appreciated, but the format and description of the tests doesn't match the others in the library, so it would be better if they did.
Removing obsolete old code is also nice, but again, I'd prefer these change sets be kept separate as it makes testing and reviewing PRs far easier and it allows far more versioning options later if something needs to be reverted, for example.

Skn0tt · 2019-10-08T19:05:00Z

Hey @gabrielliwerant, thanks for reviewing my code.

I tried to make this PR easy to review by dividing up my thoughts into individual commits so it can be reviewed step by step.
I consider regression tests as necessary. Please have a look at the code before I extracted parts of it - there were side effects all over the place, basically untestable. That's why I decided to refactor it.
Do you mean the removal of unused imports by "removing old code"? This was one of the warnings that eslint gave me.

I see that if (!data.length) return ''; would have been easier to review since it perfectly fits the stated issue.
On the other hand, I consider the underlying issue to be in the initial value of the reduce call (it just doesn't make sense to use [] and then make a string out of it after the first run, does it?)

gabrielliwerant

You're right about the reduce function and the need to refactor in order to test, and I would like to include your PR as a more proper fix in the next minor release. The difficultly though is that this takes time, and requires more detailed review, and there are places where I might like to quibble, leading to back and forth and more time spent, etc. So your PR just serves a different function in my mind that an immediate fix would, as I'd be able to deploy that sooner as a patch version.

Agreed on regression testing being really important, but a few notes on the testing. I actually want us to use assert instead of expect to keep things consistent. There's literally only a couple uses of expect now, so I'd eventually like to switch those over. Also, I'd prefer fewer describe blocks, again, for consistency's sake. I'd prefer a structure similar to most of the rest of the tests where there are longer, more explanatory it statements, and a more linear structure to the test output messages. There's nothing at all wrong with your approach, to be clear, and may even be better in many situations, but I just want the consistency to make life easier for myself and future contributors.

src/utils.js

test/utils.test.js

Skn0tt · 2019-10-26T11:52:06Z

Thanks a lot for your response. I totally understand that you want to keep the code base consistent. I've made the requested changes and converted from using expect to comply wit assert usage.

gabrielliwerant

Thanks for your understanding and for continuing to work on this.

A couple more comments for you, but we're getting close!

gabrielliwerant · 2019-10-28T21:30:54Z

src/utils.js

@@ -74,15 +74,20 @@ function createCSVDownload(columns, data, options) {
    ? options.onDownload(buildHead, buildBody, columns, data)
    : `${CSVHead}${CSVBody}`.trim();

-  if (options.onDownload && csv === false) {
-    return;
+  const downloadHasBeenAborted = csv === false;


I'm not sure that assigning to a variable here that is only used once just below is clearer than using options.onDownload. This part wasn't broken before, so I think let's just leave it as it was for now.

gabrielliwerant · 2019-10-28T21:31:33Z

src/utils.js

+  return csv;
+}
+
+function downloadCSV(csv, filename) {


Great idea to break this out into its own function! Helps a lot with the testing.

gabrielliwerant · 2019-10-28T21:34:22Z

test/utils.test.js

+      },
+    ];
+
+    describe('when given two rows', () => {


So, I think we still have too many nested describes. If you look at the getPageValue tests, you'll see that we just use one describe block for the whole function, breaking up the bits into it blocks instead. So, this one would become it('renders a two-record csv when given two rows... Let's do that for all the new tests here, just one describe for assembleCSV and everything else is an it block.

@Skn0tt

Inspired by work done by @Skn0tt in #929

gabrielliwerant · 2019-11-15T02:46:44Z

I put this on hold and continued in a separate PR here #1064. Feel free to take a look and offer feedback.

@Skn0tt

* Fix issue with download options causing side effects * Separate out download function from CSV assembly * Use string as intial value for CSV This problem was pointed out astutely by @Skn0tt * Add tests and refactor to support tests Inspired by work done by @Skn0tt in #929 * Remove unused code from test

gabrielliwerant · 2019-11-22T01:29:52Z

Fix should now be in 2.13.0.

@Skn0tt

* Fix issue with download options causing side effects * Separate out download function from CSV assembly * Use string as intial value for CSV This problem was pointed out astutely by @Skn0tt * Add tests and refactor to support tests Inspired by work done by @Skn0tt in gregnb#929 * Remove unused code from test

Skn0tt added 3 commits September 13, 2019 13:10

Extract CSV rendering into its own function

b7ac785

Create regression test

8518167

Fix crash on downloading empty csv

1ca5a18

gabrielliwerant self-requested a review September 23, 2019 20:57

gabrielliwerant added the needs review Useful to mark PRs for what's up next to review label Sep 23, 2019

Run Prettier

1f4af34

Remove unused imports

8785baa

gabrielliwerant mentioned this pull request Sep 30, 2019

Uncaught TypeError when clicking on "Download CSV" with no data in the table #967

Closed

gabrielliwerant requested changes Sep 30, 2019

View reviewed changes

gabrielliwerant added needs work and removed needs review Useful to mark PRs for what's up next to review labels Sep 30, 2019

bkiac mentioned this pull request Oct 4, 2019

Fix crash on downloading empty table as CSV #984

Merged

gabrielliwerant requested changes Oct 15, 2019

View reviewed changes

src/utils.js Show resolved Hide resolved

gabrielliwerant added the enhancement label Oct 15, 2019

gabrielliwerant reviewed Oct 15, 2019

View reviewed changes

test/utils.test.js Outdated Show resolved Hide resolved

Skn0tt added 4 commits October 26, 2019 13:38

Use "utils.js" to avoid ambiguity

07c797e

Allow download to be switched off

9343413

Merge branch 'master' of https://github.com/gregnb/mui-datatables

d7ee2a0

Use assert instead of expect

be5b934

gabrielliwerant requested changes Oct 28, 2019

View reviewed changes

gabrielliwerant added the on hold label Nov 15, 2019

gabrielliwerant added a commit that referenced this pull request Nov 15, 2019

Add tests and refactor to support tests

2840759

Inspired by work done by @Skn0tt in #929

gabrielliwerant mentioned this pull request Nov 15, 2019

Bugfix/issue 1052 csv download issues #1064

Merged

gabrielliwerant closed this Nov 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash on downloading an empty dataset as CSV #929

Fix crash on downloading an empty dataset as CSV #929

Skn0tt commented Sep 13, 2019

coveralls commented Sep 16, 2019 •

edited

Loading

Skn0tt commented Sep 28, 2019

gabrielliwerant left a comment

Skn0tt commented Oct 8, 2019

gabrielliwerant left a comment •

edited

Loading

Skn0tt commented Oct 26, 2019

gabrielliwerant left a comment

gabrielliwerant Oct 28, 2019

gabrielliwerant Oct 28, 2019

gabrielliwerant Oct 28, 2019

gabrielliwerant commented Nov 15, 2019

gabrielliwerant commented Nov 22, 2019

Fix crash on downloading an empty dataset as CSV #929

Fix crash on downloading an empty dataset as CSV #929

Conversation

Skn0tt commented Sep 13, 2019

coveralls commented Sep 16, 2019 • edited Loading

Skn0tt commented Sep 28, 2019

gabrielliwerant left a comment

Choose a reason for hiding this comment

Skn0tt commented Oct 8, 2019

gabrielliwerant left a comment • edited Loading

Choose a reason for hiding this comment

Skn0tt commented Oct 26, 2019

gabrielliwerant left a comment

Choose a reason for hiding this comment

gabrielliwerant Oct 28, 2019

Choose a reason for hiding this comment

gabrielliwerant Oct 28, 2019

Choose a reason for hiding this comment

gabrielliwerant Oct 28, 2019

Choose a reason for hiding this comment

gabrielliwerant commented Nov 15, 2019

gabrielliwerant commented Nov 22, 2019

coveralls commented Sep 16, 2019 •

edited

Loading

gabrielliwerant left a comment •

edited

Loading