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

Feature/data uploads #33

Merged
merged 11 commits into from
Aug 23, 2019
Merged

Feature/data uploads #33

merged 11 commits into from
Aug 23, 2019

Conversation

tsubik
Copy link
Contributor

@tsubik tsubik commented Aug 13, 2019

Bulk upload with keeping the history of uploads
uploading

@tsubik
Copy link
Contributor Author

tsubik commented Aug 13, 2019

When we have import errors nothing is imported until all errors are resolved. Not sure if that is good behavior @simaob

import_errors

@simaob
Copy link
Contributor

simaob commented Aug 13, 2019

When we have import errors nothing is imported until all errors are resolved. Not sure if that is good behavior @simaob

import_errors

In Climate Watch we were inserting the data that got in, and showing the errors, maybe that's a better behaviour? But we'd need to ensure that no duplicates would go in.

Btw, I tried to import a non utf-8 file (by mistake actually) and the app crashed, but I just noticed that the PR is still WIP so I can wait until it's ready!

@tsubik
Copy link
Contributor Author

tsubik commented Aug 13, 2019

Well, I'm not sure what could be better, if forcing the user to submit only valid ones wouldn't be better. Maybe we should ask. I will take a look at that UTF-8 hell stuff.

@tsubik
Copy link
Contributor Author

tsubik commented Aug 13, 2019

It's already taking some time to import a couple of hundreds of rows with some associations. Maybe we would need a background job right away.

@simaob
Copy link
Contributor

simaob commented Aug 13, 2019

OK, maybe we won't be able to escape it.

@kowal
Copy link
Contributor

kowal commented Aug 14, 2019

PR looks great so far 👍

I have one comment regarding naming/architecture. Right now DataUpload calls uploader service - Upload::Litigations - I think that this service doesn't need to know about upload context -it is more like CsvImportService, which could be used both from DataUpload and from rake tasks import jobs. In other words, could we extract a service that works on CSV files (local or S3) and can be used from both user uploads and command line layers.

@tsubik
Copy link
Contributor Author

tsubik commented Aug 14, 2019

True, I need to do that kind of refactoring here.

@tsubik tsubik requested a review from kowal August 22, 2019 18:08
@tsubik tsubik marked this pull request as ready for review August 22, 2019 18:08
@tsubik
Copy link
Contributor Author

tsubik commented Aug 22, 2019

Any refactoring to the data upload could be done in a separate PR I guess. This is the first version.

@kowal kowal merged commit 2e057b9 into develop Aug 23, 2019
@kowal kowal deleted the feature/data-uploads branch August 23, 2019 08:27
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.

3 participants