From: Jeremy Kerr Date: Sat, 20 Apr 2013 13:45:40 +0000 (+0800) Subject: bundles: Remove separate public bundle views X-Git-Url: https://git.ozlabs.org/?p=patchwork;a=commitdiff_plain;h=5d0140ef04ababd93c45b5126ee1b412bd778da5 bundles: Remove separate public bundle views Having two views for bundles (public and non-public) can cause confusion when bundle owners want to share a URL; it's not obvious that the private URL isn't shareable. This change removes the private URLs, and puts all bundles under the /bundle/// URL space. For non-public bundles, this will just 404 for non-owners. Signed-off-by: Jeremy Kerr --- diff --git a/apps/patchwork/models.py b/apps/patchwork/models.py index 86a5266..9129aab 100644 --- a/apps/patchwork/models.py +++ b/apps/patchwork/models.py @@ -363,12 +363,19 @@ class Bundle(models.Model): return None site = Site.objects.get_current() return 'http://%s%s' % (site.domain, - reverse('patchwork.views.bundle.public', + reverse('patchwork.views.bundle.bundle', kwargs = { 'username': self.owner.username, 'bundlename': self.name })) + @models.permalink + def get_absolute_url(self): + return ('patchwork.views.bundle.bundle', (), { + 'username': self.owner.username, + 'bundlename': self.name, + }) + def mbox(self): return '\n'.join([p.mbox().as_string(True) for p in self.ordered_patches()]) diff --git a/apps/patchwork/tests/bundles.py b/apps/patchwork/tests/bundles.py index 00a2323..0032978 100644 --- a/apps/patchwork/tests/bundles.py +++ b/apps/patchwork/tests/bundles.py @@ -22,9 +22,13 @@ import datetime from django.test import TestCase from django.test.client import Client from django.utils.http import urlencode +from django.conf import settings from patchwork.models import Patch, Bundle, BundlePatch, Person from patchwork.tests.utils import defaults, create_user, find_in_context +def bundle_url(bundle): + return '/bundle/%s/%s/' % (bundle.owner.username, bundle.name) + class BundleListTest(TestCase): def setUp(self): self.user = create_user() @@ -78,7 +82,7 @@ class BundleTestBase(TestCase): class BundleViewTest(BundleTestBase): def testEmptyBundle(self): - response = self.client.get('/user/bundle/%d/' % self.bundle.id) + response = self.client.get(bundle_url(self.bundle)) self.failUnlessEqual(response.status_code, 200) page = find_in_context(response.context, 'page') self.failUnlessEqual(len(page.object_list), 0) @@ -86,7 +90,7 @@ class BundleViewTest(BundleTestBase): def testNonEmptyBundle(self): self.bundle.append_patch(self.patches[0]) - response = self.client.get('/user/bundle/%d/' % self.bundle.id) + response = self.client.get(bundle_url(self.bundle)) self.failUnlessEqual(response.status_code, 200) page = find_in_context(response.context, 'page') self.failUnlessEqual(len(page.object_list), 1) @@ -95,7 +99,7 @@ class BundleViewTest(BundleTestBase): for patch in self.patches: self.bundle.append_patch(patch) - response = self.client.get('/user/bundle/%d/' % self.bundle.id) + response = self.client.get(bundle_url(self.bundle)) pos = 0 for patch in self.patches: @@ -113,7 +117,7 @@ class BundleViewTest(BundleTestBase): bundlepatch.save() i += 1 - response = self.client.get('/user/bundle/%d/' % self.bundle.id) + response = self.client.get(bundle_url(self.bundle)) pos = len(response.content) for patch in self.patches: next_pos = response.content.find(patch.name) @@ -147,7 +151,7 @@ class BundleUpdateTest(BundleTestBase): 'name': self.newname, 'public': self.publicString(not self.bundle.public) } - response = self.client.post('/user/bundle/%d/' % self.bundle.id, data) + response = self.client.post(bundle_url(self.bundle), data) self.assertEqual(response.status_code, 200) bundle = Bundle.objects.get(pk = self.bundle.pk) @@ -162,15 +166,12 @@ class BundleUpdateTest(BundleTestBase): 'name': newname, 'public': self.publicString(self.bundle.public) } - response = self.client.post('/user/bundle/%d/' % self.bundle.id, data) - self.assertEqual(response.status_code, 200) + response = self.client.post(bundle_url(self.bundle), data) bundle = Bundle.objects.get(pk = self.bundle.pk) + self.assertRedirects(response, bundle_url(bundle)) self.assertEqual(bundle.name, newname) self.assertEqual(bundle.public, self.bundle.public) - # check other forms for errors - self.checkPatchformErrors(response) - def testUpdatePublic(self): newname = 'newbundlename' data = { @@ -179,7 +180,7 @@ class BundleUpdateTest(BundleTestBase): 'name': self.bundle.name, 'public': self.publicString(not self.bundle.public) } - response = self.client.post('/user/bundle/%d/' % self.bundle.id, data) + response = self.client.post(bundle_url(self.bundle), data) self.assertEqual(response.status_code, 200) bundle = Bundle.objects.get(pk = self.bundle.pk) self.assertEqual(bundle.name, self.bundle.name) @@ -202,7 +203,7 @@ class BundlePublicViewTest(BundleTestBase): super(BundlePublicViewTest, self).setUp() self.client.logout() self.bundle.append_patch(self.patches[0]) - self.url = '/bundle/%s/%s/' % (self.user.username, self.bundle.name) + self.url = bundle_url(self.bundle) def testPublicBundle(self): self.bundle.public = True @@ -220,8 +221,52 @@ class BundlePublicViewTest(BundleTestBase): class BundlePublicViewMboxTest(BundlePublicViewTest): def setUp(self): super(BundlePublicViewMboxTest, self).setUp() - self.url = '/bundle/%s/%s/mbox/' % (self.user.username, - self.bundle.name) + self.url = bundle_url(self.bundle) + "mbox/" + +class BundlePublicModifyTest(BundleTestBase): + """Ensure that non-owners can't modify bundles""" + + def setUp(self): + super(BundlePublicModifyTest, self).setUp() + self.bundle.public = True + self.bundle.save() + self.other_user = create_user() + + def testBundleFormPresence(self): + """Check for presence of the modify form on the bundle""" + self.client.login(username = self.other_user.username, + password = self.other_user.username) + response = self.client.get(bundle_url(self.bundle)) + self.assertNotContains(response, 'name="form" value="bundle"') + + def testBundleFormSubmission(self): + oldname = 'oldbundlename' + newname = 'newbundlename' + data = { + 'form': 'bundle', + 'action': 'update', + 'name': newname, + } + self.bundle.name = oldname + self.bundle.save() + + # first, check that we can modify with the owner + self.client.login(username = self.user.username, + password = self.user.username) + response = self.client.post(bundle_url(self.bundle), data) + self.bundle = Bundle.objects.get(pk = self.bundle.pk) + self.assertEqual(self.bundle.name, newname) + + # reset bundle name + self.bundle.name = oldname + self.bundle.save() + + # log in with a different user, and check that we can no longer modify + self.client.login(username = self.other_user.username, + password = self.other_user.username) + response = self.client.post(bundle_url(self.bundle), data) + self.bundle = Bundle.objects.get(pk = self.bundle.pk) + self.assertNotEqual(self.bundle.name, newname) class BundleCreateFromListTest(BundleTestBase): def testCreateEmptyBundle(self): @@ -547,8 +592,7 @@ class BundleReorderTest(BundleTestBase): 'order_start': firstpatch.id, 'neworder': slice_ids} - response = self.client.post('/user/bundle/%d/' % self.bundle.id, - params) + response = self.client.post(bundle_url(self.bundle), params) self.failUnlessEqual(response.status_code, 200) @@ -579,3 +623,23 @@ class BundleReorderTest(BundleTestBase): def testBundleReorderMiddle(self): # reorder only 2nd, 3rd, and 4th patches self.checkReordering([0,2,3,1,4], 1, 4) + +class BundleRedirTest(BundleTestBase): + # old URL: private bundles used to be under /user/bundle/ + + def setUp(self): + super(BundleRedirTest, self).setUp() + + @unittest.skipIf(not settings.COMPAT_REDIR, "compat redirections disabled") + def testBundleRedir(self): + url = '/user/bundle/%d/' % self.bundle.id + response = self.client.get(url) + self.assertRedirects(response, bundle_url(self.bundle)) + + @unittest.skipIf(not settings.COMPAT_REDIR, "compat redirections disabled") + def testMboxRedir(self): + url = '/user/bundle/%d/mbox/' % self.bundle.id + response = self.client.get(url) + self.assertRedirects(response,'/bundle/%s/%s/mbox/' % + (self.bundle.owner.username, + self.bundle.name)) diff --git a/apps/patchwork/tests/user.py b/apps/patchwork/tests/user.py index e96e6c5..3df9dc7 100644 --- a/apps/patchwork/tests/user.py +++ b/apps/patchwork/tests/user.py @@ -24,7 +24,8 @@ from django.core import mail from django.core.urlresolvers import reverse from django.conf import settings from django.contrib.auth.models import User -from patchwork.models import EmailConfirmation, Person +from patchwork.models import EmailConfirmation, Person, Bundle +from patchwork.tests.utils import defaults def _confirmation_url(conf): return reverse('patchwork.views.confirm', kwargs = {'key': conf.key}) @@ -126,3 +127,31 @@ class UserLoginRedirectTest(TestCase): response = self.client.get(url) self.assertRedirects(response, settings.LOGIN_URL + '?next=' + url) +class UserProfileTest(TestCase): + + def setUp(self): + self.user = TestUser() + self.client.login(username = self.user.username, + password = self.user.password) + + def testUserProfile(self): + response = self.client.get('/user/') + self.assertContains(response, 'User Profile: %s' % self.user.username) + self.assertContains(response, 'User Profile: %s' % self.user.username) + + def testUserProfileNoBundles(self): + response = self.client.get('/user/') + self.assertContains(response, 'You have no bundles') + + def testUserProfileBundles(self): + project = defaults.project + project.save() + + bundle = Bundle(project = project, name = 'test-1', + owner = self.user.user) + bundle.save() + + response = self.client.get('/user/') + + self.assertContains(response, 'You have the following bundle') + self.assertContains(response, bundle.get_absolute_url()) diff --git a/apps/patchwork/urls.py b/apps/patchwork/urls.py index 3209830..a44f17c 100644 --- a/apps/patchwork/urls.py +++ b/apps/patchwork/urls.py @@ -39,10 +39,6 @@ urlpatterns = patterns('', (r'^user/bundles/$', 'patchwork.views.bundle.bundles'), - (r'^user/bundle/(?P[^/]+)/$', - 'patchwork.views.bundle.bundle'), - (r'^user/bundle/(?P[^/]+)/mbox/$', - 'patchwork.views.bundle.mbox'), (r'^user/link/$', 'patchwork.views.user.link'), (r'^user/unlink/(?P[^/]+)/$', 'patchwork.views.user.unlink'), @@ -66,9 +62,9 @@ urlpatterns = patterns('', # public view for bundles (r'^bundle/(?P[^/]*)/(?P[^/]*)/$', - 'patchwork.views.bundle.public'), + 'patchwork.views.bundle.bundle'), (r'^bundle/(?P[^/]*)/(?P[^/]*)/mbox/$', - 'patchwork.views.bundle.public_mbox'), + 'patchwork.views.bundle.mbox'), (r'^confirm/(?P[0-9a-f]+)/$', 'patchwork.views.confirm'), @@ -91,3 +87,13 @@ if settings.ENABLE_XMLRPC: (r'^project/(?P[^/]+)/pwclientrc/$', 'patchwork.views.pwclientrc'), ) + +# redirect from old urls +if settings.COMPAT_REDIR: + urlpatterns += patterns('', + (r'^user/bundle/(?P[^/]+)/$', + 'patchwork.views.bundle.bundle_redir'), + (r'^user/bundle/(?P[^/]+)/mbox/$', + 'patchwork.views.bundle.mbox_redir'), + ) + diff --git a/apps/patchwork/views/bundle.py b/apps/patchwork/views/bundle.py index 3846d23..3c89338 100644 --- a/apps/patchwork/views/bundle.py +++ b/apps/patchwork/views/bundle.py @@ -21,7 +21,7 @@ from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User from django.shortcuts import render_to_response, get_object_or_404 from patchwork.requestcontext import PatchworkRequestContext -from django.http import HttpResponse, HttpResponseRedirect +from django.http import HttpResponse, HttpResponseRedirect, HttpResponseNotFound import django.core.urlresolvers from patchwork.models import Patch, Bundle, BundlePatch, Project from patchwork.utils import get_patch_ids @@ -125,43 +125,58 @@ def bundles(request): return render_to_response('patchwork/bundles.html', context) -@login_required -def bundle(request, bundle_id): - bundle = get_object_or_404(Bundle, id = bundle_id) +def bundle(request, username, bundlename): + bundle = get_object_or_404(Bundle, owner__username = username, + name = bundlename) filter_settings = [(DelegateFilter, DelegateFilter.AnyDelegate)] - if request.method == 'POST' and request.POST.get('form') == 'bundle': - action = request.POST.get('action', '').lower() - if action == 'delete': - bundle.delete() - return HttpResponseRedirect( - django.core.urlresolvers.reverse( - 'patchwork.views.user.profile') - ) - elif action == 'update': - form = BundleForm(request.POST, instance = bundle) - if form.is_valid(): - form.save() + is_owner = request.user == bundle.owner + + if not (is_owner or bundle.public): + return HttpResponseNotFound() + + if is_owner: + if request.method == 'POST' and request.POST.get('form') == 'bundle': + action = request.POST.get('action', '').lower() + if action == 'delete': + bundle.delete() + return HttpResponseRedirect( + django.core.urlresolvers.reverse( + 'patchwork.views.user.profile') + ) + elif action == 'update': + form = BundleForm(request.POST, instance = bundle) + if form.is_valid(): + form.save() + + # if we've changed the bundle name, redirect to new URL + bundle = Bundle.objects.get(pk = bundle.pk) + if bundle.name != bundlename: + return HttpResponseRedirect(bundle.get_absolute_url()) + else: + form = BundleForm(instance = bundle) else: form = BundleForm(instance = bundle) - else: - form = BundleForm(instance = bundle) - if request.method == 'POST' and request.POST.get('form') == 'reorderform': - order = get_object_or_404(BundlePatch, bundle = bundle, - patch__id = request.POST.get('order_start')).order - - for patch_id in request.POST.getlist('neworder'): - bundlepatch = get_object_or_404(BundlePatch, - bundle = bundle, patch__id = patch_id) - bundlepatch.order = order - bundlepatch.save() - order += 1 + if request.method == 'POST' and \ + request.POST.get('form') == 'reorderform': + order = get_object_or_404(BundlePatch, bundle = bundle, + patch__id = request.POST.get('order_start')).order + + for patch_id in request.POST.getlist('neworder'): + bundlepatch = get_object_or_404(BundlePatch, + bundle = bundle, patch__id = patch_id) + bundlepatch.order = order + bundlepatch.save() + order += 1 + else: + form = None context = generic_list(request, bundle.project, 'patchwork.views.bundle.bundle', - view_args = {'bundle_id': bundle_id}, + view_args = {'username': bundle.owner.username, + 'bundlename': bundle.name}, filter_settings = filter_settings, patches = bundle.ordered_patches(), editable_order = True) @@ -171,7 +186,13 @@ def bundle(request, bundle_id): return render_to_response('patchwork/bundle.html', context) -def mbox_response(bundle): +def mbox(request, username, bundlename): + bundle = get_object_or_404(Bundle, owner__username = username, + name = bundlename) + + if not (request.user == bundle.owner or bundle.public): + return HttpResponseNotFound() + response = HttpResponse(mimetype='text/plain') response['Content-Disposition'] = \ 'attachment; filename=bundle-%d-%s.mbox' % (bundle.id, bundle.name) @@ -179,26 +200,18 @@ def mbox_response(bundle): return response @login_required -def mbox(request, bundle_id): - bundle = get_object_or_404(Bundle, id = bundle_id) - return mbox_response(bundle) - -def public(request, username, bundlename): - user = get_object_or_404(User, username = username) - bundle = get_object_or_404(Bundle, name = bundlename, owner = user, - public = True) - filter_settings = [(DelegateFilter, DelegateFilter.AnyDelegate)] - context = generic_list(request, bundle.project, - 'patchwork.views.bundle.public', - view_args = {'username': username, 'bundlename': bundlename}, - filter_settings = filter_settings, - patches = bundle.patches.all()) +def bundle_redir(request, bundle_id): + bundle = get_object_or_404(Bundle, id = bundle_id, owner = request.user) + return HttpResponseRedirect(bundle.get_absolute_url()) + +@login_required +def mbox_redir(request, bundle_id): + bundle = get_object_or_404(Bundle, id = bundle_id, owner = request.user) + return HttpResponseRedirect(django.core.urlresolvers.reverse( + 'patchwork.views.bundle.mbox', kwargs = { + 'username': request.user.username, + 'bundlename': bundle.name, + })) - 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) - return response diff --git a/apps/settings.py b/apps/settings.py index eabeffd..537c380 100644 --- a/apps/settings.py +++ b/apps/settings.py @@ -110,6 +110,10 @@ NOTIFICATION_FROM_EMAIL = DEFAULT_FROM_EMAIL # Set to True to enable the Patchwork XML-RPC interface ENABLE_XMLRPC = False +# set to True to enable redirections or URLs from previous versions +# of patchwork +COMPAT_REDIR = True + try: from local_settings import * except ImportError, ex: diff --git a/templates/patchwork/bundle-public.html b/templates/patchwork/bundle-public.html deleted file mode 100644 index 1a0347f..0000000 --- a/templates/patchwork/bundle-public.html +++ /dev/null @@ -1,20 +0,0 @@ -{% extends "base.html" %} - -{% load person %} - -{% block title %}{{project.name}}{% endblock %} -{% block heading %}bundle: {{bundle.owner.username}} / -{{bundle.name}}{% endblock %} - -{% block body %} - -

