Merge lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad

[j.c.sackett]

Attacks the +participation problem by stormifying the team_memberships method (previously myactivememberships) and using a graph traversal to determine team paths rather than multiple look ups using findPathToTeam.

Summary

Contributes to fixing the +participation time outs by converting team path look ups into one large SQL lookup followed by some graph traversal.

Previously the mp showed an implementation that largely cut-off the team_path lookup; the changes for it are still useful, so have not been removed from the branch, but the solution changed in the last few revisions.

Proposed fix

Stormifying where possible to assist db load; more importantly, using a graph of all teams the person participates in to create the team_paths for indirect teams as displayed.

Pre-implementation notes

Chatted with Curtis Hovey (sinzui)

Got schooled by Robert Collins (lifeless) in the ways of the graph, and agreed it was a much better solution.

Implementation details

Renames myactivememberships to team_memberships, per an XXX from kiko in the codebase. Updates call sites.

Stormifies team_memberships and updates callsites.

Uses a graph traversal in getPathsToTeams to find the path between the person and all teams, then filters out direct memberships from that to get indirect teams and their team_paths.

Demo and Q/A
bin/test -vvc -t TestPersonTeams
bin/test -vvc -t TestPersonParticipationView

On launchpad.dev, checkout https://launchpad.dev/~mark/+participation. The via column should show the team_path for indirect teams. I haven't found anyone with longer team_paths than one indirection in the sample data, but if you know of one you should see the full team_path.

lint
= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt
  lib/lp/registry/vocabularies.py
  lib/lp/registry/browser/person.py
  lib/lp/registry/browser/tests/test_person_view.py
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/model/person.py
  lib/lp/registry/tests/test_person.py

./lib/lp/registry/vocabularies.py
     180: E302 expected 2 blank lines, found 1
     230: E302 expected 2 blank lines, found 1
     625: E302 expected 2 blank lines, found 1
    1495: E301 expected 1 blank line, found 0
./lib/lp/registry/interfaces/person.py
     448: E302 expected 2 blank lines, found 1

The "expected n blank lines" were explained to me as a factor of lint having issues around comments and class definitions.

[Robert Collins (lifeless)]

Could you explain why the indirect team path calculations were a problem?

[j.c.sackett (jcsackett)]

The findPathToTeam calculations were a problem b/c in the cases causing the timeout, the query gets called hundreds of times, leading to the largest set of time in the process. Because of the iteration through an unknown series of team relations, there's no easy way to predict how this will work.

As an example, in https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1688ED2377 you can see the query used in findPathToTeam being called 261 times, for a total time of 4610. The other +participation OOPS reports show the same thing.

[Robert Collins (lifeless)]

On Wed, Aug 18, 2010 at 1:01 AM, j.c.sackett
<email address hidden> wrote:
> The findPathToTeam calculations were a problem b/c in the cases causing the timeout, the query gets called hundreds of times, leading to the largest set of time in the process. Because of the iteration through an unknown series of team relations, there's no easy way to predict how this will work.
>
> As an example, in https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1688ED2377 you can see the query used in findPathToTeam being called 261 times, for a total time of 4610. The other +participation OOPS reports show the same thing.

Ok, so theres basic reason for it to be slow.

Have you considered this algorithm (psuedo code):
# 1 query, we want all the teams he participates in
# (that is, all rows that have the member == user, and the team == anything
# should return a list of tuples [(user, t1), (user, t2)] or something like that
memberships = list(get_memberships(person==user))
teams = [team for _, team in memberships]
# Get all the memberships of those teams themselves; because memberships
# are transitive, this will be a subset of the first result, and lets us infer
# a valid graph cheaply (there can be multiple paths to a team, we only
# need to show one)
team_temberships = get_memberships(person in teams)
graph_edges = list(team_memberships)
graph = dict(graph_edges)
graph.update(memberships)
# now render
...

[Robert Collins (lifeless)]

Bah, I got the direct and indirect person memberships the wrong way around.

Have you considered this algorithm (psuedo code):
# 1 query, we want all the teams he participates via the expanded memo:
# (that is, all rows that have the member == user, and the team == anything
# that is directly or indirectly in. This should return a list of tuples
# [(user, t1), (user, t2)] or something like that
memberships = list(get_expanded_memberships(Class.person==user))
known_teams = [team for _, team in memberships]
known_members = known_teams + [user]
# Get all the memberships of those teams themselves; because memberships
# are transitive, we can query for the entire graph directly.
# Note that here we query direct memberships only
team_temberships = get_direct_memberships(Class.person in known_members)
graph = dict(team_memberships)
# now render
...

[Māris Fogels (mars)]

Hi Jonathan,

I reviewed the branch and both the test and implementation of the changes look good. However, I did come upon one large question: did the IPerson.findPathToTeam() interface definition change with this patch, or does this patch only change the Person class itself? If so, then the IPerson interface class will have to be updated. If you want to verify this, I suggest writing a test in test_person.py that calls verifyObject() on a Person instance - that will immediately reveal any interface errors.

Besides that, I only found minor points in the diff:

 • Does the new 'via' argument to _asParticipation() need a epydoc stanza in the method's docstring? Otherwise the argument type is not obvious.

 • Should there be a comment on the line 53 of the diff, before calling findPathToTeam(limit=1), to make the purpose of the limit argument clear? Without that 'limit=1' appears to be a magic number.

 • It looks like the 'from storm.expr' import in model/person.py was ordered alphabetically, but Desc isn't in order.

 • You probably want to use an epydoc :param: stanza for the new limit argument in the IPerson.findPathToTeam() docstring.

I am marking this as Needs Information for now. When the IPerson interface questions are answered then we can approve this to land.

Maris

review: Needs Information

[Māris Fogels (mars)]

Looking at the test commands, I see that only the TestPersonTeams and TestPersonParticipationView cases are run. It is possible that 'bin/test -t test_person' will have a test that calls verifyObject().

[j.c.sackett (jcsackett)]

Maris--

You were right, I had borked the interface. I've since updated the interface to show the new parameters.

