launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/buglinks-refactor-more into lp:launchpad.

Commit message:
Clean up and test (un)linkBug, and drop bugtasksearch's "specification" sort order.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/buglinks-refactor-more/+merge/272586

Some more preparation for the XRefification of SpecificationBug, QuestionBug and BugCve.

 - (un)linkBug get cleaned up and some basic unit tests (karma etc. to later be moved out of doctests, once they're migrated).
 - bugtasksearch no longer supports sorting by spec name. It isn't in the UI nor documented anywhere publicly, it seemingly unused, it's slow, it's confusing (sorting alphabetically by alphabetically earliest linked spec), and I really can't be bothered porting to XRef.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/buglinks-refactor-more into lp:launchpad.

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2015-09-25 09:51:25 +0000
+++ lib/lp/answers/model/question.py	2015-09-28 12:40:21 +0000
@@ -681,11 +681,7 @@
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""
-        link = Store.of(self).find(QuestionBug, question=self, bug=bug).one()
-        if link is not None:
-            Store.of(link).remove(link)
-            return True
-        return False
+        Store.of(self).find(QuestionBug, question=self, bug=bug).remove()
 
     def setCommentVisibility(self, user, comment_number, visible):
         """See `IQuestion`."""

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2015-09-25 09:51:25 +0000
+++ lib/lp/blueprints/model/specification.py	2015-09-28 12:40:21 +0000
@@ -797,12 +797,8 @@
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""
-        link = Store.of(self).find(
-            SpecificationBug, specification=self, bug=bug).one()
-        if link is not None:
-            Store.of(link).remove(link)
-            return True
-        return False
+        Store.of(self).find(
+            SpecificationBug, specification=self, bug=bug).remove()
 
     # sprint linking
     def linkSprint(self, sprint, user):

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2015-09-25 02:33:15 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2015-09-28 12:40:21 +0000
@@ -839,3 +839,40 @@
         # they are automatically subscribed, if they do not have yet
         # been granted access to the specification.
         self.run_test_setting_special_role_subscribes('approver')
+
+
+class TestBugLinks(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_link_and_unlink(self):
+        login_person(self.factory.makePerson())
+
+        bug1 = self.factory.makeBug()
+        bug2 = self.factory.makeBug()
+        spec1 = self.factory.makeSpecification()
+        spec2 = self.factory.makeSpecification()
+        self.assertContentEqual([], bug1.specifications)
+        self.assertContentEqual([], bug2.specifications)
+        self.assertContentEqual([], spec1.bugs)
+        self.assertContentEqual([], spec2.bugs)
+
+        spec1.linkBug(bug1)
+        spec2.linkBug(bug1)
+        spec1.linkBug(bug2)
+        self.assertContentEqual([bug1, bug2], spec1.bugs)
+        self.assertContentEqual([bug1], spec2.bugs)
+        self.assertContentEqual([spec1, spec2], bug1.specifications)
+        self.assertContentEqual([spec1], bug2.specifications)
+
+        spec1.unlinkBug(bug1)
+        self.assertContentEqual([bug2], spec1.bugs)
+        self.assertContentEqual([bug1], spec2.bugs)
+        self.assertContentEqual([spec2], bug1.specifications)
+        self.assertContentEqual([spec1], bug2.specifications)
+
+        spec1.unlinkBug(bug2)
+        self.assertContentEqual([], spec1.bugs)
+        self.assertContentEqual([bug1], spec2.bugs)
+        self.assertContentEqual([spec2], bug1.specifications)
+        self.assertContentEqual([], bug2.specifications)

=== modified file 'lib/lp/bugs/browser/buglisting.py'
--- lib/lp/bugs/browser/buglisting.py	2015-09-08 05:39:50 +0000
+++ lib/lp/bugs/browser/buglisting.py	2015-09-28 12:40:21 +0000
@@ -884,7 +884,6 @@
     ('latest_patch_uploaded', 'Date latest patch uploaded', 'desc'),
     ('message_count', 'Number of comments', 'desc'),
     ('milestone', 'Milestone ID', 'desc'),
-    ('specification', 'Linked blueprint', 'asc'),
     ('task', 'Bug task ID', 'desc'),
     ('users_affected_count', 'Number of affected users', 'desc'),
     ]

=== modified file 'lib/lp/bugs/javascript/tests/test_buglisting_utils.js'
--- lib/lp/bugs/javascript/tests/test_buglisting_utils.js	2013-03-20 03:41:40 +0000
+++ lib/lp/bugs/javascript/tests/test_buglisting_utils.js	2015-09-28 12:40:21 +0000
@@ -470,7 +470,6 @@
         ['latest_patch_uploaded', 'Date latest patch uploaded'],
         ['message_count', 'Number of comments'],
         ['milestone', 'Milestone ID'],
-        ['specification', 'Linked blueprint'],
         ['task', 'Bug task ID'],
         ['users_affected_count', 'Number of affected users']
     ],

