Merge lp:~sinzui/launchpad/team-renewal-0 into lp:launchpad

[Curtis Hovey]

This is my branch to prevent impossibly default renewal periods.

    lp:~sinzui/launchpad/team-renewal-0
    Diff size: 146
    Launchpad bug:
          https://bugs.launchpad.net/bugs/630907
    Test command: ./bin/test -vv \
          -t TestDefaultRenewalPeriodIsRequiredForSomeTeams
    Pre-implementation: no one
    Target release: 10.10

Prevent impossibly default renewal periods
------------------------------------------

A user entered an integer that exceeds the expectations of postgresql;
The default renewal period for a team membership was set to be older
than the Himalayas. We expect the value in days to be a few months to a
few years.

Meanwhile, we have a constraint issue. There is an invariant guarding the
minimum number of days, there could be a maximum number of days

Rules
-----

    * Revise the invariant to raise Invalid if the days are greater than
      3650 (10 years).
      * There are a few insane cases in the DB where teams have set
        the value greater than a human life time. The teams will need to
        set the renewal policy to None. or revise the number if they choose
        to edit the team details.

QA
--

    * Choose the Change details link for a team you control.
    * Set the renewal policy to automatic or ondemand
    * Set the renewal period to 9999999999 in disregard of the text
      that asks for a number from 1 to 3650.
    * Verify the form states that it wants a number from 1 to 3650.

Lint
----

Linting changed files:
  lib/lp/registry/interfaces/person.py
  lib/lp/registry/tests/test_team.py

Test
----

    * lib/lp/registry/tests/test_team.py
      * Added a test to verify the invariant that governs membership
        policy and renewal period.

Implementation
--------------

    * lib/lp/registry/interfaces/person.py
      * Added a maximum day constraint and refactored the test to raise
        Invalid to be in two parts.

[Edwin Grubbs (edwin-grubbs)]

Hi Curtis,

This looks good. I just have a couple of comments below. Feel free to ignore the comment about changing the minimum number of days since that requires more work.

>=== modified file 'lib/lp/registry/interfaces/person.py'
>--- lib/lp/registry/interfaces/person.py 2010-08-22 03:09:51 +0000
>+++ lib/lp/registry/interfaces/person.py 2010-09-22 20:06:34 +0000
>@@ -182,15 +182,19 @@
>
> def validate_person(obj, attr, value):
> """Validate the person is a real person with no other restrictions."""
>+
> def validate(person):
> return IPerson.providedBy(person)
>+
> return validate_person_common(obj, attr, value, validate)
>
>
> def validate_public_person(obj, attr, value):
> """Validate that the person identified by value is public."""
>+
> def validate(person):
> return is_public_person(person)
>+
> return validate_person_common(obj, attr, value, validate)
>
>
>@@ -1630,13 +1634,18 @@
> return
>
> renewal_period = person.defaultrenewalperiod
>- automatic, ondemand = [TeamMembershipRenewalPolicy.AUTOMATIC,
>- TeamMembershipRenewalPolicy.ONDEMAND]
>- cannot_be_none = renewal_policy in [automatic, ondemand]
>- if ((renewal_period is None and cannot_be_none)
>- or (renewal_period is not None and renewal_period <= 0)):
>+ cannot_be_none = (

It seems like it would make more sense to call this
is_required_value_missing. Otherwise, it looks like a logic error when
you check whether renewal_period is None to decide whether it cannot be
None.

>+ renewal_period is None
>+ and renewal_policy in [
>+ TeamMembershipRenewalPolicy.AUTOMATIC,
>+ TeamMembershipRenewalPolicy.ONDEMAND])
>+ out_of_range = (
>+ renewal_period is not None
>+ and (renewal_period <= 0 or renewal_period > 3650))
>+ if cannot_be_none or out_of_range:
> raise Invalid(
>- 'You must specify a default renewal period greater than 0.')
>+ 'You must specify a default renewal period greater than 0 '
>+ 'and less than 3651 days.')

Wouldn't it be easier to read "from 1 to 3650 days"?
Also, if the renewal period is less than 2 days, the user may not get
the next renewal email if the cronjob runs before they renew it.

