[tnalbant]

Fixes #8803

When rejection_behavior lets the workflow continue, persistRejection() releases the next signing order inside its transaction. activateSignersForOrder() persisted ABLE_TO_SIGN and dispatched the notification in the same method, so on an envelope with several documents the signers released for an earlier document were already notified when a later document failed and rolled the transaction back: the message went out for an activation that no longer existed.

activateSignersForOrder() now only persists the state and returns the signers it activated. activateNextOrder() exposes that for callers running inside a transaction, and notifyActivatedSigners() dispatches the notifications, which SignatureRejectionService calls after commit(). Recoverable lookup and delivery failures are isolated in the notification service. After committing the rejection, reject() dispatches SignatureRejectedEvent before notifying the released signers, so a programming error during notification cannot prevent the rejection event from being dispatched.

releaseNextOrder() keeps activating and notifying in one step, so the non-transactional callers (SignFileService, RequestSignatureService::reorderAfterDeletion) are unchanged.

Four existing tests in SignatureRejectionServiceTest are updated because the call site moved from releaseNextOrder() to activateNextOrder(); their assertions keep testing the same behaviour.

Tests:

• testARolledBackRejectionDoesNotNotifyAnOrderReleasedForAnEarlierDocument — envelope with two documents, the first releases the next order, the second throws, the transaction rolls back and no signing notification is dispatched for the released order.
• testTheReleasedOrderIsNotifiedOnlyAfterTheRejectionIsCommitted — the successful release still notifies, after the commit.
• testAnUnexpectedNotificationExceptionDoesNotRollBackACommittedRejection
• testActivateNextOrderReturnsTheActivatedSignersWithoutNotifyingThem, testNotifyActivatedSignersNotifiesEverySignerItReceives, testNotifyActivatedSignersIsANoOpWithoutActivatedSigners

Original contributor verification, before the maintainer follow-up commits: PHPUnit on the two touched test classes (82 tests) passed, php-cs-fixer reported no change over the whole repository and psalm reported no errors on the two changed services.

Maintainer follow-up validation: git diff --check passed. Runtime tests and static analysis have not been rerun locally because PHP, Composer, and a Nextcloud runtime are unavailable in the editing environment; current CI validation is pending. Regression assertions now enforce commit → rejection event → notifications and require rejection event dispatch even when a notification error propagates. Identification lookup isolation and rejection-event ordering were adapted from André Maia's contribution in #8948, credited in the signed DCO commits.

This change was prepared with AI coding assistance. I remain responsible for the submitted change.

[welcome]

Thanks for opening your first pull request in this repository! ✌️

[tnalbant]

Both review points are addressed:

• notifyActivatedSigners() now isolates each notification per signer and
  identify method. A failed delivery is logged with the sign request id and
  identify method, while the remaining activated signers are still attempted.
• SignatureRejectionService now catches recoverable \Exception failures at
  the post-commit notification boundary instead of \Throwable, so programming
  errors such as TypeError remain visible to the caller.

Regression tests cover both behaviours. The focused suites pass
(47 + 38 tests), and php-cs-fixer and Psalm are clean.

[tnalbant]

I noticed #8948 also addresses #8803 and that the issue is assigned to its author. Before updating #8912 for the latest review comments, would you prefer that I continue this PR or close it in favour of #8948?

[vitormattos]

Thanks for flagging this, and sorry for the confusion in our review process. We approved a second implementation of #8803 while your PR was already open and under review; that coordination mistake was ours.

We plan to consolidate the fix in #8948, which also includes the later notification lookup and event-ordering fixes. We have prepared additional changes based on your PR: stronger persistence and rollback assertions, an explicit no-notification expectation after a failed release, and appending activated signers without rebuilding the accumulated list. These changes have not been pushed yet because our commit-signing service failed.

Please hold off on further updates here while we finish the consolidation. Thank you for your work and for raising the overlap before spending more time on it.

[vitormattos]

Update to our earlier consolidation plan: we will merge this PR first, preserving your original implementation and commits, then adapt #8948 into a complementary follow-up for rejection-event ordering.

I pushed fb8ad1a to address the outstanding review: extracted per-signer notification handling, removed the redundant catch in SignatureRejectionService, isolated identification lookup failures, and strengthened notification and rollback assertions. The lookup isolation builds on André Maia's contribution in #8948, credited in the commit.

The commit is cryptographically signed with DCO. git diff --check passed; PHP/Composer/Nextcloud are unavailable in this environment, so runtime validation is pending CI. GitHub currently marks the workflow runs as action_required, awaiting maintainer authorization.

Sorry again for approving overlapping work while your PR was already under review. That coordination mistake was ours. Both contributions will be retained.

[vitormattos]

/backport to stable35

[github-actions]

Hi @tnalbant, your first pull request to LibreSign has been merged. 🎉

Your work is part of LibreSign now, and everyone who signs a document with it benefits from the time and knowledge you shared. Thank you.

You are very welcome to contribute again. The good first issue list is a good place to look next, and you no longer have to wonder how the process works.

If you have a few minutes and feel like it, we would love to hear how this went for you: https://ls.librecode.coop/index.php/645932?lang=en&source=github-first-merged-pr&repository=libresign. It is entirely optional; nothing is expected of you in return for contributing.

Come say hello in the LibreSign community: https://t.me/LibreSign

[vitormattos]

/backport to stable34

[vitormattos]

/backport to stable33

[backportbot-libresign]

The backport to stable34 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable34
git pull origin stable34

# Create the new backport branch
git checkout -b backport/8912/stable34

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick ed8d8335 d4fe1be5 fb8ad1a4 f067b559

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/8912/stable34

Error: Failed to check for changes with origin/stable34: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[backportbot-libresign]

The backport to stable33 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable33
git pull origin stable33

# Create the new backport branch
git checkout -b backport/8912/stable33

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick ed8d8335 d4fe1be5 fb8ad1a4 f067b559

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/8912/stable33

Error: Failed to check for changes with origin/stable33: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[vitormattos]

After taking a closer look, we don't need to backport this fix to the stable branches. The issue was introduced only on the main branch and does not affect the current stable releases.

Thanks for your contribution, and you're very welcome to join our community on Telegram: https://t.me/LibreSign

[vitormattos]

@tnalbant don't forgot the survey:
#8912 (comment)