Per suggestion from Robert, a lot has changed in this code; I'm unsure if you want to continue this MP or kill it and have me submit another. All the code reviewed is still there, as the changes were still good ones, but many of the changes are no longer taken advantage of by person/+participation, in favor of Robert's suggestion.

[j.c.sackett (jcsackett)]

Robert--

I've pushed up changes that I believe implement your idea. Rather than picking the shortest path though, it uses the oldest path to maintain consistency with the data previously presented (save of course those occasions when the shortest path is a direct_membership).

Take a look and let me know what you think.

[Robert Collins (lifeless)]

129 """Validate the person is a real person with no other restrictions."""
130 +
131 def validate(person):

That new vertical whitespace is non-PEP8 - this is a curried function, so its appropriate to have it adjacent and not break the eye's flow reading past it - please don't do that. If your editor is telling you to do it, give it a good spanking.

You also have some list reflows that make things less readable - our style guide encourages readability over sheer compactness; I'm on the fence myself, but wanted you to be aware that you have discretion there - what the code had was fine, with the lines and indentation clearly grouping the query clauses.

The implementation of a 2-query look up looks pretty nice, I'm glad it turned out well. I can't really judge the ui result trivially, perhaps a screenshot of a complexish situation?

I haven't done a full code review, I think you should carry on with Maris or whomever is reviewer-de-jour.

[j.c.sackett (jcsackett)]

Robert--

Thanks, I'll look into the reflows and that line break.

I agree on pursuing the review with Maris or whomever; I was interested in your view of my implementation of the two query graph thing. Really great idea, btw.

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

The main thing I learned from reviewing this branch is that we should
not give new employees programming tasks concerning graph traversal
unless we want to scare them away. I think that is the most confusing
code I've ever seen in the registry.

I am impressed with all that you have accomplished in this branch. Most
of my comments are related to formatting, which is to be expected with
new employees. I felt your pain.

You really need to run "make lint" against the branch, especially
test_person.py.

-Edwin

>=== modified file 'lib/lp/registry/browser/person.py'
>--- lib/lp/registry/browser/person.py 2010-08-18 03:12:35 +0000
>+++ lib/lp/registry/browser/person.py 2010-08-18 21:23:34 +0000
>@@ -3078,31 +3078,34 @@
> def label(self):
> return 'Team participation for ' + self.context.displayname
>
>- def _asParticipation(self, membership=None, team=None, team_path=None):
>+ def _asParticipation(self, membership=None, team=None, via=None):
> """Return a dict of participation information for the membership.
>
> Method requires membership or team, not both.
>+ :param via: The team through which the membership in the indirect
>+ is established.

"in the indirect" seems to be missing a noun.

> """
> if ((membership is None and team is None) or
> (membership is not None and team is not None)):
> raise AssertionError(
> "The membership or team argument must be provided, not both.")
>
>- if team_path is None:
>- via = None
>- else:
>- via = COMMASPACE.join(team.displayname for team in team_path)
>+ if via is not None:
>+ # When showing the path, it's unnecessary to show the team in
>+ # question at the beginning of the path, or the user at the
>+ # end of the path.
>+ via = COMMASPACE.join(
>+ [via_team.displayname for via_team in via[1:-1]])
>
> if membership is None:
>- #we have membership via an indirect team, and can use
>- #sane defaults
>+ # Membership is via an indirect team; sane defaults exist.
>
>- #the user can only be a member of this team
>+ # The user can only be a member of this team.

The space between these two comments makes it look like a line of
code disappeared. "sane defaults exist" kinda confuses me. I see what
you are going for with a comment for the block and a separate comment
for the first statement. It also took a second to understand what "can
only be a member" meant. Maybe, the two comments can be combined as:
  # An indirect member cannot be the Owner or Admin of the team.

> role = 'Member'
>- #the user never joined, and can't have a join date
>+ # The user never joined, and can't have a join date.

"user" almost always refers to the logged-in user, so it is better
to say "person" if the function is not dealing with the logged-in user.

> datejoined = None
> else:
>- #the member is a direct member, so we can use membership data
>+ # The member is a direct memb...

review: Needs Fixing

[j.c.sackett (jcsackett)]

Edwin--

Thanks for all the comments; I'll make those changes and push them up soon.

On Aug 18, 2010, at 9:48 PM, Edwin Grubbs wrote:

> Review: Needs Fixing
> Hi JC,
>
> The main thing I learned from reviewing this branch is that we should
> not give new employees programming tasks concerning graph traversal
> unless we want to scare them away. I think that is the most confusing
> code I've ever seen in the registry.
>
> I am impressed with all that you have accomplished in this branch. Most
> of my comments are related to formatting, which is to be expected with
> new employees. I felt your pain.
>
> You really need to run "make lint" against the branch, especially
> test_person.py.
>
>
> -Edwin
>
>
>> === modified file 'lib/lp/registry/browser/person.py'
>> --- lib/lp/registry/browser/person.py 2010-08-18 03:12:35 +0000
>> +++ lib/lp/registry/browser/person.py 2010-08-18 21:23:34 +0000
>> @@ -3078,31 +3078,34 @@
>> def label(self):
>> return 'Team participation for ' + self.context.displayname
>>
>> - def _asParticipation(self, membership=None, team=None, team_path=None):
>> + def _asParticipation(self, membership=None, team=None, via=None):
>> """Return a dict of participation information for the membership.
>>
>> Method requires membership or team, not both.
>> + :param via: The team through which the membership in the indirect
>> + is established.
>
>
> "in the indirect" seems to be missing a noun.
>
>
>> """
>> if ((membership is None and team is None) or
>> (membership is not None and team is not None)):
>> raise AssertionError(
>> "The membership or team argument must be provided, not both.")
>>
>> - if team_path is None:
>> - via = None
>> - else:
>> - via = COMMASPACE.join(team.displayname for team in team_path)
>> + if via is not None:
>> + # When showing the path, it's unnecessary to show the team in
>> + # question at the beginning of the path, or the user at the
>> + # end of the path.
>> + via = COMMASPACE.join(
>> + [via_team.displayname for via_team in via[1:-1]])
>>
>> if membership is None:
>> - #we have membership via an indirect team, and can use
>> - #sane defaults
>> + # Membership is via an indirect team; sane defaults exist.
>>
>> - #the user can only be a member of this team
>> + # The user can only be a member of this team.
>
>
>
> The space between these two comments makes it look like a line of
> code disappeared. "sane defaults exist" kinda confuses me. I see what
> you are going for with a comment for the block and a separate comment
> for the first statement. It also took a second to understand what "can
> only be a member" meant. Maybe, the two comments can be combined as:
> # An indirect member cannot be the Owner or Admin of the team.
>
>
>> role = 'Member'
>> - #the user never joined, and can't have a join date
>> + # The user never joined, and can't have a join date.
>
>
> "user" almost always ...

