Skip to content

fix(rejection): notify released signers only after the rejection commits - #8948

Merged
2 commits merged into
LibreSign:mainfrom
maia-andre:fix/notify-after-rejection-commit-8803
Oct 20, 2026
Merged

2 commits merged into
LibreSign:mainfrom
maia-andre:fix/notify-after-rejection-commit-8803

Conversation

@maia-andre

Copy link
Copy Markdown
Contributor

Resolves: #8803

📝 Summary

With rejection_behavior = continue, SignatureRejectionService::persistRejection() releases the next signing order of every document the rejection closes, inside one transaction. SequentialSigningService::releaseNextOrder() notified each released signer immediately, so if a later document of an envelope failed and the transaction rolled back, the signer had already received a "ready to sign" notification for an activation that no longer existed.

This PR separates the state change from the notification:

  • SequentialSigningService::activateNextOrder() sets the next order to ABLE_TO_SIGN and returns the activated signers without notifying them.
  • SequentialSigningService::notifyActivatedSigners() sends the notifications. This is the code that used to live inside activateSignersForOrder().
  • releaseNextOrder() calls both, so the non-transactional callers in SignFileService and reorderAfterDeletion() behave as before.
  • SignatureRejectionService collects the released signers inside the transaction and notifies them only after commit(). On rollback, nobody is notified.

A notification that fails after the commit is logged and does not turn the recorded rejection into an error for the signer. This follows the same reasoning already used for SignatureRejectedEvent.

Composition: +79/−26 lines of production code in two files, +136/−6 lines of tests. Start with SequentialSigningService.php.

🧪 How to test

composer test:unit -- --filter SequentialSigningServiceTest
composer test:unit -- --filter SignatureRejectionServiceTest

New tests:

  • SequentialSigningServiceTest: activation does not notify and returns the activated signers; a parallel flow activates nobody; every identify method of every signer is notified.
  • SignatureRejectionServiceTest: the order is activate → commit → notify; an envelope with two documents where the second fails rolls back and notifies nobody; a failing notification after the commit keeps the rejection.

