search

janko / paperclip-dropbox

[OBSOLETE] Extends Paperclip with Dropbox storage.
MIT License
148 stars 36 forks source link

Add "private" and "public" visibility modes #31

Closed dougbradbury closed 10 years ago

dougbradbury commented 10 years ago

One may want to use dropbox in full dropbox mode, but not store one's files in the Public directory, but instead choose any dropbox path for the files to live in. Then generate single-use urls to access the file.

I've implemented this by adding a 'dropbox_visibility' parameter that take either "public" or "private"

janko commented 10 years ago

Wow, this looks awesome! I will review this properly tomorrow and merge it. Could you in the meanwhile squash these commits into single one? Better for the history, and easier for me to look at the diff :)

dougbradbury commented 10 years ago

there you go!

dougbradbury commented 10 years ago

I found a bug as I was integrating this new private mode. Should I squash this as well?

janko commented 10 years ago

Yes. I'm sorry I hadn't had time to review this yet, I'm busy with something of mine. I will get back to you tomorrow :)

janko commented 10 years ago

I'm curious, why did you extract the private/public url generation into 2 additional classes?

Your pull request really made me think. Why am I enabling "app folder" access type in this gem? The permissions of "app folder" is just a subset of "full dropbox". If a user wants a separate directory, he can just create one, and still use the "full dropbox" access type. I was thinking of soon releasing the major version where I remove some garbage (like the path generation with a proc), and require a "full dropbox" access type always, so people can choose whether they want to put files in "Public" folder or somewhere else. It would be a much simpler concept. What do you think?

dougbradbury commented 10 years ago

I did that as a "replace conditional with polymorphism" refactoring. I thought I was going to end up with a third one that handled the full_dropbox / private visibility case, but it turned out to be the same as the private, app folder case. I still think it is a good refactoring because if got rid of a couple of duplicated if statements and replaced them with a polymorphic dispatch.

I still that that the app_folder use case is a good one. It's easier to get your dropbox app approved if it asks for fewer permissions. Giving an app full r/w access to your entire dropbox is a bit scary for many use cases.

The "Public" path is still a good use case as well. If you want to use this integration for public assets (like product images in a store or something) You don't want to be making a call to generate a new url every time you render a page with that image. It would end up slowing down your web app way too much. So I think the full_dropbox / public visibility is still a valid use case as well.

My use case that I added is by far the weakest one. I had a bunch of files already in dropbox that I wanted to be able to generate limited use links to. That's where full_dropbox / private visibility came into play.

The path generation is pretty crazy in there. I steered clear of it except for the bit that adds "Public" to the path. As you can see I moved that out to the storage element because I was frightened to get too deep into the path generator.

janko commented 10 years ago

Aha, I see now. I agree, "replace conditional with polymorphism" is a good thing. And it makes sense, because they generate URLs differently (public one instantly, private one with an HTTP request).

I still that that the app_folder use case is a good one. It's easier to get your dropbox app approved if it asks for fewer permissions. Giving an app full r/w access to your entire dropbox is a bit scary for many use cases.

But the only case when a developer is using paperclip-dropbox is to store things on his Dropbox, which he already authenticated before for himself. The only thing he needs to trust is my gem.

I wasn't talking about the removal of "Public" folder use case. That's the most useful one :). I was talking about removal of "app folder" access type, because you can get the identical behaviour with private "full dropbox" style.

Lol, sorry about the path generation. That's what I wanted to clean up for the major release, because in the past I enabled path generation through a proc (god knows why), and later the regular Paperclip's :path option, so that code is unnecessarily complicated now, because the proc version shouldn't have existed in the first place.