search

frictionlessdata / ckanext-datapackager

CKAN extension for importing/exporting Data Packages.
36 stars 14 forks source link

Fix #84: Get uploaded datapackage file from request #85

Closed aivuk closed 1 year ago

aivuk commented 2 years ago

Fix #84, getting the uploaded file data from toolkit.request.files

amercader commented 2 years ago

@aivuk if this was working on previous versions presumably we need to support both approaches, using version checks or checking for the necessary attributes.

And of course it would be great to have a test that tested uploading zipped data packages

aivuk commented 2 years ago

@aivuk if this was working on previous versions presumably we need to support both approaches, using version checks or checking for the necessary attributes.

And of course it would be great to have a test that tested uploading zipped data packages

yes, I agree and I'm already checking both for versions lower than 2.9 but I didn't test locally in version lower than 2.9. I'm looking at the tests with datapackages compressed as zip files.

aivuk commented 2 years ago

@amercader could you already look at this PR? Without it the upload of datapackages is not working

amercader commented 2 years ago

Sorry I wasn't clear, what I meant in my comment was that these changes might work on 2.9, but things like this will break 2.8 (where the uploads are handled differently). And as we don't have a proper upload test we are missing these failures, what doing a local test on 2.8 confirm that the uploads are not working. While we don't sort this properly just add version checks on all changes you made on this PR and we'll fix it more elegantly in the future

aivuk commented 2 years ago

Sorry I wasn't clear, what I meant in my comment was that these changes might work on 2.9, but things like this will break 2.8 (where the uploads are handled differently). And as we don't have a proper upload test we are missing these failures, what doing a local test on 2.8 confirm that the uploads are not working. While we don't sort this properly just add version checks on all changes you made on this PR and we'll fix it more elegantly in the future

Ok, I got it know. Looks like there is not much work to make it work on 2.8, just use the old code. I'm working on it and I'll test it locally in a CKAN 2.8.

aivuk commented 1 year ago

@amercader I finally fixed on CKAN 2.8, dealing with the uploaded files as we did in the past and I think the PR is good to go.