[#162] Don't cancel possible transitions even though it is the future…

… of one of those that is cancelled (#168) * [#162] Don't cancel possible transitions even though it is the future of one of those that is cancelled * [#162] Don't cancel those transitions that are actually in the possible future just because they are also in the future of impossible transition path
javrasya · Jul 20, 2020 · 80b3359 · 80b3359
1 parent f693cdb
commit 80b3359
Show file tree

Hide file tree

Showing 6 changed files with 150 additions and 47 deletions.
diff --git a/docs/changelog.rst b/docs/changelog.rst
@@ -3,8 +3,14 @@
 Change Logs
 ===========
 
-3.2.1 (Stable):
+3.2.2 (Stable):
 ---------------
+    * **Bug**         -  # 162_: Fix a bug that is causing some possible future transitions to turn to CANCELLED for some workflows.
+
+.. _162: https://github.com/javrasya/django-river/issues/159
+
+3.2.1:
+------
     * **Bug**         -  # 159_: A bug that is with having multiple cyclic dependencies in a workflow that happens when one of tem goes through has been fixed.
     * **Drop**        -        : Drop Python3.4 support since it is having incompatibilities with the module ``six``
 

diff --git a/docs/conf.py b/docs/conf.py
@@ -57,9 +57,9 @@
 # built documents.
 #
 # The short X.Y version.
-version = '3.2.1'
+version = '3.2.2'
 # The full version, including alpha/beta/rc tags.
-release = '3.2.1'
+release = '3.2.2'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

diff --git a/features/issue162_1.feature b/features/issue162_1.feature
@@ -0,0 +1,43 @@
+Feature: An example #162 Flow that is set up with django-river (https://github.com/javrasya/django-river/issues/162)
+
+  Background: some requirement of this test
+    # Groups
+    Given a group with name "Authorized Group"
+
+    # Users
+    Given a user with name authorized_user with group "Authorized Group"
+
+    # States
+    Given a state with label "Draft"
+    And a state with label "Issued"
+    And a state with label "Part Received"
+    And a state with label "Received"
+    And a state with label "Closed"
+
+    # Workflow
+    Given a workflow with an identifier "#162 Flow" and initial state "Draft"
+
+    # Transitions
+    Given a transition "Draft" -> "Issued" in "#162 Flow"
+    And a transition "Issued" -> "Part Received" in "#162 Flow"
+    And a transition "Part Received" -> "Received" in "#162 Flow"
+    And a transition "Issued" -> "Received" in "#162 Flow"
+    And a transition "Received" -> "Issued" in "#162 Flow"
+    And a transition "Received" -> "Closed" in "#162 Flow"
+
+    # Authorization Rules
+    Given an authorization rule for the transition "Draft" -> "Issued" with group "Authorized Group" and priority 0
+    Given an authorization rule for the transition "Issued" -> "Part Received" with group "Authorized Group" and priority 0
+    Given an authorization rule for the transition "Part Received" -> "Received" with group "Authorized Group" and priority 0
+    Given an authorization rule for the transition "Issued" -> "Received" with group "Authorized Group" and priority 0
+    Given an authorization rule for the transition "Received" -> "Issued" with group "Authorized Group" and priority 0
+    Given an authorization rule for the transition "Received" -> "Closed" with group "Authorized Group" and priority 0
+
+  Scenario: Should allow the state to transit all the way to Closed
+    Given a workflow object with identifier "object 1"
+    When "object 1" is attempted to be approved for next state "Issued" by authorized_user
+    And "object 1" is attempted to be approved for next state "Part Received" by authorized_user
+    And "object 1" is attempted to be approved for next state "Received" by authorized_user
+    And "object 1" is attempted to be approved for next state "Closed" by authorized_user
+    And get current state of "object 1"
+    Then return current state as "Closed"
diff --git a/river/core/instanceworkflowobject.py b/river/core/instanceworkflowobject.py
@@ -158,55 +158,31 @@ def approve(self, as_user, next_state=None):
     @atomic
     def cancel_impossible_future(self, approved_approval):
         transition = approved_approval.transition
-        qs = Q(
-            workflow=self.workflow,
-            object_id=self.workflow_object.pk,
-            iteration=transition.iteration,
-            source_state=transition.source_state,
-        ) & ~Q(destination_state=transition.destination_state)
-
-        transitions = Transition.objects.filter(qs)
-        iteration = transition.iteration + 1
-        cancelled_transitions_qs = Q(pk=-1)
-        while transitions:
-            cancelled_transitions_qs = cancelled_transitions_qs | qs
-            qs = Q(
+
+        possible_transition_ids = {transition.pk}
+
+        possible_next_states = {transition.destination_state.label}
+        while possible_next_states:
+            possible_transitions = Transition.objects.filter(
                 workflow=self.workflow,
                 object_id=self.workflow_object.pk,
-                iteration=iteration,
-                source_state__pk__in=transitions.values_list("destination_state__pk", flat=True)
-            )
-            transitions = Transition.objects.filter(qs)
-            iteration += 1
+                status=PENDING,
+                source_state__label__in=possible_next_states
+            ).exclude(pk__in=possible_transition_ids)
+
+            possible_transition_ids.update(set(possible_transitions.values_list("pk", flat=True)))
 
-        uncancelled_transitions_qs = Q(pk=-1)
-        qs = Q(
+            possible_next_states = set(possible_transitions.values_list("destination_state__label", flat=True))
+
+        cancelled_transitions = Transition.objects.filter(
             workflow=self.workflow,
             object_id=self.workflow_object.pk,
-            iteration=transition.iteration,
-            source_state=transition.source_state,
-            destination_state=transition.destination_state
-        )
-        transitions = Transition.objects.filter(qs)
-        iteration = transition.iteration + 1
-        while transitions:
-            uncancelled_transitions_qs = uncancelled_transitions_qs | qs
-            qs = Q(
-                workflow=self.workflow,
-                object_id=self.workflow_object.pk,
-                iteration=iteration,
-                source_state__pk__in=transitions.values_list("destination_state__pk", flat=True),
-                status=PENDING
-            )
-            transitions = Transition.objects.filter(qs)
-            iteration += 1
-
-        actual_cancelled_transitions = Transition.objects.select_for_update(nowait=True).filter(cancelled_transitions_qs & ~uncancelled_transitions_qs)
-        for actual_cancelled_transition in actual_cancelled_transitions:
-            actual_cancelled_transition.status = CANCELLED
-            actual_cancelled_transition.save()
+            status=PENDING,
+            iteration__gte=transition.iteration
+        ).exclude(pk__in=possible_transition_ids)
 
-        TransitionApproval.objects.filter(transition__in=actual_cancelled_transitions).update(status=CANCELLED)
+        TransitionApproval.objects.filter(transition__in=cancelled_transitions).update(status=CANCELLED)
+        cancelled_transitions.update(status=CANCELLED)
 
     def _approve_signal(self, approval):
         return ApproveSignal(self.workflow_object, self.field_name, approval)

diff --git a/river/tests/core/test__instance_api.py b/river/tests/core/test__instance_api.py
@@ -1953,3 +1953,81 @@ def test_shouldAllowMultipleCyclicTransitions(self):
 
         workflow_object.model.river.my_field.approve(as_user=authorized_user)
         assert_that(workflow_object.model.my_field, equal_to(cycle_state_1))
+
+    def test_shouldNotCancelDescendantsThatCanBeTransitedInTheFuture(self):
+        authorized_permission = PermissionObjectFactory()
+
+        authorized_user = UserObjectFactory(user_permissions=[authorized_permission])
+
+        state1 = StateObjectFactory(label="state_1")
+        state2 = StateObjectFactory(label="state_2")
+        state3 = StateObjectFactory(label="state_3")
+        final_state = StateObjectFactory(label="final_state")
+
+        workflow = WorkflowFactory(initial_state=state1, content_type=self.content_type, field_name="my_field")
+
+        transition_meta_1 = TransitionMetaFactory.create(
+            workflow=workflow,
+            source_state=state1,
+            destination_state=state2,
+        )
+
+        transition_meta_2 = TransitionMetaFactory.create(
+            workflow=workflow,
+            source_state=state1,
+            destination_state=state3,
+        )
+
+        transition_meta_3 = TransitionMetaFactory.create(
+            workflow=workflow,
+            source_state=state2,
+            destination_state=state3,
+        )
+
+        transition_meta_4 = TransitionMetaFactory.create(
+            workflow=workflow,
+            source_state=state3,
+            destination_state=final_state,
+        )
+
+        TransitionApprovalMetaFactory.create(
+            workflow=workflow,
+            transition_meta=transition_meta_1,
+            priority=0,
+            permissions=[authorized_permission]
+        )
+
+        TransitionApprovalMetaFactory.create(
+            workflow=workflow,
+            transition_meta=transition_meta_2,
+            priority=0,
+            permissions=[authorized_permission]
+        )
+
+        TransitionApprovalMetaFactory.create(
+            workflow=workflow,
+            transition_meta=transition_meta_3,
+            priority=0,
+            permissions=[authorized_permission]
+        )
+
+        finalTransitionApprovalMeta = TransitionApprovalMetaFactory.create(
+            workflow=workflow,
+            transition_meta=transition_meta_4,
+            priority=0,
+            permissions=[authorized_permission]
+        )
+
+        workflow_object = BasicTestModelObjectFactory()
+
+        assert_that(workflow_object.model.my_field, equal_to(state1))
+        workflow_object.model.river.my_field.approve(as_user=authorized_user, next_state=state2)
+        assert_that(workflow_object.model.my_field, equal_to(state2))
+
+        assert_that(
+            finalTransitionApprovalMeta.transition_approvals.all(),
+            all_of(
+                has_length(1),
+                has_item(has_property("status", PENDING))
+            )
+        ),
diff --git a/setup.py b/setup.py
@@ -13,7 +13,7 @@
 
 setup(
     name='django-river',
-    version='3.2.1',
+    version='3.2.2',
     author='Ahmet DAL',
     author_email='[email protected]',
     packages=find_packages(),