launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01160
[Merge] lp:~sinzui/launchpad/team-renewal-0 into lp:launchpad/devel
Curtis Hovey has proposed merging lp:~sinzui/launchpad/team-renewal-0 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#630907 DataError: integer out of range when defaultrenewalperiod has a ridiculous number
https://bugs.launchpad.net/bugs/630907
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.
--
https://code.launchpad.net/~sinzui/launchpad/team-renewal-0/+merge/36246
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/team-renewal-0 into lp:launchpad/devel.
=== 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 03:35:55 +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 = (
+ 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.')
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.")),
=== 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-22 03:35:55 +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('team@xxxxxxxxxxx', self.team)
self.store = IMasterStore(self.address)
@@ -102,3 +108,58 @@
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)
