Version 2.0 #60

codingjoe · 2017-08-30T16:28:04Z

Remove signing view
Widgets are now signed when rendered. This behavior saves one
call to the application server.
Add support for accept attribute
Limit upload policy MIME-Type in accept attribute.
Add support for multiple file uploads
Remove django-appconf dependency
Remove progress bar
Add middleware to allow seamless Django integration
Add automatical FileField.widget overwrite
Add tests
Reduce file size
Reduce asset size

codingjoe · 2017-08-30T22:19:24Z

@amureki would you care to review this? I completely rewrote this library.

* Remove signing view Widgets are now signed when rendered. This behavior saves one call to the application server. * Add support for `accept` attribute Limit upload policy MIME-Type in `accept` attribute. * Add support for `multiple` file uploads * Remove `django-appconf` dependency * Remove progress bar * Add middleware to allow seamless Django integration * Add automatical `FileField.widget` overwrite * Add tests * Reduce file size * Reduce asset size

grunskis

I managed to get this working in a new test app, but an example app in the repo would be of a great help :)

What I got working is file upload to tmp/ but who's supposed to move the file over to the destination specified in upload_to?

Some other general comments:

there's .gitmodule file with references to some jquery plugins
would be nice to have a changelog available to see what needs to be upgraded from 1.2.1

grunskis · 2017-09-07T13:45:52Z

README.md

+*   no JavaScript or Python dependencies (no jQuery)
+*   Python 3 and 2 support
+*   Auto swapping based on your environment
+*   works just like the buildin


Typo, s/buildin/built-in/

Also it's not entirely clear what's meant by built-in...

grunskis · 2017-09-07T13:46:23Z

README.md

+*   lightweight: less 200 lines
+*   no JavaScript or Python dependencies (no jQuery)
+*   Python 3 and 2 support
+*   Auto swapping based on your environment


It's not clear to me what you mean here

grunskis · 2017-09-07T13:56:14Z

README.md

-### Advanced usage examples
+Django does have limited [support to uploaded multiple files][uploading-multiple-files].
+S3File fully supports this feature. The custom middleware makes ensure that files
+are accessible via `request.FILES`, even tho they have been uploaded to AWS S3 directly


s/tho/though/

grunskis · 2017-09-07T13:56:28Z

README.md

+Django does have limited [support to uploaded multiple files][uploading-multiple-files].
+S3File fully supports this feature. The custom middleware makes ensure that files
+are accessible via `request.FILES`, even tho they have been uploaded to AWS S3 directly
+and not to you Django application server.


s/you/your/

grunskis · 2017-09-07T13:57:11Z

README.md

