launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/series-package-subscriptions into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #562001 in Launchpad itself: "Series structural subscriptions advanced search"
  https://bugs.launchpad.net/launchpad/+bug/562001

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/series-package-subscriptions/+merge/85188

Match package subscriptions when searching series bugs.

    Launchpad bug: https://bugs.launchpad.net/bugs/562001
    Pre-implementation: no one

Package subscriptions are only searched when the conntext bugtask
is a distribution. Search for package subscriptions for a distroseries
fails. bug teams subscribe to the package expect to search the series
to locate all the bugs that are the focus of development.

There are a few problems with the code. There is a mismatch between
BugTask columns and StructuralSubscription columns. There is no
distroseries column on SS to join on.

--------------------------------------------------------------------

RULES

    * Update the Distroseries bugtask search tests to inherit the
      ProjectGroupAndDistributionTests. This block of tests verify
      the subordinate subscription rules for uniqueness.
    * Add a test for the case where a bug team subscribed to a dsp,
      and searches the series with the structural_subscriber param.
    * Revise the distroseries clause in the
      "if params.structural_subscriber is not None:" block
      to search for exact matches on distroseries or matches to the
      distroseries's distro and package.
      ^ That is one sentence.

QA

    * Visit https://bugs.qastaging.launchpad.net/ubuntu/natty/+bugs?advanced=1
    * Enter ubuntu-server in the package subscriber field and search
    * Verify the result contains matches.


LINT

    lib/lp/bugs/browser/bugtask.py
    lib/lp/bugs/browser/tests/test_bugtask.py
    lib/lp/bugs/model/bugtask.py
    lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt
    lib/lp/bugs/tests/test_bugtask_search.py


TEST

    ./bin/test -vvc lp.bugs.tests.test_bugtask_search
    ^ This is a 30 minute unittest.

    ./bin/test -vvc -t xx-advanced-people-filters.txt lp.bugs.tests.test_doc
    ./bin/test -vvc -t label lp.bugs.browser.tests.test_bugtask


IMPLEMENTATION

Updated the distroseries bugtask search tests to include the
ProjectGroupAndDistributionTests tests.
Defined the required setUpStructuralSubscriptions.
Added a test to reproduce the production issue.
Updated the SQL to look for dsp structural subscriptions. The clause works
because the bugtasks that are not on the distroseries are rejected by
another clause.
    lib/lp/bugs/browser/tests/test_bugtask.py
    lib/lp/bugs/model/bugtask.py

Removed the extraneous comma from the label.
    lib/lp/bugs/browser/bugtask.py
    lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt
    lib/lp/bugs/tests/test_bugtask_search.py
-- 
https://code.launchpad.net/~sinzui/launchpad/series-package-subscriptions/+merge/85188
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/series-package-subscriptions into lp:launchpad.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-12-09 09:23:38 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-12-09 20:18:29 +0000
@@ -3108,7 +3108,7 @@
     @property
     def structural_subscriber_label(self):
         if IDistribution.providedBy(self.context):
-            return 'Package, or series subscriber'
+            return 'Package or series subscriber'
         elif IDistroSeries.providedBy(self.context):
             return 'Package subscriber'
         elif IProduct.providedBy(self.context):

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2011-12-08 13:23:00 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2011-12-09 20:18:29 +0000
@@ -1476,7 +1476,7 @@
         view = create_initialized_view(
             self.target, name=u'+bugs', rootsite='bugs')
         self.assertEqual(
-            'Package, or series subscriber', view.structural_subscriber_label)
+            'Package or series subscriber', view.structural_subscriber_label)
 
 
 class TestDistroSeriesBugs(TestCaseWithFactory):

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2011-11-29 12:19:38 +0000
+++ lib/lp/bugs/model/bugtask.py	2011-12-09 20:18:29 +0000
@@ -2121,11 +2121,23 @@
                         Or(BugTask.sourcepackagename ==
                             SQL('ss4.sourcepackagename'),
                            SQL('ss4.sourcepackagename IS NULL'))))))
