launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02984
[Merge] lp:~sinzui/launchpad/person-merge-job-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/person-merge-job-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #728471 in Launchpad itself: "Person:+delete timeouts : Add sanity checks to mergeAsync"
https://bugs.launchpad.net/launchpad/+bug/728471
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/person-merge-job-0/+merge/53706
Add sanity checks to mergeAsync.
Launchpad bug: https://bugs.launchpad.net/bugs/728471
Pre-implementation: allenap, jcsackett
Test command: ./bin/test -vv \
-t webservice/xx-person.txt -t registry.tests.person.TestPerson.test \
-t TestPersonMergeJob
This issue is a continuation of bug 162510. The infrastructure is in place to
merge people an team async.
- Export is_merge_pending to the API.
- No check is performed to see if either merge user is already the
subject of a scheduled merge job.
--------------------------------------------------------------------
RULES
* Export is_merge_pending()
* Add a guard to mergeAsync() to ensure a merge is not already scheduled.
* Use is_merge_pending()
* revise is_merge_pending() to check *both* 'from' and 'to' because
users can be working with several merges.
* ADDENDUM: The test setup is incorrect. Fixing it reveals that DB
permissions are wrong.
QA
* Verify that is_merge_pending is exported:
lp = Launchpad.login_with(
'testing', 'https://api.launchpad.net/', version='devel')
sinzui = lp.people['sinzui']
print sinzui.is_merge_pending
LINT
database/schema/security.cfg
lib/lp/registry/interfaces/person.py
lib/lp/registry/interfaces/persontransferjob.py
lib/lp/registry/model/person.py
lib/lp/registry/model/persontransferjob.py
lib/lp/registry/stories/webservice/xx-person.txt
lib/lp/registry/tests/test_person.py
lib/lp/registry/tests/test_person_merge_job.py
IMPLEMENTATION
Exported is_merge_pending() and updated the docstring to apply to both
profiles.
lp/registry/stories/webservice/xx-person.txt
lp/registry/interfaces/person.py
Updated is_merge_pending() to meet the needs of the UI and added a guard to
mergeAsync(). Updated the tests and the DB permissions to ensure merge works
for teams and users.
database/schema/security.cfg
lp/registry/interfaces/person.py
lib/lp/registry/interfaces/persontransferjob.py
lib/lp/registry/model/person.py
lib/lp/registry/model/persontransferjob.py
lib/lp/registry/tests/test_person.py
lib/lp/registry/tests/test_person_merge_job.py
--
https://code.launchpad.net/~sinzui/launchpad/person-merge-job-0/+merge/53706
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/person-merge-job-0 into lp:launchpad.
=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg 2011-03-14 21:07:09 +0000
+++ database/schema/security.cfg 2011-03-16 21:18:36 +0000
@@ -2128,6 +2128,7 @@
public.nameblacklist = SELECT, UPDATE
public.oauthaccesstoken = SELECT, UPDATE
public.oauthrequesttoken = SELECT, UPDATE
+public.openididentifier = SELECT, UPDATE
public.packagebugsupervisor = SELECT, UPDATE
public.packagecopyrequest = SELECT, UPDATE
public.packagediff = SELECT, UPDATE
@@ -2137,7 +2138,7 @@
public.person = SELECT, UPDATE
public.personlanguage = SELECT, UPDATE
public.personlocation = SELECT, UPDATE
-public.personnotification = SELECT, UPDATE
+public.personnotification = SELECT, INSERT, UPDATE
public.personsettings = SELECT, UPDATE
public.persontransferjob = SELECT, UPDATE
public.pocomment = SELECT, UPDATE
=== 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-16 21:18:36 +0000
@@ -974,9 +974,9 @@
description=_("Private teams are visible only to "
"their members.")))
- is_merge_pending = Bool(
- title=_("Is this person due to be merged into another?"),
- required=False, default=False)
+ is_merge_pending = exported(Bool(
+ title=_("Is this person due to be merged with another?"),
+ required=False, default=False))
@invariant
def personCannotHaveIcon(person):
@@ -2160,9 +2160,10 @@
This schedules a call to `merge()` to happen outside of the current
context/request. The intention is that it is called soon after this
method is called but there is no guarantee of that, nor is that call
- guaranteed to succeed.
+ guaranteed to succeed. If either user is in a pending person merge
+ job, None is returned.
- :return: A `PersonMergeJob`.
+ :return: A `PersonMergeJob` or None.
"""
def merge(from_person, to_person):
=== modified file 'lib/lp/registry/interfaces/persontransferjob.py'
--- lib/lp/registry/interfaces/persontransferjob.py 2011-02-23 16:33:55 +0000
+++ lib/lp/registry/interfaces/persontransferjob.py 2011-03-16 21:18:36 +0000
@@ -100,17 +100,25 @@
"""An interface for acquiring IMembershipNotificationJobs."""
def create(from_person, to_person):
- """Create a new IMembershipNotificationJob."""
-
- def find(from_person=None, to_person=None):
+ """Create a new IMembershipNotificationJob.
+
+ None is returned if either the from_person or to_person are already
+ in a pending merge.
+ """
+
+ def find(from_person=None, to_person=None, any_person=False):
"""Finds pending merge jobs.
:param from_person: Match jobs on `from_person`, or `None` to ignore
`from_person`.
:param to_person: Match jobs on `to_person`, or `None` to ignore
`from_person`.
+ :param any_person: Match jobs on the `to_person` or the `from_person`
+ when both `to_person` and `from_person` are not None.
:return: A `ResultSet` yielding `IPersonMergeJob`.
If both `from_person` and `to_person` is supplied, only jobs where
- both match are returned.
+ both match are returned by default. When any_person is True and
+ `from_person` and `to_person` are supplied, jobs with either person
+ specified are returned.
"""
=== 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-16 21:18:36 +0000
@@ -2138,7 +2138,8 @@
def is_merge_pending(self):
"""See `IPublicPerson`."""
return not getUtility(
- IPersonMergeJobSource).find(from_person=self).is_empty()
+ IPersonMergeJobSource).find(
+ from_person=self, to_person=self, any_person=True).is_empty()
def visibilityConsistencyWarning(self, new_value):
"""Warning used when changing the team's visibility.
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py 2011-03-10 19:11:04 +0000
+++ lib/lp/registry/model/persontransferjob.py 2011-03-16 21:18:36 +0000
@@ -11,7 +11,10 @@
from lazr.delegates import delegates
import simplejson
-from storm.expr import And
+from storm.expr import (
+ And,
+ Or,
+ )
from storm.locals import (
Int,
Reference,
@@ -338,22 +341,28 @@
@classmethod
def create(cls, from_person, to_person):
"""See `IPersonMergeJobSource`."""
+ if from_person.is_merge_pending or to_person.is_merge_pending:
+ return None
return super(PersonMergeJob, cls).create(
minor_person=from_person, major_person=to_person, metadata={})
@classmethod
- def find(cls, from_person=None, to_person=None):
+ def find(cls, from_person=None, to_person=None, any_person=False):
"""See `IPersonMergeJobSource`."""
conditions = [
PersonTransferJob.job_type == cls.class_job_type,
PersonTransferJob.job_id == Job.id,
Job._status.is_in(Job.PENDING_STATUSES)]
+ arg_conditions = []
if from_person is not None:
- conditions.append(
+ arg_conditions.append(
PersonTransferJob.minor_person == from_person)
if to_person is not None:
- conditions.append(
+ arg_conditions.append(
PersonTransferJob.major_person == to_person)
+ if any_person and from_person is not None and to_person is not None:
+ arg_conditions = [Or(*arg_conditions)]
+ conditions.extend(arg_conditions)
return DecoratedResultSet(
IStore(PersonTransferJob).find(
PersonTransferJob, *conditions), cls)
=== modified file 'lib/lp/registry/stories/webservice/xx-person.txt'
--- lib/lp/registry/stories/webservice/xx-person.txt 2011-02-24 23:41:43 +0000
+++ lib/lp/registry/stories/webservice/xx-person.txt 2011-03-16 21:18:36 +0000
@@ -25,6 +25,7 @@
homepage_content: None
invited_members_collection_link: u'http://.../~salgado/invited_members'
irc_nicknames_collection_link: u'http://.../~salgado/irc_nicknames'
+ is_merge_pending: False
is_probationary: True
is_team: False
is_ubuntu_coc_signer: False
@@ -84,6 +85,7 @@
invited_members_collection_link:
u'http://.../~ubuntu-team/invited_members'
irc_nicknames_collection_link: u'http://.../~ubuntu-team/irc_nicknames'
+ is_merge_pending: False
is_probationary: False
is_team: True
is_ubuntu_coc_signer: False
=== 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-16 21:18:36 +0000
@@ -284,13 +284,21 @@
self.assertFalse(person.is_merge_pending)
def test_merge_pending(self):
- # is_merge_pending returns True when this person is the "from" person
- # of an active merge job.
+ # is_merge_pending returns True when this person is being merged with
+ # another person in an active merge job.
from_person = self.factory.makePerson()
to_person = self.factory.makePerson()
getUtility(IPersonSet).mergeAsync(from_person, to_person)
self.assertTrue(from_person.is_merge_pending)
- self.assertFalse(to_person.is_merge_pending)
+ self.assertTrue(to_person.is_merge_pending)
+
+ def test_mergeAsync_success(self):
+ # mergeAsync returns a job with the from and to persons.
+ from_person = self.factory.makePerson()
+ to_person = self.factory.makePerson()
+ job = getUtility(IPersonSet).mergeAsync(from_person, to_person)
+ self.assertEqual(from_person, job.from_person)
+ self.assertEqual(to_person, job.to_person)
def test_selfgenerated_bugnotifications_none_by_default(self):
# Default for new accounts is to not get any
=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
--- lib/lp/registry/tests/test_person_merge_job.py 2011-02-24 11:52:51 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py 2011-03-16 21:18:36 +0000
@@ -12,6 +12,11 @@
from zope.interface.verify import verifyObject
from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.interfaces.lpstorm import (
+ IMasterObject,
+ IStore,
+ )
+from canonical.launchpad.interfaces.emailaddress import EmailAddressStatus
from canonical.launchpad.scripts import log
from canonical.testing import DatabaseFunctionalLayer
from lp.registry.interfaces.persontransferjob import (
@@ -23,6 +28,7 @@
from lp.services.log.logger import BufferLogger
from lp.testing import (
run_script,
+ person_logged_in,
TestCaseWithFactory,
)
@@ -33,8 +39,10 @@
def setUp(self):
super(TestPersonMergeJob, self).setUp()
- self.from_person = self.factory.makePerson(name='void')
- self.to_person = self.factory.makeTeam(name='gestalt')
+ self.from_person = self.factory.makePerson(
+ name='void', email='void@xxxxxx')
+ self.to_person = self.factory.makePerson(
+ name='gestalt', email='gestalt@xxxxxx')
self.job_source = getUtility(IPersonMergeJobSource)
self.job = self.job_source.create(
from_person=self.from_person, to_person=self.to_person)
@@ -55,14 +63,28 @@
# Newly created jobs are enqueued.
self.assertEqual([self.job], list(self.job_source.iterReady()))
+ def test_create_job_already_exists(self):
+ # create returns None if either of the persons are already
+ # in a pending merge job.
+ duplicate_job = self.job_source.create(
+ from_person=self.from_person, to_person=self.to_person)
+ inverted_job = self.job_source.create(
+ from_person=self.to_person, to_person=self.from_person)
+ self.assertEqual(None, duplicate_job)
+ self.assertEqual(None, inverted_job)
+
+ def transfer_email(self):
+ # Reassign from_person's email address over to to_person because
+ # IPersonSet.merge() does not (yet) promise to do that.
+ from_email = IMasterObject(self.from_person.preferredemail)
+ removeSecurityProxy(from_email).personID = self.to_person.id
+ removeSecurityProxy(from_email).accountID = self.to_person.accountID
+ removeSecurityProxy(from_email).status = EmailAddressStatus.NEW
+ IStore(from_email).flush()
+
def test_run(self):
- # When run it merges from_person into to_person. First we need to
- # reassign from_person's email address over to to_person because
- # IPersonSet.merge() does not (yet) promise to do that.
- from_email = self.from_person.preferredemail
- removeSecurityProxy(from_email).personID = self.to_person.id
- removeSecurityProxy(from_email).accountID = self.to_person.accountID
-
+ # When run it merges from_person into to_person.
+ self.transfer_email()
logger = BufferLogger()
with log.use(logger):
self.job.run()
@@ -72,12 +94,18 @@
["DEBUG PersonMergeJob is about to merge ~void into ~gestalt",
"DEBUG PersonMergeJob has merged ~void into ~gestalt"],
logger.getLogBuffer().splitlines())
+ self.assertEqual(self.to_person, self.from_person.merged)
def test_smoke(self):
- # Smoke test, primarily for DB permissions.
- from_email = self.from_person.preferredemail
- removeSecurityProxy(from_email).personID = self.to_person.id
- removeSecurityProxy(from_email).accountID = self.to_person.accountID
+ # Smoke test, primarily for DB permissions need for users and teams.
+ # Check the oopses in /var/tmp/lperr.test if the person.merged
+ # assertion fails.
+ self.transfer_email()
+ to_team = self.factory.makeTeam(name='legion')
+ from_team = self.factory.makeTeam(name='null')
+ with person_logged_in(from_team.teamowner):
+ from_team.teamowner.leave(from_team)
+ self.job_source.create(from_person=from_team, to_person=to_team)
transaction.commit()
out, err, exit_code = run_script(
@@ -88,7 +116,9 @@
self.addDetail("stderr", Content(UTF8_TEXT, lambda: err))
self.assertEqual(0, exit_code)
+ IStore(self.from_person).invalidate()
self.assertEqual(self.to_person, self.from_person.merged)
+ self.assertEqual(to_team, from_team.merged)
def test_repr(self):
# A useful representation is available for PersonMergeJob instances.
@@ -113,6 +143,18 @@
self.assertEqual(
[], self.find(from_person=self.to_person))
+ def test_find_any_person(self):
+ # find() any_person looks for merge jobs with either from_person
+ # or to_person is true when both are specified.
+ self.assertEqual(
+ [self.job], self.find(
+ from_person=self.to_person, to_person=self.to_person,
+ any_person=True))
+ self.assertEqual(
+ [self.job], self.find(
+ from_person=self.from_person, to_person=self.from_person,
+ any_person=True))
+
def test_find_only_pending_or_running(self):
# find() only returns jobs that are pending.
for status in JobStatus.items:
