Multifile upload support #20
Conversation
|
This is exciting but I'm too tired to review it tonight, tomorrow we'll dig in more and exercise it |
|
|
||
| if self.json: | ||
| return self.render_to_json_response({'state': upload.state, 'id': upload.id}) | ||
| return self.render_to_json_response({'state': upload.state, 'id': upload.id, 'count': UploadFile.objects.filter(upload=upload.id).count()}) |
There was a problem hiding this comment.
This is purely my ignorance, but I want to understand why 'count' is being added to this response. If it is not being specifically needed by any frontends, I'd rather not include it because making it public API means we can't really remove it, and I'm nervous about what happens if the table is large (table scan hanging the view?)
| return sizeof_fmt(self.size) | ||
|
|
||
| def file_url(self): | ||
| """ | ||
| Exposes the file url. | ||
| """ | ||
| return self.uploadfile_set.first().file.url | ||
| return '' # self.uploadfile_set.first().file.url |
There was a problem hiding this comment.
Where is this used? I'd personally rather remove the method and in that way cleanly break existing API than start returning something which may cause silent or confusing failures.
There was a problem hiding this comment.
Looks like this may be used in osgeo_importer/static/osgeo_importer/partials/upload.html around line 108. Can we check whether this breaks the Download link in the UI? If so, maybe we need to just delete it, or is there some other way we should support it (e.g. multiple download links)?
| @@ -48,9 +48,6 @@ <h2 class="page-title">{% trans "Add data" %}</h2> | |||
| <i class="fa fa-warning"> {{ error|escape }}</i> | |||
| {% endfor %} | |||
| </div> | |||
| <div class="col-md-6" style="padding-top: 5px; padding-left: 2px;"> | |||
There was a problem hiding this comment.
Is there not need for this information? We could generate it using values defined in python if we wanted.
There was a problem hiding this comment.
See my comment about this in the PR description -- essentially 1) removing it puts us in the same state as current (non-osgeo) importer, so it's not truly a UX regression; 2) yep we should be generating it dynamically based on values defined elsewhere to avoid stale markup (ref: #19)
see below comment
There was a problem hiding this comment.
Disregard my previous comment; forgot https://github.com/sarasafavi/django-osgeo-importer/blob/749c9561168df36cc2ca363bd26911b920909c4d/osgeo_importer_prj/settings.py#L83 was in here now, so I'll update shortly with a fix incorporating that settings var
There was a problem hiding this comment.
@harts-boundless this is resolved at 39522d2 but I don't know how to kill this comment thread because new-GitHub is silly... so, FYI
|
My feedback on this is pretty trivial, I think it looks good. I think we should improve test coverage beyond this, but I don't think that should be a barrier to merge |
sarasafavi
force-pushed
the
multiform
branch
2 times, most recently
from
69b78f5
to
ac5fe5f
Compare
variable name: b -> bundle and other variable names: uploadid -> upload_id, uploadobj -> upload_obj, cnt -> count uf -> uploadfile make import_layer explicitly take pk kwarg: since this method is only using kwargs to get pk, we might as well extract pk early and default it to None instead of using kwargs.get. I actually would prefer pk to be a required arg and to dispense with **kwargs, so we have a real signature that can check us when we make mistakes; but that will have to depend on an analysis of where these methods are used pass request as Bundle arg instead of setting attr after
This can help a little to make uploads more distinguishable when they don't have different names.
also permit null file_size for UploadedData. Sometimes an upload doesn't have a size (maybe it didn't finish uploading). In that case, it is not really sensible to say it is 0 bytes. It has no size.
sarasafavi
force-pushed
the
multiform
branch
from
e9e36a3
to
749c956
Compare
|
|
||
| def test_single_too_long(self): | ||
| max_length = models.UploadedData._meta.get_field('name').max_length | ||
| too_long = "ObviouslyWayWayWayWayWayWayWayWayWayWayWayWayWayWayTooLong" |
sarasafavi
force-pushed
the
multiform
branch
from
749c956
to
9fed0b7
Compare
| <div class="layer-upload-name"> {{layer.name}} | ||
| <div class="layer-upload-name"> | ||
| <span>{{layer.name || layer.file_name}}</span> | ||
| <span ng-show="layer.name && layer.name != layer.file_name"><br>(in {{layer.file_name}})</span> |
Since some of the multifile and geopackage changes, it is no longer obvious what should be the name of an upload. If it just contains one file, we can do as before, but otherwise things can become more difficult. The heuristics for this are a little complex, so they've been split off into their own method and tests created just for this method.
No longer possible just to assume that there is one upload file type and it is the same file type as a layer, so we need to detect the actual file type, store it in the database and present it to users. - add to api - add to UploadLayer as property - add to UploadFile as field - add the migration to make the column - set it from the form processing
to api, UploadLayer, set in form processing, migration to add column. layer.name keeps filename info that is used to import, so we can't stuff actual layer names (e.g. from inside gpkg) there without breaking import. If we change both, any existing databases will be storing a different kind of information in that field, which may be confusing. We can deprecate layer.name if we need to.
Whether extensions are explicitly specified in settings, or else just the default list of extensions is used, we should consider importers.VALID_EXTENSIONS as canon and use that in user-facing templates
sarasafavi
force-pushed
the
multiform
branch
from
07277e0
to
39522d2
Compare
ghost
mentioned this pull request
|
@harts-boundless reviewed your latest, looks good! I'm fine with merging if you are. |
|
This runs fine for me manually and all tests are passing. |
ghost
merged commit 
Reference @bitner's original PR at GeoNode#39
Adds "multifile" support to the upload process: now, multiple files can be selected after clicking "choose file(s)" on the "Add data" page. This allows upload of multiple layers at once, as well as uploading of unzipped shapefiles. A single upload can include any number of files in any mix of (supported) formats.
I've also removed the outdated list of "Valid file types" from the "Add data" page's template (ref: #19). This is equivalent to Exchange's current default importer (that is, no explicit list of supported file types is shown to the user at this point in the upload process), which seems acceptable at this point in development.