launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02566
[Merge] lp:~jtv/launchpad/bug-711077 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-711077 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#711077 Product:+code-index timeouts
https://bugs.launchpad.net/bugs/711077
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-711077/+merge/49242
= Summary =
Some +code-index indexes were timing out.
There were several main culprits:
* Code to figure out whether a branch is attached to bugs was too roundabout.
* Following references between objects can be costly even from cache.
* Authors' names and icons were retrieved one icon at a time.
* Complex tooltips with last-revision author info were constructed in TAL.
* Counting and querying merge proposals is complex, costly, and duplicative.
* ValidPersonCache is now a view, and hard to cache.
== Proposed fix ==
I addressed most of the issues I found, just not the last two ones: branch merge proposals and ValidPersonCache. It might help if we could get rid of the ValidPersonCache view in the schema at some point.
Fixes:
* A new getBranchesWithVisibleBugs queries exactly what's needed, no more.
* Many references can be bypassed using foreign keys' numeric values.
* A new fetchAuthorsForDisplay queries persons, along with their icon info.
* A new link formatter renders RevisionAuthor links directly in python.
== Pre-implementation notes ==
Robert felt that GIL issues might be behind the problem, so I had a profile taken. I was unable to make much of the results though, and so focused on conventional optimization.
== Implementation details ==
Tests for getBranchesWithVisibleBugs are direct descendants of existing ones for getBugBranchesForBranches, which I gradually reworked since they weren't being used anywhere else. The new code passed the old tests, but in the end I broke them down into smaller units with more explicit test goals.
== Tests ==
I ran all non-Windmill code tests plus test_bugbranch, and have a full EC2 run underway, but for a quick peek at the most you can run:
{{{
./bin/test -vvc lp.bugs.tests.test_bugbranch -t getBranchesWithVisibleBugs
./bin/test -vvc lp.code.browser.tests.test_revisionauthor -t RevisionAuthorFormatter
./bin/test -vvc lp.code.model.tests.test_revision -t fetchAuthorsForDisplay
}}}
== Demo and Q/A ==
This is an optimization branch, so don't expect any changes in functionality. But have a look at: https://code.qastaging.launchpad.net/openobject-addons/+code-index
That page should render fast enough (once qastaging's cache is warmed up to a realistic level) and not break. In fact a cowboyed version of the branch is running on qastaging as I write this.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/templates/branch-listing.pt
lib/lp/bugs/tests/test_bugbranch.py
lib/lp/code/interfaces/revision.py
BRANCH.TODO
lib/lp/app/browser/configure.zcml
lib/lp/app/browser/tales.py
lib/lp/bugs/interfaces/bugbranch.py
lib/lp/code/model/tests/test_revision.py
lib/lp/bugs/model/bugbranch.py
lib/lp/code/model/revision.py
lib/lp/code/browser/branchlisting.py
lib/lp/code/browser/tests/test_revisionauthor.py
--
https://code.launchpad.net/~jtv/launchpad/bug-711077/+merge/49242
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-711077 into lp:launchpad.
=== modified file 'lib/lp/app/browser/configure.zcml'
--- lib/lp/app/browser/configure.zcml 2011-01-20 20:35:32 +0000
+++ lib/lp/app/browser/configure.zcml 2011-02-10 16:13:03 +0000
@@ -499,6 +499,12 @@
name="fmt"
/>
<adapter
+ for="lp.code.interfaces.revision.IRevisionAuthor"
+ provides="zope.traversing.interfaces.IPathAdapter"
+ factory="lp.app.browser.tales.RevisionAuthorFormatterAPI"
+ name="fmt"
+ />
+ <adapter
for="lp.translations.interfaces.translationgroup.ITranslationGroup"
provides="zope.traversing.interfaces.IPathAdapter"
factory="lp.app.browser.tales.TranslationGroupFormatterAPI"
=== modified file 'lib/lp/app/browser/tales.py'
--- lib/lp/app/browser/tales.py 2011-01-19 22:11:23 +0000
+++ lib/lp/app/browser/tales.py 2011-02-10 16:13:03 +0000
@@ -2253,6 +2253,25 @@
return u''
+class RevisionAuthorFormatterAPI(ObjectFormatterAPI):
+ """Adapter for `IRevisionAuthor` links."""
+
+ traversable_names = {'link': 'link'}
+
+ def link(self, view_name=None, rootsite='mainsite'):
+ """See `ObjectFormatterAPI`."""
+ context = self._context
+ if context.person is not None:
+ return PersonFormatterAPI(self._context.person).link(
+ view_name, rootsite)
+ elif context.name_without_email:
+ return cgi.escape(context.name_without_email)
+ elif getUtility(ILaunchBag).user is not None:
+ return cgi.escape(context.email)
+ else:
+ return "<email address hidden>"
+
+
def clean_path_segments(request):
"""Returns list of path segments, excluding system-related segments."""
proto_host_port = request.getApplicationURL()
=== modified file 'lib/lp/bugs/interfaces/bugbranch.py'
--- lib/lp/bugs/interfaces/bugbranch.py 2010-11-09 13:32:50 +0000
+++ lib/lp/bugs/interfaces/bugbranch.py 2011-02-10 16:13:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -48,6 +48,7 @@
BugField(
title=_("Bug #"),
required=True, readonly=True))
+ branch_id = Int(title=_("Branch ID"), required=True, readonly=True)
branch = exported(
ReferenceChoice(
title=_("Branch"), schema=IBranch,
@@ -75,11 +76,14 @@
Return None if there is no such link.
"""
- def getBugBranchesForBranches(branches, user):
- """Return a sequence of IBugBranch instances associated with
- the given branches.
+ def getBranchesWithVisibleBugs(branches, user):
+ """Find which of `branches` are for bugs that `user` can see.
- Only return instances that are visible to the user.
+ :param branches: A sequence of `Branch`es to limit the search
+ to.
+ :return: A result set of `Branch` ids: a subset of the ids
+ found in `branches`, but limited to branches that are
+ visible to `user`.
"""
def getBugBranchesForBugTasks(tasks):
=== modified file 'lib/lp/bugs/model/bugbranch.py'
--- lib/lp/bugs/model/bugbranch.py 2010-10-03 15:30:06 +0000
+++ lib/lp/bugs/model/bugbranch.py 2011-02-10 16:13:03 +0000
@@ -13,24 +13,29 @@
from sqlobject import (
ForeignKey,
IN,
+ IntCol,
StringCol,
)
+from storm.expr import (
+ And,
+ Or,
+ Select,
+ )
from zope.component import getUtility
from zope.interface import implements
from canonical.database.constants import UTC_NOW
from canonical.database.datetimecol import UtcDateTimeCol
-from canonical.database.sqlbase import (
- SQLBase,
- sqlvalues,
- )
+from canonical.database.sqlbase import SQLBase
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
+from canonical.launchpad.interfaces.lpstorm import IStore
from lp.bugs.interfaces.bugbranch import (
IBugBranch,
IBugBranchSet,
)
from lp.code.interfaces.branchtarget import IHasBranchTarget
from lp.registry.interfaces.person import validate_public_person
+from lp.registry.model.teammembership import TeamParticipation
class BugBranch(SQLBase):
@@ -39,6 +44,7 @@
datecreated = UtcDateTimeCol(notNull=True, default=UTC_NOW)
bug = ForeignKey(dbName="bug", foreignKey="Bug", notNull=True)
+ branch_id = IntCol(dbName="branch", notNull=True)
branch = ForeignKey(dbName="branch", foreignKey="Branch", notNull=True)
revision_hint = StringCol(default=None)
@@ -69,41 +75,45 @@
"See `IBugBranchSet`."
return BugBranch.selectOneBy(bugID=bug.id, branchID=branch.id)
- def getBugBranchesForBranches(self, branches, user):
- "See IBugBranchSet."
+ def getBranchesWithVisibleBugs(self, branches, user):
+ """See `IBugBranchSet`."""
+ # Avoid circular imports.
+ from lp.bugs.model.bug import Bug
+ from lp.bugs.model.bugsubscription import BugSubscription
+
branch_ids = [branch.id for branch in branches]
- if not branch_ids:
+ if branch_ids == []:
return []
- where_clauses = []
-
- # Select only bug branch links for the branches specified,
- # and join with the Bug table.
- where_clauses.append("""
- BugBranch.branch in %s AND
- BugBranch.bug = Bug.id""" % sqlvalues(branch_ids))
admins = getUtility(ILaunchpadCelebrities).admin
- if user:
- if not user.inTeam(admins):
- # Enforce privacy-awareness for logged-in, non-admin users,
- # so that they can only see the private bugs that they're
- # allowed to see.
- where_clauses.append("""
- (Bug.private = FALSE OR
- Bug.id in (
- SELECT Bug.id
- FROM Bug, BugSubscription, TeamParticipation
- WHERE Bug.id = BugSubscription.bug AND
- TeamParticipation.person = %(personid)s AND
- BugSubscription.person = TeamParticipation.team))
- """ % sqlvalues(personid=user.id))
+ if user is None:
+ # Anonymous visitors only get to know about public bugs.
+ visible = And(
+ Bug.id == BugBranch.bugID,
+ Bug.private == False)
+ elif user.inTeam(admins):
+ # Administrators know about all bugs.
+ visible = True
else:
- # Anonymous user; filter to include only public bugs in
- # the search results.
- where_clauses.append("Bug.private = FALSE")
-
- return BugBranch.select(
- ' AND '.join(where_clauses), clauseTables=['Bug'])
+ # Anyone else can know about public bugs plus any private
+ # ones they may be directly or indirectly subscribed to.
+ subscribed = And(
+ TeamParticipation.teamID == BugSubscription.person_id,
+ TeamParticipation.personID == user.id)
+
+ visible = And(
+ Bug.id == BugBranch.bugID,
+ Or(
+ Bug.private == False,
+ Bug.id.is_in(Select(
+ columns=[BugSubscription.bug_id],
+ tables=[BugSubscription, TeamParticipation],
+ where=subscribed))))
+
+ return IStore(BugBranch).find(
+ BugBranch.branchID,
+ BugBranch.branch_id.is_in(branch_ids),
+ visible).config(distinct=True)
def getBugBranchesForBugTasks(self, tasks):
"See IBugBranchSet."
=== modified file 'lib/lp/bugs/tests/test_bugbranch.py'
--- lib/lp/bugs/tests/test_bugbranch.py 2010-10-26 15:47:24 +0000
+++ lib/lp/bugs/tests/test_bugbranch.py 2011-02-10 16:13:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010 Canonical Ltd. This software is licensed under the
+# Copyright 2010-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for bug-branch linking from the bugs side."""
@@ -32,46 +32,103 @@
# BugBranchSet objects provide IBugBranchSet.
self.assertProvides(BugBranchSet(), IBugBranchSet)
- def test_getBugBranchesForBranches_no_branches(self):
+ def test_getBranchesWithVisibleBugs_no_branches(self):
bug_branches = getUtility(IBugBranchSet)
- links = bug_branches.getBugBranchesForBranches(
+ links = bug_branches.getBranchesWithVisibleBugs(
[], self.factory.makePerson())
self.assertEqual([], list(links))
- def test_getBugBranchesForBranches(self):
- # IBugBranchSet.getBugBranchesForBranches returns all of the BugBranch
- # objects associated with the given branches.
+ def test_getBranchesWithVisibleBugs_finds_branches_with_public_bugs(self):
+ # IBugBranchSet.getBranchesWithVisibleBugs returns all of the
+ # Branch ids associated with the given branches that have bugs
+ # visible to the current user. Those trivially include ones
+ # for non-private bugs.
branch_1 = self.factory.makeBranch()
branch_2 = self.factory.makeBranch()
bug_a = self.factory.makeBug()
bug_b = self.factory.makeBug()
self.factory.loginAsAnyone()
- link_1 = bug_a.linkBranch(branch_1, self.factory.makePerson())
- link_2 = bug_a.linkBranch(branch_2, self.factory.makePerson())
- link_3 = bug_b.linkBranch(branch_2, self.factory.makePerson())
- self.assertEqual(
- set([link_1, link_2, link_3]),
- set(getUtility(IBugBranchSet).getBugBranchesForBranches(
- [branch_1, branch_2], self.factory.makePerson())))
-
- def test_getBugBranchesForBranches_respects_bug_privacy(self):
- # IBugBranchSet.getBugBranchesForBranches returns only the BugBranch
- # objects that are visible by the user who is asking for them.
- branch = self.factory.makeBranch()
- user = self.factory.makePerson()
- public_bug = self.factory.makeBug()
- private_visible_bug = self.factory.makeBug(private=True)
- private_invisible_bug = self.factory.makeBug(private=True)
- with celebrity_logged_in('admin'):
- public_bug.linkBranch(branch, user)
- private_visible_bug.subscribe(user, user)
- private_visible_bug.linkBranch(branch, user)
- private_invisible_bug.linkBranch(branch, user)
- bug_branches = getUtility(IBugBranchSet).getBugBranchesForBranches(
- [branch], user)
- self.assertEqual(
- set([public_bug, private_visible_bug]),
- set([link.bug for link in bug_branches]))
+ bug_a.linkBranch(branch_1, self.factory.makePerson())
+ bug_a.linkBranch(branch_2, self.factory.makePerson())
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [branch_1.id, branch_2.id],
+ utility.getBranchesWithVisibleBugs(
+ [branch_1, branch_2], self.factory.makePerson()))
+
+ def test_getBranchesWithVisibleBugs_shows_public_bugs_to_anon(self):
+ # getBranchesWithVisibleBugs shows public bugs to anyone,
+ # including anonymous users.
+ branch = self.factory.makeBranch()
+ bug = self.factory.makeBug(private=False)
+ with celebrity_logged_in('admin'):
+ bug.linkBranch(branch, self.factory.makePerson())
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [branch.id], utility.getBranchesWithVisibleBugs([branch], None))
+
+ def test_getBranchesWithVisibleBugs_ignores_duplicate_bugbranches(self):
+ # getBranchesWithVisibleBugs reports a branch only once even if
+ # it's linked to the same bug multiple times.
+ branch = self.factory.makeBranch()
+ user = self.factory.makePerson()
+ bug = self.factory.makeBug()
+ self.factory.loginAsAnyone()
+ bug.linkBranch(branch, user)
+ bug.linkBranch(branch, user)
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
+
+ def test_getBranchesWithVisibleBugs_ignores_extra_bugs(self):
+ # getBranchesWithVisibleBugs reports a branch only once even if
+ # it's liked to multiple bugs.
+ branch = self.factory.makeBranch()
+ user = self.factory.makePerson()
+ with celebrity_logged_in('admin'):
+ self.factory.makeBug().linkBranch(branch, user)
+ self.factory.makeBug().linkBranch(branch, user)
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
+
+ def test_getBranchesWithVisibleBugs_hides_private_bugs_from_anon(self):
+ # getBranchesWithVisibleBugs does not show private bugs to users
+ # who aren't logged in.
+ branch = self.factory.makeBranch()
+ bug = self.factory.makeBug(private=True)
+ with celebrity_logged_in('admin'):
+ bug.linkBranch(branch, self.factory.makePerson())
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [], utility.getBranchesWithVisibleBugs([branch], None))
+
+ def test_getBranchesWithVisibleBugs_hides_private_bugs_from_joe(self):
+ # getBranchesWithVisibleBugs does not show private bugs to
+ # arbitrary logged-in users (such as Average Joe, or J. Random
+ # Hacker).
+ branch = self.factory.makeBranch()
+ bug = self.factory.makeBug(private=True)
+ with celebrity_logged_in('admin'):
+ bug.linkBranch(branch, self.factory.makePerson())
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [],
+ utility.getBranchesWithVisibleBugs(
+ [branch], self.factory.makePerson()))
+
+ def test_getBranchesWithVisibleBugs_shows_private_bugs_to_sub(self):
+ # getBranchesWithVisibleBugs will show private bugs to their
+ # subscribers.
+ branch = self.factory.makeBranch()
+ bug = self.factory.makeBug(private=True)
+ user = self.factory.makePerson()
+ with celebrity_logged_in('admin'):
+ bug.subscribe(user, self.factory.makePerson())
+ bug.linkBranch(branch, self.factory.makePerson())
+ utility = getUtility(IBugBranchSet)
+ self.assertContentEqual(
+ [branch.id], utility.getBranchesWithVisibleBugs([branch], user))
def test_getBugBranchesForBugTasks(self):
# IBugBranchSet.getBugBranchesForBugTasks returns all of the BugBranch
=== modified file 'lib/lp/code/browser/branchlisting.py'
--- lib/lp/code/browser/branchlisting.py 2011-02-02 15:43:31 +0000
+++ lib/lp/code/browser/branchlisting.py 2011-02-10 16:13:03 +0000
@@ -439,9 +439,8 @@
@cachedproperty
def branch_ids_with_bug_links(self):
"""Return a set of branch ids that should show bug badges."""
- bug_branches = getUtility(IBugBranchSet).getBugBranchesForBranches(
- self.visible_branches_for_view, self.view_user)
- return set(bug_branch.branch.id for bug_branch in bug_branches)
+ return set(getUtility(IBugBranchSet).getBranchesWithVisibleBugs(
+ self.visible_branches_for_view, self.view_user))
@cachedproperty
def branch_ids_with_spec_links(self):
@@ -466,17 +465,25 @@
@cachedproperty
def tip_revisions(self):
"""Return a set of branch ids that should show blueprint badges."""
- revisions = getUtility(IRevisionSet).getTipRevisionsForBranches(
+ revisionset = getUtility(IRevisionSet)
+ revisions = revisionset.getTipRevisionsForBranches(
self.visible_branches_for_view)
if revisions is None:
- revision_map = {}
- else:
- # Key the revisions by revision id.
- revision_map = dict((revision.revision_id, revision)
- for revision in revisions)
+ revisions = []
+
+ # Key the revisions by revision id.
+ revision_map = dict(
+ (revision.revision_id, revision) for revision in revisions)
+
+ # Cache display information for authors of branches' respective
+ # last revisions.
+ revisionset.fetchAuthorsForDisplay(
+ [revision.revision_author for revision in revisions])
+
# Return a dict keyed on branch id.
- return dict((branch.id, revision_map.get(branch.last_scanned_id))
- for branch in self.visible_branches_for_view)
+ return dict(
+ (branch.id, revision_map.get(branch.last_scanned_id))
+ for branch in self.visible_branches_for_view)
def _createItem(self, branch):
last_commit = self.tip_revisions[branch.id]
=== added file 'lib/lp/code/browser/tests/test_revisionauthor.py'
--- lib/lp/code/browser/tests/test_revisionauthor.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/browser/tests/test_revisionauthor.py 2011-02-10 16:13:03 +0000
@@ -0,0 +1,82 @@
+# Copyright 2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests related to `RevisionAuthor`."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.app.browser.tales import (
+ PersonFormatterAPI,
+ RevisionAuthorFormatterAPI,
+ )
+from lp.testing import (
+ login,
+ TestCaseWithFactory,
+ )
+from lp.testing.sampledata import USER_EMAIL
+
+
+class TestRevisionAuthorFormatterAPI(TestCaseWithFactory):
+ """Test `RevisionAuthor` link formatter."""
+
+ layer = DatabaseFunctionalLayer
+
+ def _formatAuthorLink(self, revision):
+ """Format a link to `revision`'s author."""
+ return RevisionAuthorFormatterAPI(revision.revision_author).link()
+
+ def test_link_links_to_person_if_known(self):
+ # When a RevisionAuthor is coupled to a Person, the link
+ # formatter simply links to the Person.
+ author = self.factory.makePerson()
+ revision = self.factory.makeRevision(author=author)
+ self.assertEqual(
+ PersonFormatterAPI(author).link(None),
+ self._formatAuthorLink(revision))
+
+ def test_link_shows_name(self):
+ # When a RevisionAuthor is not coupled to a Person but does have
+ # a name attached to it, the link formatter shows the name.
+ revision = self.factory.makeRevision(author="J.R. Hacker")
+ self.assertEqual("J.R. Hacker", self._formatAuthorLink(revision))
+
+ def test_link_shows_email_if_necessary(self):
+ # If nothing else is available, the author link will show the
+ # author's email address.
+ login(USER_EMAIL)
+ email = "%s@xxxxxxxxxxx" % self.factory.getUniqueString()
+ revision = self.factory.makeRevision(author=email)
+ self.assertEqual(email, self._formatAuthorLink(revision))
+
+ def test_link_name_trumps_email(self):
+ # If both a name and an email address are available, the email
+ # address is not shown.
+ name = self.factory.getUniqueString()
+ email = "%s@xxxxxxxxxxx" % self.factory.getUniqueString()
+ full_email = "%s <%s>" % (name, email)
+ revision = self.factory.makeRevision(author=full_email)
+ self.assertEqual(name, self._formatAuthorLink(revision))
+
+ def test_email_is_never_shown_to_anonymous_users(self):
+ # Even if only an email address is available, it will not be
+ # shown to anonymous users.
+ account = self.factory.getUniqueString()
+ email = "%s@xxxxxxxxxxx" % account
+ revision = self.factory.makeRevision(author=email)
+ self.assertNotIn(account, self._formatAuthorLink(revision))
+ self.assertNotIn('@', self._formatAuthorLink(revision))
+
+ def test_name_is_escaped(self):
+ # The author's name is HTML-escaped.
+ revision = self.factory.makeRevision(author="apples & pears")
+ self.assertEqual(
+ "apples & pears", self._formatAuthorLink(revision))
+
+ def test_email_is_escaped(self):
+ # The author's email address is HTML-escaped.
+ login(USER_EMAIL)
+ raw_email = "a&b@xxxxxxxxxxx"
+ escaped_email = "a&b@xxxxxxxxxxx"
+ revision = self.factory.makeRevision(author=raw_email)
+ self.assertEqual(escaped_email, self._formatAuthorLink(revision))
=== modified file 'lib/lp/code/interfaces/revision.py'
--- lib/lp/code/interfaces/revision.py 2010-10-12 05:34:49 +0000
+++ lib/lp/code/interfaces/revision.py 2011-02-10 16:13:03 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0211,E0213
@@ -34,7 +34,10 @@
date_created = Datetime(
title=_("Date Created"), required=True, readonly=True)
log_body = Attribute("The revision log message.")
+
+ revision_author_id = Attribute("Revision author identifier id.")
revision_author = Attribute("The revision author identifier.")
+
gpgkey = Attribute("The OpenPGP key used to sign the revision.")
revision_id = Attribute("The globally unique revision identifier.")
revision_date = Datetime(
@@ -224,3 +227,13 @@
:param limit: Remove at most `limit` rows at once.
"""
+
+ def fetchAuthorsForDisplay(revision_authors):
+ """Retrieve `Person`s associated with a bunch of `RevisionAuthor`s.
+
+ Also prefetches information that will be needed in order to show
+ the authors in the user interface.
+
+ :param revision_authors: A sequence of `IRevisionAuthor`s.
+ :return: A list of `Person`s.
+ """
=== modified file 'lib/lp/code/model/revision.py'
--- lib/lp/code/model/revision.py 2010-10-12 05:34:49 +0000
+++ lib/lp/code/model/revision.py 2011-02-10 16:13:03 +0000
@@ -33,6 +33,7 @@
Asc,
Desc,
Join,
+ LeftJoin,
Not,
Or,
Select,
@@ -54,6 +55,10 @@
UTC_NOW,
)
from canonical.database.datetimecol import UtcDateTimeCol
+from canonical.launchpad.database.librarian import (
+ LibraryFileAlias,
+ LibraryFileContent,
+ )
from canonical.database.sqlbase import (
quote,
SQLBase,
@@ -81,6 +86,10 @@
from lp.registry.interfaces.person import validate_public_person
from lp.registry.interfaces.product import IProduct
from lp.registry.interfaces.projectgroup import IProjectGroup
+from lp.registry.model.person import (
+ Person,
+ ValidPersonCache,
+ )
class Revision(SQLBase):
@@ -92,8 +101,9 @@
log_body = StringCol(notNull=True)
gpgkey = ForeignKey(dbName='gpgkey', foreignKey='GPGKey', default=None)
- revision_author = ForeignKey(
- dbName='revision_author', foreignKey='RevisionAuthor', notNull=True)
+ revision_author_id = Int(name='revision_author', allow_none=False)
+ revision_author = Reference(revision_author_id, 'RevisionAuthor.id')
+
revision_id = StringCol(notNull=True, alternateID=True,
alternateMethodName='byRevisionID')
revision_date = UtcDateTimeCol(notNull=False)
@@ -430,7 +440,6 @@
# Here to stop circular imports.
from lp.code.model.branch import Branch
from lp.code.model.branchrevision import BranchRevision
- from lp.registry.model.person import ValidPersonCache
store = IStore(Revision)
results_with_dupes = store.find(
@@ -591,6 +600,31 @@
limit=limit)
store.find(RevisionCache, RevisionCache.id.is_in(subquery)).remove()
+ @staticmethod
+ def fetchAuthorsForDisplay(revision_authors):
+ """See `IRevisionSet`."""
+ person_ids = set([
+ revision_author.personID
+ for revision_author in revision_authors])
+ person_ids.discard(None)
+ if len(person_ids) == 0:
+ return []
+ store = IStore(RevisionAuthor)
+ source = store.using(
+ Person,
+ LeftJoin(LibraryFileAlias, LibraryFileAlias.id == Person.iconID),
+ LeftJoin(
+ LibraryFileContent,
+ LibraryFileContent.id == LibraryFileAlias.contentID))
+ columns = (
+ Person,
+ LibraryFileAlias,
+ LibraryFileContent,
+ )
+ prejoined_authors = source.find(
+ columns, Person.id.is_in(person_ids))
+ return [person for person, alias, content in prejoined_authors]
+
def revision_time_limit(day_limit):
"""The storm fragment to limit the revision_date field of the Revision."""
=== modified file 'lib/lp/code/model/tests/test_revision.py'
--- lib/lp/code/model/tests/test_revision.py 2010-12-20 21:46:45 +0000
+++ lib/lp/code/model/tests/test_revision.py 2011-02-10 16:13:03 +0000
@@ -262,6 +262,34 @@
found = self.revision_set.getByRevisionId('nonexistent')
self.assertIs(None, found)
+ def test_fetchAuthorsForDisplay_finds_nothing_for_empty_list(self):
+ self.assertContentEqual(
+ [], self.revision_set.fetchAuthorsForDisplay([]))
+
+ def test_fetchAuthorsForDisplay_finds_revision_author(self):
+ author = self.factory.makePerson()
+ result = self.revision_set.fetchAuthorsForDisplay(
+ [self.factory.makeRevision(author=author).revision_author])
+ self.assertEqual([author], result)
+
+ def test_fetchAuthorsForDisplay_does_not_repeat_authors(self):
+ author = self.factory.makePerson()
+ revision_authors = [
+ self.factory.makeRevision(author=author).revision_author
+ for counter in xrange(3)]
+ result = self.revision_set.fetchAuthorsForDisplay(revision_authors)
+ self.assertEqual([author], result)
+
+ def test_fetchAuthorsForDisplay_skips_None(self):
+ revision = self.factory.makeRevision(author="A. Nonnimers")
+ self.assertIs(None, revision.revision_author.person)
+ result = self.revision_set.fetchAuthorsForDisplay(
+ [revision.revision_author])
+ # A revision_author record doesn't have to have a Person
+ # attached. If it doesn't, fetchAuthorsForDisplay effectively
+ # ignores it.
+ self.assertEqual([], result)
+
class TestRevisionGetBranch(TestCaseWithFactory):
"""Test the `getBranch` method of the revision."""
@@ -445,7 +473,7 @@
rev2 = self._makeRevision(
revision_date=(now - timedelta(days=2)))
self._addRevisionsToBranch(self._makeBranch(), rev1, rev2)
- self.assertEqual([rev2], self._getRevisions(day_limit))
+ self.assertEqual([rev2], self._getRevisions(day_limit))
class TestGetPublicRevisionsForPerson(GetPublicRevisionsTestCase,
=== modified file 'lib/lp/code/templates/branch-listing.pt'
--- lib/lp/code/templates/branch-listing.pt 2010-09-28 20:46:26 +0000
+++ lib/lp/code/templates/branch-listing.pt 2011-02-10 16:13:03 +0000
@@ -160,36 +160,19 @@
<tal:revision-log replace="branch/revision_log/fmt:shorten/40"/>
</div>
<div class="popupTitle"
- tal:attributes="id string:branch-log-${branch/id};
- onmouseover string:hide_commit(${branch/id});">
- <p><strong>Author:</strong>
- <tal:known-person condition="branch/revision_author/person">
- <tal:person-link
- replace="structure branch/revision_author/person/fmt:link"/>
- </tal:known-person>
- <tal:unknown-person
- condition="not: branch/revision_author/person">
- <tal:person-text
- tal:replace="branch/revision_author/name_without_email">
- John Doe
- </tal:person-text>
- <tal:person-only-email
- condition="not: branch/revision_author/name_without_email">
- <tal:not-anonymous condition="context/view/user">
- <tal:person-text
- tal:replace="branch/revision_author/email">
- doe.john@xxxxxxxxxxx
- </tal:person-text>
- </tal:not-anonymous>
- <tal:anonymous condition="not:context/view/user">
- <email address hidden>
- </tal:anonymous>
- </tal:person-only-email>
- </tal:unknown-person>
- <br/>
- <strong>Revision Date:</strong>
- <tal:revision-date replace="branch/revision_date/fmt:datetime"/></p>
- <tal:commit-msg replace="structure branch/revision_log/fmt:text-to-html"/>
+ tal:attributes="id string:branch-log-${branch/id};
+ onmouseover string:hide_commit(${branch/id});">
+ <p>
+ <strong>Author:</strong>
+ <tal:author
+ replace="structure branch/revision_author/fmt:link" />
+ <br/>
+ <strong>Revision Date:</strong>
+ <tal:revision-date
+ replace="branch/revision_date/fmt:datetime"/>
+ </p>
+ <tal:commit-msg
+ replace="structure branch/revision_log/fmt:text-to-html"/>
</div>
</td>
Follow ups
