Merge lp:~linaro-infrastructure/launchpad/team-engineering-view into lp:launchpad

[Guilherme Salgado]

[bug=965446][r=benji] New IPerson methods to get assigned SpecificationWorkItems/BugTasks targeted to a milestone whose due date is between today and a given date.

Those new methods will be used by a new view showing the upcoming work assigned to members of a team (https://dev.launchpad.net/Projects/WorkItems).

Both queries do some LeftJoins to bring in all the related data in a single query and avoid hitting the DB again for every returned item. The new filters to BugTaskSet.search() was a suggestion from Robert and we agreed to do the conjoined master filtering in python because the existing one in BugTaskSet.search() works only when you're filtering results for a single milestone.

Also, the plan is to not do batching on those pages as the page only includes stuff assigned to a team member *and* targeted to a milestone in the next few months. However, the new page will be guarded by a feature flag and we'll be conducting user testing to make sure we can avoid batching there.

[Benji York (benji)]

This branch looks good. The only think that struck me during the review
is that the fact that milestone_dateexpected_after is really "on or
after" and milestone_dateexpected_before is "on or before" is a little
confusing.

It may be that there is no better way to phrase that in an acceptable
variable name.

Maybe the thing to do is to add "(inclusive)" to the docstrings
describing the ranges for getAssignedSpecificationWorkItemsDueBefore and
getAssignedBugTasksDueBefore.

review: Approve (code)

[Guilherme Salgado (salgado)]

Thanks for the review, Benji!

On 26/03/12 12:41, Benji York wrote:
> Review: Approve code
>
> This branch looks good. The only think that struck me during the review
> is that the fact that milestone_dateexpected_after is really "on or
> after" and milestone_dateexpected_before is "on or before" is a little
> confusing.
>
> It may be that there is no better way to phrase that in an acceptable
> variable name.

Yeah, I'm not super happy with that either but couldn't think of a
reasonable name.

> Maybe the thing to do is to add "(inclusive)" to the docstrings
> describing the ranges for getAssignedSpecificationWorkItemsDueBefore and
> getAssignedBugTasksDueBefore.

I've done that change. Would you mind landing it for me?

[Robert Collins (lifeless)]

Just a data point: you're bring back the same people multiple times.
This is likely to be much slower than separate queries, once per type
of data being retrieved. (You will be updating your storm cache once
per person per workitem per milestone). This is *horrible* overhead.

[Benji York (benji)]

The recent revisions look great. The comments are especially nice.

review: Approve (code)

=== modified file 'lib/lp/blueprints/model/specificationworkitem.py'
--- lib/lp/blueprints/model/specificationworkitem.py	2012-02-28 04:24:19 +0000
+++ lib/lp/blueprints/model/specificationworkitem.py	2012-03-27 19:49:28 +0000
@@ -47,8 +47,9 @@
 
     def __repr__(self):
         title = self.title.encode('ASCII', 'backslashreplace')
+        assignee = getattr(self.assignee, 'name', None)
         return '<SpecificationWorkItem [%s] %s: %s of %s>' % (
-            self.assignee, title, self.status, self.specification)
+            assignee, title, self.status.name, self.specification)
 
     def __init__(self, title, status, specification, assignee, milestone,
                  sequence):

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2012-03-06 21:13:36 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2012-03-27 19:49:28 +0000
@@ -1185,7 +1185,8 @@
                  linked_branches=None, linked_blueprints=None,
                  structural_subscriber=None, modified_since=None,
                  created_since=None, exclude_conjoined_tasks=False, cve=None,
-                 upstream_target=None):
+                 upstream_target=None, milestone_dateexpected_before=None,
+                 milestone_dateexpected_after=None):
 
         self.bug = bug
         self.searchtext = searchtext
@@ -1236,6 +1237,8 @@
         self.exclude_conjoined_tasks = exclude_conjoined_tasks
         self.cve = cve
         self.upstream_target = upstream_target
