Merge lp:~wallyworld/launchpad/remove-decoratedbug into lp:launchpad

[Ian Booth]

Remove the now obsolete DecoratedBug class and use Branch.getRelatedBugTasks() when displaying mp bugs

This branch is the first of 3. The bug which started it all said "Cannot view branch with revisions fixing private bugs". Fixing the root cause is to ensure that private bugs are not exposed via APIs on branch merge proposals.
The core work is done here: remove the now obsolete DecoratedBug class and the linked_bugs property on DecoratedBranch and add a new getRelatedBugTasks() API on branch merge proposal. The subsequent branches expose the new API on the webservice and do some more work to remove the merge proposal related_bugs property.

== Implementation ==

The basis of the approach is to use the recently added Branch.getRelatedBugTasks() API instead of DecoratedBranch.linked_bugs and DecoratedBug.
A new getRelatedBugTasks() API is added to IBranchMergeProposal. Some refactoring is done to use the new APIs. Some common functionality from the Branch.getRelatedBugTasks() was moved to a new filter_one_task_per_bug() helper method.

== Demo and QA ==

Create a merge proposal for a branch which is linked to at least one private bug. The private bug should not show on the merge proposal index page for an unauthorised user.
Merge the source branch of the merge proposal into another branch. The target branch's index page should show the recent revisions with the merge proposal but the private bug should not show for unauthorised users.

== Tests ==

Some new tests were added:
  - browser.tests.test_branchmergeproposal:
      test_linked_bugs_excludes_private_bugs()
  - model.tests.test_branchcollection:
      test_some_revisions_with_private_bugs()
  - model.tests.test_branchmergeproposal:
      test_related_bugtasks_includes_source_bugtasks()
      test_related_bugtasks_excludes_target_bugs()
      test_related_bugtasks_excludes_mutual_bugs()
      test_related_bugtasks_excludes_private_bugs()

Existing test_branchmergeproposal, test_branchcollection, test_branch tests were also run.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/configure.zcml
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/branchmergeproposal.py
  lib/lp/code/browser/decorations.py
  lib/lp/code/browser/tests/test_branch.py
  lib/lp/code/browser/tests/test_branchmergeproposal.py
  lib/lp/code/interfaces/branchcollection.py
  lib/lp/code/interfaces/branchmergeproposal.py
  lib/lp/code/model/branch.py
  lib/lp/code/model/branchcollection.py
  lib/lp/code/model/branchmergeproposal.py
  lib/lp/code/model/tests/test_branchcollection.py
  lib/lp/code/model/tests/test_branchmergeproposal.py
  lib/lp/code/templates/branch-macros.pt

./lib/lp/code/interfaces/branchcollection.py
      50: E301 expected 1 blank line, found 2
./lib/lp/code/model/branch.py
     317: Line exceeds 78 characters.
     739: Line exceeds 78 characters.

[Tim Penhey (thumper)]

In the function filter_one_task_per_bug, I think it would be better
to use the enumerate function to create values for the sorting, rather
that the length of the list. Also, remembering bug instead of traversing
over the attributes every time. Also indexing [1] looks kinda random.
So...

from collections import namedtuple

TaskIndex = namedtuple('TaskIndex', 'sort_order task')
for pos, task in enumerate(tasks):
    bug = task.bug
    if bug not in order:
        order[bug] = TaskIndex(pos, None)
    if task.target == bugtarget:
        order[bug].task = task
for task in tasks:
    index = order[task.bug]
    if index.task is None:
        index.task = task
return [index.task for index in sorted(order.values)]

That's all I have to say :-), and you still have more additions than
removals.

review: Approve

[Ian Booth (wallyworld)]

I extracted the filter_one_task_per_bug() code directly from the
getRelatedBugTasks() method so I could reuse it, hence I didn't pay too
much attention to it. I'll fix the issues you raise though.

