| Message ID | 1352164520-14744-1-git-send-email-idra@samba.org |
|---|---|
| State | Superseded |
| Headers | show |
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.
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
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.
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.
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
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.
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.
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
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.
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
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 %}
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(-)