launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00926
[Merge] lp:~sinzui/launchpad/inteam-db-calls-0 into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/inteam-db-calls-0 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#591544 person.inTeam(other_person) hits the db (when both not teams)
https://bugs.launchpad.net/bugs/591544
This is my branch to remove useless db checks from IPerson.inTeam().
lp:~sinzui/launchpad/inteam-db-calls-0
Diff size: 178
Launchpad bug:
https://bugs.launchpad.net/bugs/id
Test command: ./bin/test -vv -t TestPersonTeams
Pre-implementation: no one.
Target release: 10.10
Remove useless db checks from IPerson.inTeam()
----------------------------------------------
person.inTeam(other_person) hits the db when other_person is not a team.
Rules
-----
* Return early if the team is a user.
* The test will duplicate the team cache tests in doctests...remove
the doctest version because the cache is an implementation detail.
QA
--
None.
Lint
----
Linting changed files:
lib/lp/registry/doc/person.txt
lib/lp/registry/doc/teammembership.txt
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_person.py
Lint hated this file and I will fix it before I land.
./lib/lp/registry/doc/teammembership.txt
1: narrative uses a moin header.
60: narrative uses a moin header.
141: source exceeds 78 characters.
417: narrative uses a moin header.
555: narrative uses a moin header.
687: want exceeds 78 characters.
716: narrative uses a moin header.
789: narrative uses a moin header.
836: narrative uses a moin header.
881: want exceeds 78 characters.
892: narrative uses a moin header.
945: narrative uses a moin header.
634: 'Store' imported but unused
Test
----
* lib/lp/registry/doc/person.txt
* Removed implementation details from doctest.
* lib/lp/registry/doc/teammembership.txt
* Removed implementation details from doctest.
* lib/lp/registry/tests/test_person.py
* Added unittests for inTeam and its cache
* Added a test to verify that when the team is a person that the
method returns before querying the db and building the cache.
Implementation
--------------
* lib/lp/registry/model/person.py
* Added a check to return early when team argument is a user.
--
https://code.launchpad.net/~sinzui/launchpad/inteam-db-calls-0/+merge/34932
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/inteam-db-calls-0 into lp:launchpad/devel.
=== modified file 'lib/lp/registry/doc/person.txt'
--- lib/lp/registry/doc/person.txt 2010-07-27 22:13:36 +0000
+++ lib/lp/registry/doc/person.txt 2010-09-08 22:31:05 +0000
@@ -628,31 +628,6 @@
>>> vcs_imports.hasParticipationEntryFor(vcs_imports)
True
-The inTeam method is cached to avoid unnecessary database lookups - this
-was a cause of a number of timeouts
-
- >>> naked_lifeless = removeSecurityProxy(lifeless)
- >>> naked_lifeless._inTeam_cache[vcs_imports.id]
- True
-
- >>> naked_lifeless._inTeam_cache[vcs_imports.id] = False
- >>> lifeless.inTeam(vcs_imports)
- False
-
- >>> naked_lifeless._inTeam_cache[vcs_imports.id] = True
- >>> lifeless.inTeam(vcs_imports)
- True
-
-IPerson has a method to clear it; this is used between IPerson instances
-to ensure that the cache can be kept consistent when membership changes.
-
- >>> naked_lifeless.clearInTeamCache()
- >>> naked_lifeless._inTeam_cache
- {}
-
-** See lib/canonical/launchpad/doc/teammembership.txt for more
-information about team membership/participation.
-
Email notifications to teams
............................
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt 2010-08-23 08:46:04 +0000
+++ lib/lp/registry/doc/teammembership.txt 2010-09-08 22:31:05 +0000
@@ -942,43 +942,6 @@
t4
-== Membership caches ==
-
-Person instances have membership caches (_inTeam_cache) to avoid the
-need to reissue identical membership queries repeatedly. (This test uses
-Person directly to allow us access to an unproxied object, since
-_inTeam_cache is private, and it uses TeamMembershipSet directly to allow
-us to call TeamMembership.setStatus, which is private).
-
- >>> from lp.registry.model.person import Person
- >>> from lp.registry.model.teammembership import TeamMembershipSet
- >>> no_priv = Person.selectOneBy(name='no-priv')
- >>> no_priv.inTeam(admins)
- False
- >>> no_priv._inTeam_cache
- {...25: False...}
-
-This cache is cleared when memberships are created:
-
- >>> membership = TeamMembershipSet().new(
- ... no_priv, admins, TeamMembershipStatus.APPROVED, admins.teamowner)
- >>> no_priv._inTeam_cache
- {}
- >>> no_priv.inTeam(admins)
- True
- >>> no_priv._inTeam_cache
- {25: True}
-
-Or changed:
-
- >>> membership.setStatus(TeamMembershipStatus.DEACTIVATED, mark)
- True
- >>> no_priv._inTeam_cache
- {}
- >>> no_priv.inTeam(admins)
- False
-
-
== Email to invalid users ==
There are cases where teams have deactivated or suspended users as members.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2010-09-01 09:57:15 +0000
+++ lib/lp/registry/model/person.py 2010-09-08 22:31:05 +0000
@@ -1240,6 +1240,11 @@
if isinstance(team, (str, unicode)):
team = PersonSet().getByName(team)
+ if not team.is_team:
+ # It is possible that this team is really a user since teams
+ # are users are often interchangable.
+ return False
+
if self._inTeam_cache is None: # Initialize cache
self._inTeam_cache = {}
else:
@@ -1613,13 +1618,16 @@
person_table.accountID == account_table.id,
account_table.status == AccountStatus.ACTIVE)))
columns.append(account_table)
+
def handleemail(person, column):
#-- preferred email caching
if not person:
return
email = column
IPropertyCache(person).preferredemail = email
+
decorators.append(handleemail)
+
def handleaccount(person, column):
#-- validity caching
if not person:
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2010-08-22 03:09:51 +0000
+++ lib/lp/registry/tests/test_person.py 2010-09-08 22:31:05 +0000
@@ -116,7 +116,7 @@
in memberships]
self.assertEqual(expected_memberships, memberships)
- def test_getPathsToTeamsComplicated(self):
+ def test_getPathsToTeams_complicated(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)
@@ -146,7 +146,7 @@
in memberships]
self.assertEqual(expected_memberships, memberships)
- def test_getPathsToTeamsMultiplePaths(self):
+ def test_getPathsToTeams_multiple_paths(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)
@@ -168,6 +168,38 @@
in memberships]
self.assertEqual(expected_memberships, memberships)
+ def test_inTeam_direct_team(self):
+ # Verify direct membeship is True and the cache is populated.
+ self.assertTrue(self.user.inTeam(self.a_team))
+ self.assertEqual(
+ {self.a_team.id: True},
+ removeSecurityProxy(self.user)._inTeam_cache)
+
+ def test_inTeam_indirect_team(self):
+ # Verify indirect membeship is True and the cache is populated.
+ self.assertTrue(self.user.inTeam(self.b_team))
+ self.assertEqual(
+ {self.b_team.id: True},
+ removeSecurityProxy(self.user)._inTeam_cache)
+
+ def test_inTeam_cache_cleared_by_membership_change(self):
+ # Verify a change in membership clears the team cache.
+ self.user.inTeam(self.a_team)
+ with person_logged_in(self.b_team.teamowner):
+ self.b_team.addMember(self.user, self.b_team.teamowner)
+ self.assertEqual(
+ {},
+ removeSecurityProxy(self.user)._inTeam_cache)
+
+ def test_inTeam_person(self):
+ # Verify a user cannot be a member of another user and the and
+ # DB call to TeamParticipation was not made to learn this.
+ other_user = self.factory.makePerson()
+ self.assertFalse(self.user.inTeam(other_user))
+ self.assertEqual(
+ {},
+ removeSecurityProxy(self.user)._inTeam_cache)
+
class TestPerson(TestCaseWithFactory):
Follow ups