+        self.milestone_dateexpected_before = milestone_dateexpected_before
+        self.milestone_dateexpected_after = milestone_dateexpected_after
 
     def setProduct(self, product):
         """Set the upstream context on which to filter the search."""

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-03-25 23:33:53 +0000
+++ lib/lp/bugs/model/bug.py	2012-03-27 19:49:28 +0000
@@ -2129,7 +2129,7 @@
         """A set of known persons able to view this bug.
 
         This method must return an empty set or bug searches will trigger late
-        evaluation. Any 'should be set on load' propertis must be done by the
+        evaluation. Any 'should be set on load' properties must be done by the
         bug search.
 
         If you are tempted to change this method, don't. Instead see

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-03-20 09:43:46 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-03-27 19:49:28 +0000
@@ -659,6 +659,18 @@
         extra_clauses.append(nominated_for_clause)
         clauseTables.append(BugNomination)
 
+    dateexpected_before = params.milestone_dateexpected_before
+    dateexpected_after = params.milestone_dateexpected_after
+    if dateexpected_after or dateexpected_before:
+        clauseTables.append(Milestone)
+        extra_clauses.append("BugTask.milestone = Milestone.id")
+        if dateexpected_after:
+            extra_clauses.append("Milestone.dateexpected >= %s"
+                                 % sqlvalues(dateexpected_after))
+        if dateexpected_before:
+            extra_clauses.append("Milestone.dateexpected <= %s"
+                                 % sqlvalues(dateexpected_before))
+
     clause, decorator = _get_bug_privacy_filter_with_decorator(params.user)
     if clause:
         extra_clauses.append(clause)

=== modified file 'lib/lp/bugs/tests/test_bugtask_search.py'
--- lib/lp/bugs/tests/test_bugtask_search.py	2012-03-08 11:51:36 +0000
+++ lib/lp/bugs/tests/test_bugtask_search.py	2012-03-27 19:49:28 +0000
@@ -1572,6 +1572,36 @@
         return [bugtask.bug.id for bugtask in expected_bugtasks]
 
 
+class TestMilestoneDueDateFiltering(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_milestone_date_filters(self):
+        today = datetime.today().date()
+        ten_days_ago = today - timedelta(days=10)
+        ten_days_from_now = today + timedelta(days=10)
+        current_milestone = self.factory.makeMilestone(dateexpected=today)
+        old_milestone = self.factory.makeMilestone(
+            dateexpected=ten_days_ago)
+        future_milestone = self.factory.makeMilestone(
+            dateexpected=ten_days_from_now)
+        current_milestone_bug = self.factory.makeBug(
+            milestone=current_milestone)
+        old_milestone_bug = self.factory.makeBug(milestone=old_milestone)
+        future_milestone_bug = self.factory.makeBug(
+            milestone=future_milestone)
+        # Search for bugs whose milestone.dateexpected is between yesterday
+        # and tomorrow.  This will return only the one task targeted to
+        # current_milestone.
+        params = BugTaskSearchParams(
+            user=None,
+            milestone_dateexpected_after=today - timedelta(days=1),
+            milestone_dateexpected_before=today + timedelta(days=1))
+        result = getUtility(IBugTaskSet).search(params)
+        self.assertEqual(
+            current_milestone_bug.bugtasks, list(result))
+
+
 def test_suite():
     suite = unittest.TestSuite()
     loader = unittest.TestLoader()

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2012-03-02 19:53:30 +0000
+++ lib/lp/registry/interfaces/person.py	2012-03-27 19:49:28 +0000
@@ -1518,6 +1518,20 @@
         :return: a boolean.
         """
 
