]> git.ozlabs.org Git - patchwork/commitdiff
bundles: Remove separate public bundle views
authorJeremy Kerr <jk@ozlabs.org>
Sat, 20 Apr 2013 13:45:40 +0000 (21:45 +0800)
committerJeremy Kerr <jk@ozlabs.org>
Sat, 20 Apr 2013 14:45:20 +0000 (22:45 +0800)
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/<username>/<bundlename>/ URL space. For non-public bundles, this
will just 404 for non-owners.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
apps/patchwork/models.py
apps/patchwork/tests/bundles.py
apps/patchwork/tests/user.py
apps/patchwork/urls.py
apps/patchwork/views/bundle.py
apps/settings.py
templates/patchwork/bundle-public.html [deleted file]
templates/patchwork/bundle.html
templates/patchwork/bundles.html
templates/patchwork/profile.html

index 86a5266cff2d2177f682c57e6fcea8897b616b66..9129aabddbc7dae5e57bd03df03f3f0d9832356a 100644 (file)
@@ -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()])
index 00a2323f79fe02ded4d44d10fea15f075cdc8925..0032978e3eb69f3bf69fd9fd1f3b17778941341a 100644 (file)
@@ -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/<id>
+
+    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))
index e96e6c537f4fce0e4b4e80d2056bf8b6d24ce0d3..3df9dc7ddaa170d6569337bd52a5f7d5f30b9bc7 100644 (file)
@@ -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())
index 320983032f85a74a5e88731a82a3984a26a74ad5..a44f17cd88195059833fbc1f03ea4c2144629c92 100644 (file)
@@ -39,10 +39,6 @@ urlpatterns = patterns('',
 
     (r'^user/bundles/$',
         'patchwork.views.bundle.bundles'),
-    (r'^user/bundle/(?P<bundle_id>[^/]+)/$',
-        'patchwork.views.bundle.bundle'),
-    (r'^user/bundle/(?P<bundle_id>[^/]+)/mbox/$',
-        'patchwork.views.bundle.mbox'),
 
     (r'^user/link/$', 'patchwork.views.user.link'),
     (r'^user/unlink/(?P<person_id>[^/]+)/$', 'patchwork.views.user.unlink'),
@@ -66,9 +62,9 @@ urlpatterns = patterns('',
 
     # public view for bundles
     (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/$',
-                                'patchwork.views.bundle.public'),
+                                'patchwork.views.bundle.bundle'),
     (r'^bundle/(?P<username>[^/]*)/(?P<bundlename>[^/]*)/mbox/$',
-                                'patchwork.views.bundle.public_mbox'),
+                                'patchwork.views.bundle.mbox'),
 
     (r'^confirm/(?P<key>[0-9a-f]+)/$', 'patchwork.views.confirm'),
 
@@ -91,3 +87,13 @@ if settings.ENABLE_XMLRPC:
         (r'^project/(?P<project_id>[^/]+)/pwclientrc/$',
              'patchwork.views.pwclientrc'),
     )
+
+# redirect from old urls
+if settings.COMPAT_REDIR:
+    urlpatterns += patterns('',
+        (r'^user/bundle/(?P<bundle_id>[^/]+)/$',
+            'patchwork.views.bundle.bundle_redir'),
+        (r'^user/bundle/(?P<bundle_id>[^/]+)/mbox/$',
+            'patchwork.views.bundle.mbox_redir'),
+    )
+
index 3846d239b8e305acfdcbb54f5535ac2ca7371016..3c89338b40ef6ec1f518eb25ca7a2a6b9186da8c 100644 (file)
@@ -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
index eabeffdbf5fcbd86565b62e166c7c2cbf93329dc..537c38053aa92163e7cc9c1d5b157f5e54eb42ef 100644 (file)
@@ -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 (file)
index 1a0347f..0000000
+++ /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 %}
-
-<p>This bundle contains patches for the {{ bundle.project.linkname }}
-project.</p>
-
-<p><a
-href="{% url patchwork.views.bundle.public_mbox username=bundle.owner.username bundlename=bundle.name %}"
->Download bundle as mbox</a></p>
-
-{% include "patchwork/patch-list.html" %}
-
-{% endblock %}
index a2933d5ff258ea9687c1edeff4f4b3e790086646..54c2fcc74997b6669326c7c7c039c06fed8ac888 100644 (file)
   </script>
 {% endblock %}
 {% block title %}{{project.name}}{% endblock %}
-{% block heading %}bundle: {{bundle.name}}{% endblock %}
+{% block heading %}bundle: {{bundle.owner.username}} /
+{{bundle.name}}{% endblock %}
 
 {% block body %}
 
 <p>This bundle contains patches for the {{ bundle.project.linkname }}
 project.</p>
 
-<p><a href="{% url patchwork.views.bundle.mbox bundle_id=bundle.id %}"
->Download bundle as mbox</a></p>
-
+<p><a href="{% url patchwork.views.bundle.mbox username=bundle.owner.username bundlename=bundle.name %}">Download bundle as mbox</a></p>
 
+{% if bundleform %}
 <form method="post">
  {% csrf_token %}
  <input type="hidden" name="form" value="bundle"/>
@@ -45,6 +45,7 @@ project.</p>
 </form>
 
 <div style="clear: both; padding: 1em;"></div>
+{% endif %}
 
 {% include "patchwork/patch-list.html" %}
 
index 5340a6430b132c943cc9dcb4425d76324c51f845..3624f8129cce08eab4384485d726995edc7a4a88 100644 (file)
@@ -17,8 +17,7 @@
  </tr>
 {% for bundle in bundles %}
  <tr>
-  <td><a href="{% url patchwork.views.bundle.bundle bundle_id=bundle.id %}"
-   >{{ bundle.name }}</a></td>
+  <td><a href="{{ bundle.get_absolute_url }}">{{ bundle.name }}</a></td>
   <td>{{ bundle.project.linkname }}</td>
   <td>
    {% if bundle.public %}
@@ -27,7 +26,7 @@
   </td>
   <td style="text-align: right">{{ bundle.n_patches }}</td>
   <td style="text-align: center;"><a
-   href="{% url patchwork.views.bundle.mbox bundle_id=bundle.id %}"
+   href="{% url patchwork.views.bundle.mbox username=bundle.owner.username bundlename=bundle.name %}"
    ><img src="/images/16-em-down.png" width="16" height="16" alt="download"
    title="download"/></a></td>
   <td style="text-align: center;">
index 130b94732500b657f6003ac3e15b26e502c0bf97..c02845b4fbe8a5b70b81755ba69642a1f0087682 100644 (file)
@@ -104,8 +104,7 @@ address.</p>
 <p>You have the following bundle{{ bundle|length|pluralize }}:</p>
 <ul>
 {% for bundle in bundles %}
- <li><a href="{% url patchwork.views.bundle.bundle bundle_id=bundle.id %}"
-   >{{ bundle.name }}</a></li>
+ <li><a href="{{ bundle.get_absolute_url }}">{{ bundle.name }}</a></li>
 {% endfor %}
 </ul>
 <p>Visit the <a href="{%url patchwork.views.bundle.bundles %}">bundles