[j.c.sackett (jcsackett)]

More detailed reply with updated code...

> I am impressed with all that you have accomplished in this branch. Most
> of my comments are related to formatting, which is to be expected with
> new employees. I felt your pain.

Actually, once I ran through some experiments in a terminal, it wasn't *that* painful. But thanks. :-)

> You really need to run "make lint" against the branch, especially
> test_person.py.

I thought I had, but I think I ran make lint before committing some files, and so only got the issues with those files.

> The style guide doesn't explicitely mention this, but I think the
> formatting choices should be similar to a function call. Each line of
> the list comprehension should line up so they are easier to read:
> Either:
> result = [foo
> bar
> baz]
> Or:
> result = [
> foo
> bar
> baz]
>
> For actual lists, the ending square bracket has to be on a line by
> itself. There isn't an exact rule for list comprehensions, but
> pocketlint will complain if you put the square bracket on a line by
> itself without a comma on the previous line, and of course, that only
> works for lists.

I've broken up the list comprehension as

indirect_teams = [
            team for team in paths.keys() if
            team not in direct_teams]

It seems clean to break the conditional filter onto its own line.

> I don't see the limit being used anywhere? Is that feature still needed?
>
> If it's needed the docstring should describe what limit is. Look in other
> docstrings, which use the convention:
> :param PARAMNAME: DESCRIPTION

The limit isn't being used anywhere, but I had left it in b/c I could see use of this method
being a problem for someone else down the road. However, I can see it's presence now
is just confusing, and it's a sufficiently obvious solution that if it's actually needed down
the road it can be redone then.