=== modified file 'lib/lp/bugs/model/buglinktarget.py'
--- lib/lp/bugs/model/buglinktarget.py	2015-09-25 09:51:25 +0000
+++ lib/lp/bugs/model/buglinktarget.py	2015-09-28 12:40:21 +0000
@@ -78,11 +78,9 @@
         if not check_permission('launchpad.View', bug):
             raise Unauthorized(
                 "cannot unlink a private bug you don't have access to")
-
-        # see if a relevant bug link exists, and if so, delete it
-        removed = self.deleteBugLink(bug)
-        if removed:
-            notify(ObjectUnlinkedEvent(bug, self, user=user))
-            notify(ObjectUnlinkedEvent(self, bug, user=user))
-            return True
-        return False
+        if bug not in self.bugs:
+            return False
+        self.deleteBugLink(bug)
+        notify(ObjectUnlinkedEvent(bug, self, user=user))
+        notify(ObjectUnlinkedEvent(self, bug, user=user))
+        return True

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2015-01-29 16:28:30 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2015-09-28 12:40:21 +0000
@@ -39,7 +39,6 @@
 
 from lp.app.enums import PUBLIC_INFORMATION_TYPES
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
-from lp.blueprints.model.specification import Specification
 from lp.blueprints.model.specificationbug import SpecificationBug
 from lp.bugs.errors import InvalidSearchParameters
 from lp.bugs.interfaces.bugattachment import BugAttachmentType
@@ -165,27 +164,6 @@
                         order_by=BugTag.tag, limit=1))),
             ]
         ),
-    "specification": (
-        Specification.name,
-        [
-            (Specification,
-                LeftJoin(
-                    Specification,
-                    # We want at most one specification per bug.
-                    # Select the specification that comes first
-                    # in alphabetic order.
-                    Specification.id == Select(
-                        Specification.id,
-                        tables=[
-                            SpecificationBug,
-                            Join(
-                                Specification,
-                                Specification.id ==
-                                    SpecificationBug.specificationID)],
-                        where=(SpecificationBug.bugID == BugTaskFlat.bug_id),
-                        order_by=Specification.name, limit=1))),
-            ]
-        ),
     }
 
 

=== modified file 'lib/lp/bugs/model/cve.py'
--- lib/lp/bugs/model/cve.py	2015-09-25 09:51:25 +0000
+++ lib/lp/bugs/model/cve.py	2015-09-28 12:40:21 +0000
@@ -91,11 +91,7 @@
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""
-        link = Store.of(self).find(BugCve, cve=self, bug=bug).one()
-        if link is not None:
-            Store.of(link).remove(link)
-            return True
-        return False
+        Store.of(self).find(BugCve, cve=self, bug=bug).remove()
 
 
 @implementer(ICveSet)

=== modified file 'lib/lp/bugs/model/tests/test_bugtasksearch.py'
--- lib/lp/bugs/model/tests/test_bugtasksearch.py	2015-02-19 05:49:54 +0000
+++ lib/lp/bugs/model/tests/test_bugtasksearch.py	2015-09-28 12:40:21 +0000
@@ -542,26 +542,6 @@
         params = self.getBugTaskSearchParams(user=None, orderby='-tag')
         self.assertSearchFinds(params, expected)
 
