snipe / snipe-it

A free open source IT asset/license management system
https://snipeitapp.com
GNU Affero General Public License v3.0
10.88k stars 3.14k forks source link

Export problems due to allowing commas #4495

Closed ebonweaver closed 6 years ago

ebonweaver commented 6 years ago

Expected Behavior (or desired behavior if a feature request)

Either Snipe will not accept input that contains a comma, or Snipe will translate or escape the comma on export


Actual Behavior

The comma is accepted and is exported, which makes your csv export broken as it's adding a delimiter that shouldn't be there.


Please confirm you have done the following before posting your bug report:


Provide answers to these questions:

Possible solutions:

HinchK commented 6 years ago

@ebonweaver the csv importer conforms with RFC 4180. http://tools.ietf.org/html/rfc4180. commas are acceptable as long as they are escaped.

from the RFC

Fields containing line breaks (CRLF), double quotes, and commas should be enclosed in double-quotes.

Excel and Google sheets will both escape commas and quotations for you during an export; and excel by default encapsulates all text cells in quotations.

ebonweaver commented 6 years ago

I fail to see what that has to do with this export issue, other than you seem to be supporting the issue that Snipe-It does not conform to expectations (or standards).

HinchK commented 6 years ago

forgive me, i initially read it as import - thanks @ebonweaver

snipe commented 6 years ago

@HinchK ☕️ ☕️ ☕️ ☕️ ☕️

snipe commented 6 years ago

Which export are you seeing this in?

ebonweaver commented 6 years ago

First, let me apologize profusely for believing my employees without looking at the UI myself.

Second, it seems we have two minor issues.
The issue with CSV export of assets is as stated, commas in fields obviously give you additional delimiters. Adding some ability to escape those may be nice, be it manual or automatic, but I can't disagree that if you use that export format you have to know not to put commas in your data. In all reality, use one of the other formats that my techs claimed didn't exist -_-

That leads us to the apparent real issue with the UI. It's apparently confusing even to tech people to have a big button that says Download CSV. This leads to the impression it's the ONLY format you can download, and none of them ever clicked the little download button in the toolbar right below it, as it took me 10 seconds to do when I looked myself finally. Now here I'd at least agree with them that it's wonky design. Why have two buttons for the same function that work different? Seems either the big Download CSV button can just be removed, or if it does something different than choosing CSV from the download button, that should be clarified?

So on both counts, not a bug, couple minor design/ feature improvement ideas.

snipe commented 6 years ago

Why have two buttons for the same function that work different?

@ebonweaver the reason for two downloads is due to a limitation/bug/confusion in the javascript table library we use. While that export exists, it will only export what's on that page, versus exporting ALL things. This was (obviously) causing some frustration from users who were trying to export their entire asset listing and would have to do it 20, 50, or 100 assets at a time and knit those CSVs together (ugh.)

So as a workaround, until we can figure out how to fix the table export javascript, we provide an export button outside of that UI. I know it's confusing, but frankly, we needed to solve the problem quickly for users who don't really care that it's not a bug in our code, but rather in a library we use. ¯_(ツ)_/¯

We'd prefer to use the built-in table exporter, since that provide additional options without a lot of extra coding - but it's also a library, which makes that tricksier.

In the meantime, we can fix the big button version, so at least it's exporting files that aren't donked.

snipe commented 6 years ago

And to make matters more confusing, I can't reproduce this in Excel for MacOS. :-/

screen shot 2017-11-30 at 7 37 59 pm
ebonweaver commented 6 years ago

hmmm... so there IS a reason, and a darn good one... So am I understanding right that the big button exports every record, but the little one with all the format options only exports what is on the current screen and not all the other results of the current filter? Or are you saying that ONLY applies to the CSV format and not the other formats? I'm not finding anything in the documentation about this when searching for "download" or "export".

ebonweaver commented 6 years ago

You exported that as a CSV and opened it in Excel and it somehow knew not to add delimits? That would not only be counter to our experience, but to the definition of CSV....

snipe commented 6 years ago

so there IS a reason, and a darn good one

Believe it or not, there usually is ;)

So am I understanding right that the big button exports every record, but the little one with all the format options only exports what is on the current screen

That's correct. The button hits the database for ALL assets, limited only by whatever status page you're on. For example, if you're on the All Deployed page and you hit that big button, it will export all of your deployed assets. But it doesn't give a hoot about any filtering you've done within the table itself - indeed, that button has no idea what's going on in the table below it.

The table export exports whatever is currently visible in the table (including visible/hidden columns). That's all it's smart enough to do currently (hence the need for the big button).

For example:

screen shot 2017-11-30 at 7 44 11 pm

Produces this table export CSV:

screen shot 2017-11-30 at 7 43 51 pm
snipe commented 6 years ago

That would not only be counter to our experience, but to the definition of CSV....

Well, in the raw CSV file itself, it's correctly escaping....

screen shot 2017-11-30 at 7 47 05 pm

The fputcsv method automagically handles that, I think.

Do you maybe have any other weird characters in your data that might be confusing things?

