Launchpad itself

Merge lp:~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams into lp:launchpad

  1. bug-557036-bad-email-for-autorenewal-teams
  2. Merge into devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Brad Crittenden
Approved revision: no longer in the source branch.
Merged at revision: 11232
Proposed branch: lp:~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams
Merge into: lp:launchpad
Diff against target: 170 lines (+81/-11)
3 files modified
cronscripts/flag-expired-memberships.py (+3/-2)
lib/lp/registry/doc/teammembership.txt (+57/-3)
lib/lp/registry/model/teammembership.py (+21/-6)
To merge this branch: bzr merge lp:~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams
Reviewer Review Type Date Requested Status
Brad Crittenden (community) code Approve
Review via email: mp+29894@code.launchpad.net

Description of the change

Summary
-------

The flag-expired-memberships.py cronscript confusingly emails users that
their membership will expire soon and that they should contact the team
admins to get it renewed even though the team is configured to
automatically renew memberships. This causes unnecessary work for the
team admins.

Implementation details
----------------------

cronscripts/flag-expired-memberships.py
lib/lp/registry/doc/teammembership.txt
lib/lp/registry/model/teammembership.py

Tests
-----

./bin/test -vv -t doc/teammembership.txt

Demo and Q/A
------------

* On staging, create a team with automatic renewal of memberships.
* Add a member, then edit its membership so that it expires in a few
  days.
* Run the cronscript.
* Check if the user was emailed.

To post a comment you must log in.
Revision history for this message
Brad Crittenden (bac) wrote :

Thanks for the fix, Edwin.

Please s/pytz.timezone('UTC')/pytz.UTC/

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cronscripts/flag-expired-memberships.py'
2--- cronscripts/flag-expired-memberships.py 2010-04-27 19:48:39 +0000
3+++ cronscripts/flag-expired-memberships.py 2010-07-26 16:30:59 +0000
4@@ -41,8 +41,10 @@
5 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)
6 self.txn.begin()
7 for membership in membershipset.getMembershipsToExpire(
8- min_date_for_warning):
9+ min_date_for_warning, exclude_autorenewals=True):
10 membership.sendExpirationWarningEmail()
11+ self.logger.debug("Sent warning email to %s in %s team."
12+ % (membership.person.name, membership.team.name))
13 self.txn.commit()
14
15 def main(self):
16@@ -59,4 +61,3 @@
17 script = ExpireMemberships('flag-expired-memberships',
18 dbuser=config.expiredmembershipsflagger.dbuser)
19 script.lock_and_run()
20-
21
22=== modified file 'lib/lp/registry/doc/teammembership.txt'
23--- lib/lp/registry/doc/teammembership.txt 2010-01-04 22:48:23 +0000
24+++ lib/lp/registry/doc/teammembership.txt 2010-07-26 16:30:59 +0000
25@@ -622,6 +622,44 @@
26 (u'ubuntu-team', u'guadamen'), (u'name16', u'launchpad-buildd-admins'),
27 (u'name12', u'landscape-developers')]
28
29+
30+The getMembershipsToExpire() method also accepts an optional
31+'exclude_autorewals' argument. When that argument is provided,
32+memberships in teams that are configured to renew the membership
33+automatically will be excluded. In that case, there is no reason to send
34+a warning email several days before the expiration. The user will just
35+be notified that the membership has already been automatically renewed
36+on the expiration day.
37+
38+ >>> from storm.store import Store
39+ >>> autorenewal_team = factory.makeTeam(name="autorenewal-team")
40+ >>> autorenewal_team.renewal_policy = (
41+ ... TeamMembershipRenewalPolicy.AUTOMATIC)
42+ >>> autorenewal_team.defaultrenewalperiod = 73
43+ >>> otto = factory.makePerson(name='otto')
44+ >>> ignore = autorenewal_team.addMember(otto, salgado)
45+ >>> otto_membership = otto.myactivememberships[0]
46+ >>> otto_membership.setExpirationDate(utc_now, salgado)
47+ >>> original_otto_date_expires = otto_membership.dateexpires
48+
49+ >>> do_not_warn = factory.makePerson(name='do-not-warn')
50+ >>> ignore = autorenewal_team.addMember(do_not_warn, salgado)
51+ >>> do_not_warn.myactivememberships[0].setExpirationDate(
52+ ... tomorrow, salgado)
53+
54+ # Not excluding teams with automatic renewals.
55+ >>> [(membership.person.name, membership.team.name)
56+ ... for membership in membershipset.getMembershipsToExpire()
57+ ... if membership.team.name == 'autorenewal-team']
58+ [(u'otto', u'autorenewal-team')]
59+
60+ # Excluding teams with automatic renewals.
61+ >>> [(membership.person.name, membership.team.name)
62+ ... for membership in membershipset.getMembershipsToExpire(
63+ ... exclude_autorenewals=True)
64+ ... if membership.team.name == 'autorenewal-team']
65+ []
66+
67 Now we commit the changes and run the cronscript.
68 XXX: flush_database_updates() shouldn't be needed. This seems to be
69 Bug 3989 -- StuarBishop 20060713
70@@ -631,12 +669,21 @@
71
72 >>> import subprocess
73 >>> process = subprocess.Popen(
74- ... 'cronscripts/flag-expired-memberships.py -q', shell=True,
75+ ... 'cronscripts/flag-expired-memberships.py -v', shell=True,
76 ... stdin=subprocess.PIPE, stdout=subprocess.PIPE,
77 ... stderr=subprocess.PIPE)
78 >>> (out, err) = process.communicate()
79- >>> out, err
80- ('', '')
81+ >>> print out
82+
83+A warning email should not have been sent to otto because it was renewed
84+inside the cronscript, and it should not be sent to do_not_warn, because
85+teams with automatic renewal don't need warnings.
86+
87+ >>> print '\n'.join(
88+ ... line for line in err.split('\n') if 'INFO' not in line)
89+ DEBUG Sent warning email to name16 in launchpad-buildd-admins team.
90+ DEBUG Sent warning email to name12 in landscape-developers team.
91+ DEBUG Removing lock file: /var/lock/launchpad-flag-expired-memberships.lock
92 >>> process.returncode
93 0
94 >>> transaction.abort()
95@@ -657,6 +704,13 @@
96 >>> sp_on_ubuntu_translators.status.title
97 'Deactivated'
98
99+ >>> otto_on_autorenewal_team = membershipset.getByPersonAndTeam(
100+ ... otto, autorenewal_team)
101+ >>> otto_on_autorenewal_team.status.title
102+ 'Approved'
103+ >>> otto_on_autorenewal_team.dateexpires - original_otto_date_expires
104+ datetime.timedelta(73)
105+
106
107 == Renewing team memberships ==
108
109
110=== modified file 'lib/lp/registry/model/teammembership.py'
111--- lib/lp/registry/model/teammembership.py 2010-01-04 17:15:00 +0000
112+++ lib/lp/registry/model/teammembership.py 2010-07-26 16:30:59 +0000
113@@ -21,6 +21,7 @@
114
115 from sqlobject import ForeignKey, StringCol
116
117+from canonical.launchpad.interfaces.lpstorm import IStore
118 from canonical.database.sqlbase import (
119 flush_database_updates, SQLBase, sqlvalues)
120 from canonical.database.constants import UTC_NOW
121@@ -97,7 +98,7 @@
122 ondemand = TeamMembershipRenewalPolicy.ONDEMAND
123 admin = TeamMembershipStatus.APPROVED
124 approved = TeamMembershipStatus.ADMIN
125- date_limit = datetime.now(pytz.timezone('UTC')) + timedelta(
126+ date_limit = datetime.now(pytz.UTC) + timedelta(
127 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)
128 return (self.status in (admin, approved)
129 and self.team.renewal_policy == ondemand
130@@ -208,6 +209,12 @@
131 assert self.dateexpires > datetime.now(pytz.timezone('UTC')), (
132 "This membership's expiration date must be in the future: %s"
133 % self.dateexpires.strftime('%Y-%m-%d'))
134+ if self.team.renewal_policy == TeamMembershipRenewalPolicy.AUTOMATIC:
135+ # An email will be sent later by handleMembershipsExpiringToday()
136+ # when the membership is automatically renewed.
137+ raise AssertionError(
138+ 'Team %r with automatic renewals should not send expiration '
139+ 'warnings.' % self.team.name)
140 member = self.person
141 team = self.team
142 if member.isTeam():
143@@ -522,14 +529,22 @@
144 """See `ITeamMembershipSet`."""
145 return TeamMembership.selectOneBy(person=person, team=team)
146
147- def getMembershipsToExpire(self, when=None):
148+ def getMembershipsToExpire(self, when=None, exclude_autorenewals=False):
149 """See `ITeamMembershipSet`."""
150 if when is None:
151 when = datetime.now(pytz.timezone('UTC'))
152- query = ("date_expires <= %s AND status IN (%s, %s)"
153- % sqlvalues(when, TeamMembershipStatus.ADMIN,
154- TeamMembershipStatus.APPROVED))
155- return TeamMembership.select(query)
156+ conditions = [
157+ TeamMembership.dateexpires <= when,
158+ TeamMembership.status.is_in(
159+ [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED])
160+ ]
161+ if exclude_autorenewals:
162+ # Avoid circular import.
163+ from lp.registry.model.person import Person
164+ conditions.append(TeamMembership.team == Person.id)
165+ conditions.append(
166+ Person.renewal_policy != TeamMembershipRenewalPolicy.AUTOMATIC)
167+ return IStore(TeamMembership).find(TeamMembership, *conditions)
168
169
170 class TeamParticipation(SQLBase):