Merge lp:~sinzui/launchpad/person-merge-job-1 into lp:launchpad

[Curtis Hovey]

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

[Robert Collins (lifeless)]

75 + def getErrorRecipients(self):
76 + """See `BaseRunnableJob`."""
77 +
78

thats a little odd - if the method is on the base interface, its redundant, and if its not on the base interface, a reference to that is pointless.

+ team_or_person = self
92 + # Recurse through team ownership until an email address is found.
93 + while len(to_addrs) == 0:
94 + if team_or_person.is_team:
95 + for admin in team_or_person.getDirectAdministrators():
96 + to_addrs.update(get_contact_email_addresses(admin))
97 + team_or_person = team_or_person.teamowner
98 + else:
99 + to_addrs.update(get_contact_email_addresses(team_or_person))

confused me a little. And once I wasn't confused I concluded it was buggy. Its buggy because it will only notify one administrator. (consider this: the owner is a team with two members. one member is a person, one member is another team with people in it.

The for loop will on the first pass, get one email address in to_addrs, and then go to the *owning team* of the owning team, and terminate. What should happen is that all the *members* of the owning team get mailed - we don't want to traverse owner of owner, just owner. (I think. Am I wrong?)

Perhaps I am wrong. Perhaps it actually says 'if the owning team is totally uncontactable, try harder'. Which seems like a pathological case - you'd need an empty team with only owners as an owning team. But we should handle it... perhaps by fixing emailPeople ? ['if you contact a team with no members the owner is contacted']

In fact, the more I think about it, the more that makes sense: emailPeople is wrong.

The rest seems uncontentious.

review: Needs Information

[Curtis Hovey (sinzui)]

Hi Robert.

The believe oddness of the getErrorRecipients() interface is caused by PersonTransferJobDerived which breaks the permission chain.

{{{
class PersonTransferJobDerived(BaseRunnableJob):
    delegates(IPersonTransferJob)
}}}

Thank you for looking at the loop to get the the admin email addresses. I see it is repeating the problems of "contact this team" that I wanted to avoid. We want to send an email to the most immediate members who know about the team, the action, and can do something about it. I think there are two issues in the loop.

1 getDirectAdministrators() does not (get direct administrators). I own several teams that I am not a member of. I am *not* an admin, but this method is injecting the team owner into the list. I can use team.adminmembers to get just the members with the status ADMIN. This method is repeating the mistakes of the past where a user want to contact a team, but we walk the hierarchy of owner instead. Mark gets email about teams that are 3 and 4 teams removed from him :( This implementation was chosen because send emails to the individual members of the owner team fails when the owners do not have email address (team). This is a rare case, but it affects prominent teams.

2 If I had written a test where one of the admins was a team without an email address, I would have noticed that the rules do not traverse admin teams as I want. The code needs to locate an email address for the admin team. Choosing to get the contact address of the members will fail for the same reason as owner described above. The code needs to traverse a hierarchy to get a person or team with an email address.

Lets discuss this when you have time later today. I will work on the next branch which is independent of this branch.

[Curtis Hovey (sinzui)]

Hi Robert
On Wed, 2011-03-23 at 01:55 +0000, Robert Collins wrote:
> + team_or_person = self
> 92 + # Recurse through team ownership until an email
> address is found.
> 93 + while len(to_addrs) == 0:
> 94 + if team_or_person.is_team:
> 95 + for admin in
> team_or_person.getDirectAdministrators():
> 96 +
> to_addrs.update(get_contact_email_addresses(admin))
> 97 + team_or_person = team_or_person.teamowner
> 98 + else:
> 99 +
> to_addrs.update(get_contact_email_addresses(team_or_person))

I switch to team.adminmembers to avoid special casing the team owner.
I removed the recursive loop to find an email address.
The code is very simple now, mostly because
get_contact_email_addresses(team) knows how to handle a team without a
contact address.

I updated the tests. Notably, I added a test to show there can be no
contact address when there are no admins. I added a test to verify the
case of an admining team without a contact address. I think this case
was missing in my first effort. There were no surprises that it worked
since get_contact_email_addresses is smart.

I have attached a diff, but you might want to read def
getTeamAdminsEmailAddresses and TestTeamGetTeamAdminsEmailAddresses to
verify the whole method is consistent.

[Robert Collins (lifeless)]

 def test_admins_are_teams_with_preferred_email_addresses(self):
106 + # The team's admin is a team without a contact address.
107 + # The admin team members provide the email addresses.
108 + admin_team = self.setUpAdminingTeam(self.team)
109 + admin_team.setContactAddress(
110 + self.factory.makeEmail('<email address hidden>', admin_team))
111 + self.assertEqual(
112 + ['<email address hidden>'], self.team.getTeamAdminsEmailAddresses())
113 +
114 + def test_admins_are_teams_without_preferred_email_addresses(self):

looks like you have them inverted: that is that the name and comment of the former test describes the behaviour of the latter test, and vice versa.

Other than that this looks good.

review: Approve

=== 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-04-08 02:56:00 +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-04-08 02:56:00 +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-25 23:42:33 +0000
+++ lib/lp/registry/interfaces/persontransferjob.py	2011-04-08 02:56:00 +0000
@@ -95,6 +95,9 @@
         vocabulary='ValidPersonOrTeam',
         required=True)
 
