launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03040
[Merge] lp:~sinzui/launchpad/person-merge-job-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/person-merge-job-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #740559 in Launchpad itself: "Update PersonNotification to support teams"
https://bugs.launchpad.net/launchpad/+bug/740559
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/person-merge-job-1/+merge/54440
PersonNotification supports teams.
Launchpad bug: https://bugs.launchpad.net/bugs/728471
Pre-implementation: jcsackett
Test command: ./bin/test -vv \
-t person-merge -t test_person_merge_job
-t test_personnotification -t TestTeamGetTeamAdminsEmailAddresses
Before bug 736421 can be addressed, there are several infrastructure issues
that must be solved first:
* Person.merge needs send success emails to teams.
* PersonNotification.send() must support teams.
* Team.getTeamAdminsEmailAddresses() must guarantee for admins.
* The job.getErrorRecipients must return the team admin addresses.
--------------------------------------------------------------------
RULES
* Update Team.getTeamAdminsEmailAddresses() to recurse through
team owner to locate team admins with email addresses.
* Update PersonNotification.send() to send to team admins if the
person is a team.
* Consider a property that can be tested because the method
and PersonNotificationManager are testing person.preferredemail
* Update Person.merge to send to teams
* Add a team email template.
* Refine PersonMergeJob.getErrorRecipients to provide the user or
team email addresses.
QA
* Merge a team with a team you admin on qastaging.
* Run cronscripts/send-person-notifications.py on qastaging.
* Verify a message to you is in the staging sent folder.
LINT
lib/lp/registry/doc/person-merge.txt
lib/lp/registry/emailtemplates/team-merged.txt
lib/lp/registry/interfaces/personnotification.py
lib/lp/registry/interfaces/persontransferjob.py
lib/lp/registry/model/person.py
lib/lp/registry/model/personnotification.py
lib/lp/registry/model/persontransferjob.py
lib/lp/registry/scripts/personnotification.py
lib/lp/registry/tests/test_person_merge_job.py
lib/lp/registry/tests/test_personnotification.py
lib/lp/registry/tests/test_team.py
IMPLEMENTATION
Updated Team.getTeamAdminsEmailAddresses() to recurse through
team owner to locate team admins with email addresses. This applies the
same kind of loop used in contact-this-teams-owner, but uses the
getDirectTeamAdmins() method instead. Then solves cases where the immediate
admin is a team without a contact address.
lib/lp/registry/model/person.py
lib/lp/registry/tests/test_team.py
Updated PersonNotification.send() to send to team admins when the
person is a team. Added to_addresses and can_send properties so that
the send method and PersonNotificationManager can handles teams as easily
as users. The to_addresses property uses the revised
Team.getTeamAdminsEmailAddresses() method.
lib/lp/registry/interfaces/personnotification.py
lib/lp/registry/model/personnotification.py
lib/lp/registry/scripts/personnotification.py
lib/lp/registry/tests/test_personnotification.py
Refined PersonMergeJob.getErrorRecipients to provide the user or
team email addresses. I had to explicitly define the interfaces because
of some nonsense in decorates() that is used to construct the PersonMergeJob.
The method uses the revised Team.getTeamAdminsEmailAddresses() method.
lib/lp/registry/interfaces/persontransferjob.py
lib/lp/registry/model/persontransferjob.py
lib/lp/registry/tests/test_person_merge_job.py
Send merge success emails to the team. I added an email template and updated
the existing test to show it was sent. This implementation is not ideal; I
know that ~registry admins do not care to get team delete emails. I will
address this in my next branch which deal with integrating premerge/delete
logic into the model.
lib/lp/registry/emailtemplates/team-merged.txt
lib/lp/registry/model/person.py
lib/lp/registry/doc/person-merge.txt
--
https://code.launchpad.net/~sinzui/launchpad/person-merge-job-1/+merge/54440
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/person-merge-job-1 into lp:launchpad.
=== modified file 'lib/lp/registry/doc/person-merge.txt'
--- lib/lp/registry/doc/person-merge.txt 2011-03-17 20:17:37 +0000
+++ lib/lp/registry/doc/person-merge.txt 2011-03-22 23:07:39 +0000
@@ -484,6 +484,21 @@
>>> list(IPollSubset(landscape).getAll())
[]
+An email is sent to the team confirming the teams were merged.
+
+ >>> notifications = notification_set.getNotificationsToSend()
+ >>> notifications.count()
+ 2
+ >>> notification = notifications[-1]
+ >>> print notification.person.name
+ landscape-developers
+ >>> print notification.subject
+ Launchpad teams merged
+ >>> print notification.body
+ The Launchpad team named 'test-team-merged' was merged into the team
+ named 'landscape-developers'. You can review the changes at:
+ https://launchpad.net/~landscape-developers...
+
A person with a PPA can't be merged.
>>> person_with_ppa = factory.makePerson()
=== added file 'lib/lp/registry/emailtemplates/team-merged.txt'
--- lib/lp/registry/emailtemplates/team-merged.txt 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/emailtemplates/team-merged.txt 2011-03-22 23:07:39 +0000
@@ -0,0 +1,8 @@
+
+The Launchpad team named '%(dupename)s' was merged into the team
+named '%(person)s'. You can review the changes at:
+
+ https://launchpad.net/~%(person)s
+
+Regards,
+Launchpad
=== modified file 'lib/lp/registry/interfaces/personnotification.py'
--- lib/lp/registry/interfaces/personnotification.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/interfaces/personnotification.py 2011-03-22 23:07:39 +0000
@@ -11,7 +11,10 @@
'IPersonNotificationSet',
]
-from zope.interface import Interface
+from zope.interface import (
+ Attribute,
+ Interface,
+ )
from zope.schema import (
Datetime,
Object,
@@ -38,6 +41,11 @@
body = Text(title=_("Notification body."))
subject = TextLine(title=_("Notification subject."))
+ can_send = Attribute("Can the notification be sent?")
+
+ to_addresses = Attribute(
+ "The list of addresses to send the notification to.")
+
def destroySelf():
"""Delete this notification."""
=== modified file 'lib/lp/registry/interfaces/persontransferjob.py'
--- lib/lp/registry/interfaces/persontransferjob.py 2011-03-14 20:44:49 +0000
+++ lib/lp/registry/interfaces/persontransferjob.py 2011-03-22 23:07:39 +0000
@@ -95,6 +95,9 @@
vocabulary='ValidPersonOrTeam',
required=True)
+ def getErrorRecipients(self):
+ """See `BaseRunnableJob`."""
+
class IPersonMergeJobSource(IJobSource):
"""An interface for acquiring IMembershipNotificationJobs."""
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2011-03-22 04:40:12 +0000
+++ lib/lp/registry/model/person.py 2011-03-22 23:07:39 +0000
@@ -1417,8 +1417,15 @@
"""See `IPerson`."""
assert self.is_team
to_addrs = set()
- for person in self.getDirectAdministrators():
- to_addrs.update(get_contact_email_addresses(person))
+ team_or_person = self
+ # Recurse through team ownership until an email address is found.
+ while len(to_addrs) == 0:
+ if team_or_person.is_team:
+ for admin in team_or_person.getDirectAdministrators():
+ to_addrs.update(get_contact_email_addresses(admin))
+ team_or_person = team_or_person.teamowner
+ else:
+ to_addrs.update(get_contact_email_addresses(team_or_person))
return sorted(to_addrs)
def addMember(self, person, reviewer, comment=None, force_team_add=False,
@@ -4041,16 +4048,20 @@
""" % sqlvalues(to_person.accountID, from_person.accountID))
# Inform the user of the merge changes.
- if not to_person.isTeam():
+ if to_person.isTeam():
+ mail_text = get_email_template(
+ 'team-merged.txt', app='registry')
+ subject = 'Launchpad teams merged'
+ else:
mail_text = get_email_template(
'person-merged.txt', app='registry')
- mail_text = mail_text % {
- 'dupename': from_person.name,
- 'person': to_person.name,
- }
subject = 'Launchpad accounts merged'
- getUtility(IPersonNotificationSet).addNotification(
- to_person, subject, mail_text)
+ mail_text = mail_text % {
+ 'dupename': from_person.name,
+ 'person': to_person.name,
+ }
+ getUtility(IPersonNotificationSet).addNotification(
+ to_person, subject, mail_text)
def getValidPersons(self, persons):
"""See `IPersonSet.`"""
=== modified file 'lib/lp/registry/model/personnotification.py'
--- lib/lp/registry/model/personnotification.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/model/personnotification.py 2011-03-22 23:07:39 +0000
@@ -29,6 +29,7 @@
IPersonNotification,
IPersonNotificationSet,
)
+from lp.services.propertycache import cachedproperty
from lp.services.mail.sendmail import (
format_address,
simple_sendmail,
@@ -45,14 +46,29 @@
body = StringCol(notNull=True)
subject = StringCol(notNull=True)
+ @cachedproperty
+ def to_addresses(self):
+ """See `IPersonNotification`."""
+ if self.person.is_team:
+ return self.person.getTeamAdminsEmailAddresses()
+ elif self.person.preferredemail is None:
+ return []
+ else:
+ return [format_address(
+ self.person.displayname, self.person.preferredemail.email)]
+
+ @property
+ def can_send(self):
+ """See `IPersonNotification`."""
+ return len(self.to_addresses) > 0
+
def send(self):
"""See `IPersonNotification`."""
- assert self.person.preferredemail is not None, (
- "Can't send a notification to a person without an email.")
+ if not self.can_send:
+ raise AssertionError(
+ "Can't send a notification to a person without an email.")
from_addr = config.canonical.bounce_address
- to_addr = format_address(
- self.person.displayname, self.person.preferredemail.email)
- simple_sendmail(from_addr, to_addr, self.subject, self.body)
+ simple_sendmail(from_addr, self.to_addresses, self.subject, self.body)
self.date_emailed = datetime.now(pytz.timezone('UTC'))
@@ -73,4 +89,3 @@
"""See `IPersonNotificationSet`."""
return PersonNotification.select(
'date_created < %s' % sqlvalues(time_limit))
-
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py 2011-03-14 20:44:49 +0000
+++ lib/lp/registry/model/persontransferjob.py 2011-03-22 23:07:39 +0000
@@ -64,6 +64,7 @@
from lp.services.database.stormbase import StormBase
from lp.services.job.model.job import Job
from lp.services.job.runner import BaseRunnableJob
+from lp.services.mail.sendmail import format_address_for_person
class PersonTransferJob(StormBase):
@@ -381,6 +382,13 @@
def log_name(self):
return self.__class__.__name__
+ 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)]
+
def run(self):
"""Perform the merge."""
from_person_name = self.from_person.name
@@ -390,7 +398,6 @@
log.debug(
"%s is about to merge ~%s into ~%s", self.log_name,
from_person_name, to_person_name)
-
getUtility(IPersonSet).merge(
from_person=self.from_person, to_person=self.to_person)
=== modified file 'lib/lp/registry/scripts/personnotification.py'
--- lib/lp/registry/scripts/personnotification.py 2010-12-08 17:22:23 +0000
+++ lib/lp/registry/scripts/personnotification.py 2011-03-22 23:07:39 +0000
@@ -38,14 +38,15 @@
'%d notification(s) to send.' % pending_notifications.count())
for notification in pending_notifications:
person = notification.person
- if person.preferredemail is None:
+ if not notification.can_send:
unsent_notifications.append(notification)
self.logger.info(
"%s has no email address." % person.name)
continue
self.logger.info(
"Sending notification to %s <%s>."
- % (person.name, removeSecurityProxy(person).preferredemail.email))
+ % (person.name,
+ removeSecurityProxy(person).preferredemail.email))
notification.send()
notifications_sent = True
# Commit after each email sent, so that we won't re-mail the
=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
--- lib/lp/registry/tests/test_person_merge_job.py 2011-03-15 20:56:37 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py 2011-03-22 23:07:39 +0000
@@ -26,6 +26,7 @@
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.log.logger import BufferLogger
+from lp.services.mail.sendmail import format_address_for_person
from lp.testing import (
run_script,
person_logged_in,
@@ -39,10 +40,8 @@
def setUp(self):
super(TestPersonMergeJob, self).setUp()
- self.from_person = self.factory.makePerson(
- name='void', email='void@xxxxxx')
- self.to_person = self.factory.makePerson(
- name='gestalt', email='gestalt@xxxxxx')
+ self.from_person = self.factory.makePerson(name='void')
+ self.to_person = self.factory.makePerson(name='gestalt')
self.job_source = getUtility(IPersonMergeJobSource)
self.job = self.job_source.create(
from_person=self.from_person, to_person=self.to_person)
@@ -59,6 +58,19 @@
self.assertEqual(self.to_person, self.job.major_person)
self.assertEqual({}, self.job.metadata)
+ def test_getErrorRecipients_user(self):
+ # The to_person is the recipient.
+ email_id = format_address_for_person(self.to_person)
+ 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)
+ 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()))
=== modified file 'lib/lp/registry/tests/test_personnotification.py'
--- lib/lp/registry/tests/test_personnotification.py 2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_personnotification.py 2011-03-22 23:07:39 +0000
@@ -27,6 +27,43 @@
from lp.testing import TestCaseWithFactory
+class TestPersonNotification(TestCaseWithFactory):
+ """Tests for PersonNotification."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestPersonNotificationManager, self).setUp()
+ self.notification_set = getUtility(IPersonNotificationSet)
+
+ def test_to_addresses_user(self):
+ # The to_addresses list is the user's preferred email address.
+ user = self.factory.makePerson()
+ notification = self.notification_set.addNotification(
+ user, 'subject', 'body')
+ self.assertEqual(
+ [user.preferredemail.email], notification.to_addresses)
+ self.assertTrue(notification.can_send)
+
+ def test_to_addresses_user_without_address(self):
+ # The to_addresses list is empty and the notification cannot be sent.
+ user = self.factory.makePerson()
+ user.setPreferredEmail(None)
+ notification = self.notification_set.addNotification(
+ user, 'subject', 'body')
+ self.assertEqual([], notification.to_addresses)
+ self.assertFalse(notification.can_send)
+
+ def test_to_addresses_team(self):
+ # The to_addresses list is the team admin addresses.
+ team = self.factory.makeTeam()
+ notification = self.notification_set.addNotification(
+ team, 'subject', 'body')
+ self.assertEqual(
+ [team.teamowner.preferredemail.email], notification.to_addresses)
+ self.assertTrue(notification.can_send)
+
+
class TestPersonNotificationManager(TestCaseWithFactory):
"""Tests for the PersonNotificationManager use in scripts."""
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py 2011-03-17 18:35:30 +0000
+++ lib/lp/registry/tests/test_team.py 2011-03-22 23:07:39 +0000
@@ -30,6 +30,7 @@
TeamMembershipRenewalPolicy,
TeamSubscriptionPolicy,
)
+from lp.registry.interfaces.teammembership import TeamMembershipStatus
from lp.registry.model.persontransferjob import PersonTransferJob
from lp.soyuz.enums import ArchiveStatus
from lp.testing import (
@@ -124,6 +125,72 @@
self.assertEqual([], self.getAllEmailAddresses())
+class TestTeamGetTeamAdminsEmailAddresses(TestCaseWithFactory):
+ """Test the rules of IPerson.getTeamAdminsEmailAddresses()."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestTeamGetTeamAdminsEmailAddresses, self).setUp()
+ self.team = self.factory.makeTeam(name='finch')
+ self.address = self.factory.makeEmail('team@xxxxxx', self.team)
+ login_celebrity('admin')
+
+ def test_team_direct_owner(self):
+ # The team owner's email address is only email address.
+ email = self.team.teamowner.preferredemail.email
+ self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
+
+ def test_team_direct_owner_not_member(self):
+ # The team owner's email address is only email address, even when
+ # the owner is not a team member.
+ email = self.team.teamowner.preferredemail.email
+ self.team.teamowner.leave(self.team)
+ self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
+
+ def test_team_direct_admin(self):
+ # The team's owner and direct admins provide the email addresses.
+ admin = self.factory.makePerson()
+ self.team.addMember(admin, self.team.teamowner)
+ for membership in self.team.member_memberships:
+ membership.setStatus(
+ TeamMembershipStatus.ADMIN, self.team.teamowner)
+ email_1 = self.team.teamowner.preferredemail.email
+ email_2 = admin.preferredemail.email
+ self.assertEqual(
+ [email_1, email_2], self.team.getTeamAdminsEmailAddresses())
+
+ def test_team_indirect_owner(self):
+ # The team owner's of the owning team is only email address.
+ owning_team = self.factory.makeTeam()
+ member = self.team.teamowner
+ self.team.teamowner = owning_team
+ for membership in self.team.member_memberships:
+ membership.setStatus(
+ TeamMembershipStatus.APPROVED, owning_team.teamowner)
+ email = owning_team.teamowner.preferredemail.email
+ self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
+
+ def test_team_indirect_admin(self):
+ # The team owner and admin of the owning team provide the
+ # email addresses.
+ owning_team = self.factory.makeTeam()
+ member = self.team.teamowner
+ self.team.teamowner = owning_team
+ for membership in self.team.member_memberships:
+ membership.setStatus(
+ TeamMembershipStatus.APPROVED, owning_team.teamowner)
+ admin = self.factory.makePerson()
+ owning_team.addMember(admin, owning_team.teamowner)
+ for membership in owning_team.member_memberships:
+ membership.setStatus(
+ TeamMembershipStatus.ADMIN, owning_team.teamowner)
+ email_1 = owning_team.teamowner.preferredemail.email
+ email_2 = admin.preferredemail.email
+ self.assertEqual(
+ [email_1, email_2], self.team.getTeamAdminsEmailAddresses())
+
+
class TestDefaultRenewalPeriodIsRequiredForSomeTeams(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
