launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/launchpad/choose-affected-product-oops-bug-370117 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #370117 in Launchpad itself: "BugTask:+choose-affected-product oops: getBugFilingAndSearchLinks needs more than 1 value to unpack"
  https://bugs.launchpad.net/launchpad/+bug/370117

For more details, see:
https://code.launchpad.net/~gmb/launchpad/choose-affected-product-oops-bug-370117/+merge/62704

This branch fixes an OOPS with Bug:+choose-affected-product. The OOPS was caused by code that didn't account for the fact that it might encounter data that wasn't valid in the context in which it was being called. I've fixed this by wrapping it in a try:except and have added a regression test.
-- 
https://code.launchpad.net/~gmb/launchpad/choose-affected-product-oops-bug-370117/+merge/62704
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/launchpad/choose-affected-product-oops-bug-370117 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtracker.py'
--- lib/lp/bugs/model/bugtracker.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/model/bugtracker.py	2011-05-27 16:17:32 +0000
@@ -448,11 +448,16 @@
         description = description.encode('utf-8')
 
         if self.bugtrackertype == BugTrackerType.SOURCEFORGE:
-            # SourceForge bug trackers use a group ID and an ATID to
-            # file a bug, rather than a product name. remote_product
-            # should be an ampersand-separated string in the form
-            # 'group_id&atid'
-            group_id, at_id = remote_product.split('&')
+            try:
+                # SourceForge bug trackers use a group ID and an ATID to
+                # file a bug, rather than a product name. remote_product
+                # should be an ampersand-separated string in the form
+                # 'group_id&atid'
+                group_id, at_id = remote_product.split('&')
+            except ValueError:
+                # If remote_product contains something that's not valid
+                # in a SourceForge context we just return early.
+                return None
 
             # If this bug tracker is the SourceForge celebrity the link
             # is to the new bug tracker rather than the old one.

=== modified file 'lib/lp/bugs/tests/test_bugtracker.py'
--- lib/lp/bugs/tests/test_bugtracker.py	2011-03-23 16:28:51 +0000
+++ lib/lp/bugs/tests/test_bugtracker.py	2011-05-27 16:17:32 +0000
@@ -351,6 +351,23 @@
         self.assertRaises(BugTrackerConnectError, tracker._csv_data)
 
 
+class TestSourceForge(TestCaseWithFactory):
+    """Tests for SourceForge-specific BugTracker code."""
+
+    layer = DatabaseFunctionalLayer
+
+    def test_getBugFilingAndSearchLinks_handles_bad_data_correctly(self):
+        # It's possible for Product.remote_product to contain data
+        # that's not valid for SourceForge BugTrackers.
+        # getBugFilingAndSearchLinks() will return None if it encounters
+        # bad data in the remote_product field.
+        remote_product = "this is not valid"
+        bug_tracker = self.factory.makeBugTracker(
+            bugtrackertype=BugTrackerType.SOURCEFORGE)
+        self.assertIs(
+            None, bug_tracker.getBugFilingAndSearchLinks(remote_product))
+
+
 class TestMakeBugtrackerName(TestCase):
     """Tests for make_bugtracker_name."""