On 04/03/11 13:08, Tim Penhey wrote:
> Review: Approve
> In the function filter_one_task_per_bug, I think it would be better
> to use the enumerate function to create values for the sorting, rather
> that the length of the list. Also, remembering bug instead of traversing
> over the attributes every time. Also indexing [1] looks kinda random.
> So...
>
> from collections import namedtuple
>
> TaskIndex = namedtuple('TaskIndex', 'sort_order task')
> for pos, task in enumerate(tasks):
> bug = task.bug
> if bug not in order:
> order[bug] = TaskIndex(pos, None)
> if task.target == bugtarget:
> order[bug].task = task
> for task in tasks:
> index = order[task.bug]
> if index.task is None:
> index.task = task
> return [index.task for index in sorted(order.values)]
>
> That's all I have to say :-), and you still have more additions than
> removals.
>

[Robert Collins (lifeless)]

On Fri, Mar 4, 2011 at 4:08 PM, Tim Penhey <email address hidden> wrote:
> Review: Approve
> In the function filter_one_task_per_bug, I think it would be better
> to use the enumerate function to create values for the sorting, rather
> that the length of the list.  Also, remembering bug instead of traversing
> over the attributes every time.  Also indexing [1] looks kinda random.
> So...
>
> from collections import namedtuple
>
> TaskIndex = namedtuple('TaskIndex', 'sort_order task')
> for pos, task in enumerate(tasks):
>    bug = task.bug
>    if bug not in order:
>        order[bug] = TaskIndex(pos, None)
>    if task.target == bugtarget:
>        order[bug].task = task
> for task in tasks:
>    index = order[task.bug]
>    if index.task is None:
>        index.task = task
> return [index.task for index in sorted(order.values)]
>
> That's all I have to say :-), and you still have more additions than
> removals.

That code won't work because tuples are immutable :) I suspect you'll
end up with a longer function that is less clear once you fix that.

[Ian Booth (wallyworld)]

>
> That code won't work because tuples are immutable :) I suspect you'll
> end up with a longer function that is less clear once you fix that.
>

Yes, so I've just used the suggested enumerate improvement.

There's some code which provides a mutable named tuple implementation:

http://code.activestate.com/recipes/576555/

We could consider using this at some point.

=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py	2011-02-27 19:45:44 +0000
+++ lib/lp/code/browser/branch.py	2011-03-04 11:06:55 +0000
@@ -613,7 +613,7 @@
     def revision_info(self):
         collection = getUtility(IAllBranches).visibleByUser(self.user)
         return collection.getExtendedRevisionDetails(
-            self.context.latest_revisions)
+            self.user, self.context.latest_revisions)
 
     @cachedproperty
     def latest_code_import_results(self):

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2011-02-27 20:49:43 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2011-03-04 11:06:55 +0000
@@ -94,10 +94,7 @@
 from lp.app.browser.tales import DateTimeFormatterAPI
 from lp.code.adapters.branch import BranchMergeProposalDelta
 from lp.code.browser.codereviewcomment import CodeReviewDisplayComment