I also ran the other service tests that use SequentialSigningService (RequestSignatureService, SignFileService, SignRequest/*, SignerValidator) and the whole SignatureRejection directory. Psalm and php-cs-fixer report nothing on the changed files.

⚙️ API / Back‑end changes

  • Notifications for signers released by a rejection are sent only after the rejection transaction commits
  • Unit and/or integration tests added – required for backend changes

No Behat scenario was added: the issue asks for the test at the service/event boundary, and the existing continue scenario in sign/reject.feature uses a parallel flow, which does not release a next order.

✅ Checklist

🤖 AI (if applicable)

  • The content of this PR was partially or fully generated using AI

@maia-andre
maia-andre requested a review from a team as a code owner October 1, 2026 01:50
@vitormattos vitormattos added this to the Next Major (36) milestone Oct 1, 2026 — with ChatGPT Codex Connector
@vitormattos
vitormattos force-pushed the fix/notify-after-rejection-commit-8803 branch from 76aba49 to 3926542 Compare October 1, 2026 21:14
@maia-andre
maia-andre force-pushed the fix/notify-after-rejection-commit-8803 branch from 3926542 to 02b718f Compare October 2, 2026 00:27
With `rejection_behavior = continue`, a rejection releases the next
signing order of every document it closes inside one transaction.
`SequentialSigningService::releaseNextOrder()` notified each released
signer right away, so when a later document of an envelope failed and
the transaction rolled back, the signer had already been told that a
document was ready to sign.

`activateNextOrder()` now performs only the state change and returns
the signers it activated, and `notifyActivatedSigners()` sends their
notifications. `releaseNextOrder()` calls both, so the signing flow and
`reorderAfterDeletion()` keep their behavior. The rejection collects the
released signers and notifies them after the commit.

`notifyActivatedSigners()` owns the delivery error policy: each signer
is notified by `notifySigner()`, which isolates every identify method,
so one failed delivery does not keep the remaining signers, already
able to sign, from being told. Only `\Exception` is isolated and logged
with the sign request and identify method; an `\Error` is a programming
error and stays visible.

Resolves: LibreSign#8803
Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
@maia-andre
maia-andre force-pushed the fix/notify-after-rejection-commit-8803 branch from 02b718f to 0eb8da9 Compare October 2, 2026 23:10
@YvesCesar
YvesCesar self-requested a review October 3, 2026 14:00

@YvesCesar YvesCesar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks, @maia-andre !

I added a comment for review.

Comment thread lib/Service/SequentialSigningService.php Outdated
…d signers

Notifying the signers released by a rejection runs after the commit, so
nothing raised there may turn a recorded rejection into an error. Two
gaps remained:

- `SequentialSigningService::notifySigner()` loaded the identify methods
  outside the isolated delivery, so an exception from that lookup (a lost
  database connection, for example) escaped `reject()`. The signer got an
  error for a rejection that was already saved and could not retry.
- `reject()` notified the released signers before dispatching
  `SignatureRejectedEvent`, so an error while notifying skipped the event.

The lookup is now isolated and logged like a failed delivery, and the
remaining signers are still notified. `persistRejection()` returns the
released signers and `reject()` dispatches the event before notifying
them.

Assisted-by: Claude Code:claude-opus-5-5
Signed-off-by: André Maia <andrefnkmm@gmail.com>
@maia-andre
maia-andre requested a review from YvesCesar October 3, 2026 23:56
vitormattos added a commit to tnalbant/libresign that referenced this pull request Oct 6, 2026
Keep recoverable notification failures in SequentialSigningService,
extract per-signer delivery, and preserve rollback assertions.
Adapt identification lookup isolation from PR LibreSign#8948 while keeping
rejection event ordering for that follow-up PR.

Co-authored-by: André Maia <andrefnkmm@gmail.com>

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
vitormattos added a commit to tnalbant/libresign that referenced this pull request Oct 6, 2026
Return activated signers after committing the rejection and dispatch
SignatureRejectedEvent before attempting their notifications. Preserve
event delivery when a post-commit notification error propagates.

Adapt the event-ordering correction from PR LibreSign#8948 and strengthen the
successful-order and notification-error regression assertions.

Co-authored-by: André Maia <andrefnkmm@gmail.com>

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
@vitormattos vitormattos closed this Oct 6, 2026
@vitormattos vitormattos reopened this Oct 6, 2026
vitormattos added a commit that referenced this pull request Oct 6, 2026
Preserve the remaining applicable coverage from André Maia's #8948
series after resolving the follow-up against current main. The production
fixes and error-ordering changes from 7cee8ff
are already present through #8912, so only the distinct notification
coverage remains to apply.

Cherry-picked-from: 7cee8ff
Co-authored-by: André Maia <andrefnkmm@gmail.com>

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
@maia-andre

Copy link
Copy Markdown
Contributor Author

Thanks for reconciling this, @vitormattos, and for keeping me as co-author on a800b90.

I compared this branch with main (after #8912) plus #9041, test by test. Everything here is now covered:

Here main + #9041
testSignersReleasedByARejectionAreNotifiedOnlyAfterTheCommit testTheReleasedOrderIsNotifiedOnlyAfterTheRejectionIsCommitted
testTheRejectionEventIsDispatchedBeforeTheReleasedSignersAreNotified same test (asserts commit → event → notification)
testARolledBackRejectionNotifiesNobodyItHadReleased testARolledBackRejectionDoesNotNotifyAnOrderReleasedForAnEarlierDocument
testAnErrorWhileNotifyingAfterTheCommitIsNotHidden testAnUnexpectedNotificationExceptionDoesNotRollBackACommittedRejection, testAProgrammingErrorWhileNotifyingIsNotConvertedIntoADeliveryFailure
testAFailedIdentifyMethodLookupDoesNotStopTheOtherActivatedSigners testNotifyActivatedSignersContinuesAfterAnIdentificationLookupFails
testAFailedNotificationDoesNotStopTheOtherActivatedSigners testNotifyActivatedSignersKeepsNotifyingTheRemainingSignersWhenADeliveryFails
testAProgrammingErrorWhileNotifyingIsNotHidden testNotifyActivatedSignersDoesNotSwallowProgrammingErrors

The three points @YvesCesar raised are also in main: the identify-method lookup is inside a try, SignatureRejectedEvent is dispatched before notifyActivatedSigners(), and a failed lookup is tested.

So nothing from this PR is left outside #9041. Unless you see something I missed, I'd close this one in favor of #9041. Should I close it now, or wait until #9041 is merged?

@maia-andre

Copy link
Copy Markdown
Contributor Author

@vitormattos a gentle follow-up on the question above: #9041 still covers everything from this PR. Should I close this one now, or wait until #9041 is merged? Once this is settled I'll push the second part of #8915, which builds on it.

vitormattos added a commit that referenced this pull request Oct 19, 2026
Preserve the original commits from #8948 as ancestors of this merge while reconciling the branch with current main and the focused tests from #9041.

The production fix and overlapping tests remain as integrated by #8912; the diff against main consists exclusively of the 38 additional test lines from #9041.

Signed-off-by: Vitor Mattos <1079143+vitormattos@users.noreply.github.com>
@vitormattos vitormattos closed this pull request by merging all changes into LibreSign:main in b3f7269 Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Avoid sending next-order notifications before a rejection transaction commits

3 participants