Allow to download public bundle as mbox

Submitted by Simo Sorce on Nov. 6, 2012, 1:15 a.m.

Details

Message ID 1352164520-14744-1-git-send-email-idra@samba.org
State Superseded
Headers show

Commit Message

Simo Sorce Nov. 6, 2012, 1:15 a.m.
Downloading single patches anonimously is allowed, we may as well allow
downloading public bundles as mboxes.

Signed-off-by: Simo Sorce <idra@samba.org>
---
 apps/patchwork/urls.py                 |  2 ++
 apps/patchwork/views/bundle.py         | 18 +++++++++++++-----
 templates/patchwork/bundle-public.html |  2 ++
 3 files changed, 17 insertions(+), 5 deletions(-)

Comments

Simo Sorce Nov. 6, 2012, 1:19 a.m.
On Mon, 2012-11-05 at 20:15 -0500, Simo Sorce wrote:
> Downloading single patches anonimously is allowed, we may as well
> allow
> downloading public bundles as mboxes.

Jeremy,
the patch now should have all the changes you asked for.

This time the urls.py change worked fine, I guess I wasn't as aggressive
as I should have been with httpd restarts the first time around.

Helper function to return bundle mboxes has been added too.

Simo.
Jeremy Kerr Nov. 6, 2012, 8:45 a.m.
Hi Simo,

> Downloading single patches anonimously is allowed, we may as well allow
> downloading public bundles as mboxes.

Looks good. This does raise one question for everyone though:

>
> +def mbox_response(bundle, name):
>       response = HttpResponse(mimetype='text/plain')
> -    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
> -        bundle.id
> +    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
> +        name
>       response.write(bundle.mbox())
>       return response

Does anyone mind if we change the Content-Disposition HTTP header for 
downloading private bundles from the form:

    bundle-<ID>.mbox

to:

    bundle-<NAME>.mbox

?

In other words, does anyone rely on the bundle name to contain the ID?

Cheers,


Jeremy
Benjamin Herrenschmidt Nov. 6, 2012, 2:05 p.m.
On Tue, 2012-11-06 at 16:45 +0800, Jeremy Kerr wrote:

>     bundle-<ID>.mbox
> 
> to:
> 
>     bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

Not me. I always rename them to something like
bundle-<tree_to_apply>-date-index.mbox :-)

Cheers,
Ben.
Simo Sorce Nov. 6, 2012, 3:17 p.m.
On Tue, 2012-11-06 at 16:45 +0800, Jeremy Kerr wrote:
> Hi Simo,
> 
> > Downloading single patches anonimously is allowed, we may as well allow
> > downloading public bundles as mboxes.
> 
> Looks good. This does raise one question for everyone though:
> 
> >
> > +def mbox_response(bundle, name):
> >       response = HttpResponse(mimetype='text/plain')
> > -    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
> > -        bundle.id
> > +    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
> > +        name
> >       response.write(bundle.mbox())
> >       return response
> 
> Does anyone mind if we change the Content-Disposition HTTP header for 
> downloading private bundles from the form:
> 
>     bundle-<ID>.mbox
> 
> to:
> 
>     bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

Note that I didn't really change it, for normal mboxes the name is
str(bundle.id) :-)

Simo.
Jeremy Kerr Nov. 6, 2012, 3:19 p.m.
Hi Simo,

>> In other words, does anyone rely on the bundle name to contain the ID?
>
> Note that I didn't really change it, for normal mboxes the name is
> str(bundle.id) :-)

Yep, but if we unify them, it'll be more consistent. Using the name 
seems to be the way to go.

(however, we should also clean the filename, so that we don't get 
user-provided data ending up in a HTTP header field)

Cheers,


Jeremy
Simo Sorce Nov. 6, 2012, 3:34 p.m.
On Tue, 2012-11-06 at 23:19 +0800, Jeremy Kerr wrote:
> Hi Simo,
> 
> >> In other words, does anyone rely on the bundle name to contain the ID?
> >
> > Note that I didn't really change it, for normal mboxes the name is
> > str(bundle.id) :-)
> 
> Yep, but if we unify them, it'll be more consistent. Using the name 
> seems to be the way to go.
> 
> (however, we should also clean the filename, so that we don't get 
> user-provided data ending up in a HTTP header field)

I checked earlier that the bundle name is properly sanitized when input.
If we were to use the patch name coming from the email parser, then yeah
we need to be more careful.

Simo.
David Miller Nov. 6, 2012, 5:46 p.m.
From: Jeremy Kerr <jk@ozlabs.org>
Date: Tue, 06 Nov 2012 16:45:07 +0800

