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

Add csv file type validation for all platforms #5785

Merged
merged 2 commits into from
Feb 2, 2020

Conversation

trojanh
Copy link
Contributor

@trojanh trojanh commented Nov 6, 2019

Resolves N.A.
Impact: minor
Type: bugfix

Issue

Products filter by .csv file doesn't have file validation which makes any type of files to be uploaded causing the following error on browser console:
Screenshot 2019-11-06 at 5 43 40 PM

Solution

Adding following MIME type for csv validation works on all platforms

text/plain
text/x-csv
application/vnd.ms-excel
application/csv
application/x-csv
text/csv
text/comma-separated-values
text/x-comma-separated-values
text/tab-separated-values

ref: https://www.christianwood.net/csv-file-upload-validation/

Testing

  1. Go to products page
  2. Click on Actions dropdown and now click on Filter by file option
  3. Upload non-csv file and see the browser console after clicking on Filter Products

Signed-off-by: trojanh <rajan.tiwari@kiprosh.com>
Signed-off-by: trojanh <rajan.tiwari@kiprosh.com>
@mikemurray
Copy link
Member

Testing must occur on Windows before this PR is to be merged. We removed the validation because of an issue with windows. I hope that these new mime types do work, but again, it needs to be tested on Windows, not just on macOS or Linux.

@spencern
Copy link
Contributor

spencern commented Dec 9, 2019

To add on to what @mikemurray mentioned here - we've removed the MIME type validation because of an issue that was reported using Windows with the previous type validation we had

Your solution is more substantial than the initial text/csv type that was built in - I'm hopeful that this issue is still resolved with this PR. #5700

We need someone to test this PR on several browsers on Windows before merging this.

@trojanh
Copy link
Contributor Author

trojanh commented Dec 9, 2019

Thanks @spencern and @mikemurray, I couldn't test it too but I thought this solution will help achieve the wider goal of allowing variety of file types on different platforms. So let's wait till we have this tested by someone on different platforms.

@aldeed
Copy link
Contributor

aldeed commented Feb 2, 2020

Verified works with Excel CSVs and on Windows OS. I implemented the same changes in Reaction Admin 3.0 here: reactioncommerce/reaction-admin#197

Note that 2.x is in fixes-only phase, so this is getting merged but will only get released if another release is deemed necessary at some point.

@aldeed aldeed merged commit 42ba81c into reactioncommerce:develop Feb 2, 2020
@trojanh
Copy link
Contributor Author

trojanh commented Feb 3, 2020

awesome, thanks @aldeed , 👍

@kieckhafer kieckhafer mentioned this pull request Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants