Merge lp:~brian-murray/launchpad/bug-666496-findSimilar into lp:launchpad/db-devel

[Brian Murray]

Prevent returning the bug one started a findSimilarBugs() search with.

Bug 666496 is about findSimilarBugs() returning the same bug that a search started from, this happens for bug reports containing duplicate bugs. This branch fixes that and adds a test for it to lib/lp/bugs/doc/bugtask-find-similar.txt.

It can be tested via:

bin/test -cvvt bugtask-find-similar.txt

[Abel Deuring (adeuring)]

Looks good. Just one nitpick:

+XXX bdmurray 2010-11-05: This would be more appropriate as a unit test.

I appreciate that you mention unit tests, but our XXX policy says that an XXX should always have a related bug. But writing a unit test with just the test "no duplicate results" is IMHO pointless: other parts of lib/lp/bugs/doc/bugtask-find-similar.txt would also better live in a unit test, but that's beyond your bug fix. So I think you can simply remove the XXX, instead of explicitly adding a bug report. We already know that we want more unit tests ;)

review: Approve (code)

[lp:~brian-murray/launchpad/bug-666496-findSimilar]

11825. By Brian Murray on 2010-11-05

modifications based on reviewer feedback

=== modified file 'lib/lp/bugs/doc/bugtask-find-similar.txt'
--- lib/lp/bugs/doc/bugtask-find-similar.txt	2010-10-18 22:24:59 +0000
+++ lib/lp/bugs/doc/bugtask-find-similar.txt	2010-11-05 18:02:55 +0000
@@ -15,7 +15,8 @@
     >>> from lp.registry.interfaces.person import IPersonSet
     >>> from lp.registry.interfaces.product import IProductSet
     >>> firefox = getUtility(IProductSet).getByName('firefox')
-    >>> sample_person = getUtility(IPersonSet).getByEmail('[e-mail]')
+    >>> sample_person = getUtility(IPersonSet).getByEmail(
+    ...     '[e-mail]')
     >>> similar_bugs = getUtility(IBugTaskSet).findSimilar(
     ...     sample_person, '', product=firefox)
     >>> similar_bugs.count()
@@ -137,3 +138,21 @@
     ...     print bugtask.bug.title
     Thunderbird crashes
     >>> #another test bug
+
+== Not returning the same bug ==
+
+findSimilarBugs() does not include the bug of the bugtask upon which
+it is invoked.
+
+    >>> orig_bug = factory.makeBug(
+    ...     title="So you walk into this restaurant",
+    ...     owner=firefox.owner, product=firefox)
+
+    >>> dupe_bug = factory.makeBug(
+    ...     title="So you walk into this restaurant",
+    ...     owner=firefox.owner, product=firefox)
+    >>> dupe_bug.markAsDuplicate(orig_bug)
+
+    >>> similar_bugs = orig_bug.default_bugtask.findSimilarBugs(firefox.owner)
+    >>> orig_bug in similar_bugs
+    False

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2010-11-03 18:43:07 +0000
+++ lib/lp/bugs/model/bugtask.py	2010-11-05 18:02:55 +0000
@@ -667,16 +667,11 @@
         matching_bugtasks = getUtility(IBugTaskSet).findSimilar(
             user, self.bug.title, **context_params)
 
-        # Make sure to exclude the current BugTask from the list of
-        # matching tasks. We use 4*limit as an arbitrary value here to
-        # make sure we select more than :limit: bugtasks.
-        matching_bugtasks = [
-            bug_task for bug_task in matching_bugtasks[:4*limit]
-            if bug_task != self]
-
         matching_bugs = getUtility(IBugSet).getDistinctBugsForBugTasks(
             matching_bugtasks, user, limit)
-        return matching_bugs
+
+        # Make sure to exclude the bug of the current bugtask.
+        return [bug for bug in matching_bugs if bug.id != self.bugID]
 
     def subscribe(self, person, subscribed_by):
         """See `IBugTask`."""

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-11-04 19:59:02 +0000
+++ lib/lp/testing/factory.py	2010-11-05 18:02:55 +0000
@@ -1407,7 +1407,7 @@
             owner = self.makePerson()
 
         if IProductSeries.providedBy(target):
-            # We can't have a series task without a distribution task.
+            # We can't have a series task without a product task.
             self.makeBugTask(bug, target.product)
         if IDistroSeries.providedBy(target):
             # We can't have a series task without a distribution task.