-    def test_sort_by_linked_specification(self):
-        with person_logged_in(self.owner):
-            spec_1 = self.factory.makeSpecification(
-                name='spec-1', owner=self.owner)
-            spec_1.linkBug(self.bugtasks[2].bug)
-            spec_1_1 = self.factory.makeSpecification(
-                name='spec-1-1', owner=self.owner)
-            spec_1_1.linkBug(self.bugtasks[2].bug)
-            spec_2 = self.factory.makeSpecification(
-                name='spec-2', owner=self.owner)
-            spec_2.linkBug(self.bugtasks[1].bug)
-        params = self.getBugTaskSearchParams(
-            user=None, orderby='specification')
-        expected = [self.bugtasks[2], self.bugtasks[1], self.bugtasks[0]]
-        self.assertSearchFinds(params, expected)
-        expected.reverse()
-        params = self.getBugTaskSearchParams(
-            user=None, orderby='-specification')
-        self.assertSearchFinds(params, expected)
-
     def test_sort_by_information_type(self):
         with person_logged_in(self.owner):
             self.bugtasks[0].bug.transitionToInformationType(

=== modified file 'lib/lp/bugs/tests/test_cve.py'
--- lib/lp/bugs/tests/test_cve.py	2012-12-26 01:32:19 +0000
+++ lib/lp/bugs/tests/test_cve.py	2015-09-28 12:40:21 +0000
@@ -8,6 +8,7 @@
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
 from lp.bugs.interfaces.cve import ICveSet
 from lp.testing import (
+    login_person,
     person_logged_in,
     TestCaseWithFactory,
     verifyObject,
@@ -75,3 +76,40 @@
             u'CVE-2000-0001', u'CVE-2000-0002', u'CVE-2000-0003',
             u'CVE-2000-0004']
         self.assertEqual(expected, cve_data)
+
+
+class TestBugLinks(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_link_and_unlink(self):
+        login_person(self.factory.makePerson())
+
+        bug1 = self.factory.makeBug()
+        bug2 = self.factory.makeBug()
+        cve1 = self.factory.makeCVE(sequence='2099-1234')
+        cve2 = self.factory.makeCVE(sequence='2099-2468')
+        self.assertContentEqual([], bug1.cves)
+        self.assertContentEqual([], bug2.cves)
+        self.assertContentEqual([], cve1.bugs)
+        self.assertContentEqual([], cve2.bugs)
+
+        cve1.linkBug(bug1)
+        cve2.linkBug(bug1)
+        cve1.linkBug(bug2)
+        self.assertContentEqual([bug1, bug2], cve1.bugs)
+        self.assertContentEqual([bug1], cve2.bugs)
+        self.assertContentEqual([cve1, cve2], bug1.cves)
+        self.assertContentEqual([cve1], bug2.cves)
+
+        cve1.unlinkBug(bug1)
+        self.assertContentEqual([bug2], cve1.bugs)
+        self.assertContentEqual([bug1], cve2.bugs)
+        self.assertContentEqual([cve2], bug1.cves)
+        self.assertContentEqual([cve1], bug2.cves)
+
+        cve1.unlinkBug(bug2)
+        self.assertContentEqual([], cve1.bugs)
+        self.assertContentEqual([bug1], cve2.bugs)
+        self.assertContentEqual([cve2], bug1.cves)
+        self.assertContentEqual([], bug2.cves)

=== added file 'lib/lp/coop/answersbugs/tests/test_questionbug.py'
--- lib/lp/coop/answersbugs/tests/test_questionbug.py	1970-01-01 00:00:00 +0000
+++ lib/lp/coop/answersbugs/tests/test_questionbug.py	2015-09-28 12:40:21 +0000
@@ -0,0 +1,45 @@
+# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from lp.testing import (
+    login_person,
+    TestCaseWithFactory,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestQuestionBugLinks(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def test_link_and_unlink(self):
+        login_person(self.factory.makePerson())
+
+        bug1 = self.factory.makeBug()
+        bug2 = self.factory.makeBug()
+        question1 = self.factory.makeQuestion()
+        question2 = self.factory.makeQuestion()
+        self.assertContentEqual([], bug1.questions)
+        self.assertContentEqual([], bug2.questions)
+        self.assertContentEqual([], question1.bugs)
+        self.assertContentEqual([], question2.bugs)
+
+        question1.linkBug(bug1)
+        question2.linkBug(bug1)
+        question1.linkBug(bug2)
+        self.assertContentEqual([bug1, bug2], question1.bugs)
+        self.assertContentEqual([bug1], question2.bugs)
+        self.assertContentEqual([question1, question2], bug1.questions)
+        self.assertContentEqual([question1], bug2.questions)
+
+        question1.unlinkBug(bug1)
+        self.assertContentEqual([bug2], question1.bugs)
+        self.assertContentEqual([bug1], question2.bugs)
+        self.assertContentEqual([question2], bug1.questions)
+        self.assertContentEqual([question1], bug2.questions)
+
+        question1.unlinkBug(bug2)
+        self.assertContentEqual([], question1.bugs)
+        self.assertContentEqual([bug1], question2.bugs)
+        self.assertContentEqual([question2], bug1.questions)
+        self.assertContentEqual([], bug2.questions)