Repository: hasgeek/coaster

Issue:
StateManager needs an AbortTransition exception
If a transition does `raise AbortTransition(result)`, the state should not change, and the `result` should be returned to the caller as the return value of the transition method.

The anticipated use case is that callers will use something like `success = obj.transition(…)` or `status, message = obj.transition(…)`. All exceptions except `AbortTransition` should pass through uncaught.

Discussion:

Patch:
diff --git a/.gitignore b/.gitignore
index 5e6c4a0..f1aea8c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -14,3 +14,4 @@ error.log
 __pycache__
 .pytest_cache
 .cache
+.vscode
diff --git a/coaster/sqlalchemy/statemanager.py b/coaster/sqlalchemy/statemanager.py
index de2f75e..a2a5f0e 100644
--- a/coaster/sqlalchemy/statemanager.py
+++ b/coaster/sqlalchemy/statemanager.py
@@ -67,6 +67,18 @@ control state change via transitions. Sample usage::
             # A transition can do additional housekeeping
             self.datetime = datetime.utcnow()
 
+            # If AbortTransition is raised in this method, the state wont be changed.
+            # AbortTransition will return anything passed to it as return value of the method
+            # E.g.
+            # success, message = post.publish()
+
+            success = False
+            message = "A transition is not desirable right now"
+            raise AbortTransition(success, message)
+
+            # Any other exception will be raised
+            raise RandomError("This will be raised")
+
         # A transition can use a conditional state. The condition is evaluated
         # before the transition can proceed
         @state.transition(state.RECENT, state.PENDING)
@@ -204,7 +216,7 @@ from ..utils import is_collection, NameTitle
 from ..signals import coaster_signals
 from .roles import RoleMixin
 
-__all__ = ['StateManager', 'StateTransitionError',
+__all__ = ['StateManager', 'StateTransitionError', 'AbortTransition',
     'transition_error', 'transition_before', 'transition_after', 'transition_exception']
 
 
@@ -230,6 +242,21 @@ class StateTransitionError(BadRequest, TypeError):
     pass
 
 
+class AbortTransition(Exception):
+    """
+    Transitions may raise :exc:`AbortTransition` to return without changing
+    state. The parameter to this exception is returned as the transition's
+    result.
+
+    This exception is a signal to :class:`StateTransition` and will not be
+    raised to the transition's caller.
+
+    :param result: Value to return to the transition's caller
+    """
+    def __init__(self, result=None):
+        super(AbortTransition, self).__init__(result)
+
+
 # --- Classes -----------------------------------------------------------------
 
 class ManagedState(object):
@@ -458,6 +485,7 @@ class StateTransition(object):
 
     def __set_name__(self, owner, name):  # pragma: no cover
         self.name = name
+        self.data['name'] = name
 
     # Make the transition a non-data descriptor
     def __get__(self, obj, cls=None):
@@ -521,19 +549,23 @@ class StateTransitionWrapper(object):
                     label=repr(state_invalid[2])
                     ))
 
-        # Raise a transition-before signal
+        # Send a transition-before signal
         transition_before.send(self.obj, transition=self.statetransition)
-        # Call the transition function
+        # Call the transition method
         try:
             result = self.statetransition.func(self.obj, *args, **kwargs)
+        except AbortTransition as e:
+            transition_exception.send(self.obj, transition=self.statetransition, exception=e)
+            return e.args[0]
         except Exception as e:
             transition_exception.send(self.obj, transition=self.statetransition, exception=e)
             raise
+
         # Change the state for each of the state managers
         for statemanager, conditions in self.statetransition.transitions.items():
             if conditions['to'] is not None:  # Allow to=None for the @requires decorator
                 statemanager._set(self.obj, conditions['to'].value)  # Change state
-        # Raise a transition-after signal
+        # Send a transition-after signal
         transition_after.send(self.obj, transition=self.statetransition)
         return result
 
@@ -677,11 +709,12 @@ class StateManager(object):
 
     def transition(self, from_, to, if_=None, **data):
         """
-        Decorates a function to transition from one state to another. The
-        decorated function can accept any necessary parameters and perform
+        Decorates a method to transition from one state to another. The
+        decorated method can accept any necessary parameters and perform
         additional processing, or raise an exception to abort the transition.
         If it returns without an error, the state value is updated
-        automatically.
+        automatically. Transitions may also abort without raising an exception
+        using :exc:`AbortTransition`.
 
         :param from_: Required state to allow this transition (can be a state group)
         :param to: The state of the object after this transition (automatically set if no exception is raised)


Test patch:
diff --git a/tests/test_statemanager.py b/tests/test_statemanager.py
index ad61af4..51ab10f 100644
--- a/tests/test_statemanager.py
+++ b/tests/test_statemanager.py
@@ -9,7 +9,7 @@ from flask_sqlalchemy import SQLAlchemy
 from coaster.utils import LabeledEnum
 from coaster.auth import add_auth_attribute
 from coaster.sqlalchemy import (with_roles, BaseMixin,
-    StateManager, StateTransitionError)
+    StateManager, StateTransitionError, AbortTransition)
 from coaster.sqlalchemy.statemanager import ManagedStateWrapper
 
 
@@ -113,6 +113,17 @@ class MyPost(BaseMixin, db.Model):
     def rewind(self):
         self.datetime = datetime.utcnow() - timedelta(hours=2)
 
+    @with_roles(call={'author'})
+    @state.transition(state.UNPUBLISHED, state.PUBLISHED, message=u"Abort this transition")
+    @reviewstate.transition(reviewstate.UNLOCKED, reviewstate.PENDING, title="Publish")
+    def abort(self, success=False, empty_abort=False):
+        if not success:
+            if empty_abort:
+                raise AbortTransition()
+            else:
+                raise AbortTransition((success, 'failed'))
+        return success, 'passed'
+
     def roles_for(self, actor, anchors=()):
         roles = super(MyPost, self).roles_for(actor, anchors)
         # Cheap hack for the sake of testing, using strings instead of objects
@@ -509,6 +520,26 @@ class TestStateManager(unittest.TestCase):
         self.assertFalse(self.post.reviewstate.LOCKED)
         self.assertTrue(self.post.state.PENDING)
 
+    def test_transition_abort(self):
+        """Transitions can abort without changing state or raising an exception"""
+        self.assertTrue(self.post.state.DRAFT)
+
+        # A transition can abort returning a value (a 2-tuple here)
+        success, message = self.post.abort(success=False)
+        self.assertEqual(success, False)
+        self.assertEqual(message, "failed")
+        self.assertTrue(self.post.state.DRAFT)  # state has not changed
+
+        # A transition can abort without returning a value
+        result = self.post.abort(success=False, empty_abort=True)
+        self.assertEqual(result, None)
+        self.assertTrue(self.post.state.DRAFT)  # state has not changed
+
+        success, message = self.post.abort(success=True)
+        self.assertEqual(success, True)
+        self.assertEqual(message, 'passed')
+        self.assertTrue(self.post.state.PUBLISHED)  # state has changed
+
     def test_transition_is_available(self):
         """A transition's is_available property is reliable"""
         self.assertTrue(self.post.state.DRAFT)