+    def getAssignedSpecificationWorkItemsDueBefore(date):
+        """Return SpecificationWorkItems assigned to this person (or members
+        of this team) and whose milestone is due between today and the given
+        date (inclusive).
+        """
+
+    def getAssignedBugTasksDueBefore(date, user):
+        """Get all BugTasks assigned to this person (or members of this team)
+        and whose milestone is due between today and the given date
+        (inclusive).
+        """
+
+    participant_ids = List(
+        title=_("The DB IDs of this team's participants"), value_type=Int())
     active_member_count = Attribute(
         "The number of real people who are members of this team.")
     # activemembers.value_type.schema will be set to IPerson once

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-03-25 23:33:53 +0000
+++ lib/lp/registry/model/person.py	2012-03-27 19:49:28 +0000
@@ -62,6 +62,7 @@
 from storm.expr import (
     Alias,
     And,
+    Coalesce,
     Desc,
     Exists,
     In,
@@ -127,6 +128,7 @@
     SpecificationImplementationStatus,
     SpecificationSort,
     )
+from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.blueprints.model.specification import (
     HasSpecificationsMixin,
     Specification,
@@ -221,6 +223,7 @@
     KarmaCategory,
     KarmaTotalCache,
     )
+from lp.registry.model.milestone import Milestone
 from lp.registry.model.personlocation import PersonLocation
 from lp.registry.model.pillar import PillarName
 from lp.registry.model.sourcepackagename import SourcePackageName
@@ -230,7 +233,10 @@
     TeamParticipation,
     )
 from lp.services.config import config
-from lp.services.database import postgresql
+from lp.services.database import (
+    bulk,
+    postgresql,
+    )
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.datetimecol import UtcDateTimeCol
 from lp.services.database.decoratedresultset import DecoratedResultSet
@@ -290,6 +296,7 @@
     REDEEMABLE_VOUCHER_STATUSES,
     VOUCHER_STATUSES,
     )
+from lp.services.searchbuilder import any
 from lp.services.statistics.interfaces.statistic import ILaunchpadStatisticSet
 from lp.services.verification.interfaces.authtoken import LoginTokenType
 from lp.services.verification.interfaces.logintoken import ILoginTokenSet
@@ -1468,6 +1475,104 @@
         """See `IPerson`."""
         self._inTeam_cache = {}
 
