-
Notifications
You must be signed in to change notification settings - Fork 2.9k
CSV file import #6561
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
CSV file import #6561
Conversation
jeremystretch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start, thank you for your contribution. Just needs a bit of cleaning up before we can merge it.
| # Iterate through CSV data and bind each row to a new model form instance. | ||
| with transaction.atomic(): | ||
| headers, records = form.cleaned_data['csv'] | ||
| if request.FILES: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a validation method to the form to check for both csv and upload_csv set, and raise a ValidationError if so as a sanity check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a method for this within the _import_form definition
jlost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
|
@jeremystretch This would be useful to me, and it looks like all requested changes have been addressed. Could you give this another peek when you have a chance? |
|
Might need a bit of cleanup (e.g. stripping whitespace from uploaded files) but I can take care of that. Thanks! |
Fixes: #6560
An example / first draft of a solution for #6560 .
In the current implementation, if users try to use both the
CSVFileFieldand theCSVDataField, theCSVDataFieldwill be ignored and only the objects in the file will be imported. It should be unusual that both fields are populated, but let me know if you prefer to handle that case differently, such as by raising a validation error. The csv file is checked for errors (incorrect formatting, objects already exist, invalid columns) before importing.This solution involves some duplication between
CSVFileFieldandCSVDataField. If you prefer, I could remove most of the duplication by refactoring them to both inherit from a single class - say,CSVParsingField.