launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00623
[Merge] lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel
j.c.sackett has proposed merging lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#607879 ~person/+participation timeouts
https://bugs.launchpad.net/bugs/607879
Summary
Contributes to the +participation time outs by limiting the team_path lookups to just one level of traversal. Also stormifies the team_membership query, previously myactivememberships
Proposed fix
Stormifying where possible to assist db load, and limiting the team_path look ups for indirect teams, which proved to be a painful set of queries.
Pre-implementation notes
Chatted with Curtis Hovey (sinzui)
Implementation details
Renames myactivememberships to team_memberships, per an XXX from kiko in the codebase. Updates call sites.
Stormifies team_memberships and updates callsites.
The big deal is the change to findPathForTeam, which now accepts a limit argument to cut the depth of traversal; this is used to set the limit to one to just find the next team in the chain to the person for indirect_teams.
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 only show "Membership via [team]" for indirect teams, rather than the "[team1], [team2]..." data previously shown.
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.
--
https://code.launchpad.net/~jcsackett/launchpad/plus-participation-additional-fixes/+merge/32820
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/plus-participation-additional-fixes into lp:launchpad/devel.
=== 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-16 21:51:00 +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-13 07:48:54 +0000
+++ lib/lp/registry/browser/person.py 2010-08-16 21:51:00 +0000
@@ -3078,7 +3078,7 @@
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.
@@ -3088,10 +3088,8 @@
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:
+ via = "Membership through %s" % via.displayname
if membership is None:
#we have membership via an indirect team, and can use
@@ -3129,17 +3127,16 @@
def active_participations(self):
"""Return the participation information for active memberships."""
participations = [self._asParticipation(membership=membership)
- for membership in self.context.myactivememberships
+ for membership in self.context.team_memberships
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):
continue
# The key points of the path for presentation are:
# [-?] indirect memberships, [-2] direct membership, [-1] team.
- team_path = self.context.findPathToTeam(team)
+ team_path = self.context.findPathToTeam(team, limit=1)
participations.append(
- self._asParticipation(team_path=team_path[:-1], team=team))
+ self._asParticipation(via=team_path[-2], team=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-13 07:48:54 +0000
+++ lib/lp/registry/browser/tests/test_person_view.py 2010-08-16 21:51:00 +0000
@@ -253,7 +253,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
@@ -344,8 +344,8 @@
participation['displayname'] for participation in participations]
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('Membership through A', participations[1]['via'])
+ self.assertEqual('Membership through B', participations[2]['via'])
def test_has_participations_false(self):
participations = self.view.active_participations
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2010-08-03 08:49:19 +0000
+++ lib/lp/registry/interfaces/person.py 2010-08-16 21:51:00 +0000
@@ -131,6 +131,7 @@
def validate_person(obj, attr, value):
"""Validate the person is a real person with no other restrictions."""
+
def validate(person):
return IPerson.providedBy(person)
return validate_person_common(obj, attr, value, validate)
@@ -610,9 +611,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),
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-08-13 08:26:15 +0000
+++ lib/lp/registry/model/person.py 2010-08-16 21:51:00 +0000
@@ -48,7 +48,7 @@
StringCol)
from sqlobject.sqlbuilder import AND, OR, SQLConstant
from storm.store import EmptyResultSet, Store
-from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL
+from storm.expr import And, In, Join, LeftJoin, Lower, Not, Or, SQL, Desc
from storm.info import ClassAlias
from canonical.config import config
@@ -749,8 +749,15 @@
packages.sort(key=lambda x: x.name)
return packages
- def findPathToTeam(self, team):
- """See `IPerson`."""
+ def findPathToTeam(self, team, limit=None):
+ """See `IPerson`.
+
+ The limit parameter sets the depth of traversal along the path. A
+ limit of 0 returns the passed in team, 1 returns the team
+ providing membership to the passed in team. 2 returns the membership
+ for the team returned with limit 1, and so on. None returns the path
+ all the way to the team Person has direct membership in.
+ """
# This is our guarantee that _getDirectMemberIParticipateIn() will
# never return None
assert self.hasParticipationEntryFor(team), (
@@ -759,9 +766,13 @@
assert team.is_team, "You can't pass a person to this method."
path = [team]
team = self._getDirectMemberIParticipateIn(team)
+ increment = 0
while team != self:
+ if increment == limit:
+ break
path.insert(0, team)
team = self._getDirectMemberIParticipateIn(team)
+ increment += 1
return path
def _getDirectMemberIParticipateIn(self, team):
@@ -1557,19 +1568,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(
+ [TeamMembershipStatus.APPROVED, TeamMembershipStatus.ADMIN])))
def _getMappedParticipantsLocations(self, limit=None):
"""See `IPersonViewRestricted`."""
@@ -1675,7 +1683,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
@@ -1922,8 +1930,9 @@
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]
@property
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-08-13 07:48:54 +0000
+++ lib/lp/registry/tests/test_person.py 2010-08-16 21:51:00 +0000
@@ -44,19 +44,45 @@
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_no_limit(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_path_to_team_with_limit(self):
+ path_to_c_0 = self.user.findPathToTeam(self.c_team, limit=0)
+ path_to_c_1 = self.user.findPathToTeam(self.c_team, limit=1)
+ path_to_c_2 = self.user.findPathToTeam(self.c_team, limit=2)
+
+ self.assertEqual([self.c_team], path_to_c_0)
+ self.assertEqual([self.b_team, self.c_team], path_to_c_1)
+ self.assertEqual([self.a_team, self.b_team, self.c_team], path_to_c_2)
+
class TestPerson(TestCaseWithFactory):
=== modified file 'lib/lp/registry/vocabularies.py'
--- lib/lp/registry/vocabularies.py 2010-07-29 20:16:23 +0000
+++ lib/lp/registry/vocabularies.py 2010-08-16 21:51:00 +0000
@@ -220,8 +220,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()
@@ -268,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)
return self.emptySelectResults()
@@ -545,11 +543,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()
@@ -789,7 +784,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):
@@ -980,9 +975,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:
@@ -1268,8 +1261,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
@@ -1407,11 +1399,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
@@ -1425,8 +1414,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
Follow ups
