launchpad-reviewers team mailing list archive

[Edwin Grubbs <edwin.grubbs@xxxxxxxxxxxxx>]

Edwin Grubbs has proposed merging lp:~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #557036 Teams with membership auto-renewal send out expiration emails requesting user to contact admin
  https://bugs.launchpad.net/bugs/557036


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.

-- 
https://code.launchpad.net/~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams/+merge/29894
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~edwin-grubbs/launchpad/bug-557036-bad-email-for-autorenewal-teams into lp:launchpad/devel.

=== 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-14 15:34:47 +0000
@@ -41,8 +41,10 @@
             days=DAYS_BEFORE_EXPIRATION_WARNING_IS_SENT)
         self.txn.begin()
         for membership in membershipset.getMembershipsToExpire(
-                min_date_for_warning):
+            min_date_for_warning, exclude_autorenewals=True):
             membership.sendExpirationWarningEmail()
+            self.logger.debug("Sent warning email to %s in %s team."
+                          % (membership.person.name, membership.team.name))
         self.txn.commit()
 
     def main(self):
@@ -59,4 +61,3 @@
     script = ExpireMemberships('flag-expired-memberships',
                                dbuser=config.expiredmembershipsflagger.dbuser)
     script.lock_and_run()
-

=== 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-14 15:34:47 +0000
@@ -622,6 +622,44 @@
      (u'ubuntu-team', u'guadamen'), (u'name16', u'launchpad-buildd-admins'),
      (u'name12', u'landscape-developers')]
 
+
+The getMembershipsToExpire() method also accepts an optional
+'exclude_autorewals' argument.  When that argument is provided,
+memberships in teams that are configured to renew the membership
+automatically will be excluded. In that case, there is no reason to send
+a warning email several days before the expiration. The user will just
+be notified that the membership has already been automatically renewed
+on the expiration day.
+
+    >>> from storm.store import Store
+    >>> autorenewal_team = factory.makeTeam(name="autorenewal-team")
+    >>> autorenewal_team.renewal_policy = (
+    ...     TeamMembershipRenewalPolicy.AUTOMATIC)
+    >>> autorenewal_team.defaultrenewalperiod = 73
+    >>> otto = factory.makePerson(name='otto')
+    >>> ignore = autorenewal_team.addMember(otto, salgado)
+    >>> otto_membership = otto.myactivememberships[0]
+    >>> otto_membership.setExpirationDate(utc_now, salgado)
+    >>> original_otto_date_expires = otto_membership.dateexpires
+
+    >>> do_not_warn = factory.makePerson(name='do-not-warn')
+    >>> ignore = autorenewal_team.addMember(do_not_warn, salgado)
+    >>> do_not_warn.myactivememberships[0].setExpirationDate(
+    ...     tomorrow, salgado)
+
+    # Not excluding teams with automatic renewals.
+    >>> [(membership.person.name, membership.team.name)
+    ...     for membership in membershipset.getMembershipsToExpire()
+    ...     if membership.team.name == 'autorenewal-team']
+    [(u'otto', u'autorenewal-team')]
+
+    # Excluding teams with automatic renewals.
+    >>> [(membership.person.name, membership.team.name)
+    ...     for membership in membershipset.getMembershipsToExpire(
+    ...         exclude_autorenewals=True)
+    ...     if membership.team.name == 'autorenewal-team']
+    []
+
 Now we commit the changes and run the cronscript.
 XXX: flush_database_updates() shouldn't be needed. This seems to be
 Bug 3989 -- StuarBishop 20060713
@@ -631,12 +669,21 @@
 
     >>> import subprocess
     >>> process = subprocess.Popen(
-    ...     'cronscripts/flag-expired-memberships.py -q', shell=True,
+    ...     'cronscripts/flag-expired-memberships.py -v', shell=True,
     ...     stdin=subprocess.PIPE, stdout=subprocess.PIPE,
     ...     stderr=subprocess.PIPE)
     >>> (out, err) = process.communicate()
-    >>> out, err
-    ('', '')
+    >>> print out
+
+A warning email should not have been sent to otto because it was renewed
+inside the cronscript, and it should not be sent to do_not_warn, because
+teams with automatic renewal don't need warnings.
+
+    >>> print '\n'.join(
+    ...     line for line in err.split('\n') if 'INFO' not in line)
+    DEBUG   Sent warning email to name16 in launchpad-buildd-admins team.
+    DEBUG   Sent warning email to name12 in landscape-developers team.
+    DEBUG   Removing lock file: /var/lock/launchpad-flag-expired-memberships.lock
     >>> process.returncode
     0
     >>> transaction.abort()
@@ -657,6 +704,13 @@
     >>> sp_on_ubuntu_translators.status.title
     'Deactivated'
 
+    >>> otto_on_autorenewal_team = membershipset.getByPersonAndTeam(
+    ...     otto, autorenewal_team)
+    >>> otto_on_autorenewal_team.status.title
+    'Approved'
+    >>> otto_on_autorenewal_team.dateexpires - original_otto_date_expires
+    datetime.timedelta(73)
+
 
 == Renewing team memberships ==
 

=== 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-14 15:34:47 +0000
@@ -21,6 +21,7 @@
 
 from sqlobject import ForeignKey, StringCol
 
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.database.sqlbase import (
     flush_database_updates, SQLBase, sqlvalues)
 from canonical.database.constants import UTC_NOW
@@ -208,6 +209,12 @@
         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.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 '
+                'warnings.' % self.team.name)
         member = self.person
         team = self.team
         if member.isTeam():
@@ -522,14 +529,22 @@
         """See `ITeamMembershipSet`."""
         return TeamMembership.selectOneBy(person=person, team=team)
 
-    def getMembershipsToExpire(self, when=None):
+    def getMembershipsToExpire(self, when=None, exclude_autorenewals=False):
         """See `ITeamMembershipSet`."""
         if when is None:
             when = datetime.now(pytz.timezone('UTC'))
-        query = ("date_expires <= %s AND status IN (%s, %s)"
-                 % sqlvalues(when, TeamMembershipStatus.ADMIN,
-                             TeamMembershipStatus.APPROVED))
-        return TeamMembership.select(query)
+        conditions = [
+            TeamMembership.dateexpires <= when,
+            TeamMembership.status.is_in(
+                [TeamMembershipStatus.ADMIN, TeamMembershipStatus.APPROVED])
+            ]
+        if exclude_autorenewals:
+            # Avoid circular import.
+            from lp.registry.model.person import Person
+            conditions.append(TeamMembership.team == Person.id)
+            conditions.append(
+                Person.renewal_policy != TeamMembershipRenewalPolicy.AUTOMATIC)
+        return IStore(TeamMembership).find(TeamMembership, *conditions)
 
 
 class TeamParticipation(SQLBase):