Code review comment for lp:~sinzui/launchpad/person-merge-job-1

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.