Add FileBrowseUploadField

[sehmaschine]

though experimental, since we've used it for a while without any bigger issues we should add this field to the filebrowser.

[ad-m]

Is it possible to get code of FileBrowseUploadField? Is it published anywhere?

[shayneoneill]

I can't find this in any of the branches. Is this available?

[sehmaschine]

the code is currently not available. we've been using this field for a while and we'll probably add it with the next release. the ticket is mainly a reminder for myself.

[andybak]

I was about to add a feature request ticket based on user feedback - "Lots of clicks to upload then select an image for FileBrowseFields'

I was going to suggest adding a second js button to FileBrowseField that:

1. Opened the upload page
2. Immediately after upload, closed the window and selected the uploaded file

I suspect this might be related?

[sehmaschine]

yes, it's related ... although I'd try to avoid the upload-page.

[andybak]

I just tried it and it works great. However - it's rather thrown the baby out with the bath-water.

The great thing about FileBrowseField is being able to use existing uploads. The problem with it is that it's laborious for the equally common case of a new upload.

FileBrowseUploadField solves the latter but leaves you not much better off than if you were using a normal FileField.

I am hoping to have the best of both - i.e. a field with two buttons - browse and upload.

[andybak]

I did a really dumb and naive implementation - I basically diffed the two fields and combined them into one: https://gist.github.com/andybak/a5d9bcffd06abdeae2e9

Amazingly it seems to work although it's only had cursory testing so far. If you're interested I can put together a proper pull request.

[sehmaschine]

@andybak you are right – it should contain 2 buttons (upload, browse). a PR is very much appreciated.

[andybak]

@sehmaschine - slightly tricky as I'm using the no-grappelli fork. There's some slight differences in the template and fields.py that I don't fully understand the reasons for. It also means I can't test against your version very easily..

How about I create a separate repo with standalone replacement field in it. It should be fairly trivial for you to incorporate from that.

[andybak]

OK: https://github.com/DjangoAdminHackers/django-filebrowser-browse-upload-field

I'm slightly hampered by the lack of docs for the new field. My expectation of the correct behaviour might be incorrect.

What's 'temp' about the temp_upload_dir and how does it interact with upload_to and directory?

If I have the following:

photo = FileBrowseUploadField(max_length=512, null=True, blank=True, format="image", directory='events', upload_to='events', temp_upload_dir='_temp')

I was expecting the file to end up in 'events' - maybe after form.save()

If we are combining browse and upload then I would imagine we need something like this. Allowing browsing of the temp_upload_dir doesn't seem like the correct behaviour.

[andybak]

Alternatively - maybe temp_upload_dir isn't intended to be 'temp' at all - in which case why not just use upload_to?

Speaking of which - unless I've missed something 'upload_to' doesn't seem to be used for anything.

And shouldn't https://github.com/sehmaschine/django-filebrowser/blob/master/filebrowser/fields.py#L162 be something like:

final_attrs['temp_upload_dir'] = self.temp_upload_dir or UPLOAD_TEMPDIR

[andybak]

I've updated https://github.com/DjangoAdminHackers/django-filebrowser-browse-upload-field

I went with a simpler solution of not using temp uploads at all. There are drawbacks to this (it will clutter up the upload_to dir with images that are never saved) but it was a heck of a lot simpler.

I've also included a FilebrowserSite subclass that makes allows us to update the thumbnail after upload. It was a single line change - I added the following to ret_json:

'admin_thumbnail_url': f.version_generate(ADMIN_THUMBNAIL).url

but I couldn't do this without copy+pasting the entire method.

(Actually - I could have parsed the json and reconstructed the response but that was yukky)

I figure that you'll want to modify the method directly if you incorporate this in core. If not then I'll rethink that method as it will probably break if you change the original.

[sehmaschine]

@andybak one note (without checking your code): the temp upload dir is there, because upload_to doesn't need to exist when the file is being uploaded (e.g. if upload_to contains the ID of an object which is being added – that object is saved AFTER the file is uploaded).

[sehmaschine]

besides, it's much easier to clean up the temp dir. and since we need a versions-dir anyway, why not add a temp dir as well.

[andybak]

the temp upload dir is there, because upload_to doesn't need to exist when the file is being uploaded

OK. My simpler solution works for my use case (I never have a scenario where upload_to doesn't exist)

But using your original upload field - the file never got moved from the temp to the real directory. What was I doing wrong? Where's the code that does this or does it not actually get moved?

[sehmaschine]

you are right ... we used a signal for moving the file. I think that a scenario where upload_to doesn't exist is pretty common (maybe it's just me, but we have that all the time when using the object ID as the document folder).

[andybak]

we used a signal for moving the file.

I spent a while trying to decide where I would put this functionality. I think I settled on the pre_save method of the model field before I chose to go with the simpler 'non-temp-dir' approach.

I'm generally not a fan of signals except as a way for one app to respond to something in a different app. Using signals inside an app seems worse than taking action in a method. Do you agree and if so - do you have any opinion on which method should be responsible for this?

Also - can you post your signal code so I can take a look?

I think that a scenario where upload_to doesn't exist is pretty common

Yeah. I agree. I just don't need myself immediately.

[sehmaschine]

I can't think of another solution besides using signals. But I'm open for any suggestions.

Here's what we currently use with our projects ...
https://gist.github.com/sehmaschine/8ec052d7423dbd307fce

Please note that this is a custom solution (e.g., we check for a field name "image" with signals, because we know that this field exists).

[andybak]

I'm trying to think of a generic solution. Aside from my other doubts about signals - this means that any model that uses FileBrowseUploadField has to write - or at least wire up - a signal handler.

I wonder if some 'contribute_to_class' magic could be used?: https://github.com/carljm/django-model-utils/blob/master/model_utils/fields.py#L104

[andybak]

contribute_to_class is looking pretty plausible. I should have something working tomorrow.

Related question - should upload_to etc accept callables the same as FileField?

[sehmaschine]

upload_to should definitely accept callables.

[sehmaschine]

agreed, contribute_to_class is probably better than using signals.

[andybak]

I've actually ended up using contribute_to_class to attach the signals automatically. There wasn't another way I could find to trigger this action after the model was saved (ModelFields only have a pre_save method hook and I had trouble getting access to the instance in other methods)

The latest commit seems to work ok: https://github.com/DjangoAdminHackers/django-filebrowser-browse-upload-field

I'm going to tackle passing callables to upload_to next but I'm hoping that's fairly simple now we have a model field method than runs post-save.

[andybak]

Callables work. What doesn't work is the upload when you use inlines and the dynamic 'add another'. To fix this I'll need to move the javascript out of the field template and into form.media as a normal static file - then rewrite it work dynamically and not have Django stuff hardcoded into it.