Repository navigation
fix: apply signer removal to every document in an envelope - #9101
carlomarmo wants to merge 2 commits into
Conversation
Keep the signer's identification details available when removing their requests from the documents in an envelope. Add tests for removing one of two signers and removing the last signer. Refs LibreSign#9085 Signed-off-by: Carlo Marmo <carlo.marmo@gmail.com>
|
Thanks for opening your first pull request in this repository! ✌️ |
YvesCesar
left a comment
YvesCesar
left a comment
There was a problem hiding this comment.
Thanks @carlomarmo !
I made a few revisions. Could you take a look?
| ->reorderAfterDeletion($file->getId(), $deletedOrder); | ||
|
|
||
| $this->propagateSignerDeletionToChildren($file, $signRequest); | ||
| $this->propagateSignerDeletionToChildren($file, $groupedIdentifyMethods); |
There was a problem hiding this comment.
With this change, the propagation actually runs. Each child removal goes through unassociateToUser() (line 673), which calls dispatchCancellationEventIfNeeded() again. Child sign requests are ABLE_TO_SIGN when the envelope was sent, so removing a signer from a 2-document envelope dispatches 3 SignRequestCanceledEvent: the signer gets 3 cancellation emails, 3 notifications and 3 activity entries.
The cancellation listeners don't skip child files (unlike sendSignedMailNotification, which returns early on hasParent()).
When adding signers, propagateSignersToChildren() sets notify = 0 so only the envelope notifies. Removal should do the same: dispatch the event only for the envelope request and skip it for the child requests removed by the propagation. Could you also extend the Behat scenario to check that the signer receives a single cancellation notification?
| /** | ||
| * @param array<string, array<IIdentifyMethod>> $identifyMethods | ||
| */ | ||
| private function propagateSignerDeletionToChildren(FileEntity $envelope, array $identifyMethods): void { |
There was a problem hiding this comment.
Nit: since $identifyMethods is now a parameter, the empty($identifyMethods) early return can come before getChildrenFiles(), which avoids the query when there is nothing to propagate.
Send the cancellation event for the envelope, not each document updated with it. Keep notifications for direct document removals. Check the email, notification and activity counts, and skip the child lookup when there are no identification methods. Signed-off-by: Carlo Marmo <carlo.marmo@gmail.com>
|
Thanks @YvesCesar. Removing a signer from the envelope now sends one cancellation notification, email and activity entry. Removing a signer directly from a document still notifies as before. I've added tests for both and moved the empty check before the child lookup. |
This branch has not been deployed
2 participants
Removing someone from an envelope currently leaves their signing requests open on the documents inside it. Remove the last signer and the envelope returns to draft, but its documents still wait for a signature. The person managing the package cannot rely on the envelope's signer list matching its documents.
The removal code was deleting the signer's identification details before using them to find the matching requests on the documents. This fix reuses the details already read before deletion, so removal reaches every document and leaves the other signers alone.
Two added tests cover removing one of two signers and removing the last signer. Both fail without the fix; all 15 focused Behat scenarios pass with it. PHP syntax and whitespace checks also pass.
Checked locally with PHP 8.3.6, Nextcloud 36 (development version) and SQLite. PHPUnit, static analysis, GitHub CI and the full supported-version matrix have not run.
Related to #9085.