This bundle contains patches for the {{ bundle.project.linkname }} -project.

- -

Download bundle as mbox

- -{% include "patchwork/patch-list.html" %} - -{% endblock %} diff --git a/templates/patchwork/bundle.html b/templates/patchwork/bundle.html index a2933d5..54c2fcc 100644 --- a/templates/patchwork/bundle.html +++ b/templates/patchwork/bundle.html @@ -14,17 +14,17 @@ {% endblock %} {% block title %}{{project.name}}{% endblock %} -{% block heading %}bundle: {{bundle.name}}{% endblock %} +{% block heading %}bundle: {{bundle.owner.username}} / +{{bundle.name}}{% endblock %} {% block body %}

This bundle contains patches for the {{ bundle.project.linkname }} project.

-

Download bundle as mbox

- +

Download bundle as mbox

+{% if bundleform %}
{% csrf_token %} @@ -45,6 +45,7 @@ project.

+{% endif %} {% include "patchwork/patch-list.html" %} diff --git a/templates/patchwork/bundles.html b/templates/patchwork/bundles.html index 5340a64..3624f81 100644 --- a/templates/patchwork/bundles.html +++ b/templates/patchwork/bundles.html @@ -17,8 +17,7 @@ {% for bundle in bundles %} - {{ bundle.name }} + {{ bundle.name }} {{ bundle.project.linkname }} {% if bundle.public %} @@ -27,7 +26,7 @@ {{ bundle.n_patches }} download diff --git a/templates/patchwork/profile.html b/templates/patchwork/profile.html index 130b947..c02845b 100644 --- a/templates/patchwork/profile.html +++ b/templates/patchwork/profile.html @@ -104,8 +104,7 @@ address.

You have the following bundle{{ bundle|length|pluralize }}:

Visit the bundles