launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #10321
[Merge] lp:~abentley/launchpad/person-merge into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/person-merge into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #805544 in Launchpad itself: "persontransferjob lacks details for oopses"
https://bugs.launchpad.net/launchpad/+bug/805544
Bug #918742 in Launchpad itself: "PersonMergeJob does not specify its operation type"
https://bugs.launchpad.net/launchpad/+bug/918742
Bug #918745 in Launchpad itself: "PersonMergeJob sends emails to people who do not care"
https://bugs.launchpad.net/launchpad/+bug/918745
For more details, see:
https://code.launchpad.net/~abentley/launchpad/person-merge/+merge/116964
= Summary =
Fix bugs:
#918742: PersonMergeJob does not specify its operation type
#918745 PersonMergeJob sends emails to people who do not care
== Proposed fix ==
Describe the operation as "merging ~foo into ~bar".
Send oops messages to the person who requested the job.
== Pre-implementation notes ==
Discussed with sinzui
== LOC Rationale ==
I have a LOC credit of 1928
== Implementation details ==
In order to use the requester (Job.requester), callers must be adjusted to
specify requester.
Added getOperationDescription to IRunnableJob, since getErrorRecipients is already part of it.
== Tests ==
bin/test -t person.txt -t merge-people.txt -t test_personset -t test_person
== Demo and Q/A ==
Untestable, since I don't know of any way to get an oops from merging people.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/registry/tests/test_personset.py
lib/lp/registry/browser/peoplemerge.py
lib/lp/services/job/interfaces/job.py
lib/lp/services/verification/browser/logintoken.py
lib/lp/registry/model/persontransferjob.py
lib/lp/registry/tests/test_person_merge_job.py
lib/lp/registry/interfaces/person.py
lib/lp/registry/browser/tests/test_person.py
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_person.py
--
https://code.launchpad.net/~abentley/launchpad/person-merge/+merge/116964
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/person-merge into lp:launchpad.
=== modified file 'lib/lp/registry/browser/peoplemerge.py'
--- lib/lp/registry/browser/peoplemerge.py 2012-07-16 21:01:48 +0000
+++ lib/lp/registry/browser/peoplemerge.py 2012-07-26 21:00:45 +0000
@@ -148,7 +148,7 @@
naked_email.status = EmailAddressStatus.NEW
getUtility(IPersonSet).mergeAsync(
self.dupe_person, self.target_person, reviewer=self.user,
- delete=self.delete)
+ delete=self.delete, requester=self.user)
self.request.response.addInfoNotification(self.merge_message)
self.next_url = self.success_url
=== modified file 'lib/lp/registry/browser/tests/test_person.py'
--- lib/lp/registry/browser/tests/test_person.py 2012-07-12 00:46:07 +0000
+++ lib/lp/registry/browser/tests/test_person.py 2012-07-26 21:00:45 +0000
@@ -122,8 +122,10 @@
def test_isMergePending(self):
dupe_person = self.factory.makePerson(name='finch')
target_person = self.factory.makePerson()
+ requester = self.factory.makePerson()
job_source = getUtility(IPersonMergeJobSource)
- job_source.create(from_person=dupe_person, to_person=target_person)
+ job_source.create(from_person=dupe_person, to_person=target_person,
+ requester=requester)
view = create_initialized_view(dupe_person, name="+index")
notifications = view.request.response.notifications
message = 'Finch is queued to be merged in a few minutes.'
=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py 2012-07-12 01:16:04 +0000
+++ lib/lp/registry/interfaces/person.py 2012-07-26 21:00:45 +0000
@@ -2329,7 +2329,8 @@
address.
"""
- def mergeAsync(from_person, to_person, reviewer=None, delete=False):
+ def mergeAsync(from_person, to_person, requester, reviewer=None,
+ delete=False):
"""Merge a person/team into another asynchronously.
This schedules a call to `merge()` to happen outside of the current
@@ -2340,6 +2341,8 @@
:param from_person: An IPerson or ITeam that is a duplicate.
:param to_person: An IPerson or ITeam that is a master.
+ :param requester: The IPerson who requested the merge. Should not be
+ an ITeam.
:param reviewer: An IPerson who approved the ITeam merger.
:param delete: The merge is really a deletion.
:return: A `PersonMergeJob` or None.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2012-07-25 15:29:06 +0000
+++ lib/lp/registry/model/person.py 2012-07-26 21:00:45 +0000
@@ -4157,11 +4157,12 @@
naked_from_team.retractTeamMembership(team, reviewer)
IStore(from_team).flush()
- def mergeAsync(self, from_person, to_person, reviewer=None, delete=False):
+ def mergeAsync(self, from_person, to_person, requester, reviewer=None,
+ delete=False):
"""See `IPersonSet`."""
return getUtility(IPersonMergeJobSource).create(
- from_person=from_person, to_person=to_person, reviewer=reviewer,
- delete=delete)
+ from_person=from_person, to_person=to_person, requester=requester,
+ reviewer=reviewer, delete=delete)
def delete(self, from_person, reviewer):
"""See `IPersonSet`."""
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py 2012-07-07 19:30:24 +0000
+++ lib/lp/registry/model/persontransferjob.py 2012-07-26 21:00:45 +0000
@@ -96,7 +96,8 @@
def metadata(self):
return simplejson.loads(self._json_data)
- def __init__(self, minor_person, major_person, job_type, metadata):
+ def __init__(self, minor_person, major_person, job_type, metadata,
+ requester=None):
"""Constructor.
:param minor_person: The person or team being added to or removed
@@ -108,7 +109,7 @@
dict.
"""
super(PersonTransferJob, self).__init__()
- self.job = Job()
+ self.job = Job(requester=requester)
self.job_type = job_type
self.major_person = major_person
self.minor_person = minor_person
@@ -140,7 +141,7 @@
self.context = job
@classmethod
- def create(cls, minor_person, major_person, metadata):
+ def create(cls, minor_person, major_person, metadata, requester=None):
"""See `IPersonTransferJob`."""
if not IPerson.providedBy(minor_person):
raise TypeError("minor_person must be IPerson: %s"
@@ -152,7 +153,8 @@
minor_person=minor_person,
major_person=major_person,
job_type=cls.class_job_type,
- metadata=metadata)
+ metadata=metadata,
+ requester=requester)
derived = cls(job)
derived.celeryRunOnCommit()
return derived
@@ -356,7 +358,8 @@
config = config.IPersonMergeJobSource
@classmethod
- def create(cls, from_person, to_person, reviewer=None, delete=False):
+ def create(cls, from_person, to_person, requester, reviewer=None,
+ delete=False):
"""See `IPersonMergeJobSource`."""
if (from_person.isMergePending() or
(not delete and to_person.isMergePending())):
@@ -372,7 +375,7 @@
to_person = getUtility(ILaunchpadCelebrities).registry_experts
return super(PersonMergeJob, cls).create(
minor_person=from_person, major_person=to_person,
- metadata=metadata)
+ metadata=metadata, requester=requester)
@classmethod
def find(cls, from_person=None, to_person=None, any_person=False):
@@ -418,10 +421,7 @@
def getErrorRecipients(self):
"""See `IPersonMergeJob`."""
- if self.to_person.is_team:
- return self.to_person.getTeamAdminsEmailAddresses()
- else:
- return [format_address_for_person(self.to_person)]
+ return [format_address_for_person(self.requester)]
def run(self):
"""Perform the merge."""
@@ -456,3 +456,7 @@
"<{self.__class__.__name__} to merge "
"~{self.from_person.name} into ~{self.to_person.name}; "
"status={self.job.status}>").format(self=self)
+
+ def getOperationDescription(self):
+ return ('merging ~%s into ~%s' %
+ (self.from_person.name, self.to_person.name))
=== modified file 'lib/lp/registry/tests/test_person.py'
--- lib/lp/registry/tests/test_person.py 2012-07-07 19:30:24 +0000
+++ lib/lp/registry/tests/test_person.py 2012-07-26 21:00:45 +0000
@@ -301,17 +301,22 @@
# another person in an active merge job.
from_person = self.factory.makePerson()
to_person = self.factory.makePerson()
- getUtility(IPersonSet).mergeAsync(from_person, to_person)
+ requester = self.factory.makePerson()
+ getUtility(IPersonSet).mergeAsync(from_person, to_person, requester)
self.assertTrue(from_person.isMergePending())
self.assertFalse(to_person.isMergePending())
def test_mergeAsync_success(self):
- # mergeAsync returns a job with the from and to persons.
+ # mergeAsync returns a job with the from and to persons, and the
+ # requester.
from_person = self.factory.makePerson()
to_person = self.factory.makePerson()
- job = getUtility(IPersonSet).mergeAsync(from_person, to_person)
+ requester = self.factory.makePerson()
+ person_set = getUtility(IPersonSet)
+ job = person_set.mergeAsync(from_person, to_person, requester)
self.assertEqual(from_person, job.from_person)
self.assertEqual(to_person, job.to_person)
+ self.assertEqual(requester, job.requester)
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 2012-04-25 18:25:15 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py 2012-07-26 21:00:45 +0000
@@ -46,8 +46,9 @@
"""
from_person = factory.makePerson(name='void')
to_person = factory.makePerson(name='gestalt')
+ requester = factory.makePerson(name='requester')
return getUtility(IPersonMergeJobSource).create(
- from_person=from_person, to_person=to_person)
+ from_person=from_person, to_person=to_person, requester=requester)
def transfer_email(job):
@@ -72,6 +73,7 @@
self.job = create_job(self.factory)
self.from_person = self.job.from_person
self.to_person = self.job.to_person
+ self.requester = self.job.requester
def test_interface(self):
# PersonMergeJob implements IPersonMergeJob.
@@ -85,21 +87,11 @@
self.assertEqual(self.to_person, self.job.major_person)
self.assertEqual({'delete': False}, self.job.metadata)
- def test_getErrorRecipients_user(self):
+ def test_getErrorRecipients(self):
# The to_person is the recipient.
- email_id = format_address_for_person(self.to_person)
+ email_id = format_address_for_person(self.requester)
self.assertEqual([email_id], self.job.getErrorRecipients())
- def test_getErrorRecipients_team(self):
- # The to_person admins are the recipients.
- to_team = self.factory.makeTeam()
- from_team = self.factory.makeTeam()
- job = self.job_source.create(
- from_person=from_team, to_person=to_team,
- reviewer=to_team.teamowner)
- self.assertEqual(
- to_team.getTeamAdminsEmailAddresses(), job.getErrorRecipients())
-
def test_enqueue(self):
# Newly created jobs are enqueued.
self.assertEqual([self.job], list(self.job_source.iterReady()))
@@ -108,9 +100,11 @@
# 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)
+ from_person=self.from_person, to_person=self.to_person,
+ requester=self.requester)
inverted_job = self.job_source.create(
- from_person=self.to_person, to_person=self.from_person)
+ from_person=self.to_person, to_person=self.from_person,
+ requester=self.requester)
self.assertEqual(None, duplicate_job)
self.assertEqual(None, inverted_job)
@@ -144,7 +138,7 @@
from_team.teamowner.leave(from_team)
self.job_source.create(
from_person=from_team, to_person=to_team,
- reviewer=from_team.teamowner)
+ reviewer=from_team.teamowner, requester=self.factory.makePerson())
transaction.commit()
out, err, exit_code = run_script(
@@ -165,6 +159,10 @@
"<PersonMergeJob to merge ~void into ~gestalt; status=Waiting>",
repr(self.job))
+ def test_getOperationDescription(self):
+ self.assertEqual('merging ~void into ~gestalt',
+ self.job.getOperationDescription())
+
def find(self, **kwargs):
return list(self.job_source.find(**kwargs))
@@ -203,6 +201,14 @@
else:
self.assertEqual([], self.find())
+ def test_create_requester(self):
+ requester = self.factory.makePerson()
+ from_person = self.factory.makePerson()
+ to_person = self.factory.makePerson()
+ job = getUtility(IPersonMergeJobSource).create(
+ from_person, to_person, requester)
+ self.assertEqual(requester, job.requester)
+
class TestViaCelery(TestCaseWithFactory):
"""Test that PersonMergeJob runs under Celery."""
=== modified file 'lib/lp/registry/tests/test_personset.py'
--- lib/lp/registry/tests/test_personset.py 2012-07-24 15:50:19 +0000
+++ lib/lp/registry/tests/test_personset.py 2012-07-26 21:00:45 +0000
@@ -665,9 +665,10 @@
from_person = self.factory.makePerson()
to_person = self.factory.makePerson()
login_person(from_person)
- job = self.person_set.mergeAsync(from_person, to_person)
+ job = self.person_set.mergeAsync(from_person, to_person, from_person)
self.assertEqual(from_person, job.from_person)
self.assertEqual(to_person, job.to_person)
+ self.assertEqual(from_person, job.requester)
def test_mergeProposedInvitedTeamMembership(self):
# Proposed and invited memberships are declined.
=== modified file 'lib/lp/services/job/interfaces/job.py'
--- lib/lp/services/job/interfaces/job.py 2012-04-12 15:28:10 +0000
+++ lib/lp/services/job/interfaces/job.py 2012-07-26 21:00:45 +0000
@@ -156,6 +156,13 @@
These vars should help determine why the jobs OOPsed.
"""
+ def getOperationDescription():
+ """Describe the operation being performed, for use in oops emails.
+
+ Should grammatically fit the phrase "error while FOO", e.g. "error
+ while sending mail."
+ """
+
user_error_types = Attribute(
'A tuple of exception classes which result from user error.')
=== modified file 'lib/lp/services/verification/browser/logintoken.py'
--- lib/lp/services/verification/browser/logintoken.py 2012-06-29 08:40:05 +0000
+++ lib/lp/services/verification/browser/logintoken.py 2012-07-26 21:00:45 +0000
@@ -600,7 +600,7 @@
self.mergeCompleted = False
return
getUtility(IPersonSet).mergeAsync(
- self.dupe, requester, reviewer=requester)
+ self.dupe, requester, requester, reviewer=requester)
merge_message = _(
'A merge is queued and is expected to complete in a few minutes.')
self.request.response.addInfoNotification(merge_message)
Follow ups
