Merge lp:~sinzui/launchpad/person-merge-job-1 into lp:launchpad
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Collins | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12783 | ||||
Proposed branch: | lp:~sinzui/launchpad/person-merge-job-1 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
385 lines (+187/-23) 10 files modified
lib/lp/registry/emailtemplates/team-merged.txt (+8/-0) lib/lp/registry/interfaces/personnotification.py (+9/-1) lib/lp/registry/interfaces/persontransferjob.py (+3/-0) lib/lp/registry/model/person.py (+13/-9) lib/lp/registry/model/personnotification.py (+21/-6) lib/lp/registry/model/persontransferjob.py (+8/-1) lib/lp/registry/scripts/personnotification.py (+3/-2) lib/lp/registry/tests/test_person_merge_job.py (+18/-4) lib/lp/registry/tests/test_personnotification.py (+38/-0) lib/lp/registry/tests/test_team.py (+66/-0) |
||||
To merge this branch: | bzr merge lp:~sinzui/launchpad/person-merge-job-1 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Approve | ||
Review via email: mp+54440@code.launchpad.net |
Description of the change
PersonNotification supports teams.
Launchpad bug: https:/
Pre-
Test command: ./bin/test -vv \
-t person-merge -t test_person_
-t test_personnoti
Before bug 736421 can be addressed, there are several infrastructure issues
that must be solved first:
* Person.merge needs send success emails to teams.
* PersonNotificat
* Team.getTeamAdm
* The job.getErrorRec
-------
RULES
* Update Team.getTeamAdm
team owner to locate team admins with email addresses.
* Update PersonNotificat
person is a team.
* Consider a property that can be tested because the method
and PersonNotificat
* Update Person.merge to send to teams
* Add a team email template.
* Refine PersonMergeJob.
team email addresses.
QA
* Merge a team with a team you admin on qastaging.
* Run cronscripts/
* Verify a message to you is in the staging sent folder.
LINT
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
lib/
IMPLEMENTATION
Updated Team.getTeamAdm
team owner to locate team admins with email addresses. This applies the
same kind of loop used in contact-
getDirectTeamAd
admin is a team without a contact address.
lib/
lib/
Updated PersonNotificat
person is a team. Added to_addresses and can_send properties so that
the send method and PersonNotificat
as users. The to_addresses property uses the revised
Team.getTeamAdm
lib/
lib/
lib/
lib/
Refined PersonMergeJob.
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.getTeamAdm
lib/
lib/
lib/
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/
lib/
lib/
75 + def getErrorRecipie nts(self) : b`."""
76 + """See `BaseRunnableJo
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 person. is_team: person. getDirectAdmini strators( ): update( get_contact_ email_addresses (admin) ) person. teamowner update( get_contact_ email_addresses (team_or_ person) )
92 + # Recurse through team ownership until an email address is found.
93 + while len(to_addrs) == 0:
94 + if team_or_
95 + for admin in team_or_
96 + to_addrs.
97 + team_or_person = team_or_
98 + else:
99 + to_addrs.
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.