Merge branch 'workflow-attribute-dependencies' into 'master'

Handle a missing user when a workflow is restored

Between the creation of a subscription workflow and its approval, the user may have been merged with another user (it did happen). In this case, use the associated address to find the new user.

This requires that the address be restored before the user. The current method to restore does not guarantee the order, so I changed it to allow attributes to depend on one another.

See merge request !190

4 files changed, 103 insertions, 3 deletions

diff --git a/src/mailman/app/subscriptions.py b/src/mailman/app/subscriptions.py
index 9e8d8dd9d..f88a967c2 100644
--- a/src/mailman/app/subscriptions.py
+++ b/src/mailman/app/subscriptions.py
@@ -102,7 +102,8 @@ class _SubscriptionWorkflowCommon(Workflow):
         # For restore.
         uid = uuid.UUID(hex_key)
         self.user = getUtility(IUserManager).get_user_by_id(uid)
-        assert self.user is not None
+        if self.user is None:
+            self.user = self.address.user
 
     @property
     def address_key(self):
diff --git a/src/mailman/app/tests/test_subscriptions.py b/src/mailman/app/tests/test_subscriptions.py
index f7b9b584e..b960b7904 100644
--- a/src/mailman/app/tests/test_subscriptions.py
+++ b/src/mailman/app/tests/test_subscriptions.py
@@ -675,3 +675,45 @@ approval:
         self.assertIsNone(workflow.token)
         self.assertEqual(workflow.token_owner, TokenOwner.no_one)
         self.assertEqual(workflow.member.address, anne)
+
+    def test_restore_user_absorbed(self):
+        # The subscribing user is absorbed (and thus deleted) before the
+        # moderator approves the subscription.
+        self._mlist.subscription_policy = SubscriptionPolicy.moderate
+        anne = self._user_manager.create_user(self._anne)
+        bill = self._user_manager.create_user('bill@example.com')
+        set_preferred(bill)
+        # anne subscribes.
+        workflow = SubscriptionWorkflow(self._mlist, anne, pre_verified=True)
+        list(workflow)
+        # bill absorbs anne.
+        bill.absorb(anne)
+        # anne's subscription request is approved.
+        approved_workflow = SubscriptionWorkflow(self._mlist)
+        approved_workflow.token = workflow.token
+        approved_workflow.restore()
+        self.assertEqual(approved_workflow.user, bill)
+        # Run the workflow through.
+        list(approved_workflow)
+
+    def test_restore_address_absorbed(self):
+        # The subscribing user is absorbed (and thus deleted) before the
+        # moderator approves the subscription.
+        self._mlist.subscription_policy = SubscriptionPolicy.moderate
+        anne = self._user_manager.create_user(self._anne)
+        anne_address = anne.addresses[0]
+        bill = self._user_manager.create_user('bill@example.com')
+        # anne subscribes.
+        workflow = SubscriptionWorkflow(
+            self._mlist, anne_address, pre_verified=True)
+        list(workflow)
+        # bill absorbs anne.
+        bill.absorb(anne)
+        self.assertIn(anne_address, bill.addresses)
+        # anne's subscription request is approved.
+        approved_workflow = SubscriptionWorkflow(self._mlist)
+        approved_workflow.token = workflow.token
+        approved_workflow.restore()
+        self.assertEqual(approved_workflow.user, bill)
+        # Run the workflow through.
+        list(approved_workflow)
diff --git a/src/mailman/app/tests/test_workflow.py b/src/mailman/app/tests/test_workflow.py
index a5bbd0792..8ff4f34b9 100644
--- a/src/mailman/app/tests/test_workflow.py
+++ b/src/mailman/app/tests/test_workflow.py
@@ -17,10 +17,13 @@
 
 """App-level workflow tests."""
 
+import json
 import unittest
 
 from mailman.app.workflow import Workflow
+from mailman.interfaces.workflow import IWorkflowStateManager
 from mailman.testing.layers import ConfigLayer
+from zope.component import getUtility
 
 
 class MyWorkflow(Workflow):
@@ -111,6 +114,56 @@ class TestWorkflow(unittest.TestCase):
         self.assertEqual(new_workflow.cat, 7)
         self.assertEqual(new_workflow.dog, 4)
 
+    def test_save_and_restore_dependant_attributes(self):
+        # Attributes must be restored in the order they are declared in
+        # SAVE_ATTRIBUTES.
+
+        class DependantWorkflow(MyWorkflow):
+            SAVE_ATTRIBUTES = ('ant', 'bee', 'cat', 'elf')
+
+            def __init__(self):
+                super().__init__()
+                self._elf = 5
+
+            @property
+            def elf(self):
+                return self._elf
+
+            @elf.setter
+            def elf(self, value):
+                # This attribute depends on other attributes.
+                assert self.ant is not None
+                assert self.bee is not None
+                assert self.cat is not None
+                self._elf = value
+
+        workflow = iter(DependantWorkflow())
+        workflow.elf = 6
+        workflow.save()
+        new_workflow = DependantWorkflow()
+        # The elf attribute must be restored last, set triggering values for
+        # attributes it depends on.
+        new_workflow.ant = new_workflow.bee = new_workflow.cat = None
+        new_workflow.restore()
+        self.assertEqual(new_workflow.elf, 6)
+
+    def test_save_and_restore_obsolete_attributes(self):
+        # Obsolete saved attributes are ignored.
+        state_manager = getUtility(IWorkflowStateManager)
+        # Save the state of an old version of the workflow that would not have
+        # the cat attribute.
+        state_manager.save(
+            self._workflow.token, 'first',
+            json.dumps({'ant': 1, 'bee': 2}))
+        # Restore in the current version that needs the cat attribute.
+        new_workflow = MyWorkflow()
+        try:
+            new_workflow.restore()
+        except KeyError:
+            self.fail("Restore does not handle obsolete attributes")
+        # Restoring must not raise an exception, the default value is kept.
+        self.assertEqual(new_workflow.cat, 3)
+
     def test_run_thru(self):
         # Run all steps through the given one.
         results = self._workflow.run_thru('second')
diff --git a/src/mailman/app/workflow.py b/src/mailman/app/workflow.py
index 36fd7d611..cd9124993 100644
--- a/src/mailman/app/workflow.py
+++ b/src/mailman/app/workflow.py
@@ -143,5 +143,9 @@ class Workflow:
         self._next.clear()
         if state.step:
             self._next.append(state.step)
-        for attr, value in json.loads(state.data).items():
-            setattr(self, attr, value)
+        data = json.loads(state.data)
+        for attr in self.SAVE_ATTRIBUTES:
+            try:
+                setattr(self, attr, data[attr])
+            except KeyError:
+                pass