-from lp.code.browser.decorations import (
-    DecoratedBranch,
-    DecoratedBug,
-    )
+from lp.code.browser.decorations import DecoratedBranch
 from lp.code.enums import (
     BranchMergeProposalStatus,
     BranchType,
@@ -681,18 +678,12 @@
         """Return whether or not the merge proposal has a linked bug or spec.
         """
         branch = self.context.source_branch
-        return branch.linked_bugs or branch.spec_links
-
-    @cachedproperty
-    def linked_bugs(self):
-        """Return DecoratedBugs linked to the source branch."""
-        return [DecoratedBug(bug, self.context.source_branch)
-                for bug in self.context.related_bugs]
+        return self.linked_bugtasks or branch.spec_links
 
     @cachedproperty
     def linked_bugtasks(self):
-        """Return DecoratedBugs linked to the source branch."""
-        return [bug.bugtask for bug in self.linked_bugs]
+        """Return BugTasks linked to the source branch."""
+        return self.context.getRelatedBugTasks(self.user)
 
     @property
     def edit_description_link_class(self):

=== modified file 'lib/lp/code/browser/decorations.py'
--- lib/lp/code/browser/decorations.py	2011-02-25 09:35:49 +0000
+++ lib/lp/code/browser/decorations.py	2011-03-04 11:06:55 +0000
@@ -6,16 +6,10 @@
 __metaclass__ = type
 __all__ = [
     'DecoratedBranch',
-    'DecoratedBug',
     ]
 
-
-from collections import defaultdict
-
 from lazr.delegates import delegates
 
-from canonical.launchpad.webapp.authorization import check_permission
-from lp.bugs.interfaces.bug import IBug
 from lp.code.interfaces.branch import (
     BzrIdentityMixin,
     IBranch,
@@ -23,64 +17,6 @@
 from lp.services.propertycache import cachedproperty
 
 
-class DecoratedBug:
-    """Provide some cached attributes to a normal bug.
-
-    We provide cached properties where sensible, and override default bug
-    behaviour where the cached properties can be used to avoid extra database
-    queries.
-    """
-    delegates(IBug, 'bug')
-
-    def __init__(self, bug, branch, tasks=None):
-        self.bug = bug
-        self.branch = branch
-        if tasks is None:
-            tasks = self.bug.bugtasks
-        self.tasks = tasks
-
-    @property
-    def bugtasks(self):
-        """This needs to be a property rather than an attribute.
-
-        If we try to assign to self.bugtasks, the lazr.delegates things we are
-        trying to assign to the property of the bug.
-        """
-        return self.tasks
-
-    @property
-    def default_bugtask(self):
-        """Use the first bugtask.
-
-        Use the cached tasks as calling default_bugtask on the bug object
-        causes a DB query.
-        """
-        return self.bugtasks[0]
-
-    def getBugTask(self, target):
-        """Get the bugtask for a specific target.
-
-        This method is overridden rather than letting it fall through to the
-        underlying bug as the underlying implementation gets the bugtasks from
-        self, which would in that case be the normal bug model object, which
-        would then hit the database to get the tasks.
-        """
-        # Copied from Bug.getBugTarget to avoid importing.
-        for bugtask in self.bugtasks:
-            if bugtask.target == target:
-                return bugtask
-        return None
-
-    @property
-    def bugtask(self):
-        """Return the bugtask for the branch project, or the default bugtask.
-
-        This method defers the identitication of the appropriate task to the
-        branch target.
-        """
-        return self.branch.target.getBugTask(self)
-
-
 class DecoratedBranch(BzrIdentityMixin):
     """Wrap a number of the branch accessors to cache results.
 
@@ -91,25 +27,6 @@
     def __init__(self, branch):
         self.branch = branch
 
-    @cachedproperty
-    def linked_bugs(self):
-        """Override the default behaviour of the branch object.
-
-        The default behaviour is just to get the bugs.  We want to display the
-        tasks however, and to avoid the extra database queries to get the
-        tasks, we get them all at once, and provide decorated bugs (that have
-        their tasks cached).
-        """
-        # To whomever it may concern, this function should be pushed down to
-        # the model, and the related visibility checks made part of the query.
-        # Alternatively it may be unused at this stage.
-        bugs = defaultdict(list)
-        for bug in self.branch.linked_bugs:
-            bugs[bug.id].extend(bug.bugtasks)
-        return [DecoratedBug(bug, self.branch, tasks)
-                for bug, tasks in bugs.iteritems()
-                if check_permission('launchpad.View', bug)]
-
     @property
     def displayname(self):
         """Override the default model property.

=== modified file 'lib/lp/code/browser/tests/test_branch.py'
--- lib/lp/code/browser/tests/test_branch.py	2011-02-27 21:14:53 +0000
+++ lib/lp/code/browser/tests/test_branch.py	2011-03-04 11:06:55 +0000
@@ -508,7 +508,8 @@
             for x in range(0, 2):
                 bug = self.factory.makeBug()
                 mp.source_branch.linkBug(bug, branch.owner)
-                linked_bug_urls.append(canonical_url(bug, rootsite='bugs'))
+                linked_bug_urls.append(
+                    canonical_url(bug.default_bugtask, rootsite='bugs'))
                 bug_text = "Bug #%s: %s" % (bug.id, bug.title)
                 linked_bug_text.append(bug_text)
 
@@ -534,7 +535,7 @@
             .*
             Testing the email address in revisions\n
             email Bob \([e-mail]\) for details.
-            Merged branch %s with linked bugs
+            Merged branch %s
             %s
             """ % (branch_display_name, linked_bug_rendered_text)
 

=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2010-12-22 00:26:20 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2011-03-04 11:06:55 +0000
@@ -726,7 +726,18 @@
         self.bmp.source_branch.linkBug(bug, self.bmp.registrant)
         self.bmp.target_branch.linkBug(bug, self.bmp.registrant)
         view = create_initialized_view(self.bmp, '+index')
-        self.assertEqual([], view.linked_bugs)
+        self.assertEqual([], view.linked_bugtasks)
+
+    def test_linked_bugs_excludes_private_bugs(self):
+        """List bugs that are linked to the source only."""
+        bug = self.factory.makeBug()
+        person = self.factory.makePerson()
+        private_bug = self.factory.makeBug(owner=person, private=True)
+        self.bmp.source_branch.linkBug(bug, self.bmp.registrant)
+        with person_logged_in(person):
+            self.bmp.source_branch.linkBug(private_bug, self.bmp.registrant)
+        view = create_initialized_view(self.bmp, '+index')
+        self.assertEqual([bug.default_bugtask], view.linked_bugtasks)
 
     def makeRevisionGroups(self):
         review_date = datetime(2009, 9, 10, tzinfo=pytz.UTC)

=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2011-03-02 13:22:31 +0000
+++ lib/lp/code/configure.zcml	2011-03-04 11:06:55 +0000
@@ -248,6 +248,7 @@
                     votes
                     all_comments
                     related_bugs
+                    getRelatedBugTasks
                     revision_end_date
                     isMergable
                     getComment

=== modified file 'lib/lp/code/interfaces/branchcollection.py'
--- lib/lp/code/interfaces/branchcollection.py	2011-03-03 01:13:47 +0000
+++ lib/lp/code/interfaces/branchcollection.py	2011-03-04 11:06:55 +0000
@@ -102,12 +102,16 @@
             are returned.
         """
 
-    def getExtendedRevisionDetails(revisions):
+    def getExtendedRevisionDetails(user, revisions):
         """Return information about the specified revisions on a branch.
 
         For each revision, see if the revision resulted from merging in a
         merge proposal, and if so package up the merge proposal and any linked
-        bugs on the merge proposal's source branch.
+        bug tasks on the merge proposal's source branch.
+
+        :param user: The user who is making the request. Only bug tasks
+            visible to this user are returned.
+        :param revisions: The revisions we want details for.
         """
 
     def getTeamsWithBranches(person):

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2011-02-04 16:13:13 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2011-03-04 11:06:55 +0000
@@ -296,6 +296,9 @@
     def getComment(id):
         """Return the CodeReviewComment with the specified ID."""
 
+    def getRelatedBugTasks(user):
+        """Return the Bug tasks related to this merge proposal."""
+
     def getRevisionsSinceReviewStart():
         """Return all the revisions added since the review began.
 

=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py	2011-03-03 01:13:47 +0000
+++ lib/lp/code/model/branch.py	2011-03-04 11:06:55 +0000
@@ -7,6 +7,7 @@
 __all__ = [
     'Branch',
     'BranchSet',
+    'filter_one_task_per_bug',
     ]
 
 from datetime import datetime
@@ -308,31 +309,14 @@
         'Bug', joinColumn='branch', otherColumn='bug',
         intermediateTable='BugBranch', orderBy='id')
 
-    def getLinkedBugTasks(self, user, status_filter):
+    def getLinkedBugTasks(self, user, status_filter=None):
         """See `IBranch`."""
         params = BugTaskSearchParams(user=user, linked_branches=self.id,
             status=status_filter)
         tasks = shortlist(getUtility(IBugTaskSet).search(params), 1000)
         # Post process to discard irrelevant tasks: we only return one task per
         # bug, and cannot easily express this in sql (yet).
-        order = {}
-        bugtarget = self.target.context
-        # First pass calculates the order and selects the bugtasks that match
-        # our target.
-        # Second pass selects the earliest bugtask where the bug has no task on
-        # our target.
-        for task in tasks:
-            if task.bug not in order:
-                order[task.bug] = [len(order) + 1, None]
-            if task.target == bugtarget:
-                order[task.bug][1] = task
-        for task in tasks:
-            if order[task.bug][1] is None:
-                order[task.bug][1] = task
-        # Now we pull out the tasks
-        result = order.values()
-        result.sort()
-        return [task for pos, task in result]
+        return filter_one_task_per_bug(self, tasks)
 
     def linkBug(self, bug, registrant):
         """See `IBranch`."""
@@ -1404,3 +1388,27 @@
     """
     update_trigger_modified_fields(branch)
     send_branch_modified_notifications(branch, event)
+
+
+def filter_one_task_per_bug(branch, tasks):
+    """Given bug tasks for a branch, discard irrelevant ones.
+
+    Cannot easily be expressed in SQL yet, so we need this helper method.
+    """
+    order = {}
+    bugtarget = branch.target.context
+    # First pass calculates the order and selects the bugtasks that match
+    # our target.
+    # Second pass selects the earliest bugtask where the bug has no task on
+    # our target.
+    for pos, task in enumerate(tasks):
+        bug = task.bug
+        if bug not in order:
+            order[bug] = [pos, None]
+        if task.target == bugtarget:
+            order[bug][1] = task
+    for task in tasks:
+        index = order[task.bug]
+        if index[1] is None:
+            index[1] = task
+    return [task for pos, task in sorted(order.values())]

=== modified file 'lib/lp/code/model/branchcollection.py'
--- lib/lp/code/model/branchcollection.py	2011-03-03 03:04:30 +0000
+++ lib/lp/code/model/branchcollection.py	2011-03-04 11:06:55 +0000
@@ -26,15 +26,21 @@
 from canonical.launchpad.components.decoratedresultset import (
     DecoratedResultSet,
     )
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
     IStoreSelector,
     MAIN_STORE,
     )
+from canonical.launchpad.searchbuilder import any
 from canonical.launchpad.webapp.vocabulary import CountableIterator
 from canonical.lazr.utils import safe_hasattr
-from lp.bugs.model.bug import Bug
+from lp.bugs.interfaces.bugtask import (
+    IBugTaskSet,
+    BugTaskSearchParams,
+    )
 from lp.bugs.model.bugbranch import BugBranch
+from lp.bugs.model.bugtask import BugTask
 from lp.code.interfaces.branch import user_has_special_branch_access
 from lp.code.interfaces.branchcollection import (
     IBranchCollection,
@@ -46,7 +52,10 @@
 from lp.code.enums import BranchMergeProposalStatus
 from lp.code.interfaces.branchlookup import IBranchLookup
 from lp.code.interfaces.codehosting import LAUNCHPAD_SERVICES
-from lp.code.model.branch import Branch
+from lp.code.model.branch import (
+    Branch,
+    filter_one_task_per_bug,
+    )
 from lp.code.model.branchmergeproposal import BranchMergeProposal
 from lp.code.model.branchsubscription import BranchSubscription
 from lp.code.model.codereviewcomment import CodeReviewComment
@@ -158,6 +167,7 @@
         resultset = self.store.using(*tables).find(Branch, *expressions)
         if not eager_load:
             return resultset
+
         def do_eager_load(rows):
             branch_ids = set(branch.id for branch in rows)
             if not branch_ids:
@@ -254,23 +264,24 @@
         proposals.order_by(Desc(CodeReviewComment.vote))
         return proposals
 
-    def getExtendedRevisionDetails(self, revisions):
+    def getExtendedRevisionDetails(self, user, revisions):
         """See `IBranchCollection`."""
 
         if not revisions:
             return []
         branch = revisions[0].branch
 
-        def make_rev_info(branch_revision, merge_proposal_revs, linked_bugs):
+        def make_rev_info(
+                branch_revision, merge_proposal_revs, linked_bugtasks):
             rev_info = {
                 'revision': branch_revision,
-                'linked_bugs': None,
+                'linked_bugtasks': None,
                 'merge_proposal': None,
                 }
             merge_proposal = merge_proposal_revs.get(branch_revision.sequence)
             rev_info['merge_proposal'] = merge_proposal
             if merge_proposal is not None:
-                rev_info['linked_bugs'] = linked_bugs.get(
+                rev_info['linked_bugtasks'] = linked_bugtasks.get(
                     merge_proposal.source_branch.id)
             return rev_info
 
@@ -281,19 +292,40 @@
         merge_proposal_revs = dict(
                 [(mp.merged_revno, mp) for mp in merge_proposals])
         source_branch_ids = [mp.source_branch.id for mp in merge_proposals]
-
-        filter = [
-            BugBranch.bug == Bug.id,
-            BugBranch.branchID.is_in(
-                source_branch_ids),
-            ]
-        bugs = self.store.find((Bug, BugBranch), filter)
-        linked_bugs = defaultdict(list)
-        for (bug, bugbranch) in bugs:
-            linked_bugs[bugbranch.branchID].append(bug)
+        linked_bugtasks = defaultdict(list)
+
+        if source_branch_ids:
+            # We get the bugtasks for our merge proposal branches
+
+            # First, the bug ids
+            params = BugTaskSearchParams(
+                user=user, status=None,
+                linked_branches=any(*source_branch_ids))
+            bug_ids = getUtility(IBugTaskSet).searchBugIds(params)
+
+            # Then the bug tasks and branches
+            store = IStore(BugBranch)
+            rs = store.using(
+                BugBranch,
+                Join(BugTask, BugTask.bugID == BugBranch.bugID),
+            ).find(
+                (BugTask, BugBranch),
+                BugBranch.bugID.is_in(bug_ids),
+                BugBranch.branchID.is_in(source_branch_ids)
+            )
+
+            # Build up a collection of bugtasks for each branch
+            bugtasks_for_branch = defaultdict(list)
+            for bugtask, bugbranch in rs:
+                bugtasks_for_branch[bugbranch.branch].append(bugtask)
+
+            # Now filter those down to one bugtask per branch
+            for branch, tasks in bugtasks_for_branch.iteritems():
+                linked_bugtasks[branch.id].extend(
+                    filter_one_task_per_bug(branch, tasks))
 
         return [make_rev_info(
-                rev, merge_proposal_revs, linked_bugs)
+                rev, merge_proposal_revs, linked_bugtasks)
                 for rev in revisions]
 
     def getTeamsWithBranches(self, person):

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2011-01-27 00:31:44 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2011-03-04 11:06:55 +0000
@@ -242,8 +242,22 @@
 
         Implies that these bugs would be fixed, in the target, by the merge.
         """
-        return (bug for bug in self.source_branch.linked_bugs
-                if bug not in self.target_branch.linked_bugs)
+        from operator import attrgetter
+        bugtasks = self.getRelatedBugTasks(self.registrant)
+        bugs = set()
+        for bugtask in bugtasks:
+            bugs.add(bugtask.bug)
+        return sorted(bugs, attrgetter('id'))
+
+    def getRelatedBugTasks(self, user):
+        """Bug tasks which are linked to the source but not the target.
+
+        Implies that these would be fixed, in the target, by the merge.
+        """
+        source_tasks = self.source_branch.getLinkedBugTasks(user)
+        target_tasks = self.target_branch.getLinkedBugTasks(user)
+        return [bugtask
+            for bugtask in source_tasks if bugtask not in target_tasks]
 
     @property
     def address(self):

=== modified file 'lib/lp/code/model/tests/test_branchcollection.py'
--- lib/lp/code/model/tests/test_branchcollection.py	2011-01-27 14:02:38 +0000
+++ lib/lp/code/model/tests/test_branchcollection.py	2011-03-04 11:06:55 +0000
@@ -613,17 +613,14 @@
         self.all_branches = getUtility(IAllBranches)
 
     def test_empty_revisions(self):
-        rev_details = self.all_branches.getExtendedRevisionDetails([])
-        self.assertEqual([], rev_details)
-        rev_details = self.all_branches.getExtendedRevisionDetails(None)
-        self.assertEqual([], rev_details)
-
-    def test_some_revisions(self):
-        branch = self.factory.makeBranch()
-        merge_proposals = [
-            self.factory.makeBranchMergeProposal(target_branch=branch)
-            for x in range(0, 2)]
-
+        person = self.factory.makePerson()
+        rev_details = self.all_branches.getExtendedRevisionDetails(person, [])
+        self.assertEqual([], rev_details)
+        rev_details = self.all_branches.getExtendedRevisionDetails(
+            person, None)
+        self.assertEqual([], rev_details)
+
+    def _makeBranchRevisions(self, merge_proposals, branch):
         expected_rev_details = []
         with person_logged_in(branch.owner):
             self.factory.makeRevisionsForBranch(branch, 3)
@@ -632,7 +629,7 @@
                 branch_revision = branch_revisions[x]
                 rev_info = {
                     'revision': branch_revision,
-                    'linked_bugs': None,
+                    'linked_bugtasks': None,
                     'merge_proposal': None,
                     }
                 if x < len(merge_proposals):
@@ -640,20 +637,64 @@
                             branch_revision.sequence)
                     rev_info['merge_proposal'] = merge_proposals[x]
                 expected_rev_details.append(rev_info)
+        return expected_rev_details, branch_revisions
+
+    def test_some_revisions_with_no_bugs(self):
+        branch = self.factory.makeBranch()
+        merge_proposals = [
+            self.factory.makeBranchMergeProposal(target_branch=branch)
+            for x in range(0, 2)]
+
+        expected_rev_details, branch_revisions = (
+            self._makeBranchRevisions(merge_proposals, branch))
 
         result = self.all_branches.getExtendedRevisionDetails(
-            branch_revisions)
+            branch.owner, branch_revisions)
         self.assertEqual(sorted(expected_rev_details), sorted(result))
 
-        linked_bugs = []
+    def test_some_revisions_with_bugs(self):
+        branch = self.factory.makeBranch()
+        merge_proposals = [
+            self.factory.makeBranchMergeProposal(target_branch=branch)
+            for x in range(0, 2)]
+
+        expected_rev_details, branch_revisions = (
+            self._makeBranchRevisions(merge_proposals, branch))
+
+        linked_bugtasks = []
         with person_logged_in(branch.owner):
             for x in range(0, 2):
                 bug = self.factory.makeBug()
                 merge_proposals[0].source_branch.linkBug(bug, branch.owner)
-                linked_bugs.append(bug)
-        expected_rev_details[0]['linked_bugs'] = linked_bugs
-        result = self.all_branches.getExtendedRevisionDetails(
-            branch_revisions)
+                linked_bugtasks.append(bug.default_bugtask)
+        expected_rev_details[0]['linked_bugtasks'] = linked_bugtasks
+        result = self.all_branches.getExtendedRevisionDetails(
+            branch.owner, branch_revisions)
+        self.assertEqual(sorted(expected_rev_details), sorted(result))
+
+    def test_some_revisions_with_private_bugs(self):
+        branch = self.factory.makeBranch()
+        merge_proposals = [
+            self.factory.makeBranchMergeProposal(target_branch=branch)
+            for x in range(0, 2)]
+
+        expected_rev_details, branch_revisions = (
+            self._makeBranchRevisions(merge_proposals, branch))
+
+        linked_bugtasks = []
+        with person_logged_in(branch.owner):
+            for x in range(0, 4):
+                private = x%2
+                bug = self.factory.makeBug(
+                    owner=branch.owner, private=private)
+                merge_proposals[0].source_branch.linkBug(bug, branch.owner)
+                if not private:
+                    linked_bugtasks.append(bug.default_bugtask)
+        expected_rev_details[0]['linked_bugtasks'] = linked_bugtasks
+
+        person = self.factory.makePerson()
+        result = self.all_branches.getExtendedRevisionDetails(
+            person, branch_revisions)
         self.assertEqual(sorted(expected_rev_details), sorted(result))
 
 

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2011-01-27 00:31:44 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2011-03-04 11:06:55 +0000
@@ -3,8 +3,6 @@
 
 # pylint: disable-msg=F0401
 
-from __future__ import with_statement
-
 """Tests for BranchMergeProposals."""
 
 __metaclass__ = type
@@ -28,10 +26,7 @@
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.constants import UTC_NOW
-from canonical.launchpad.ftests import (
-    import_secret_test_key,
-    syncUpdate,
-    )
+from canonical.launchpad.ftests import import_secret_test_key
 from canonical.launchpad.interfaces.launchpad import IPrivacy
 from canonical.launchpad.interfaces.message import IMessageJob
 from canonical.launchpad.webapp import canonical_url
@@ -83,7 +78,6 @@
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProductSet
 from lp.testing import (
-    ANONYMOUS,
     login,
     login_person,
     person_logged_in,
@@ -1272,6 +1266,46 @@
         bmp.target_branch.linkBug(bug, bmp.registrant)
         self.assertEqual([], list(bmp.related_bugs))
 
+    def test_related_bugtasks_includes_source_bugtasks(self):
+        """related_bugtasks includes bugtasks linked to the source branch."""
+        bmp = self.factory.makeBranchMergeProposal()
+        source_branch = bmp.source_branch
+        bug = self.factory.makeBug()
+        source_branch.linkBug(bug, bmp.registrant)
+        self.assertEqual(
+            bug.bugtasks, list(bmp.getRelatedBugTasks(self.user)))
+
+    def test_related_bugtasks_excludes_target_bugs(self):
+        """related_bugtasks ignores bugs linked to the source branch."""
+        bmp = self.factory.makeBranchMergeProposal()
+        bug = self.factory.makeBug()
+        bmp.target_branch.linkBug(bug, bmp.registrant)
+        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
+
+    def test_related_bugtasks_excludes_mutual_bugs(self):
+        """related_bugtasks ignores bugs linked to both branches."""
+        bmp = self.factory.makeBranchMergeProposal()
+        bug = self.factory.makeBug()
+        bmp.source_branch.linkBug(bug, bmp.registrant)
+        bmp.target_branch.linkBug(bug, bmp.registrant)
+        self.assertEqual([], list(bmp.getRelatedBugTasks(self.user)))
+
+    def test_related_bugtasks_excludes_private_bugs(self):
+        """related_bugtasks ignores private bugs for non-authorised users."""
+        bmp = self.factory.makeBranchMergeProposal()
+        bug = self.factory.makeBug()
+        bmp.source_branch.linkBug(bug, bmp.registrant)
+        person = self.factory.makePerson()
+        with person_logged_in(person):
+            private_bug = self.factory.makeBug(private=True, owner=person)
+            bmp.source_branch.linkBug(private_bug, person)
+        self.assertEqual(
+            bug.bugtasks, list(bmp.getRelatedBugTasks(self.user)))
+        all_bugtasks = list(bug.bugtasks)
+        all_bugtasks.extend(private_bug.bugtasks)
+        self.assertEqual(
+            all_bugtasks, list(bmp.getRelatedBugTasks(person)))
+
 
 class TestNotifyModified(TestCaseWithFactory):
 

=== modified file 'lib/lp/code/templates/branch-macros.pt'
--- lib/lp/code/templates/branch-macros.pt	2011-02-25 05:12:46 +0000
+++ lib/lp/code/templates/branch-macros.pt	2011-03-04 11:06:55 +0000
@@ -179,7 +179,7 @@
           revisions - the revisions to list.
         or
           revision_info - extended revision information (revision,
-                          merge_proposal, linked_bugs) to list.
+                          merge_proposal, linked_bugtasks) to list.
 
   </tal:comment>
   <style type="text/css">
@@ -214,7 +214,7 @@
           rev_no - the branch revision to be displayed.
         or
           rev_info - a dict of the branch revision information (revision,
-                     merge_proposal, linked_bugs) to be displayed.
+                     merge_proposal, linked_bugtasks) to be displayed.
       codebrowse - the branch's codebrowse root.
 
     It is expected that this macro is called from within a definition
@@ -263,12 +263,12 @@
         Merged branch <a tal:attributes="href merge_proposal/fmt:url"
                     tal:content="merge_proposal/source_branch/displayname">source branch</a>
         <tal:linkedbugs
-                define="linked_bugs python:rev_info['linked_bugs']"
-                condition="linked_bugs">with linked bugs
+                define="linked_bugtasks python:rev_info['linked_bugtasks']"
+                condition="linked_bugtasks">
           <dl>
-              <tal:bug repeat="bug linked_bugs">
+              <tal:bug repeat="bugtask linked_bugtasks">
                 <dd class="subordinate revision-comment">
-                  <tal:bug-link replace="structure bug/fmt:link"/>
+                  <tal:bug-link replace="structure bugtask/fmt:link"/>
                 </dd>
               </tal:bug>
           </dl>