launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03568
[Merge] lp:~sinzui/launchpad/person-merge-oopses-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/person-merge-oopses-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #779150 in Launchpad itself: "person merge: ProgrammingError: permission denied for relation job"
https://bugs.launchpad.net/launchpad/+bug/779150
Bug #779490 in Launchpad itself: "person_merge_job AssertionError: from_person still has email addresses."
https://bugs.launchpad.net/launchpad/+bug/779490
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/person-merge-oopses-0/+merge/60521
Fix permission and assertion oopses in PersonMergeJob.
Launchpad bug:
https://bugs.launchpad.net/bugs/777915
https://bugs.launchpad.net/bugs/779150
https://bugs.launchpad.net/bugs/779490
Pre-implementation: no one
Bug 777915 and bug 779150 show two tables that PersonMergeJob needs access
to. Bug 779490 is caused by a fault in the setContactAddress code that
wrongly preserves purged mailing list email address.
--------------------------------------------------------------------
RULES
* Run TestPersonSetMerge as the same db user as PersonMergeJob to ensure
all permissions are exercised.
* Use MailingListStatus.PURGED to ensure all deletable addresses are
found.
QA
* Delete a team with a recipe
* Delete a team with a milestone structural subscription
* Delete a team with members
* Delete a team with an historic list address (~libbls-dev-list)
LINT
database/schema/security.cfg
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_person.py
lib/lp/registry/tests/test_team.py
^ The new linter see unused variables and white space issues. I can fix these
after the review.
TEST
./bin/test -vv -t TestPersonSetMerge -t TestTeamContactAddress
IMPLEMENTATION
Updated the TestPersonSetMerge testcase to do the merge as the same db user
as IPersonMergeJob. Added some helper functions to re-get the objects that
were invalidated by the store change. Several tests were broken, notably those
related to recipes, subscriptions, and membership changes. I updated
security.cfg to make the broken tests pass.
database/schema/security.cfg
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_person.py
Updated setContactAddress to exclude purged mailing lists when building the
tuple of addresses to keep.
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_team.py
--
https://code.launchpad.net/~sinzui/launchpad/person-merge-oopses-0/+merge/60521
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/person-merge-oopses-0 into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-05-10 04:01:13 +0000
+++ database/schema/security.cfg 2011-05-10 15:36:40 +0000
@@ -3,7 +3,7 @@
#
# Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
#
-# Note that we can't have INSERT only tables if we are using SQLObject, as it
+# Note that we cannot have INSERT only tables if we are using SQLObject, as it
# creates new entries by first doing an insert (to get the id) and then
# issuing an update
[DEFAULT]
@@ -1988,7 +1988,7 @@
public.distroarchseries = SELECT, UPDATE
public.distrocomponentuploader = SELECT, UPDATE
public.distroseries = SELECT, UPDATE
-public.emailaddress = SELECT, UPDATE
+public.emailaddress = SELECT, UPDATE, DELETE
public.entitlement = SELECT, UPDATE
public.faq = SELECT, UPDATE
public.featureflagchangelogentry = SELECT, UPDATE
@@ -1996,9 +1996,10 @@
public.hwsubmission = SELECT, UPDATE
public.ircid = SELECT, UPDATE
public.jabberid = SELECT, UPDATE
-public.job = SELECT, UPDATE
+public.job = SELECT, UPDATE, INSERT
public.karma = SELECT, UPDATE
public.karmacache = SELECT, DELETE
+public.karmacategory = SELECT, DELETE
public.karmatotalcache = SELECT, UPDATE, DELETE
public.logintoken = SELECT, UPDATE
public.mailinglist = SELECT, UPDATE
@@ -2007,6 +2008,7 @@
public.mentoringoffer = SELECT, UPDATE
public.message = SELECT, UPDATE
public.messageapproval = SELECT, UPDATE
+public.milestone = SELECT, UPDATE
public.mirror = SELECT, UPDATE
public.nameblacklist = SELECT, UPDATE
public.oauthaccesstoken = SELECT, UPDATE
@@ -2023,7 +2025,7 @@
public.personlocation = SELECT, UPDATE
public.personnotification = SELECT, INSERT, UPDATE
public.personsettings = SELECT, UPDATE
-public.persontransferjob = SELECT, UPDATE
+public.persontransferjob = SELECT, INSERT, UPDATE
public.pocomment = SELECT, UPDATE
public.poexportrequest = SELECT, UPDATE, DELETE
public.pofile = SELECT, UPDATE
@@ -2044,9 +2046,12 @@
public.revisionauthor = SELECT, UPDATE
public.seriessourcepackagebranch = SELECT, UPDATE
public.signedcodeofconduct = SELECT, UPDATE
+public.sourcepackagename = SELECT
public.sourcepackagepublishinghistory = SELECT, UPDATE
public.sourcepackagerecipe = SELECT, UPDATE
public.sourcepackagerecipebuild = SELECT, UPDATE
+public.sourcepackagerecipedata = SELECT, UPDATE
+public.sourcepackagerecipedatainstruction = SELECT, UPDATE
public.sourcepackagerelease = SELECT, UPDATE
public.specification = SELECT, UPDATE
public.specificationbranch = SELECT, UPDATE
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-05-04 07:48:37 +0000
+++ lib/lp/registry/model/person.py 2011-05-10 15:36:40 +0000
@@ -2427,7 +2427,8 @@
self._setPreferredEmail(email)
# A team can have up to two addresses, the preferred one and one used
# by the team mailing list.
- if self.mailing_list is not None:
+ if (self.mailing_list is not None
+ and self.mailing_list.status != MailingListStatus.PURGED):
mailing_list_email = getUtility(IEmailAddressSet).getByEmail(
self.mailing_list.address)
if mailing_list_email is not None:
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2011-05-04 07:48:37 +0000
+++ lib/lp/registry/tests/test_person.py 2011-05-10 15:36:40 +0000
@@ -88,6 +88,22 @@
from lp.testing.views import create_initialized_view
+def reload_object(obj):
+ """Return a new instance of a storm objet from the store."""
+ store = IStore(Person)
+ return store.get(removeSecurityProxy(obj).__class__, obj.id)
+
+
+def reload_dsp(dsp):
+ """Return a new instance of a DistributionSourcePackage from the store."""
+ store = IStore(Person)
+ distribution_class = removeSecurityProxy(dsp.distribution.__class__)
+ distribution = store.get(distribution_class, dsp.distribution.id)
+ spn_class = removeSecurityProxy(dsp.sourcepackagename.__class__)
+ spn = store.get(spn_class, dsp.sourcepackagename.id)
+ return distribution.getSourcePackage(name=spn.name)
+
+
class TestPersonTeams(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -145,7 +161,7 @@
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')
+ self.factory.makeTeam(name='unrelated')
login_person(self.a_team.teamowner)
d_team.addMember(self.user, d_team.teamowner)
login_person(e_team.teamowner)
@@ -672,6 +688,17 @@
transaction.commit()
logout()
+ def _do_merge(self, from_person, to_person, reviewer=None):
+ # Perform the merge as the db user that will be used by the jobs.
+ transaction.commit()
+ reconnect_stores('IPersonMergeJobSource')
+ from_person = reload_object(from_person)
+ to_person = reload_object(to_person)
+ if reviewer is not None:
+ reviewer = reload_object(reviewer)
+ self.person_set.merge(from_person, to_person, reviewer=reviewer)
+ return from_person, to_person
+
def _get_testable_account(self, person, date_created, openid_identifier):
# Return a naked account with predictable attributes.
account = removeSecurityProxy(person.account)
@@ -689,7 +716,7 @@
person.account).openid_identifiers.any().identifier
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
self.assertEqual(
0,
removeSecurityProxy(duplicate.account).openid_identifiers.count())
@@ -715,7 +742,7 @@
person = self.factory.makePerson()
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
self.assertEqual([], duplicate.karma_category_caches)
self.assertEqual(0, duplicate.karma)
self.assertEqual(15, person.karma)
@@ -738,7 +765,7 @@
person = self.person_set.get(person.id)
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
self.assertEqual([], duplicate.karma_category_caches)
self.assertEqual(0, duplicate.karma)
self.assertEqual(28, person.karma)
@@ -752,14 +779,14 @@
removeSecurityProxy(duplicate).datecreated = oldest_date
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
self.assertEqual(oldest_date, person.datecreated)
def test_team_with_active_mailing_list_raises_error(self):
# A team with an active mailing list cannot be merged.
target_team = self.factory.makeTeam()
test_team = self.factory.makeTeam()
- mailing_list = self.factory.makeMailingList(
+ self.factory.makeMailingList(
test_team, test_team.teamowner)
self.assertRaises(
AssertionError, self.person_set.merge, test_team, target_team)
@@ -772,9 +799,11 @@
test_team, test_team.teamowner)
mailing_list.deactivate()
mailing_list.transitionToStatus(MailingListStatus.INACTIVE)
- self.person_set.merge(test_team, target_team, test_team.teamowner)
+ test_team, target_team = self._do_merge(
+ test_team, target_team, test_team.teamowner)
self.assertEqual(target_team, test_team.merged)
- self.assertEqual(MailingListStatus.PURGED, mailing_list.status)
+ self.assertEqual(
+ MailingListStatus.PURGED, test_team.mailing_list.status)
emails = getUtility(IEmailAddressSet).getByPerson(target_team).count()
self.assertEqual(0, emails)
@@ -787,7 +816,8 @@
mailing_list.deactivate()
mailing_list.transitionToStatus(MailingListStatus.INACTIVE)
mailing_list.purge()
- self.person_set.merge(test_team, target_team, test_team.teamowner)
+ test_team, target_team = self._do_merge(
+ test_team, target_team, test_team.teamowner)
self.assertEqual(target_team, test_team.merged)
def test_team_with_members(self):
@@ -797,7 +827,8 @@
former_member = self.factory.makePerson()
with person_logged_in(test_team.teamowner):
test_team.addMember(former_member, test_team.teamowner)
- self.person_set.merge(test_team, target_team, test_team.teamowner)
+ test_team, target_team = self._do_merge(
+ test_team, target_team, test_team.teamowner)
self.assertEqual(target_team, test_team.merged)
self.assertEqual([], list(former_member.super_teams))
@@ -807,7 +838,7 @@
test_team = self.factory.makeTeam()
target_team = self.factory.makeTeam()
login_person(test_team.teamowner)
- self.person_set.merge(test_team, target_team, test_team.teamowner)
+ self._do_merge(test_team, target_team, test_team.teamowner)
def test_team_with_super_teams(self):
# A team with superteams can be merged, but the memberships
@@ -817,7 +848,8 @@
target_team = self.factory.makeTeam()
login_person(test_team.teamowner)
test_team.join(super_team, test_team.teamowner)
- self.person_set.merge(test_team, target_team, test_team.teamowner)
+ test_team, target_team = self._do_merge(
+ test_team, target_team, test_team.teamowner)
self.assertEqual(target_team, test_team.merged)
self.assertEqual([], list(target_team.super_teams))
@@ -826,9 +858,10 @@
# are moved.
person = self.factory.makePerson()
branch = self.factory.makeBranch()
+ duplicate = branch.owner
self._do_premerge(branch.owner, person)
login_person(person)
- self.person_set.merge(branch.owner, person)
+ duplicate, person = self._do_merge(duplicate, person)
branches = person.getBranches()
self.assertEqual(1, branches.count())
@@ -839,9 +872,10 @@
from_branch = self.factory.makeBranch(name='foo', product=product)
to_branch = self.factory.makeBranch(name='foo', product=product)
mergee = to_branch.owner
- self._do_premerge(from_branch.owner, mergee)
+ duplicate = from_branch.owner
+ self._do_premerge(duplicate, mergee)
login_person(mergee)
- self.person_set.merge(from_branch.owner, mergee)
+ duplicate, mergee = self._do_merge(duplicate, mergee)
branches = [b.name for b in mergee.getBranches()]
self.assertEqual(2, len(branches))
self.assertEqual([u'foo', u'foo-1'], branches)
@@ -851,12 +885,13 @@
# moved.
person = self.factory.makePerson()
recipe = self.factory.makeSourcePackageRecipe()
+ duplicate = recipe.owner
# Delete the PPA, which is required for the merge to work.
- with person_logged_in(recipe.owner):
+ with person_logged_in(duplicate):
recipe.owner.archive.status = ArchiveStatus.DELETED
- self._do_premerge(recipe.owner, person)
+ self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(recipe.owner, person)
+ duplicate, person = self._do_merge(duplicate, person)
self.assertEqual(1, person.recipes.count())
def test_merge_with_duplicated_recipes(self):
@@ -866,20 +901,21 @@
name=u'foo', description=u'FROM')
merge_to = self.factory.makeSourcePackageRecipe(
name=u'foo', description=u'TO')
+ duplicate = merge_from.owner
mergee = merge_to.owner
# Delete merge_from's PPA, which is required for the merge to work.
with person_logged_in(merge_from.owner):
merge_from.owner.archive.status = ArchiveStatus.DELETED
self._do_premerge(merge_from.owner, mergee)
login_person(mergee)
- self.person_set.merge(merge_from.owner, merge_to.owner)
+ duplicate, mergee = self._do_merge(duplicate, mergee)
recipes = mergee.recipes
self.assertEqual(2, recipes.count())
descriptions = [r.description for r in recipes]
self.assertEqual([u'TO', u'FROM'], descriptions)
self.assertEqual(u'foo-1', recipes[1].name)
- def assertSubscriptionMerges(self, target):
+ def assertSubscriptionMerges(self, target, reloader=reload_object):
# Given a subscription target, we want to make sure that subscriptions
# that the duplicate person made are carried over to the merged
# account.
@@ -889,13 +925,15 @@
person = self.factory.makePerson()
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
# The merged person has the subscription, and the duplicate person
# does not.
+ target = reloader(target)
self.assertTrue(target.getSubscription(person) is not None)
self.assertTrue(target.getSubscription(duplicate) is None)
- def assertConflictingSubscriptionDeletes(self, target):
+ def assertConflictingSubscriptionDeletes(self, target,
+ reloader=reload_object):
# Given a subscription target, we want to make sure that subscriptions
# that the duplicate person made that conflict with existing
# subscriptions in the merged account are deleted.
@@ -910,7 +948,8 @@
u'a marker')
self._do_premerge(duplicate, person)
login_person(person)
- self.person_set.merge(duplicate, person)
+ duplicate, person = self._do_merge(duplicate, person)
+ target = reloader(target)
# The merged person still has the original subscription, as shown
# by the marker name.
self.assertEqual(
@@ -973,13 +1012,13 @@
def test_merge_with_sourcepackage_subscription(self):
# See comments in assertSubscriptionMerges.
- self.assertSubscriptionMerges(
- self.factory.makeDistributionSourcePackage())
+ dsp = self.factory.makeDistributionSourcePackage()
+ self.assertSubscriptionMerges(dsp, reloader=reload_dsp)
def test_merge_with_conflicting_sourcepackage_subscription(self):
# See comments in assertConflictingSubscriptionDeletes.
- self.assertConflictingSubscriptionDeletes(
- self.factory.makeDistributionSourcePackage())
+ dsp = self.factory.makeDistributionSourcePackage()
+ self.assertConflictingSubscriptionDeletes(dsp, reloader=reload_dsp)
def test_mergeAsync(self):
# mergeAsync() creates a new `PersonMergeJob`.
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py 2011-04-08 02:51:34 +0000
+++ lib/lp/registry/tests/test_team.py 2011-05-10 15:36:40 +0000
@@ -104,6 +104,16 @@
self.assertEqual(None, self.team.preferredemail)
self.assertEqual([list_address], self.getAllEmailAddresses())
+ def test_setContactAddress_with_purged_mailing_list_to_none(self):
+ # Purging a mailing list will delete the list address, but this was
+ # not always the case. The address will be deleted if it still exists.
+ list_address = self.createMailingListAndGetAddress()
+ naked_mailing_list = removeSecurityProxy(self.team.mailing_list)
+ naked_mailing_list.status = MailingListStatus.PURGED
+ self.team.setContactAddress(None)
+ self.assertEqual(None, self.team.preferredemail)
+ self.assertEqual([], self.getAllEmailAddresses())
+
def test_setContactAddress_after_purged_mailing_list_and_rename(self):
# This is the rare case where a list is purged for a team rename,
# then the contact address is set/unset sometime afterwards.