>
>
>> # This is our guarantee that _getDirectMemberIParticipateIn() will
>> # never return None
>> assert self.hasParticipationEntryFor(team), (
>> @@ -1687,19 +1699,16 @@
>> self.invited_members,
>> orderBy=self._sortingColumnsForSetOperations)
>>
>> - # XXX: kiko 2005-10-07:
>> - # myactivememberships should be renamed to team_memberships and be
>> - # described as the set of memberships for the object's teams.
>> @property
>> - def myactivememberships(self):
>> + def team_memberships(self):
>> """See `IPerson`."""
>> - return TeamMembership.select("""
>> - TeamMembership.person = %s AND status in %s AND
>> - Person.id = TeamMembership.team
>> - """ % sqlvalues(self.id, [TeamMembershipStatus.APPROVED,
>> - TeamMembershipStatus.ADMIN]),
>> - clauseTables=['Person'],
>> - orderBy=Person.sortingColumns)
>> + Team = ClassAlias(Person, "Team")
>> + store = Store.of(self)
>> + return store.find(TeamMembership,
>> + And(TeamMembership.personID == self.id,
>> + Team.id == TeamMembership.teamID,
>> + TeamMembership.status.is_in(
>> + ...

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

I just have a few comments more before it's ready to land.

-Edwin

> > The style guide doesn't explicitely mention this, but I think the
> > formatting choices should be similar to a function call. Each line of
> > the list comprehension should line up so they are easier to read:
> > Either:
> > result = [foo
> > bar
> > baz]
> > Or:
> > result = [
> > foo
> > bar
> > baz]
> >
> > For actual lists, the ending square bracket has to be on a line by
> > itself. There isn't an exact rule for list comprehensions, but
> > pocketlint will complain if you put the square bracket on a line by
> > itself without a comma on the previous line, and of course, that only
> > works for lists.
>
> I've broken up the list comprehension as
>
> indirect_teams = [
> team for team in paths.keys() if
> team not in direct_teams]
>
> It seems clean to break the conditional filter onto its own line.

When scanning code, a person often doesn't notice the very end of the line. Here is an extreme example to illustrate my point:
   result = [
       some_big_complicated_function(a) for a in alist if
       a in blist]
The user might think you are iterating over blist instead of alist. While your list comprehension isn't nearly as hazardous, I think putting the "if" at the start of the line will help readers parse it quickly.

> > I don't see the limit being used anywhere? Is that feature still needed?
> >
> > If it's needed the docstring should describe what limit is. Look in other
> > docstrings, which use the convention:
> > :param PARAMNAME: DESCRIPTION
>
> The limit isn't being used anywhere, but I had left it in b/c I could see use
> of this method
> being a problem for someone else down the road. However, I can see it's
> presence now
> is just confusing, and it's a sufficiently obvious solution that if it's
> actually needed down
> the road it can be redone then.
>
>
> >
> >
> >> # This is our guarantee that _getDirectMemberIParticipateIn() will
> >> # never return None
> >> assert self.hasParticipationEntryFor(team), (
> >> @@ -1687,19 +1699,16 @@
> >> self.invited_members,
> >> orderBy=self._sortingColumnsForSetOperations)
> >>
> >> - # XXX: kiko 2005-10-07:
> >> - # myactivememberships should be renamed to team_memberships and be
> >> - # described as the set of memberships for the object's teams.
> >> @property
> >> - def myactivememberships(self):
> >> + def team_memberships(self):
> >> """See `IPerson`."""
> >> - return TeamMembership.select("""
> >> - TeamMembership.person = %s AND status in %s AND
> >> - Person.id = TeamMembership.team
> >> - """ % sqlvalues(self.id, [TeamMembershipStatus.APPROVED,
> >> - TeamMembershipStatus.ADMIN]),
> >> - clauseTables=['Person'],
> >> - orderBy=Person.sortingColumns)
> >> + Team = ClassAlias(Person, "Team")
> >> + store = Store.of(self)
> >> + return store.find(TeamMembership,
> >> + And(TeamMembership.personID == self.id,
> >> + ...

[Edwin Grubbs (edwin-grubbs)]

Hi JC,

This looks good. BTW, it is customary to add a comment with the incremental diff of changes. This way the reviewer gets all the changes they need to look at in an email. I've included them below in case anybody else is following the progress on this.

-Edwin

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2010-08-19 14:52:19 +0000
+++ lib/lp/registry/browser/person.py 2010-08-19 17:39:30 +0000
@@ -3101,7 +3101,7 @@
             # Membership is via an indirect team so sane defaults exist.
             # An indirect member cannot be an Owner or Admin of a team.
             role = 'Member'
- # The Person joined, and can't have a join date.
+ # The Person never joined, and can't have a join date.
             datejoined = None
         else:
             # The member is a direct member; use the membership data.
@@ -3133,8 +3133,8 @@
         paths, memberships = self.context.getPathsToTeams()
         direct_teams = [membership.team for membership in memberships]
         indirect_teams = [
- team for team in paths.keys() if
- team not in direct_teams]
+ team for team in paths.keys()
+ if team not in direct_teams]
         participations = []

         # First, create a participation for all direct memberships.

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-08-19 17:29:38 +0000
+++ lib/lp/registry/model/person.py 2010-08-19 17:39:30 +0000
@@ -1712,7 +1712,6 @@
         store = Store.of(self)
         return store.find(TeamMembership,
             And(TeamMembership.personID == self.id,
- Team.id == TeamMembership.teamID,
                 TeamMembership.status.is_in(
                 [TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN])))

@@ -2080,7 +2079,7 @@

         # Get all of the memberships for any of the teams this person is
         # a participant of. This must be ordered by date and id because
- # because the graph the results will create needs to contain
+ # because the graph of the results will create needs to contain
         # the oldest path information to be consistent with results from
         # IPerson.findPathToTeam.
         store = Store.of(self)
@@ -2114,7 +2113,7 @@
                 step = graph[step]
                 path.append(step)
             paths[team] = path
- return (paths, user_memberships)
+ return (paths, user_memberships)

     @property
     def teams_participated_in(self):

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-08-19 14:48:21 +0000
+++ lib/lp/registry/tests/test_person.py 2010-08-19 17:39:30 +0000
@@ -75,7 +75,7 @@
         memberships = [(m.person, m.team) for m in memberships]
         self.assertEqual([(self.user, self.a_team)], memberships)

- def test_path_to_team_no_limit(self):
+ def test_path_to_team(self):
         path_to_a = self.user.findPathToTeam(self.a_team)
         path_to_b = self.user.findPathToTeam(self.b_team)
         path_to_c = self.user.findPathToTeam(self.c_team)
@@ -84,15 +84,6 @@
         ...

[Edwin Grubbs (edwin-grubbs)]

review: Approve (code)

[j.c.sackett (jcsackett)]

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-08-22 03:09:51 +0000
+++ lib/lp/registry/model/person.py 2010-08-22 04:16:05 +0000
@@ -2203,6 +2203,10 @@
         # Get all of the teams this person participates in.
         teams = list(self.teams_participated_in)

+ # For cases where self is a team, we don't need self as a team
+ # participated in.
+ teams = [team for team in teams if team != self]
+
         # Get all of the memberships for any of the teams this person is
         # a participant of. This must be ordered by date and id because
         # because the graph of the results will create needs to contain
@@ -2210,9 +2214,16 @@
         # IPerson.findPathToTeam.
         store = Store.of(self)
         all_direct_memberships = store.find(TeamMembership,
- TeamMembership.personID.is_in(
- [team.id for team in teams] + [self.id])).order_by(
- Desc(TeamMembership.datejoined), Desc(TeamMembership.id))
+ And(
+ TeamMembership.personID.is_in(
+ [team.id for team in teams] + [self.id]),
+ TeamMembership.teamID != self.id,
+ TeamMembership.status.is_in([
+ TeamMembershipStatus.APPROVED,
+ TeamMembershipStatus.ADMIN,
+ ]))).order_by(
+ Desc(TeamMembership.datejoined),
+ Desc(TeamMembership.id))
         # Cast the results to list now, because they will be iterated over
         # several times.
         all_direct_memberships = list(all_direct_memberships)

[Robert Collins (lifeless)]

On Sun, Aug 22, 2010 at 4:46 PM, j.c.sackett
<email address hidden> wrote:
> === modified file 'lib/lp/registry/model/person.py'
> --- lib/lp/registry/model/person.py     2010-08-22 03:09:51 +0000
> +++ lib/lp/registry/model/person.py     2010-08-22 04:16:05 +0000
> @@ -2203,6 +2203,10 @@
>         # Get all of the teams this person participates in.
>         teams = list(self.teams_participated_in)
>
> +        # For cases where self is a team, we don't need self as a team
> +        # participated in.
> +        teams = [team for team in teams if team != self]

This is not idiomatic python. Does the following work?
teams = [team for team in teams if team is not self]

It may not due to security proxies; worth a comment if it doesn't to
avoid cleanups taking someones time.

-Rob

[Jelmer Vernooij (jelmer)]

+1 on the improvements after the ec2 test run.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/doc/sample-data-assertions.txt'
--- lib/canonical/launchpad/doc/sample-data-assertions.txt	2007-02-21 15:55:46 +0000
+++ lib/canonical/launchpad/doc/sample-data-assertions.txt	2010-08-22 17:16:44 +0000
@@ -22,14 +22,14 @@
   This user is not supposed to be a member of any teams.
 
   >>> no_team_memberships = personset.getByName('no-team-memberships')
-  >>> no_team_memberships.myactivememberships.count()
+  >>> no_team_memberships.team_memberships.count()
   0
 
 * One Team Membership
   This user is supposed to be a member of only one team, the "Simple Team".
 
   >>> one_membership = personset.getByName('one-membership')
-  >>> for t in one_membership.myactivememberships: print t.team.displayname
+  >>> for t in one_membership.team_memberships: print t.team.displayname
   Simple Team
 
 

=== modified file 'lib/lp/bugs/doc/initial-bug-contacts.txt'
--- lib/lp/bugs/doc/initial-bug-contacts.txt	2009-08-25 11:21:05 +0000
+++ lib/lp/bugs/doc/initial-bug-contacts.txt	2010-08-22 17:16:44 +0000
@@ -403,7 +403,7 @@
 
     >>> sample_person = view.user
     >>> ["%s: %s" % (membership.team.displayname, membership.status.name)
-    ...  for membership in sample_person.myactivememberships]
+    ...  for membership in sample_person.team_memberships]
     [u'HWDB Team: APPROVED',
      u'Landscape Developers: ADMIN',
      u'Launchpad Users: ADMIN',

=== modified file 'lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt'
--- lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt	2009-07-17 17:59:07 +0000
+++ lib/lp/bugs/templates/person-portlet-team-assignedbugs.pt	2010-08-22 17:16:44 +0000
@@ -5,7 +5,7 @@
   omit-tag="">
 
 <div class="portlet" id="portlet-team-assigned-bugs"
-     tal:condition="context/myactivememberships">
+     tal:condition="context/team_memberships">
 
   <h2><span tal:replace="context/title" />'s teams</h2>
 

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/person.py	2010-08-22 17:16:44 +0000
@@ -3162,31 +3162,33 @@
     def label(self):
         return 'Team participation for ' + self.context.displayname
 
-    def _asParticipation(self, membership=None, team=None, team_path=None):
+    def _asParticipation(self, membership=None, team=None, via=None):
         """Return a dict of participation information for the membership.
 
         Method requires membership or team, not both.
+        :param via: The team through which the membership in the indirect
+        team is established.
         """
         if ((membership is None and team is None) or
             (membership is not None and team is not None)):
             raise AssertionError(
                 "The membership or team argument must be provided, not both.")
 
-        if team_path is None:
-            via = None
-        else:
-            via = COMMASPACE.join(team.displayname for team in team_path)
+        if via is not None:
+            # When showing the path, it's unnecessary to show the team in
+            # question at the beginning of the path, or the user at the
+            # end of the path.
+            via = COMMASPACE.join(
+                [via_team.displayname for via_team in via[1:-1]])
 
         if membership is None:
-            #we have membership via an indirect team, and can use
-            #sane defaults
-
-            #the user can only be a member of this team
+            # Membership is via an indirect team so sane defaults exist.
+            # An indirect member cannot be an Owner or Admin of a team.
             role = 'Member'
-            #the user never joined, and can't have a join date
+            # The Person never joined, and can't have a join date.
             datejoined = None
         else:
-            #the member is a direct member, so we can use membership data
+            # The member is a direct member; use the membership data.
             team = membership.team
             datejoined = membership.datejoined
             if membership.person == team.teamowner:
@@ -3212,18 +3214,29 @@
     @cachedproperty
     def active_participations(self):
         """Return the participation information for active memberships."""
-        participations = [self._asParticipation(membership=membership)
-                for membership in self.context.myactivememberships
-                if check_permission('launchpad.View', membership.team)]
-        membership_set = getUtility(ITeamMembershipSet)
-        for team in self.context.teams_indirectly_participated_in:
-            if not check_permission('launchpad.View', team):
+        paths, memberships = self.context.getPathsToTeams()
+        direct_teams = [membership.team for membership in memberships]
+        indirect_teams = [
+            team for team in paths.keys()
+            if team not in direct_teams]
+        participations = []
+
+        # First, create a participation for all direct memberships.
+        for membership in memberships:
+            # Add a participation record for the membership if allowed.
+            if check_permission('launchpad.View', membership.team):
+                participations.append(
+                    self._asParticipation(membership=membership))
+
+        # Second, create a participation for all indirect memberships,
+        # using the remaining paths.
+        for indirect_team in indirect_teams:
+            if not check_permission('launchpad.View', indirect_team):
                 continue
-            # The key points of the path for presentation are:
-            # [-?] indirect memberships, [-2] direct membership, [-1] team.
-            team_path = self.context.findPathToTeam(team)
             participations.append(
-                self._asParticipation(team_path=team_path[:-1], team=team))
+                self._asParticipation(
+                    via=paths[indirect_team],
+                    team=indirect_team))
         return sorted(participations, key=itemgetter('displayname'))
 
     @cachedproperty

=== modified file 'lib/lp/registry/browser/tests/test_person_view.py'
--- lib/lp/registry/browser/tests/test_person_view.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py	2010-08-22 17:16:44 +0000
@@ -268,7 +268,7 @@
         team = self.factory.makeTeam()
         login_person(team.teamowner)
         team.addMember(self.user, team.teamowner)
-        for membership in self.user.myactivememberships:
+        for membership in self.user.team_memberships:
             membership.setStatus(
                 TeamMembershipStatus.ADMIN, team.teamowner)
         [participation] = self.view.active_participations
@@ -360,7 +360,7 @@
         self.assertEqual(['A', 'B', 'C'], display_names)
         self.assertEqual(None, participations[0]['via'])
         self.assertEqual('A', participations[1]['via'])
-        self.assertEqual('A, B', participations[2]['via'])
+        self.assertEqual('B, A', participations[2]['via'])
 
     def test_has_participations_false(self):
         participations = self.view.active_participations

=== modified file 'lib/lp/registry/doc/person-account.txt'
--- lib/lp/registry/doc/person-account.txt	2010-08-17 13:41:56 +0000
+++ lib/lp/registry/doc/person-account.txt	2010-08-22 17:16:44 +0000
@@ -80,7 +80,7 @@
     >>> spec = Specification.selectFirst("assignee IS NULL", orderBy='id')
     >>> spec.assignee = foobar
 
-    >>> [membership.team.name for membership in foobar.myactivememberships]
+    >>> [membership.team.name for membership in foobar.team_memberships]
     [u'canonical-partner-dev', u'guadamen', u'hwdb-team', u'admins',
      u'launchpad-buildd-admins', u'launchpad', u'testing-spanish-team',
      u'name18', u'ubuntu-team', u'vcs-imports']
@@ -152,7 +152,7 @@
 
 ...to have no team memberships...
 
-    >>> [membership.team.name for membership in foobar.myactivememberships]
+    >>> [membership.team.name for membership in foobar.team_memberships]
     []
 
 ...and no validated/preferred email addresses...

=== modified file 'lib/lp/registry/doc/private-team-visibility.txt'
--- lib/lp/registry/doc/private-team-visibility.txt	2010-08-13 01:43:26 +0000
+++ lib/lp/registry/doc/private-team-visibility.txt	2010-08-22 17:16:44 +0000
@@ -40,7 +40,7 @@
     >>> commercial_admin = login_celebrity('commercial_admin')
     >>> check_permission('launchpad.View', priv_team)
     True
-    >>> team_membership = priv_member.myactivememberships[0]
+    >>> team_membership = priv_member.team_memberships[0]
     >>> check_permission('launchpad.View', team_membership)
     True
 

=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt	2010-08-17 11:50:43 +0000
+++ lib/lp/registry/doc/teammembership.txt	2010-08-22 17:16:44 +0000
@@ -427,7 +427,7 @@
     # Foo Bar is a launchpad admin, but even so he can't change a membership's
     # status/expiry-date by hand.
     >>> login_person(foobar)
-    >>> membership = foobar.myactivememberships[0]
+    >>> membership = foobar.team_memberships[0]
     >>> membership.status = None
     Traceback (most recent call last):
     ...
@@ -638,13 +638,13 @@
     >>> autorenewal_team.defaultrenewalperiod = 73
     >>> otto = factory.makePerson(name='otto')
     >>> ignore = autorenewal_team.addMember(otto, salgado)
-    >>> otto_membership = otto.myactivememberships[0]
+    >>> otto_membership = otto.team_memberships[0]
     >>> otto_membership.setExpirationDate(utc_now, salgado)
     >>> original_otto_date_expires = otto_membership.dateexpires
 
     >>> do_not_warn = factory.makePerson(name='do-not-warn')
     >>> ignore = autorenewal_team.addMember(do_not_warn, salgado)
-    >>> do_not_warn.myactivememberships[0].setExpirationDate(
+    >>> do_not_warn.team_memberships[0].setExpirationDate(
     ...     tomorrow, salgado)
 
     # Not excluding teams with automatic renewals.
@@ -788,10 +788,10 @@
 
 == Querying team memberships ==
 
-You can check a person's direct memberships by using myactivememberships:
+You can check a person's direct memberships by using team_memberships:
 
     >>> [(membership.team.name, membership.status.title)
-    ...  for membership in salgado.myactivememberships]
+    ...  for membership in salgado.team_memberships]
     [(u'hwdb-team', 'Approved'), (u'landscape-developers', 'Approved'),
      (u'admins', 'Administrator'), (u't4', 'Approved')]
 

=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt	2010-08-13 21:30:24 +0000
+++ lib/lp/registry/doc/vocabularies.txt	2010-08-22 17:16:44 +0000
@@ -320,7 +320,7 @@
 
     >>> sample_person = person_set.getByName('name12')
     >>> [membership.team.name
-    ...  for membership in sample_person.myactivememberships]
+    ...  for membership in sample_person.team_memberships]
     [u'hwdb-team', u'landscape-developers', u'launchpad-users', u'name20']
     >>> [team.name for team in sample_person.teams_participated_in]
     [u'hwdb-team', u'landscape-developers', u'launchpad-users', u'name18',

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/person.py	2010-08-22 17:16:44 +0000
@@ -189,10 +189,8 @@
 
 def validate_public_person(obj, attr, value):
     """Validate that the person identified by value is public."""
-
     def validate(person):
         return is_public_person(person)
-
     return validate_person_common(obj, attr, value, validate)
 
 
@@ -661,9 +659,9 @@
                         readonly=True, required=False,
                         value_type=Reference(schema=IJabberID)),
         exported_as='jabber_ids')
