From 34975c2d425e428d06c73f7ecae95e6058e5d058 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 20 Mar 2015 16:31:41 +0100 Subject: Resurrect Barry's subpolicy branch (lp:~barry/mailman/subpolicy) --- src/mailman/app/docs/moderator.rst | 30 +++--- src/mailman/app/membership.py | 137 ++++++++++++------------ src/mailman/app/moderator.py | 57 +++++----- src/mailman/app/subscriptions.py | 140 +++++++++++++++++++++--- src/mailman/app/tests/test_bounces.py | 21 ++-- src/mailman/app/tests/test_membership.py | 159 ++++++++++++++++------------ src/mailman/app/tests/test_moderation.py | 30 +++++- src/mailman/app/tests/test_notifications.py | 38 ++++--- src/mailman/app/tests/test_subscriptions.py | 35 +++++- src/mailman/chains/docs/moderation.rst | 9 +- src/mailman/commands/cli_members.py | 12 +-- src/mailman/commands/docs/members.rst | 34 ++---- src/mailman/interfaces/mailinglist.py | 16 +++ src/mailman/interfaces/subscriptions.py | 16 +++ src/mailman/interfaces/usermanager.py | 16 +++ src/mailman/model/docs/usermanager.rst | 22 ++++ src/mailman/model/mailinglist.py | 6 +- src/mailman/model/roster.py | 4 +- src/mailman/model/tests/test_usermanager.py | 42 +++++++- src/mailman/model/usermanager.py | 21 ++++ src/mailman/mta/tests/test_delivery.py | 9 +- src/mailman/rest/docs/moderation.rst | 25 ++--- src/mailman/rest/root.py | 1 + src/mailman/rest/tests/test_moderation.py | 11 +- src/mailman/runners/docs/outgoing.rst | 19 +--- src/mailman/styles/base.py | 4 +- src/mailman/templates/en/unsubauth.txt | 2 +- src/mailman/testing/helpers.py | 33 ++++-- 28 files changed, 636 insertions(+), 313 deletions(-) diff --git a/src/mailman/app/docs/moderator.rst b/src/mailman/app/docs/moderator.rst index 490c9630a..82d29074a 100644 --- a/src/mailman/app/docs/moderator.rst +++ b/src/mailman/app/docs/moderator.rst @@ -238,14 +238,16 @@ Holding subscription requests For closed lists, subscription requests will also be held for moderator approval. In this case, several pieces of information related to the subscription must be provided, including the subscriber's address and real -name, their password (possibly hashed), what kind of delivery option they are -choosing and their preferred language. +name, what kind of delivery option they are choosing and their preferred +language. >>> from mailman.app.moderator import hold_subscription >>> from mailman.interfaces.member import DeliveryMode + >>> from mailman.interfaces.subscriptions import RequestRecord >>> req_id = hold_subscription( - ... mlist, 'fred@example.org', 'Fred Person', - ... '{NONE}abcxyz', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('fred@example.org', 'Fred Person', + ... DeliveryMode.regular, 'en')) Disposing of membership change requests @@ -269,8 +271,9 @@ The held subscription can also be discarded. Gwen tries to subscribe to the mailing list, but... >>> req_id = hold_subscription( - ... mlist, 'gwen@example.org', 'Gwen Person', - ... '{NONE}zyxcba', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('gwen@example.org', 'Gwen Person', + ... DeliveryMode.regular, 'en')) ...her request is rejected... @@ -305,8 +308,9 @@ mailing list. >>> mlist.send_welcome_message = False >>> req_id = hold_subscription( - ... mlist, 'herb@example.org', 'Herb Person', - ... 'abcxyz', DeliveryMode.regular, 'en') + ... mlist, + ... RequestRecord('herb@example.org', 'Herb Person', + ... DeliveryMode.regular, 'en')) The moderators accept the subscription request. @@ -399,8 +403,9 @@ list is configured to send them. Iris tries to subscribe to the mailing list. - >>> req_id = hold_subscription(mlist, 'iris@example.org', 'Iris Person', - ... 'password', DeliveryMode.regular, 'en') + >>> req_id = hold_subscription(mlist, + ... RequestRecord('iris@example.org', 'Iris Person', + ... DeliveryMode.regular, 'en')) There's now a message in the virgin queue, destined for the list owner. @@ -491,8 +496,9 @@ can get a welcome message. >>> mlist.admin_notify_mchanges = False >>> mlist.send_welcome_message = True - >>> req_id = hold_subscription(mlist, 'kate@example.org', 'Kate Person', - ... 'password', DeliveryMode.regular, 'en') + >>> req_id = hold_subscription(mlist, + ... RequestRecord('kate@example.org', 'Kate Person', + ... DeliveryMode.regular, 'en')) >>> handle_subscription(mlist, req_id, Action.accept) >>> messages = get_queue_messages('virgin') >>> len(messages) diff --git a/src/mailman/app/membership.py b/src/mailman/app/membership.py index 996493dc4..b64291d7c 100644 --- a/src/mailman/app/membership.py +++ b/src/mailman/app/membership.py @@ -40,8 +40,7 @@ from zope.component import getUtility -def add_member(mlist, email, display_name, password, delivery_mode, language, - role=MemberRole.member): +def add_member(mlist, record, role=MemberRole.member): """Add a member right now. The member's subscription must be approved by whatever policy the list @@ -49,16 +48,8 @@ def add_member(mlist, email, display_name, password, delivery_mode, language, :param mlist: The mailing list to add the member to. :type mlist: `IMailingList` - :param email: The email address to subscribe. - :type email: str - :param display_name: The subscriber's full name. - :type display_name: str - :param password: The subscriber's plain text password. - :type password: str - :param delivery_mode: The delivery mode the subscriber has chosen. - :type delivery_mode: DeliveryMode - :param language: The language that the subscriber is going to use. - :type language: str + :param record: a subscription request record. + :type record: RequestRecord :param role: The membership role for this subscription. :type role: `MemberRole` :return: The just created member. @@ -69,62 +60,74 @@ def add_member(mlist, email, display_name, password, delivery_mode, language, :raises MembershipIsBannedError: if the membership is not allowed. """ # Check to see if the email address is banned. - if IBanManager(mlist).is_banned(email): - raise MembershipIsBannedError(mlist, email) - # See if there's already a user linked with the given address. + if IBanManager(mlist).is_banned(record.email): + raise MembershipIsBannedError(mlist, record.email) + # Make sure there is a user linked with the given address. user_manager = getUtility(IUserManager) - user = user_manager.get_user(email) - if user is None: - # A user linked to this address does not yet exist. Is the address - # itself known but just not linked to a user? - address = user_manager.get_address(email) - if address is None: - # Nope, we don't even know about this address, so create both the - # user and address now. - user = user_manager.create_user(email, display_name) - # Do it this way so we don't have to flush the previous change. - address = list(user.addresses)[0] - else: - # The address object exists, but it's not linked to a user. - # Create the user and link it now. - user = user_manager.create_user() - user.display_name = ( - display_name if display_name else address.display_name) - user.link(address) - # Encrypt the password using the currently selected hash scheme. - user.password = config.password_context.encrypt(password) - user.preferences.preferred_language = language - member = mlist.subscribe(address, role) - member.preferences.delivery_mode = delivery_mode - else: - # The user exists and is linked to the case-insensitive address. - # We're looking for two versions of the email address, the case - # preserved version and the case insensitive version. We'll - # subscribe the version with matching case if it exists, otherwise - # we'll use one of the matching case-insensitively ones. It's - # undefined which one we pick. - case_preserved = None - case_insensitive = None - for address in user.addresses: - if address.original_email == email: - case_preserved = address - if address.email == email.lower(): - case_insensitive = address - assert case_preserved is not None or case_insensitive is not None, ( - 'Could not find a linked address for: {}'.format(email)) - address = (case_preserved if case_preserved is not None - else case_insensitive) - # Create the member and set the appropriate preferences. It's - # possible we're subscribing the lower cased version of the address; - # if that's already subscribed re-issue the exception with the correct - # email address (i.e. the one passed in here). - try: - member = mlist.subscribe(address, role) - except AlreadySubscribedError as error: - raise AlreadySubscribedError( - error.fqdn_listname, email, error.role) - member.preferences.preferred_language = language - member.preferences.delivery_mode = delivery_mode + user = user_manager.make_user(record.email, record.display_name) + # Encrypt the password using the currently selected hash scheme. + user.preferences.preferred_language = record.language + # Subscribe the address, not the user. + address = user_manager.get_address(record.email) + if address is None or address.user is not user: + raise AssertionError( + 'User should have had linked address: {0}'.format(address)) + # Create the member and set the appropriate preferences. + member = mlist.subscribe(address, role) + member.preferences.preferred_language = record.language + member.preferences.delivery_mode = record.delivery_mode +# user = user_manager.get_user(email) +# if user is None: +# # A user linked to this address does not yet exist. Is the address +# # itself known but just not linked to a user? +# address = user_manager.get_address(email) +# if address is None: +# # Nope, we don't even know about this address, so create both the +# # user and address now. +# user = user_manager.create_user(email, display_name) +# # Do it this way so we don't have to flush the previous change. +# address = list(user.addresses)[0] +# else: +# # The address object exists, but it's not linked to a user. +# # Create the user and link it now. +# user = user_manager.create_user() +# user.display_name = ( +# display_name if display_name else address.display_name) +# user.link(address) +# # Encrypt the password using the currently selected hash scheme. +# user.password = config.password_context.encrypt(password) +# user.preferences.preferred_language = language +# member = mlist.subscribe(address, role) +# member.preferences.delivery_mode = delivery_mode +# else: +# # The user exists and is linked to the case-insensitive address. +# # We're looking for two versions of the email address, the case +# # preserved version and the case insensitive version. We'll +# # subscribe the version with matching case if it exists, otherwise +# # we'll use one of the matching case-insensitively ones. It's +# # undefined which one we pick. +# case_preserved = None +# case_insensitive = None +# for address in user.addresses: +# if address.original_email == email: +# case_preserved = address +# if address.email == email.lower(): +# case_insensitive = address +# assert case_preserved is not None or case_insensitive is not None, ( +# 'Could not find a linked address for: {}'.format(email)) +# address = (case_preserved if case_preserved is not None +# else case_insensitive) +# # Create the member and set the appropriate preferences. It's +# # possible we're subscribing the lower cased version of the address; +# # if that's already subscribed re-issue the exception with the correct +# # email address (i.e. the one passed in here). +# try: +# member = mlist.subscribe(address, role) +# except AlreadySubscribedError as error: +# raise AlreadySubscribedError( +# error.fqdn_listname, email, error.role) +# member.preferences.preferred_language = language +# member.preferences.delivery_mode = delivery_mode return member diff --git a/src/mailman/app/moderator.py b/src/mailman/app/moderator.py index b55388e6a..0e4d59479 100644 --- a/src/mailman/app/moderator.py +++ b/src/mailman/app/moderator.py @@ -44,6 +44,7 @@ from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, NotAMemberError) from mailman.interfaces.messages import IMessageStore from mailman.interfaces.requests import IListRequests, RequestType +from mailman.interfaces.subscriptions import RequestRecord from mailman.utilities.datetime import now from mailman.utilities.i18n import make from zope.component import getUtility @@ -192,26 +193,26 @@ def handle_message(mlist, id, action, -def hold_subscription(mlist, address, display_name, password, mode, language): +def hold_subscription(mlist, record): data = dict(when=now().isoformat(), - address=address, - display_name=display_name, - password=password, - delivery_mode=mode.name, - language=language) - # Now hold this request. We'll use the address as the key. + email=record.email, + display_name=record.display_name, + delivery_mode=record.delivery_mode.name, + language=record.language) + # Now hold this request. We'll use the email address as the key. requestsdb = IListRequests(mlist) request_id = requestsdb.hold_request( - RequestType.subscription, address, data) + RequestType.subscription, record.email, data) vlog.info('%s: held subscription request from %s', - mlist.fqdn_listname, address) + mlist.fqdn_listname, record.email) # Possibly notify the administrator in default list language if mlist.admin_immed_notify: + email = record.email # XXX: seems unnecessary subject = _( - 'New subscription request to $mlist.display_name from $address') + 'New subscription request to $mlist.display_name from $email') text = make('subauth.txt', mailing_list=mlist, - username=address, + username=record.email, listname=mlist.fqdn_listname, admindb_url=mlist.script_url('admindb'), ) @@ -236,19 +237,19 @@ def handle_subscription(mlist, id, action, comment=None): elif action is Action.reject: key, data = requestdb.get_request(id) _refuse(mlist, _('Subscription request'), - data['address'], + data['email'], comment or _('[No reason given]'), lang=getUtility(ILanguageManager)[data['language']]) elif action is Action.accept: key, data = requestdb.get_request(id) delivery_mode = DeliveryMode[data['delivery_mode']] - address = data['address'] + email = data['email'] display_name = data['display_name'] language = getUtility(ILanguageManager)[data['language']] - password = data['password'] try: - add_member(mlist, address, display_name, password, - delivery_mode, language) + add_member( + mlist, + RequestRecord(email, display_name, delivery_mode, language)) except AlreadySubscribedError: # The address got subscribed in some other way after the original # request was made and accepted. @@ -256,9 +257,9 @@ def handle_subscription(mlist, id, action, comment=None): else: if mlist.admin_notify_mchanges: send_admin_subscription_notice( - mlist, address, display_name, language) + mlist, email, display_name, language) slog.info('%s: new %s, %s %s', mlist.fqdn_listname, - delivery_mode, formataddr((display_name, address)), + delivery_mode, formataddr((display_name, email)), 'via admin approval') else: raise AssertionError('Unexpected action: {0}'.format(action)) @@ -267,20 +268,20 @@ def handle_subscription(mlist, id, action, comment=None): -def hold_unsubscription(mlist, address): - data = dict(address=address) +def hold_unsubscription(mlist, email): + data = dict(email=email) requestsdb = IListRequests(mlist) request_id = requestsdb.hold_request( - RequestType.unsubscription, address, data) + RequestType.unsubscription, email, data) vlog.info('%s: held unsubscription request from %s', - mlist.fqdn_listname, address) + mlist.fqdn_listname, email) # Possibly notify the administrator of the hold if mlist.admin_immed_notify: subject = _( - 'New unsubscription request from $mlist.display_name by $address') + 'New unsubscription request from $mlist.display_name by $email') text = make('unsubauth.txt', mailing_list=mlist, - address=address, + email=email, listname=mlist.fqdn_listname, admindb_url=mlist.script_url('admindb'), ) @@ -297,7 +298,7 @@ def hold_unsubscription(mlist, address): def handle_unsubscription(mlist, id, action, comment=None): requestdb = IListRequests(mlist) key, data = requestdb.get_request(id) - address = data['address'] + email = data['email'] if action is Action.defer: # Nothing to do. return @@ -306,16 +307,16 @@ def handle_unsubscription(mlist, id, action, comment=None): pass elif action is Action.reject: key, data = requestdb.get_request(id) - _refuse(mlist, _('Unsubscription request'), address, + _refuse(mlist, _('Unsubscription request'), email, comment or _('[No reason given]')) elif action is Action.accept: key, data = requestdb.get_request(id) try: - delete_member(mlist, address) + delete_member(mlist, email) except NotAMemberError: # User has already been unsubscribed. pass - slog.info('%s: deleted %s', mlist.fqdn_listname, address) + slog.info('%s: deleted %s', mlist.fqdn_listname, email) else: raise AssertionError('Unexpected action: {0}'.format(action)) # Delete the request from the database. diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index bcb9e6585..eff7c12af 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -19,28 +19,35 @@ __all__ = [ 'SubscriptionService', + 'SubscriptionWorkflow', 'handle_ListDeletingEvent', ] +from collections import deque from operator import attrgetter -from passlib.utils import generate_password as generate +#from passlib.utils import generate_password as generate from sqlalchemy import and_, or_ from uuid import UUID from zope.component import getUtility from zope.interface import implementer from mailman.app.membership import add_member, delete_member -from mailman.config import config +#from mailman.config import config +from mailman.app.moderator import hold_subscription from mailman.core.constants import system_preferences from mailman.database.transaction import dbconnection +from mailman.interfaces.address import IAddress from mailman.interfaces.listmanager import ( IListManager, ListDeletingEvent, NoSuchListError) +from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.subscriptions import ( - ISubscriptionService, MissingUserError) + ISubscriptionService, MissingUserError, RequestRecord) +from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager from mailman.model.member import Member +from mailman.utilities.datetime import now @@ -53,6 +60,118 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) + +class SubscriptionWorkflow: + """Workflow of a subscription request.""" + + def __init__(self, mlist, subscriber, + pre_verified, pre_confirmed, pre_approved): + self.mlist = mlist + # The subscriber must be either an IUser or IAddress. + if IAddress.providedBy(subscriber): + self.address = subscriber + self.user = self.address.user + elif IUser.providedBy(subscriber): + self.address = subscriber.preferred_address + self.user = subscriber + self.subscriber = subscriber + self.pre_verified = pre_verified + self.pre_confirmed = pre_confirmed + self.pre_approved = pre_approved + # Prepare the state machine. + self._next = deque() + self._next.append(self._verification_check) + + def __iter__(self): + return self + + def _pop(self): + step = self._next.popleft() + # step could be a partial or a method. + name = getattr(step, 'func', step).__name__ + return step, name + + def __next__(self): + try: + step, name = self._pop() + step() + except IndexError: + raise StopIteration + except: + raise + + def _maybe_set_preferred_address(self): + if self.user is None: + # The address has no linked user so create one, link it, and set + # the user's preferred address. + assert self.address is not None, 'No address or user' + self.user = getUtility(IUserManager).make_user(self.address.email) + self.user.preferred_address = self.address + elif self.user.preferred_address is None: + assert self.address is not None, 'No address or user' + # The address has a linked user, but no preferred address is set + # yet. This is required, so use the address. + self.user.preferred_address = self.address + + def _verification_check(self): + if self.address.verified_on is not None: + # The address is already verified. Give the user a preferred + # address if it doesn't already have one. We may still have to do + # a subscription confirmation check. See below. + self._maybe_set_preferred_address() + else: + # The address is not yet verified. Maybe we're pre-verifying it. + # If so, we also want to give the user a preferred address if it + # doesn't already have one. We may still have to do a + # subscription confirmation check. See below. + if self.pre_verified: + self.address.verified_on = now() + self._maybe_set_preferred_address() + else: + # Since the address was not already verified, and not + # pre-verified, we have to send a confirmation check, which + # doubles as a verification step. Skip to that now. + self._next.append(self._send_confirmation) + return + self._next.append(self._confirmation_check) + + def _confirmation_check(self): + # Must the user confirm their subscription request? If the policy is + # open subscriptions, then we need neither confirmation nor moderator + # approval, so just subscribe them now. + if self.mlist.subscription_policy == SubscriptionPolicy.open: + self._next.append(self._do_subscription) + elif self.pre_confirmed: + # No confirmation is necessary. We can skip to seeing whether a + # moderator confirmation is necessary. + self._next.append(self._moderation_check) + else: + self._next.append(self._send_confirmation) + + def _moderation_check(self): + # Does the moderator need to approve the subscription request? + if self.mlist.subscription_policy in ( + SubscriptionPolicy.moderate, + SubscriptionPolicy.confirm_then_moderate): + self._next.append(self._get_moderator_approval) + else: + # The moderator does not need to approve the subscription, so go + # ahead and do that now. + self._next.append(self._do_subscription) + + def _get_moderator_approval(self): + # In order to get the moderator's approval, we need to hold the + # subscription request in the database + request = RequestRecord( + self.address.email, self.subscriber.display_name, + DeliveryMode.regular, 'en') + hold_subscription(self._mlist, request) + + def _do_subscription(self): + # We can immediately subscribe the user to the mailing list. + self.mlist.subscribe(self.subscriber) + + @implementer(ISubscriptionService) class SubscriptionService: @@ -148,16 +267,11 @@ class SubscriptionService: if isinstance(subscriber, str): if display_name is None: display_name, at, domain = subscriber.partition('@') - # Because we want to keep the REST API simple, there is no - # password or language given to us. We'll use the system's - # default language for the user's default language. We'll set the - # password to a system default. This will have to get reset since - # it can't be retrieved. Note that none of these are used unless - # the address is completely new to us. - password = generate(int(config.passwords.password_length)) - return add_member(mlist, subscriber, display_name, password, - delivery_mode, - system_preferences.preferred_language, role) + return add_member( + mlist, + RequestRecord(subscriber, display_name, delivery_mode, + system_preferences.preferred_language), + role) else: # We have to assume it's a UUID. assert isinstance(subscriber, UUID), 'Not a UUID' diff --git a/src/mailman/app/tests/test_bounces.py b/src/mailman/app/tests/test_bounces.py index a84ef24c2..119fcf7f9 100644 --- a/src/mailman/app/tests/test_bounces.py +++ b/src/mailman/app/tests/test_bounces.py @@ -36,15 +36,15 @@ import unittest from mailman.app.bounces import ( ProbeVERP, StandardVERP, bounce_message, maybe_forward, send_probe) from mailman.app.lifecycle import create_list -from mailman.app.membership import add_member from mailman.config import config from mailman.interfaces.bounce import UnrecognizedBounceDisposition from mailman.interfaces.languages import ILanguageManager -from mailman.interfaces.member import DeliveryMode, MemberRole +from mailman.interfaces.member import MemberRole from mailman.interfaces.pending import IPendings from mailman.interfaces.usermanager import IUserManager from mailman.testing.helpers import ( - LogFileMark, get_queue_messages, specialized_message_from_string as mfs) + LogFileMark, get_queue_messages, specialized_message_from_string as mfs, + subscribe_ex) from mailman.testing.layers import ConfigLayer from zope.component import getUtility @@ -193,9 +193,8 @@ class TestSendProbe(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') self._mlist.send_welcome_message = False - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = subscribe_ex( + self._mlist, 'Anne', email='anne@example.com') self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com @@ -285,9 +284,8 @@ class TestSendProbeNonEnglish(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = subscribe_ex( + self._mlist, 'Anne', email='anne@example.com') self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com @@ -351,9 +349,8 @@ class TestProbe(unittest.TestCase): def setUp(self): self._mlist = create_list('test@example.com') self._mlist.send_welcome_message = False - self._member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'xxx', - DeliveryMode.regular, 'en') + self._member = subscribe_ex( + self._mlist, 'Anne', email='anne@example.com') self._msg = mfs("""\ From: bouncer@example.com To: anne@example.com diff --git a/src/mailman/app/tests/test_membership.py b/src/mailman/app/tests/test_membership.py index cdf0641ea..73e2ced07 100644 --- a/src/mailman/app/tests/test_membership.py +++ b/src/mailman/app/tests/test_membership.py @@ -19,7 +19,6 @@ __all__ = [ 'TestAddMember', - 'TestAddMemberPassword', 'TestDeleteMember', ] @@ -33,6 +32,7 @@ from mailman.interfaces.bans import IBanManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, MemberRole, MembershipIsBannedError, NotAMemberError) +from mailman.interfaces.subscriptions import RequestRecord from mailman.interfaces.usermanager import IUserManager from mailman.testing.layers import ConfigLayer from zope.component import getUtility @@ -48,9 +48,11 @@ class TestAddMember(unittest.TestCase): def test_add_member_new_user(self): # Test subscribing a user to a mailing list when the email address has # not yet been associated with a user. - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') self.assertEqual(member.role, MemberRole.member) @@ -60,9 +62,11 @@ class TestAddMember(unittest.TestCase): # already been associated with a user. user_manager = getUtility(IUserManager) user_manager.create_user('aperson@example.com', 'Anne Person') - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') @@ -71,9 +75,11 @@ class TestAddMember(unittest.TestCase): # subscribe to the mailing list. IBanManager(self._mlist).ban('anne@example.com') with self.assertRaises(MembershipIsBannedError) as cm: - add_member(self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, - system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual( str(cm.exception), 'anne@example.com is not allowed to subscribe to test@example.com') @@ -84,17 +90,21 @@ class TestAddMember(unittest.TestCase): IBanManager(None).ban('anne@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_banned_from_different_list(self): # Test that members who are banned by on a different list can still be # subscribed to other mlists. sample_list = create_list('sample@example.com') IBanManager(sample_list).ban('anne@example.com') - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'anne@example.com') def test_add_member_banned_by_pattern(self): @@ -102,33 +112,41 @@ class TestAddMember(unittest.TestCase): IBanManager(self._mlist).ban('^.*@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_globally_banned_by_pattern(self): # Addresses matching global regexp ban patterns cannot subscribe. IBanManager(None).ban('^.*@example.com') self.assertRaises( MembershipIsBannedError, - add_member, self._mlist, 'anne@example.com', 'Anne Person', - '123', DeliveryMode.regular, system_preferences.preferred_language) + add_member, self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) def test_add_member_banned_from_different_list_by_pattern(self): # Addresses matching regexp ban patterns on one list can still # subscribe to other mailing lists. sample_list = create_list('sample@example.com') IBanManager(sample_list).ban('^.*@example.com') - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language) + member = add_member( + self._mlist, + RequestRecord('anne@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(member.address.email, 'anne@example.com') def test_add_member_moderator(self): # Test adding a moderator to a mailing list. - member = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.moderator) + member = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.moderator) self.assertEqual(member.address.email, 'aperson@example.com') self.assertEqual(member.list_id, 'test.example.com') self.assertEqual(member.role, MemberRole.moderator) @@ -136,29 +154,37 @@ class TestAddMember(unittest.TestCase): def test_add_member_twice(self): # Adding a member with the same role twice causes an # AlreadySubscribedError to be raised. - add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) + add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) + add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) self.assertEqual(cm.exception.fqdn_listname, 'test@example.com') self.assertEqual(cm.exception.email, 'aperson@example.com') self.assertEqual(cm.exception.role, MemberRole.member) def test_add_member_with_different_roles(self): # Adding a member twice with different roles is okay. - member_1 = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.member) - member_2 = add_member(self._mlist, 'aperson@example.com', - 'Anne Person', '123', DeliveryMode.regular, - system_preferences.preferred_language, - MemberRole.owner) + member_1 = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.member) + member_2 = add_member( + self._mlist, + RequestRecord('aperson@example.com', 'Anne Person', + DeliveryMode.regular, + system_preferences.preferred_language), + MemberRole.owner) self.assertEqual(member_1.list_id, member_2.list_id) self.assertEqual(member_1.address, member_2.address) self.assertEqual(member_1.user, member_2.user) @@ -171,43 +197,38 @@ class TestAddMember(unittest.TestCase): # test subscribes the lower case address and ensures the original # mixed case address can't be subscribed. email = 'APerson@example.com' - add_member(self._mlist, email.lower(), 'Ann Person', '123', - DeliveryMode.regular, system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email.lower(), 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, email, 'Ann Person', '123', - DeliveryMode.regular, - system_preferences.preferred_language) - self.assertEqual(cm.exception.email, email) - + add_member( + self._mlist, + RequestRecord(email, 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) + self.assertEqual(cm.exception.email, email.lower()) + def test_add_member_with_lower_case_email(self): # LP: #1425359 - Mailman is case-perserving, case-insensitive. This # test subscribes the mixed case address and ensures the lower cased # address can't be added. email = 'APerson@example.com' - add_member(self._mlist, email, 'Ann Person', '123', - DeliveryMode.regular, system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email, 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) with self.assertRaises(AlreadySubscribedError) as cm: - add_member(self._mlist, email.lower(), 'Ann Person', '123', - DeliveryMode.regular, - system_preferences.preferred_language) + add_member( + self._mlist, + RequestRecord(email.lower(), 'Ann Person', + DeliveryMode.regular, + system_preferences.preferred_language)) self.assertEqual(cm.exception.email, email.lower()) - -class TestAddMemberPassword(unittest.TestCase): - layer = ConfigLayer - - def setUp(self): - self._mlist = create_list('test@example.com') - - def test_add_member_password(self): - # Test that the password stored with the new user is encrypted. - member = add_member(self._mlist, 'anne@example.com', - 'Anne Person', 'abc', DeliveryMode.regular, - system_preferences.preferred_language) - self.assertEqual(member.user.password, '{plaintext}abc') - - class TestDeleteMember(unittest.TestCase): layer = ConfigLayer diff --git a/src/mailman/app/tests/test_moderation.py b/src/mailman/app/tests/test_moderation.py index 72a25253e..a371d8520 100644 --- a/src/mailman/app/tests/test_moderation.py +++ b/src/mailman/app/tests/test_moderation.py @@ -19,16 +19,21 @@ __all__ = [ 'TestModeration', + 'TestUnsubscription', ] import unittest from mailman.app.lifecycle import create_list -from mailman.app.moderator import handle_message, hold_message +from mailman.app.moderator import ( + handle_message, handle_subscription, handle_unsubscription, hold_message, + hold_subscription, hold_unsubscription) from mailman.interfaces.action import Action +from mailman.interfaces.member import DeliveryMode from mailman.interfaces.messages import IMessageStore from mailman.interfaces.requests import IListRequests +from mailman.interfaces.subscriptions import RequestRecord from mailman.runners.incoming import IncomingRunner from mailman.runners.outgoing import OutgoingRunner from mailman.runners.pipeline import PipelineRunner @@ -148,3 +153,26 @@ Message-ID: 'Forward of moderated message') self.assertEqual(messages[0].msgdata['recipients'], ['zack@example.com']) + + + +class TestUnsubscription(unittest.TestCase): + """Test unsubscription requests.""" + + layer = SMTPLayer + + def setUp(self): + self._mlist = create_list('test@example.com') + self._request_db = IListRequests(self._mlist) + + def test_unsubscribe_defer(self): + # When unsubscriptions must be approved by the moderator, but the + # moderator defers this decision. + token = hold_subscription( + self._mlist, + RequestRecord('anne@example.org', 'Anne Person', + DeliveryMode.regular, 'en')) + handle_subscription(self._mlist, token, Action.accept) + # Now hold and handle an unsubscription request. + token = hold_unsubscription(self._mlist, 'anne@example.org') + handle_unsubscription(self._mlist, token, Action.defer) diff --git a/src/mailman/app/tests/test_notifications.py b/src/mailman/app/tests/test_notifications.py index e46a50ebd..19c11d0a4 100644 --- a/src/mailman/app/tests/test_notifications.py +++ b/src/mailman/app/tests/test_notifications.py @@ -28,11 +28,11 @@ import tempfile import unittest from mailman.app.lifecycle import create_list -from mailman.app.membership import add_member from mailman.config import config from mailman.interfaces.languages import ILanguageManager -from mailman.interfaces.member import DeliveryMode, MemberRole -from mailman.testing.helpers import get_queue_messages +from mailman.interfaces.member import MemberRole +from mailman.interfaces.usermanager import IUserManager +from mailman.testing.helpers import get_queue_messages, subscribe, subscribe_ex from mailman.testing.layers import ConfigLayer from zope.component import getUtility @@ -42,6 +42,7 @@ class TestNotifications(unittest.TestCase): """Test notifications.""" layer = ConfigLayer + maxDiff = None def setUp(self): self._mlist = create_list('test@example.com') @@ -78,8 +79,7 @@ Welcome to the $list_name mailing list. shutil.rmtree(self.var_dir) def test_welcome_message(self): - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'en') + subscribe(self._mlist, 'Anne', email='anne@example.com') # Now there's one message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 1) @@ -104,8 +104,12 @@ Welcome to the Test List mailing list. # Add the xx language and subscribe Anne using it. manager = getUtility(ILanguageManager) manager.add('xx', 'us-ascii', 'Xlandia') - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx') + # We can't use the subscribe_ex() helper because that would send the + # welcome message before we set the member's preferred language. + address = getUtility(IUserManager).create_address( + 'anne@example.com', 'Anne Person') + address.preferences.preferred_language = 'xx' + self._mlist.subscribe(address) # Now there's one message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 1) @@ -118,27 +122,29 @@ Welcome to the Test List mailing list. def test_no_welcome_message_to_owners(self): # Welcome messages go only to mailing list members, not to owners. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.owner) + member = subscribe_ex( + self._mlist, 'Anne', MemberRole.owner, email='anne@example.com') + member.preferences.preferred_language = 'xx' # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) def test_no_welcome_message_to_nonmembers(self): # Welcome messages go only to mailing list members, not to nonmembers. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.nonmember) + member = subscribe_ex( + self._mlist, 'Anne', MemberRole.nonmember, + email='anne@example.com') + member.preferences.preferred_language = 'xx' # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) def test_no_welcome_message_to_moderators(self): # Welcome messages go only to mailing list members, not to moderators. - add_member(self._mlist, 'anne@example.com', 'Anne Person', - 'password', DeliveryMode.regular, 'xx', - MemberRole.moderator) + member = subscribe_ex( + self._mlist, 'Anne', MemberRole.moderator, + email='anne@example.com') + member.preferences.preferred_language = 'xx' # There is no welcome message in the virgin queue. messages = get_queue_messages('virgin') self.assertEqual(len(messages), 0) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 8ba5f52ff..72892b886 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -18,7 +18,8 @@ """Tests for the subscription service.""" __all__ = [ - 'TestJoin' + 'TestJoin', + 'TestSubscriptionWorkflow', ] @@ -26,11 +27,14 @@ import uuid import unittest from mailman.app.lifecycle import create_list +from mailman.app.subscriptions import SubscriptionWorkflow from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.member import MemberRole, MissingPreferredAddressError from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) from mailman.testing.layers import ConfigLayer +from mailman.interfaces.mailinglist import SubscriptionPolicy +from mailman.interfaces.usermanager import IUserManager from zope.component import getUtility @@ -65,3 +69,32 @@ class TestJoin(unittest.TestCase): self._service.join, 'test.example.com', anne.user.user_id, role=MemberRole.owner) + + + +class TestSubscriptionWorkflow(unittest.TestCase): + layer = ConfigLayer + + def setUp(self): + self._mlist = create_list('test@example.com') + self._anne = 'anne@example.com' + self._user_manager = getUtility(IUserManager) + + def test_preverified_address_joins_open_list(self): + # The mailing list has an open subscription policy, so the subscriber + # becomes a member with no human intervention. + self._mlist.subscription_policy = SubscriptionPolicy.open + anne = self._user_manager.create_address(self._anne, 'Anne Person') + self.assertIsNone(anne.verified_on) + self.assertIsNone(anne.user) + self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=False, pre_approved=False) + # Run the state machine to the end. The result is that her address + # will be verified, linked to a user, and subscribed to the mailing + # list. + list(workflow) + self.assertIsNotNone(anne.verified_on) + self.assertIsNotNone(anne.user) + self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) diff --git a/src/mailman/chains/docs/moderation.rst b/src/mailman/chains/docs/moderation.rst index 1fe7e40cb..fefc4f543 100644 --- a/src/mailman/chains/docs/moderation.rst +++ b/src/mailman/chains/docs/moderation.rst @@ -32,12 +32,11 @@ deferred. The default setting for the moderation action of new members is determined by the mailing list's settings. By default, a mailing list is not set to moderate new member postings. - >>> from mailman.app.membership import add_member - >>> from mailman.interfaces.member import DeliveryMode - >>> member = add_member(mlist, 'anne@example.com', 'Anne', 'aaa', - ... DeliveryMode.regular, 'en') + >>> from mailman.testing.helpers import subscribe_ex + >>> member = subscribe_ex(mlist, 'Anne', email='anne@example.com') >>> member - on test@example.com as MemberRole.member> + + on test@example.com as MemberRole.member> >>> print(member.moderation_action) Action.defer diff --git a/src/mailman/commands/cli_members.py b/src/mailman/commands/cli_members.py index e4cad5966..ccacbeeb8 100644 --- a/src/mailman/commands/cli_members.py +++ b/src/mailman/commands/cli_members.py @@ -27,15 +27,14 @@ import codecs from email.utils import formataddr, parseaddr from mailman.app.membership import add_member -from mailman.config import config from mailman.core.i18n import _ from mailman.database.transaction import transactional from mailman.interfaces.command import ICLISubCommand from mailman.interfaces.listmanager import IListManager from mailman.interfaces.member import ( AlreadySubscribedError, DeliveryMode, DeliveryStatus) +from mailman.interfaces.subscriptions import RequestRecord from operator import attrgetter -from passlib.utils import generate_password as generate from zope.component import getUtility from zope.interface import implementer @@ -193,12 +192,11 @@ class Members: continue # Parse the line and ensure that the values are unicodes. display_name, email = parseaddr(line) - # Give the user a default, user-friendly password. - password = generate(int(config.passwords.password_length)) try: - add_member(mlist, email, display_name, password, - DeliveryMode.regular, - mlist.preferred_language.code) + add_member(mlist, + RequestRecord(email, display_name, + DeliveryMode.regular, + mlist.preferred_language.code)) except AlreadySubscribedError: # It's okay if the address is already subscribed, just # print a warning and continue. diff --git a/src/mailman/commands/docs/members.rst b/src/mailman/commands/docs/members.rst index c90418181..0f4a4afa1 100644 --- a/src/mailman/commands/docs/members.rst +++ b/src/mailman/commands/docs/members.rst @@ -34,17 +34,9 @@ options. To start with, there are no members of the mailing list. Once the mailing list add some members, they will be displayed. :: - >>> from mailman.interfaces.member import DeliveryMode - >>> from mailman.app.membership import add_member - >>> add_member(mlist1, 'anne@example.com', 'Anne Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) - - on test1@example.com as MemberRole.member> - >>> add_member(mlist1, 'bart@example.com', 'Bart Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) - - on test1@example.com as MemberRole.member> - + >>> from mailman.testing.helpers import subscribe + >>> subscribe(mlist1, 'Anne', email='anne@example.com') + >>> subscribe(mlist1, 'Bart', email='bart@example.com') >>> command.process(args) Anne Person Bart Person @@ -52,11 +44,7 @@ Once the mailing list add some members, they will be displayed. Members are displayed in alphabetical order based on their address. :: - >>> add_member(mlist1, 'anne@aaaxample.com', 'Anne Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) - - on test1@example.com as MemberRole.member> - + >>> subscribe(mlist1, 'Anne', email='anne@aaaxample.com') >>> command.process(args) Anne Person Anne Person @@ -92,6 +80,7 @@ Filtering on delivery mode You can limit output to just the regular non-digest members... + >>> from mailman.interfaces.member import DeliveryMode >>> args.regular = True >>> member = mlist1.members.get_member('anne@example.com') >>> member.preferences.delivery_mode = DeliveryMode.plaintext_digests @@ -136,21 +125,16 @@ status is enabled... :: >>> from mailman.interfaces.member import DeliveryStatus + >>> from mailman.testing.helpers import subscribe_ex >>> member = mlist1.members.get_member('anne@aaaxample.com') >>> member.preferences.delivery_status = DeliveryStatus.by_moderator >>> member = mlist1.members.get_member('bart@example.com') >>> member.preferences.delivery_status = DeliveryStatus.by_user - >>> member = add_member( - ... mlist1, 'cris@example.com', 'Cris Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> member = subscribe_ex(mlist1, 'Cris', email='cris@example.com') >>> member.preferences.delivery_status = DeliveryStatus.unknown - >>> member = add_member( - ... mlist1, 'dave@example.com', 'Dave Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> member = subscribe_ex(mlist1, 'Dave', email='dave@example.com') >>> member.preferences.delivery_status = DeliveryStatus.enabled - >>> member = add_member( - ... mlist1, 'elly@example.com', 'Elly Person', 'xxx', - ... DeliveryMode.regular, mlist1.preferred_language.code) + >>> member = subscribe_ex(mlist1, 'Elly', email='elly@example.com') >>> member.preferences.delivery_status = DeliveryStatus.by_bounces >>> args.nomail = 'enabled' diff --git a/src/mailman/interfaces/mailinglist.py b/src/mailman/interfaces/mailinglist.py index 23d2fadf4..f112b2a11 100644 --- a/src/mailman/interfaces/mailinglist.py +++ b/src/mailman/interfaces/mailinglist.py @@ -25,6 +25,7 @@ __all__ = [ 'IMailingList', 'Personalization', 'ReplyToMunging', + 'SubscriptionPolicy', ] @@ -53,6 +54,18 @@ class ReplyToMunging(Enum): explicit_header = 2 +class SubscriptionPolicy(Enum): + # Neither confirmation, nor moderator approval is required. + open = 0 + # The user must confirm the subscription. + confirm = 1 + # The moderator must approve the subscription. + moderate = 2 + # The user must first confirm their subscription, and then if that is + # successful, the moderator must also approve it. + confirm_then_moderate = 3 + + class IMailingList(Interface): """A mailing list.""" @@ -234,6 +247,9 @@ class IMailingList(Interface): deliver disabled or not, or of the type of digest they are to receive.""") + subscription_policy = Attribute( + """The policy for subscribing new members to the list.""") + subscribers = Attribute( """An iterator over all IMembers subscribed to this list, with any role. diff --git a/src/mailman/interfaces/subscriptions.py b/src/mailman/interfaces/subscriptions.py index c72a902cb..677f591ef 100644 --- a/src/mailman/interfaces/subscriptions.py +++ b/src/mailman/interfaces/subscriptions.py @@ -19,9 +19,12 @@ __all__ = [ 'ISubscriptionService', + 'RequestRecord', ] +from collections import namedtuple + from mailman.interfaces.errors import MailmanError from mailman.interfaces.member import DeliveryMode, MemberRole from zope.interface import Interface @@ -39,6 +42,19 @@ class MissingUserError(MailmanError): return self.user_id + +_RequestRecord = namedtuple( + 'RequestRecord', + 'email display_name delivery_mode, language') +def RequestRecord(email, display_name='', + delivery_mode=DeliveryMode.regular, + language=None): + if language is None: + from mailman.core.constants import system_preferences + language = system_preferences.preferred_language + return _RequestRecord(email, display_name, delivery_mode, language) + + class ISubscriptionService(Interface): """General Subscription services.""" diff --git a/src/mailman/interfaces/usermanager.py b/src/mailman/interfaces/usermanager.py index 798d1d127..5f3a324cc 100644 --- a/src/mailman/interfaces/usermanager.py +++ b/src/mailman/interfaces/usermanager.py @@ -43,6 +43,22 @@ class IUserManager(Interface): registered. """ + def make_user(email, display_name=None): + """Create a new user linked to an address object. + + If ``email`` is already associated with an existing `IAddress` + object, use that, otherwise create a new `IAddress`. If the + address object already points to an `IUser` return it. If a new + `IUser` is created, link the address to the user. + + :param email: The email address. + :type email: str + :param display_name: The display name. + :type display_name: str + :return: the IUser object that exists or was created. + :rtype: IUser + """ + def delete_user(user): """Delete the given user. diff --git a/src/mailman/model/docs/usermanager.rst b/src/mailman/model/docs/usermanager.rst index ba328b54b..8e40b621e 100644 --- a/src/mailman/model/docs/usermanager.rst +++ b/src/mailman/model/docs/usermanager.rst @@ -179,3 +179,25 @@ There are now four members in the system. Sort them by address then role. test.example.com bperson@example.com MemberRole.owner test.example.com eperson@example.com MemberRole.member test.example.com fperson@example.com MemberRole.member + + +Creating a new user +=================== + +A common situation (especially during the subscription life cycle) is to +create a user linked to an address, with a preferred address. Say for +example, we are asked to subscribe a new address we have never seen before. + + >>> cris = user_manager.make_user('cris@example.com', 'Cris Person') + +Since we've never seen ``cris@example.com`` before, this call creates a new +user with the given email and display name. + + >>> cris + + +The user has a single unverified address object. + + >>> for address in cris.addresses: + ... print(repr(address)) + [not verified] at ...> diff --git a/src/mailman/model/mailinglist.py b/src/mailman/model/mailinglist.py index a204d54cd..a5ecfddbe 100644 --- a/src/mailman/model/mailinglist.py +++ b/src/mailman/model/mailinglist.py @@ -38,7 +38,7 @@ from mailman.interfaces.domain import IDomainManager from mailman.interfaces.languages import ILanguageManager from mailman.interfaces.mailinglist import ( IAcceptableAlias, IAcceptableAliasSet, IListArchiver, IListArchiverSet, - IMailingList, Personalization, ReplyToMunging) + IMailingList, Personalization, ReplyToMunging, SubscriptionPolicy) from mailman.interfaces.member import ( AlreadySubscribedError, MemberRole, MissingPreferredAddressError, SubscriptionEvent) @@ -197,6 +197,8 @@ class MailingList(Model): self._list_id = '{0}.{1}'.format(listname, hostname) # For the pending database self.next_request_id = 1 + # XXX Make this a database column: Enum(SubscriptionPolicy) + self.subscription_policy = None # We need to set up the rosters. Normally, this method will get called # when the MailingList object is loaded from the database, but when the # constructor is called, SQLAlchemy's `load` event isn't triggered. @@ -455,6 +457,8 @@ class MailingList(Model): return self.owners elif role is MemberRole.moderator: return self.moderators + elif role is MemberRole.nonmember: + return self.nonmembers else: raise TypeError('Undefined MemberRole: {}'.format(role)) diff --git a/src/mailman/model/roster.py b/src/mailman/model/roster.py index 91211c665..ef24d896b 100644 --- a/src/mailman/model/roster.py +++ b/src/mailman/model/roster.py @@ -99,9 +99,7 @@ class AbstractRoster: @dbconnection def get_member(self, store, address): """See `IRoster`.""" - results = store.query(Member).filter( - Member.list_id == self._mlist.list_id, - Member.role == self.role, + results = self._query().filter( Address.email == address, Member.address_id == Address.id) if results.count() == 0: diff --git a/src/mailman/model/tests/test_usermanager.py b/src/mailman/model/tests/test_usermanager.py index 90fcdac0c..31f1a7275 100644 --- a/src/mailman/model/tests/test_usermanager.py +++ b/src/mailman/model/tests/test_usermanager.py @@ -33,18 +33,50 @@ from zope.component import getUtility class TestUserManager(unittest.TestCase): layer = ConfigLayer + def setUp(self): + self._usermanager = getUtility(IUserManager) + def test_create_user_with_existing_address(self): # LP: #1418280. If a user is created when an email address is passed # in, and that address already exists, the user object should not get # created. - manager = getUtility(IUserManager) # Create the address we're going to try to duplicate. - manager.create_address('anne@example.com') + self._usermanager.create_address('anne@example.com') # There are no users. - self.assertEqual(len(list(manager.users)), 0) + self.assertEqual(len(list(self._usermanager.users)), 0) # Now create the user with an already existing address. with self.assertRaises(ExistingAddressError) as cm: - manager.create_user('anne@example.com') + self._usermanager.create_user('anne@example.com') self.assertEqual(cm.exception.address, 'anne@example.com') # There are still no users. - self.assertEqual(len(list(manager.users)), 0) + self.assertEqual(len(list(self._usermanager.users)), 0) + + def test_make_new_user(self): + # Neither the user nor address objects exist yet. + self.assertIsNone(self._usermanager.get_user('anne@example.com')) + self.assertIsNone(self._usermanager.get_address('anne@example.com')) + user = self._usermanager.make_user('anne@example.com', 'Anne Person') + self.assertIn('anne@example.com', + [address.email for address in user.addresses]) + addresses = list(user.addresses) + self.assertEqual(len(addresses), 1) + address = addresses[0] + self.assertEqual(address.email, 'anne@example.com') + self.assertEqual(address.display_name, 'Anne Person') + self.assertEqual(address.user.display_name, 'Anne Person') + self.assertIs(address.user, user) + + def test_make_linked_user(self): + # The address exists, but there is no linked user. + self.assertIsNone(self._usermanager.get_user('anne@example.com')) + address = self._usermanager.create_address('anne@example.com') + user = self._usermanager.make_user('anne@example.com', 'Anne Person') + self.assertIsNotNone(address.user) + self.assertIs(user, address.user) + self.assertIn(address, user.addresses) + self.assertEqual(user.display_name, 'Anne Person') + + def test_make_user_exists(self): + user = self._usermanager.create_user('anne@example.com', 'Anne Person') + other_user = self._usermanager.make_user('anne@example.com') + self.assertIs(user, other_user) diff --git a/src/mailman/model/usermanager.py b/src/mailman/model/usermanager.py index 11557bc25..9b82f10c2 100644 --- a/src/mailman/model/usermanager.py +++ b/src/mailman/model/usermanager.py @@ -46,6 +46,27 @@ class UserManager: user.link(address) return user + def make_user(self, email, display_name=None): + """See `IUserManager`.""" + # See if there's already a user linked with the given address. + user = self.get_user(email) + if user is None: + # A user linked to this address does not yet exist. Is the + # address itself known but just not linked to a user? + address = self.get_address(email) + if address is None: + # Nope, we don't even know about this address, so create both + # the user and address now. + return self.create_user(email, display_name) + # The address exists, but it's not yet linked to a user. Create + # the empty user object and link them together. + user = self.create_user() + user.display_name = ( + display_name if display_name else address.display_name) + user.link(address) + return user + return user + @dbconnection def delete_user(self, store, user): """See `IUserManager`.""" diff --git a/src/mailman/mta/tests/test_delivery.py b/src/mailman/mta/tests/test_delivery.py index 77d31d3a3..0ac8489a2 100644 --- a/src/mailman/mta/tests/test_delivery.py +++ b/src/mailman/mta/tests/test_delivery.py @@ -28,13 +28,11 @@ import tempfile import unittest from mailman.app.lifecycle import create_list -from mailman.app.membership import add_member from mailman.config import config from mailman.interfaces.mailinglist import Personalization -from mailman.interfaces.member import DeliveryMode from mailman.mta.deliver import Deliver from mailman.testing.helpers import ( - specialized_message_from_string as mfs) + specialized_message_from_string as mfs, subscribe_ex) from mailman.testing.layers import ConfigLayer @@ -63,9 +61,8 @@ class TestIndividualDelivery(unittest.TestCase): self._mlist = create_list('test@example.com') self._mlist.personalize = Personalization.individual # Make Anne a member of this mailing list. - self._anne = add_member(self._mlist, - 'anne@example.org', 'Anne Person', - 'xyz', DeliveryMode.regular, 'en') + self._anne = subscribe_ex( + self._mlist, 'Anne', email='anne@example.org') # Clear out any results from the previous test. del _deliveries[:] self._msg = mfs("""\ diff --git a/src/mailman/rest/docs/moderation.rst b/src/mailman/rest/docs/moderation.rst index 6aec921f0..bdb8d9f1c 100644 --- a/src/mailman/rest/docs/moderation.rst +++ b/src/mailman/rest/docs/moderation.rst @@ -219,21 +219,21 @@ moderator approval. >>> from mailman.app.moderator import hold_subscription >>> from mailman.interfaces.member import DeliveryMode - >>> sub_req_id = hold_subscription( - ... ant, 'anne@example.com', 'Anne Person', - ... 'password', DeliveryMode.regular, 'en') + >>> from mailman.interfaces.subscriptions import RequestRecord + >>> sub_req_id = hold_subscription(ant, + ... RequestRecord('anne@example.com', 'Anne Person', + ... DeliveryMode.regular, 'en')) >>> transaction.commit() The subscription request is available from the mailing list. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') entry 0: - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 @@ -247,10 +247,9 @@ Viewing unsubscription requests Bart tries to leave a mailing list, but he may not be allowed to. - >>> from mailman.app.membership import add_member + >>> from mailman.testing.helpers import subscribe >>> from mailman.app.moderator import hold_unsubscription - >>> bart = add_member(ant, 'bart@example.com', 'Bart Person', - ... 'password', DeliveryMode.regular, 'en') + >>> bart = subscribe(ant, 'Bart', email='bart@example.com') >>> unsub_req_id = hold_unsubscription(ant, 'bart@example.com') >>> transaction.commit() @@ -258,17 +257,16 @@ The unsubscription request is also available from the mailing list. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/requests') entry 0: - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 entry 1: - address: bart@example.com + email: bart@example.com http_etag: "..." request_id: ... type: unsubscription @@ -285,12 +283,11 @@ request id. Anne's subscription request looks like this. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/' ... 'requests/{}'.format(sub_req_id)) - address: anne@example.com delivery_mode: regular display_name: Anne Person + email: anne@example.com http_etag: "..." language: en - password: password request_id: ... type: subscription when: 2005-08-01T07:49:23 @@ -299,7 +296,7 @@ Bart's unsubscription request looks like this. >>> dump_json('http://localhost:9001/3.0/lists/ant@example.com/' ... 'requests/{}'.format(unsub_req_id)) - address: bart@example.com + email: bart@example.com http_etag: "..." request_id: ... type: unsubscription diff --git a/src/mailman/rest/root.py b/src/mailman/rest/root.py index 0861a9a5b..9ec84da68 100644 --- a/src/mailman/rest/root.py +++ b/src/mailman/rest/root.py @@ -182,6 +182,7 @@ class TopLevel: @child() def lists(self, request, segments): """//lists + //lists/styles //lists/ //lists//... """ diff --git a/src/mailman/rest/tests/test_moderation.py b/src/mailman/rest/tests/test_moderation.py index 262a7ec60..c77ae2aca 100644 --- a/src/mailman/rest/tests/test_moderation.py +++ b/src/mailman/rest/tests/test_moderation.py @@ -29,6 +29,7 @@ from mailman.app.moderator import hold_message, hold_subscription from mailman.config import config from mailman.database.transaction import transaction from mailman.interfaces.member import DeliveryMode +from mailman.interfaces.subscriptions import RequestRecord from mailman.testing.helpers import ( call_api, specialized_message_from_string as mfs) from mailman.testing.layers import RESTLayer @@ -76,8 +77,9 @@ Something else. # in the database. held_id = hold_message(self._mlist, self._msg) subscribe_id = hold_subscription( - self._mlist, 'bperson@example.net', 'Bart Person', 'xyz', - DeliveryMode.regular, 'en') + self._mlist, + RequestRecord('bperson@example.net', 'Bart Person', + DeliveryMode.regular, 'en')) config.db.store.commit() url = 'http://localhost:9001/3.0/lists/ant@example.com/held/{0}' with self.assertRaises(HTTPError) as cm: @@ -114,8 +116,9 @@ Something else. def test_bad_subscription_action(self): # POSTing to a held message with a bad action. held_id = hold_subscription( - self._mlist, 'cperson@example.net', 'Cris Person', 'xyz', - DeliveryMode.regular, 'en') + self._mlist, + RequestRecord('cperson@example.net', 'Cris Person', + DeliveryMode.regular, 'en')) config.db.store.commit() url = 'http://localhost:9001/3.0/lists/ant@example.com/requests/{0}' with self.assertRaises(HTTPError) as cm: diff --git a/src/mailman/runners/docs/outgoing.rst b/src/mailman/runners/docs/outgoing.rst index 7c3d1a989..598622775 100644 --- a/src/mailman/runners/docs/outgoing.rst +++ b/src/mailman/runners/docs/outgoing.rst @@ -11,24 +11,13 @@ a *delivery module*, essentially a pluggable interface for determining how the recipient set will be batched, whether messages will be personalized and VERP'd, etc. The outgoing runner doesn't itself support retrying but it can move messages to the 'retry queue' for handling delivery failures. -:: + >>> from mailman.testing.helpers import subscribe >>> mlist = create_list('test@example.com') - >>> from mailman.app.membership import add_member - >>> from mailman.interfaces.member import DeliveryMode - >>> add_member(mlist, 'aperson@example.com', 'Anne Person', - ... 'password', DeliveryMode.regular, 'en') - - on test@example.com as MemberRole.member> - >>> add_member(mlist, 'bperson@example.com', 'Bart Person', - ... 'password', DeliveryMode.regular, 'en') - - on test@example.com as MemberRole.member> - >>> add_member(mlist, 'cperson@example.com', 'Cris Person', - ... 'password', DeliveryMode.regular, 'en') - - on test@example.com as MemberRole.member> + >>> subscribe(mlist, 'Anne', email='aperson@example.com') + >>> subscribe(mlist, 'Bart', email='bperson@example.com') + >>> subscribe(mlist, 'Cris', email='cperson@example.com') Normally, messages would show up in the outgoing queue after the message has been processed by the rule set and pipeline. But we can simulate that here by diff --git a/src/mailman/styles/base.py b/src/mailman/styles/base.py index 50cddbc32..7a77af609 100644 --- a/src/mailman/styles/base.py +++ b/src/mailman/styles/base.py @@ -41,7 +41,8 @@ from mailman.interfaces.archiver import ArchivePolicy from mailman.interfaces.autorespond import ResponseAction from mailman.interfaces.bounce import UnrecognizedBounceDisposition from mailman.interfaces.digests import DigestFrequency -from mailman.interfaces.mailinglist import Personalization, ReplyToMunging +from mailman.interfaces.mailinglist import ( + Personalization, ReplyToMunging, SubscriptionPolicy) from mailman.interfaces.nntp import NewsgroupModeration @@ -75,6 +76,7 @@ class BasicOperation: mlist.personalize = Personalization.none mlist.default_member_action = Action.defer mlist.default_nonmember_action = Action.hold + mlist.subscription_policy = SubscriptionPolicy.confirm # Notify the administrator of pending requests and membership changes. mlist.admin_immed_notify = True mlist.admin_notify_mchanges = False diff --git a/src/mailman/templates/en/unsubauth.txt b/src/mailman/templates/en/unsubauth.txt index 5975a2ce8..e6a6a82f2 100644 --- a/src/mailman/templates/en/unsubauth.txt +++ b/src/mailman/templates/en/unsubauth.txt @@ -1,7 +1,7 @@ Your authorization is required for a mailing list unsubscription request approval: - By: $address + By: $email From: $listname At your convenience, visit: diff --git a/src/mailman/testing/helpers.py b/src/mailman/testing/helpers.py index a869c8d55..476211a08 100644 --- a/src/mailman/testing/helpers.py +++ b/src/mailman/testing/helpers.py @@ -33,6 +33,7 @@ __all__ = [ 'reset_the_world', 'specialized_message_from_string', 'subscribe', + 'subscribe_ex', 'temporary_db', 'wait_for_webservice', ] @@ -435,10 +436,11 @@ class chdir: -def subscribe(mlist, first_name, role=MemberRole.member): +def subscribe(mlist, first_name, role=MemberRole.member, email=None): """Helper for subscribing a sample person to a mailing list.""" user_manager = getUtility(IUserManager) - email = '{0}person@example.com'.format(first_name[0].lower()) + email = ('{0}person@example.com'.format(first_name[0].lower()) + if email is None else email) full_name = '{0} Person'.format(first_name) with transaction(): person = user_manager.get_user(email) @@ -446,13 +448,30 @@ def subscribe(mlist, first_name, role=MemberRole.member): address = user_manager.get_address(email) if address is None: person = user_manager.create_user(email, full_name) - preferred_address = list(person.addresses)[0] - mlist.subscribe(preferred_address, role) + subscription_address = list(person.addresses)[0] else: - mlist.subscribe(address, role) + subscription_address = address else: - preferred_address = list(person.addresses)[0] - mlist.subscribe(preferred_address, role) + subscription_address = list(person.addresses)[0] + # We can't return the newly created member because that will + # implicitly open a new transaction, which can break doctests. If you + # really need the newly created member, look it up. + mlist.subscribe(subscription_address, role) + + +def subscribe_ex(mlist, first_name, role=MemberRole.member, email=None): + """Like ``subscribe()`` but returns the newly created member object. + + Only use this in contexts where you can accept the opening of an implicit + transaction (i.e. *not* in REST tests) unless you explicitly close said + transaction. Otherwise you will lock the database. + """ + # Blarg. I wish we didn't have to duplicate this logic. + email = ('{0}person@example.com'.format(first_name[0].lower()) + if email is None else email) + subscribe(mlist, first_name, role, email) + roster = mlist.get_roster(role) + return roster.get_member(email) -- cgit v1.2.3-70-g09d2 From c906e12bfaefa2a81ad8e024d4e0b3fd7353901c Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 20 Mar 2015 16:51:14 +0100 Subject: Remove commented-out code --- src/mailman/app/membership.py | 53 ------------------------------------------- 1 file changed, 53 deletions(-) diff --git a/src/mailman/app/membership.py b/src/mailman/app/membership.py index b64291d7c..3d06c7503 100644 --- a/src/mailman/app/membership.py +++ b/src/mailman/app/membership.py @@ -27,7 +27,6 @@ __all__ = [ from email.utils import formataddr from mailman.app.notifications import ( send_goodbye_message, send_welcome_message) -from mailman.config import config from mailman.core.i18n import _ from mailman.email.message import OwnerNotification from mailman.interfaces.bans import IBanManager @@ -76,58 +75,6 @@ def add_member(mlist, record, role=MemberRole.member): member = mlist.subscribe(address, role) member.preferences.preferred_language = record.language member.preferences.delivery_mode = record.delivery_mode -# user = user_manager.get_user(email) -# if user is None: -# # A user linked to this address does not yet exist. Is the address -# # itself known but just not linked to a user? -# address = user_manager.get_address(email) -# if address is None: -# # Nope, we don't even know about this address, so create both the -# # user and address now. -# user = user_manager.create_user(email, display_name) -# # Do it this way so we don't have to flush the previous change. -# address = list(user.addresses)[0] -# else: -# # The address object exists, but it's not linked to a user. -# # Create the user and link it now. -# user = user_manager.create_user() -# user.display_name = ( -# display_name if display_name else address.display_name) -# user.link(address) -# # Encrypt the password using the currently selected hash scheme. -# user.password = config.password_context.encrypt(password) -# user.preferences.preferred_language = language -# member = mlist.subscribe(address, role) -# member.preferences.delivery_mode = delivery_mode -# else: -# # The user exists and is linked to the case-insensitive address. -# # We're looking for two versions of the email address, the case -# # preserved version and the case insensitive version. We'll -# # subscribe the version with matching case if it exists, otherwise -# # we'll use one of the matching case-insensitively ones. It's -# # undefined which one we pick. -# case_preserved = None -# case_insensitive = None -# for address in user.addresses: -# if address.original_email == email: -# case_preserved = address -# if address.email == email.lower(): -# case_insensitive = address -# assert case_preserved is not None or case_insensitive is not None, ( -# 'Could not find a linked address for: {}'.format(email)) -# address = (case_preserved if case_preserved is not None -# else case_insensitive) -# # Create the member and set the appropriate preferences. It's -# # possible we're subscribing the lower cased version of the address; -# # if that's already subscribed re-issue the exception with the correct -# # email address (i.e. the one passed in here). -# try: -# member = mlist.subscribe(address, role) -# except AlreadySubscribedError as error: -# raise AlreadySubscribedError( -# error.fqdn_listname, email, error.role) -# member.preferences.preferred_language = language -# member.preferences.delivery_mode = delivery_mode return member -- cgit v1.2.3-70-g09d2 From d014e75b913fde6e78a0c97e5c8cb7c16266ac2a Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 20 Mar 2015 17:14:09 +0100 Subject: Restore the features of commit 7301.3.1 which were lost in the merge --- src/mailman/app/membership.py | 33 ++++++++++++++++++++++++-------- src/mailman/app/tests/test_membership.py | 2 +- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/mailman/app/membership.py b/src/mailman/app/membership.py index 3d06c7503..b1675dae1 100644 --- a/src/mailman/app/membership.py +++ b/src/mailman/app/membership.py @@ -67,14 +67,31 @@ def add_member(mlist, record, role=MemberRole.member): # Encrypt the password using the currently selected hash scheme. user.preferences.preferred_language = record.language # Subscribe the address, not the user. - address = user_manager.get_address(record.email) - if address is None or address.user is not user: - raise AssertionError( - 'User should have had linked address: {0}'.format(address)) - # Create the member and set the appropriate preferences. - member = mlist.subscribe(address, role) - member.preferences.preferred_language = record.language - member.preferences.delivery_mode = record.delivery_mode + # We're looking for two versions of the email address, the case + # preserved version and the case insensitive version. We'll + # subscribe the version with matching case if it exists, otherwise + # we'll use one of the matching case-insensitively ones. It's + # undefined which one we pick. + case_preserved = None + case_insensitive = None + for address in user.addresses: + if address.original_email == record.email: + case_preserved = address + if address.email == record.email.lower(): + case_insensitive = address + assert case_preserved is not None or case_insensitive is not None, ( + 'Could not find a linked address for: {}'.format(record.email)) + address = (case_preserved if case_preserved is not None + else case_insensitive) + # Create the member and set the appropriate preferences. It's + # possible we're subscribing the lower cased version of the address; + # if that's already subscribed re-issue the exception with the correct + # email address (i.e. the one passed in here). + try: + member = mlist.subscribe(address, role) + except AlreadySubscribedError as error: + raise AlreadySubscribedError( + error.fqdn_listname, record.email, error.role) return member diff --git a/src/mailman/app/tests/test_membership.py b/src/mailman/app/tests/test_membership.py index 73e2ced07..481de2bb8 100644 --- a/src/mailman/app/tests/test_membership.py +++ b/src/mailman/app/tests/test_membership.py @@ -208,7 +208,7 @@ class TestAddMember(unittest.TestCase): RequestRecord(email, 'Ann Person', DeliveryMode.regular, system_preferences.preferred_language)) - self.assertEqual(cm.exception.email, email.lower()) + self.assertEqual(cm.exception.email, email) def test_add_member_with_lower_case_email(self): # LP: #1425359 - Mailman is case-perserving, case-insensitive. This -- cgit v1.2.3-70-g09d2 From cd0538af71d18873c95d9af9bf933c18d209aeb1 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 20 Mar 2015 17:38:22 +0100 Subject: Typo in the previous commit --- src/mailman/app/membership.py | 2 ++ src/mailman/app/subscriptions.py | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mailman/app/membership.py b/src/mailman/app/membership.py index b1675dae1..757a87393 100644 --- a/src/mailman/app/membership.py +++ b/src/mailman/app/membership.py @@ -92,6 +92,8 @@ def add_member(mlist, record, role=MemberRole.member): except AlreadySubscribedError as error: raise AlreadySubscribedError( error.fqdn_listname, record.email, error.role) + member.preferences.preferred_language = record.language + member.preferences.delivery_mode = record.delivery_mode return member diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index eff7c12af..481fb17b3 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -26,14 +26,12 @@ __all__ = [ from collections import deque from operator import attrgetter -#from passlib.utils import generate_password as generate from sqlalchemy import and_, or_ from uuid import UUID from zope.component import getUtility from zope.interface import implementer from mailman.app.membership import add_member, delete_member -#from mailman.config import config from mailman.app.moderator import hold_subscription from mailman.core.constants import system_preferences from mailman.database.transaction import dbconnection -- cgit v1.2.3-70-g09d2 From 179c011f2dd6c488010bd963fbe92c9fb49ca0f8 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Sat, 21 Mar 2015 12:39:46 +0100 Subject: Make the subscription policy a database column --- .../16c2b25c7b_list_subscription_policy.py | 41 ++++++++++++++++++++++ src/mailman/database/tests/test_factory.py | 11 +++--- src/mailman/model/mailinglist.py | 3 +- 3 files changed, 46 insertions(+), 9 deletions(-) create mode 100644 src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py diff --git a/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py b/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py new file mode 100644 index 000000000..d5f8c3d96 --- /dev/null +++ b/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py @@ -0,0 +1,41 @@ +"""List subscription policy + +Revision ID: 16c2b25c7b +Revises: 33e1f5f6fa8 +Create Date: 2015-03-21 11:00:44.634883 + +""" + +# revision identifiers, used by Alembic. +revision = '16c2b25c7b' +down_revision = '33e1f5f6fa8' + +from alembic import op +import sqlalchemy as sa + +from mailman.database.types import Enum +from mailman.interfaces.mailinglist import SubscriptionPolicy + + +def upgrade(): + + ### Update the schema + op.add_column('mailinglist', sa.Column( + 'subscription_policy', Enum(SubscriptionPolicy), nullable=True)) + + ### Now migrate the data + # don't import the table definition from the models, it may break this + # migration when the model is updated in the future (see the Alembic doc) + mlist = sa.sql.table('mailinglist', + sa.sql.column('subscription_policy', Enum(SubscriptionPolicy)) + ) + # there were no enforced subscription policy before, so all lists are + # considered open + op.execute(mlist.update().values( + {'subscription_policy': op.inline_literal(SubscriptionPolicy.open)})) + + +def downgrade(): + if op.get_bind().dialect.name != 'sqlite': + # SQLite does not support dropping columns. + op.drop_column('mailinglist', 'subscription_policy') diff --git a/src/mailman/database/tests/test_factory.py b/src/mailman/database/tests/test_factory.py index 6a16c74ab..acb7c32a4 100644 --- a/src/mailman/database/tests/test_factory.py +++ b/src/mailman/database/tests/test_factory.py @@ -129,17 +129,14 @@ class TestSchemaManager(unittest.TestCase): md.tables['alembic_version'].select()).scalar() self.assertEqual(current_rev, head_rev) - @patch('alembic.command.stamp') - def test_storm(self, alembic_command_stamp): + @patch('alembic.command') + def test_storm(self, alembic_command): # Existing Storm database. Model.metadata.create_all(config.db.engine) self._create_storm_database(LAST_STORM_SCHEMA_VERSION) self.schema_mgr.setup_database() - self.assertFalse(alembic_command_stamp.called) - self.assertTrue( - self._table_exists('mailinglist') - and self._table_exists('alembic_version') - and not self._table_exists('version')) + self.assertFalse(alembic_command.stamp.called) + self.assertTrue(alembic_command.upgrade.called) @patch('alembic.command') def test_old_storm(self, alembic_command): diff --git a/src/mailman/model/mailinglist.py b/src/mailman/model/mailinglist.py index a5ecfddbe..0607b90e4 100644 --- a/src/mailman/model/mailinglist.py +++ b/src/mailman/model/mailinglist.py @@ -183,6 +183,7 @@ class MailingList(Model): send_goodbye_message = Column(Boolean) send_welcome_message = Column(Boolean) subject_prefix = Column(Unicode) + subscription_policy = Column(Enum(SubscriptionPolicy)) topics = Column(PickleType) topics_bodylines_limit = Column(Integer) topics_enabled = Column(Boolean) @@ -197,8 +198,6 @@ class MailingList(Model): self._list_id = '{0}.{1}'.format(listname, hostname) # For the pending database self.next_request_id = 1 - # XXX Make this a database column: Enum(SubscriptionPolicy) - self.subscription_policy = None # We need to set up the rosters. Normally, this method will get called # when the MailingList object is loaded from the database, but when the # constructor is called, SQLAlchemy's `load` event isn't triggered. -- cgit v1.2.3-70-g09d2 From 0ab304f8fb2c853cc9e65eccd33274ed8618a5ce Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Sat, 21 Mar 2015 15:00:12 +0100 Subject: Import the subscription_policy from Mailman2 --- src/mailman/utilities/importer.py | 3 +++ src/mailman/utilities/tests/test_import.py | 27 ++++++++++++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/mailman/utilities/importer.py b/src/mailman/utilities/importer.py index 8590d9b1b..d41179f67 100644 --- a/src/mailman/utilities/importer.py +++ b/src/mailman/utilities/importer.py @@ -40,6 +40,7 @@ from mailman.interfaces.digests import DigestFrequency from mailman.interfaces.languages import ILanguageManager from mailman.interfaces.mailinglist import IAcceptableAliasSet from mailman.interfaces.mailinglist import Personalization, ReplyToMunging +from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import DeliveryMode, DeliveryStatus, MemberRole from mailman.interfaces.nntp import NewsgroupModeration from mailman.interfaces.usermanager import IUserManager @@ -177,6 +178,7 @@ TYPES = dict( personalize=Personalization, preferred_language=check_language_code, reply_goes_to_list=ReplyToMunging, + subscription_policy=SubscriptionPolicy, topics_enabled=bool, ) @@ -201,6 +203,7 @@ NAME_MAPPINGS = dict( real_name='display_name', send_goodbye_msg='send_goodbye_message', send_welcome_msg='send_welcome_message', + subscribe_policy='subscription_policy', ) # These DateTime fields of the mailinglist table need a type conversion to diff --git a/src/mailman/utilities/tests/test_import.py b/src/mailman/utilities/tests/test_import.py index b0ab9938d..e42bb0a7e 100644 --- a/src/mailman/utilities/tests/test_import.py +++ b/src/mailman/utilities/tests/test_import.py @@ -43,7 +43,8 @@ from mailman.interfaces.autorespond import ResponseAction from mailman.interfaces.bans import IBanManager from mailman.interfaces.bounce import UnrecognizedBounceDisposition from mailman.interfaces.languages import ILanguageManager -from mailman.interfaces.mailinglist import IAcceptableAliasSet +from mailman.interfaces.mailinglist import ( + IAcceptableAliasSet, SubscriptionPolicy) from mailman.interfaces.member import DeliveryMode, DeliveryStatus from mailman.interfaces.nntp import NewsgroupModeration from mailman.interfaces.templates import ITemplateLoader @@ -300,6 +301,30 @@ class TestBasicImport(unittest.TestCase): self._import() self.assertEqual(self._mlist.encode_ascii_prefixes, True) + def test_subscription_policy_open(self): + self._mlist.subscription_policy = SubscriptionPolicy.confirm + self._pckdict['subscribe_policy'] = 0 + self._import() + self.assertEqual(self._mlist.subscription_policy, SubscriptionPolicy.open) + + def test_subscription_policy_confirm(self): + self._mlist.subscription_policy = SubscriptionPolicy.open + self._pckdict['subscribe_policy'] = 1 + self._import() + self.assertEqual(self._mlist.subscription_policy, SubscriptionPolicy.confirm) + + def test_subscription_policy_moderate(self): + self._mlist.subscription_policy = SubscriptionPolicy.open + self._pckdict['subscribe_policy'] = 2 + self._import() + self.assertEqual(self._mlist.subscription_policy, SubscriptionPolicy.moderate) + + def test_subscription_policy_confirm_then_moderate(self): + self._mlist.subscription_policy = SubscriptionPolicy.open + self._pckdict['subscribe_policy'] = 3 + self._import() + self.assertEqual(self._mlist.subscription_policy, SubscriptionPolicy.confirm_then_moderate) + class TestArchiveImport(unittest.TestCase): -- cgit v1.2.3-70-g09d2 From e4527bd505ca3019de5d7bd699732b848b6f6afc Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Wed, 25 Mar 2015 17:04:52 +0100 Subject: SubscriptionWorkflow: add a test for moderated lists --- src/mailman/app/subscriptions.py | 6 +----- src/mailman/app/tests/test_subscriptions.py | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 481fb17b3..5ff4f3a3e 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -48,7 +48,6 @@ from mailman.model.member import Member from mailman.utilities.datetime import now - def _membership_sort_key(member): """Sort function for find_members(). @@ -58,7 +57,6 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) - class SubscriptionWorkflow: """Workflow of a subscription request.""" @@ -163,14 +161,13 @@ class SubscriptionWorkflow: request = RequestRecord( self.address.email, self.subscriber.display_name, DeliveryMode.regular, 'en') - hold_subscription(self._mlist, request) + hold_subscription(self.mlist, request) def _do_subscription(self): # We can immediately subscribe the user to the mailing list. self.mlist.subscribe(self.subscriber) - @implementer(ISubscriptionService) class SubscriptionService: """Subscription services for the REST API.""" @@ -287,7 +284,6 @@ class SubscriptionService: delete_member(mlist, email, False, False) - def handle_ListDeletingEvent(event): """Delete a mailing list's members when the list is being deleted.""" diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 72892b886..7bb59bb45 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -30,11 +30,13 @@ from mailman.app.lifecycle import create_list from mailman.app.subscriptions import SubscriptionWorkflow from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.member import MemberRole, MissingPreferredAddressError +from mailman.interfaces.requests import IListRequests, RequestType from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager +from mailman.utilities.datetime import now from zope.component import getUtility @@ -98,3 +100,28 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertIsNotNone(anne.verified_on) self.assertIsNotNone(anne.user) self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) + + def test_verified_address_joins_moderated_list(self): + # The mailing list is moderated but the subscriber is not a verified + # address and the subscription request is not pre-verified. + # A confirmation email must be sent, it will serve as the verification + # email too. + anne = self._user_manager.create_address(self._anne, 'Anne Person') + request_db = IListRequests(self._mlist) + def _do_check(): + anne.verified_on = now() + self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=False, pre_confirmed=True, pre_approved=False) + # Run the state machine to the end. + list(workflow) + # Look in the requests db + requests = list(request_db.of_type(RequestType.subscription)) + self.assertEqual(len(requests), 1) + self.assertEqual(requests[0].key, anne.email) + request_db.delete_request(requests[0].id) + self._mlist.subscription_policy = SubscriptionPolicy.moderate + _do_check() + self._mlist.subscription_policy = SubscriptionPolicy.confirm_then_moderate + _do_check() -- cgit v1.2.3-70-g09d2 From d2b5c8c7a11be437ac0c705fb95502d0a83e74b9 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Wed, 25 Mar 2015 17:14:01 +0100 Subject: Store method names in the SubscriptionWorkflow to allow state saving --- src/mailman/app/subscriptions.py | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 5ff4f3a3e..ca84e091f 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -76,15 +76,14 @@ class SubscriptionWorkflow: self.pre_approved = pre_approved # Prepare the state machine. self._next = deque() - self._next.append(self._verification_check) + self._next.append("verification_check") def __iter__(self): return self def _pop(self): - step = self._next.popleft() - # step could be a partial or a method. - name = getattr(step, 'func', step).__name__ + name = self._next.popleft() + step = getattr(self, '_step_{}'.format(name)) return step, name def __next__(self): @@ -109,7 +108,7 @@ class SubscriptionWorkflow: # yet. This is required, so use the address. self.user.preferred_address = self.address - def _verification_check(self): + def _step_verification_check(self): if self.address.verified_on is not None: # The address is already verified. Give the user a preferred # address if it doesn't already have one. We may still have to do @@ -127,35 +126,35 @@ class SubscriptionWorkflow: # Since the address was not already verified, and not # pre-verified, we have to send a confirmation check, which # doubles as a verification step. Skip to that now. - self._next.append(self._send_confirmation) + self._next.append("send_confirmation") return - self._next.append(self._confirmation_check) + self._next.append("confirmation_check") - def _confirmation_check(self): + def _step_confirmation_check(self): # Must the user confirm their subscription request? If the policy is # open subscriptions, then we need neither confirmation nor moderator # approval, so just subscribe them now. if self.mlist.subscription_policy == SubscriptionPolicy.open: - self._next.append(self._do_subscription) + self._next.append("do_subscription") elif self.pre_confirmed: # No confirmation is necessary. We can skip to seeing whether a # moderator confirmation is necessary. - self._next.append(self._moderation_check) + self._next.append("moderation_check") else: - self._next.append(self._send_confirmation) + self._next.append("send_confirmation") - def _moderation_check(self): + def _step_moderation_check(self): # Does the moderator need to approve the subscription request? if self.mlist.subscription_policy in ( SubscriptionPolicy.moderate, SubscriptionPolicy.confirm_then_moderate): - self._next.append(self._get_moderator_approval) + self._next.append("get_moderator_approval") else: # The moderator does not need to approve the subscription, so go # ahead and do that now. - self._next.append(self._do_subscription) + self._next.append("do_subscription") - def _get_moderator_approval(self): + def _step_get_moderator_approval(self): # In order to get the moderator's approval, we need to hold the # subscription request in the database request = RequestRecord( @@ -163,7 +162,7 @@ class SubscriptionWorkflow: DeliveryMode.regular, 'en') hold_subscription(self.mlist, request) - def _do_subscription(self): + def _step_do_subscription(self): # We can immediately subscribe the user to the mailing list. self.mlist.subscribe(self.subscriber) -- cgit v1.2.3-70-g09d2 From 990f3c63814bcfddef2e661248c44e67e945de04 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Wed, 25 Mar 2015 18:11:11 +0100 Subject: Add a table to store workflow states --- src/mailman/config/configure.zcml | 5 +++ .../versions/2bb9b382198_workflow_state_table.py | 28 +++++++++++++++ src/mailman/interfaces/workflowstate.py | 42 ++++++++++++++++++++++ src/mailman/model/workflowstate.py | 41 +++++++++++++++++++++ 4 files changed, 116 insertions(+) create mode 100644 src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py create mode 100644 src/mailman/interfaces/workflowstate.py create mode 100644 src/mailman/model/workflowstate.py diff --git a/src/mailman/config/configure.zcml b/src/mailman/config/configure.zcml index 24061f0f0..6f5876aa8 100644 --- a/src/mailman/config/configure.zcml +++ b/src/mailman/config/configure.zcml @@ -117,4 +117,9 @@ factory="mailman.app.templates.TemplateLoader" /> + + diff --git a/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py new file mode 100644 index 000000000..23b3adebb --- /dev/null +++ b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py @@ -0,0 +1,28 @@ +"""Workflow state table + +Revision ID: 2bb9b382198 +Revises: 16c2b25c7b +Create Date: 2015-03-25 18:09:18.338790 + +""" + +# revision identifiers, used by Alembic. +revision = '2bb9b382198' +down_revision = '16c2b25c7b' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.create_table('workflowstate', + sa.Column('name', sa.Unicode(), nullable=False), + sa.Column('key', sa.Unicode(), nullable=False), + sa.Column('step', sa.Unicode(), nullable=False), + sa.Column('data', sa.Unicode(), nullable=True), + sa.PrimaryKeyConstraint('name', 'key') + ) + + +def downgrade(): + op.drop_table('workflowstate') diff --git a/src/mailman/interfaces/workflowstate.py b/src/mailman/interfaces/workflowstate.py new file mode 100644 index 000000000..bb08fef2d --- /dev/null +++ b/src/mailman/interfaces/workflowstate.py @@ -0,0 +1,42 @@ +# Copyright (C) 2007-2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Interface describing the state of a workflow.""" + +__all__ = [ + 'IWorkflowState', + ] + + +from zope.interface import Interface, Attribute + + + +class IWorkflowState(Interface): + """A basic user.""" + + name = Attribute( + """The name of the workflow.""") + + key = Attribute( + """A unique key identifying the workflow instance.""") + + step = Attribute( + """This workflow's next step.""") + + data = Attribute( + """Additional data (may be JSON-encodedeJSON .""") diff --git a/src/mailman/model/workflowstate.py b/src/mailman/model/workflowstate.py new file mode 100644 index 000000000..5e3301dcf --- /dev/null +++ b/src/mailman/model/workflowstate.py @@ -0,0 +1,41 @@ +# Copyright (C) 2007-2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Model for workflow states.""" + +__all__ = [ + 'WorkflowState', + ] + + +from mailman.database.model import Model +from mailman.interfaces.workflowstate import IWorkflowState +from sqlalchemy import Column, Unicode +from zope.interface import implementer + + + +@implementer(IWorkflowState) +class WorkflowState(Model): + """Workflow states.""" + + __tablename__ = 'workflowstate' + + name = Column(Unicode, primary_key=True) + key = Column(Unicode, primary_key=True) + step = Column(Unicode, nullable=False) + data = Column(Unicode) -- cgit v1.2.3-70-g09d2 From 035d023662e1347e7dc3a5347d17932bfbe8c7e3 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Wed, 25 Mar 2015 19:21:38 +0100 Subject: Save the workflow state in the database --- src/mailman/app/subscriptions.py | 16 +++++++++++++++ src/mailman/app/tests/test_subscriptions.py | 15 +++++++++++++++ src/mailman/config/configure.zcml | 4 ++-- src/mailman/interfaces/workflowstate.py | 30 ++++++++++++++++++++++++++++- src/mailman/model/workflowstate.py | 27 +++++++++++++++++++++++++- 5 files changed, 88 insertions(+), 4 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index ca84e091f..04e0955cb 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -44,6 +44,7 @@ from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager +from mailman.interfaces.workflowstate import IWorkflowStateManager from mailman.model.member import Member from mailman.utilities.datetime import now @@ -74,6 +75,7 @@ class SubscriptionWorkflow: self.pre_verified = pre_verified self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved + self._save_key = "{}:{}".format(self.mlist.list_id, self.address.email) # Prepare the state machine. self._next = deque() self._next.append("verification_check") @@ -95,6 +97,20 @@ class SubscriptionWorkflow: except: raise + def save_state(self): + manager = getUtility(IWorkflowStateManager) + # Note: only the next step is saved, not the whole stack. Not an issue + # since there's never more than a single step in the queue anyway. + # Also: we don't save & restore the self.pre_* variables, but we could, + # using the data argument. + manager.save(self.__class__.__name__, self._save_key, self._next[0]) + + def restore_state(self): + manager = getUtility(IWorkflowStateManager) + state = manager.restore(self.__class__.__name__, self._save_key) + if state is not None: + self._next[0] = state.step + def _maybe_set_preferred_address(self): if self.user is None: # The address has no linked user so create one, link it, and set diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 7bb59bb45..97dbd8382 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -82,6 +82,21 @@ class TestSubscriptionWorkflow(unittest.TestCase): self._anne = 'anne@example.com' self._user_manager = getUtility(IUserManager) + def test_save_restore(self): + anne = self._user_manager.create_address(self._anne, 'Anne Person') + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=False, pre_approved=False) + next(workflow) + next_step = workflow._next[0] + workflow.save_state() + # Now create a new instance and restore + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=False, pre_approved=False) + workflow.restore_state() + self.assertEqual(next_step, workflow._next[0]) + def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber # becomes a member with no human intervention. diff --git a/src/mailman/config/configure.zcml b/src/mailman/config/configure.zcml index 6f5876aa8..f6cb6dac1 100644 --- a/src/mailman/config/configure.zcml +++ b/src/mailman/config/configure.zcml @@ -118,8 +118,8 @@ /> diff --git a/src/mailman/interfaces/workflowstate.py b/src/mailman/interfaces/workflowstate.py index bb08fef2d..186386170 100644 --- a/src/mailman/interfaces/workflowstate.py +++ b/src/mailman/interfaces/workflowstate.py @@ -19,6 +19,7 @@ __all__ = [ 'IWorkflowState', + 'IWorkflowStateManager', ] @@ -27,7 +28,7 @@ from zope.interface import Interface, Attribute class IWorkflowState(Interface): - """A basic user.""" + """The state of a workflow.""" name = Attribute( """The name of the workflow.""") @@ -40,3 +41,30 @@ class IWorkflowState(Interface): data = Attribute( """Additional data (may be JSON-encodedeJSON .""") + + + +class IWorkflowStateManager(Interface): + """The workflow states manager.""" + + def save(name, key, step, data=None): + """Save the state of a workflow + + :param name: The name of the workflow. + :type name: str + :param key: A unique key identifying this workflow instance. + :type key: str + :param step: The next step for this workflow. + :type step: str + :param data: Additional data (workflow-specific). + :type data: str + """ + + def restore(name, key): + """Get the saved state for a workflow or None if nothing was saved. + + :param name: The name of the workflow. + :type name: str + :param key: A unique key identifying this workflow instance. + :type key: str + """ diff --git a/src/mailman/model/workflowstate.py b/src/mailman/model/workflowstate.py index 5e3301dcf..ee0226d61 100644 --- a/src/mailman/model/workflowstate.py +++ b/src/mailman/model/workflowstate.py @@ -19,11 +19,14 @@ __all__ = [ 'WorkflowState', + 'WorkflowStateManager', ] from mailman.database.model import Model -from mailman.interfaces.workflowstate import IWorkflowState +from mailman.database.transaction import dbconnection +from mailman.interfaces.workflowstate import ( + IWorkflowState, IWorkflowStateManager) from sqlalchemy import Column, Unicode from zope.interface import implementer @@ -39,3 +42,25 @@ class WorkflowState(Model): key = Column(Unicode, primary_key=True) step = Column(Unicode, nullable=False) data = Column(Unicode) + + + +@implementer(IWorkflowStateManager) +class WorkflowStateManager: + """See `IWorkflowStateManager`.""" + + @dbconnection + def save(self, store, name, key, step, data=None): + """See `IWorkflowStateManager`.""" + state = store.query(WorkflowState).get((name, key)) + if state is None: + state = store.add(WorkflowState( + name=name, key=key, step=step, data=data)) + else: + state.step = step + state.data = data + + @dbconnection + def restore(self, store, name, key): + """See `IWorkflowStateManager`.""" + return store.query(WorkflowState).get((name, key)) -- cgit v1.2.3-70-g09d2 From ba4570b9a2db63ce4b448fe293677fdb808daca1 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Thu, 26 Mar 2015 08:47:25 +0100 Subject: Save and restore attributes --- src/mailman/app/subscriptions.py | 22 ++++++++++++++++------ src/mailman/app/tests/test_subscriptions.py | 5 ++++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 04e0955cb..33f70241b 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -24,6 +24,7 @@ __all__ = [ ] +import json from collections import deque from operator import attrgetter from sqlalchemy import and_, or_ @@ -75,7 +76,10 @@ class SubscriptionWorkflow: self.pre_verified = pre_verified self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved + # State saving self._save_key = "{}:{}".format(self.mlist.list_id, self.address.email) + self._save_attributes = ("pre_verified", "pre_confirmed", + "pre_approved") # Prepare the state machine. self._next = deque() self._next.append("verification_check") @@ -98,18 +102,24 @@ class SubscriptionWorkflow: raise def save_state(self): - manager = getUtility(IWorkflowStateManager) + state_manager = getUtility(IWorkflowStateManager) + data = {attr: getattr(self, attr) for attr in self._save_attributes} # Note: only the next step is saved, not the whole stack. Not an issue # since there's never more than a single step in the queue anyway. - # Also: we don't save & restore the self.pre_* variables, but we could, - # using the data argument. - manager.save(self.__class__.__name__, self._save_key, self._next[0]) + state_manager.save( + self.__class__.__name__, + self._save_key, + self._next[0], + json.dumps(data)) def restore_state(self): - manager = getUtility(IWorkflowStateManager) - state = manager.restore(self.__class__.__name__, self._save_key) + state_manager = getUtility(IWorkflowStateManager) + state = state_manager.restore(self.__class__.__name__, self._save_key) if state is not None: self._next[0] = state.step + if state.data is not None: + for attr, value in json.loads(state.data).items(): + setattr(self, attr, value) def _maybe_set_preferred_address(self): if self.user is None: diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 97dbd8382..fde33b91e 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -93,9 +93,12 @@ class TestSubscriptionWorkflow(unittest.TestCase): # Now create a new instance and restore workflow = SubscriptionWorkflow( self._mlist, anne, - pre_verified=True, pre_confirmed=False, pre_approved=False) + pre_verified=None, pre_confirmed=None, pre_approved=None) workflow.restore_state() self.assertEqual(next_step, workflow._next[0]) + self.assertEqual(workflow.pre_verified, True) + self.assertEqual(workflow.pre_confirmed, False) + self.assertEqual(workflow.pre_approved, False) def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber -- cgit v1.2.3-70-g09d2 From 7f5d0e4d2b1d4043247543bb9c067d96e8280c2d Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Thu, 26 Mar 2015 09:51:20 +0100 Subject: Factor generic workflow operations in their own class --- src/mailman/app/subscriptions.py | 60 +++++++++++++++++++++++++--------------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 33f70241b..507f8bf28 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -59,30 +59,16 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) -class SubscriptionWorkflow: - """Workflow of a subscription request.""" +class Workflow: + """Generic workflow.""" + # TODO: move this class to a more generic module - def __init__(self, mlist, subscriber, - pre_verified, pre_confirmed, pre_approved): - self.mlist = mlist - # The subscriber must be either an IUser or IAddress. - if IAddress.providedBy(subscriber): - self.address = subscriber - self.user = self.address.user - elif IUser.providedBy(subscriber): - self.address = subscriber.preferred_address - self.user = subscriber - self.subscriber = subscriber - self.pre_verified = pre_verified - self.pre_confirmed = pre_confirmed - self.pre_approved = pre_approved - # State saving - self._save_key = "{}:{}".format(self.mlist.list_id, self.address.email) - self._save_attributes = ("pre_verified", "pre_confirmed", - "pre_approved") - # Prepare the state machine. - self._next = deque() - self._next.append("verification_check") + _save_key = None + _save_attributes = [] + _initial_state = [] + + def __init__(self): + self._next = deque(self._initial_state) def __iter__(self): return self @@ -106,6 +92,9 @@ class SubscriptionWorkflow: data = {attr: getattr(self, attr) for attr in self._save_attributes} # Note: only the next step is saved, not the whole stack. Not an issue # since there's never more than a single step in the queue anyway. + # If we want to support more than a single step in the queue AND want + # to support state saving/restoring, change this method and + # restore_state(). state_manager.save( self.__class__.__name__, self._save_key, @@ -121,6 +110,31 @@ class SubscriptionWorkflow: for attr, value in json.loads(state.data).items(): setattr(self, attr, value) + +class SubscriptionWorkflow(Workflow): + """Workflow of a subscription request.""" + + _save_attributes = ["pre_verified", "pre_confirmed", "pre_approved"] + _initial_state = ["verification_check"] + + def __init__(self, mlist, subscriber, + pre_verified, pre_confirmed, pre_approved): + super(SubscriptionWorkflow, self).__init__() + self.mlist = mlist + # The subscriber must be either an IUser or IAddress. + if IAddress.providedBy(subscriber): + self.address = subscriber + self.user = self.address.user + elif IUser.providedBy(subscriber): + self.address = subscriber.preferred_address + self.user = subscriber + self.subscriber = subscriber + self.pre_verified = pre_verified + self.pre_confirmed = pre_confirmed + self.pre_approved = pre_approved + # State saving + self._save_key = "{}:{}".format(self.mlist.list_id, self.address.email) + def _maybe_set_preferred_address(self): if self.user is None: # The address has no linked user so create one, link it, and set -- cgit v1.2.3-70-g09d2 From 2e4367b6aaba7b16a371cc98036ce2cbdeb35fbf Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 27 Mar 2015 12:53:59 +0100 Subject: Workflow: allow saving and restoring with an empty queue --- src/mailman/app/subscriptions.py | 19 +++++++++++++++---- src/mailman/app/tests/test_subscriptions.py | 15 +++++++++++++++ .../versions/2bb9b382198_workflow_state_table.py | 2 +- src/mailman/model/workflowstate.py | 4 ++-- 4 files changed, 33 insertions(+), 7 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 507f8bf28..375fe159d 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -93,19 +93,30 @@ class Workflow: # Note: only the next step is saved, not the whole stack. Not an issue # since there's never more than a single step in the queue anyway. # If we want to support more than a single step in the queue AND want - # to support state saving/restoring, change this method and - # restore_state(). + # to support state saving/restoring, change this method and the + # restore_state() method. + if len(self._next) == 0: + step = None + elif len(self._next) == 1: + step = self._next[0] + else: + raise AssertionError( + "Can't save a workflow state with more than one step " + "in the queue") state_manager.save( self.__class__.__name__, self._save_key, - self._next[0], + step, json.dumps(data)) def restore_state(self): state_manager = getUtility(IWorkflowStateManager) state = state_manager.restore(self.__class__.__name__, self._save_key) if state is not None: - self._next[0] = state.step + if not state.step: + self._next.clear() + else: + self._next[0] = state.step if state.data is not None: for attr, value in json.loads(state.data).items(): setattr(self, attr, value) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index fde33b91e..05646c929 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -100,6 +100,21 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertEqual(workflow.pre_confirmed, False) self.assertEqual(workflow.pre_approved, False) + def test_save_restore_no_next_step(self): + anne = self._user_manager.create_address(self._anne, 'Anne Person') + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=False, pre_approved=False) + workflow._next.pop() + self.assertEqual(len(workflow._next), 0) + workflow.save_state() + # Now create a new instance and restore + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=None, pre_confirmed=None, pre_approved=None) + workflow.restore_state() + self.assertEqual(len(workflow._next), 0) + def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber # becomes a member with no human intervention. diff --git a/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py index 23b3adebb..e6608b6ce 100644 --- a/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py +++ b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py @@ -18,7 +18,7 @@ def upgrade(): op.create_table('workflowstate', sa.Column('name', sa.Unicode(), nullable=False), sa.Column('key', sa.Unicode(), nullable=False), - sa.Column('step', sa.Unicode(), nullable=False), + sa.Column('step', sa.Unicode(), nullable=True), sa.Column('data', sa.Unicode(), nullable=True), sa.PrimaryKeyConstraint('name', 'key') ) diff --git a/src/mailman/model/workflowstate.py b/src/mailman/model/workflowstate.py index ee0226d61..229a2240b 100644 --- a/src/mailman/model/workflowstate.py +++ b/src/mailman/model/workflowstate.py @@ -40,7 +40,7 @@ class WorkflowState(Model): name = Column(Unicode, primary_key=True) key = Column(Unicode, primary_key=True) - step = Column(Unicode, nullable=False) + step = Column(Unicode) data = Column(Unicode) @@ -50,7 +50,7 @@ class WorkflowStateManager: """See `IWorkflowStateManager`.""" @dbconnection - def save(self, store, name, key, step, data=None): + def save(self, store, name, key, step=None, data=None): """See `IWorkflowStateManager`.""" state = store.query(WorkflowState).get((name, key)) if state is None: -- cgit v1.2.3-70-g09d2 From e98c240509d0797a5280f334b0efad4861b0f75b Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 27 Mar 2015 16:19:18 +0100 Subject: Start implmenting the send_confirmation step --- src/mailman/app/subscriptions.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 375fe159d..fc07a754e 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -194,6 +194,13 @@ class SubscriptionWorkflow(Workflow): else: self._next.append("send_confirmation") + def _step_send_confirmation(self): + self._next.append("moderation_check") + self.save_state() + self._next.clear() # stop iteration until we get confirmation + # XXX: create the Pendable, send the ConfirmationNeededEvent + # (see Registrar.register) + def _step_moderation_check(self): # Does the moderator need to approve the subscription request? if self.mlist.subscription_policy in ( -- cgit v1.2.3-70-g09d2 From 9fe551853e8170040a1f09fa340b1a6a4ff4194c Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 27 Mar 2015 16:39:06 +0100 Subject: Write and move tests for the bare Workflow class --- src/mailman/app/subscriptions.py | 9 ++-- src/mailman/app/tests/test_subscriptions.py | 74 ++++++++++++++++------------- 2 files changed, 46 insertions(+), 37 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index fc07a754e..0fc10572a 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -63,7 +63,7 @@ class Workflow: """Generic workflow.""" # TODO: move this class to a more generic module - _save_key = None + _save_key = "" _save_attributes = [] _initial_state = [] @@ -113,10 +113,9 @@ class Workflow: state_manager = getUtility(IWorkflowStateManager) state = state_manager.restore(self.__class__.__name__, self._save_key) if state is not None: - if not state.step: - self._next.clear() - else: - self._next[0] = state.step + self._next.clear() + if state.step: + self._next.append(state.step) if state.data is not None: for attr, value in json.loads(state.data).items(): setattr(self, attr, value) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 05646c929..325126214 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -27,7 +27,7 @@ import uuid import unittest from mailman.app.lifecycle import create_list -from mailman.app.subscriptions import SubscriptionWorkflow +from mailman.app.subscriptions import Workflow, SubscriptionWorkflow from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.member import MemberRole, MissingPreferredAddressError from mailman.interfaces.requests import IListRequests, RequestType @@ -37,6 +37,7 @@ from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager from mailman.utilities.datetime import now +from mock import Mock from zope.component import getUtility @@ -74,46 +75,55 @@ class TestJoin(unittest.TestCase): -class TestSubscriptionWorkflow(unittest.TestCase): +class TestWorkflow(unittest.TestCase): layer = ConfigLayer def setUp(self): - self._mlist = create_list('test@example.com') - self._anne = 'anne@example.com' - self._user_manager = getUtility(IUserManager) + self.workflow = Workflow() + self.workflow._test_attribute = "test-value" + self.workflow._step_test = Mock() + self.workflow._next.append("test") + + def test_iter_steps(self): + next(self.workflow) + self.assertTrue(self.workflow._step_test.called) + self.assertEqual(len(self.workflow._next), 0) + try: + next(self.workflow) + except StopIteration: + pass + else: + self.fail() def test_save_restore(self): - anne = self._user_manager.create_address(self._anne, 'Anne Person') - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=True, pre_confirmed=False, pre_approved=False) - next(workflow) - next_step = workflow._next[0] - workflow.save_state() + self.workflow.save_state() # Now create a new instance and restore - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=None, pre_confirmed=None, pre_approved=None) - workflow.restore_state() - self.assertEqual(next_step, workflow._next[0]) - self.assertEqual(workflow.pre_verified, True) - self.assertEqual(workflow.pre_confirmed, False) - self.assertEqual(workflow.pre_approved, False) + new_workflow = Workflow() + self.assertEqual(len(new_workflow._next), 0) + self.assertFalse(hasattr(new_workflow, "_test_attribute")) + new_workflow.restore_state() + self.assertEqual(len(new_workflow._next), 1) + self.assertEqual(new_workflow._next[0], "test") + self.assertEqual(self.workflow._test_attribute, "test-value") def test_save_restore_no_next_step(self): - anne = self._user_manager.create_address(self._anne, 'Anne Person') - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=True, pre_confirmed=False, pre_approved=False) - workflow._next.pop() - self.assertEqual(len(workflow._next), 0) - workflow.save_state() + self.workflow._next.clear() + self.workflow.save_state() # Now create a new instance and restore - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=None, pre_confirmed=None, pre_approved=None) - workflow.restore_state() - self.assertEqual(len(workflow._next), 0) + new_workflow = Workflow() + new_workflow._next.append("test") + new_workflow.restore_state() + self.assertEqual(len(new_workflow._next), 0) + + + +class TestSubscriptionWorkflow(unittest.TestCase): + layer = ConfigLayer + + def setUp(self): + self._mlist = create_list('test@example.com') + self._anne = 'anne@example.com' + self._user_manager = getUtility(IUserManager) def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber -- cgit v1.2.3-70-g09d2 From b62a026ad136147d9f1e1e9604f98bd4d98a3389 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Fri, 27 Mar 2015 16:50:01 +0100 Subject: Test when confirmations are required --- src/mailman/app/subscriptions.py | 2 +- src/mailman/app/tests/test_subscriptions.py | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 0fc10572a..1e45da43e 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -202,7 +202,7 @@ class SubscriptionWorkflow(Workflow): def _step_moderation_check(self): # Does the moderator need to approve the subscription request? - if self.mlist.subscription_policy in ( + if not self.pre_approved and self.mlist.subscription_policy in ( SubscriptionPolicy.moderate, SubscriptionPolicy.confirm_then_moderate): self._next.append("get_moderator_approval") diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 325126214..b19dc5398 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -166,5 +166,24 @@ class TestSubscriptionWorkflow(unittest.TestCase): request_db.delete_request(requests[0].id) self._mlist.subscription_policy = SubscriptionPolicy.moderate _do_check() - self._mlist.subscription_policy = SubscriptionPolicy.confirm_then_moderate + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate _do_check() + + def test_confirmation_required(self): + # Tests subscriptions where user confirmation is required + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + anne = self._user_manager.create_address(self._anne, 'Anne Person') + self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) + workflow = SubscriptionWorkflow( + self._mlist, anne, + pre_verified=True, pre_confirmed=False, pre_approved=True) + # Run the state machine to the end. + list(workflow) + # A confirmation request must be pending + # TODO: test it + # Now restore and re-run the state machine as if we got the confirmation + workflow.restore_state() + list(workflow) + self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) -- cgit v1.2.3-70-g09d2 From 9de0ea02dc757e41a5013bb41e68d04f44ed5066 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Sun, 29 Mar 2015 17:14:09 -0400 Subject: Refactorings and tests. * Move the basic Workflow class to a module in mailman.app. * Rename the interface and model modules. * Update the configure.zcml. * Minor style fixes. * Add a test for the workflow model. --- src/mailman/app/subscriptions.py | 76 ++------------ src/mailman/app/workflow.py | 91 +++++++++++++++++ src/mailman/config/configure.zcml | 4 +- .../versions/2bb9b382198_workflow_state_table.py | 2 +- src/mailman/interfaces/workflow.py | 66 +++++++++++++ src/mailman/interfaces/workflowstate.py | 70 ------------- src/mailman/model/tests/test_workflow.py | 110 +++++++++++++++++++++ src/mailman/model/workflow.py | 65 ++++++++++++ src/mailman/model/workflowstate.py | 66 ------------- 9 files changed, 341 insertions(+), 209 deletions(-) create mode 100644 src/mailman/app/workflow.py create mode 100644 src/mailman/interfaces/workflow.py delete mode 100644 src/mailman/interfaces/workflowstate.py create mode 100644 src/mailman/model/tests/test_workflow.py create mode 100644 src/mailman/model/workflow.py delete mode 100644 src/mailman/model/workflowstate.py diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 1e45da43e..8cd507bd2 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -24,16 +24,10 @@ __all__ = [ ] -import json -from collections import deque -from operator import attrgetter -from sqlalchemy import and_, or_ -from uuid import UUID -from zope.component import getUtility -from zope.interface import implementer from mailman.app.membership import add_member, delete_member from mailman.app.moderator import hold_subscription +from mailman.app.workflow import Workflow from mailman.core.constants import system_preferences from mailman.database.transaction import dbconnection from mailman.interfaces.address import IAddress @@ -45,9 +39,13 @@ from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager -from mailman.interfaces.workflowstate import IWorkflowStateManager from mailman.model.member import Member from mailman.utilities.datetime import now +from operator import attrgetter +from sqlalchemy import and_, or_ +from uuid import UUID +from zope.component import getUtility +from zope.interface import implementer def _membership_sort_key(member): @@ -59,68 +57,6 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) -class Workflow: - """Generic workflow.""" - # TODO: move this class to a more generic module - - _save_key = "" - _save_attributes = [] - _initial_state = [] - - def __init__(self): - self._next = deque(self._initial_state) - - def __iter__(self): - return self - - def _pop(self): - name = self._next.popleft() - step = getattr(self, '_step_{}'.format(name)) - return step, name - - def __next__(self): - try: - step, name = self._pop() - step() - except IndexError: - raise StopIteration - except: - raise - - def save_state(self): - state_manager = getUtility(IWorkflowStateManager) - data = {attr: getattr(self, attr) for attr in self._save_attributes} - # Note: only the next step is saved, not the whole stack. Not an issue - # since there's never more than a single step in the queue anyway. - # If we want to support more than a single step in the queue AND want - # to support state saving/restoring, change this method and the - # restore_state() method. - if len(self._next) == 0: - step = None - elif len(self._next) == 1: - step = self._next[0] - else: - raise AssertionError( - "Can't save a workflow state with more than one step " - "in the queue") - state_manager.save( - self.__class__.__name__, - self._save_key, - step, - json.dumps(data)) - - def restore_state(self): - state_manager = getUtility(IWorkflowStateManager) - state = state_manager.restore(self.__class__.__name__, self._save_key) - if state is not None: - self._next.clear() - if state.step: - self._next.append(state.step) - if state.data is not None: - for attr, value in json.loads(state.data).items(): - setattr(self, attr, value) - - class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py new file mode 100644 index 000000000..91c8c9f84 --- /dev/null +++ b/src/mailman/app/workflow.py @@ -0,0 +1,91 @@ +# Copyright (C) 2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Generic workflow.""" + +__all__ = [ + 'Workflow', + ] + + +import json + +from collections import deque +from mailman.interfaces.workflow import IWorkflowStateManager +from zope.component import getUtility + + + +class Workflow: + """Generic workflow.""" + + _save_key = '' + _save_attributes = [] + _initial_state = [] + + def __init__(self): + self._next = deque(self._initial_state) + + def __iter__(self): + return self + + def _pop(self): + name = self._next.popleft() + step = getattr(self, '_step_{}'.format(name)) + return step, name + + def __next__(self): + try: + step, name = self._pop() + step() + except IndexError: + raise StopIteration + except: + raise + + def save_state(self): + state_manager = getUtility(IWorkflowStateManager) + data = {attr: getattr(self, attr) for attr in self._save_attributes} + # Note: only the next step is saved, not the whole stack. Not an issue + # since there's never more than a single step in the queue anyway. + # If we want to support more than a single step in the queue AND want + # to support state saving/restoring, change this method and the + # restore_state() method. + if len(self._next) == 0: + step = None + elif len(self._next) == 1: + step = self._next[0] + else: + raise AssertionError( + "Can't save a workflow state with more than one step " + "in the queue") + state_manager.save( + self.__class__.__name__, + self._save_key, + step, + json.dumps(data)) + + def restore_state(self): + state_manager = getUtility(IWorkflowStateManager) + state = state_manager.restore(self.__class__.__name__, self._save_key) + if state is not None: + self._next.clear() + if state.step: + self._next.append(state.step) + if state.data is not None: + for attr, value in json.loads(state.data).items(): + setattr(self, attr, value) diff --git a/src/mailman/config/configure.zcml b/src/mailman/config/configure.zcml index f6cb6dac1..1f2283b02 100644 --- a/src/mailman/config/configure.zcml +++ b/src/mailman/config/configure.zcml @@ -118,8 +118,8 @@ /> diff --git a/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py index e6608b6ce..59cb1121e 100644 --- a/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py +++ b/src/mailman/database/alembic/versions/2bb9b382198_workflow_state_table.py @@ -21,7 +21,7 @@ def upgrade(): sa.Column('step', sa.Unicode(), nullable=True), sa.Column('data', sa.Unicode(), nullable=True), sa.PrimaryKeyConstraint('name', 'key') - ) + ) def downgrade(): diff --git a/src/mailman/interfaces/workflow.py b/src/mailman/interfaces/workflow.py new file mode 100644 index 000000000..b5aeec093 --- /dev/null +++ b/src/mailman/interfaces/workflow.py @@ -0,0 +1,66 @@ +# Copyright (C) 2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Interfaces describing the state of a workflow.""" + +__all__ = [ + 'IWorkflowState', + 'IWorkflowStateManager', + ] + + +from zope.interface import Attribute, Interface + + + +class IWorkflowState(Interface): + """The state of a workflow.""" + + name = Attribute('The name of the workflow.') + + key = Attribute('A unique key identifying the workflow instance.') + + step = Attribute("This workflow's next step.") + + data = Attribute('Additional data (may be JSON-encodedeJSON .') + + + +class IWorkflowStateManager(Interface): + """The workflow states manager.""" + + def save(name, key, step, data=None): + """Save the state of a workflow + + :param name: The name of the workflow. + :type name: str + :param key: A unique key identifying this workflow instance. + :type key: str + :param step: The next step for this workflow. + :type step: str + :param data: Additional data (workflow-specific). + :type data: str + """ + + def restore(name, key): + """Get the saved state for a workflow or None if nothing was saved. + + :param name: The name of the workflow. + :type name: str + :param key: A unique key identifying this workflow instance. + :type key: str + """ diff --git a/src/mailman/interfaces/workflowstate.py b/src/mailman/interfaces/workflowstate.py deleted file mode 100644 index 186386170..000000000 --- a/src/mailman/interfaces/workflowstate.py +++ /dev/null @@ -1,70 +0,0 @@ -# Copyright (C) 2007-2015 by the Free Software Foundation, Inc. -# -# This file is part of GNU Mailman. -# -# GNU Mailman is free software: you can redistribute it and/or modify it under -# the terms of the GNU General Public License as published by the Free -# Software Foundation, either version 3 of the License, or (at your option) -# any later version. -# -# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for -# more details. -# -# You should have received a copy of the GNU General Public License along with -# GNU Mailman. If not, see . - -"""Interface describing the state of a workflow.""" - -__all__ = [ - 'IWorkflowState', - 'IWorkflowStateManager', - ] - - -from zope.interface import Interface, Attribute - - - -class IWorkflowState(Interface): - """The state of a workflow.""" - - name = Attribute( - """The name of the workflow.""") - - key = Attribute( - """A unique key identifying the workflow instance.""") - - step = Attribute( - """This workflow's next step.""") - - data = Attribute( - """Additional data (may be JSON-encodedeJSON .""") - - - -class IWorkflowStateManager(Interface): - """The workflow states manager.""" - - def save(name, key, step, data=None): - """Save the state of a workflow - - :param name: The name of the workflow. - :type name: str - :param key: A unique key identifying this workflow instance. - :type key: str - :param step: The next step for this workflow. - :type step: str - :param data: Additional data (workflow-specific). - :type data: str - """ - - def restore(name, key): - """Get the saved state for a workflow or None if nothing was saved. - - :param name: The name of the workflow. - :type name: str - :param key: A unique key identifying this workflow instance. - :type key: str - """ diff --git a/src/mailman/model/tests/test_workflow.py b/src/mailman/model/tests/test_workflow.py new file mode 100644 index 000000000..b5e915df4 --- /dev/null +++ b/src/mailman/model/tests/test_workflow.py @@ -0,0 +1,110 @@ +# Copyright (C) 2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Test the workflow model.""" + +__all__ = [ + 'TestWorkflow', + ] + + +import unittest + +from mailman.interfaces.workflow import IWorkflowStateManager +from mailman.testing.layers import ConfigLayer +from zope.component import getUtility + + + +class TestWorkflow(unittest.TestCase): + layer = ConfigLayer + + def setUp(self): + self._manager = getUtility(IWorkflowStateManager) + + def test_save_restore_workflow(self): + # Save and restore a workflow. + name = 'ant' + key = 'bee' + step = 'cat' + data = 'dog' + self._manager.save(name, key, step, data) + workflow = self._manager.restore(name, key) + self.assertEqual(workflow.name, name) + self.assertEqual(workflow.key, key) + self.assertEqual(workflow.step, step) + self.assertEqual(workflow.data, data) + + def test_save_restore_workflow_without_step(self): + # Save and restore a workflow that contains no step. + name = 'ant' + key = 'bee' + data = 'dog' + self._manager.save(name, key, data=data) + workflow = self._manager.restore(name, key) + self.assertEqual(workflow.name, name) + self.assertEqual(workflow.key, key) + self.assertIsNone(workflow.step) + self.assertEqual(workflow.data, data) + + def test_save_restore_workflow_without_data(self): + # Save and restore a workflow that contains no data. + name = 'ant' + key = 'bee' + step = 'cat' + self._manager.save(name, key, step) + workflow = self._manager.restore(name, key) + self.assertEqual(workflow.name, name) + self.assertEqual(workflow.key, key) + self.assertEqual(workflow.step, step) + self.assertIsNone(workflow.data) + + def test_save_restore_workflow_without_step_or_data(self): + # Save and restore a workflow that contains no step or data. + name = 'ant' + key = 'bee' + self._manager.save(name, key) + workflow = self._manager.restore(name, key) + self.assertEqual(workflow.name, name) + self.assertEqual(workflow.key, key) + self.assertIsNone(workflow.step) + self.assertIsNone(workflow.data) + + def test_restore_workflow_with_no_matching_name(self): + # Try to restore a workflow that has no matching name in the database. + name = 'ant' + key = 'bee' + self._manager.save(name, key) + workflow = self._manager.restore('ewe', key) + self.assertIsNone(workflow) + + def test_restore_workflow_with_no_matching_key(self): + # Try to restore a workflow that has no matching key in the database. + name = 'ant' + key = 'bee' + self._manager.save(name, key) + workflow = self._manager.restore(name, 'fly') + self.assertIsNone(workflow) + + def test_restore_workflow_with_no_matching_key_or_name(self): + # Try to restore a workflow that has no matching key or name in the + # database. + name = 'ant' + key = 'bee' + self._manager.save(name, key) + workflow = self._manager.restore('ewe', 'fly') + self.assertIsNone(workflow) diff --git a/src/mailman/model/workflow.py b/src/mailman/model/workflow.py new file mode 100644 index 000000000..53c9f05ea --- /dev/null +++ b/src/mailman/model/workflow.py @@ -0,0 +1,65 @@ +# Copyright (C) 2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Model for workflow states.""" + +__all__ = [ + 'WorkflowState', + 'WorkflowStateManager', + ] + + +from mailman.database.model import Model +from mailman.database.transaction import dbconnection +from mailman.interfaces.workflow import IWorkflowState, IWorkflowStateManager +from sqlalchemy import Column, Unicode +from zope.interface import implementer + + + +@implementer(IWorkflowState) +class WorkflowState(Model): + """Workflow states.""" + + __tablename__ = 'workflowstate' + + name = Column(Unicode, primary_key=True) + key = Column(Unicode, primary_key=True) + step = Column(Unicode) + data = Column(Unicode) + + + +@implementer(IWorkflowStateManager) +class WorkflowStateManager: + """See `IWorkflowStateManager`.""" + + @dbconnection + def save(self, store, name, key, step=None, data=None): + """See `IWorkflowStateManager`.""" + state = store.query(WorkflowState).get((name, key)) + if state is None: + state = WorkflowState(name=name, key=key, step=step, data=data) + store.add(state) + else: + state.step = step + state.data = data + + @dbconnection + def restore(self, store, name, key): + """See `IWorkflowStateManager`.""" + return store.query(WorkflowState).get((name, key)) diff --git a/src/mailman/model/workflowstate.py b/src/mailman/model/workflowstate.py deleted file mode 100644 index 229a2240b..000000000 --- a/src/mailman/model/workflowstate.py +++ /dev/null @@ -1,66 +0,0 @@ -# Copyright (C) 2007-2015 by the Free Software Foundation, Inc. -# -# This file is part of GNU Mailman. -# -# GNU Mailman is free software: you can redistribute it and/or modify it under -# the terms of the GNU General Public License as published by the Free -# Software Foundation, either version 3 of the License, or (at your option) -# any later version. -# -# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for -# more details. -# -# You should have received a copy of the GNU General Public License along with -# GNU Mailman. If not, see . - -"""Model for workflow states.""" - -__all__ = [ - 'WorkflowState', - 'WorkflowStateManager', - ] - - -from mailman.database.model import Model -from mailman.database.transaction import dbconnection -from mailman.interfaces.workflowstate import ( - IWorkflowState, IWorkflowStateManager) -from sqlalchemy import Column, Unicode -from zope.interface import implementer - - - -@implementer(IWorkflowState) -class WorkflowState(Model): - """Workflow states.""" - - __tablename__ = 'workflowstate' - - name = Column(Unicode, primary_key=True) - key = Column(Unicode, primary_key=True) - step = Column(Unicode) - data = Column(Unicode) - - - -@implementer(IWorkflowStateManager) -class WorkflowStateManager: - """See `IWorkflowStateManager`.""" - - @dbconnection - def save(self, store, name, key, step=None, data=None): - """See `IWorkflowStateManager`.""" - state = store.query(WorkflowState).get((name, key)) - if state is None: - state = store.add(WorkflowState( - name=name, key=key, step=step, data=data)) - else: - state.step = step - state.data = data - - @dbconnection - def restore(self, store, name, key): - """See `IWorkflowStateManager`.""" - return store.query(WorkflowState).get((name, key)) -- cgit v1.2.3-70-g09d2 From 72d42234668213130d79eee4e57df94339f57567 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Sun, 29 Mar 2015 17:18:30 -0400 Subject: Fix merge turd. --- src/mailman/runners/docs/outgoing.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mailman/runners/docs/outgoing.rst b/src/mailman/runners/docs/outgoing.rst index cc544d6eb..8888aee5e 100644 --- a/src/mailman/runners/docs/outgoing.rst +++ b/src/mailman/runners/docs/outgoing.rst @@ -11,8 +11,8 @@ a *delivery module*, essentially a pluggable interface for determining how the recipient set will be batched, whether messages will be personalized and VERP'd, etc. The outgoing runner doesn't itself support retrying but it can move messages to the 'retry queue' for handling delivery failures. +:: - >>> from mailman.testing.helpers import subscribe >>> mlist = create_list('test@example.com') >>> from mailman.testing.helpers import subscribe -- cgit v1.2.3-70-g09d2 From 2c416eea7ff40af50234f0ea9a9a0f6abfb99251 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Sun, 29 Mar 2015 17:21:52 -0400 Subject: Fix merge turd. --- src/mailman/app/subscriptions.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 8cd507bd2..5dbc3a5f6 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -48,6 +48,7 @@ from zope.component import getUtility from zope.interface import implementer + def _membership_sort_key(member): """Sort function for find_members(). @@ -57,6 +58,7 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) + class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" @@ -276,6 +278,7 @@ class SubscriptionService: delete_member(mlist, email, False, False) + def handle_ListDeletingEvent(event): """Delete a mailing list's members when the list is being deleted.""" -- cgit v1.2.3-70-g09d2 From 45ae1b259e2ee56f6146bc1a3acf884b0907999d Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Sun, 29 Mar 2015 22:05:43 -0400 Subject: * Refactor test_workflow into a separate module. * save_state() -> save() * restore_state() -> restore() * Add push() as the public API for pushing new state on the deque. * Uppercase class attributes. * Add logging on exception. * Minor style fixes. --- src/mailman/app/subscriptions.py | 28 ++++--- src/mailman/app/tests/test_subscriptions.py | 47 +---------- src/mailman/app/tests/test_workflow.py | 118 ++++++++++++++++++++++++++++ src/mailman/app/workflow.py | 45 +++++++---- 4 files changed, 164 insertions(+), 74 deletions(-) create mode 100644 src/mailman/app/tests/test_workflow.py diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 5dbc3a5f6..d72873616 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -62,8 +62,12 @@ def _membership_sort_key(member): class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" - _save_attributes = ["pre_verified", "pre_confirmed", "pre_approved"] - _initial_state = ["verification_check"] + INITIAL_STATE = 'verification_check' + SAVE_ATTRIBUTES = ( + 'pre_approved', + 'pre_confirmed', + 'pre_verified', + ) def __init__(self, mlist, subscriber, pre_verified, pre_confirmed, pre_approved): @@ -81,7 +85,7 @@ class SubscriptionWorkflow(Workflow): self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved # State saving - self._save_key = "{}:{}".format(self.mlist.list_id, self.address.email) + self.SAVE_KEY = '{}:{}'.format(self.mlist.list_id, self.address.email) def _maybe_set_preferred_address(self): if self.user is None: @@ -114,26 +118,26 @@ class SubscriptionWorkflow(Workflow): # Since the address was not already verified, and not # pre-verified, we have to send a confirmation check, which # doubles as a verification step. Skip to that now. - self._next.append("send_confirmation") + self._next.append('send_confirmation') return - self._next.append("confirmation_check") + self._next.append('confirmation_check') def _step_confirmation_check(self): # Must the user confirm their subscription request? If the policy is # open subscriptions, then we need neither confirmation nor moderator # approval, so just subscribe them now. if self.mlist.subscription_policy == SubscriptionPolicy.open: - self._next.append("do_subscription") + self._next.append('do_subscription') elif self.pre_confirmed: # No confirmation is necessary. We can skip to seeing whether a # moderator confirmation is necessary. - self._next.append("moderation_check") + self._next.append('moderation_check') else: - self._next.append("send_confirmation") + self._next.append('send_confirmation') def _step_send_confirmation(self): - self._next.append("moderation_check") - self.save_state() + self._next.append('moderation_check') + self.save() self._next.clear() # stop iteration until we get confirmation # XXX: create the Pendable, send the ConfirmationNeededEvent # (see Registrar.register) @@ -143,11 +147,11 @@ class SubscriptionWorkflow(Workflow): if not self.pre_approved and self.mlist.subscription_policy in ( SubscriptionPolicy.moderate, SubscriptionPolicy.confirm_then_moderate): - self._next.append("get_moderator_approval") + self._next.append('get_moderator_approval') else: # The moderator does not need to approve the subscription, so go # ahead and do that now. - self._next.append("do_subscription") + self._next.append('do_subscription') def _step_get_moderator_approval(self): # In order to get the moderator's approval, we need to hold the diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index b19dc5398..5a767f463 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -27,7 +27,7 @@ import uuid import unittest from mailman.app.lifecycle import create_list -from mailman.app.subscriptions import Workflow, SubscriptionWorkflow +from mailman.app.subscriptions import SubscriptionWorkflow from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.member import MemberRole, MissingPreferredAddressError from mailman.interfaces.requests import IListRequests, RequestType @@ -37,7 +37,6 @@ from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager from mailman.utilities.datetime import now -from mock import Mock from zope.component import getUtility @@ -74,48 +73,6 @@ class TestJoin(unittest.TestCase): role=MemberRole.owner) - -class TestWorkflow(unittest.TestCase): - layer = ConfigLayer - - def setUp(self): - self.workflow = Workflow() - self.workflow._test_attribute = "test-value" - self.workflow._step_test = Mock() - self.workflow._next.append("test") - - def test_iter_steps(self): - next(self.workflow) - self.assertTrue(self.workflow._step_test.called) - self.assertEqual(len(self.workflow._next), 0) - try: - next(self.workflow) - except StopIteration: - pass - else: - self.fail() - - def test_save_restore(self): - self.workflow.save_state() - # Now create a new instance and restore - new_workflow = Workflow() - self.assertEqual(len(new_workflow._next), 0) - self.assertFalse(hasattr(new_workflow, "_test_attribute")) - new_workflow.restore_state() - self.assertEqual(len(new_workflow._next), 1) - self.assertEqual(new_workflow._next[0], "test") - self.assertEqual(self.workflow._test_attribute, "test-value") - - def test_save_restore_no_next_step(self): - self.workflow._next.clear() - self.workflow.save_state() - # Now create a new instance and restore - new_workflow = Workflow() - new_workflow._next.append("test") - new_workflow.restore_state() - self.assertEqual(len(new_workflow._next), 0) - - class TestSubscriptionWorkflow(unittest.TestCase): layer = ConfigLayer @@ -184,6 +141,6 @@ class TestSubscriptionWorkflow(unittest.TestCase): # A confirmation request must be pending # TODO: test it # Now restore and re-run the state machine as if we got the confirmation - workflow.restore_state() + workflow.restore() list(workflow) self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) diff --git a/src/mailman/app/tests/test_workflow.py b/src/mailman/app/tests/test_workflow.py new file mode 100644 index 000000000..6bae46e8a --- /dev/null +++ b/src/mailman/app/tests/test_workflow.py @@ -0,0 +1,118 @@ +# Copyright (C) 2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""App-level workflow tests.""" + +__all__ = [ + 'TestWorkflow', + ] + + +import unittest + +from mailman.app.workflow import Workflow +from mailman.testing.layers import ConfigLayer + + +class MyWorkflow(Workflow): + INITIAL_STATE = 'first' + SAVE_ATTRIBUTES = ('ant', 'bee', 'cat') + SAVE_KEY = 'test-workflow' + + def __init__(self): + super().__init__() + self.ant = 1 + self.bee = 2 + self.cat = 3 + self.dog = 4 + + def _step_first(self): + self.push('second') + return 'one' + + def _step_second(self): + self.push('third') + return 'two' + + def _step_third(self): + return 'three' + + + +class TestWorkflow(unittest.TestCase): + layer = ConfigLayer + + def setUp(self): + self._workflow = iter(MyWorkflow()) + + def test_basic_workflow(self): + # The work flows from one state to the next. + results = list(self._workflow) + self.assertEqual(results, ['one', 'two', 'three']) + + def test_partial_workflow(self): + # You don't have to flow through every step. + results = next(self._workflow) + self.assertEqual(results, 'one') + + def test_exhaust_workflow(self): + # Manually flow through a few steps, then consume the whole thing. + results = [next(self._workflow)] + results.extend(self._workflow) + self.assertEqual(results, ['one', 'two', 'three']) + + def test_save_and_restore_workflow(self): + # Without running any steps, save and restore the workflow. Then + # consume the restored workflow. + self._workflow.save() + new_workflow = MyWorkflow() + new_workflow.restore() + results = list(new_workflow) + self.assertEqual(results, ['one', 'two', 'three']) + + def test_save_and_restore_partial_workflow(self): + # After running a few steps, save and restore the workflow. Then + # consume the restored workflow. + next(self._workflow) + self._workflow.save() + new_workflow = MyWorkflow() + new_workflow.restore() + results = list(new_workflow) + self.assertEqual(results, ['two', 'three']) + + def test_save_and_restore_exhausted_workflow(self): + # After consuming the entire workflow, save and restore it. + list(self._workflow) + self._workflow.save() + new_workflow = MyWorkflow() + new_workflow.restore() + results = list(new_workflow) + self.assertEqual(len(results), 0) + + def test_save_and_restore_attributes(self): + # Saved attributes are restored. + self._workflow.ant = 9 + self._workflow.bee = 8 + self._workflow.cat = 7 + # Don't save .dog. + self._workflow.save() + new_workflow = MyWorkflow() + new_workflow.restore() + self.assertEqual(new_workflow.ant, 9) + self.assertEqual(new_workflow.bee, 8) + self.assertEqual(new_workflow.cat, 7) + self.assertEqual(new_workflow.dog, 4) diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index 91c8c9f84..004b15c75 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -23,48 +23,59 @@ __all__ = [ import json +import logging from collections import deque from mailman.interfaces.workflow import IWorkflowStateManager from zope.component import getUtility +COMMASPACE = ', ' +log = logging.getLogger('mailman.error') + + class Workflow: """Generic workflow.""" - _save_key = '' - _save_attributes = [] - _initial_state = [] + SAVE_KEY = '' + SAVE_ATTRIBUTES = () + INITIAL_STATE = None def __init__(self): - self._next = deque(self._initial_state) + self._next = deque() + self.push(self.INITIAL_STATE) def __iter__(self): return self + def push(self, step): + self._next.append(step) + def _pop(self): name = self._next.popleft() step = getattr(self, '_step_{}'.format(name)) - return step, name + return name, step def __next__(self): try: - step, name = self._pop() - step() + name, step = self._pop() + return step() except IndexError: raise StopIteration except: + log.exception('deque: {}'.format(COMMASPACE.join(self._next))) raise - def save_state(self): + def save(self): + assert self.SAVE_KEY, 'Workflow SAVE_KEY must be set' state_manager = getUtility(IWorkflowStateManager) - data = {attr: getattr(self, attr) for attr in self._save_attributes} - # Note: only the next step is saved, not the whole stack. Not an issue - # since there's never more than a single step in the queue anyway. - # If we want to support more than a single step in the queue AND want - # to support state saving/restoring, change this method and the - # restore_state() method. + data = {attr: getattr(self, attr) for attr in self.SAVE_ATTRIBUTES} + # Note: only the next step is saved, not the whole stack. This is not + # an issue in practice, since there's never more than a single step in + # the queue anyway. If we want to support more than a single step in + # the queue *and* want to support state saving/restoring, change this + # method and the restore_state() method. if len(self._next) == 0: step = None elif len(self._next) == 1: @@ -75,13 +86,13 @@ class Workflow: "in the queue") state_manager.save( self.__class__.__name__, - self._save_key, + self.SAVE_KEY, step, json.dumps(data)) - def restore_state(self): + def restore(self): state_manager = getUtility(IWorkflowStateManager) - state = state_manager.restore(self.__class__.__name__, self._save_key) + state = state_manager.restore(self.__class__.__name__, self.SAVE_KEY) if state is not None: self._next.clear() if state.step: -- cgit v1.2.3-70-g09d2 From 017decd6fb8288ec498a4ab32be2273699c7cdaf Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 30 Mar 2015 09:10:34 -0400 Subject: Fix some typos. --- src/mailman/app/workflow.py | 2 +- src/mailman/interfaces/workflow.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index 004b15c75..bba5bb694 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -75,7 +75,7 @@ class Workflow: # an issue in practice, since there's never more than a single step in # the queue anyway. If we want to support more than a single step in # the queue *and* want to support state saving/restoring, change this - # method and the restore_state() method. + # method and the restore() method. if len(self._next) == 0: step = None elif len(self._next) == 1: diff --git a/src/mailman/interfaces/workflow.py b/src/mailman/interfaces/workflow.py index b5aeec093..49fbc85d4 100644 --- a/src/mailman/interfaces/workflow.py +++ b/src/mailman/interfaces/workflow.py @@ -36,7 +36,7 @@ class IWorkflowState(Interface): step = Attribute("This workflow's next step.") - data = Attribute('Additional data (may be JSON-encodedeJSON .') + data = Attribute('Additional data (may be JSON-encoded).') @@ -44,7 +44,7 @@ class IWorkflowStateManager(Interface): """The workflow states manager.""" def save(name, key, step, data=None): - """Save the state of a workflow + """Save the state of a workflow. :param name: The name of the workflow. :type name: str -- cgit v1.2.3-70-g09d2 From f75287d17af321a5ee85c499688a5c7ad0aae589 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Tue, 31 Mar 2015 12:41:05 +0200 Subject: Advertise the subscription policy in the REST API --- src/mailman/rest/docs/listconf.rst | 3 +++ src/mailman/rest/listconf.py | 4 +++- src/mailman/rest/tests/test_listconf.py | 1 + 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/mailman/rest/docs/listconf.rst b/src/mailman/rest/docs/listconf.rst index 841ab3c27..bcf4f856e 100644 --- a/src/mailman/rest/docs/listconf.rst +++ b/src/mailman/rest/docs/listconf.rst @@ -61,6 +61,7 @@ All readable attributes for a list are available on a sub-resource. scheme: http send_welcome_message: True subject_prefix: [Ant] + subscription_policy: confirm volume: 1 web_host: lists.example.com welcome_message_uri: mailman:///welcome.txt @@ -106,6 +107,7 @@ When using ``PUT``, all writable attributes must be included. ... reply_to_address='bee@example.com', ... send_welcome_message=False, ... subject_prefix='[ant]', + ... subscription_policy='moderate', ... welcome_message_uri='mailman:///welcome.txt', ... default_member_action='hold', ... default_nonmember_action='discard', @@ -156,6 +158,7 @@ These values are changed permanently. ... send_welcome_message: False subject_prefix: [ant] + subscription_policy: moderate ... welcome_message_uri: mailman:///welcome.txt diff --git a/src/mailman/rest/listconf.py b/src/mailman/rest/listconf.py index e83f52833..92d1169d4 100644 --- a/src/mailman/rest/listconf.py +++ b/src/mailman/rest/listconf.py @@ -29,7 +29,8 @@ from mailman.core.errors import ( from mailman.interfaces.action import Action from mailman.interfaces.archiver import ArchivePolicy from mailman.interfaces.autorespond import ResponseAction -from mailman.interfaces.mailinglist import IAcceptableAliasSet, ReplyToMunging +from mailman.interfaces.mailinglist import ( + IAcceptableAliasSet, ReplyToMunging, SubscriptionPolicy) from mailman.rest.helpers import ( GetterSetter, bad_request, etag, no_content, okay) from mailman.rest.validator import PatchValidator, Validator, enum_validator @@ -142,6 +143,7 @@ ATTRIBUTES = dict( scheme=GetterSetter(None), send_welcome_message=GetterSetter(as_boolean), subject_prefix=GetterSetter(str), + subscription_policy=GetterSetter(enum_validator(SubscriptionPolicy)), volume=GetterSetter(None), web_host=GetterSetter(None), welcome_message_uri=GetterSetter(str), diff --git a/src/mailman/rest/tests/test_listconf.py b/src/mailman/rest/tests/test_listconf.py index b0107b199..ddb43a8ea 100644 --- a/src/mailman/rest/tests/test_listconf.py +++ b/src/mailman/rest/tests/test_listconf.py @@ -79,6 +79,7 @@ class TestConfiguration(unittest.TestCase): reply_to_address='bee@example.com', send_welcome_message=False, subject_prefix='[ant]', + subscription_policy='confirm_then_moderate', welcome_message_uri='mailman:///welcome.txt', default_member_action='hold', default_nonmember_action='discard', -- cgit v1.2.3-70-g09d2 From bf00467f633ae6a8523189c1b922ca6dcd6636b8 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 6 Apr 2015 22:12:37 -0400 Subject: Check pointing --- src/mailman/app/subscriptions.py | 6 +++--- src/mailman/app/tests/test_subscriptions.py | 15 +++++++++++++++ src/mailman/app/tests/test_workflow.py | 10 +++++++++- src/mailman/app/workflow.py | 8 ++++---- 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index d72873616..ebb4198bb 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -71,7 +71,7 @@ class SubscriptionWorkflow(Workflow): def __init__(self, mlist, subscriber, pre_verified, pre_confirmed, pre_approved): - super(SubscriptionWorkflow, self).__init__() + super().__init__() self.mlist = mlist # The subscriber must be either an IUser or IAddress. if IAddress.providedBy(subscriber): @@ -80,12 +80,12 @@ class SubscriptionWorkflow(Workflow): elif IUser.providedBy(subscriber): self.address = subscriber.preferred_address self.user = subscriber + else: + raise AssertionError('subscriber is neither an IUser nor IAddress') self.subscriber = subscriber self.pre_verified = pre_verified self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved - # State saving - self.SAVE_KEY = '{}:{}'.format(self.mlist.list_id, self.address.email) def _maybe_set_preferred_address(self): if self.user is None: diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 5a767f463..d320284ce 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -82,6 +82,21 @@ class TestSubscriptionWorkflow(unittest.TestCase): self._anne = 'anne@example.com' self._user_manager = getUtility(IUserManager) + def test_user_or_address_required(self): + # The `subscriber` attribute must be a user or address. + self.assertRaises(AssertionError, SubscriptionWorkflow, + self._mlist, + 'not a user', False, False, False) + + def test_user_without_preferred_address_gets_one(self): + # When subscribing a user without a preferred address, the first step + # in the workflow is to give the user a preferred address. + anne = self._user_manager.create_user(self._anne) + self.assertIsNone(anne.preferred_address) + workflow = SubscriptionWorkflow(self._mlist, anne, False, False, False) + next(workflow) + + def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber # becomes a member with no human intervention. diff --git a/src/mailman/app/tests/test_workflow.py b/src/mailman/app/tests/test_workflow.py index 6bae46e8a..0f70042af 100644 --- a/src/mailman/app/tests/test_workflow.py +++ b/src/mailman/app/tests/test_workflow.py @@ -31,10 +31,10 @@ from mailman.testing.layers import ConfigLayer class MyWorkflow(Workflow): INITIAL_STATE = 'first' SAVE_ATTRIBUTES = ('ant', 'bee', 'cat') - SAVE_KEY = 'test-workflow' def __init__(self): super().__init__() + self.token = 'test-workflow' self.ant = 1 self.bee = 2 self.cat = 3 @@ -116,3 +116,11 @@ class TestWorkflow(unittest.TestCase): self.assertEqual(new_workflow.bee, 8) self.assertEqual(new_workflow.cat, 7) self.assertEqual(new_workflow.dog, 4) + + def test_run_thru(self): + # Run all steps through the given one. + results = self._workflow.run_thru(second) + self.assertEqual(results, ['one', 'two']) + + def test_run_until(self): + # Run until (but not including diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index bba5bb694..f9a951157 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -38,11 +38,11 @@ log = logging.getLogger('mailman.error') class Workflow: """Generic workflow.""" - SAVE_KEY = '' SAVE_ATTRIBUTES = () INITIAL_STATE = None def __init__(self): + self.token = None self._next = deque() self.push(self.INITIAL_STATE) @@ -68,7 +68,7 @@ class Workflow: raise def save(self): - assert self.SAVE_KEY, 'Workflow SAVE_KEY must be set' + assert self.token, 'Workflow token must be set' state_manager = getUtility(IWorkflowStateManager) data = {attr: getattr(self, attr) for attr in self.SAVE_ATTRIBUTES} # Note: only the next step is saved, not the whole stack. This is not @@ -86,13 +86,13 @@ class Workflow: "in the queue") state_manager.save( self.__class__.__name__, - self.SAVE_KEY, + self.token, step, json.dumps(data)) def restore(self): state_manager = getUtility(IWorkflowStateManager) - state = state_manager.restore(self.__class__.__name__, self.SAVE_KEY) + state = state_manager.restore(self.__class__.__name__, self.token) if state is not None: self._next.clear() if state.step: -- cgit v1.2.3-70-g09d2 From 3a0c1191d8c12a8f4d4e5acb1f167331dfabc7d8 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 7 Apr 2015 15:32:30 -0400 Subject: Add run_thru() and run_until() to workflow, mostly for testing purposes. --- src/mailman/app/tests/test_workflow.py | 6 +++-- src/mailman/app/workflow.py | 42 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/mailman/app/tests/test_workflow.py b/src/mailman/app/tests/test_workflow.py index 0f70042af..51beceb86 100644 --- a/src/mailman/app/tests/test_workflow.py +++ b/src/mailman/app/tests/test_workflow.py @@ -119,8 +119,10 @@ class TestWorkflow(unittest.TestCase): def test_run_thru(self): # Run all steps through the given one. - results = self._workflow.run_thru(second) + results = self._workflow.run_thru('second') self.assertEqual(results, ['one', 'two']) def test_run_until(self): - # Run until (but not including + # Run until (but not including) the given step. + results = self._workflow.run_until('second') + self.assertEqual(results, ['one']) diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index f9a951157..8275addc0 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -67,6 +67,48 @@ class Workflow: log.exception('deque: {}'.format(COMMASPACE.join(self._next))) raise + def run_thru(self, stop_after): + """Run the state machine through and including the given step. + + :param stop_after: Name of method, sans prefix to run the + state machine through. In other words, the state machine runs + until the named method completes. + """ + results = [] + while True: + try: + name, step = self._pop() + except (StopIteration, IndexError): + # We're done. + break + results.append(step()) + if name == stop_after: + break + return results + + def run_until(self, stop_before): + """Trun the state machine until (not including) the given step. + + :param stop_before: Name of method, sans prefix that the + state machine is run until the method is reached. Unlike + `run_thru()` the named method is not run. + """ + results = [] + while True: + try: + name, step = self._pop() + except (StopIteration, IndexError): + # We're done. + break + if name == stop_before: + # Stop executing, but not before we push the last state back + # onto the deque. Otherwise, resuming the state machine would + # skip this step. + self._next.appendleft(step) + break + results.append(step()) + return results + def save(self): assert self.token, 'Workflow token must be set' state_manager = getUtility(IWorkflowStateManager) -- cgit v1.2.3-70-g09d2 From 1783ef4646dfe8f2a398799fe6c712e619579dc9 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 7 Apr 2015 15:37:41 -0400 Subject: Rebase Alembic revisions. --- .../database/alembic/versions/16c2b25c7b_list_subscription_policy.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py b/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py index d5f8c3d96..8534f4b73 100644 --- a/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py +++ b/src/mailman/database/alembic/versions/16c2b25c7b_list_subscription_policy.py @@ -1,14 +1,14 @@ """List subscription policy Revision ID: 16c2b25c7b -Revises: 33e1f5f6fa8 +Revises: 46e92facee7 Create Date: 2015-03-21 11:00:44.634883 """ # revision identifiers, used by Alembic. revision = '16c2b25c7b' -down_revision = '33e1f5f6fa8' +down_revision = '46e92facee7' from alembic import op import sqlalchemy as sa -- cgit v1.2.3-70-g09d2 From 8412b68294435320ec2a55ac5114c34e410e4e71 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 9 Apr 2015 22:44:42 -0400 Subject: Subscription workflow checkpointing. * TO DO: - hook up sending of confirmation - processing confirmations and continuing workflow - get tokens for saving workflows - integrate with RequestRecord - integrate with hold_subscription - after getting moderator approval, continue workflow --- src/mailman/app/subscriptions.py | 119 ++++++------- src/mailman/app/tests/test_subscriptions.py | 255 ++++++++++++++++++++++++++-- src/mailman/app/workflow.py | 6 + src/mailman/model/roster.py | 12 +- src/mailman/model/usermanager.py | 1 - 5 files changed, 318 insertions(+), 75 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index ebb4198bb..2deec131b 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -62,15 +62,15 @@ def _membership_sort_key(member): class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" - INITIAL_STATE = 'verification_check' + INITIAL_STATE = 'sanity_checks' SAVE_ATTRIBUTES = ( 'pre_approved', 'pre_confirmed', 'pre_verified', ) - def __init__(self, mlist, subscriber, - pre_verified, pre_confirmed, pre_approved): + def __init__(self, mlist, subscriber, *, + pre_verified=False, pre_confirmed=False, pre_approved=False): super().__init__() self.mlist = mlist # The subscriber must be either an IUser or IAddress. @@ -87,71 +87,73 @@ class SubscriptionWorkflow(Workflow): self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved - def _maybe_set_preferred_address(self): + def _step_sanity_checks(self): + # Ensure that we have both an address and a user, even if the address + # is not verified. We can't set the preferred address until it is + # verified. if self.user is None: # The address has no linked user so create one, link it, and set # the user's preferred address. assert self.address is not None, 'No address or user' self.user = getUtility(IUserManager).make_user(self.address.email) - self.user.preferred_address = self.address - elif self.user.preferred_address is None: - assert self.address is not None, 'No address or user' - # The address has a linked user, but no preferred address is set - # yet. This is required, so use the address. - self.user.preferred_address = self.address + if self.address is None: + assert self.user.preferred_address is None, ( + "Preferred address exists, but wasn't used in constructor") + addresses = list(self.user.addresses) + if len(addresses) == 0: + raise AssertionError('User has no addresses: {}'.format( + self.user)) + # This is rather arbitrary, but we have no choice. + self.address = addresses[0] + assert self.user is not None and self.address is not None, ( + 'Insane sanity check results') + self.push('verification_checks') - def _step_verification_check(self): - if self.address.verified_on is not None: - # The address is already verified. Give the user a preferred - # address if it doesn't already have one. We may still have to do - # a subscription confirmation check. See below. - self._maybe_set_preferred_address() - else: - # The address is not yet verified. Maybe we're pre-verifying it. - # If so, we also want to give the user a preferred address if it - # doesn't already have one. We may still have to do a - # subscription confirmation check. See below. + def _step_verification_checks(self): + # Is the address already verified, or is the pre-verified flag set? + if self.address.verified_on is None: if self.pre_verified: self.address.verified_on = now() - self._maybe_set_preferred_address() else: - # Since the address was not already verified, and not - # pre-verified, we have to send a confirmation check, which - # doubles as a verification step. Skip to that now. - self._next.append('send_confirmation') + # The address being subscribed is not yet verified, so we need + # to send a validation email that will also confirm that the + # user wants to be subscribed to this mailing list. + self.push('send_confirmation') return - self._next.append('confirmation_check') - - def _step_confirmation_check(self): - # Must the user confirm their subscription request? If the policy is - # open subscriptions, then we need neither confirmation nor moderator - # approval, so just subscribe them now. - if self.mlist.subscription_policy == SubscriptionPolicy.open: - self._next.append('do_subscription') - elif self.pre_confirmed: - # No confirmation is necessary. We can skip to seeing whether a - # moderator confirmation is necessary. - self._next.append('moderation_check') - else: - self._next.append('send_confirmation') + self.push('confirmation_checks') - def _step_send_confirmation(self): - self._next.append('moderation_check') - self.save() - self._next.clear() # stop iteration until we get confirmation - # XXX: create the Pendable, send the ConfirmationNeededEvent - # (see Registrar.register) + def _step_confirmation_checks(self): + # If the list's subscription policy is open, then the user can be + # subscribed right here and now. + if self.mlist.subscription_policy is SubscriptionPolicy.open: + self.push('do_subscription') + return + # If we do not need the user's confirmation, then skip to the + # moderation checks. + if self.mlist.subscription_policy is SubscriptionPolicy.moderate: + self.push('moderation_checks') + return + # If the subscription has been pre-confirmed, then we can skip to the + # moderation checks. + if self.pre_confirmed: + self.push('moderation_checks') + return + # The user must confirm their subscription. + self.push('send_confirmation') - def _step_moderation_check(self): + def _step_moderation_checks(self): # Does the moderator need to approve the subscription request? - if not self.pre_approved and self.mlist.subscription_policy in ( - SubscriptionPolicy.moderate, - SubscriptionPolicy.confirm_then_moderate): - self._next.append('get_moderator_approval') + assert self.mlist.subscription_policy in ( + SubscriptionPolicy.moderate, + SubscriptionPolicy.confirm_then_moderate) + if self.pre_approved: + self.push('do_subscription') else: - # The moderator does not need to approve the subscription, so go - # ahead and do that now. - self._next.append('do_subscription') + self.push('get_moderator_approval') + + def _step_do_subscription(self): + # We can immediately subscribe the user to the mailing list. + self.mlist.subscribe(self.subscriber) def _step_get_moderator_approval(self): # In order to get the moderator's approval, we need to hold the @@ -161,9 +163,12 @@ class SubscriptionWorkflow(Workflow): DeliveryMode.regular, 'en') hold_subscription(self.mlist, request) - def _step_do_subscription(self): - # We can immediately subscribe the user to the mailing list. - self.mlist.subscribe(self.subscriber) + def _step_send_confirmation(self): + self._next.append('moderation_check') + self.save() + self._next.clear() # stop iteration until we get confirmation + # XXX: create the Pendable, send the ConfirmationNeededEvent + # (see Registrar.register) @implementer(ISubscriptionService) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index d320284ce..836e7f7b7 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -37,6 +37,7 @@ from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager from mailman.utilities.datetime import now +from unittest.mock import patch from zope.component import getUtility @@ -84,18 +85,247 @@ class TestSubscriptionWorkflow(unittest.TestCase): def test_user_or_address_required(self): # The `subscriber` attribute must be a user or address. - self.assertRaises(AssertionError, SubscriptionWorkflow, - self._mlist, - 'not a user', False, False, False) - - def test_user_without_preferred_address_gets_one(self): - # When subscribing a user without a preferred address, the first step - # in the workflow is to give the user a preferred address. - anne = self._user_manager.create_user(self._anne) - self.assertIsNone(anne.preferred_address) - workflow = SubscriptionWorkflow(self._mlist, anne, False, False, False) - next(workflow) - + self.assertRaises( + AssertionError, SubscriptionWorkflow, self._mlist, 'not a user') + + def test_sanity_checks_address(self): + # Ensure that the sanity check phase, when given an IAddress, ends up + # with a linked user. + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + self.assertIsNotNone(workflow.address) + self.assertIsNone(workflow.user) + workflow.run_thru('sanity_checks') + self.assertIsNotNone(workflow.address) + self.assertIsNotNone(workflow.user) + self.assertEqual(list(workflow.user.addresses)[0].email, self._anne) + + def test_sanity_checks_user_with_preferred_address(self): + # Ensure that the sanity check phase, when given an IUser with a + # preferred address, ends up with an address. + anne = self._user_manager.make_user(self._anne) + address = list(anne.addresses)[0] + address.verified_on = now() + anne.preferred_address = address + workflow = SubscriptionWorkflow(self._mlist, anne) + # The constructor sets workflow.address because the user has a + # preferred address. + self.assertEqual(workflow.address, address) + self.assertEqual(workflow.user, anne) + workflow.run_thru('sanity_checks') + self.assertEqual(workflow.address, address) + self.assertEqual(workflow.user, anne) + + def test_sanity_checks_user_without_preferred_address(self): + # Ensure that the sanity check phase, when given a user without a + # preferred address, but with at least one linked address, gets an + # address. + anne = self._user_manager.make_user(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + self.assertIsNone(workflow.address) + self.assertEqual(workflow.user, anne) + workflow.run_thru('sanity_checks') + self.assertIsNotNone(workflow.address) + self.assertEqual(workflow.user, anne) + + def test_sanity_checks_user_with_multiple_linked_addresses(self): + # Ensure that the santiy check phase, when given a user without a + # preferred address, but with multiple linked addresses, gets of of + # those addresses (exactly which one is undefined). + anne = self._user_manager.make_user(self._anne) + anne.link(self._user_manager.create_address('anne@example.net')) + anne.link(self._user_manager.create_address('anne@example.org')) + workflow = SubscriptionWorkflow(self._mlist, anne) + self.assertIsNone(workflow.address) + self.assertEqual(workflow.user, anne) + workflow.run_thru('sanity_checks') + self.assertIn(workflow.address.email, ['anne@example.com', + 'anne@example.net', + 'anne@example.org']) + self.assertEqual(workflow.user, anne) + + def test_sanity_checks_user_without_addresses(self): + # It is an error to try to subscribe a user with no linked addresses. + user = self._user_manager.create_user() + workflow = SubscriptionWorkflow(self._mlist, user) + self.assertRaises(AssertionError, workflow.run_thru, 'sanity_checks') + + def test_verification_checks_with_verified_address(self): + # When the address is already verified, we skip straight to the + # confirmation checks. + anne = self._user_manager.create_address(self._anne) + anne.verified_on = now() + workflow = SubscriptionWorkflow(self._mlist, anne) + workflow.run_thru('verification_checks') + with patch.object(workflow, '_step_confirmation_checks') as step: + next(workflow) + step.assert_called_once_with() + + def test_verification_checks_with_pre_verified_address(self): + # When the address is not yet verified, but the pre-verified flag is + # passed to the workflow, we skip to the confirmation checks. + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('verification_checks') + with patch.object(workflow, '_step_confirmation_checks') as step: + next(workflow) + step.assert_called_once_with() + # And now the address is verified. + self.assertIsNotNone(anne.verified_on) + + def test_verification_checks_confirmation_needed(self): + # The address is neither verified, nor is the pre-verified flag set. + # A confirmation message must be sent to the user which will also + # verify their address. + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + workflow.run_thru('verification_checks') + with patch.object(workflow, '_step_send_confirmation') as step: + next(workflow) + step.assert_called_once_with() + # The address still hasn't been verified. + self.assertIsNone(anne.verified_on) + + def test_confirmation_checks_open_list(self): + # A subscription to an open list does not need to be confirmed or + # moderated. + self._mlist.subscription_policy = SubscriptionPolicy.open + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_do_subscription') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_no_user_confirmation_needed(self): + # A subscription to a list which does not need user confirmation skips + # to the moderation checks. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_moderation_checks') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_confirm_pre_confirmed(self): + # The subscription policy requires user confirmation, but their + # subscription is pre-confirmed. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_moderation_checks') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_confirm_and_moderate_pre_confirmed(self): + # The subscription policy requires user confirmation and moderation, + # but their subscription is pre-confirmed. + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_moderation_checks') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_confirmation_needed(self): + # The subscription policy requires confirmation and the subscription + # is not pre-confirmed. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_send_confirmation') as step: + next(workflow) + step.assert_called_once_with() + + def test_confirmation_checks_moderate_confirmation_needed(self): + # The subscription policy requires confirmation and moderation, and the + # subscription is not pre-confirmed. + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('confirmation_checks') + with patch.object(workflow, '_step_send_confirmation') as step: + next(workflow) + step.assert_called_once_with() + + def test_moderation_checks_pre_approved(self): + # The subscription is pre-approved by the moderator. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_approved=True) + workflow.run_thru('moderation_checks') + with patch.object(workflow, '_step_do_subscription') as step: + next(workflow) + step.assert_called_once_with() + + def test_moderation_checks_approval_required(self): + # The moderator must approve the subscription. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + workflow.run_thru('moderation_checks') + with patch.object(workflow, '_step_get_moderator_approval') as step: + next(workflow) + step.assert_called_once_with() + + def test_do_subscription(self): + # An open subscription policy plus a pre-verified address means the + # user gets subscribed to the mailing list without any further + # confirmations or approvals. + self._mlist.subscription_policy = SubscriptionPolicy.open + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) + # Consume the entire state machine. + list(workflow) + # Anne is now a member of the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + + def test_do_subscription_pre_approved(self): + # An moderation-requiring subscription policy plus a pre-verified and + # pre-approved address means the user gets subscribed to the mailing + # list without any further confirmations or approvals. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_approved=True) + # Consume the entire state machine. + list(workflow) + # Anne is now a member of the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + + def test_do_subscription_pre_approved_pre_confirmed(self): + # An moderation-requiring subscription policy plus a pre-verified and + # pre-approved address means the user gets subscribed to the mailing + # list without any further confirmations or approvals. + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True, + pre_approved=True) + # Consume the entire state machine. + list(workflow) + # Anne is now a member of the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + + # XXX def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber @@ -142,6 +372,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): SubscriptionPolicy.confirm_then_moderate _do_check() + @unittest.expectedFailure def test_confirmation_required(self): # Tests subscriptions where user confirmation is required self._mlist.subscription_policy = \ diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index 8275addc0..9395dc7b7 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -22,6 +22,7 @@ __all__ = [ ] +import sys import json import logging @@ -45,6 +46,8 @@ class Workflow: self.token = None self._next = deque() self.push(self.INITIAL_STATE) + self.debug = False + self._count = 0 def __iter__(self): return self @@ -55,6 +58,9 @@ class Workflow: def _pop(self): name = self._next.popleft() step = getattr(self, '_step_{}'.format(name)) + self._count += 1 + if self.debug: + print('[{:02d}] -> {}'.format(self._count, name), file=sys.stderr) return name, step def __next__(self): diff --git a/src/mailman/model/roster.py b/src/mailman/model/roster.py index ef24d896b..e386ec3ad 100644 --- a/src/mailman/model/roster.py +++ b/src/mailman/model/roster.py @@ -97,10 +97,10 @@ class AbstractRoster: yield member.address @dbconnection - def get_member(self, store, address): + def get_member(self, store, email): """See `IRoster`.""" results = self._query().filter( - Address.email == address, + Address.email == email, Member.address_id == Address.id) if results.count() == 0: return None @@ -158,13 +158,13 @@ class AdministratorRoster(AbstractRoster): Member.role == MemberRole.moderator)) @dbconnection - def get_member(self, store, address): + def get_member(self, store, email): """See `IRoster`.""" results = store.query(Member).filter( Member.list_id == self._mlist.list_id, or_(Member.role == MemberRole.moderator, Member.role == MemberRole.owner), - Address.email == address, + Address.email == email, Member.address_id == Address.id) if results.count() == 0: return None @@ -179,6 +179,8 @@ class AdministratorRoster(AbstractRoster): class DeliveryMemberRoster(AbstractRoster): """Return all the members having a particular kind of delivery.""" + role = MemberRole.member + @property def member_count(self): """See `IRoster`.""" @@ -283,7 +285,7 @@ class Memberships: yield address @dbconnection - def get_member(self, store, address): + def get_member(self, store, email): """See `IRoster`.""" results = store.query(Member).filter( Member.address_id == Address.id, diff --git a/src/mailman/model/usermanager.py b/src/mailman/model/usermanager.py index ae499452b..3d7777099 100644 --- a/src/mailman/model/usermanager.py +++ b/src/mailman/model/usermanager.py @@ -64,7 +64,6 @@ class UserManager: user.display_name = ( display_name if display_name else address.display_name) user.link(address) - return user return user @dbconnection -- cgit v1.2.3-70-g09d2 From f7bfdc4f04f8a8c709695cb37a4cdfa08e670a5a Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Thu, 9 Apr 2015 22:46:10 -0400 Subject: to do --- TODO.rst | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 TODO.rst diff --git a/TODO.rst b/TODO.rst new file mode 100644 index 000000000..1adf03944 --- /dev/null +++ b/TODO.rst @@ -0,0 +1,7 @@ +* TO DO: + - hook up sending of confirmation + - processing confirmations and continuing workflow + - get tokens for saving workflows + - integrate with RequestRecord + - integrate with hold_subscription + - after getting moderator approval, continue workflow -- cgit v1.2.3-70-g09d2 From 9806f9c751f85b6ad6e9dea58d9e3dde36c7f2bc Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 10:46:37 -0400 Subject: Handle save/restore of the subscription workflow. To handle the case of restoring a saved workflow, allow the subscriber to be None in the constructor, but assert that it is not None after the first workflow step. Add setters/getters for handling save/restore. In the base workflow interface, rename key to token. --- TODO.rst | 1 + src/mailman/app/subscriptions.py | 72 +++++++++++++++++++++++++++-- src/mailman/app/tests/test_subscriptions.py | 31 ++++++++++++- src/mailman/interfaces/workflow.py | 14 +++--- src/mailman/model/tests/test_usermanager.py | 5 ++ src/mailman/model/tests/test_workflow.py | 54 +++++++++++----------- src/mailman/model/workflow.py | 12 ++--- 7 files changed, 143 insertions(+), 46 deletions(-) diff --git a/TODO.rst b/TODO.rst index 1adf03944..4a467ca96 100644 --- a/TODO.rst +++ b/TODO.rst @@ -1,4 +1,5 @@ * TO DO: + - add full RequestRecord to SubscriptionWorkflow ctor - hook up sending of confirmation - processing confirmations and continuing workflow - get tokens for saving workflows diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 2deec131b..22f4bdf56 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -25,6 +25,9 @@ __all__ = [ +import uuid + +from enum import Enum from mailman.app.membership import add_member, delete_member from mailman.app.moderator import hold_subscription from mailman.app.workflow import Workflow @@ -58,6 +61,11 @@ def _membership_sort_key(member): return (member.list_id, member.address.email, member.role.value) +class WhichSubscriber(Enum): + address = 1 + user = 2 + + class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" @@ -67,26 +75,63 @@ class SubscriptionWorkflow(Workflow): 'pre_approved', 'pre_confirmed', 'pre_verified', + 'address_key', + 'subscriber_key', + 'user_key', ) - def __init__(self, mlist, subscriber, *, + def __init__(self, mlist, subscriber=None, *, pre_verified=False, pre_confirmed=False, pre_approved=False): super().__init__() self.mlist = mlist + self.address = None + self.user = None + self.which = None # The subscriber must be either an IUser or IAddress. if IAddress.providedBy(subscriber): self.address = subscriber self.user = self.address.user + self.which = WhichSubscriber.address elif IUser.providedBy(subscriber): self.address = subscriber.preferred_address self.user = subscriber - else: - raise AssertionError('subscriber is neither an IUser nor IAddress') + self.which = WhichSubscriber.user self.subscriber = subscriber self.pre_verified = pre_verified self.pre_confirmed = pre_confirmed self.pre_approved = pre_approved + @property + def user_key(self): + # For save. + return self.user.user_id.hex + + @user_key.setter + def user_key(self, hex_key): + # For restore. + uid = uuid.UUID(hex_key) + self.user = getUtility(IUserManager).get_user_by_id(uid) + assert self.user is not None + + @property + def address_key(self): + # For save. + return self.address.email + + @address_key.setter + def address_key(self, email): + # For restore. + self.address = getUtility(IUserManager).get_address(email) + assert self.address is not None + + @property + def subscriber_key(self): + return self.which.value + + @subscriber_key.setter + def subscriber_key(self, key): + self.which = WhichSubscriber(key) + def _step_sanity_checks(self): # Ensure that we have both an address and a user, even if the address # is not verified. We can't set the preferred address until it is @@ -160,8 +205,27 @@ class SubscriptionWorkflow(Workflow): # subscription request in the database request = RequestRecord( self.address.email, self.subscriber.display_name, + # XXX Need to get these last to into the constructor. DeliveryMode.regular, 'en') - hold_subscription(self.mlist, request) + self.token = hold_subscription(self.mlist, request) + # Here's the next step in the workflow, assuming the moderator + # approves of the subscription. If they don't, the workflow and + # subscription request will just be thrown away. + self.push('subscribe_from_restored') + self.save() + # The workflow must stop running here. + raise StopIteration + + def _step_subscribe_from_restored(self): + # Restore a little extra state that can't be stored in the database + # (because the order of setattr() on restore is indeterminate), then + # subscribe the user. + if self.which is WhichSubscriber.address: + self.subscriber = self.address + else: + assert self.which is WhichSubscriber.user + self.subscriber = self.user + self.push('do_subscription') def _step_send_confirmation(self): self._next.append('moderation_check') diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 836e7f7b7..61d341d6d 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -85,8 +85,8 @@ class TestSubscriptionWorkflow(unittest.TestCase): def test_user_or_address_required(self): # The `subscriber` attribute must be a user or address. - self.assertRaises( - AssertionError, SubscriptionWorkflow, self._mlist, 'not a user') + workflow = SubscriptionWorkflow(self._mlist) + self.assertRaises(AssertionError, list, workflow) def test_sanity_checks_address(self): # Ensure that the sanity check phase, when given an IAddress, ends up @@ -325,8 +325,34 @@ class TestSubscriptionWorkflow(unittest.TestCase): member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + def test_moderator_approves(self): + # The workflow runs until moderator approval is required, at which + # point the workflow is saved. Once the moderator approves, the + # workflow resumes and the user is subscribed. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + # Consume the entire state machine. + list(workflow) + # The user is not currently subscribed to the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertIsNone(member) + # Create a new workflow with the previous workflow's save token, and + # restore its state. This models an approved subscription and should + # result in the user getting subscribed. + approved_workflow = SubscriptionWorkflow(self._mlist) + approved_workflow.token = workflow.token + approved_workflow.restore() + list(approved_workflow) + # Now the user is subscribed to the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + # XXX + @unittest.expectedFailure def test_preverified_address_joins_open_list(self): # The mailing list has an open subscription policy, so the subscriber # becomes a member with no human intervention. @@ -346,6 +372,7 @@ class TestSubscriptionWorkflow(unittest.TestCase): self.assertIsNotNone(anne.user) self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) + @unittest.expectedFailure def test_verified_address_joins_moderated_list(self): # The mailing list is moderated but the subscriber is not a verified # address and the subscription request is not pre-verified. diff --git a/src/mailman/interfaces/workflow.py b/src/mailman/interfaces/workflow.py index 49fbc85d4..4a4fb4412 100644 --- a/src/mailman/interfaces/workflow.py +++ b/src/mailman/interfaces/workflow.py @@ -32,7 +32,7 @@ class IWorkflowState(Interface): name = Attribute('The name of the workflow.') - key = Attribute('A unique key identifying the workflow instance.') + token = Attribute('A unique key identifying the workflow instance.') step = Attribute("This workflow's next step.") @@ -43,24 +43,24 @@ class IWorkflowState(Interface): class IWorkflowStateManager(Interface): """The workflow states manager.""" - def save(name, key, step, data=None): + def save(name, token, step, data=None): """Save the state of a workflow. :param name: The name of the workflow. :type name: str - :param key: A unique key identifying this workflow instance. - :type key: str + :param token: A unique token identifying this workflow instance. + :type token: str :param step: The next step for this workflow. :type step: str :param data: Additional data (workflow-specific). :type data: str """ - def restore(name, key): + def restore(name, token): """Get the saved state for a workflow or None if nothing was saved. :param name: The name of the workflow. :type name: str - :param key: A unique key identifying this workflow instance. - :type key: str + :param token: A unique token identifying this workflow instance. + :type token: str """ diff --git a/src/mailman/model/tests/test_usermanager.py b/src/mailman/model/tests/test_usermanager.py index 31f1a7275..e441ed713 100644 --- a/src/mailman/model/tests/test_usermanager.py +++ b/src/mailman/model/tests/test_usermanager.py @@ -80,3 +80,8 @@ class TestUserManager(unittest.TestCase): user = self._usermanager.create_user('anne@example.com', 'Anne Person') other_user = self._usermanager.make_user('anne@example.com') self.assertIs(user, other_user) + + def test_get_user_by_id(self): + original = self._usermanager.make_user('anne@example.com') + copy = self._usermanager.get_user_by_id(original.user_id) + self.assertEqual(original, copy) diff --git a/src/mailman/model/tests/test_workflow.py b/src/mailman/model/tests/test_workflow.py index b5e915df4..6081e5b57 100644 --- a/src/mailman/model/tests/test_workflow.py +++ b/src/mailman/model/tests/test_workflow.py @@ -39,72 +39,72 @@ class TestWorkflow(unittest.TestCase): def test_save_restore_workflow(self): # Save and restore a workflow. name = 'ant' - key = 'bee' + token = 'bee' step = 'cat' data = 'dog' - self._manager.save(name, key, step, data) - workflow = self._manager.restore(name, key) + self._manager.save(name, token, step, data) + workflow = self._manager.restore(name, token) self.assertEqual(workflow.name, name) - self.assertEqual(workflow.key, key) + self.assertEqual(workflow.token, token) self.assertEqual(workflow.step, step) self.assertEqual(workflow.data, data) def test_save_restore_workflow_without_step(self): # Save and restore a workflow that contains no step. name = 'ant' - key = 'bee' + token = 'bee' data = 'dog' - self._manager.save(name, key, data=data) - workflow = self._manager.restore(name, key) + self._manager.save(name, token, data=data) + workflow = self._manager.restore(name, token) self.assertEqual(workflow.name, name) - self.assertEqual(workflow.key, key) + self.assertEqual(workflow.token, token) self.assertIsNone(workflow.step) self.assertEqual(workflow.data, data) def test_save_restore_workflow_without_data(self): # Save and restore a workflow that contains no data. name = 'ant' - key = 'bee' + token = 'bee' step = 'cat' - self._manager.save(name, key, step) - workflow = self._manager.restore(name, key) + self._manager.save(name, token, step) + workflow = self._manager.restore(name, token) self.assertEqual(workflow.name, name) - self.assertEqual(workflow.key, key) + self.assertEqual(workflow.token, token) self.assertEqual(workflow.step, step) self.assertIsNone(workflow.data) def test_save_restore_workflow_without_step_or_data(self): # Save and restore a workflow that contains no step or data. name = 'ant' - key = 'bee' - self._manager.save(name, key) - workflow = self._manager.restore(name, key) + token = 'bee' + self._manager.save(name, token) + workflow = self._manager.restore(name, token) self.assertEqual(workflow.name, name) - self.assertEqual(workflow.key, key) + self.assertEqual(workflow.token, token) self.assertIsNone(workflow.step) self.assertIsNone(workflow.data) def test_restore_workflow_with_no_matching_name(self): # Try to restore a workflow that has no matching name in the database. name = 'ant' - key = 'bee' - self._manager.save(name, key) - workflow = self._manager.restore('ewe', key) + token = 'bee' + self._manager.save(name, token) + workflow = self._manager.restore('ewe', token) self.assertIsNone(workflow) - def test_restore_workflow_with_no_matching_key(self): - # Try to restore a workflow that has no matching key in the database. + def test_restore_workflow_with_no_matching_token(self): + # Try to restore a workflow that has no matching token in the database. name = 'ant' - key = 'bee' - self._manager.save(name, key) + token = 'bee' + self._manager.save(name, token) workflow = self._manager.restore(name, 'fly') self.assertIsNone(workflow) - def test_restore_workflow_with_no_matching_key_or_name(self): - # Try to restore a workflow that has no matching key or name in the + def test_restore_workflow_with_no_matching_token_or_name(self): + # Try to restore a workflow that has no matching token or name in the # database. name = 'ant' - key = 'bee' - self._manager.save(name, key) + token = 'bee' + self._manager.save(name, token) workflow = self._manager.restore('ewe', 'fly') self.assertIsNone(workflow) diff --git a/src/mailman/model/workflow.py b/src/mailman/model/workflow.py index 53c9f05ea..d9f23f53b 100644 --- a/src/mailman/model/workflow.py +++ b/src/mailman/model/workflow.py @@ -38,7 +38,7 @@ class WorkflowState(Model): __tablename__ = 'workflowstate' name = Column(Unicode, primary_key=True) - key = Column(Unicode, primary_key=True) + token = Column(Unicode, primary_key=True) step = Column(Unicode) data = Column(Unicode) @@ -49,17 +49,17 @@ class WorkflowStateManager: """See `IWorkflowStateManager`.""" @dbconnection - def save(self, store, name, key, step=None, data=None): + def save(self, store, name, token, step=None, data=None): """See `IWorkflowStateManager`.""" - state = store.query(WorkflowState).get((name, key)) + state = store.query(WorkflowState).get((name, token)) if state is None: - state = WorkflowState(name=name, key=key, step=step, data=data) + state = WorkflowState(name=name, token=token, step=step, data=data) store.add(state) else: state.step = step state.data = data @dbconnection - def restore(self, store, name, key): + def restore(self, store, name, token): """See `IWorkflowStateManager`.""" - return store.query(WorkflowState).get((name, key)) + return store.query(WorkflowState).get((name, token)) -- cgit v1.2.3-70-g09d2 From c595c3d907f111d80852837c686b23aea9ee40c1 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 12:16:28 -0400 Subject: Subsume hold_subscription() functionality into the subscription workflow. Specifically, log a message and send a notification to the list owners when a subscription is held and the list is so configured. --- TODO.rst | 10 ++--- src/mailman/app/subscriptions.py | 56 +++++++++++++++++++++++---- src/mailman/app/tests/test_subscriptions.py | 60 +++++++++++++++++++++++++++++ src/mailman/templates/en/subauth.txt | 6 --- 4 files changed, 111 insertions(+), 21 deletions(-) diff --git a/TODO.rst b/TODO.rst index 4a467ca96..2f97b538f 100644 --- a/TODO.rst +++ b/TODO.rst @@ -1,8 +1,4 @@ * TO DO: - - add full RequestRecord to SubscriptionWorkflow ctor - - hook up sending of confirmation - - processing confirmations and continuing workflow - - get tokens for saving workflows - - integrate with RequestRecord - - integrate with hold_subscription - - after getting moderator approval, continue workflow + - get rid of hold_subscription + - subsume handle_subscription + - workflow for unsubscription diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 22f4bdf56..982c04547 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -26,24 +26,30 @@ __all__ = [ import uuid +import logging +from email.utils import formataddr from enum import Enum +from datetime import timedelta from mailman.app.membership import add_member, delete_member -from mailman.app.moderator import hold_subscription from mailman.app.workflow import Workflow from mailman.core.constants import system_preferences +from mailman.core.i18n import _ from mailman.database.transaction import dbconnection +from mailman.email.message import UserNotification from mailman.interfaces.address import IAddress from mailman.interfaces.listmanager import ( IListManager, ListDeletingEvent, NoSuchListError) from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import DeliveryMode, MemberRole +from mailman.interfaces.pending import IPendable, IPendings from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager from mailman.model.member import Member from mailman.utilities.datetime import now +from mailman.utilities.i18n import make from operator import attrgetter from sqlalchemy import and_, or_ from uuid import UUID @@ -51,6 +57,9 @@ from zope.component import getUtility from zope.interface import implementer +log = logging.getLogger('mailman.subscribe') + + def _membership_sort_key(member): """Sort function for find_members(). @@ -66,6 +75,11 @@ class WhichSubscriber(Enum): user = 2 +@implementer(IPendable) +class Pendable(dict): + pass + + class SubscriptionWorkflow(Workflow): """Workflow of a subscription request.""" @@ -201,18 +215,44 @@ class SubscriptionWorkflow(Workflow): self.mlist.subscribe(self.subscriber) def _step_get_moderator_approval(self): - # In order to get the moderator's approval, we need to hold the - # subscription request in the database - request = RequestRecord( - self.address.email, self.subscriber.display_name, - # XXX Need to get these last to into the constructor. - DeliveryMode.regular, 'en') - self.token = hold_subscription(self.mlist, request) + # Getting the moderator's approval requires several steps. We'll need + # to suspend this workflow for an indeterminate amount of time while + # we wait for that approval. We need a unique token for this + # suspended workflow, so we'll create a minimal pending record. We + # also might need to send an email notification to the list + # moderators. + # + # Start by creating the pending record. This will give us a hash + # token we can use to uniquely name this workflow. It only needs to + # contain the current date, which we'll use to expire requests from + # the database, say if the moderator never approves the request. + pendable = Pendable(when=now().isoformat()) + self.token = getUtility(IPendings).add(pendable, timedelta(days=3650)) # Here's the next step in the workflow, assuming the moderator # approves of the subscription. If they don't, the workflow and # subscription request will just be thrown away. self.push('subscribe_from_restored') self.save() + log.info('{}: held subscription request from {}'.format( + self.mlist.fqdn_listname, self.address.email)) + # Possibly send a notification to the list moderators. + if self.mlist.admin_immed_notify: + subject = _( + 'New subscription request to $self.mlist.display_name ' + 'from $self.address.email') + username = formataddr( + (self.subscriber.display_name, self.address.email)) + text = make('subauth.txt', + mailing_list=self.mlist, + username=username, + listname=self.mlist.fqdn_listname, + ) + # This message should appear to come from the -owner so as + # to avoid any useless bounce processing. + msg = UserNotification( + self.mlist.owner_address, self.mlist.owner_address, + subject, text, self.mlist.preferred_language) + msg.send(self.mlist, tomoderators=True) # The workflow must stop running here. raise StopIteration diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 61d341d6d..45e17a9e5 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -33,6 +33,7 @@ from mailman.interfaces.member import MemberRole, MissingPreferredAddressError from mailman.interfaces.requests import IListRequests, RequestType from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) +from mailman.testing.helpers import LogFileMark, get_queue_messages from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager @@ -77,9 +78,11 @@ class TestJoin(unittest.TestCase): class TestSubscriptionWorkflow(unittest.TestCase): layer = ConfigLayer + maxDiff = None def setUp(self): self._mlist = create_list('test@example.com') + self._mlist.admin_immed_notify = False self._anne = 'anne@example.com' self._user_manager = getUtility(IUserManager) @@ -350,6 +353,63 @@ class TestSubscriptionWorkflow(unittest.TestCase): member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + def test_get_moderator_approval_log_on_hold(self): + # When the subscription is held for moderator approval, a message is + # logged. + mark = LogFileMark('mailman.subscribe') + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + # Consume the entire state machine. + list(workflow) + line = mark.readline() + self.assertEqual( + line[29:-1], + 'test@example.com: held subscription request from anne@example.com' + ) + + def test_get_moderator_approval_notifies_moderators(self): + # When the subscription is held for moderator approval, and the list + # is so configured, a notification is sent to the list moderators. + self._mlist.admin_immed_notify = True + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + # Consume the entire state machine. + list(workflow) + items = get_queue_messages('virgin') + self.assertEqual(len(items), 1) + message = items[0].msg + self.assertEqual(message['From'], 'test-owner@example.com') + self.assertEqual(message['To'], 'test-owner@example.com') + self.assertEqual( + message['Subject'], + 'New subscription request to Test from anne@example.com') + self.assertEqual(message.get_payload(), """\ +Your authorization is required for a mailing list subscription request +approval: + + For: anne@example.com + List: test@example.com""") + + def test_get_moderator_approval_no_notifications(self): + # When the subscription is held for moderator approval, and the list + # is so configured, a notification is sent to the list moderators. + self._mlist.admin_immed_notify = False + self._mlist.subscription_policy = SubscriptionPolicy.moderate + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True) + # Consume the entire state machine. + list(workflow) + items = get_queue_messages('virgin') + self.assertEqual(len(items), 0) + # XXX @unittest.expectedFailure diff --git a/src/mailman/templates/en/subauth.txt b/src/mailman/templates/en/subauth.txt index 1b13ebaeb..041be5e55 100644 --- a/src/mailman/templates/en/subauth.txt +++ b/src/mailman/templates/en/subauth.txt @@ -3,9 +3,3 @@ approval: For: $username List: $listname - -At your convenience, visit: - - $admindb_url - -to process the request. -- cgit v1.2.3-70-g09d2 From 044a780a7a62fabc4f97975dac2725e8df8cdebe Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 12:49:45 -0400 Subject: Fix regression. --- src/mailman/app/docs/moderator.rst | 1 - 1 file changed, 1 deletion(-) diff --git a/src/mailman/app/docs/moderator.rst b/src/mailman/app/docs/moderator.rst index 82d29074a..43dc7688f 100644 --- a/src/mailman/app/docs/moderator.rst +++ b/src/mailman/app/docs/moderator.rst @@ -424,7 +424,6 @@ There's now a message in the virgin queue, destined for the list owner. For: iris@example.org List: ant@example.com - ... Similarly, the administrator gets notifications on unsubscription requests. Jeff is a member of the mailing list, and chooses to unsubscribe. -- cgit v1.2.3-70-g09d2 From 6f43b64a27f7e38df80a59f9111b5dedfac0d70f Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 13:16:22 -0400 Subject: Subscription workflows now support bans. --- src/mailman/app/subscriptions.py | 7 ++++++- src/mailman/app/tests/test_subscriptions.py | 18 +++++++++++++++++- src/mailman/interfaces/member.py | 2 +- 3 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 982c04547..9df85c819 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -38,10 +38,12 @@ from mailman.core.i18n import _ from mailman.database.transaction import dbconnection from mailman.email.message import UserNotification from mailman.interfaces.address import IAddress +from mailman.interfaces.bans import IBanManager from mailman.interfaces.listmanager import ( IListManager, ListDeletingEvent, NoSuchListError) from mailman.interfaces.mailinglist import SubscriptionPolicy -from mailman.interfaces.member import DeliveryMode, MemberRole +from mailman.interfaces.member import ( + DeliveryMode, MemberRole, MembershipIsBannedError) from mailman.interfaces.pending import IPendable, IPendings from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) @@ -166,6 +168,9 @@ class SubscriptionWorkflow(Workflow): self.address = addresses[0] assert self.user is not None and self.address is not None, ( 'Insane sanity check results') + # Is this email address banned? + if IBanManager(self.mlist).is_banned(self.address.email): + raise MembershipIsBannedError(self.mlist, self.address.email) self.push('verification_checks') def _step_verification_checks(self): diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 45e17a9e5..31218d2d7 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -29,7 +29,9 @@ import unittest from mailman.app.lifecycle import create_list from mailman.app.subscriptions import SubscriptionWorkflow from mailman.interfaces.address import InvalidEmailAddressError -from mailman.interfaces.member import MemberRole, MissingPreferredAddressError +from mailman.interfaces.bans import IBanManager +from mailman.interfaces.member import ( + MemberRole, MembershipIsBannedError, MissingPreferredAddressError) from mailman.interfaces.requests import IListRequests, RequestType from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) @@ -153,6 +155,20 @@ class TestSubscriptionWorkflow(unittest.TestCase): workflow = SubscriptionWorkflow(self._mlist, user) self.assertRaises(AssertionError, workflow.run_thru, 'sanity_checks') + def test_sanity_checks_globally_banned_address(self): + # An exception is raised if the address is globally banned. + anne = self._user_manager.create_address(self._anne) + IBanManager(None).ban(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + self.assertRaises(MembershipIsBannedError, list, workflow) + + def test_sanity_checks_banned_address(self): + # An exception is raised if the address is banned by the mailing list. + anne = self._user_manager.create_address(self._anne) + IBanManager(self._mlist).ban(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne) + self.assertRaises(MembershipIsBannedError, list, workflow) + def test_verification_checks_with_verified_address(self): # When the address is already verified, we skip straight to the # confirmation checks. diff --git a/src/mailman/interfaces/member.py b/src/mailman/interfaces/member.py index c06cc95b1..4e1ef6d37 100644 --- a/src/mailman/interfaces/member.py +++ b/src/mailman/interfaces/member.py @@ -123,7 +123,7 @@ class MembershipIsBannedError(MembershipError): """The address is not allowed to subscribe to the mailing list.""" def __init__(self, mlist, address): - super(MembershipIsBannedError, self).__init__() + super().__init__() self._mlist = mlist self._address = address -- cgit v1.2.3-70-g09d2 From 2b9ef541cf80e5bcf413c2ba234856a7bc77baad Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 16:23:50 -0400 Subject: SubscriptionPolicy is largely done, modulo bugs, and the unknown-unknown of course. This commit contains test failures because the next step is to hook everything up. Changes here include: * confirm.txt removes the URL since we can't know that. * Changes to the ConfirmationNeededEvent * Create the Pendable early in the workflow. * Clean up test. * Update IRegistrar docstring. --- src/mailman/app/registrar.py | 17 ++-- src/mailman/app/subscriptions.py | 65 ++++++++----- src/mailman/app/tests/test_subscriptions.py | 141 ++++++++++++++++------------ src/mailman/interfaces/registrar.py | 19 +--- src/mailman/templates/en/confirm.txt | 4 +- 5 files changed, 137 insertions(+), 109 deletions(-) diff --git a/src/mailman/app/registrar.py b/src/mailman/app/registrar.py index 9272b95d7..db68432d3 100644 --- a/src/mailman/app/registrar.py +++ b/src/mailman/app/registrar.py @@ -156,18 +156,17 @@ def handle_ConfirmationNeededEvent(event): # the Subject header, or they can click on the URL in the body of the # message and confirm through the web. subject = 'confirm ' + event.token - mlist = getUtility(IListManager).get_by_list_id(event.pendable['list_id']) - confirm_address = mlist.confirm_address(event.token) + confirm_address = event.mlist.confirm_address(event.token) # For i18n interpolation. - confirm_url = mlist.domain.confirm_url(event.token) - email_address = event.pendable['email'] - domain_name = mlist.domain.mail_host - contact_address = mlist.owner_address + confirm_url = event.mlist.domain.confirm_url(event.token) + email_address = event.email + domain_name = event.mlist.domain.mail_host + contact_address = event.mlist.owner_address # Send a verification email to the address. template = getUtility(ITemplateLoader).get( 'mailman:///{0}/{1}/confirm.txt'.format( - mlist.fqdn_listname, - mlist.preferred_language.code)) + event.mlist.fqdn_listname, + event.mlist.preferred_language.code)) text = _(template) msg = UserNotification(email_address, confirm_address, subject, text) - msg.send(mlist) + msg.send(event.mlist) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 9df85c819..6c60a71a4 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -45,6 +45,7 @@ from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.member import ( DeliveryMode, MemberRole, MembershipIsBannedError) from mailman.interfaces.pending import IPendable, IPendings +from mailman.interfaces.registrar import ConfirmationNeededEvent from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.user import IUser @@ -56,6 +57,7 @@ from operator import attrgetter from sqlalchemy import and_, or_ from uuid import UUID from zope.component import getUtility +from zope.event import notify from zope.interface import implementer @@ -171,6 +173,14 @@ class SubscriptionWorkflow(Workflow): # Is this email address banned? if IBanManager(self.mlist).is_banned(self.address.email): raise MembershipIsBannedError(self.mlist, self.address.email) + # Create a pending record. This will give us the hash token we can use + # to uniquely name this workflow. + pendable = Pendable( + when=now().isoformat(), + list_id=self.mlist.list_id, + address=self.address.email, + ) + self.token = getUtility(IPendings).add(pendable, timedelta(days=3650)) self.push('verification_checks') def _step_verification_checks(self): @@ -215,24 +225,7 @@ class SubscriptionWorkflow(Workflow): else: self.push('get_moderator_approval') - def _step_do_subscription(self): - # We can immediately subscribe the user to the mailing list. - self.mlist.subscribe(self.subscriber) - def _step_get_moderator_approval(self): - # Getting the moderator's approval requires several steps. We'll need - # to suspend this workflow for an indeterminate amount of time while - # we wait for that approval. We need a unique token for this - # suspended workflow, so we'll create a minimal pending record. We - # also might need to send an email notification to the list - # moderators. - # - # Start by creating the pending record. This will give us a hash - # token we can use to uniquely name this workflow. It only needs to - # contain the current date, which we'll use to expire requests from - # the database, say if the moderator never approves the request. - pendable = Pendable(when=now().isoformat()) - self.token = getUtility(IPendings).add(pendable, timedelta(days=3650)) # Here's the next step in the workflow, assuming the moderator # approves of the subscription. If they don't, the workflow and # subscription request will just be thrown away. @@ -272,12 +265,42 @@ class SubscriptionWorkflow(Workflow): self.subscriber = self.user self.push('do_subscription') + def _step_do_subscription(self): + # We can immediately subscribe the user to the mailing list. + self.mlist.subscribe(self.subscriber) + def _step_send_confirmation(self): - self._next.append('moderation_check') + self.push('do_confirm_verify') self.save() - self._next.clear() # stop iteration until we get confirmation - # XXX: create the Pendable, send the ConfirmationNeededEvent - # (see Registrar.register) + # Triggering this event causes the confirmation message to be sent. + notify(ConfirmationNeededEvent( + self.mlist, self.token, self.address.email)) + # Now we wait for the confirmation. + raise StopIteration + + def _step_do_confirm_verify(self): + # Restore a little extra state that can't be stored in the database + # (because the order of setattr() on restore is indeterminate), then + # continue with the confirmation/verification step. + if self.which is WhichSubscriber.address: + self.subscriber = self.address + else: + assert self.which is WhichSubscriber.user + self.subscriber = self.user + # The user has confirmed their subscription request, and also verified + # their email address if necessary. This latter needs to be set on the + # IAddress, but there's nothing more to do about the confirmation step. + # We just continue along with the workflow. + if self.address.verified_on is None: + self.address.verified_on = now() + # The next step depends on the mailing list's subscription policy. + next_step = ('moderation_check' + if self.mlist.subscription_policy in ( + SubscriptionPolicy.moderate, + SubscriptionPolicy.confirm_then_moderate, + ) + else 'do_subscription') + self.push(next_step) @implementer(ISubscriptionService) diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index 31218d2d7..bdfa72e29 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -32,7 +32,6 @@ from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.bans import IBanManager from mailman.interfaces.member import ( MemberRole, MembershipIsBannedError, MissingPreferredAddressError) -from mailman.interfaces.requests import IListRequests, RequestType from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) from mailman.testing.helpers import LogFileMark, get_queue_messages @@ -426,70 +425,88 @@ approval: items = get_queue_messages('virgin') self.assertEqual(len(items), 0) - # XXX + def test_send_confirmation(self): + # A confirmation message gets sent when the address is not verified. + anne = self._user_manager.create_address(self._anne) + self.assertIsNone(anne.verified_on) + # Run the workflow to model the confirmation step. + workflow = SubscriptionWorkflow(self._mlist, anne) + list(workflow) + items = get_queue_messages('virgin') + self.assertEqual(len(items), 1) + message = items[0].msg + token = workflow.token + self.assertEqual(message['Subject'], 'confirm {}'.format(token)) + self.assertEqual( + message['From'], 'test-confirm+{}@example.com'.format(token)) - @unittest.expectedFailure - def test_preverified_address_joins_open_list(self): - # The mailing list has an open subscription policy, so the subscriber - # becomes a member with no human intervention. - self._mlist.subscription_policy = SubscriptionPolicy.open - anne = self._user_manager.create_address(self._anne, 'Anne Person') + def test_send_confirmation_pre_confirmed(self): + # A confirmation message gets sent when the address is not verified + # but the subscription is pre-confirmed. + anne = self._user_manager.create_address(self._anne) self.assertIsNone(anne.verified_on) - self.assertIsNone(anne.user) - self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=True, pre_confirmed=False, pre_approved=False) - # Run the state machine to the end. The result is that her address - # will be verified, linked to a user, and subscribed to the mailing - # list. + # Run the workflow to model the confirmation step. + workflow = SubscriptionWorkflow(self._mlist, anne, pre_confirmed=True) list(workflow) - self.assertIsNotNone(anne.verified_on) - self.assertIsNotNone(anne.user) - self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) - - @unittest.expectedFailure - def test_verified_address_joins_moderated_list(self): - # The mailing list is moderated but the subscriber is not a verified - # address and the subscription request is not pre-verified. - # A confirmation email must be sent, it will serve as the verification - # email too. - anne = self._user_manager.create_address(self._anne, 'Anne Person') - request_db = IListRequests(self._mlist) - def _do_check(): - anne.verified_on = now() - self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=False, pre_confirmed=True, pre_approved=False) - # Run the state machine to the end. - list(workflow) - # Look in the requests db - requests = list(request_db.of_type(RequestType.subscription)) - self.assertEqual(len(requests), 1) - self.assertEqual(requests[0].key, anne.email) - request_db.delete_request(requests[0].id) - self._mlist.subscription_policy = SubscriptionPolicy.moderate - _do_check() - self._mlist.subscription_policy = \ - SubscriptionPolicy.confirm_then_moderate - _do_check() + items = get_queue_messages('virgin') + self.assertEqual(len(items), 1) + message = items[0].msg + token = workflow.token + self.assertEqual( + message['Subject'], 'confirm {}'.format(workflow.token)) + self.assertEqual( + message['From'], 'test-confirm+{}@example.com'.format(token)) - @unittest.expectedFailure - def test_confirmation_required(self): - # Tests subscriptions where user confirmation is required - self._mlist.subscription_policy = \ - SubscriptionPolicy.confirm_then_moderate - anne = self._user_manager.create_address(self._anne, 'Anne Person') - self.assertIsNone(self._mlist.subscribers.get_member(self._anne)) - workflow = SubscriptionWorkflow( - self._mlist, anne, - pre_verified=True, pre_confirmed=False, pre_approved=True) - # Run the state machine to the end. + def test_send_confirmation_pre_verified(self): + # A confirmation message gets sent even when the address is verified + # when the subscription must be confirmed. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + anne = self._user_manager.create_address(self._anne) + self.assertIsNone(anne.verified_on) + # Run the workflow to model the confirmation step. + workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True) list(workflow) - # A confirmation request must be pending - # TODO: test it - # Now restore and re-run the state machine as if we got the confirmation - workflow.restore() + items = get_queue_messages('virgin') + self.assertEqual(len(items), 1) + message = items[0].msg + token = workflow.token + self.assertEqual( + message['Subject'], 'confirm {}'.format(workflow.token)) + self.assertEqual( + message['From'], 'test-confirm+{}@example.com'.format(token)) + + def test_do_confirm_verify_address(self): + # The address is not yet verified, nor are we pre-verifying. A + # confirmation message will be sent. When the user confirms their + # subscription request, the address will end up being verified. + anne = self._user_manager.create_address(self._anne) + self.assertIsNone(anne.verified_on) + # Run the workflow to model the confirmation step. + workflow = SubscriptionWorkflow(self._mlist, anne) list(workflow) - self.assertIsNotNone(self._mlist.subscribers.get_member(self._anne)) + # The address is still not verified. + self.assertIsNone(anne.verified_on) + confirm_workflow = SubscriptionWorkflow(self._mlist) + confirm_workflow.token = workflow.token + confirm_workflow.restore() + confirm_workflow.run_thru('do_confirm_verify') + # The address is now verified. + self.assertIsNotNone(anne.verified_on) + + def test_do_confirmation_subscribes_user(self): + # Subscriptions to the mailing list must be confirmed. Once that's + # done, the user's address (which is not initially verified) gets + # subscribed to the mailing list. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + anne = self._user_manager.create_address(self._anne) + self.assertIsNone(anne.verified_on) + workflow = SubscriptionWorkflow(self._mlist, anne) + list(workflow) + self.assertIsNone(self._mlist.regular_members.get_member(self._anne)) + confirm_workflow = SubscriptionWorkflow(self._mlist) + confirm_workflow.token = workflow.token + confirm_workflow.restore() + list(confirm_workflow) + self.assertIsNotNone(anne.verified_on) + self.assertEqual( + self._mlist.regular_members.get_member(self._anne).address, anne) diff --git a/src/mailman/interfaces/registrar.py b/src/mailman/interfaces/registrar.py index 7d3cf9c25..504442f7e 100644 --- a/src/mailman/interfaces/registrar.py +++ b/src/mailman/interfaces/registrar.py @@ -35,23 +35,14 @@ from zope.interface import Interface class ConfirmationNeededEvent: """Triggered when an address needs confirmation. - Addresses must be verified before they can receive messages or post to - mailing list. When an address is registered with Mailman, via the - `IRegistrar` interface, an `IPendable` is created which represents the - pending registration. This pending registration is stored in the - database, keyed by a token. Then this event is triggered. - - There may be several ways to confirm an email address. On some sites, - registration may immediately produce a verification, e.g. because it is on - a known intranet. Or verification may occur via external database lookup - (e.g. LDAP). On most public mailing lists, a mail-back confirmation is - sent to the address, and only if they reply to the mail-back, or click on - an embedded link, is the registered address confirmed. + Addresses must be verified before they can receive messages or post + to mailing list. The confirmation message is sent to the user when + this event is triggered. """ - def __init__(self, mlist, pendable, token): + def __init__(self, mlist, token, email): self.mlist = mlist - self.pendable = pendable self.token = token + self.email = email diff --git a/src/mailman/templates/en/confirm.txt b/src/mailman/templates/en/confirm.txt index d02cb462b..7c8bee75f 100644 --- a/src/mailman/templates/en/confirm.txt +++ b/src/mailman/templates/en/confirm.txt @@ -8,9 +8,7 @@ We have received a registration request for the email address Before you can start using GNU Mailman at this site, you must first confirm that this is your email address. You can do this by replying to this message, -keeping the Subject header intact. Or you can visit this web page - - $confirm_url +keeping the Subject header intact. If you do not wish to register this email address simply disregard this message. If you think you are being maliciously subscribed to the list, or -- cgit v1.2.3-70-g09d2 From 3126d190d9c8a9b37da952cba42ea6e3b838a2c3 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 17:26:52 -0400 Subject: Give IPendings a count of entries. --- src/mailman/interfaces/pending.py | 3 +++ src/mailman/model/docs/pending.rst | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/mailman/interfaces/pending.py b/src/mailman/interfaces/pending.py index 9907aa779..09c8b44cb 100644 --- a/src/mailman/interfaces/pending.py +++ b/src/mailman/interfaces/pending.py @@ -94,3 +94,6 @@ class IPendings(Interface): def evict(): """Remove all pended items whose lifetime has expired.""" + + def count(): + """The number of pendables in the pendings database.""" diff --git a/src/mailman/model/docs/pending.rst b/src/mailman/model/docs/pending.rst index a634322a1..11335f054 100644 --- a/src/mailman/model/docs/pending.rst +++ b/src/mailman/model/docs/pending.rst @@ -13,6 +13,11 @@ In order to pend an event, you first need a pending database. >>> from zope.component import getUtility >>> pendingdb = getUtility(IPendings) +There are nothing in the pendings database. + + >>> pendingdb.count() + 0 + The pending database can add any ``IPendable`` to the database, returning a token that can be used in urls and such. :: @@ -33,6 +38,11 @@ token that can be used in urls and such. >>> len(token) 40 +There's exactly one entry in the pendings database now. + + >>> pendingdb.count() + 1 + There's not much you can do with tokens except to *confirm* them, which basically means returning the `IPendable` structure (as a dictionary) from the database that matches the token. If the token isn't in the database, None is -- cgit v1.2.3-70-g09d2 From f7a4e76d24898ec942a0f3a9a932916d9d0662bc Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 18:05:26 -0400 Subject: * Give the workflow state manager a count method. * The workflow state manager no longer allows updates via .save(). * Restoring a workflow state evicts the record from the database. --- src/mailman/interfaces/workflow.py | 5 +++++ src/mailman/model/tests/test_workflow.py | 20 ++++++++++++++++++++ src/mailman/model/workflow.py | 19 +++++++++++-------- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/mailman/interfaces/workflow.py b/src/mailman/interfaces/workflow.py index 4a4fb4412..b60537652 100644 --- a/src/mailman/interfaces/workflow.py +++ b/src/mailman/interfaces/workflow.py @@ -63,4 +63,9 @@ class IWorkflowStateManager(Interface): :type name: str :param token: A unique token identifying this workflow instance. :type token: str + :raises LookupError: when there's no token associated with the given + name in the workflow table. """ + + def count(): + """The number of saved workflows in the database.""" diff --git a/src/mailman/model/tests/test_workflow.py b/src/mailman/model/tests/test_workflow.py index 6081e5b57..ccf618c2b 100644 --- a/src/mailman/model/tests/test_workflow.py +++ b/src/mailman/model/tests/test_workflow.py @@ -108,3 +108,23 @@ class TestWorkflow(unittest.TestCase): self._manager.save(name, token) workflow = self._manager.restore('ewe', 'fly') self.assertIsNone(workflow) + + def test_restore_removes_record(self): + name = 'ant' + token = 'bee' + self.assertEqual(self._manager.count(), 0) + self._manager.save(name, token) + self.assertEqual(self._manager.count(), 1) + self._manager.restore(name, token) + self.assertEqual(self._manager.count(), 0) + + def test_save_after_restore(self): + name = 'ant' + token = 'bee' + self.assertEqual(self._manager.count(), 0) + self._manager.save(name, token) + self.assertEqual(self._manager.count(), 1) + self._manager.restore(name, token) + self.assertEqual(self._manager.count(), 0) + self._manager.save(name, token) + self.assertEqual(self._manager.count(), 1) diff --git a/src/mailman/model/workflow.py b/src/mailman/model/workflow.py index d9f23f53b..6ac3fa76a 100644 --- a/src/mailman/model/workflow.py +++ b/src/mailman/model/workflow.py @@ -51,15 +51,18 @@ class WorkflowStateManager: @dbconnection def save(self, store, name, token, step=None, data=None): """See `IWorkflowStateManager`.""" - state = store.query(WorkflowState).get((name, token)) - if state is None: - state = WorkflowState(name=name, token=token, step=step, data=data) - store.add(state) - else: - state.step = step - state.data = data + state = WorkflowState(name=name, token=token, step=step, data=data) + store.add(state) @dbconnection def restore(self, store, name, token): """See `IWorkflowStateManager`.""" - return store.query(WorkflowState).get((name, token)) + state = store.query(WorkflowState).get((name, token)) + if state is not None: + store.delete(state) + return state + + @dbconnection + def count(self, store): + """See `IWorkflowStateManager`.""" + return store.query(WorkflowState).count() -- cgit v1.2.3-70-g09d2 From 24c01dbd8e93acdc61884b3b9783a0e71fd6df23 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Mon, 13 Apr 2015 18:28:59 -0400 Subject: The SubscriptionWorkflow class doesn't need to include the expiry date in its pendable data since that gets added automatically by the IPendings utility. After the user is subscribed, clean up the saved workflows and reset the token to None. Give the SubscriptionWorkflow a name property for convenience. --- src/mailman/app/subscriptions.py | 5 ++++- src/mailman/app/tests/test_subscriptions.py | 28 ++++++++++++++++++++++++++++ src/mailman/app/workflow.py | 4 ++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 6c60a71a4..999b04270 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -50,6 +50,7 @@ from mailman.interfaces.subscriptions import ( ISubscriptionService, MissingUserError, RequestRecord) from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager +from mailman.interfaces.workflow import IWorkflowStateManager from mailman.model.member import Member from mailman.utilities.datetime import now from mailman.utilities.i18n import make @@ -176,7 +177,6 @@ class SubscriptionWorkflow(Workflow): # Create a pending record. This will give us the hash token we can use # to uniquely name this workflow. pendable = Pendable( - when=now().isoformat(), list_id=self.mlist.list_id, address=self.address.email, ) @@ -268,6 +268,9 @@ class SubscriptionWorkflow(Workflow): def _step_do_subscription(self): # We can immediately subscribe the user to the mailing list. self.mlist.subscribe(self.subscriber) + # This workflow is done so throw away any associated state. + getUtility(IWorkflowStateManager).restore(self.name, self.token) + self.token = None def _step_send_confirmation(self): self.push('do_confirm_verify') diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py index bdfa72e29..7bb635a16 100644 --- a/src/mailman/app/tests/test_subscriptions.py +++ b/src/mailman/app/tests/test_subscriptions.py @@ -32,12 +32,14 @@ from mailman.interfaces.address import InvalidEmailAddressError from mailman.interfaces.bans import IBanManager from mailman.interfaces.member import ( MemberRole, MembershipIsBannedError, MissingPreferredAddressError) +from mailman.interfaces.pending import IPendings from mailman.interfaces.subscriptions import ( MissingUserError, ISubscriptionService) from mailman.testing.helpers import LogFileMark, get_queue_messages from mailman.testing.layers import ConfigLayer from mailman.interfaces.mailinglist import SubscriptionPolicy from mailman.interfaces.usermanager import IUserManager +from mailman.interfaces.workflow import IWorkflowStateManager from mailman.utilities.datetime import now from unittest.mock import patch from zope.component import getUtility @@ -343,6 +345,32 @@ class TestSubscriptionWorkflow(unittest.TestCase): member = self._mlist.regular_members.get_member(self._anne) self.assertEqual(member.address, anne) + def test_do_subscription_cleanups(self): + # Once the user is subscribed, the token, and its associated pending + # database record will be removed from the database. + self._mlist.subscription_policy = SubscriptionPolicy.open + anne = self._user_manager.create_address(self._anne) + workflow = SubscriptionWorkflow(self._mlist, anne, + pre_verified=True, + pre_confirmed=True, + pre_approved=True) + # Cache the token. + token = workflow.token + # Consume the entire state machine. + list(workflow) + # Anne is now a member of the mailing list. + member = self._mlist.regular_members.get_member(self._anne) + self.assertEqual(member.address, anne) + # The workflow is done, so it has no token. + self.assertIsNone(workflow.token) + # The pendable associated with the token has been evicted. + self.assertIsNone(getUtility(IPendings).confirm(token, expunge=False)) + # There is no saved workflow associated with the token. + new_workflow = SubscriptionWorkflow(self._mlist) + new_workflow.token = token + new_workflow.restore() + self.assertIsNone(new_workflow.which) + def test_moderator_approves(self): # The workflow runs until moderator approval is required, at which # point the workflow is saved. Once the moderator approves, the diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py index 9395dc7b7..8006f8e51 100644 --- a/src/mailman/app/workflow.py +++ b/src/mailman/app/workflow.py @@ -49,6 +49,10 @@ class Workflow: self.debug = False self._count = 0 + @property + def name(self): + return self.__class__.__name__ + def __iter__(self): return self -- cgit v1.2.3-70-g09d2 From 85afb7bac938eb2c2f00507482886e1470bdcaa1 Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 14 Apr 2015 12:35:53 -0400 Subject: Added IMember.subscriber to definitively return how a member is subscribed to the mailing list (via preferred address/user or explicit address). IMember.get_member() is defined to return the explicit address when members are subscribed in both ways. IMember.get_memberships() returns a sequence of length 0, 1, or 2 containing all the member records associated with the email address. Fixed the AbstractMemberRoster methods query to properly return subscriptions via the user's preferred address and via an explicit address. --- src/mailman/interfaces/member.py | 8 +++++ src/mailman/interfaces/roster.py | 21 +++++++++++-- src/mailman/model/docs/membership.rst | 38 +++++++++++++++++++++-- src/mailman/model/member.py | 4 +++ src/mailman/model/roster.py | 56 +++++++++++++++++++++++++++------- src/mailman/model/tests/test_roster.py | 52 ++++++++++++++++++++++++++++++- 6 files changed, 162 insertions(+), 17 deletions(-) diff --git a/src/mailman/interfaces/member.py b/src/mailman/interfaces/member.py index c06cc95b1..10bb3bc59 100644 --- a/src/mailman/interfaces/member.py +++ b/src/mailman/interfaces/member.py @@ -175,6 +175,14 @@ class IMember(Interface): user = Attribute( """The user associated with this member.""") + subscriber = Attribute( + """The object representing how this member is subscribed. + + This will be an ``IAddress`` if the user is subscribed via an explicit + address, otherwise if the the user is subscribed via their preferred + address, it will be an ``IUser``. + """) + preferences = Attribute( """This member's preferences.""") diff --git a/src/mailman/interfaces/roster.py b/src/mailman/interfaces/roster.py index 5d0b9d6c2..af473a553 100644 --- a/src/mailman/interfaces/roster.py +++ b/src/mailman/interfaces/roster.py @@ -53,11 +53,26 @@ class IRoster(Interface): managed by this roster. """) - def get_member(address): + def get_member(email): """Get the member for the given address. - :param address: The email address to search for. - :type address: text + *Note* that it is possible for an email to be subscribed to a + mailing list twice, once through its explicit address and once + indirectly through a user's preferred address. In this case, + this API always returns the explicit address. Use + ``get_memberships()`` to return them all. + + :param email: The email address to search for. + :type email: string :return: The member if found, otherwise None :rtype: `IMember` or None """ + + def get_memberships(email): + """Get the memberships for the given address. + + :param email: The email address to search for. + :type email: string + :return: All the memberships associated with this email address. + :rtype: sequence of length 0, 1, or 2 of ``IMember`` + """ diff --git a/src/mailman/model/docs/membership.rst b/src/mailman/model/docs/membership.rst index 60ccd1ac1..0fd748d6a 100644 --- a/src/mailman/model/docs/membership.rst +++ b/src/mailman/model/docs/membership.rst @@ -228,6 +228,38 @@ regardless of their role. fperson@example.com MemberRole.nonmember +Subscriber type +=============== + +Members can be subscribed to a mailing list either via an explicit address, or +indirectly through a user's preferred address. Sometimes you want to know +which one it is. + +Herb subscribes to the mailing list via an explicit address. + + >>> herb = user_manager.create_address( + ... 'hperson@example.com', 'Herb Person') + >>> herb_member = mlist.subscribe(herb) + +Iris subscribes to the mailing list via her preferred address. + + >>> iris = user_manager.make_user( + ... 'iperson@example.com', 'Iris Person') + >>> preferred = list(iris.addresses)[0] + >>> from mailman.utilities.datetime import now + >>> preferred.verified_on = now() + >>> iris.preferred_address = preferred + >>> iris_member = mlist.subscribe(iris) + +When we need to know which way a member is subscribed, we can look at the this +attribute. + + >>> herb_member.subscriber + [not verified] at ...> + >>> iris_member.subscriber + + + Moderation actions ================== @@ -250,6 +282,8 @@ should go through the normal moderation checks. aperson@example.com MemberRole.member Action.defer bperson@example.com MemberRole.member Action.defer cperson@example.com MemberRole.member Action.defer + hperson@example.com MemberRole.member Action.defer + iperson@example.com MemberRole.member Action.defer Postings by nonmembers are held for moderator approval by default. @@ -272,7 +306,7 @@ though that the address they're changing to must be verified. >>> gwen_member = bee.subscribe(gwen_address) >>> for m in bee.members.members: ... print(m.member_id.int, m.mailing_list.list_id, m.address.email) - 7 bee.example.com gwen@example.com + 9 bee.example.com gwen@example.com Gwen gets a email address. @@ -288,7 +322,7 @@ Now her membership reflects the new address. >>> for m in bee.members.members: ... print(m.member_id.int, m.mailing_list.list_id, m.address.email) - 7 bee.example.com gperson@example.com + 9 bee.example.com gperson@example.com Events diff --git a/src/mailman/model/member.py b/src/mailman/model/member.py index ee6d246f5..e6e4933f9 100644 --- a/src/mailman/model/member.py +++ b/src/mailman/model/member.py @@ -135,6 +135,10 @@ class Member(Model): if self._address is None else getUtility(IUserManager).get_user(self._address.email)) + @property + def subscriber(self): + return (self._user if self._address is None else self._address) + def _lookup(self, preference, default=None): pref = getattr(self.preferences, preference) if pref is not None: diff --git a/src/mailman/model/roster.py b/src/mailman/model/roster.py index 91211c665..d319aa819 100644 --- a/src/mailman/model/roster.py +++ b/src/mailman/model/roster.py @@ -97,21 +97,48 @@ class AbstractRoster: yield member.address @dbconnection - def get_member(self, store, address): - """See `IRoster`.""" - results = store.query(Member).filter( + def _get_all_memberships(self, store, email): + # Avoid circular imports. + from mailman.model.user import User + # Here's a query that finds all members subscribed with an explicit + # email address. + members_a = store.query(Member).filter( Member.list_id == self._mlist.list_id, Member.role == self.role, - Address.email == address, + Address.email == email, Member.address_id == Address.id) - if results.count() == 0: + # Here's a query that finds all members subscribed with their + # preferred address. + members_u = store.query(Member).filter( + Member.list_id == self._mlist.list_id, + Member.role == self.role, + Address.email==email, + Member.user_id == User.id) + return members_a.union(members_u).all() + + def get_member(self, email): + """See ``IRoster``.""" + memberships = self._get_all_memberships(email) + count = len(memberships) + if count == 0: return None - elif results.count() == 1: - return results[0] - else: - raise AssertionError( - 'Too many matching member results: {0}'.format( - results.count())) + elif count == 1: + return memberships[0] + assert count == 2, 'Unexpected membership count: {}'.format(count) + # This is the case where the email address is subscribed both + # explicitly and indirectly through the preferred address. By + # definition, we return the explicit address membership only. + return (memberships[0] + if memberships[0]._address is not None + else memberships[1]) + + def get_memberships(self, email): + """See ``IRoster``.""" + memberships = self._get_all_memberships(email) + count = len(memberships) + assert 0 <= count <= 2, 'Unexpected membership count: {}'.format( + count) + return memberships @@ -298,3 +325,10 @@ class Memberships: raise AssertionError( 'Too many matching member results: {0}'.format( results.count())) + + @dbconnection + def get_memberships(self, store, address): + """See `IRoster`.""" + # 2015-04-14 BAW: See LP: #1444055 -- this currently exists just to + # pass a test. + raise NotImplementedError diff --git a/src/mailman/model/tests/test_roster.py b/src/mailman/model/tests/test_roster.py index 44735cf4b..ca950cfc5 100644 --- a/src/mailman/model/tests/test_roster.py +++ b/src/mailman/model/tests/test_roster.py @@ -26,7 +26,9 @@ __all__ = [ import unittest from mailman.app.lifecycle import create_list +from mailman.interfaces.address import IAddress from mailman.interfaces.member import DeliveryMode, MemberRole +from mailman.interfaces.user import IUser from mailman.interfaces.usermanager import IUserManager from mailman.testing.layers import ConfigLayer from mailman.utilities.datetime import now @@ -136,7 +138,8 @@ class TestMembershipsRoster(unittest.TestCase): self._ant = create_list('ant@example.com') self._bee = create_list('bee@example.com') user_manager = getUtility(IUserManager) - self._anne = user_manager.create_user('anne@example.com') + self._anne = user_manager.make_user( + 'anne@example.com', 'Anne Person') preferred = list(self._anne.addresses)[0] preferred.verified_on = now() self._anne.preferred_address = preferred @@ -144,9 +147,56 @@ class TestMembershipsRoster(unittest.TestCase): def test_no_memberships(self): # An unsubscribed user has no memberships. self.assertEqual(self._anne.memberships.member_count, 0) + self.assertIsNone(self._ant.members.get_member('anne@example.com')) + self.assertEqual( + self._ant.members.get_memberships('anne@example.com'), + []) def test_subscriptions(self): # Anne subscribes to a couple of mailing lists. self._ant.subscribe(self._anne) self._bee.subscribe(self._anne) self.assertEqual(self._anne.memberships.member_count, 2) + + def test_subscribed_as_user(self): + # Anne subscribes to a mailing list as a user and the member roster + # contains her membership. + self._ant.subscribe(self._anne) + self.assertEqual( + self._ant.members.get_member('anne@example.com').user, + self._anne) + memberships = self._ant.members.get_memberships('anne@example.com') + self.assertEqual( + [member.address.email for member in memberships], + ['anne@example.com']) + + def test_subscribed_as_user_and_address(self): + # Anne subscribes to a mailing list twice, once as a user and once + # with an explicit address. She has two memberships. + self._ant.subscribe(self._anne) + self._ant.subscribe(self._anne.preferred_address) + self.assertEqual(self._anne.memberships.member_count, 2) + self.assertEqual(self._ant.members.member_count, 2) + self.assertEqual( + [member.address.email for member in self._ant.members.members], + ['anne@example.com', 'anne@example.com']) + # get_member() is defined to return the explicit address. + member = self._ant.members.get_member('anne@example.com') + subscriber = member.subscriber + self.assertTrue(IAddress.providedBy(subscriber)) + self.assertFalse(IUser.providedBy(subscriber)) + # get_memberships() returns them all. + memberships = self._ant.members.get_memberships('anne@example.com') + self.assertEqual(len(memberships), 2) + as_address = (memberships[0] + if IAddress.providedBy(memberships[0].subscriber) + else memberships[1]) + as_user = (memberships[1] + if IUser.providedBy(memberships[1].subscriber) + else memberships[0]) + self.assertEqual(as_address.subscriber, self._anne.preferred_address) + self.assertEqual(as_user.subscriber, self._anne) + # All the email addresses match. + self.assertEqual( + [record.address.email for record in memberships], + ['anne@example.com', 'anne@example.com']) -- cgit v1.2.3-70-g09d2 From 2787473f0bd4ca3efeadb7f44c8f61c3695e7ecd Mon Sep 17 00:00:00 2001 From: Barry Warsaw Date: Tue, 14 Apr 2015 12:46:11 -0400 Subject: Checkpointing. --- TODO.rst | 3 + src/mailman/app/registrar.py | 106 ++------- src/mailman/app/subscriptions.py | 2 +- src/mailman/app/tests/test_registrar.py | 170 ++++++++++++++ src/mailman/app/tests/test_registration.py | 128 ----------- src/mailman/app/workflow.py | 16 +- src/mailman/commands/eml_confirm.py | 2 +- src/mailman/commands/eml_membership.py | 3 +- src/mailman/commands/tests/test_confirm.py | 6 +- src/mailman/config/configure.zcml | 11 +- src/mailman/interfaces/pending.py | 4 +- src/mailman/interfaces/registrar.py | 62 ++--- src/mailman/model/docs/registration.rst | 353 +++++------------------------ src/mailman/model/pending.py | 6 +- src/mailman/model/tests/test_registrar.py | 64 ------ src/mailman/runners/docs/command.rst | 4 +- src/mailman/runners/tests/test_confirm.py | 4 +- src/mailman/runners/tests/test_join.py | 2 +- 18 files changed, 310 insertions(+), 636 deletions(-) create mode 100644 src/mailman/app/tests/test_registrar.py delete mode 100644 src/mailman/app/tests/test_registration.py delete mode 100644 src/mailman/model/tests/test_registrar.py diff --git a/TODO.rst b/TODO.rst index 2f97b538f..6ff3c3562 100644 --- a/TODO.rst +++ b/TODO.rst @@ -2,3 +2,6 @@ - get rid of hold_subscription - subsume handle_subscription - workflow for unsubscription + - make sure a user can't double confirm to bypass moderator approval + - make sure registration checks IEmailValidator + - rosters must return users subscribed via their preferred email diff --git a/src/mailman/app/registrar.py b/src/mailman/app/registrar.py index db68432d3..5c4c46c1f 100644 --- a/src/mailman/app/registrar.py +++ b/src/mailman/app/registrar.py @@ -25,18 +25,13 @@ __all__ = [ import logging +from mailman.app.subscriptions import SubscriptionWorkflow from mailman.core.i18n import _ from mailman.email.message import UserNotification -from mailman.interfaces.address import IEmailValidator -from mailman.interfaces.listmanager import IListManager -from mailman.interfaces.member import DeliveryMode, MemberRole from mailman.interfaces.pending import IPendable, IPendings from mailman.interfaces.registrar import ConfirmationNeededEvent, IRegistrar from mailman.interfaces.templates import ITemplateLoader -from mailman.interfaces.usermanager import IUserManager -from mailman.utilities.datetime import now from zope.component import getUtility -from zope.event import notify from zope.interface import implementer @@ -54,92 +49,27 @@ class PendableRegistration(dict): class Registrar: """Handle registrations and confirmations for subscriptions.""" - def register(self, mlist, email, display_name=None, delivery_mode=None): + def __init__(self, mlist): + self._mlist = mlist + + def register(self, subscriber=None, *, + pre_verified=False, pre_confirmed=False, pre_approved=False): """See `IRegistrar`.""" - if delivery_mode is None: - delivery_mode = DeliveryMode.regular - # First, do validation on the email address. If the address is - # invalid, it will raise an exception, otherwise it just returns. - getUtility(IEmailValidator).validate(email) - # Create a pendable for the registration. - pendable = PendableRegistration( - type=PendableRegistration.PEND_KEY, - email=email, - display_name=display_name, - delivery_mode=delivery_mode.name, - list_id=mlist.list_id) - token = getUtility(IPendings).add(pendable) - # We now have everything we need to begin the confirmation dance. - # Trigger the event to start the ball rolling, and return the - # generated token. - notify(ConfirmationNeededEvent(mlist, pendable, token)) - return token + workflow = SubscriptionWorkflow( + self._mlist, subscriber, + pre_verified=pre_verified, + pre_confirmed=pre_confirmed, + pre_approved=pre_approved) + list(workflow) + return workflow.token def confirm(self, token): """See `IRegistrar`.""" - # For convenience - pendable = getUtility(IPendings).confirm(token) - if pendable is None: - return False - missing = object() - email = pendable.get('email', missing) - display_name = pendable.get('display_name', missing) - pended_delivery_mode = pendable.get('delivery_mode', 'regular') - try: - delivery_mode = DeliveryMode[pended_delivery_mode] - except ValueError: - log.error('Invalid pended delivery_mode for {0}: {1}', - email, pended_delivery_mode) - delivery_mode = DeliveryMode.regular - if pendable.get('type') != PendableRegistration.PEND_KEY: - # It seems like it would be very difficult to accurately guess - # tokens, or brute force an attack on the SHA1 hash, so we'll just - # throw the pendable away in that case. It's possible we'll need - # to repend the event or adjust the API to handle this case - # better, but for now, the simpler the better. - return False - # We are going to end up with an IAddress for the verified address - # and an IUser linked to this IAddress. See if any of these objects - # currently exist in our database. - user_manager = getUtility(IUserManager) - address = (user_manager.get_address(email) - if email is not missing else None) - user = (user_manager.get_user(email) - if email is not missing else None) - # If there is neither an address nor a user matching the confirmed - # record, then create the user, which will in turn create the address - # and link the two together - if address is None: - assert user is None, 'How did we get a user but not an address?' - user = user_manager.create_user(email, display_name) - # Because the database changes haven't been flushed, we can't use - # IUserManager.get_address() to find the IAddress just created - # under the hood. Instead, iterate through the IUser's addresses, - # of which really there should be only one. - for address in user.addresses: - if address.email == email: - break - else: - raise AssertionError('Could not find expected IAddress') - elif user is None: - user = user_manager.create_user() - user.display_name = display_name - user.link(address) - else: - # The IAddress and linked IUser already exist, so all we need to - # do is verify the address. - pass - address.verified_on = now() - # If this registration is tied to a mailing list, subscribe the person - # to the list right now. That will generate a SubscriptionEvent, - # which can be used to send a welcome message. - list_id = pendable.get('list_id') - if list_id is not None: - mlist = getUtility(IListManager).get_by_list_id(list_id) - if mlist is not None: - member = mlist.subscribe(address, MemberRole.member) - member.preferences.delivery_mode = delivery_mode - return True + workflow = SubscriptionWorkflow(self._mlist) + workflow.token = token + workflow.debug = True + workflow.restore() + list(workflow) def discard(self, token): # Throw the record away. diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py index 999b04270..7b46aee84 100644 --- a/src/mailman/app/subscriptions.py +++ b/src/mailman/app/subscriptions.py @@ -297,7 +297,7 @@ class SubscriptionWorkflow(Workflow): if self.address.verified_on is None: self.address.verified_on = now() # The next step depends on the mailing list's subscription policy. - next_step = ('moderation_check' + next_step = ('moderation_checks' if self.mlist.subscription_policy in ( SubscriptionPolicy.moderate, SubscriptionPolicy.confirm_then_moderate, diff --git a/src/mailman/app/tests/test_registrar.py b/src/mailman/app/tests/test_registrar.py new file mode 100644 index 000000000..c8e0044de --- /dev/null +++ b/src/mailman/app/tests/test_registrar.py @@ -0,0 +1,170 @@ +# Copyright (C) 2012-2015 by the Free Software Foundation, Inc. +# +# This file is part of GNU Mailman. +# +# GNU Mailman is free software: you can redistribute it and/or modify it under +# the terms of the GNU General Public License as published by the Free +# Software Foundation, either version 3 of the License, or (at your option) +# any later version. +# +# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +# more details. +# +# You should have received a copy of the GNU General Public License along with +# GNU Mailman. If not, see . + +"""Test email address registration.""" + +__all__ = [ + 'TestRegistrar', + ] + + +import unittest + +from mailman.app.lifecycle import create_list +from mailman.interfaces.mailinglist import SubscriptionPolicy +from mailman.interfaces.pending import IPendings +from mailman.interfaces.registrar import IRegistrar +from mailman.interfaces.usermanager import IUserManager +from mailman.testing.layers import ConfigLayer +from mailman.utilities.datetime import now +from zope.component import getUtility + + + +class TestRegistrar(unittest.TestCase): + """Test registration.""" + + layer = ConfigLayer + + def setUp(self): + self._mlist = create_list('ant@example.com') + self._registrar = IRegistrar(self._mlist) + self._pendings = getUtility(IPendings) + self._anne = getUtility(IUserManager).create_address( + 'anne@example.com') + + def test_unique_token(self): + # Registering a subscription request provides a unique token associated + # with a pendable. + self.assertEqual(self._pendings.count(), 0) + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + self.assertEqual(self._pendings.count(), 1) + record = self._pendings.confirm(token, expunge=False) + self.assertEqual(record['list_id'], self._mlist.list_id) + self.assertEqual(record['address'], 'anne@example.com') + + def test_no_token(self): + # Registering a subscription request where no confirmation or + # moderation steps are needed, leaves us with no token, since there's + # nothing more to do. + self._mlist.subscription_policy = SubscriptionPolicy.open + self._anne.verified_on = now() + token = self._registrar.register(self._anne) + self.assertIsNone(token) + record = self._pendings.confirm(token, expunge=False) + self.assertIsNone(record) + + def test_is_subscribed(self): + # Where no confirmation or moderation steps are needed, registration + # happens immediately. + self._mlist.subscription_policy = SubscriptionPolicy.open + self._anne.verified_on = now() + self._registrar.register(self._anne) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(member.address, self._anne) + + def test_no_such_token(self): + # Given a token which is not in the database, a LookupError is raised. + self._registrar.register(self._anne) + self.assertRaises(LookupError, self._registrar.confirm, 'not-a-token') + + def test_confirm_because_verify(self): + # We have a subscription request which requires the user to confirm + # (because she does not have a verified address), but not the moderator + # to approve. Running the workflow gives us a token. Confirming the + # token subscribes the user. + self._mlist.subscription_policy = SubscriptionPolicy.open + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Now confirm the subscription. + self._registrar.confirm(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(member.address, self._anne) + + def test_confirm_because_confirm(self): + # We have a subscription request which requires the user to confirm + # (because of list policy), but not the moderator to approve. Running + # the workflow gives us a token. Confirming the token subscribes the + # user. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + self._anne.verified_on = now() + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Now confirm the subscription. + self._registrar.confirm(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(member.address, self._anne) + + def test_confirm_because_moderation(self): + # We have a subscription request which requires the moderator to + # approve. Running the workflow gives us a token. Confirming the + # token subscribes the user. + self._mlist.subscription_policy = SubscriptionPolicy.moderate + self._anne.verified_on = now() + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Now confirm the subscription. + self._registrar.confirm(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(member.address, self._anne) + + def test_confirm_because_confirm_then_moderation(self): + # We have a subscription request which requires the user to confirm + # (because she does not have a verified address) and the moderator to + # approve. Running the workflow gives us a token. Confirming the + # token runs the workflow a little farther, but still gives us a + # token. Confirming again subscribes the user. + self._mlist.subscription_policy = \ + SubscriptionPolicy.confirm_then_moderate + self._anne.verified_on = now() + # Runs until subscription confirmation. + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Now confirm the subscription, and wait for the moderator to approve + # the subscription. She is still not subscribed. + self._registrar.confirm(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Confirm once more, this time as the moderator approving the + # subscription. Now she's a member. + self._registrar.confirm(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertEqual(member.address, self._anne) + + def test_discard_waiting_for_confirmation(self): + # While waiting for a user to confirm their subscription, we discard + # the workflow. + self._mlist.subscription_policy = SubscriptionPolicy.confirm + self._anne.verified_on = now() + # Runs until subscription confirmation. + token = self._registrar.register(self._anne) + self.assertIsNotNone(token) + member = self._mlist.regular_members.get_member('anne@example.com') + self.assertIsNone(member) + # Now discard the subscription request. + self._registrar.discard(token) + # Trying to confirm the token now results in an exception. + self.assertRaises(LookupError, self._registrar.confirm, token) diff --git a/src/mailman/app/tests/test_registration.py b/src/mailman/app/tests/test_registration.py deleted file mode 100644 index ccc485492..000000000 --- a/src/mailman/app/tests/test_registration.py +++ /dev/null @@ -1,128 +0,0 @@ -# Copyright (C) 2012-2015 by the Free Software Foundation, Inc. -# -# This file is part of GNU Mailman. -# -# GNU Mailman is free software: you can redistribute it and/or modify it under -# the terms of the GNU General Public License as published by the Free -# Software Foundation, either version 3 of the License, or (at your option) -# any later version. -# -# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT -# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or -# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for -# more details. -# -# You should have received a copy of the GNU General Public License along with -# GNU Mailman. If not, see . - -"""Test email address registration.""" - -__all__ = [ - 'TestEmailValidation', - 'TestRegistration', - ] - - -import unittest - -from mailman.app.lifecycle import create_list -from mailman.interfaces.address import InvalidEmailAddressError -from mailman.interfaces.pending import IPendings -from mailman.interfaces.registrar import ConfirmationNeededEvent, IRegistrar -from mailman.testing.helpers import event_subscribers -from mailman.testing.layers import ConfigLayer -from zope.component import getUtility - - - -class TestEmailValidation(unittest.TestCase): - """Test basic email validation.""" - - layer = ConfigLayer - - def setUp(self): - self.registrar = getUtility(IRegistrar) - self.mlist = create_list('alpha@example.com') - - def test_empty_string_is_invalid(self): - self.assertRaises(InvalidEmailAddressError, - self.registrar.register, self.mlist, - '') - - def test_no_spaces_allowed(self): - self.assertRaises(InvalidEmailAddressError, - self.registrar.register, self.mlist, - 'some name@example.com') - - def test_no_angle_brackets(self): - self.assertRaises(InvalidEmailAddressError, - self.registrar.register, self.mlist, - '