+    @cachedproperty
+    def participant_ids(self):
+        """See `IPerson`."""
+        return list(Store.of(self).find(
+            TeamParticipation.personID, TeamParticipation.teamID == self.id))
+
+    def getAssignedSpecificationWorkItemsDueBefore(self, date):
+        """See `IPerson`."""
+        from lp.registry.model.person import Person
+        from lp.registry.model.product import Product
+        from lp.registry.model.distribution import Distribution
+        store = Store.of(self)
+        WorkItem = SpecificationWorkItem
+        origin = [
+            WorkItem,
+            Join(Specification, WorkItem.specification == Specification.id),
+            # WorkItems may not have a milestone and in that case they inherit
+            # the one from the spec.
+            Join(Milestone,
+                 Coalesce(WorkItem.milestone_id,
+                          Specification.milestoneID) == Milestone.id),
+            ]
+        today = datetime.today().date()
+        query = AND(
+            Milestone.dateexpected <= date, Milestone.dateexpected >= today,
+            OR(WorkItem.assignee_id.is_in(self.participant_ids),
+               AND(WorkItem.assignee == None,
+                   Specification.assigneeID.is_in(self.participant_ids))))
+        result = store.using(*origin).find(WorkItem, query)
+        def eager_load(workitems):
+            specs = bulk.load_related(
+                Specification, workitems, ['specification_id'])
+            bulk.load_related(Product, specs, ['productID'])
+            bulk.load_related(Distribution, specs, ['distributionID'])
+            assignee_ids = set(
+                [workitem.assignee_id for workitem in workitems]
+                + [spec.assigneeID for spec in specs])
+            assignee_ids.discard(None)
+            bulk.load(Person, assignee_ids, store)
+            milestone_ids = set(
+                [workitem.milestone_id for workitem in workitems]
+                + [spec.milestoneID for spec in specs])
+            milestone_ids.discard(None)
+            bulk.load(Milestone, milestone_ids, store)
+        return DecoratedResultSet(result, pre_iter_hook=eager_load)
+
+    def getAssignedBugTasksDueBefore(self, date, user):
+        """See `IPerson`."""
+        from lp.bugs.model.bugtask import BugTask
+        from lp.registry.model.distribution import Distribution
+        from lp.registry.model.distroseries import DistroSeries
+        from lp.registry.model.productseries import ProductSeries
+        today = datetime.today().date()
+        search_params = BugTaskSearchParams(
+            user, assignee=any(*self.participant_ids),
+            milestone_dateexpected_before=date,
+            milestone_dateexpected_after=today)
+
+        # Cast to a list to avoid DecoratedResultSet running pre_iter_hook
+        # multiple times when load_related() iterates over through the tasks.
+        tasks = list(getUtility(IBugTaskSet).search(search_params))
+        # Eager load the things we need that are not already eager loaded by
+        # BugTaskSet.search().
+        bulk.load_related(ProductSeries, tasks, ['productseriesID'])
+        bulk.load_related(Distribution, tasks, ['distributionID'])
+        bulk.load_related(DistroSeries, tasks, ['distroseriesID'])
+        bulk.load_related(Person, tasks, ['assigneeID'])
+        bulk.load_related(Milestone, tasks, ['milestoneID'])
+
+        for task in tasks:
+            # We skip masters (instead of slaves) from conjoined relationships
+            # because we can do that without hittind the DB, which would not
+            # be possible if we wanted to skip the slaves. The simple (but
+            # expensive) way to skip the slaves would be to skip any tasks
+            # that have a non-None .conjoined_master.
+            productseries = task.productseries
+            distroseries = task.distroseries
+            if productseries is not None and task.product is None:
+                dev_focus_id = productseries.product.development_focusID
+                if (productseries.id == dev_focus_id and
+                    task.status not in BugTask._NON_CONJOINED_STATUSES):
+                    continue
+            elif distroseries is not None:
+                candidate = None
+                for possible_slave in tasks:
+                    sourcepackagename_id = possible_slave.sourcepackagenameID
+                    if sourcepackagename_id == task.sourcepackagenameID:
+                        candidate = possible_slave
+                # Distribution.currentseries is expensive to run for every
+                # bugtask (as it goes through every series of that
+                # distribution), but it's a cached property and there's only
+                # one distribution with bugs in LP, so we can afford to do
+                # it here.
+                if (candidate is not None and
+                    distroseries.distribution.currentseries == distroseries):
+                    continue
+            yield task
+
     #
     # ITeam methods
     #

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2012-02-28 04:24:19 +0000
+++ lib/lp/registry/tests/test_person.py	2012-03-27 19:49:28 +0000
@@ -3,7 +3,10 @@
 
 __metaclass__ = type
 
-from datetime import datetime
+from datetime import (
+    datetime,
+    timedelta,
+    )
 
 from lazr.lifecycle.snapshot import Snapshot
 from lazr.restful.utils import smartquote
@@ -42,6 +45,10 @@
     get_recipients,
     Person,
     )
+from lp.services.database.sqlbase import (
+    flush_database_caches,
+    flush_database_updates,
+    )
 from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.account import AccountStatus
 from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
@@ -1113,3 +1120,338 @@
         self.assertContentEqual(
             [team2.teamowner],
             get_recipients(team2))
