diff --git a/signoffs/core/tests/test_view_actions.py b/signoffs/core/tests/test_view_actions.py index 06057e5..739a337 100644 --- a/signoffs/core/tests/test_view_actions.py +++ b/signoffs/core/tests/test_view_actions.py @@ -2,6 +2,7 @@ App-independent tests for view.actions - no app logic """ import django_fsm as fsm +from django.db import transaction from django.db.models import Model, TextChoices from django.test import TestCase @@ -66,6 +67,46 @@ def test_invalid_data(self): self.assertIsNone(bf) +class SignoffCommitterTests(TestCase): + def setUp(self): + self.user = fixtures.get_user(perms=("add_signoff",)) + self.signoff = signoff_type() + + def test_create(self): + committer = actions.BasicSignoffCommitter(self.user) + self.assertEqual(committer.user, self.user) + + def test_sign(self): + committer = actions.BasicSignoffCommitter(self.user) + committer.sign(self.signoff) + self.assertTrue(self.signoff.is_signed()) + + def test_revoke(self): + self.signoff.sign(self.user) + committer = actions.BasicSignoffCommitter(self.user) + committer.revoke(self.signoff) + self.assertFalse(self.signoff.is_signed()) + + def test_post_signoff_hook(self): + obj = lambda: None + obj.called = False + def hook(signoff): + obj.called = True + committer = actions.BasicSignoffCommitter(self.user, post_signoff_hook=hook) + committer.sign(self.signoff) + self.assertTrue(obj.called) + + def test_post_revoke_hook(self): + obj = lambda: None + obj.called = False + def hook(signoff): + obj.called = True + self.signoff.sign(self.user) + committer = actions.BasicSignoffCommitter(self.user, post_revoke_hook=hook) + committer.revoke(self.signoff) + self.assertTrue(obj.called) + + class BasicUserSignoffActionsTests(TestCase): def setUp(self): self.user = fixtures.get_user(perms=("add_signoff",)) @@ -192,6 +233,62 @@ class ActionsApproval(BaseApproval): signing_order = so.SigningOrder(first_signoff, final_signoff) +class ApprovalSignoffCommitterTests(TestCase): + def setUp(self): + self.user = fixtures.get_user(perms=("add_signoff",)) + self.approval = ActionsApproval.create() + self.signoff = ActionsApproval.first_signoff(stamp=self.approval.stamp) + + def test_create(self): + committer = actions.ApprovalSignoffCommitter(self.user, self.approval) + self.assertEqual(committer.user, self.user) + self.assertTrue(isinstance(committer.signoff_committer, actions.BasicSignoffCommitter)) + self.assertEqual(committer.signoff_committer.user, self.user) + + def test_sign(self): + committer = actions.ApprovalSignoffCommitter(self.user, self.approval) + committer.sign(self.signoff) + self.assertTrue(self.signoff.is_signed()) + + def test_revoke(self): + self.signoff.sign(self.user) + committer = actions.ApprovalSignoffCommitter(self.user, self.approval) + committer.revoke(self.signoff) + self.assertFalse(self.signoff.is_signed()) + + def test_default_post_signoff_hook(self): + """Default post_signoff_hook approves the approval when signing order complete""" + committer = actions.ApprovalSignoffCommitter(self.user, self.approval, + post_signoff_hook=lambda s,a: self.approval.approve_if_ready()) + committer.sign(self.approval.first_signoff(stamp=self.approval.stamp)) + committer.sign(self.approval.final_signoff(stamp=self.approval.stamp)) + self.assertTrue(self.approval.is_approved()) + + def test_post_signoff_hook(self): + """Custom post_signoff_hook runs in an atomic transaction to maintain integrity of triggered DB ops""" + obj = lambda: None + obj.called = 0 + def hook(signoff, approval): + self.assertFalse(transaction.get_autocommit()) + obj.called +=1 + committer = actions.ApprovalSignoffCommitter(self.user, self.approval, post_signoff_hook=hook) + committer.sign(self.approval.first_signoff(stamp=self.approval.stamp)) + committer.sign(self.approval.final_signoff(stamp=self.approval.stamp)) + self.assertFalse(self.approval.is_approved()) + self.assertEqual(obj.called, 2) + + def test_post_revoke_hook(self): + obj = lambda: None + obj.called = False + def hook(signoff, approval): + self.assertFalse(transaction.get_autocommit()) + obj.called = True + self.signoff.sign(self.user) + committer = actions.ApprovalSignoffCommitter(self.user, self.approval, post_revoke_hook=hook) + committer.revoke(self.signoff) + self.assertTrue(obj.called) + + class BasicUserApprovalActionsTests(TestCase): def setUp(self): self.user = fixtures.get_user(perms=("add_signoff",)) diff --git a/signoffs/core/views/actions.py b/signoffs/core/views/actions.py index 944ac85..f5b8140 100644 --- a/signoffs/core/views/actions.py +++ b/signoffs/core/views/actions.py @@ -102,14 +102,23 @@ class BasicSignoffCommitter: """ user: User + """The user who is signing the signoff""" + post_signoff_hook: Callable[[AbstractSignoff], None] = lambda s: None + """A function that takes the signed signoff as argument, called in atomic transaction after signing""" + post_revoke_hook: Callable[[AbstractSignoff], None] = lambda s: None + """A function that takes the revoked signoff as argument, called in atomic transaction after revoking""" def sign(self, signoff: AbstractSignoff): """Sign and commit the signoff for given user - no validation, just do it!""" - signoff.sign(self.user) + with transaction.atomic(): + signoff.sign(self.user) + self.post_signoff_hook(signoff) def revoke(self, signoff: AbstractSignoff): """Revoke the signoff for given user and commit changes to DB - no validation, just do it!""" - signoff.revoke(self.user) + with transaction.atomic(): + signoff.revoke(self.user) + self.post_revoke_hook(signoff) class SignoffRequestFormHandler(Protocol): @@ -360,24 +369,43 @@ def is_valid_revoke_request(self, signoff: AbstractSignoff) -> bool: @dataclass -class ApprovalSignoffCommitter(BasicSignoffCommitter): +class ApprovalSignoffCommitter: """ Default commit logic for requests to Signoff on an Approval. - Approves and commits the approval if it is ready after the current signoff is committed. + Implements `SignoffCommitter` Protocol + Signoff on a signoff of this approval. :::{caution} - Potential sync. issue here if approval has cached / prefetched signoffs - don't do that! + Potential sync. issue if post_signoff_hook tries to approve the approval (i.e., the default hook!) + but approval has cached / prefetched signoffs - don't do that! ::: """ + user: User + """The user who is signing the signoff""" approval: AbstractApproval - approve_if_ready: Callable[[AbstractApproval], bool] + """The approval that will be signed off (or have signoff revoked)""" + signoff_committer: SignoffCommitter = None + """The SingoffComitter object used to do the actual signoff, None to use a `BasisSignoffCommitter`""" + post_signoff_hook: Callable[[AbstractSignoff, AbstractApproval], None] = lambda s,a: None + """A function that takes the signed signoff and approval as arguments, called in atomic transaction after signing""" + post_revoke_hook: Callable[[AbstractSignoff, AbstractApproval], None] = lambda s,a: None + """A function that takes the revoked signoff and approval as argument, called in atomic transaction after revoking""" + + def __post_init__(self): + self.signoff_committer = self.signoff_committer or BasicSignoffCommitter(self.user) def sign(self, signoff: AbstractSignoff): - """Sign and commit the signoff, and approve approval if ready - no validation, just do it!""" + """Sign and commit the signoff, and call post_signoff_hook - no validation, just do it!""" with transaction.atomic(): - super().sign(signoff) - self.approve_if_ready(self.approval) + self.signoff_committer.sign(signoff) + self.post_signoff_hook(signoff, self.approval) + + def revoke(self, signoff: AbstractSignoff): + """Revoke the signoff and commit changes to DB - no validation, just do it!""" + with transaction.atomic(): + self.signoff_committer.revoke(signoff) + self.post_revoke_hook(signoff, self.approval) class ApprovalRequestActions(Protocol): @@ -428,8 +456,14 @@ class BasicUserApprovalActions: """ validator_class: SignoffValidator = ApprovalSignoffValidator + """Default `validator` uses get_verify_signet and get_verify_stamp as verifiers""" committer_class: SignoffCommitter = ApprovalSignoffCommitter + """Default `committer` approves the approval when it is `ready_to_approve` + + Override `approve()` method if you just need to extend the post_signoff_hook action + """ signoff_actions_class = BasicUserSignoffActions + """Default `signoff_actions` is constructed from the other components.""" def __init__( self, @@ -461,7 +495,7 @@ def __init__( verify_stamp=self.verify_stamp, ) committer = committer or self.committer_class( - user, approval, approve_if_ready=lambda a: self.approve() + user, approval, post_signoff_hook=lambda s,a: self.approve() ) forms = self.signoff_actions_class.form_handler_class( self.data, signoff_subject=self.approval