launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02998
[Merge] lp:~sinzui/launchpad/deactivate-all-members-fix-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/deactivate-all-members-fix-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #733881 in Launchpad itself: "+participation oops because membership and teamparticipation disagree"
https://bugs.launchpad.net/launchpad/+bug/733881
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/deactivate-all-members-fix-0/+merge/53885
deactivateAllMembers() must use TM._cleanTeamParticipation.
Launchpad bug:
https://bugs.launchpad.net/bugs/733881
Pre-implementation: jcsackett
Test command: ./bin/test -vv \
-t team_membership -t registry/tests/test_person \
-t registry/tests/test_team -t doc/person-merge -t peoplemerge \t
-t doc/teammembership -t doc/vocabularies
Oopses like "AssertionError: sinzui is an indirect member of ubuntu-
translations-coordinators but sinzui is not a participant in any direct member
of ubuntu-translations-coordinators" was caused by a deletion on team. All
the users in the delete team were members of ~rosetta-admins via another
team. The TeamParticipation entries for ~rosetta-admins should not have been
deleted.
Person.deactivateAllMembers() was called many steps before merge was called
to ensure there were no TP entries for the team. This method does a simple
delete of TP based on the active membership. It does not recursively check if
the user has an other path to participation in a team via another team
membership.
TeamMembership._cleanTeamParticipation does does build a exclude list of
participation entries. The fix might be to replace the custom query with a
call to TM._cleanTeamParticipation(child, parent). I see store.invalidate() is
called twice; I think once will suffice. The final storm call to remove the
direct participants may not be needed either.
--------------------------------------------------------------------
RULES
* Remove the redundant call to store.invalidate().
* Replace the inline sql with a call to TM._cleanTeamParticipation
* Remove the code to that deals with the direct participants if
the master function does it.
* ADDENDUM: Move this method to ITeamMembershipSet to encourage the
one true way to manange TeamParticipation
QA
* On staging or QA, staging, delete ~launchpad-chr.
* Verify that ~sinzui/+participation still works.
LINT
lib/lp/registry/configure.zcml
lib/lp/registry/browser/peoplemerge.py
lib/lp/registry/doc/person-merge.txt
lib/lp/registry/doc/teammembership.txt
lib/lp/registry/doc/vocabularies.txt
lib/lp/registry/interfaces/person.py
lib/lp/registry/interfaces/teammembership.py
lib/lp/registry/model/person.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/tests/test_person.py
lib/lp/registry/tests/test_team.py
lib/lp/registry/tests/test_teammembership.py
^ lint does not like some of the doctests. I can clean them up before
landing this branch.
IMPLEMENTATION
Added a test to verify the expected behaviour of mass member deactivation.
Refactored deactivateAllMembers to use TM._cleanTeamParticipation, which
allowed me to delete 2/3 of the method. The storm bug mentioned in the
comment was fixed a long time ago, so I fixed the enums. I then moved this
method to TMSet.deactivateActiveMemberships to keep the TeamParticipation
rules in one module. The existing tests from duplicated the existing
tests, but there was no test to verify that the comment and user were
saved with the mass deactivation. I revised one of the old tests to verify
that deactivateActiveMemberships stores the right information.
lib/lp/registry/interfaces/teammembership.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/tests/test_teammembership.py
lib/lp/registry/tests/test_team.py
Updated call sites to use TMSet.deactivateActiveMemberships. Removed
the old method which also permited the removal of an interface, and the
deletion of an duplicate tests.
lib/lp/registry/configure.zcml
lib/lp/registry/browser/peoplemerge.py
lib/lp/registry/doc/person-merge.txt
lib/lp/registry/doc/teammembership.txt
lib/lp/registry/doc/vocabularies.txt
lib/lp/registry/interfaces/person.py
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_person.py
--
https://code.launchpad.net/~sinzui/launchpad/deactivate-all-members-fix-0/+merge/53885
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/deactivate-all-members-fix-0 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py 2011-02-16 17:31:11 +0000
+++ lib/lp/registry/browser/peoplemerge.py 2011-03-17 19:27:33 +0000
@@ -47,6 +47,7 @@
IPersonSet,
IRequestPeopleMerge,
)
+from lp.registry.interfaces.teammembership import ITeamMembershipSet
from lp.services.propertycache import cachedproperty
from lp.soyuz.enums import ArchiveStatus
from lp.soyuz.interfaces.archive import IArchiveSet
@@ -317,7 +318,9 @@
'issues with this change.'
% (self.target_person.unique_displayname,
canonical_url(self.target_person)))
- self.dupe_person.deactivateAllMembers(comment, self.user)
+ membershipset = getUtility(ITeamMembershipSet)
+ membershipset.deactivateActiveMemberships(
+ self.dupe_person, comment, self.user)
flush_database_updates()
self.doMerge(data)
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-03-10 19:11:04 +0000
+++ lib/lp/registry/configure.zcml 2011-03-17 19:27:33 +0000
@@ -893,9 +893,6 @@
permission="launchpad.Moderate"
set_attributes="name"/>
<require
- permission="launchpad.Moderate"
- interface="lp.registry.interfaces.person.IPersonModerate" />
- <require
permission="launchpad.Commercial"
set_schema="lp.registry.interfaces.person.IPersonCommAdminWriteRestricted"/>
<require
=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt 2011-01-04 16:08:57 +0000
+++ lib/lp/registry/doc/person-merge.txt 2011-03-17 19:27:33 +0000
@@ -418,15 +418,18 @@
For cases like this we have an API which takes care of deactivating all
active members of the team so that we can perform the merge.
+ >>> from lp.registry.interfaces.teammembership import ITeamMembershipSet
+
# Only admins can use that API, though, so we need to login as an admin.
>>> login('foo.bar@xxxxxxxxxxxxx')
>>> comment = "We're merging this team into another one."
- >>> test_team.deactivateAllMembers(comment, personset.getByName('name16'))
+ >>> membershipset = getUtility(ITeamMembershipSet)
+ >>> membershipset.deactivateActiveMemberships(
+ ... test_team, comment, personset.getByName('name16'))
>>> for team in test_team.super_teams:
... test_team.retractTeamMembership(team, test_team.teamowner)
-
>>> personset.merge(test_team, landscape)
# The resulting Landscape-developers no new super teams, has
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt 2011-02-28 23:25:52 +0000
+++ lib/lp/registry/doc/teammembership.txt 2011-03-17 19:27:33 +0000
@@ -895,7 +895,8 @@
If a team is merged it will not show up in the set of administered teams.
>>> login('foo.bar@xxxxxxxxxxxxx')
- >>> cprov_team.deactivateAllMembers("Merging", foobar)
+ >>> membershipset.deactivateActiveMemberships(
+ ... cprov_team, "Merging", foobar)
>>> personset.merge(cprov_team, guadamen_team)
>>> [team.name for team in cprov.getAdministratedTeams()]
[u'canonical-partner-dev', u'guadamen', u'launchpad-buildd-admins']
=== modified file 'lib/lp/registry/doc/vocabularies.txt'
--- lib/lp/registry/doc/vocabularies.txt 2010-12-21 21:59:19 +0000
+++ lib/lp/registry/doc/vocabularies.txt 2011-03-17 19:27:33 +0000
@@ -740,8 +740,11 @@
Valid teams do not include teams that have been merged.
+ >>> from lp.registry.interfaces.teammembership import ITeamMembershipSet
>>> login_person(foo_bar)
- >>> ephemeral.deactivateAllMembers("Merging", foo_bar)
+ >>> membershipset = getUtility(ITeamMembershipSet)
+ >>> membershipset.deactivateActiveMemberships(
+ ... ephemeral, "Merging", foo_bar)
>>> person_set.merge(ephemeral, foo_bar)
>>> login_person(sample_person)
>>> sorted(person.name for person in vocab.search('team'))
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2011-03-01 09:41:39 +0000
+++ lib/lp/registry/interfaces/person.py 2011-03-17 19:27:33 +0000
@@ -1716,13 +1716,6 @@
"""
-class IPersonModerate(Interface):
- """IPerson attributes that require launchpad.Moderate."""
-
- def deactivateAllMembers(comment, reviewer):
- """Deactivate all the members of this team."""
-
-
class IPersonCommAdminWriteRestricted(Interface):
"""IPerson attributes that require launchpad.Admin permission to set."""
@@ -1771,7 +1764,7 @@
class IPerson(IPersonPublic, IPersonViewRestricted, IPersonEditRestricted,
IPersonCommAdminWriteRestricted, IPersonSpecialRestricted,
- IPersonModerate, IHasStanding, ISetLocation, IRootContext):
+ IHasStanding, ISetLocation, IRootContext):
"""A Person."""
export_as_webservice_entry(plural_name='people')
=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py 2011-03-01 03:52:30 +0000
+++ lib/lp/registry/interfaces/teammembership.py 2011-03-17 19:27:33 +0000
@@ -302,6 +302,16 @@
TeamMembership and I'll return None.
"""
+ def deactivateActiveMemberships(team, comment, reviewer):
+ """Deactivate all team members in ACTIVE_STATES.
+
+ This is a convenience method used before teams are deleted.
+
+ :param team: The team to deactivate.
+ :param comment: An explanation for the deactivation.
+ :param reviewer: The user doing the deactivation.
+ """
+
class ITeamParticipation(Interface):
"""A TeamParticipation.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-03-09 18:18:02 +0000
+++ lib/lp/registry/model/person.py 2011-03-17 19:27:33 +0000
@@ -1547,89 +1547,6 @@
tm.dateexpires += timedelta(days=team.defaultrenewalperiod)
tm.sendSelfRenewalNotification()
- def deactivateAllMembers(self, comment, reviewer):
- """Deactivate all members of this team.
-
- This method circuments the TeamMembership.setStatus() method
- to improve performance; therefore, it does not send out any
- status change noticiations to the team admins.
-
- :param comment: Explanation of the change.
- :param reviewer: Person who made the change.
- """
- assert self.is_team, "This method is only available for teams."
- now = datetime.now(pytz.timezone('UTC'))
- store = Store.of(self)
- cur = cursor()
-
- # Deactivate the approved/admin team members.
- # XXX: EdwinGrubbs 2009-07-08 bug=397072
- # There are problems using storm to write an update
- # statement using DBITems in the comparison.
- cur.execute("""
- UPDATE TeamMembership
- SET status=%(status)s,
- last_changed_by=%(last_changed_by)s,
- last_change_comment=%(comment)s,
- date_last_changed=%(date_last_changed)s
- WHERE
- TeamMembership.team = %(team)s
- AND TeamMembership.status IN %(original_statuses)s
- """,
- dict(
- status=TeamMembershipStatus.DEACTIVATED,
- last_changed_by=reviewer.id,
- comment=comment,
- date_last_changed=now,
- team=self.id,
- original_statuses=(
- TeamMembershipStatus.ADMIN.value,
- TeamMembershipStatus.APPROVED.value)))
-
- # Since we've updated the database behind Storm's back,
- # flush its caches.
- store.invalidate()
-
- # Remove all indirect TeamParticipation entries resulting from this
- # team. If this were just a select, it would be a complicated but
- # feasible set of joins. Since it's a delete, we have to use
- # some sub selects.
- cur.execute('''
- DELETE FROM TeamParticipation
- WHERE
- -- The person needs to be a member of the team in question
- person IN
- (SELECT person from TeamParticipation WHERE
- team = %(team)s) AND
-
- -- The teams being deleted should be teams that this team
- -- is a member of.
- team IN
- (SELECT team from TeamMembership WHERE
- person = %(team)s) AND
-
- -- The person needs to not have direct membership in the
- -- team.
- NOT EXISTS
- (SELECT tm1.person from TeamMembership tm1
- WHERE
- tm1.person = TeamParticipation.person and
- tm1.team = TeamParticipation.team and
- tm1.status IN %(active_states)s);
- ''', dict(team=self.id, active_states=ACTIVE_STATES))
-
- # Since we've updated the database behind Storm's back yet again,
- # we need to flush its caches, again.
- store.invalidate()
-
- # Remove all members from the TeamParticipation table
- # except for the team, itself.
- participants = store.find(
- TeamParticipation,
- TeamParticipation.teamID == self.id,
- TeamParticipation.personID != self.id)
- participants.remove()
-
def setMembershipData(self, person, status, reviewer, expires=None,
comment=None):
"""See `IPerson`."""
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2011-03-09 18:18:02 +0000
+++ lib/lp/registry/model/teammembership.py 2011-03-17 19:27:33 +0000
@@ -30,6 +30,7 @@
from canonical.database.datetimecol import UtcDateTimeCol
from canonical.database.enumcol import EnumCol
from canonical.database.sqlbase import (
+ cursor,
flush_database_updates,
SQLBase,
sqlvalues,
@@ -493,6 +494,33 @@
TeamMembershipRenewalPolicy.AUTOMATIC)
return IStore(TeamMembership).find(TeamMembership, *conditions)
+ def deactivateActiveMemberships(self, team, comment, reviewer):
+ """See `ITeamMembershipSet`."""
+ now = datetime.now(pytz.timezone('UTC'))
+ store = Store.of(team)
+ cur = cursor()
+ all_members = list(team.activemembers)
+ cur.execute("""
+ UPDATE TeamMembership
+ SET status=%(status)s,
+ last_changed_by=%(last_changed_by)s,
+ last_change_comment=%(comment)s,
+ date_last_changed=%(date_last_changed)s
+ WHERE
+ TeamMembership.team = %(team)s
+ AND TeamMembership.status IN %(original_statuses)s
+ """,
+ dict(
+ status=TeamMembershipStatus.DEACTIVATED,
+ last_changed_by=reviewer.id,
+ comment=comment,
+ date_last_changed=now,
+ team=team.id,
+ original_statuses=ACTIVE_STATES))
+ for member in all_members:
+ # store.invalidate() is called for each iteration.
+ _cleanTeamParticipation(member, team)
+
class TeamParticipation(SQLBase):
implements(ITeamParticipation)
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-03-08 22:31:58 +0000
+++ lib/lp/registry/tests/test_person.py 2011-03-17 19:27:33 +0000
@@ -55,6 +55,7 @@
PersonVisibility,
)
from lp.registry.interfaces.product import IProductSet
+from lp.registry.interfaces.teammembership import ITeamMembershipSet
from lp.registry.model.karma import (
KarmaCategory,
KarmaTotalCache,
@@ -684,8 +685,9 @@
self.assertEqual(oldest_date, person.datecreated)
def _doMerge(self, test_team, target_team):
- test_team.deactivateAllMembers(
- comment='',
+ membershipset = getUtility(ITeamMembershipSet)
+ membershipset.deactivateActiveMemberships(
+ test_team, comment='',
reviewer=test_team.teamowner)
self.person_set.merge(test_team, target_team)
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py 2011-02-01 19:16:43 +0000
+++ lib/lp/registry/tests/test_team.py 2011-03-17 19:27:33 +0000
@@ -425,68 +425,3 @@
members = list(team.approvedmembers)
self.assertEqual(1, len(members))
self.assertEqual(user, members[0])
-
-
-class TestMembershipManagement(TestCaseWithFactory):
-
- layer = DatabaseFunctionalLayer
-
- def test_deactivateAllMembers_cleans_up_teamparticipation_deactivated(
- self):
- superteam = self.factory.makeTeam(name='super')
- targetteam = self.factory.makeTeam(name='target')
- login_celebrity('admin')
- targetteam.join(superteam, targetteam.teamowner)
-
- # Now we create a deactivated link for the target team's teamowner.
- targetteam.teamowner.join(superteam, targetteam.teamowner)
- targetteam.teamowner.leave(superteam)
-
- self.assertEqual(
- sorted([superteam, targetteam]),
- sorted([team for team in
- targetteam.teamowner.teams_participated_in]))
- targetteam.deactivateAllMembers(
- comment='test',
- reviewer=targetteam.teamowner)
- self.assertEqual(
- [],
- sorted([team for team in
- targetteam.teamowner.teams_participated_in]))
-
- def test_deactivateAllMembers_cleans_up_teamparticipation_teamowner(self):
- superteam = self.factory.makeTeam(name='super')
- targetteam = self.factory.makeTeam(name='target')
- login_celebrity('admin')
- targetteam.join(superteam, targetteam.teamowner)
- self.assertEqual(
- sorted([superteam, targetteam]),
- sorted([team for team
- in targetteam.teamowner.teams_participated_in]))
- targetteam.deactivateAllMembers(
- comment='test',
- reviewer=targetteam.teamowner)
- self.assertEqual(
- [],
- sorted([team for team
- in targetteam.teamowner.teams_participated_in]))
-
- def test_deactivateAllMembers_cleans_up_team_participation(self):
- superteam = self.factory.makeTeam(name='super')
- sharedteam = self.factory.makeTeam(name='shared')
- anotherteam = self.factory.makeTeam(name='another')
- targetteam = self.factory.makeTeam(name='target')
- person = self.factory.makePerson()
- login_celebrity('admin')
- person.join(targetteam)
- person.join(sharedteam)
- person.join(anotherteam)
- targetteam.join(superteam, targetteam.teamowner)
- targetteam.join(sharedteam, targetteam.teamowner)
- self.assertTrue(superteam in person.teams_participated_in)
- targetteam.deactivateAllMembers(
- comment='test',
- reviewer=targetteam.teamowner)
- self.assertEqual(
- sorted([sharedteam, anotherteam]),
- sorted([team for team in person.teams_participated_in]))
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py 2011-03-01 03:52:30 +0000
+++ lib/lp/registry/tests/test_teammembership.py 2011-03-17 19:27:33 +0000
@@ -51,16 +51,18 @@
TeamParticipation,
)
from lp.testing import (
+ login_celebrity,
person_logged_in,
TestCaseWithFactory,
)
from lp.testing.mail_helpers import pop_notifications
-class TestTeamMembershipSet(TestCase):
+class TestTeamMembershipSet(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
def setUp(self):
+ super(TestTeamMembershipSet, self).setUp()
login('test@xxxxxxxxxxxxx')
self.membershipset = getUtility(ITeamMembershipSet)
self.personset = getUtility(IPersonSet)
@@ -166,6 +168,24 @@
sample_person_on_motu.status, TeamMembershipStatus.EXPIRED)
self.failIf(sample_person.inTeam(motu))
+ def test_deactivateActiveMemberships(self):
+ superteam = self.factory.makeTeam(name='super')
+ targetteam = self.factory.makeTeam(name='target')
+ member = self.factory.makePerson()
+ login_celebrity('admin')
+ targetteam.join(superteam, targetteam.teamowner)
+ targetteam.addMember(member, targetteam.teamowner)
+ targetteam.teamowner.join(superteam, targetteam.teamowner)
+ self.membershipset.deactivateActiveMemberships(
+ targetteam, comment='test', reviewer=targetteam.teamowner)
+ membership = self.membershipset.getByPersonAndTeam(member, targetteam)
+ self.assertEqual('test', membership.last_change_comment)
+ self.assertEqual(targetteam.teamowner, membership.last_changed_by)
+ self.assertEqual([], list(targetteam.allmembers))
+ self.assertEqual(
+ [superteam], list(targetteam.teamowner.teams_participated_in))
+ self.assertEqual([], list(member.teams_participated_in))
+
class TeamParticipationTestCase(TestCaseWithFactory):
"""Tests for team participation using 5 teams."""
@@ -465,6 +485,31 @@
previous_count-10,
self.getTeamParticipationCount())
+ def testTeam3_deactivateActiveMemberships(self):
+ # Removing all the members of team2 will not remove memberships
+ # to super teams from other paths.
+ non_member = self.factory.makePerson()
+ self.team3.addMember(non_member, self.foo_bar, force_team_add=True)
+ previous_count = self.getTeamParticipationCount()
+ membershipset = getUtility(ITeamMembershipSet)
+ membershipset.deactivateActiveMemberships(
+ self.team3, 'gone', self.foo_bar)
+ self.assertEqual([], list(self.team3.allmembers))
+ self.assertParticipantsEquals(
+ ['name16', 'no-priv', 'team2', 'team3', 'team4', 'team5'],
+ self.team1)
+ self.assertParticipantsEquals(
+ ['name16', 'no-priv', 'team3', 'team4', 'team5'], self.team2)
+ self.assertParticipantsEquals(
+ [], self.team3)
+ self.assertParticipantsEquals(
+ ['name16', 'no-priv', 'team5'], self.team4)
+ self.assertParticipantsEquals(['name16', 'no-priv'], self.team5)
+ self.assertParticipantsEquals(
+ ['name16', 'no-priv', 'team2', 'team3', 'team4', 'team5'],
+ self.team6)
+ self.assertEqual(previous_count - 8, self.getTeamParticipationCount())
+
class TestTeamMembership(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