+
+
+class Test_getAssignedSpecificationWorkItemsDueBefore(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(Test_getAssignedSpecificationWorkItemsDueBefore, self).setUp()
+        self.team = self.factory.makeTeam()
+        today = datetime.today().date()
+        next_month = today + timedelta(days=30)
+        next_year = today + timedelta(days=366)
+        self.current_milestone = self.factory.makeMilestone(
+            dateexpected=next_month)
+        self.product = self.current_milestone.product
+        self.future_milestone = self.factory.makeMilestone(
+            dateexpected=next_year, product=self.product)
+
+    def test_basic(self):
+        assigned_spec = self.factory.makeSpecification(
+            assignee=self.team.teamowner, milestone=self.current_milestone,
+            product=self.product)
+        # Create a workitem with no explicit assignee/milestone. This way it
+        # will inherit the ones from the spec it belongs to.
+        workitem = self.factory.makeSpecificationWorkItem(
+            title=u'workitem 1', specification=assigned_spec)
+
+        # Create a workitem with somebody who's not a member of our team as
+        # the assignee. This workitem must not be in the list returned by
+        # getAssignedSpecificationWorkItemsDueBefore().
+        self.factory.makeSpecificationWorkItem(
+            title=u'workitem 2', specification=assigned_spec,
+            assignee=self.factory.makePerson())
+
+        # Create a workitem targeted to a milestone too far in the future.
+        # This workitem must not be in the list returned by
+        # getAssignedSpecificationWorkItemsDueBefore().
+        self.factory.makeSpecificationWorkItem(
+            title=u'workitem 3', specification=assigned_spec,
+            milestone=self.future_milestone)
+
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            self.current_milestone.dateexpected)
+
+        self.assertEqual([workitem], list(workitems))
+
+    def test_skips_workitems_with_milestone_in_the_past(self):
+        today = datetime.today().date()
+        milestone = self.factory.makeMilestone(
+            dateexpected=today - timedelta(days=1))
+        spec = self.factory.makeSpecification(
+            assignee=self.team.teamowner, milestone=milestone,
+            product=milestone.product)
+        self.factory.makeSpecificationWorkItem(
+            title=u'workitem 1', specification=spec)
+
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(today)
+
+        self.assertEqual([], list(workitems))
+
+    def test_includes_workitems_from_future_spec(self):
+        assigned_spec = self.factory.makeSpecification(
+            assignee=self.team.teamowner, milestone=self.future_milestone,
+            product=self.product)
+        # This workitem inherits the spec's milestone and that's too far in
+        # the future so it won't be in the returned list.
+        self.factory.makeSpecificationWorkItem(
+            title=u'workitem 1', specification=assigned_spec)
+        # This one, on the other hand, is explicitly targeted to the current
+        # milestone, so it is included in the returned list even though its
+        # spec is targeted to the future milestone.
+        workitem = self.factory.makeSpecificationWorkItem(
+            title=u'workitem 2', specification=assigned_spec,
+            milestone=self.current_milestone)
+
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            self.current_milestone.dateexpected)
+
+        self.assertEqual([workitem], list(workitems))
+
+    def test_includes_workitems_from_foreign_spec(self):
+        # This spec is assigned to a person who's not a member of our team, so
+        # only the workitems that are explicitly assigned to a member of our
+        # team will be in the returned list.
+        foreign_spec = self.factory.makeSpecification(
+            assignee=self.factory.makePerson(),
+            milestone=self.current_milestone, product=self.product)
+        # This one is not explicitly assigned to anyone, so it inherits the
+        # assignee of its spec and hence is not in the returned list.
+        self.factory.makeSpecificationWorkItem(
+            title=u'workitem 1', specification=foreign_spec)
+
+        # This one, on the other hand, is explicitly assigned to the a member
+        # of our team, so it is included in the returned list even though its
+        # spec is not assigned to a member of our team.
+        workitem = self.factory.makeSpecificationWorkItem(
+            title=u'workitem 2', specification=foreign_spec,
+            assignee=self.team.teamowner)
+
+        workitems = self.team.getAssignedSpecificationWorkItemsDueBefore(
+            self.current_milestone.dateexpected)
+
+        self.assertEqual([workitem], list(workitems))
+
+    def _makeProductSpec(self, milestone_dateexpected):
+        assignee = self.factory.makePerson()
+        with person_logged_in(self.team.teamowner):
+            self.team.addMember(assignee, reviewer=self.team.teamowner)
+        milestone = self.factory.makeMilestone(
+            dateexpected=milestone_dateexpected)
+        spec = self.factory.makeSpecification(
+            product=milestone.product, milestone=milestone, assignee=assignee)
+        return spec
+
+    def _makeDistroSpec(self, milestone_dateexpected):
+        assignee = self.factory.makePerson()
+        with person_logged_in(self.team.teamowner):
+            self.team.addMember(assignee, reviewer=self.team.teamowner)
+        distro = self.factory.makeDistribution()
+        milestone = self.factory.makeMilestone(
+            dateexpected=milestone_dateexpected, distribution=distro)
+        spec = self.factory.makeSpecification(
+            distribution=distro, milestone=milestone, assignee=assignee)
+        return spec
+
+    def test_query_count(self):
+        dateexpected = self.current_milestone.dateexpected
+        # Create 10 SpecificationWorkItems, each of them with a different
+        # specification, milestone and assignee. Also, half of the
+        # specifications will have a Product as a target and the other half
+        # will have a Distribution.
+        for i in range(5):
+            spec = self._makeProductSpec(dateexpected)
+            self.factory.makeSpecificationWorkItem(
+                title=u'product work item %d' % i, assignee=spec.assignee,
+                milestone=spec.milestone, specification=spec)
+            spec2 = self._makeDistroSpec(dateexpected)
+            self.factory.makeSpecificationWorkItem(
+                title=u'distro work item %d' % i, assignee=spec2.assignee,
+                milestone=spec2.milestone, specification=spec2)
+        flush_database_updates()
+        flush_database_caches()
+        with StormStatementRecorder() as recorder:
+            workitems = list(
+                self.team.getAssignedSpecificationWorkItemsDueBefore(
+                    dateexpected))
+            for workitem in workitems:
+                workitem.assignee
+                workitem.milestone
+                workitem.specification
+                workitem.specification.assignee
+                workitem.specification.milestone
+                workitem.specification.target
+        self.assertEqual(10, len(workitems))
+        # 1. One query to get all team members;
+        # 2. One to get all SpecWorkItems;
+        # 3. One to get all Specifications;
+        # 4. One to get all SpecWorkItem/Specification assignees;
+        # 5. One to get all SpecWorkItem/Specification milestones;
+        # 6. One to get all Specification products;
+        # 7. One to get all Specification distributions;
+        self.assertThat(recorder, HasQueryCount(Equals(7)))
+
+
+class Test_getAssignedBugTasksDueBefore(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(Test_getAssignedBugTasksDueBefore, self).setUp()
+        self.team = self.factory.makeTeam()
+        self.today = datetime.today().date()
+
+    def _assignBugTaskToTeamOwner(self, bugtask):
+        removeSecurityProxy(bugtask).assignee = self.team.teamowner
+
+    def test_basic(self):
+        milestone = self.factory.makeMilestone(dateexpected=self.today)
+        # This bug is assigned to a team member and targeted to a milestone
+        # whose due date is before the cutoff date we pass in, so it will be
+        # included in the return of getAssignedBugTasksDueBefore().
+        milestoned_bug = self.factory.makeBug(milestone=milestone)
+        self._assignBugTaskToTeamOwner(milestoned_bug.bugtasks[0])
+        # This one is assigned to a team member but not milestoned, so it is
+        # not included in the return of getAssignedBugTasksDueBefore().
+        non_milestoned_bug = self.factory.makeBug()
+        self._assignBugTaskToTeamOwner(non_milestoned_bug.bugtasks[0])
+        # This one is milestoned but not assigned to a team member, so it is
+        # not included in the return of getAssignedBugTasksDueBefore() either.
+        non_assigned_bug = self.factory.makeBug()
+        self._assignBugTaskToTeamOwner(non_assigned_bug.bugtasks[0])
+
+        bugtasks = list(self.team.getAssignedBugTasksDueBefore(
+            self.today + timedelta(days=1), user=None))
+
+        self.assertEqual(1, len(bugtasks))
+        self.assertEqual(milestoned_bug.bugtasks[0], bugtasks[0])
+
+    def test_skips_tasks_targeted_to_old_milestones(self):
+        past_milestone = self.factory.makeMilestone(
+            dateexpected=self.today - timedelta(days=1))
+        bug = self.factory.makeBug(milestone=past_milestone)
+        self._assignBugTaskToTeamOwner(bug.bugtasks[0])
+
+        bugtasks = list(self.team.getAssignedBugTasksDueBefore(
+            self.today + timedelta(days=1), user=None))
+
+        self.assertEqual(0, len(bugtasks))
+
+    def test_skips_private_bugs_the_user_is_not_allowed_to_see(self):
+        milestone = self.factory.makeMilestone(dateexpected=self.today)
+        private_bug = removeSecurityProxy(
+            self.factory.makeBug(milestone=milestone, private=True))
+        self._assignBugTaskToTeamOwner(private_bug.bugtasks[0])
+        private_bug2 = removeSecurityProxy(
+            self.factory.makeBug(milestone=milestone, private=True))
+        self._assignBugTaskToTeamOwner(private_bug2.bugtasks[0])
+
+        with person_logged_in(private_bug2.owner):
+            bugtasks = list(self.team.getAssignedBugTasksDueBefore(
+                self.today + timedelta(days=1),
+                removeSecurityProxy(private_bug2).owner))
+
+            self.assertEqual(private_bug2.bugtasks, bugtasks)
+
+    def test_skips_distroseries_task_that_is_a_conjoined_master(self):
+        distroseries = self.factory.makeDistroSeries()
+        sourcepackagename = self.factory.makeSourcePackageName()
+        milestone = self.factory.makeMilestone(
+            distroseries=distroseries, dateexpected=self.today)
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=distroseries, sourcepackagename=sourcepackagename)
+        bug = self.factory.makeBug(
+            milestone=milestone, sourcepackagename=sourcepackagename,
+            distribution=distroseries.distribution)
+        package = distroseries.getSourcePackage(sourcepackagename.name)
+        removeSecurityProxy(bug).addTask(bug.owner, package)
+        self.assertEqual(2, len(bug.bugtasks))
+        slave, master = bug.bugtasks
+        self._assignBugTaskToTeamOwner(master)
+        self.assertEqual(None, master.conjoined_master)
+        self.assertEqual(master, slave.conjoined_master)
+        self.assertEqual(slave.milestone, master.milestone)
+        self.assertEqual(slave.assignee, master.assignee)
+
+        bugtasks = list(self.team.getAssignedBugTasksDueBefore(
+            self.today + timedelta(days=1), user=None))
+
+        self.assertEqual([slave], bugtasks)
+
+    def test_skips_productseries_task_that_is_a_conjoined_master(self):
+        milestone = self.factory.makeMilestone(dateexpected=self.today)
+        removeSecurityProxy(milestone.product).development_focus = (
+            milestone.productseries)
+        bug = self.factory.makeBug(
+            series=milestone.productseries, milestone=milestone)
+        self.assertEqual(2, len(bug.bugtasks))
+        slave, master = bug.bugtasks
+
+        # This will cause the assignee to propagate to the other bugtask as
+        # well since they're conjoined.
+        self._assignBugTaskToTeamOwner(slave)
+        self.assertEqual(master, slave.conjoined_master)
+        self.assertEqual(slave.milestone, master.milestone)
+        self.assertEqual(slave.assignee, master.assignee)
+
+        bugtasks = list(self.team.getAssignedBugTasksDueBefore(
+            self.today + timedelta(days=1), user=None))
+
+        self.assertEqual([slave], bugtasks)
+
+    def _assignBugTaskToTeamOwnerAndSetMilestone(self, task, milestone):
+        self._assignBugTaskToTeamOwner(task)
+        removeSecurityProxy(task).milestone = milestone
+
+    def test_query_count(self):
+        # Create one Product bugtask;
+        milestone = self.factory.makeMilestone(dateexpected=self.today)
+        product_bug = self.factory.makeBug(product=milestone.product)
+        self._assignBugTaskToTeamOwnerAndSetMilestone(
+            product_bug.bugtasks[0], milestone)
+
+        # One ProductSeries bugtask;
+        productseries_bug = self.factory.makeBug(
+            series=milestone.productseries)
+        self._assignBugTaskToTeamOwnerAndSetMilestone(
+            productseries_bug.bugtasks[1], milestone)
+
+        # One DistroSeries bugtask;
+        distro = self.factory.makeDistribution()
+        distro_milestone = self.factory.makeMilestone(
+            distribution=distro, dateexpected=self.today)
+        distroseries_bug = self.factory.makeBug(
+            series=distro_milestone.distroseries)
+        self._assignBugTaskToTeamOwnerAndSetMilestone(
+            distroseries_bug.bugtasks[1], distro_milestone)
+
+        # One Distribution bugtask;
+        distro_bug = self.factory.makeBug(
+            distribution=distro_milestone.distribution)
+        self._assignBugTaskToTeamOwnerAndSetMilestone(
+            distro_bug.bugtasks[0], distro_milestone)
+
+        # One SourcePackage bugtask;
+        distroseries = distro_milestone.distroseries
+        sourcepackagename = self.factory.makeSourcePackageName()
+        self.factory.makeSourcePackagePublishingHistory(
+            distroseries=distroseries,
+            sourcepackagename=sourcepackagename)
+        sourcepackage_bug = self.factory.makeBug(
+            sourcepackagename=sourcepackagename, distribution=distro)
+        self._assignBugTaskToTeamOwnerAndSetMilestone(
+            sourcepackage_bug.bugtasks[0], distro_milestone)
+
+        flush_database_updates()
+        flush_database_caches()
+        with StormStatementRecorder() as recorder:
+            tasks = list(self.team.getAssignedBugTasksDueBefore(
+                self.today + timedelta(days=1), user=None))
+            for task in tasks:
+                task.bug
+                task.target
+                task.milestone
+                task.assignee
+        self.assertEqual(5, len(tasks))
+        # 1. One query to get all team members;
+        # 2. One to get all BugTasks;
+        # 3. One to get all assignees;
+        # 4. One to get all milestones;
+        # 5. One to get all products;
+        # 6. One to get all productseries;
+        # 7. One to get all distributions;
+        # 8. One to get all distroseries;
+        # 9. One to get all sourcepackagenames;
+        # 10. One to get all distroseries of a bug's distro. (See comment on
+        # getAssignedBugTasksDueBefore() to understand why it's needed)
+        self.assertThat(recorder, HasQueryCount(Equals(10)))

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2012-03-26 07:05:16 +0000
+++ lib/lp/testing/factory.py	2012-03-27 19:49:28 +0000
@@ -844,23 +844,26 @@
         return getUtility(ITranslatorSet).new(group, language, person)
 
     def makeMilestone(self, product=None, distribution=None,
-                      productseries=None, name=None, active=True):
-        if product is None and distribution is None and productseries is None:
+                      productseries=None, name=None, active=True,
+                      dateexpected=None, distroseries=None):
+        if (product is None and distribution is None and productseries is None
+            and distroseries is None):
             product = self.makeProduct()
-        if distribution is None:
+        if distribution is None and distroseries is None:
             if productseries is not None:
                 product = productseries.product
             else:
                 productseries = self.makeProductSeries(product=product)
-            distroseries = None
+        elif distroseries is None:
+            distroseries = self.makeDistroSeries(distribution=distribution)
         else:
-            distroseries = self.makeDistroSeries(distribution=distribution)
+            distribution = distroseries.distribution
         if name is None:
             name = self.getUniqueString()
         return ProxyFactory(
             Milestone(product=product, distribution=distribution,
                       productseries=productseries, distroseries=distroseries,
-                      name=name, active=active))
+                      name=name, active=active, dateexpected=dateexpected))
 
     def makeProcessor(self, family=None, name=None, title=None,
                       description=None):
@@ -1656,7 +1659,7 @@
             or distribution parameters, or the those parameters must be None.
         :param series: If set, the series.product must match the product
             parameter, or the series.distribution must match the distribution
-            parameter, or the those parameters must be None.
+            parameter, or those parameters must be None.
         :param tags: If set, the tags to be added with the bug.
         :param distribution: If set, the sourcepackagename is used as the
             default bug target.