> teamdescription = exported(
> Text(title=_('Team Description'), required=False, readonly=False,
>@@ -1676,6 +1685,7 @@
> Int(title=_('Renewal period'), required=False,
> description=_(
> "Number of days a subscription lasts after being renewed. "
>+ "The number can be from 1 to 3650 (10 years). "
> "You can customize the lengths of individual renewals, but "
> "this is what's used for auto-renewed and user-renewed "
> "memberships.")),
>

review: Approve (code)

=== modified file 'lib/lp/registry/interfaces/person.py'
--- lib/lp/registry/interfaces/person.py	2010-08-22 03:09:51 +0000
+++ lib/lp/registry/interfaces/person.py	2010-09-23 13:01:29 +0000
@@ -182,15 +182,19 @@
 
 def validate_person(obj, attr, value):
     """Validate the person is a real person with no other restrictions."""
+
     def validate(person):
         return IPerson.providedBy(person)
+
     return validate_person_common(obj, attr, value, validate)
 
 
 def validate_public_person(obj, attr, value):
     """Validate that the person identified by value is public."""
+
     def validate(person):
         return is_public_person(person)
+
     return validate_person_common(obj, attr, value, validate)
 
 
@@ -1630,13 +1634,18 @@
             return
 
         renewal_period = person.defaultrenewalperiod
-        automatic, ondemand = [TeamMembershipRenewalPolicy.AUTOMATIC,
-                               TeamMembershipRenewalPolicy.ONDEMAND]
-        cannot_be_none = renewal_policy in [automatic, ondemand]
-        if ((renewal_period is None and cannot_be_none)
-            or (renewal_period is not None and renewal_period <= 0)):
+        is_required_value_missing = (
+            renewal_period is None
+            and renewal_policy in [
+                TeamMembershipRenewalPolicy.AUTOMATIC,
+                TeamMembershipRenewalPolicy.ONDEMAND])
+        out_of_range = (
+            renewal_period is not None
+            and (renewal_period <= 0 or renewal_period > 3650))
+        if is_required_value_missing or out_of_range:
             raise Invalid(
-                'You must specify a default renewal period greater than 0.')
+                'You must specify a default renewal period '
+                'from 1 to 3650 days.')
 
     teamdescription = exported(
         Text(title=_('Team Description'), required=False, readonly=False,
@@ -1664,7 +1673,7 @@
                default=TeamMembershipRenewalPolicy.NONE))
 
     defaultmembershipperiod = exported(
-        Int(title=_('Subscription period'), required=False,
+        Int(title=_('Subscription period'), required=False, max=3650,
             description=_(
                 "Number of days a new subscription lasts before expiring. "
                 "You can customize the length of an individual subscription "
@@ -1676,6 +1685,7 @@
         Int(title=_('Renewal period'), required=False,
             description=_(
                 "Number of days a subscription lasts after being renewed. "
+                "The number can be from 1 to 3650 (10 years). "
                 "You can customize the lengths of individual renewals, but "
                 "this is what's used for auto-renewed and user-renewed "
                 "memberships.")),

=== modified file 'lib/lp/registry/stories/team/xx-team-edit.txt'
--- lib/lp/registry/stories/team/xx-team-edit.txt	2009-12-07 16:07:43 +0000
+++ lib/lp/registry/stories/team/xx-team-edit.txt	2010-09-23 13:01:29 +0000
@@ -1,4 +1,5 @@
-= Editing team details =
+Editing team details
+====================
 
 Only an administrator (as well as the team owner) of a team can change the
 details of that team.
@@ -65,23 +66,9 @@
     There is 1 error.
     mark is already in use by another person or team.
 
-We also get an error message if we attempt to create a team with a renewal
-policy other than None and don't specify a default renewal period.
-
-    >>> browser.getControl('Name', index=0).value = 'ubuntuteam'
-    >>> browser.getControl(
-    ...     'invite them to renew their own membership').selected = True
-    >>> browser.getControl('Save').click()
-
-    >>> browser.url
-    'http://launchpad.dev/%7Eubuntuteam/+edit'
-    >>> for tag in find_tags_by_class(browser.contents, 'message'):
-    ...     print extract_text(tag)
-    There is 1 error.
-    You must specify a default renewal period greater than 0.
-
-
-== Teams with mailing lists may not be renamed ==
+
+Teams with mailing lists may not be renamed
+-------------------------------------------
 
 Because renaming mailing lists is non-trivial in Mailman 2.1, renaming teams
 with mailing lists is prohibited.  This is done by making the 'name' field of
@@ -92,14 +79,9 @@
     >>> browser.getControl('Apply for Mailing List').click()
 
     # Approval of mailing lists is not yet exposed through the web.
-    >>> from zope.component import getUtility
-    >>> from canonical.launchpad.interfaces import (
-    ...     MailingListStatus, ILaunchpadCelebrities)
     >>> from lp.registry.model.mailinglist import MailingListSet
     >>> from canonical.launchpad.ftests import login, logout
     >>> login('[e-mail]')
-    >>> experts = getUtility(ILaunchpadCelebrities).mailing_list_experts
-    >>> admin = list(experts.allmembers)[0]
     >>> mailing_list = MailingListSet().get('landscape-developers')
     >>> mailing_list.syncUpdate()
     >>> logout()
@@ -132,12 +114,12 @@
     'guadamen'
 
 
-== Private teams may not be renamed ==
+Private teams may not be renamed
+--------------------------------
 
     >>> # Create the necessary teams.
     >>> login('[e-mail]')
-    >>> from lp.registry.interfaces.person import (
-    ...     IPersonSet, PersonVisibility)
+    >>> from lp.registry.interfaces.person import PersonVisibility
     >>> owner = factory.makePerson(name='team-owner')
     >>> priv_team = factory.makeTeam(name='private-team',
     ...     owner=owner,
@@ -161,7 +143,8 @@
     ...
 
 
-== Multiple conditions blocking team renaming ==
+Multiple conditions blocking team renaming
+------------------------------------------
 
 Since public teams can have mailing lists and PPAs simultaneously,
 there will be scenarios where more than one of these conditions are

=== removed file 'lib/lp/registry/stories/teammembership/xx-new-team.txt'
--- lib/lp/registry/stories/teammembership/xx-new-team.txt	2009-09-18 15:24:30 +0000
+++ lib/lp/registry/stories/teammembership/xx-new-team.txt	1970-01-01 00:00:00 +0000
@@ -1,68 +0,0 @@
-== Validating teams ==
-
-The team renewal period cannot be less than 1 day, and when the renewal
-policy is is On Demand or Automatic, it cannot be None. The renewal
-period is checked during creation and editing.
-
-Sample Person cannot create a team without a renewal period when
-he chooses a renewal policy of Automatic. He is shown an error
-message when the from is redisplayed.
-
-    >>> browser = setupBrowser(auth='Basic [e-mail]:test')
-
-    >>> browser.open('http://launchpad.dev/people/')
-    >>> browser.getLink('Register a team').click()
-
-    >>> browser.title
-    'Register a new team in Launchpad'
-
-    >>> browser.getControl(name='field.name').value = 'newtestteam'
-    >>> browser.getControl('Display Name').value = 'New Test Team'
-    >>> browser.getControl(
-    ...     'Contact Email Address').value = '[e-mail]'
-    >>> browser.getControl('Team Description').value = 'The team description'
-    >>> browser.getControl('Subscription period').value = '365'
-    >>> browser.getControl(
-    ...     'renew their membership automatically, '
-    ...     'also notifying the admins').selected = True
-    >>> browser.getControl('Create').click()
-    >>> browser.title
-    'Register a new team in Launchpad'
-    
-    >>> for message in get_feedback_messages(browser.contents):
-    ...     print message
-    There is 1 error.
-    You must specify a default renewal period greater than 0.
-
-Changing the renewal policy to On Demand also redisplays the form with
-an error message.
-    
-    >>> browser.getControl(
-    ...     'invite them to renew their own membership').selected = True
-    >>> browser.getControl('Create').click()
-    
-    >>> for message in get_feedback_messages(browser.contents):
-    ...     print message
-    There is 1 error.
-    You must specify a default renewal period greater than 0.
-
-Sample Person finaly sets a sane renewal period and created the team.
-
-    >>> browser.getControl('Renewal period').value = '365'
-    >>> browser.getControl('Create').click()
-    >>> browser.title
-    'New Test Team in Launchpad'
-
-Sample Person cannot change the renewal period 'New Test Team' to zero.
-
-    >>> browser.getLink('Change details').click()
-    >>> browser.title
-    'Edit "New Test Team" team...
-
-    >>> browser.getControl('Renewal period').value = '0'
-    >>> browser.getControl('Save').click()
-    
-    >>> for message in get_feedback_messages(browser.contents):
-    ...     print message
-    There is 1 error.
-    You must specify a default renewal period greater than 0.

=== modified file 'lib/lp/registry/tests/test_team.py'
--- lib/lp/registry/tests/test_team.py	2010-08-20 20:31:18 +0000
+++ lib/lp/registry/tests/test_team.py	2010-09-23 13:01:29 +0000
@@ -7,12 +7,17 @@
 
 import transaction
 from zope.component import getUtility
+from zope.interface.exceptions import Invalid
 
 from canonical.launchpad.database.emailaddress import EmailAddress
 from canonical.launchpad.interfaces.emailaddress import IEmailAddressSet
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from canonical.testing import DatabaseFunctionalLayer
 from lp.registry.interfaces.mailinglist import MailingListStatus
+from lp.registry.interfaces.person import (
+    ITeamPublic,
+    TeamMembershipRenewalPolicy,
+    )
 from lp.testing import (
     login_celebrity,
     login_person,
@@ -38,6 +43,7 @@
 
     def setUp(self):
         super(TestTeamContactAddress, self).setUp()
+
         self.team = self.factory.makeTeam(name='alpha')
         self.address = self.factory.makeEmail('[e-mail]', self.team)
         self.store = IMasterStore(self.address)
@@ -102,3 +108,81 @@
         self.team.setContactAddress(None)
         self.assertEqual(None, self.team.preferredemail)
         self.assertEqual([], self.getAllEmailAddresses())
+
+
+class TestDefaultRenewalPeriodIsRequiredForSomeTeams(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDefaultRenewalPeriodIsRequiredForSomeTeams, self).setUp()
+        self.team = self.factory.makeTeam()
+        login_person(self.team.teamowner)
+
+    def assertInvalid(self, policy, period):
+        self.team.renewal_policy = policy
+        self.team.defaultrenewalperiod = period
+        self.assertRaises(Invalid, ITeamPublic.validateInvariants, self.team)
+
+    def assertValid(self, policy, period):
+        self.team.renewal_policy = policy
+        self.team.defaultrenewalperiod = period
+        ITeamPublic.validateInvariants(self.team)
+
+    def test_policy_automatic_period_none(self):
+        # Automatic policy cannot have a none day period.
+        self.assertInvalid(
+            TeamMembershipRenewalPolicy.AUTOMATIC, None)
+
+    def test_policy_ondemand_period_none(self):
+        # Ondemand policy cannot have a none day period.
+        self.assertInvalid(
+            TeamMembershipRenewalPolicy.ONDEMAND, None)
+
+    def test_policy_none_period_none(self):
+        # None policy can have a None day period.
+        self.assertValid(
+            TeamMembershipRenewalPolicy.NONE, None)
+
+    def test_policy_requres_period_below_minimum(self):
+        # Automatic and ondemand policy cannot have a zero day period.
+        self.assertInvalid(
+            TeamMembershipRenewalPolicy.AUTOMATIC, 0)
+
+    def test_policy_requres_period_minimum(self):
+        # Automatic and ondemand policy can have a 1 day period.
+        self.assertValid(
+            TeamMembershipRenewalPolicy.AUTOMATIC, 1)
+
+    def test_policy_requres_period_maximum(self):
+        # Automatic and ondemand policy cannot have a 3650 day max value.
+        self.assertValid(
+            TeamMembershipRenewalPolicy.AUTOMATIC, 3650)
+
+    def test_policy_requres_period_over_maximum(self):
+        # Automatic and ondemand policy cannot have a 3650 day max value.
+        self.assertInvalid(
+            TeamMembershipRenewalPolicy.AUTOMATIC, 3651)
+
+
+class TestDefaultMembershipPeriod(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDefaultMembershipPeriod, self).setUp()
+        self.team = self.factory.makeTeam()
+        login_person(self.team.teamowner)
+
+    def test_default_membership_period_over_maximum(self):
+        self.assertRaises(
+            Invalid, ITeamPublic['defaultmembershipperiod'].validate, 3651)
+
+    def test_default_membership_period_none(self):
+        ITeamPublic['defaultmembershipperiod'].validate(None)
+
+    def test_default_membership_period_zero(self):
+        ITeamPublic['defaultmembershipperiod'].validate(0)
+
+    def test_default_membership_period_maximum(self):
+        ITeamPublic['defaultmembershipperiod'].validate(3650)