[views] Check count() for duplicate bundle patches, rather than try/except

If the exception is raised, the transaction will still be in a bad
state, due to the foreign key constraint.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

2 files changed, 49 insertions, 4 deletions

diff --git a/apps/patchwork/tests/bundles.py b/apps/patchwork/tests/bundles.py
index d9c0bed..2f2f122 100644
--- a/apps/patchwork/tests/bundles.py
+++ b/apps/patchwork/tests/bundles.py
@@ -240,6 +240,49 @@ class BundleAddFromListTest(BundleTestBase):
                 for i in [0, 1] ]
         self.failUnless(bps[0].order < bps[1].order)
 
+    def testAddDuplicate(self):
+        self.bundle.append_patch(self.patches[0])
+        count = self.bundle.patches.count()
+        patch = self.patches[0]
+
+        params = {'form': 'patchlistform',
+                  'action': 'Add',
+                  'project': defaults.project.id,
+                  'bundle_id': self.bundle.id,
+                  'patch_id:%d' % patch.id: 'checked'}
+
+        response = self.client.post(
+                '/project/%s/list/' % defaults.project.linkname,
+                params)
+
+        self.assertContains(response, 'Patch &#39;%s&#39; already in bundle' \
+                            % patch.name, count = 1, status_code = 200)
+
+        self.assertEquals(count, self.bundle.patches.count())
+
+    def testAddNewAndDuplicate(self):
+        self.bundle.append_patch(self.patches[0])
+        count = self.bundle.patches.count()
+        patch = self.patches[0]
+
+        params = {'form': 'patchlistform',
+                  'action': 'Add',
+                  'project': defaults.project.id,
+                  'bundle_id': self.bundle.id,
+                  'patch_id:%d' % patch.id: 'checked',
+                  'patch_id:%d' % self.patches[1].id: 'checked'}
+
+        response = self.client.post(
+                '/project/%s/list/' % defaults.project.linkname,
+                params)
+
+        self.assertContains(response, 'Patch &#39;%s&#39; already in bundle' \
+                            % patch.name, count = 1, status_code = 200)
+        self.assertContains(response, 'Patch &#39;%s&#39; added to bundle' \
+                            % self.patches[1].name, count = 1,
+                            status_code = 200)
+        self.assertEquals(count + 1, self.bundle.patches.count())
+
 class BundleAddFromPatchTest(BundleTestBase):
     def testAddToEmptyBundle(self):
         patch = self.patches[0]
diff --git a/apps/patchwork/utils.py b/apps/patchwork/utils.py
index 3737e89..797951c 100644
--- a/apps/patchwork/utils.py
+++ b/apps/patchwork/utils.py
@@ -117,13 +117,15 @@ def set_bundle(user, project, action, data, patches, context):
 
     for patch in patches:
         if action == 'create' or action == 'add':
-            try:
+            bundlepatch_count = BundlePatch.objects.filter(bundle = bundle,
+                        patch = patch).count()
+            if bundlepatch_count == 0:
                 bundle.append_patch(patch)
                 context.add_message("Patch '%s' added to bundle %s" % \
                         (patch.name, bundle.name))
-            except Exception, ex:
-                context.add_message("Couldn't add patch '%s' to bundle: %s" % \
-                        (patch.name, ex.message))
+            else:
+                context.add_message("Patch '%s' already in bundle %s" % \
+                        (patch.name, bundle.name))
 
         elif action == 'remove':
             try: