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
=== modified file 'cronscripts/flag-expired-memberships.py'
--- cronscripts/flag-expired-memberships.py 2010-04-27 19:48:39 +0000
+++ cronscripts/flag-expired-memberships.py 2010-07-26 16:30:59 +0000
@@ -41,8 +41,10 @@
41 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)41 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)
42 self.txn.begin()42 self.txn.begin()
43 for membership in membershipset.getMembershipsToExpire(43 for membership in membershipset.getMembershipsToExpire(
44 min_date_for_warning):44 min_date_for_warning, exclude_autorenewals=True):
45 membership.sendExpirationWarningEmail()45 membership.sendExpirationWarningEmail()
46 self.logger.debug("Sent warning email to %s in %s team."
47 % (membership.person.name, membership.team.name))
46 self.txn.commit()48 self.txn.commit()
4749
48 def main(self):50 def main(self):
@@ -59,4 +61,3 @@
59 script = ExpireMemberships('flag-expired-memberships',61 script = ExpireMemberships('flag-expired-memberships',
60 dbuser=config.expiredmembershipsflagger.dbuser)62 dbuser=config.expiredmembershipsflagger.dbuser)
61 script.lock_and_run()63 script.lock_and_run()
62
6364
=== modified file 'lib/lp/registry/doc/teammembership.txt'
--- lib/lp/registry/doc/teammembership.txt 2010-01-04 22:48:23 +0000
+++ lib/lp/registry/doc/teammembership.txt 2010-07-26 16:30:59 +0000
@@ -622,6 +622,44 @@
622 (u'ubuntu-team', u'guadamen'), (u'name16', u'launchpad-buildd-admins'),622 (u'ubuntu-team', u'guadamen'), (u'name16', u'launchpad-buildd-admins'),
623 (u'name12', u'landscape-developers')]623 (u'name12', u'landscape-developers')]
624624
625
626The getMembershipsToExpire() method also accepts an optional
627'exclude_autorewals' argument. When that argument is provided,
628memberships in teams that are configured to renew the membership
629automatically will be excluded. In that case, there is no reason to send
630a warning email several days before the expiration. The user will just
631be notified that the membership has already been automatically renewed
632on the expiration day.
633
634 >>> from storm.store import Store
635 >>> autorenewal_team = factory.makeTeam(name="autorenewal-team")
636 >>> autorenewal_team.renewal_policy = (
637 ... TeamMembershipRenewalPolicy.AUTOMATIC)
638 >>> autorenewal_team.defaultrenewalperiod = 73
639 >>> otto = factory.makePerson(name='otto')
640 >>> ignore = autorenewal_team.addMember(otto, salgado)
641 >>> otto_membership = otto.myactivememberships[0]
642 >>> otto_membership.setExpirationDate(utc_now, salgado)
643 >>> original_otto_date_expires = otto_membership.dateexpires
644
645 >>> do_not_warn = factory.makePerson(name='do-not-warn')
646 >>> ignore = autorenewal_team.addMember(do_not_warn, salgado)
647 >>> do_not_warn.myactivememberships[0].setExpirationDate(
648 ... tomorrow, salgado)
649
650 # Not excluding teams with automatic renewals.
651 >>> [(membership.person.name, membership.team.name)
652 ... for membership in membershipset.getMembershipsToExpire()
653 ... if membership.team.name == 'autorenewal-team']
654 [(u'otto', u'autorenewal-team')]
655
656 # Excluding teams with automatic renewals.
657 >>> [(membership.person.name, membership.team.name)
658 ... for membership in membershipset.getMembershipsToExpire(
659 ... exclude_autorenewals=True)
660 ... if membership.team.name == 'autorenewal-team']
661 []
662
625Now we commit the changes and run the cronscript.663Now we commit the changes and run the cronscript.
626XXX: flush_database_updates() shouldn't be needed. This seems to be664XXX: flush_database_updates() shouldn't be needed. This seems to be
627Bug 3989 -- StuarBishop 20060713665Bug 3989 -- StuarBishop 20060713
@@ -631,12 +669,21 @@
631669
632 >>> import subprocess670 >>> import subprocess
633 >>> process = subprocess.Popen(671 >>> process = subprocess.Popen(
634 ... 'cronscripts/flag-expired-memberships.py -q', shell=True,672 ... 'cronscripts/flag-expired-memberships.py -v', shell=True,
635 ... stdin=subprocess.PIPE, stdout=subprocess.PIPE,673 ... stdin=subprocess.PIPE, stdout=subprocess.PIPE,
636 ... stderr=subprocess.PIPE)674 ... stderr=subprocess.PIPE)
637 >>> (out, err) = process.communicate()675 >>> (out, err) = process.communicate()
638 >>> out, err676 >>> print out
639 ('', '')677
678A warning email should not have been sent to otto because it was renewed
679inside the cronscript, and it should not be sent to do_not_warn, because
680teams with automatic renewal don't need warnings.
681
682 >>> print '\n'.join(
683 ... line for line in err.split('\n') if 'INFO' not in line)
684 DEBUG Sent warning email to name16 in launchpad-buildd-admins team.
685 DEBUG Sent warning email to name12 in landscape-developers team.
686 DEBUG Removing lock file: /var/lock/launchpad-flag-expired-memberships.lock
640 >>> process.returncode687 >>> process.returncode
641 0688 0
642 >>> transaction.abort()689 >>> transaction.abort()
@@ -657,6 +704,13 @@
657 >>> sp_on_ubuntu_translators.status.title704 >>> sp_on_ubuntu_translators.status.title
658 'Deactivated'705 'Deactivated'
659706
707 >>> otto_on_autorenewal_team = membershipset.getByPersonAndTeam(
708 ... otto, autorenewal_team)
709 >>> otto_on_autorenewal_team.status.title
710 'Approved'
711 >>> otto_on_autorenewal_team.dateexpires - original_otto_date_expires
712 datetime.timedelta(73)
713
660714
661== Renewing team memberships ==715== Renewing team memberships ==
662716
663717
=== modified file 'lib/lp/registry/model/teammembership.py'
--- lib/lp/registry/model/teammembership.py 2010-01-04 17:15:00 +0000
+++ lib/lp/registry/model/teammembership.py 2010-07-26 16:30:59 +0000
@@ -21,6 +21,7 @@
2121
22from sqlobject import ForeignKey, StringCol22from sqlobject import ForeignKey, StringCol
2323
24from canonical.launchpad.interfaces.lpstorm import IStore
24from canonical.database.sqlbase import (25from canonical.database.sqlbase import (
25 flush_database_updates, SQLBase, sqlvalues)26 flush_database_updates, SQLBase, sqlvalues)
26from canonical.database.constants import UTC_NOW27from canonical.database.constants import UTC_NOW
@@ -97,7 +98,7 @@
97 ondemand = TeamMembershipRenewalPolicy.ONDEMAND98 ondemand = TeamMembershipRenewalPolicy.ONDEMAND
98 admin = TeamMembershipStatus.APPROVED99 admin = TeamMembershipStatus.APPROVED
99 approved = TeamMembershipStatus.ADMIN100 approved = TeamMembershipStatus.ADMIN
100 date_limit = datetime.now(pytz.timezone('UTC')) + timedelta(101 date_limit = datetime.now(pytz.UTC) + timedelta(
101 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)102 days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)
102 return (self.status in (admin, approved)103 return (self.status in (admin, approved)
103 and self.team.renewal_policy == ondemand104 and self.team.renewal_policy == ondemand
@@ -208,6 +209,12 @@
208 assert self.dateexpires > datetime.now(pytz.timezone('UTC')), (209 assert self.dateexpires > datetime.now(pytz.timezone('UTC')), (
209 "This membership's expiration date must be in the future: %s"210 "This membership's expiration date must be in the future: %s"
210 % self.dateexpires.strftime('%Y-%m-%d'))211 % self.dateexpires.strftime('%Y-%m-%d'))
212 if self.team.renewal_policy == TeamMembershipRenewalPolicy.AUTOMATIC:
213 # An email will be sent later by handleMembershipsExpiringToday()
214 # when the membership is automatically renewed.
215 raise AssertionError(
216 'Team %r with automatic renewals should not send expiration '
217 'warnings.' % self.team.name)
211 member = self.person218 member = self.person
212 team = self.team219 team = self.team
213 if member.isTeam():220 if member.isTeam():
@@ -522,14 +529,22 @@
522 """See `ITeamMembershipSet`."""529 """See `ITeamMembershipSet`."""
523 return TeamMembership.selectOneBy(person=person, team=team)530 return TeamMembership.selectOneBy(person=person, team=team)
524531
525 def getMembershipsToExpire(self, when=None):532 def getMembershipsToExpire(self, when=None, exclude_autorenewals=False):
526 """See `ITeamMembershipSet`."""533 """See `ITeamMembershipSet`."""
527 if when is None:534 if when is None:
528 when = datetime.now(pytz.timezone('UTC'))535 when = datetime.now(pytz.timezone('UTC'))
529 query = ("date_expires <= %s AND status IN (%s, %s)"536 conditions = [
530 % sqlvalues(when, TeamMembershipStatus.ADMIN,537 TeamMembership.dateexpires <= when,
531 TeamMembershipStatus.APPROVED))538 TeamMembership.status.is_in(
532 return TeamMembership.select(query)539 [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED])
540 ]
541 if exclude_autorenewals:
542 # Avoid circular import.
543 from lp.registry.model.person import Person
544 conditions.append(TeamMembership.team == Person.id)
545 conditions.append(
546 Person.renewal_policy != TeamMembershipRenewalPolicy.AUTOMATIC)
547 return IStore(TeamMembership).find(TeamMembership, *conditions)
533548
534549
535class TeamParticipation(SQLBase):550class TeamParticipation(SQLBase):