4 files changed, 164 insertions, 74 deletions

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
 
 
@@ -75,48 +74,6 @@ class TestJoin(unittest.TestCase):
 
 
 
-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 <http://www.gnu.org/licenses/>.
+
+"""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: