Merge lp:~cjohnston/launchpad/1270141 into lp:launchpad

Proposed by Chris Johnston
Status: Rejected
Rejected by: William Grant
Proposed branch: lp:~cjohnston/launchpad/1270141
Merge into: lp:launchpad
Diff against target: 29 lines (+5/-2)
2 files modified
lib/lp/registry/model/person.py (+2/-0)
lib/lp/registry/tests/test_team.py (+3/-2)
To merge this branch: bzr merge lp:~cjohnston/launchpad/1270141
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+222080@code.launchpad.net

Commit message

When there are no team admins, find the team owner's preferred email address and mail the team owner instead

Description of the change

If a team has no admins, getTeamAdminsEmailAddresses() fails and causes an oops. This was seen when attempting to add a commercial subscription to a project who's team had no admins. When there are no admins, default back to using the team's owner.

To post a comment you must log in.
Revision history for this message
Curtis Hovey (sinzui) wrote :

A team with admins is leaderless. The team owner is negligent, not being an admin or appointing other admins to lead. There is no one to take responsability...it is not trustworthy. I don't think it should be included in the list of valid teams.

The rules once did send emails to the team owner, but this lead to trust issue or complaints from owner who had delegated the leadership of the team to others. In general, the team owner is not to receive any emails except about admin changes.

Revision history for this message
Chris Johnston (cjohnston) wrote :

Curtis, this is the way William said to fix it, so I guess I will let you two debate it.

Revision history for this message
William Grant (wgrant) wrote :

> A team with admins is leaderless. The team owner is negligent, not being an
> admin or appointing other admins to lead. There is no one to take
> responsability...it is not trustworthy. I don't think it should be included in
> the list of valid teams.
>
> The rules once did send emails to the team owner, but this lead to trust issue
> or complaints from owner who had delegated the leadership of the team to
> others. In general, the team owner is not to receive any emails except about
> admin changes.

The team owner is negligent, indeed, so they should probably be convinced to add some admins to stop getting spammed. Do you have a suggested alternate solution for this particular case, of sending commercial project expiration emails?

Revision history for this message
Curtis Hovey (sinzui) wrote :

Sorry for not following up. I in the case of commercial expiration, the owner is fine given the owner is probably the one that chooses to purchase.

Revision history for this message
William Grant (wgrant) wrote :

On 10/06/14 03:41, Curtis Hovey wrote:
> Sorry for not following up. I in the case of commercial expiration, the owner is fine given the owner is probably the one that chooses to purchase.

It seems like a bad idea to special-case that just there. Why should the
fallback not apply everywere that the team's admins are wanted?

Revision history for this message
William Grant (wgrant) wrote :

Curtis, do you have any more comments, or should we go ahead and make this change?

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2014-03-11 11:34:08 +0000
+++ lib/lp/registry/model/person.py 2014-06-04 17:21:05 +0000
@@ -1593,6 +1593,8 @@
1593 to_addrs = set()1593 to_addrs = set()
1594 for admin in self.adminmembers:1594 for admin in self.adminmembers:
1595 to_addrs.update(get_contact_email_addresses(admin))1595 to_addrs.update(get_contact_email_addresses(admin))
1596 if not to_addrs:
1597 to_addrs.update(get_contact_email_addresses(self.teamowner))
1596 return sorted(to_addrs)1598 return sorted(to_addrs)
15971599
1598 def addMember(self, person, reviewer, comment=None, force_team_add=False,1600 def addMember(self, person, reviewer, comment=None, force_team_add=False,
15991601
=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py 2013-06-20 05:50:00 +0000
+++ lib/lp/registry/tests/test_team.py 2014-06-04 17:21:05 +0000
@@ -157,9 +157,10 @@
157 self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())157 self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
158158
159 def test_no_admins(self):159 def test_no_admins(self):
160 # A team without admins has no email addresses.160 # A team without admins returns team owners email address.
161 self.team.teamowner.leave(self.team)161 self.team.teamowner.leave(self.team)
162 self.assertEqual([], self.team.getTeamAdminsEmailAddresses())162 email = self.team.teamowner.preferredemail.email
163 self.assertEqual([email], self.team.getTeamAdminsEmailAddresses())
163164
164 def test_admins_are_users_with_preferred_email_addresses(self):165 def test_admins_are_users_with_preferred_email_addresses(self):
165 # The team's admins are users, and they provide the email addresses.166 # The team's admins are users, and they provide the email addresses.