launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14561
[Merge] lp:~sinzui/launchpad/team-without-admin into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/team-without-admin into lp:launchpad.
Commit message:
Explain when a team has no admins to contact.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1082766 in Launchpad itself: "cannot contact team without admins"
https://bugs.launchpad.net/launchpad/+bug/1082766
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/team-without-admin/+merge/136274
The "Contact this team's admins" form oopses when the team does not
have any admins. This can happen when the team owner leaves the team,
but fails to delegate the admin responsibility to others.
--------------------------------------------------------------------
RULES
Pre-implementation: no one
* Update send_direct_contact_email() to only record the message
was sent if there was a message.
* Update page to explain that the team team does not have any
admins and advise the team owner to delegate one or more
people to be admins.
ADDENDUM
* The view and the recipient set do check if there are people to send
the email to, but the rule is broken. The recipient set used to
send to the owner, now it sends to admins. The owner rules were
removed, but the admin case needed to be handled.
QA
* Visit https://qastaging.launchpad.net/~motu/+contactuser
* Verify the page explains that the team has no admins so the
owner should be contacted instead.
LoC
I have more than 16,000 lines of credit.
LINT
lib/lp/registry/browser/person.py
lib/lp/registry/browser/tests/test_person_contact.py
lib/lp/registry/mail/notification.py
lib/lp/registry/templates/contact-user.pt
lib/lp/registry/tests/test_notification.py
TEST
./bin/test -vvc lp.registry.tests.test_notification
./bin/test -vvc lp.registry.browser.tests.test_person_contact
./bin/test -vvc -t user-to-user -t person-views lp.registry.browser.tests
^ These last tests have lot over overlap. I think we want to move
all the overlap into user-to-user.txt, then rewrite the tests as
unit tests to horrendous reduce the duplication the doctests.
IMPLEMENTATION
I updated the send_direct_contact_email() to not assume that a message was
sent. I created a new test module for this code. It duplicates quota testing
in doctests. I will try to remove the duplication, or move the doctests into
the unit test before I land this branch.
lib/lp/registry/mail/notification.py
lib/lp/registry/tests/test_notification.py
I fixed the len() check on the recipient set. I choose to use a common
cached property that is already confirmed to select the correct recipients
instead of adding duplicate code to determine the length of the set for
TO_ADMIN and TO_MEMBER cases. The text shown when the there is no-one to
contact must have been wrong, it was just the same message shown in the form.
I added a property to the view to explain why the user or team could not
be contacted. I made the recipient set's primary_reason attribute public
so that the view did not need to duplicate code.
lib/lp/registry/browser/person.py
lib/lp/registry/browser/tests/test_person_contact.py
lib/lp/registry/templates/contact-user.pt
--
https://code.launchpad.net/~sinzui/launchpad/team-without-admin/+merge/136274
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/team-without-admin into lp:launchpad.
=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py 2012-11-19 21:59:03 +0000
+++ lib/lp/registry/browser/person.py 2012-11-26 21:37:19 +0000
@@ -1750,7 +1750,7 @@
TO_MEMBERS
"""
return ContactViaWebNotificationRecipientSet(
- self.user, self.context)._primary_reason
+ self.user, self.context).primary_reason
@property
def contact_link_title(self):
@@ -3919,7 +3919,7 @@
"""
self.user = user
self.description = None
- self._primary_reason = None
+ self.primary_reason = None
self._primary_recipient = None
self._reason = None
self._header = None
@@ -3955,12 +3955,12 @@
:param person_or_team: The party that is the context of the email.
:type person_or_team: `IPerson`.
"""
- if self._primary_reason is self.TO_USER:
+ if self.primary_reason is self.TO_USER:
reason = (
'using the "Contact this user" link on your profile page\n'
'(%s)' % canonical_url(person_or_team))
header = 'ContactViaWeb user'
- elif self._primary_reason is self.TO_ADMINS:
+ elif self.primary_reason is self.TO_ADMINS:
reason = (
'using the "Contact this team\'s admins" link on the '
'%s team page\n(%s)' % (
@@ -3968,7 +3968,7 @@
canonical_url(person_or_team)))
header = 'ContactViaWeb owner (%s team)' % person_or_team.name
else:
- # self._primary_reason is self.TO_MEMBERS.
+ # self.primary_reason is self.TO_MEMBERS.
reason = (
'to each member of the %s team using the '
'"Contact this team" link on the %s team page\n(%s)' % (
@@ -3984,11 +3984,11 @@
:param person_or_team: The party that is the context of the email.
:type person_or_team: `IPerson`.
"""
- if self._primary_reason is self.TO_USER:
+ if self.primary_reason is self.TO_USER:
return (
'You are contacting %s (%s).' %
(person_or_team.displayname, person_or_team.name))
- elif self._primary_reason is self.TO_ADMINS:
+ elif self.primary_reason is self.TO_ADMINS:
return (
'You are contacting the %s (%s) team admins.' %
(person_or_team.displayname, person_or_team.name))
@@ -4008,12 +4008,12 @@
def _all_recipients(self):
"""Set the cache of all recipients."""
all_recipients = {}
- if self._primary_reason is self.TO_MEMBERS:
+ if self.primary_reason is self.TO_MEMBERS:
team = self._primary_recipient
for recipient in team.getMembersWithPreferredEmails():
email = removeSecurityProxy(recipient).preferredemail.email
all_recipients[email] = recipient
- elif self._primary_reason is self.TO_ADMINS:
+ elif self.primary_reason is self.TO_ADMINS:
team = self._primary_recipient
for admin in team.adminmembers:
# This method is similar to getTeamAdminsEmailAddresses, but
@@ -4063,13 +4063,12 @@
"""The number of recipients in the set."""
if self._count_recipients is None:
recipient = self._primary_recipient
- if self._primary_reason is self.TO_MEMBERS:
- self._count_recipients = (
- recipient.getMembersWithPreferredEmailsCount())
+ if self.primary_reason in (self.TO_MEMBERS, self.TO_ADMINS):
+ self._count_recipients = (len(self._all_recipients))
elif recipient.is_valid_person_or_team:
self._count_recipients = 1
else:
- # The user or team owner is deactivated.
+ # The user is deactivated.
self._count_recipients = 0
return self._count_recipients
@@ -4094,7 +4093,7 @@
recipients.
"""
self._reset_state()
- self._primary_reason = self._getPrimaryReason(person)
+ self.primary_reason = self._getPrimaryReason(person)
self._primary_recipient = person
if reason is None:
reason, header = self._getReasonAndHeader(person)
@@ -4216,6 +4215,18 @@
return throttle_date + interval
@property
+ def contact_not_possible_reason(self):
+ """The reason the person cannot be contacted."""
+ if self.has_valid_email_address:
+ return None
+ elif self.recipients.primary_reason is self.recipients.TO_USER:
+ return "The user is not active."
+ elif self.recipients.primary_reason is self.recipients.TO_ADMINS:
+ return "The team has no admins. Contact the team owner instead."
+ else:
+ return "The team has no members."
+
+ @property
def page_title(self):
"""Return the appropriate pagetitle."""
if self.context.is_team:
=== added file 'lib/lp/registry/browser/tests/test_person_contact.py'
--- lib/lp/registry/browser/tests/test_person_contact.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/browser/tests/test_person_contact.py 2012-11-26 21:37:19 +0000
@@ -0,0 +1,78 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+"""Test views and helpers related to the contact person feature."""
+
+__metaclass__ = type
+
+from lp.registry.browser.person import ContactViaWebNotificationRecipientSet
+from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.views import create_initialized_view
+
+
+class ContactViaWebNotificationRecipientSetTestCase(TestCaseWithFactory):
+ """Tests the behaviour of ContactViaWebNotificationRecipientSet."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_len(self):
+ # The recipient set length is based on the number or recipient
+ # found by the selection rules.
+ sender = self.factory.makePerson(email='me@xxxxxx')
+ # Contact user.
+ user = self.factory.makePerson(email='him@xxxxxx')
+ self.assertEqual(
+ 1, len(ContactViaWebNotificationRecipientSet(sender, user)))
+ # Contact team admins.
+ team = self.factory.makeTeam()
+ self.assertEqual(
+ 1, len(ContactViaWebNotificationRecipientSet(sender, team)))
+ with person_logged_in(team.teamowner):
+ team.teamowner.leave(team)
+ self.assertEqual(
+ 0, len(ContactViaWebNotificationRecipientSet(sender, team)))
+ # Contact team members.
+ sender_team = self.factory.makeTeam(members=[sender])
+ owner = sender_team.teamowner
+ self.assertEqual(
+ 2, len(ContactViaWebNotificationRecipientSet(owner, sender_team)))
+ with person_logged_in(owner):
+ owner.leave(sender_team)
+ self.assertEqual(
+ 1, len(ContactViaWebNotificationRecipientSet(owner, sender_team)))
+
+
+class EmailToPersonViewTestCase(TestCaseWithFactory):
+ """Tests the behaviour of EmailToPersonView."""
+
+ layer = DatabaseFunctionalLayer
+
+ def test_contact_not_possible_reason(self):
+ # When the no recipients to send an email to, the view provides a
+ # reason to show the user.
+ user = self.factory.makePerson()
+ # Contact inactive user.
+ inactive_user = self.factory.makePerson(
+ email_address_status=EmailAddressStatus.NEW)
+ with person_logged_in(user):
+ view = create_initialized_view(inactive_user, '+contactuser')
+ self.assertEqual(
+ "The user is not active.", view.contact_not_possible_reason)
+ # Contact team without admins.
+ team = self.factory.makeTeam()
+ with person_logged_in(team.teamowner):
+ team.teamowner.leave(team)
+ with person_logged_in(user):
+ view = create_initialized_view(team, '+contactuser')
+ self.assertEqual(
+ "The team has no admins. Contact the team owner instead.",
+ view.contact_not_possible_reason)
+ # Contact team without members.
+ with person_logged_in(team.teamowner):
+ view = create_initialized_view(team, '+contactuser')
+ self.assertEqual(
+ "The team has no members.", view.contact_not_possible_reason)
=== modified file 'lib/lp/registry/mail/notification.py'
--- lib/lp/registry/mail/notification.py 2012-08-14 23:27:07 +0000
+++ lib/lp/registry/mail/notification.py 2012-11-26 21:37:19 +0000
@@ -396,15 +396,7 @@
message['X-Launchpad-Message-Rationale'] = rational_header
# Send the message.
sendmail(message, bulk=False)
- # BarryWarsaw 19-Nov-2008: If any messages were sent, record the fact that
- # the sender contacted the team. This is not perfect though because we're
- # really recording the fact that the person contacted the last member of
- # the team. There's little we can do better though because the team has
- # no contact address, and so there isn't actually an address to record as
- # the team's recipient. It currently doesn't matter though because we
- # don't actually do anything with the recipient information yet. All we
- # care about is the sender, for quota purposes. We definitely want to
- # record the contact outside the above loop though, because if there are
- # 10 members of the team with no contact address, one message should not
- # consume the sender's entire quota.
- authorization.record(message)
+ # Use the information from the last message sent to record the action
+ # taken. The record will be used to throttle user-to-user emails.
+ if message is not None:
+ authorization.record(message)
=== modified file 'lib/lp/registry/templates/contact-user.pt'
--- lib/lp/registry/templates/contact-user.pt 2012-02-10 19:52:27 +0000
+++ lib/lp/registry/templates/contact-user.pt 2012-11-26 21:37:19 +0000
@@ -31,7 +31,7 @@
</p>
</tal:disallowed>
<tal:disallowed tal:condition="not:view/has_valid_email_address">
- <p tal:content="view/recipients/description"/>
+ <p tal:content="view/contact_not_possible_reason"/>
</tal:disallowed>
</div>
</body>
=== added file 'lib/lp/registry/tests/test_notification.py'
--- lib/lp/registry/tests/test_notification.py 1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_notification.py 2012-11-26 21:37:19 +0000
@@ -0,0 +1,81 @@
+# Copyright 2012 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test notification classes and functions."""
+
+__metaclass__ = type
+
+from lp.registry.mail.notification import send_direct_contact_email
+from lp.services.mail.notificationrecipientset import NotificationRecipientSet
+from lp.services.messages.interfaces.message import (
+ IDirectEmailAuthorization,
+ QuotaReachedError,
+ )
+from lp.testing import TestCaseWithFactory
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.mail_helpers import pop_notifications
+
+
+class SendDirectContactEmailTestCase(TestCaseWithFactory):
+
+ layer = DatabaseFunctionalLayer
+
+ def test_send_message(self):
+ self.factory.makePerson(email='me@xxxxxx', name='me')
+ user = self.factory.makePerson(email='him@xxxxxx', name='him')
+ subject = 'test subject'
+ body = 'test body'
+ recipients_set = NotificationRecipientSet()
+ recipients_set.add(user, 'test reason', 'test rationale')
+ pop_notifications()
+ send_direct_contact_email('me@xxxxxx', recipients_set, subject, body)
+ notifications = pop_notifications()
+ notification = notifications[0]
+ self.assertEqual(1, len(notifications))
+ self.assertEqual('Me <me@xxxxxx>', notification['From'])
+ self.assertEqual('Him <him@xxxxxx>', notification['To'])
+ self.assertEqual(subject, notification['Subject'])
+ self.assertEqual(
+ 'test rationale', notification['X-Launchpad-Message-Rationale'])
+ self.assertIs(None, notification['Precedence'])
+ self.assertTrue('launchpad' in notification['Message-ID'])
+ self.assertEqual(
+ '\n'.join([
+ '%s' % body,
+ '-- ',
+ 'This message was sent from Launchpad by',
+ 'Me (http://launchpad.dev/~me)',
+ 'test reason.',
+ 'For more information see',
+ 'https://help.launchpad.net/YourAccount/ContactingPeople']),
+ notification.get_payload())
+
+ def test_quota_reached_error(self):
+ # An error is raised if the user has reached the daily quota.
+ self.factory.makePerson(email='me@xxxxxx', name='me')
+ user = self.factory.makePerson(email='him@xxxxxx', name='him')
+ recipients_set = NotificationRecipientSet()
+ old_message = self.factory.makeSignedMessage(email_address='me@xxxxxx')
+ authorization = IDirectEmailAuthorization(user)
+ for action in xrange(authorization.message_quota):
+ authorization.record(old_message)
+ self.assertRaises(
+ QuotaReachedError, send_direct_contact_email,
+ 'me@xxxxxx', recipients_set, 'subject', 'body')
+
+ def test_empty_recipient_set(self):
+ # The recipient set can be empty. No messages are sent and the
+ # action does not count toward the daily quota.
+ self.factory.makePerson(email='me@xxxxxx', name='me')
+ user = self.factory.makePerson(email='him@xxxxxx', name='him')
+ recipients_set = NotificationRecipientSet()
+ old_message = self.factory.makeSignedMessage(email_address='me@xxxxxx')
+ authorization = IDirectEmailAuthorization(user)
+ for action in xrange(authorization.message_quota - 1):
+ authorization.record(old_message)
+ pop_notifications()
+ send_direct_contact_email(
+ 'me@xxxxxx', recipients_set, 'subject', 'body')
+ notifications = pop_notifications()
+ self.assertEqual(0, len(notifications))
+ self.assertTrue(authorization.is_allowed)
Follow ups