> Hi Simo,
> 
>> Downloading single patches anonimously is allowed, we may as well
>> allow
>> downloading public bundles as mboxes.
> 
> Looks good. This does raise one question for everyone though:
> 
>>
>> +def mbox_response(bundle, name):
>>       response = HttpResponse(mimetype='text/plain')
>> -    response['Content-Disposition'] = 'attachment;
>> -    filename=bundle-%d.mbox' % \
>> -        bundle.id
>> + response['Content-Disposition'] = 'attachment;
>> filename=bundle-%s.mbox' % \
>> +        name
>>       response.write(bundle.mbox())
>>       return response
> 
> Does anyone mind if we change the Content-Disposition HTTP header for
> downloading private bundles from the form:
> 
>    bundle-<ID>.mbox
> 
> to:
> 
>    bundle-<NAME>.mbox
> 
> ?
> 
> In other words, does anyone rely on the bundle name to contain the ID?

I use throwaway bundles having the same name over and over again (like
"net" and "net-next") and I sometimes leave old bundle files around by
accident, so using the name could definitely cause problems for me as
the new ID generated each time lets me know that it's a new bundle
rather than a dup.
Jeremy Kerr Nov. 7, 2012, 2:01 a.m.
Hi Simo,

>> In other words, does anyone rely on the bundle name to contain the ID?
>
> I use throwaway bundles having the same name over and over again (like
> "net" and "net-next") and I sometimes leave old bundle files around by
> accident, so using the name could definitely cause problems for me as
> the new ID generated each time lets me know that it's a new bundle
> rather than a dup.

OK, let's go with bundle-<ID>-<name>.mbox then? This will be unique, and 
a little more descriptive than just the ID.

Cheers,


Jeremy
David Miller Nov. 7, 2012, 5:39 a.m.
From: Jeremy Kerr <jk@ozlabs.org>
Date: Wed, 07 Nov 2012 10:01:00 +0800

> Hi Simo,

My name is not Simo.

>>> In other words, does anyone rely on the bundle name to contain the ID?
>>
>> I use throwaway bundles having the same name over and over again (like
>> "net" and "net-next") and I sometimes leave old bundle files around by
>> accident, so using the name could definitely cause problems for me as
>> the new ID generated each time lets me know that it's a new bundle
>> rather than a dup.
> 
> OK, let's go with bundle-<ID>-<name>.mbox then? This will be unique,
> and a little more descriptive than just the ID.

Works for me.
Jeremy Kerr Nov. 7, 2012, 7:05 a.m.
Hi David,

>> Hi Simo,
>
> My name is not Simo.

Didn't you get the memo? We had it changed. :D

Sorry about the To: vs. CC: confusion - Simo is proposing the changes to 
implement public mbox downloads, we're just working out the filename 
details here.

Cheers,


Jeremy

Patch hide | download patch | download mbox

diff --git a/apps/patchwork/urls.py b/apps/patchwork/urls.py
index 10fc3b9..3209830 100644
--- a/apps/patchwork/urls.py
+++ b/apps/patchwork/urls.py
@@ -67,6 +67,8 @@  urlpatterns = patterns('',
     # public view for bundles
     (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/$',
                                 'patchwork.views.bundle.public'),
+    (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/mbox/$',
+                                'patchwork.views.bundle.public_mbox'),
 
     (r'^confirm/(?P<key>[0-9a-f]+)/$', 'patchwork.views.confirm'),
 
diff --git a/apps/patchwork/views/bundle.py b/apps/patchwork/views/bundle.py
index e418b3a..e592bca 100644
--- a/apps/patchwork/views/bundle.py
+++ b/apps/patchwork/views/bundle.py
@@ -168,15 +168,18 @@  def bundle(request, bundle_id):
 
     return render_to_response('patchwork/bundle.html', context)
 
-@login_required
-def mbox(request, bundle_id):
-    bundle = get_object_or_404(Bundle, id = bundle_id)
+def mbox_response(bundle, name):
     response = HttpResponse(mimetype='text/plain')
-    response['Content-Disposition'] = 'attachment; filename=bundle-%d.mbox' % \
-        bundle.id
+    response['Content-Disposition'] = 'attachment; filename=bundle-%s.mbox' % \
+        name
     response.write(bundle.mbox())
     return response
 
+@login_required
+def mbox(request, bundle_id):
+    bundle = get_object_or_404(Bundle, id = bundle_id)
+    return mbox_response(bundle, str(bundle.id))
+
 def public(request, username, bundlename):
     user = get_object_or_404(User, username = username)
     bundle = get_object_or_404(Bundle, name = bundlename, owner = user,
@@ -191,3 +194,8 @@  def public(request, username, bundlename):
     context['bundle'] = bundle
 
     return render_to_response('patchwork/bundle-public.html', context)
+
+def public_mbox(request, username, bundlename):
+    bundle = get_object_or_404(Bundle, name = bundlename, public = True)
+    return mbox_response(bundle, bundle.name)
+    return response
diff --git a/templates/patchwork/bundle-public.html b/templates/patchwork/bundle-public.html
index 3590c46..4947a7e 100644
--- a/templates/patchwork/bundle-public.html
+++ b/templates/patchwork/bundle-public.html
@@ -7,6 +7,8 @@ 
 
 {% block body %}
 
+<a href="{% url patchwork.views.bundle.public_mbox username=bundle.owner bundlename=bundle.name %}">Download</a> bundle as mbox
+
 {% include "patchwork/patch-list.html" %}
 
 {% endblock %}