launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02721
[Merge] lp:~gary/launchpad/subscriptions_for_bug-1 into lp:launchpad
Gary Poster has proposed merging lp:~gary/launchpad/subscriptions_for_bug-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~gary/launchpad/subscriptions_for_bug-1/+merge/50788
This branch adds an API to get all of the subscriptions for a set of bugtasks. I had a pre-implementation call with Graham Binns and a mid-implementation call with Danilo Segan.
I extracted a helper function from getStructuralSubscribers because my new method, getAllStructuralSubscriptions, needed some of the same logic. That logic did not appear to be tested well, so I added tests for it in addition to the functionality of the new method.
This method will be used on a view of the subscriptions for a bug. I exposed it there and added a simple test.
I agonized a bit about the location and name of the new method. In particular, the semantics of getStructuralSubscribers uses filters to determine which subscriptions are pertinent, while I wanted this new method to ignore filters. I used the word "All" in the name of the new method ("getAllStructuralSubscriptions") to try and highlight the difference.
I made several changes to placate lint. The whitespace changes for closing brackets seemed questionable to me, but it was simplest to go along with it; the rest of the lint changes were good.
--
https://code.launchpad.net/~gary/launchpad/subscriptions_for_bug-1/+merge/50788
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/subscriptions_for_bug-1 into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugsubscription.py'
--- lib/lp/bugs/browser/bugsubscription.py 2011-02-11 11:12:18 +0000
+++ lib/lp/bugs/browser/bugsubscription.py 2011-02-22 17:37:24 +0000
@@ -19,6 +19,7 @@
from zope import formlib
from zope.app.form import CustomWidgetFactory
from zope.app.form.browser.itemswidgets import RadioWidget
+from zope.component import getUtility
from zope.schema import Choice
from zope.schema.vocabulary import (
SimpleTerm,
@@ -40,6 +41,7 @@
from lp.bugs.browser.bug import BugViewMixin
from lp.bugs.enum import BugNotificationLevel
from lp.bugs.interfaces.bugsubscription import IBugSubscription
+from lp.bugs.interfaces.bugtask import IBugTaskSet
from lp.services import features
from lp.services.propertycache import cachedproperty
@@ -116,8 +118,7 @@
# drop the NOTHING option since it just makes the UI
# confusing.
for level in sorted(BugNotificationLevel.items, reverse=True)
- if level != BugNotificationLevel.NOTHING
- ]
+ if level != BugNotificationLevel.NOTHING]
bug_notification_vocabulary = SimpleVocabulary(
bug_notification_level_terms)
@@ -515,8 +516,7 @@
SubscriptionAttrDecorator(subscription)
for subscription in sorted(
self.context.getSubscriptionsFromDuplicates(),
- key=(lambda subscription: subscription.person.displayname))
- ]
+ key=(lambda subscription: subscription.person.displayname))]
class BugPortletSubcribersIds(LaunchpadView, BugViewMixin):
@@ -554,3 +554,8 @@
self.user.displayname, self.context.bug.id)
page_title = label
+
+ @property
+ def structural_subscriptions(self):
+ return getUtility(IBugTaskSet).getAllStructuralSubscriptions(
+ self.context.bug.bugtasks, self.user)
=== modified file 'lib/lp/bugs/browser/tests/test_bugsubscription_views.py'
--- lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-02-11 11:12:18 +0000
+++ lib/lp/bugs/browser/tests/test_bugsubscription_views.py 2011-02-22 17:37:24 +0000
@@ -10,7 +10,7 @@
from lp.bugs.browser.bugsubscription import (
BugPortletSubcribersIds,
- BugSubscriptionAddView,
+ BugSubscriptionListView,
BugSubscriptionSubscribeSelfView,
)
from lp.bugs.enum import BugNotificationLevel
@@ -63,7 +63,8 @@
level, subscription.bug_notification_level,
"Bug notification level of subscription should be %s, is "
"actually %s." % (
- level.title, subscription.bug_notification_level.title))
+ level.title,
+ subscription.bug_notification_level.title))
def test_nothing_is_not_a_valid_level(self):
# BugNotificationLevel.NOTHING isn't considered valid when
@@ -275,5 +276,18 @@
def setUp(self):
super(BugSubscriptionsListViewTestCase, self).setUp()
- self.bug = self.factory.makeBug()
+ self.product = self.factory.makeProduct(
+ name='widgetsrus', displayname='Widgets R Us')
+ self.bug = self.factory.makeBug(product=self.product)
self.subscriber = self.factory.makePerson()
+
+ def test_identify_structural_subscriptions(self):
+ # This shows simply that we can identify the structural
+ # subscriptions for the page. The content will come later.
+ with person_logged_in(self.subscriber):
+ sub = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ harness = LaunchpadFormHarness(
+ self.bug.default_bugtask, BugSubscriptionListView)
+ self.assertEqual(
+ list(harness.view.structural_subscriptions), [sub])
=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py 2011-02-21 01:10:31 +0000
+++ lib/lp/bugs/interfaces/bugtask.py 2011-02-22 17:37:24 +0000
@@ -1250,12 +1250,12 @@
elif not distroseries.query_values and distributions.query_values:
self.distributions = distributions
else:
- # Either we have both distroseries and distributions or neither
- # of them: set both, which will give us the cross product
- # due to the search of source packages being sourcepackagename
- # specific rather than actually context specific. This is not
- # ideal but is tolerable given no actual use of mixed type
- # any() exists today.
+ # Either we have both distroseries and distributions or
+ # neither of them: set both, which will give us the
+ # cross product due to the search of source packages
+ # being sourcepackagename specific rather than actually
+ # context specific. This is not ideal but is tolerable
+ # given no actual use of mixed type any() exists today.
self.distroseries = distroseries
self.distributions = distributions
return
@@ -1274,9 +1274,10 @@
does not need to be known to the caller.
:param target: A `IHasBug`, or some search term like all/any/none on
- `IHasBug`. If using all/any all the targets must be of the same
- type due to implementation limitations. Currently only distroseries
- and productseries `IHasBug` implementations are supported.
+ `IHasBug`. If using all/any all the targets must be of the
+ same type due to implementation limitations. Currently only
+ distroseries and productseries `IHasBug` implementations are
+ supported.
"""
# Yay circular deps.
from lp.registry.interfaces.distroseries import IDistroSeries
@@ -1492,8 +1493,8 @@
:param param: A BugTaskSearchParams object.
:param group_on: The column(s) group on - .e.g (
- Bugtask.distroseriesID, BugTask.milestoneID) will cause grouping by
- distro series and then milestone.
+ Bugtask.distroseriesID, BugTask.milestoneID) will cause
+ grouping by distro series and then milestone.
:return: A dict {group_instance: count, ...}
"""
@@ -1594,6 +1595,12 @@
def getOpenBugTasksPerProduct(user, products):
"""Return open bugtask count for multiple products."""
+ def getAllStructuralSubscriptions(bugtasks):
+ """Return all potential structural subscriptions for the bugtasks.
+
+ This method ignores subscription filters.
+ """
+
def getStructuralSubscribers(bugtasks, recipients=None, level=None):
"""Return `IPerson`s subscribed to the given bug tasks.
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2011-02-21 01:10:31 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-02-22 17:37:24 +0000
@@ -37,7 +37,6 @@
from storm.expr import (
Alias,
And,
- Count,
Desc,
In,
Join,
@@ -2227,8 +2226,7 @@
AND MessageChunk.fti @@ ftq(%s))""" % searchtext_quoted
text_search_clauses = [
"Bug.fti @@ ftq(%s)" % searchtext_quoted,
- "BugTask.fti @@ ftq(%s)" % searchtext_quoted
- ]
+ "BugTask.fti @@ ftq(%s)" % searchtext_quoted]
no_targetnamesearch = bool(features.getFeatureFlag(
'malone.disable_targetnamesearch'))
if not no_targetnamesearch:
@@ -2470,12 +2468,13 @@
eager_load = None
else:
requested_joins = kwargs.get('prejoins', [])
- # NB: We could save later work by predicting what sort of targets
- # we might be interested in here, but as at any point we're dealing
- # with relatively few results, this is likely to be a small win.
+ # NB: We could save later work by predicting what sort of
+ # targets we might be interested in here, but as at any
+ # point we're dealing with relatively few results, this is
+ # likely to be a small win.
prejoins = [
- (Bug, Join(Bug, BugTask.bug == Bug.id))
- ] + requested_joins
+ (Bug, Join(Bug, BugTask.bug == Bug.id))] + requested_joins
+
def eager_load(results):
product_ids = set([row[0].productID for row in results])
product_ids.discard(None)
@@ -2502,7 +2501,8 @@
def countBugs(self, params, group_on):
"""See `IBugTaskSet`."""
resultset = self._search(
- group_on + (SQL("COUNT(Distinct BugTask.bug)"),), [], None, params).result_set
+ group_on + (SQL("COUNT(Distinct BugTask.bug)"),),
+ [], None, params).result_set
# We group on the related field:
resultset.group_by(*group_on)
resultset.order_by()
@@ -3060,22 +3060,42 @@
return counts
- def getStructuralSubscribers(self, bugtasks, recipients=None, level=None):
- """See `IBugTaskSet`."""
- query_arguments = []
+ def _getStructuralSubscriptionTargets(self, bugtasks):
+ """Return (bugtask, target) pairs for each target of the bugtasks.
+
+ Each bugtask may be responsible theoretically for 0 or more targets.
+ In practice, each generates one, two or three.
+ """
for bugtask in bugtasks:
if IStructuralSubscriptionTarget.providedBy(bugtask.target):
- query_arguments.append((bugtask.target, bugtask))
+ yield (bugtask, bugtask.target)
if bugtask.target.parent_subscription_target is not None:
- query_arguments.append(
- (bugtask.target.parent_subscription_target, bugtask))
+ yield (bugtask, bugtask.target.parent_subscription_target)
if ISourcePackage.providedBy(bugtask.target):
# Distribution series bug tasks with a package have the source
# package set as their target, so we add the distroseries
# explicitly to the set of subscription targets.
- query_arguments.append((bugtask.distroseries, bugtask))
+ yield (bugtask, bugtask.distroseries)
if bugtask.milestone is not None:
- query_arguments.append((bugtask.milestone, bugtask))
+ yield (bugtask, bugtask.milestone)
+
+ def getAllStructuralSubscriptions(self, bugtasks, recipient):
+ """See `IBugTaskSet`."""
+ targets = [target for bugtask, target
+ in self._getStructuralSubscriptionTargets(bugtasks)]
+ if len(targets) == 0:
+ return EmptyResultSet()
+ union = lambda left, right: (
+ removeSecurityProxy(left).union(
+ removeSecurityProxy(right)))
+ queries = (
+ target.getSubscriptions(recipient) for target in targets)
+ return reduce(union, queries)
+
+ def getStructuralSubscribers(self, bugtasks, recipients=None, level=None):
+ """See `IBugTaskSet`."""
+ query_arguments = list(
+ self._getStructuralSubscriptionTargets(bugtasks))
if len(query_arguments) == 0:
return EmptyResultSet()
@@ -3091,7 +3111,7 @@
removeSecurityProxy(right)))
queries = (
target.getSubscriptionsForBugTask(bugtask, level)
- for target, bugtask in query_arguments)
+ for bugtask, target in query_arguments)
subscriptions = reduce(union, queries)
# Pull all the subscriptions in.
=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py 2011-02-21 16:03:38 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py 2011-02-22 17:37:24 +0000
@@ -15,6 +15,7 @@
)
from zope.component import getUtility
from zope.interface import providedBy
+from zope.security.proxy import removeSecurityProxy
from canonical.database.sqlbase import flush_database_updates
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
@@ -1342,6 +1343,146 @@
self.assertNotIn(BugTaskStatus.UNKNOWN, UNRESOLVED_BUGTASK_STATUSES)
+class TestGetStructuralSubscriptionTargets(TestCaseWithFactory):
+ # This tests a private method because it has some subtleties that are
+ # nice to test in isolation.
+
+ layer = DatabaseFunctionalLayer
+
+ def getStructuralSubscriptionTargets(self, bugtasks):
+ unwrapped = removeSecurityProxy(getUtility(IBugTaskSet))
+ return unwrapped._getStructuralSubscriptionTargets(bugtasks)
+
+ def test_product_target(self):
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(product=product)
+ bugtask = bug.bugtasks[0]
+ result = self.getStructuralSubscriptionTargets(bug.bugtasks)
+ self.assertEqual(list(result), [(bugtask, product)])
+
+ def test_milestone_target(self):
+ actor = self.factory.makePerson()
+ login_person(actor)
+ product = self.factory.makeProduct()
+ milestone = self.factory.makeMilestone(product=product)
+ bug = self.factory.makeBug(product=product, milestone=milestone)
+ bugtask = bug.bugtasks[0]
+ result = self.getStructuralSubscriptionTargets(bug.bugtasks)
+ self.assertEqual(set(result), set(
+ ((bugtask, product), (bugtask, milestone))))
+
+ def test_sourcepackage_target(self):
+ actor = self.factory.makePerson()
+ login_person(actor)
+ distroseries = self.factory.makeDistroSeries()
+ sourcepackage = self.factory.makeSourcePackage(
+ distroseries=distroseries)
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(product=product)
+ bug.addTask(actor, sourcepackage)
+ product_bugtask = bug.bugtasks[0]
+ sourcepackage_bugtask = bug.bugtasks[1]
+ result = self.getStructuralSubscriptionTargets(bug.bugtasks)
+ self.assertEqual(set(result), set(
+ ((product_bugtask, product),
+ (sourcepackage_bugtask, distroseries))))
+
+ def test_distribution_source_package_target(self):
+ actor = self.factory.makePerson()
+ login_person(actor)
+ distribution = self.factory.makeDistribution()
+ dist_sourcepackage = self.factory.makeDistributionSourcePackage(
+ distribution=distribution)
+ product = self.factory.makeProduct()
+ bug = self.factory.makeBug(product=product)
+ bug.addTask(actor, dist_sourcepackage)
+ product_bugtask = bug.bugtasks[0]
+ dist_sourcepackage_bugtask = bug.bugtasks[1]
+ result = self.getStructuralSubscriptionTargets(bug.bugtasks)
+ self.assertEqual(set(result), set(
+ ((product_bugtask, product),
+ (dist_sourcepackage_bugtask, dist_sourcepackage),
+ (dist_sourcepackage_bugtask, distribution))))
+
+
+class TestGetAllStructuralSubscriptions(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def getAllStructuralSubscriptions(self, bugtasks, recipient):
+ # Call IBugTaskSet.getAllStructuralSubscriptions() and check that the
+ # result is security proxied.
+ result = getUtility(IBugTaskSet).getAllStructuralSubscriptions(
+ bugtasks, recipient)
+ self.assertTrue(is_security_proxied_or_harmless(result))
+ return result
+
+ def setUp(self):
+ super(TestGetAllStructuralSubscriptions, self).setUp()
+ self.subscriber = self.factory.makePerson()
+ login_person(self.subscriber)
+ self.product = self.factory.makeProduct()
+ self.milestone = self.factory.makeMilestone(product=self.product)
+ self.bug = self.factory.makeBug(
+ product=self.product, milestone=self.milestone)
+
+ def test_no_subscriptions(self):
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertIsInstance(subscriptions, ResultSet)
+ self.assertEqual([], list(subscriptions))
+
+ def test_one_subscription(self):
+ sub = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertEqual([sub], list(subscriptions))
+
+ def test_two_subscriptions(self):
+ sub1 = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ sub2 = self.milestone.addBugSubscription(
+ self.subscriber, self.subscriber)
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertEqual(set([sub1, sub2]), set(subscriptions))
+
+ def test_two_bugtasks_one_subscription(self):
+ sub = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ product2 = self.factory.makeProduct()
+ self.bug.addTask(self.subscriber, product2)
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertEqual([sub], list(subscriptions))
+
+ def test_two_bugtasks_two_subscriptions(self):
+ sub1 = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ product2 = self.factory.makeProduct()
+ self.bug.addTask(self.subscriber, product2)
+ sub2 = product2.addBugSubscription(
+ self.subscriber, self.subscriber)
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertEqual(set([sub1, sub2]), set(subscriptions))
+
+ def test_ignore_other_subscriptions(self):
+ sub1 = self.product.addBugSubscription(
+ self.subscriber, self.subscriber)
+ another_subscriber = self.factory.makePerson()
+ login_person(another_subscriber)
+ sub2 = self.product.addBugSubscription(
+ another_subscriber, another_subscriber)
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, self.subscriber)
+ self.assertEqual([sub1], list(subscriptions))
+ subscriptions = self.getAllStructuralSubscriptions(
+ self.bug.bugtasks, another_subscriber)
+ self.assertEqual([sub2], list(subscriptions))
+
+
class TestGetStructuralSubscribers(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
=== modified file 'lib/lp/bugs/templates/bug-subscription-list.pt'
--- lib/lp/bugs/templates/bug-subscription-list.pt 2011-02-11 11:12:18 +0000
+++ lib/lp/bugs/templates/bug-subscription-list.pt 2011-02-22 17:37:24 +0000
@@ -15,7 +15,12 @@
<div id="maincontent">
<div id="nonportlets" class="readable">
-
+ <!-- This demo content shows use of view/structural_subscriptions. -->
+ <div tal:repeat="subscription view/structural_subscriptions">
+ <span tal:replace="string:
+ ${subscription/target/displayname} (${subscription/target/name})"
+ />
+ </div>
</div>
</div>
