From 6c621405c88671a58ef24cd84a9bd74ca324207e Mon Sep 17 00:00:00 2001 From: J08nY Date: Wed, 5 Jul 2017 01:07:37 +0200 Subject: Migrate the [un]subscription_policy attribute. - This is quite a huge commit, since it changes the type of the MailingList.subscription_policy and unsubscription_policy attributes to the new names of pluggable workflows, in all occurences. - Also adds a migration to migrate the attributes to the new types. - Adds tests for the migration. --- src/mailman/rest/tests/test_listconf.py | 6 +++--- src/mailman/rest/tests/test_membership.py | 19 +++++-------------- src/mailman/rest/tests/test_moderation.py | 7 +++---- src/mailman/rest/tests/test_users.py | 2 +- 4 files changed, 12 insertions(+), 22 deletions(-) (limited to 'src/mailman/rest/tests') diff --git a/src/mailman/rest/tests/test_listconf.py b/src/mailman/rest/tests/test_listconf.py index 782effd29..bb88feb61 100644 --- a/src/mailman/rest/tests/test_listconf.py +++ b/src/mailman/rest/tests/test_listconf.py @@ -22,11 +22,11 @@ import unittest from mailman.app.lifecycle import create_list from mailman.database.transaction import transaction from mailman.interfaces.digests import DigestFrequency -from mailman.interfaces.mailinglist import ( - IAcceptableAliasSet, SubscriptionPolicy) +from mailman.interfaces.mailinglist import IAcceptableAliasSet from mailman.interfaces.template import ITemplateManager from mailman.testing.helpers import call_api from mailman.testing.layers import RESTLayer +from mailman.workflows.subscription import ConfirmModerationSubscriptionPolicy from urllib.error import HTTPError from zope.component import getUtility @@ -197,7 +197,7 @@ class TestConfiguration(unittest.TestCase): self.assertEqual(response.status_code, 204) # And now we verify that it has the requested setting. self.assertEqual(self._mlist.subscription_policy, - SubscriptionPolicy.confirm_then_moderate) + ConfirmModerationSubscriptionPolicy) def test_patch_attribute_double(self): with self.assertRaises(HTTPError) as cm: diff --git a/src/mailman/rest/tests/test_membership.py b/src/mailman/rest/tests/test_membership.py index e5a2ce283..bb0134ac5 100644 --- a/src/mailman/rest/tests/test_membership.py +++ b/src/mailman/rest/tests/test_membership.py @@ -23,7 +23,6 @@ from mailman.app.lifecycle import create_list from mailman.config import config from mailman.database.transaction import transaction from mailman.interfaces.bans import IBanManager -from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.subscriptions import ISubscriptionManager, TokenOwner from mailman.interfaces.usermanager import IUserManager @@ -33,6 +32,8 @@ from mailman.testing.helpers import ( set_preferred, subscribe, wait_for_webservice) from mailman.testing.layers import ConfigLayer, RESTLayer from mailman.utilities.datetime import now +from mailman.workflows.subscription import ( + ConfirmModerationSubscriptionPolicy, ModerationSubscriptionPolicy) from urllib.error import HTTPError from zope.component import getUtility @@ -93,7 +94,6 @@ class TestMembership(unittest.TestCase): 'subscriber': 'anne@example.com', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, 'Member already subscribed') @@ -108,7 +108,6 @@ class TestMembership(unittest.TestCase): 'subscriber': 'anne@example.com', 'pre_verified': False, 'pre_confirmed': False, - 'pre_approved': False, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, 'Member already subscribed') @@ -123,7 +122,6 @@ class TestMembership(unittest.TestCase): 'subscriber': '00000000000000000000000000000001', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(cm.exception.code, 400) self.assertEqual(cm.exception.reason, 'User has no preferred address') @@ -135,7 +133,6 @@ class TestMembership(unittest.TestCase): 'subscriber': '00000000000000000000000000000801', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(cm.exception.code, 400) self.assertEqual(cm.exception.reason, 'No such user') @@ -153,7 +150,6 @@ class TestMembership(unittest.TestCase): 'subscriber': 'ANNE@example.com', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, 'Member already subscribed') @@ -171,7 +167,6 @@ class TestMembership(unittest.TestCase): 'subscriber': 'anne@example.com', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, 'Member already subscribed') @@ -195,7 +190,6 @@ class TestMembership(unittest.TestCase): 'display_name': 'Hugh Person', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(json, None) self.assertEqual(response.status_code, 201) @@ -231,10 +225,10 @@ class TestMembership(unittest.TestCase): # to subscribe again. registrar = ISubscriptionManager(self._mlist) with transaction(): - self._mlist.subscription_policy = SubscriptionPolicy.moderate + self._mlist.subscription_policy = ModerationSubscriptionPolicy anne = self._usermanager.create_address('anne@example.com') token, token_owner, member = registrar.register( - anne, pre_verified=True, pre_confirmed=True) + anne, pre_verified=True) self.assertEqual(token_owner, TokenOwner.moderator) self.assertIsNone(member) with self.assertRaises(HTTPError) as cm: @@ -242,7 +236,6 @@ class TestMembership(unittest.TestCase): 'list_id': 'test.example.com', 'subscriber': 'anne@example.com', 'pre_verified': True, - 'pre_confirmed': True, }) self.assertEqual(cm.exception.code, 409) self.assertEqual(cm.exception.reason, @@ -255,7 +248,7 @@ class TestMembership(unittest.TestCase): registrar = ISubscriptionManager(self._mlist) with transaction(): self._mlist.subscription_policy = ( - SubscriptionPolicy.confirm_then_moderate) + ConfirmModerationSubscriptionPolicy) anne = self._usermanager.create_address('anne@example.com') token, token_owner, member = registrar.register( anne, pre_verified=True) @@ -645,7 +638,6 @@ class TestAPI31Members(unittest.TestCase): 'subscriber': '00000000000000000000000000000001', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) self.assertEqual(response.status_code, 201) self.assertEqual( @@ -681,7 +673,6 @@ class TestAPI31Members(unittest.TestCase): 'subscriber': '1', 'pre_verified': True, 'pre_confirmed': True, - 'pre_approved': True, }) # This is a bad request because the `subscriber` value isn't something # that's known to the system, in API 3.1. It's not technically a 404 diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py index 0a9bb2608..5b55eb575 100644 --- a/src/mailman/rest/tests/test_moderation.py +++ b/src/mailman/rest/tests/test_moderation.py @@ -24,7 +24,6 @@ from mailman.app.lifecycle import create_list from mailman.app.moderator import hold_message from mailman.database.transaction import transaction from mailman.interfaces.bans import IBanManager -from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.requests import IListRequests, RequestType from mailman.interfaces.subscriptions import ISubscriptionManager from mailman.interfaces.usermanager import IUserManager @@ -32,6 +31,7 @@ from mailman.testing.helpers import ( call_api, get_queue_messages, set_preferred, specialized_message_from_string as mfs) from mailman.testing.layers import RESTLayer +from mailman.workflows.subscription import ModerationSubscriptionPolicy from pkg_resources import resource_filename from urllib.error import HTTPError from zope.component import getUtility @@ -296,10 +296,9 @@ class TestSubscriptionModeration(unittest.TestCase): # Anne tries to subscribe to a list that only requests moderator # approval. with transaction(): - self._mlist.subscription_policy = SubscriptionPolicy.moderate + self._mlist.subscription_policy = ModerationSubscriptionPolicy token, token_owner, member = self._registrar.register( - self._anne, - pre_verified=True, pre_confirmed=True) + self._anne, pre_verified=True) # There's now one request in the queue, and it's waiting on moderator # approval. json, response = call_api( diff --git a/src/mailman/rest/tests/test_users.py b/src/mailman/rest/tests/test_users.py index 97f4b76d2..4c97e5c1d 100644 --- a/src/mailman/rest/tests/test_users.py +++ b/src/mailman/rest/tests/test_users.py @@ -437,7 +437,7 @@ class TestLP1074374(unittest.TestCase): list_id='test.example.com', subscriber='anne@example.com', role='member', - pre_verified=True, pre_confirmed=True, pre_approved=True)) + pre_verified=True, pre_confirmed=True)) # This is not the Anne you're looking for. (IOW, the new Anne is a # different user). json, response = call_api( -- cgit v1.2.3-70-g09d2 From b902d7858d8302d248add89a5983c521c3581c4c Mon Sep 17 00:00:00 2001 From: J08nY Date: Tue, 25 Jul 2017 23:08:35 +0200 Subject: Add more tests for coverage. --- src/mailman/app/subscriptions.py | 2 +- src/mailman/model/tests/test_mailinglist.py | 24 +++++++++++++ src/mailman/rest/tests/test_validator.py | 44 ++++++++++++++++++++++- src/mailman/workflows/common.py | 2 +- src/mailman/workflows/tests/test_subscriptions.py | 14 +++++++- 5 files changed, 82 insertions(+), 4 deletions(-) (limited to 'src/mailman/rest/tests') diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index a0c0c8059..ba62e9f52 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -59,7 +59,7 @@ class SubscriptionManager: workflow_type = pendable.get('type') if workflow_type in {PendableSubscription.PEND_TYPE, PendableUnsubscription.PEND_TYPE}: - workflow = (self._mlist.subscription_policy + workflow = (self._mlist.subscription_policy # pragma: nocover if workflow_type == PendableSubscription.PEND_TYPE else self._mlist.unsubscription_policy)(self._mlist) else: diff --git a/src/mailman/model/tests/test_mailinglist.py b/src/mailman/model/tests/test_mailinglist.py index 72232cb53..a402c6dc4 100644 --- a/src/mailman/model/tests/test_mailinglist.py +++ b/src/mailman/model/tests/test_mailinglist.py @@ -28,6 +28,8 @@ from mailman.interfaces.mailinglist import ( from mailman.interfaces.member import ( AlreadySubscribedError, MemberRole, MissingPreferredAddressError) from mailman.interfaces.usermanager import IUserManager +from mailman.interfaces.workflows import ( + ISubscriptionWorkflow, IUnsubscriptionWorkflow) from mailman.testing.helpers import ( configuration, get_queue_messages, set_preferred) from mailman.testing.layers import ConfigLayer @@ -408,3 +410,25 @@ class TestHeaderMatch(unittest.TestCase): header_matches = IHeaderMatchList(self._mlist) with self.assertRaises(IndexError): del header_matches[0] + + def test_subscription_policy(self): + for workflow_class in config.workflows: + if ISubscriptionWorkflow.implementedBy(workflow_class): + self._mlist.subscription_policy = workflow_class.name + self.assertEqual(self._mlist.subscription_policy, + workflow_class) + + def test_subscription_policy_invalid(self): + with self.assertRaises(ValueError): + self._mlist.subscription_policy = 'not-a-subscription-policy' + + def test_unsubscription_policy(self): + for workflow_class in config.workflows: + if IUnsubscriptionWorkflow.implementedBy(workflow_class): + self._mlist.unsubscription_policy = workflow_class.name + self.assertEqual(self._mlist.unsubscription_policy, + workflow_class) + + def test_unsubscription_policy_invalid(self): + with self.assertRaises(ValueError): + self._mlist.unsubscription_policy = 'not-an-unsubscription-policy' diff --git a/src/mailman/rest/tests/test_validator.py b/src/mailman/rest/tests/test_validator.py index 0d197032e..95855d170 100644 --- a/src/mailman/rest/tests/test_validator.py +++ b/src/mailman/rest/tests/test_validator.py @@ -22,9 +22,14 @@ import unittest from mailman.core.api import API30, API31 from mailman.interfaces.action import Action from mailman.interfaces.usermanager import IUserManager +from mailman.interfaces.workflows import (ISubscriptionWorkflow, + IUnsubscriptionWorkflow) from mailman.rest.validator import ( - enum_validator, list_of_strings_validator, subscriber_validator) + enum_validator, list_of_strings_validator, policy_validator, + subscriber_validator) from mailman.testing.layers import RESTLayer +from mailman.workflows.subscription import ConfirmSubscriptionPolicy +from mailman.workflows.unsubscription import ConfirmUnsubscriptionPolicy from zope.component import getUtility @@ -91,3 +96,40 @@ class TestValidators(unittest.TestCase): def test_enum_validator_blank(self): self.assertEqual(enum_validator(Action, allow_blank=True)(''), None) + + def test_policy_validator_wrong_policy_class(self): + self.assertRaises(ValueError, policy_validator, None) + + def test_policy_validator_sub_name(self): + self.assertEqual(policy_validator(ISubscriptionWorkflow)( + ConfirmSubscriptionPolicy.name), ConfirmSubscriptionPolicy) + + def test_policy_validator_sub_class(self): + self.assertEqual(policy_validator(ISubscriptionWorkflow)( + ConfirmSubscriptionPolicy), ConfirmSubscriptionPolicy) + + def test_policy_validator_sub_backward_compat(self): + self.assertEqual(policy_validator(ISubscriptionWorkflow)('confirm'), + ConfirmSubscriptionPolicy) + + def test_policy_validator_sub_wrong_policy(self): + validator = policy_validator(ISubscriptionWorkflow) + with self.assertRaises(ValueError): + validator('not a subscription policy') + + def test_policy_validator_unsub_name(self): + self.assertEqual(policy_validator(IUnsubscriptionWorkflow)( + ConfirmUnsubscriptionPolicy.name), ConfirmUnsubscriptionPolicy) + + def test_policy_validator_unsub_class(self): + self.assertEqual(policy_validator(IUnsubscriptionWorkflow)( + ConfirmUnsubscriptionPolicy), ConfirmUnsubscriptionPolicy) + + def test_policy_validator_unsub_backward_compat(self): + self.assertEqual(policy_validator(IUnsubscriptionWorkflow)('confirm'), + ConfirmUnsubscriptionPolicy) + + def test_policy_validator_unsub_wrong_policy(self): + validator = policy_validator(IUnsubscriptionWorkflow) + with self.assertRaises(ValueError): + validator('not an unsubscription policy') diff --git a/src/mailman/workflows/common.py b/src/mailman/workflows/common.py index 1f63d3d1d..c250785c2 100644 --- a/src/mailman/workflows/common.py +++ b/src/mailman/workflows/common.py @@ -211,7 +211,7 @@ class SubscriptionBase(SubscriptionWorkflowCommon): # verified by a full coverage run, but diffcov for some reason # claims that the test added in the branch that added this code # does not cover the change. That seems like a bug in diffcov. - raise AlreadySubscribedError( # pragma: no cover + raise AlreadySubscribedError( # pragma: nocover self.mlist.fqdn_listname, self.address.email, MemberRole.member) diff --git a/src/mailman/workflows/tests/test_subscriptions.py b/src/mailman/workflows/tests/test_subscriptions.py index 23487ab1c..65569691b 100644 --- a/src/mailman/workflows/tests/test_subscriptions.py +++ b/src/mailman/workflows/tests/test_subscriptions.py @@ -24,7 +24,9 @@ from mailman.app.lifecycle import create_list from mailman.interfaces.bans import IBanManager from mailman.interfaces.member import MemberRole, MembershipIsBannedError from mailman.interfaces.pending import IPendings -from mailman.interfaces.subscriptions import TokenOwner +from mailman.interfaces.subscriptions import ( + SubscriptionPendingError, + TokenOwner) from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import ( LogFileMark, get_queue_messages, set_preferred) @@ -157,6 +159,16 @@ class TestSubscriptionWorkflow(unittest.TestCase): workflow = ConfirmSubscriptionPolicy(self._mlist, anne) self.assertRaises(MembershipIsBannedError, list, workflow) + def test_sanity_checks_already_requested(self): + # An exception is raised if there is already a subscription request. + anne = self._user_manager.create_address(self._anne) + workflow = ConfirmSubscriptionPolicy(self._mlist, anne) + list(workflow) + other_workflow = ConfirmSubscriptionPolicy(self._mlist, anne) + self.assertRaises(SubscriptionPendingError, list, other_workflow) + # The original workflow token is still in the database. + self._expected_pendings_count = 1 + def test_verification_checks_with_verified_address(self): # When the address is already verified, we skip straight to the # confirmation checks. -- cgit v1.2.3-70-g09d2