-        MyS3FileView.as_view(), name='s3file-sign'),
-)
-```
+S3File uses a strict policy and signature to grand clients permission to upload


s/grand/grant/

grunskis · 2017-09-07T14:53:02Z

tests/testapp/forms.py

    class Meta:
        model = FileModel
        fields = ('file',)
-        widgets = {'file': AutoFileInput}


 class ClearableUploadForm(forms.ModelForm):


This form doesn't seem to be used anywhere and can be removed

tests/test_forms.py:85 ;)

Right, somehow missed that. But actually do you really need these 2 identical forms here?

grunskis · 2017-09-07T15:00:36Z

s3file/static/s3file/js/s3file.js

-    return unescape(tag.childNodes[0].nodeValue)
+(() => {
+  function parseURL (text) {
+    console.log(text)


Do you want to keep this in?

grunskis · 2017-09-07T15:26:52Z

README.md

-urlpatterns = patterns(
-    ...
-    url(r'^s3file/', include('s3file.urls')),
+MIDDLEWARE_CLASSES = (


When creating a new Django app the setting is now called MIDDLEWARE and middleware are processed a bit differently. Docs should probably reflect that. In case you want to support both middleware versions (as IMO this library should) then there's https://docs.djangoproject.com/en/1.11/topics/http/middleware/#upgrading-pre-django-1-10-style-middleware

grunskis · 2017-09-07T16:36:49Z

s3file/forms.py

+        self.access_key = settings.AWS_ACCESS_KEY_ID
+        self.secret_access_key = settings.AWS_SECRET_ACCESS_KEY
+        self.bucket_name = settings.AWS_STORAGE_BUCKET_NAME
+        self.upload_path = getattr(settings, 'S3FILE_UPLOAD_PATH', os.path.join('tmp', 's3fine'))


Probably you meant tmp/s3file not tmp/s3fine as the default

grunskis · 2017-09-07T16:38:55Z

s3file/forms.py

+            'data-policy': force_text(self.get_policy()),
+            'data-signature': self.get_signature(),
+            'data-key': self.upload_folder,
+            'data-s3-url': 'https://s3.amazonaws.com/%s' % self.bucket_name,


https://s3.amazonaws.com/ works for us-east-1 and maybe some other regions but didn't work for me for eu-west-1

dang. can you show me tomorrow?

Here's the list of regions and s3 endpoints: http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region

I think you would need an extra setting where to specify S3 region for the bucket if you want to support regions other than us-east-1

codingjoe · 2017-09-08T14:07:50Z

@grunskis I think that should resolve most issues.
I decided to use boto3's buildin method generate_presigned_post. This makes it a lot easier. I don't have to deal with sining and different bucket setups. As long as the storage works, so does the upload.

grunskis · 2017-09-08T14:09:05Z

Ok, will take a look again.

grunskis

Cool stuff! Now works much better :)

grunskis · 2017-09-08T14:52:54Z

setup.py

@@ -24,6 +24,6 @@
    packages=['s3file'],
    include_package_data=True,
    install_requires=[
-        'django-appconf>=0.6',
+        'django-storages',
    ],


You should also include boto or boto3 here or document that one of these is required.

Well none of this works, if the BotoStorage isn't setup and configured. This usually is the first step. But I will make sure to mention this, in the setup guide. Thanks! having documentation reviewed is always really helpful.

codingjoe force-pushed the version2 branch 6 times, most recently from 2e85d5f to f2c9d9c Compare August 30, 2017 18:15

codingjoe changed the title ~~Version 2~~ version 2 Aug 30, 2017

codingjoe changed the title ~~version 2~~ Version 2.0 Aug 30, 2017

codingjoe force-pushed the version2 branch 6 times, most recently from eabe36c to 9147424 Compare August 30, 2017 22:18

codingjoe force-pushed the version2 branch from 9147424 to 86b0390 Compare August 31, 2017 00:32

grunskis suggested changes Sep 7, 2017

View reviewed changes

codingjoe added 3 commits September 7, 2017 20:45

Address review comments by @grunskis, thx <3

766c9a7

Fix more review comments

53174a7

Switch to boto3's generate_presigned_post method

d37ee31

fix isort

2f1a65c

grunskis approved these changes Sep 8, 2017

View reviewed changes

codingjoe added 2 commits September 8, 2017 19:57

Add tests for automatic widget swap

e71d51d

Extend documentation

c0cb969

codingjoe merged commit 02ccea9 into master Sep 8, 2017

codingjoe deleted the version2 branch September 8, 2017 18:39

schwartzmx mentioned this pull request May 21, 2019

Upload Progress Output #95

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version 2.0 #60

Version 2.0 #60

codingjoe commented Aug 30, 2017

codingjoe commented Aug 30, 2017

grunskis left a comment

grunskis Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

codingjoe Sep 7, 2017

grunskis Sep 8, 2017

grunskis Sep 7, 2017

codingjoe Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

grunskis Sep 7, 2017

codingjoe Sep 7, 2017

grunskis Sep 8, 2017

codingjoe commented Sep 8, 2017

grunskis commented Sep 8, 2017

grunskis left a comment

grunskis Sep 8, 2017

codingjoe Sep 8, 2017

Version 2.0 #60

Version 2.0 #60

Conversation

codingjoe commented Aug 30, 2017

codingjoe commented Aug 30, 2017

grunskis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingjoe commented Sep 8, 2017

grunskis commented Sep 8, 2017

grunskis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment