This looks good. There's a little bit more I'd like to see before this goes in.

I'd like to see an UPLOADS_URL as well so that if people have some custom path for all uploaded files, they can say where it is. This will be important down the road if we support, say, Amazon S3 as a destination for uploads.

Also not sure if I'd prefer UPLOAD_* or UPLOADS_*. I almost like UPLOAD_* better but I'm okay with either.

1. Chris Lamb 3 months ago (August 20th, 2008, 8:52 p.m.)

   I agree that would be a useful feature. However, looking at the code it seems that all users of djblets would need to depend on this additional setting - is that okay?
   
   (Looking at the UPLOADS_* diff vs UPLOAD_*, it looks much better so will change that next round)

2. Christian Hammond 3 months ago (August 22nd, 2008, 12:40 a.m.)

   I may be confused. Why would UPLOAD_URL impact Djblets?

3. Chris Lamb 3 months ago (August 22nd, 2008, 1:55 a.m.)

   I believe the `thumbnail` method in util/templatetags/djblets_images.py will need to be modified such that the UPLOAD_URL does not have to be under MEDIA_URL?

4. Christian Hammond 3 months ago (August 22nd, 2008, 2:14 a.m.)

   Ah, I see. Yes, and crop_image. Both of these have problems with MEDIA_ROOT vs UPLOAD_ROOT anyway, though. They assume these images exist in MEDIA_ROOT and that may no longer be the case.
   
   Now for the thing that will really throw all this off: Django validates that uploaded files exist in MEDIA_ROOT, so UPLOAD_ROOT will never be able to be outside of MEDIA_ROOT. Which almost makes it pointless to have a custom variable. Maybe we should just document how to alias the path in the web server configuration...
   
   Thoughts?

5. Chris Lamb 3 months ago (August 22nd, 2008, 3:13 a.m.)

   Urrgh, yes, I ACK that security feature.
   
   I'll have a think about this today and get back to you here. I'm very interested in the instancibility of Review Board, meaning that as little as possible needs to be copied or duplicated for each instance - I think the remarks in your comment imply that MEDIA_ROOT must be instance-specific, requiring symbolic links back inside that to the "system-wide" subdirectories of media. (Moving the current contents of media/ to a single subdirectory would be help a lot if this was the solution.)
   
   Any thoughts? I appreciate that this aspect may not be of any real use to you.

6. Christian Hammond 2 months, 4 weeks ago (August 22nd, 2008, 3:20 p.m.)

   I actually really want this too. I started work on a similar patch just before I saw that you posted this up. I didn't take into consideration that Django was doing this validation.
   
   I think if we can't figure out something clever here, we can always set up an Alias in the apache config for setting /media/uploaded to point to somewhere else.
   
   We're going to have this issue for more than just /media/uploaded. When extensions land post-1.0, we'll have another directory (/media/ext/) that the web server must be able to write to.
   
   Another option is to have individual installs set up their own htdocs, creating symlinks to the rb/yui/yui-ext/djblets directories, and providing their own uploaded and ext directories.
   
   I'm working on a script for making initial installation really easy. It could handle all this. Of course, on Windows we'd have to copy the files. Or maybe we could just provide a .htaccess that aliases the directories we care about to the system installs.

7. Christian Hammond 1 week ago (November 13th, 2008, 2:02 a.m.)

   So what I've ended up doing was implementing a script to maintain Review Board site trees. This handles installation and upgrades, and basically maintains the htdocs/media/ directory structure (symlinking when possible, erasing/copying the directories on Windows).
   
   What this boils down to is that uploaded/ is now its own directory inside the site's htdocs/media/ directory, and everything else is a symlink to a system-installed directory. So this change and the problems we discovered shouldn't really be relevant anymore. As such, I'm discarding this, and hopefully the new script will just work well enough for people that they don't run into any upload issues anymore.

/trunk/reviewboard/templates/admin/manual-updates/media-upload-dir.html (Diff revision 2)
6	Your Review Board installation does not have a {{MEDIA_ROOT}}/uploaded/images directory.	6	Your Review Board installation does not have a {{UPLOAD_ROOT}}/images directory.
7	{% endblocktrans %}	7	{% endblocktrans %}
8	</p>	8	</p>
9	<h3>{% trans "If this is a new installation..." %}</h3>	9	<h3>{% trans "If this is a new installation..." %}</h3>
10	<p>	10	<p>
11	{% blocktrans %}	11	{% blocktrans %}
12	Create both <tt>{{MEDIA_ROOT}}/uploaded</tt> and	12	Create both <tt>{{UPLOAD_ROOT}}</tt> and
13	<tt>{{MEDIA_ROOT}}/uploaded/images</tt> on the server and make both directories writable by the web	13	<tt>{{UPLOAD_ROOT}}/images</tt> on the server and make both directories writable by the web
14	server. You can do so with the following commands:	14	server. You can do so with the following commands:
15	{% endblocktrans %}	15	{% endblocktrans %}
16	</p>	16	</p>
17	<pre>	17	<pre>
18	$ cd {{MEDIA_ROOT}}	18	$ mkdir -p {{UPLOAD_ROOT}}/images
19	$ mkdir -p uploaded/images	19	$ sudo chown -R www-data:www-data {{UPLOAD_ROOT}}

Should be UPLOADS_ROOT.