launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02809
[Merge] lp:~sinzui/launchpad/send-expire-memberships-0 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/send-expire-memberships-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#611900 unhelpful exception info in sendExpirationWarningEmail
https://bugs.launchpad.net/bugs/611900
#726628 flag-expired-memberships can trip an assertion check
https://bugs.launchpad.net/bugs/726628
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/send-expire-memberships-0/+merge/51693
Allow sendExpirationWarningEmail to send emails shortly after expiration.
Launchpad bug:
https://bugs.launchpad.net/bugs/726628
https://bugs.launchpad.net/bugs/611900
Pre-implementation: jcsackett
Test command: ./bin/test -vv \
-t TestTeamMembershipSendExpirationWarningEmail -t doc/teammembership
sendExpirationWarningEmail will raise an AssertionError (This membership's
expiration date must be in the future) if the date changes will processing the
list of memberships in flag-expired-memberships.py Both are working this dates
in the same manner. This is just an issue minutes passing between when the
membership is selected and when the method is called.
The assertions in this method are written for debugging; the method should
explicitly raise the AssertionErrors. The messages need to include the
team and person so that the issue can be investigated.
--------------------------------------------------------------------
RULES
* Replace some, maybe all, the doctests with unittests.
* Change the assert into a guard that silently returns.
* Replace the `assert` syntax with an explicit raise.
* Include the team and person so that the issue can be debugged.
QA
* None.
LINT
lib/lp/registry/doc/teammembership.txt
lib/lp/registry/interfaces/teammembership.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/tests/test_teammembership.py
IMPLEMENTATION
I intended to change the dateexpires assertion to allow for 12 hours
past the expiration, but the expiration messages were wrong, eg:
your membership will expire 2 hours ago.
By which point links the email to renew are invalid. The user will have
received 6 other message, so I decided to return early. I Added a unittest
for the assertions and the expected behaviour. I rewrote the error messages
to include the member and team. I updated the interface documentation to
explain what will happen if sendExpirationWarningEmail was called for one
of the special cases.
lib/lp/registry/interfaces/teammembership.py
lib/lp/registry/model/teammembership.py
lib/lp/registry/tests/test_teammembership.py
Removed a duplicate test that was easier to verify in a unittest
lib/lp/registry/doc/teammembership.txt
--
https://code.launchpad.net/~sinzui/launchpad/send-expire-memberships-0/+merge/51693
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/send-expire-memberships-0 into lp:launchpad.
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt 2010-12-07 14:42:33 +0000
+++ lib/lp/registry/doc/teammembership.txt 2011-03-01 04:03:17 +0000
@@ -953,59 +953,3 @@
mailing-list-experts
t1
t4
-
-
-Email to invalid users
-----------------------
-
-There are cases where teams have deactivated or suspended users as members.
-This is a data error that needs fixing, but should not interfere with
-any notification processes. An OOPS is reported in such cases and the
-method ignores the user.
-
-Bad user joins a team, and he is approved. An admin later suspends him.
-
- >>> from canonical.launchpad.interfaces.account import AccountStatus
- >>> from canonical.launchpad.interfaces.emailaddress import (
- ... EmailAddressStatus)
-
- >>> bad_user = factory.makePerson(
- ... email='bad-user@xxxxxxxxxxxxx',
- ... name='bad-user',
- ... password='invalid')
-
- >>> login('bad-user@xxxxxxxxxxxxx')
- >>> bad_user.join(t3)
-
- >>> login_person(t3.teamowner)
- >>> t3.setMembershipData(
- ... bad_user, TeamMembershipStatus.APPROVED, reviewer)
-
- >>> login('foo.bar@xxxxxxxxxxxxx')
- >>> from canonical.launchpad.interfaces.lpstorm import IMasterObject
- >>> IMasterObject(bad_user.account).status = AccountStatus.SUSPENDED
- >>> IMasterObject(bad_user.preferredemail).status = EmailAddressStatus.OLD
- >>> from lp.services.propertycache import get_property_cache
- >>> del get_property_cache(removeSecurityProxy(bad_user)).preferredemail
- >>> transaction.commit()
-
- >>> [m.displayname for m in t3.allmembers]
- [u'Bad-user', u'Jeff Waugh']
- >>> bad_user.is_valid_person
- False
- >>> print bad_user.preferredemail
- None
-
-The teams's notifications do not fail when they process bad user.
-
- >>> bad_membership = membershipset.getByPersonAndTeam(bad_user, t3)
- >>> bad_membership.setExpirationDate(utc_now + timedelta(days=9), foobar)
- >>> bad_membership.sendExpirationWarningEmail()
-
- >>> t3.renewal_policy = TeamMembershipRenewalPolicy.AUTOMATIC
- >>> t3.defaultrenewalperiod = 1
- >>> removeSecurityProxy(bad_membership).dateexpires = utc_now
- >>> bad_membership.sendAutoRenewalNotification()
-
- >>> bad_membership.setStatus(TeamMembershipStatus.EXPIRED, bad_user)
- True
=== modified file 'lib/lp/registry/interfaces/teammembership.py'
--- lib/lp/registry/interfaces/teammembership.py 2011-01-05 19:06:27 +0000
+++ lib/lp/registry/interfaces/teammembership.py 2011-03-01 04:03:17 +0000
@@ -230,8 +230,14 @@
"""
def sendExpirationWarningEmail():
- """Send an email to the member warning him that this membership will
- expire soon.
+ """Send the member an email warning that the membership will expire.
+
+ This method cannot be called for memberships without an expiration
+ date. Emails are not sent to members if their membership has already
+ expired or if the member is no longer active.
+
+ :raises AssertionError: if the member has no expiration date of the
+ team or if the TeamMembershipRenewalPolicy is AUTOMATIC.
"""
@call_with(user=REQUEST_USER)
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2011-01-10 21:34:24 +0000
+++ lib/lp/registry/model/teammembership.py 2011-03-01 04:03:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
# pylint: disable-msg=E0611,W0212
@@ -227,17 +227,21 @@
def sendExpirationWarningEmail(self):
"""See `ITeamMembership`."""
- assert self.dateexpires is not None, (
- 'This membership has no expiration date')
- assert self.dateexpires > datetime.now(pytz.timezone('UTC')), (
- "This membership's expiration date must be in the future: %s"
- % self.dateexpires.strftime('%Y-%m-%d'))
+ if self.dateexpires is None:
+ raise AssertionError(
+ '%s in team %s has no membership expiration date.' %
+ (self.person.name, self.team.name))
if self.team.renewal_policy == TeamMembershipRenewalPolicy.AUTOMATIC:
# An email will be sent later by handleMembershipsExpiringToday()
# when the membership is automatically renewed.
raise AssertionError(
- 'Team %r with automatic renewals should not send expiration '
+ 'Team %s with automatic renewals should not send expiration '
'warnings.' % self.team.name)
+ if self.dateexpires < datetime.now(pytz.timezone('UTC')):
+ # The membership has reached expiration. Silently return because
+ # there is nothing to do. The member will have received emails
+ # from previous calls by flag-expired-memberships.py
+ return
member = self.person
team = self.team
if member.isTeam():
=== modified file 'lib/lp/registry/tests/test_teammembership.py'
--- lib/lp/registry/tests/test_teammembership.py 2011-01-12 17:02:07 +0000
+++ lib/lp/registry/tests/test_teammembership.py 2011-03-01 04:03:17 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -16,6 +16,7 @@
import pytz
from zope.component import getUtility
+from zope.security.proxy import removeSecurityProxy
from canonical.database.sqlbase import (
cursor,
@@ -37,6 +38,7 @@
from canonical.testing.layers import DatabaseFunctionalLayer
from lp.registry.interfaces.person import (
IPersonSet,
+ TeamMembershipRenewalPolicy,
TeamSubscriptionPolicy,
)
from lp.registry.interfaces.teammembership import (
@@ -48,7 +50,11 @@
TeamMembership,
TeamParticipation,
)
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+ person_logged_in,
+ TestCaseWithFactory,
+ )
+from lp.testing.mail_helpers import pop_notifications
class TestTeamMembershipSet(TestCase):
@@ -136,7 +142,6 @@
# manually because otherwise we would only be allowed to set an
# expiration date in the future.
now = datetime.now(pytz.UTC)
- from zope.security.proxy import removeSecurityProxy
sample_person_on_motu = removeSecurityProxy(
self.membershipset.getByPersonAndTeam(sample_person, motu))
sample_person_on_motu.dateexpires = now
@@ -775,6 +780,74 @@
self.assertEqual(TeamMembershipStatus.DEACTIVATED, tm.status)
+class TestTeamMembershipSendExpirationWarningEmail(TestCaseWithFactory):
+ """Test the behaviour of sendExpirationWarningEmail()."""
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestTeamMembershipSendExpirationWarningEmail, self).setUp()
+ self.member = self.factory.makePerson(name='green')
+ self.team = self.factory.makeTeam(name='red')
+ login_person(self.team.teamowner)
+ self.team.addMember(self.member, self.team.teamowner)
+ self.tm = getUtility(ITeamMembershipSet).getByPersonAndTeam(
+ self.member, self.team)
+ pop_notifications()
+
+ def test_error_raised_when_no_expiration(self):
+ # An exception is raised if the membership does not have an
+ # expiration date.
+ self.assertEqual(None, self.tm.dateexpires)
+ message = 'green in team red has no membership expiration date.'
+ self.assertRaisesWithContent(
+ AssertionError, message, self.tm.sendExpirationWarningEmail)
+
+ def test_error_raised_for_team_with_automatic_renewal(self):
+ # An exception is raised if the team's TeamMembershipRenewalPolicy
+ # is AUTOMATIC.
+ self.team.renewal_policy = TeamMembershipRenewalPolicy.AUTOMATIC
+ self.team.defaultrenewalperiod = 365
+ tomorrow = datetime.now(pytz.UTC) + timedelta(days=1)
+ removeSecurityProxy(self.tm).dateexpires = tomorrow
+ message = (
+ 'Team red with automatic renewals should not send '
+ 'expiration warnings.')
+ self.assertRaisesWithContent(
+ AssertionError, message, self.tm.sendExpirationWarningEmail)
+
+ def test_message_sent_for_future_expiration(self):
+ # An email is sent to the user whose membership will expire.
+ tomorrow = datetime.now(pytz.UTC) + timedelta(days=1)
+ removeSecurityProxy(self.tm).dateexpires = tomorrow
+ self.tm.sendExpirationWarningEmail()
+ notifications = pop_notifications()
+ self.assertEqual(1, len(notifications))
+ message = notifications[0]
+ self.assertEqual(
+ 'Your membership in red is about to expire', message['subject'])
+ self.assertEqual(
+ self.member.preferredemail.email, message['to'])
+
+ def test_no_message_sent_for_expired_memberships(self):
+ # Members whose membership has expired do not get a message.
+ yesterday = datetime.now(pytz.UTC) - timedelta(days=1)
+ removeSecurityProxy(self.tm).dateexpires = yesterday
+ self.tm.sendExpirationWarningEmail()
+ notifications = pop_notifications()
+ self.assertEqual(0, len(notifications))
+
+ def test_no_message_sent_for_non_active_users(self):
+ # Non-active users do not get an expiration message.
+ with person_logged_in(self.member):
+ self.member.deactivateAccount('Goodbye.')
+ IStore(self.member).flush()
+ now = datetime.now(pytz.UTC)
+ removeSecurityProxy(self.tm).dateexpires = now + timedelta(days=1)
+ self.tm.sendExpirationWarningEmail()
+ notifications = pop_notifications()
+ self.assertEqual(0, len(notifications))
+
+
class TestCheckTeamParticipationScript(TestCase):
layer = DatabaseFunctionalLayer