+            if params.distroseries is not None:
+                parent_distro_id = params.distroseries.distributionID
+            else:
+                parent_distro_id = 0
             join_tables.append(
                 (None,
                  LeftJoin(
                     SQL('ss ss5'),
-                    BugTask.distroseries == SQL('ss5.distroseries'))))
+                    Or(BugTask.distroseries == SQL('ss5.distroseries'),
+                        # There is a mismatch between BugTask and
+                        # StructuralSubscription. SS does not support
+                        # distroseries. This clause works because other
+                        # joins ensure the match bugtask is the right
+                        # series.
+                        And(parent_distro_id == SQL('ss5.distribution'),
+                            BugTask.sourcepackagename == SQL(
+                                'ss5.sourcepackagename'))))))
             join_tables.append(
                 (None,
                  LeftJoin(

=== modified file 'lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt'
--- lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt	2011-12-05 16:05:20 +0000
+++ lib/lp/bugs/stories/bugtask-searches/xx-advanced-people-filters.txt	2011-12-09 20:18:29 +0000
@@ -140,7 +140,7 @@
 distribution, package, or series subscriber.
 
     >>> anon_browser.open('http://bugs.launchpad.dev/ubuntu/+bugs?advanced=1')
-    >>> anon_browser.getControl('Package, or series subscriber') is not None
+    >>> anon_browser.getControl('Package or series subscriber') is not None
     True
 
 Entering an existing person shows all bugs for packages or products that
@@ -150,7 +150,7 @@
 aren't any bugs open for pmount.
 
     >>> anon_browser.getControl(
-    ...     'Package, or series subscriber').value = 'foo.bar@xxxxxxxxxxxxx'
+    ...     'Package or series subscriber').value = 'foo.bar@xxxxxxxxxxxxx'
     >>> anon_browser.getControl('Search', index=0).click()
 
     >>> from lp.bugs.tests.bug import print_bugtasks

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2011-11-28 00:35:15 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2011-12-09 20:18:29 +0000
@@ -1158,7 +1158,7 @@
         return self.bugtasks[1:] + self.bugtasks[:1]
 
 
-class DistroseriesTarget(BugTargetTestBase):
+class DistroseriesTarget(BugTargetTestBase, ProjectGroupAndDistributionTests):
     """Use a distro series as the bug target."""
 
     def setUp(self):
@@ -1190,6 +1190,43 @@
             self.bugtasks[2].transitionToMilestone(milestone_2, self.owner)
         return self.bugtasks[1:] + self.bugtasks[:1]
 
+    def setUpStructuralSubscriptions(self, subscribe_search_target=True):
+        # See `ProjectGroupAndDistributionTests`.
+        # Users can search for series and package subscriptions. Users
+        # subscribe to packages at the distro level.
+        subscriber = self.factory.makePerson()
+        if subscribe_search_target:
+            self.subscribeToTarget(subscriber)
+        # Create a bug in a package in the series being searched.
+        sourcepackage = self.factory.makeSourcePackage(
+            distroseries=self.searchtarget)
+        self.bugtasks.append(self.factory.makeBugTask(target=sourcepackage))
+        # Create a bug in another series for the same package.
+        other_series = self.factory.makeDistroSeries(
+            distribution=self.searchtarget.distribution)
+        other_sourcepackage = self.factory.makeSourcePackage(
+            distroseries=other_series,
+            sourcepackagename=sourcepackage.sourcepackagename)
+        self.factory.makeBugTask(target=other_sourcepackage)
+        # Create a bug in the same distrubution package.
+        dsp = self.searchtarget.distribution.getSourcePackage(
+            sourcepackage.name)
+        self.factory.makeBugTask(target=dsp)
+        # Subscribe to the DSP to search both DSPs and SPs.
+        with person_logged_in(subscriber):
+            dsp.addSubscription(
+                subscriber, subscribed_by=subscriber)
+        return subscriber
+
+    def test_subordinate_structural_subscribers(self):
+        # Searching for a subscriber who is subscribed to only subordinate
+        # objects will match those objects
+        subscriber = self.setUpStructuralSubscriptions(
+            subscribe_search_target=False)
+        params = self.getBugTaskSearchParams(
+            user=None, structural_subscriber=subscriber)
+        self.assertSearchFinds(params, [self.bugtasks[-1]])
+
 
 class SourcePackageTarget(BugTargetTestBase):
     """Use a source package as the bug target."""