-    myactivememberships = exported(
+    team_memberships = exported(
         CollectionField(
-            title=_("All TeamMemberships for Teams this Person is an "
+            title=_("All TeamMemberships for Teams this Team or Person is an "
                     "active member of."),
             value_type=Reference(schema=ITeamMembership),
             readonly=True, required=False),
@@ -979,8 +977,7 @@
     # @operation_returns_collection_of(Interface) # Really IPerson
     # @export_read_operation()
     def findPathToTeam(team):
-        """Return the teams that cause this person to be a participant of the
-        given team.
+        """Return the teams providing membership to the given team.
 
         If there is more than one path leading this person to the given team,
         only the one with the oldest teams is returned.
@@ -1157,6 +1154,9 @@
     def getLatestApprovedMembershipsForPerson(limit=5):
         """Return the <limit> latest approved membrships for this person."""
 
+    def getPathsToTeams():
+        """Return the paths to all teams related to this person."""
+
     def addLanguage(language):
         """Add a language to this person's preferences.
 
@@ -1383,7 +1383,7 @@
 
     def getMembersWithPreferredEmailsCount():
         """Returns the count of persons/teams with preferred emails.
-        
+
         See also getMembersWithPreferredEmails.
         """
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/person.py	2010-08-22 17:16:44 +0000
@@ -55,6 +55,7 @@
 from storm.expr import (
     Alias,
     And,
+    Desc,
     Exists,
     In,
     Join,
@@ -65,6 +66,7 @@
     Or,
     Select,
     SQL,
+    Upper,
     )
 from storm.info import ClassAlias
 from storm.store import (
@@ -1618,7 +1620,8 @@
         if need_location:
             # New people have no location rows
             origin.append(
-                LeftJoin(PersonLocation, PersonLocation.person == Person.id))
+                LeftJoin(PersonLocation,
+                    PersonLocation.person == Person.id))
             columns.append(PersonLocation)
         if need_archive:
             # Not everyone has PPA's
@@ -1821,19 +1824,22 @@
             self.invited_members,
             orderBy=self._sortingColumnsForSetOperations)
 
-    # XXX: kiko 2005-10-07:
-    # myactivememberships should be renamed to team_memberships and be
-    # described as the set of memberships for the object's teams.
     @property
-    def myactivememberships(self):
+    def team_memberships(self):
         """See `IPerson`."""
-        return TeamMembership.select("""
-            TeamMembership.person = %s AND status in %s AND
-            Person.id = TeamMembership.team
-            """ % sqlvalues(self.id, [TeamMembershipStatus.APPROVED,
-                                      TeamMembershipStatus.ADMIN]),
-            clauseTables=['Person'],
-            orderBy=Person.sortingColumns)
+        Team = ClassAlias(Person, "Team")
+        store = Store.of(self)
+        # Join on team to sort by team names. Upper is used in the sort so
+        # sorting works as is user expected, e.g. (A b C) not (A C b).
+        return store.find(TeamMembership,
+            And(TeamMembership.personID == self.id,
+                TeamMembership.teamID == Team.id,
+                TeamMembership.status.is_in([
+                    TeamMembershipStatus.APPROVED,
+                    TeamMembershipStatus.ADMIN,
+                    ]))).order_by(
+                        Upper(Team.displayname),
+                        Upper(Team.name))
 
     def _getMappedParticipantsLocations(self, limit=None):
         """See `IPersonViewRestricted`."""
@@ -1939,7 +1945,7 @@
         assert self.is_valid_person, (
             "You can only deactivate an account of a valid person.")
 
-        for membership in self.myactivememberships:
+        for membership in self.team_memberships:
             self.leave(membership.team)
 
         # Deactivate CoC signatures, invalidate email addresses, unassign bug
@@ -2186,10 +2192,66 @@
 
     def getLatestApprovedMembershipsForPerson(self, limit=5):
         """See `IPerson`."""
-        result = self.myactivememberships
-        result = result.orderBy(['-date_joined', '-id'])
+        result = self.team_memberships
+        result = result.order_by(
+            Desc(TeamMembership.datejoined),
+            Desc(TeamMembership.id))
         return result[:limit]
 
+    def getPathsToTeams(self):
+        """See `Iperson`."""
+        # Get all of the teams this person participates in.
+        teams = list(self.teams_participated_in)
+
+        # For cases where self is a team, we don't need self as a team
+        # participated in.
+        teams = [team for team in teams if team is not self]
+
+        # Get all of the memberships for any of the teams this person is
+        # a participant of. This must be ordered by date and id because
+        # because the graph of the results will create needs to contain
+        # the oldest path information to be consistent with results from
+        # IPerson.findPathToTeam.
+        store = Store.of(self)
+        all_direct_memberships = store.find(TeamMembership,
+            And(
+                TeamMembership.personID.is_in(
+                    [team.id for team in teams] + [self.id]),
+                TeamMembership.teamID != self.id,
+                TeamMembership.status.is_in([
+                    TeamMembershipStatus.APPROVED,
+                    TeamMembershipStatus.ADMIN,
+                    ]))).order_by(
+                        Desc(TeamMembership.datejoined),
+                        Desc(TeamMembership.id))
+        # Cast the results to list now, because they will be iterated over
+        # several times.
+        all_direct_memberships = list(all_direct_memberships)
+
+        # Pull out the memberships directly used by this person.
+        user_memberships = [
+            membership for membership in
+            all_direct_memberships
+            if membership.person == self]
+
+        all_direct_memberships = [
+            (membership.team, membership.person) for membership in
+            all_direct_memberships]
+
+        # Create a graph from the edges provided by the other data sets.
+        graph = dict(all_direct_memberships)
+
+        # Build the teams paths from that graph.
+        paths = {}
+        for team in teams:
+            path = [team]
+            step = team
+            while path[-1] != self:
+                step = graph[step]
+                path.append(step)
+            paths[team] = path
+        return (paths, user_memberships)
+
     @property
     def teams_participated_in(self):
         """See `IPerson`."""

=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_person.py	2010-08-22 17:16:44 +0000
@@ -67,19 +67,107 @@
 
     layer = DatabaseFunctionalLayer
 
+    def setUp(self):
+        super(TestPersonTeams, self).setUp()
+        self.user = self.factory.makePerson()
+        self.a_team = self.factory.makeTeam(name='a')
+        self.b_team = self.factory.makeTeam(name='b', owner=self.a_team)
+        self.c_team = self.factory.makeTeam(name='c', owner=self.b_team)
+        login_person(self.a_team.teamowner)
+        self.a_team.addMember(self.user, self.a_team.teamowner)
+
     def test_teams_indirectly_participated_in(self):
-        self.user = self.factory.makePerson()
-        a_team = self.factory.makeTeam(name='a')
-        b_team = self.factory.makeTeam(name='b', owner=a_team)
-        c_team = self.factory.makeTeam(name='c', owner=b_team)
-        login_person(a_team.teamowner)
-        a_team.addMember(self.user, a_team.teamowner)
         indirect_teams = self.user.teams_indirectly_participated_in
-        expected_teams = [b_team, c_team]
+        expected_teams = [self.b_team, self.c_team]
         test_teams = sorted(indirect_teams,
             key=lambda team: team.displayname)
         self.assertEqual(expected_teams, test_teams)
 
+    def test_team_memberships(self):
+        memberships = self.user.team_memberships
+        memberships = [(m.person, m.team) for m in memberships]
+        self.assertEqual([(self.user, self.a_team)], memberships)
+
+    def test_path_to_team(self):
+        path_to_a = self.user.findPathToTeam(self.a_team)
+        path_to_b = self.user.findPathToTeam(self.b_team)
+        path_to_c = self.user.findPathToTeam(self.c_team)
+
+        self.assertEqual([self.a_team], path_to_a)
+        self.assertEqual([self.a_team, self.b_team], path_to_b)
+        self.assertEqual([self.a_team, self.b_team, self.c_team], path_to_c)
+
+    def test_teams_participated_in(self):
+        teams = self.user.teams_participated_in
+        teams = sorted(list(teams), key=lambda x: x.displayname)
+        expected_teams = [self.a_team, self.b_team, self.c_team]
+        self.assertEqual(expected_teams, teams)
+
+    def test_getPathsToTeams(self):
+        paths, memberships = self.user.getPathsToTeams()
+        expected_paths = {self.a_team: [self.a_team, self.user],
+            self.b_team: [self.b_team, self.a_team, self.user],
+            self.c_team: [self.c_team, self.b_team, self.a_team, self.user]}
+        self.assertEqual(expected_paths, paths)
+
+        expected_memberships = [(self.a_team, self.user)]
+        memberships = [
+            (membership.team, membership.person) for membership
+            in memberships]
+        self.assertEqual(expected_memberships, memberships)
+
+    def test_getPathsToTeamsComplicated(self):
+        d_team = self.factory.makeTeam(name='d', owner=self.b_team)
+        e_team = self.factory.makeTeam(name='e')
+        f_team = self.factory.makeTeam(name='f', owner=e_team)
+        unrelated_team = self.factory.makeTeam(name='unrelated')
+        login_person(self.a_team.teamowner)
+        d_team.addMember(self.user, d_team.teamowner)
+        login_person(e_team.teamowner)
+        e_team.addMember(self.user, e_team.teamowner)
+
+        paths, memberships = self.user.getPathsToTeams()
+        expected_paths = {
+            self.a_team: [self.a_team, self.user],
+            self.b_team: [self.b_team, self.a_team, self.user],
+            self.c_team: [self.c_team, self.b_team, self.a_team, self.user],
+            d_team: [d_team, self.b_team, self.a_team, self.user],
+            e_team: [e_team, self.user],
+            f_team: [f_team, e_team, self.user]}
+        self.assertEqual(expected_paths, paths)
+
+        expected_memberships = [
+            (e_team, self.user),
+            (d_team, self.user),
+            (self.a_team, self.user),
+            ]
+        memberships = [
+            (membership.team, membership.person) for membership
+            in memberships]
+        self.assertEqual(expected_memberships, memberships)
+
+    def test_getPathsToTeamsMultiplePaths(self):
+        d_team = self.factory.makeTeam(name='d', owner=self.b_team)
+        login_person(self.a_team.teamowner)
+        self.c_team.addMember(d_team, self.c_team.teamowner)
+
+        paths, memberships = self.user.getPathsToTeams()
+        # getPathsToTeams should not randomly pick one path or another
+        # when multiples exist; it sorts to use the oldest path, so
+        # the expected paths below should be the returned result.
+        expected_paths = {
+            self.a_team: [self.a_team, self.user],
+            self.b_team: [self.b_team, self.a_team, self.user],
+            self.c_team: [self.c_team, self.b_team, self.a_team, self.user],
+            d_team: [d_team, self.b_team, self.a_team, self.user]}
+        self.assertEqual(expected_paths, paths)
+
+        expected_memberships = [(self.a_team, self.user)]
+        memberships = [
+            (membership.team, membership.person) for membership
+            in memberships]
+        self.assertEqual(expected_memberships, memberships)
+
 
 class TestPerson(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/vocabularies.py	2010-08-22 17:16:44 +0000
@@ -267,8 +267,7 @@
             like_query = "'%%' || %s || '%%'" % quote_like(query)
             fti_query = quote(query)
             sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                    like_query, fti_query
-                    )
+                    like_query, fti_query)
             return self._table.select(sql, orderBy=self._orderBy)
         return self.emptySelectResults()
 
@@ -315,8 +314,7 @@
             like_query = "'%%' || %s || '%%'" % quote_like(query)
             fti_query = quote(query)
             sql = "active = 't' AND (name LIKE %s OR fti @@ ftq(%s))" % (
-                    like_query, fti_query
-                    )
+                    like_query, fti_query)
             return self._table.select(sql)
         return self.emptySelectResults()
 
@@ -592,11 +590,8 @@
                        # Or a person who has an active account and a working
                        # email address.
                        And(Account.status == AccountStatus.ACTIVE,
-                           EmailAddress.status.is_in(valid_email_statuses))
-                       ),
-                    self.extra_clause
-                    )
-                )
+                           EmailAddress.status.is_in(valid_email_statuses))),
+                    self.extra_clause))
             # The public query doesn't need to be ordered as it will be done
             # at the end.
             public_result.order_by()
@@ -836,7 +831,7 @@
     def _get_teams(self):
         """The teams that the vocabulary is built from."""
         return [membership.team for membership
-                in self.context.myactivememberships
+                in self.context.team_memberships
                 if membership.team.visibility == PersonVisibility.PUBLIC]
 
     def __len__(self):
@@ -1027,9 +1022,7 @@
                 Milestone.q.productseriesID == ProductSeries.q.id,
                 ProductSeries.q.productID == Product.q.id,
                 Product.q.name == productname,
-                ProductSeries.q.name == productseriesname
-                )
-            )
+                ProductSeries.q.name == productseriesname))
         try:
             return self.toTerm(obj)
         except IndexError:
@@ -1315,8 +1308,7 @@
         return [
             project for project in sorted(projects,
                                           key=attrgetter('displayname'))
-            if not project.qualifies_for_free_hosting
-            ]
+            if not project.qualifies_for_free_hosting]
 
     def _doSearch(self, query=None):
         """Return terms where query is in the text of name
@@ -1454,11 +1446,8 @@
                     Distribution.q.id == DistroSeries.q.distributionID,
                     OR(
                         CONTAINSSTRING(Distribution.q.name, query),
-                        CONTAINSSTRING(DistroSeries.q.name, query)
-                        )
-                    ),
-                orderBy=self._orderBy
-                )
+                        CONTAINSSTRING(DistroSeries.q.name, query))),
+                    orderBy=self._orderBy)
         return objs
 
 
@@ -1472,8 +1461,7 @@
         """See `IVocabulary`."""
         if IPillarName.providedBy(obj):
             assert obj.active, 'Inactive object %s %d' % (
-                    obj.__class__.__name__, obj.id
-                    )
+                    obj.__class__.__name__, obj.id)
             obj = obj.pillar
 
         # It is a hack using the class name here, but it works