ebonweaver commented 6 years ago

Ok so doing some tests for myself...

Yeah, the download button re-renders the screen before download and wipes out all the other pages so you only get one page of results. Significant issue, because it means CSV is the ONLY format where download actually works, via the big button. It would be very useful to be able to get all assets right to Excel as well. I guess here's hoping the library gets fixed?

I'm also seeing that all downloads from the little button download twice. The first is less than half of what is in the second, but neither has all the records, only what fits on one screen because again it eliminates all other pages of the search results before downloading. So that seems to be an issue as well.

The two different CSV exports export different columns as well, and neither includes all fields, which means there is no export at all that actually gives you everything. That's an issue. It looks like the little button only does shown columns, which makes sense. The big button looks to try to do everything as you say, but fails. I see at least 3 missing columns, including asset name, device image, and category.

As you say, the export is escaping so I see no issue, which is puzzling. I tested both Mac and Windows and it's fine. I can only conclude once again my techs did something off the rails and unexpected, but heck if I know what. Possibly importing the CSV into Excel instead of just opening it is all I can guess at, but that's operational error on their part then.... sigh apologies once again that I didn't test for myself before believing the report from my own team -_-

Looks like the issue I tried to report isn't an issue, but there are other issues instead?

snipe commented 6 years ago

Unfortunately, the issue in the library has been ongoing for years, so I’m not super hopeful on a fix TBH. I monitor all of the threads related to the issues, but I haven’t seen a ton of progress. I think I can at least handle the full vs partial download, but it will take a little time. It’s been on my list for a while, but was less urgent because we have the big button. I didn’t realize the big button one was missing columns - that part is actually easy to fix, and I’m happy to handle that for you tomorrow.

snipe commented 6 years ago

Sigh. Didn’t mean to close that - am on mobile :(

Can you confirm for me which columns are missing? I’ll add them in tomorrow afternoon.

ebonweaver commented 6 years ago

Cool, at least I found something that legit is bugged and has a fix :) The 3 I mentioned certainly, I'll try to catch some time tomorrow to check the rest. It's hard to verify easily as they are not exported in order from the big button like the little one, so I have to look for each one across some 40 columns.

ebonweaver commented 6 years ago

Ah HA! So having interrogated my people, I have found out the real issue. Reports! If you generate a custom Asset report, it pulls down a CSV that does NOT escape commas, so the data is messed up. Having accurate info is SO helpful... I feel like we're now in a creep state on this issue, if you want me to open new and individual tickets I'll do so. If you're cool tracking these all here fine by me. Sorry again for the mess.

snipe commented 6 years ago

So, asset name is definitely showing for me. It's under the "Name" column.

screen shot 2017-12-01 at 2 43 16 pm

Category is missing. That's a bug - I just pushed out a fix.

Device image is omitted because we can't embed images in CSV, and it's not a very useful field for most folks as a text string. Those that want that text string should probably use the API. (It just creates lots of clutter in the CSV.)

Also fixed the custom report (and added some filters to it.)

screen shot 2017-12-01 at 2 56 48 pm
ebonweaver commented 6 years ago

Ah, that's literally a picture of the system, right, I was thinking image as in system software image. If it's just a blank column, why have it the file at all?

The two downloads using two different headers (Name vs Asset Name) threw me off on that one. May be minor, but may be good to have them match. Looks like Cost and Purchase Cost may be the same deal?

Not seeing ID (questionable if useful), Warranty, Warranty Expires, Employee No, Created At, Updated At, or Expected Checkin Date columns in the Downloaded CSV. Can you verify those?

Did you perchance add a Select All button on the custom asset report screen? That would be sweet, lot of check boxes.

snipe commented 6 years ago

If it's just a blank column, why have it the file at all?

I'm not sure which export you're talking about now. I don't think it's in the big button or custom field export. It will only export in the table export if you have that column selected.

The two downloads using two different headers (Name vs Asset Name) threw me off on that one. May be minor, but may be good to have them match. Looks like Cost and Purchase Cost may be the same deal?

Yeah, it's on the list of things to do.

Not seeing ID (questionable if useful),

It's only used internally for Snipe-IT, so not particularly useful - and was confusing people, as they thought it was an asset tag.

Warranty, Warranty Expires, Employee No, Created At, Updated At, or Expected Checkin Date columns in the Downloaded CSV. Can you verify those?

At this point, why use the Download CSV at all? Why not just use the custom report?

Did you perchance add a Select All button on the custom asset report screen? That would be sweet, lot of check boxes.

Per the screenshot, yes I did.

ebonweaver commented 6 years ago

Yeah side track, and I guess the fact that it's a column shown means it's a column downloaded (little button) even if it is blank.

At this point good question, maybe just remove the big download csv button if the custom report does the same and more? Sweet additions on the report!

snipe commented 6 years ago

The only reason I can think of to not just direct people to the custom report would be if they don't have reporting permissions.

I've switched that button to point to the custom report. We'll see how many people freak out.

Here's what the custom report form looks like now.

screen shot 2017-12-01 at 5 07 38 pm