+    def getErrorRecipients(self):
+        """See `BaseRunnableJob`."""
+
 
 class IPersonMergeJobSource(IJobSource):
     """An interface for acquiring IPersonMergeJobs."""
 
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2011-03-31 19:10:35 +0000
+++ lib/lp/registry/model/person.py	2011-04-08 02:56:00 +0000
@@ -1423,8 +1423,8 @@
         """See `IPerson`."""
         assert self.is_team
         to_addrs = set()
-        for person in self.getDirectAdministrators():
-            to_addrs.update(get_contact_email_addresses(person))
+        for admin in self.adminmembers:
+            to_addrs.update(get_contact_email_addresses(admin))
         return sorted(to_addrs)
 
     def addMember(self, person, reviewer, comment=None, force_team_add=False,
@@ -4065,16 +4065,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-04-08 02:56:00 +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-25 23:42:33 +0000
+++ lib/lp/registry/model/persontransferjob.py	2011-04-08 02:56:00 +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):
@@ -393,6 +394,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
@@ -402,7 +410,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,
             reviewer=self.reviewer)
 
=== 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-04-08 02:56:00 +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-25 23:42:33 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py	2011-04-08 02:56:00 +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='[e-mail]')
-        self.to_person = self.factory.makePerson(
-            name='gestalt', email='[e-mail]')
+        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,21 @@
         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,
+            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()))
 
=== 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-04-08 02:56:00 +0000
@@ -27,6 +27,44 @@
 from lp.testing import TestCaseWithFactory
 
 
+class TestPersonNotification(TestCaseWithFactory):
+    """Tests for PersonNotification."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestPersonNotification, 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')
+        email = '%s <%s>' % (
+            user.displayname, removeSecurityProxy(user.preferredemail).email)
+        self.assertEqual([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')
+        email = removeSecurityProxy(team.teamowner.preferredemail).email
+        self.assertEqual([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-23 16:28:51 +0000
+++ lib/lp/registry/tests/test_team.py	2011-04-08 02:56:00 +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,71 @@
         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')
+        login_celebrity('admin')
+
+    def test_admin_is_user(self):
+        # The team owner is a user and admin who provides the email address.
+        email = self.team.teamowner.preferredemail.email
+        self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
+
+    def test_no_admins(self):
+        # A team without admins has no email addresses.
+        self.team.teamowner.leave(self.team)
+        self.assertEqual([], self.team.getTeamAdminsEmailAddresses())
+
+    def test_admins_are_users_with_preferred_email_addresses(self):
+        # The team's admins are users, and they 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 setUpAdminingTeam(self, team):
+        """Return a new team set as the admin of the provided team."""
+        admin_team = self.factory.makeTeam()
+        admin_member = self.factory.makePerson()
+        admin_team.addMember(admin_member, admin_team.teamowner)
+        team.addMember(
+            admin_team, team.teamowner, force_team_add=True)
+        for membership in team.member_memberships:
+            membership.setStatus(
+                TeamMembershipStatus.ADMIN, admin_team.teamowner)
+        approved_member = self.factory.makePerson()
+        team.addMember(approved_member, team.teamowner)
+        team.teamowner.leave(team)
+        return admin_team
+
+    def test_admins_are_teams_with_preferred_email_addresses(self):
+        # The team's admin is a team with a contact address.
+        admin_team = self.setUpAdminingTeam(self.team)
+        admin_team.setContactAddress(
+            self.factory.makeEmail('[e-mail]', admin_team))
+        self.assertEqual(
+            ['[e-mail]'], self.team.getTeamAdminsEmailAddresses())
+
+    def test_admins_are_teams_without_preferred_email_addresses(self):
+        # The team's admin is a team without a contact address.
+        # The admin team members provide the email addresses.
+        admin_team = self.setUpAdminingTeam(self.team)
+        emails = sorted(
+            m.preferredemail.email for m in admin_team.activemembers)
+        self.assertEqual(
+            emails, self.team.getTeamAdminsEmailAddresses())
+
+
 class TestDefaultRenewalPeriodIsRequiredForSomeTeams(TestCaseWithFactory):
 
     layer = DatabaseFunctionalLayer