diff --git a/e2e/console/document_version_test.go b/e2e/console/document_version_test.go index 7227ad2fd..24afa3656 100644 --- a/e2e/console/document_version_test.go +++ b/e2e/console/document_version_test.go @@ -92,59 +92,15 @@ func createTestDocument(t *testing.T, owner *testutil.Client) (docID string, doc func approveTestDocument(t *testing.T, owner *testutil.Client, docID string) { t.Helper() - requestQuery := ` - mutation RequestApproval($input: PublishDocumentInput!) { - publishDocument(input: $input) { - approvalQuorum { - id - } - } - } - ` + requestDocumentApproval(t, owner, docID, []string{getOwnerProfileID(t, owner)}) + approveLatestDocumentVersion(t, owner, docID) +} - // Use the owner's profile as the approver - approverID := getOwnerProfileID(t, owner) +// latestDocumentVersionID returns the ID of the document's most recently created version. +func latestDocumentVersionID(t *testing.T, owner *testutil.Client, docID string) string { + t.Helper() - _, err := owner.Do(requestQuery, map[string]any{ - "input": map[string]any{ - "minor": false, - "documentId": docID, - "approverIds": []string{approverID}, - "changelog": "Test changelog", - }, - }) - require.NoError(t, err) - - // Approve for each approver - approveQuery := ` - mutation ApproveDocumentVersion($input: ApproveDocumentVersionInput!) { - approveDocumentVersion(input: $input) { - approvalDecision { - id - state - } - } - } - ` - - // Get the latest version ID - versionQuery := ` - query GetVersions($id: ID!) { - node(id: $id) { - ... on Document { - versions(first: 1, orderBy: { field: CREATED_AT, direction: DESC }) { - edges { - node { - id - } - } - } - } - } - } - ` - - var versionResult struct { + var result struct { Node struct { Versions struct { Edges []struct { @@ -156,15 +112,161 @@ func approveTestDocument(t *testing.T, owner *testutil.Client, docID string) { } `json:"node"` } - err = owner.Execute(versionQuery, map[string]any{"id": docID}, &versionResult) + err := owner.Execute(` + query($id: ID!) { + node(id: $id) { + ... on Document { + versions(first: 1, orderBy: { field: CREATED_AT, direction: DESC }) { + edges { node { id } } + } + } + } + } + `, map[string]any{"id": docID}, &result) require.NoError(t, err) - require.NotEmpty(t, versionResult.Node.Versions.Edges) + require.NotEmpty(t, result.Node.Versions.Edges) - versionID := versionResult.Node.Versions.Edges[0].Node.ID + return result.Node.Versions.Edges[0].Node.ID +} - _, err = owner.Do(approveQuery, map[string]any{ +// requestDocumentApproval opens a major approval quorum on the document's draft. +func requestDocumentApproval(t *testing.T, owner *testutil.Client, docID string, approverIDs []string) { + t.Helper() + + _, err := owner.Do(` + mutation($input: PublishDocumentInput!) { + publishDocument(input: $input) { + approvalQuorum { id } + } + } + `, map[string]any{ + "input": map[string]any{ + "minor": false, + "documentId": docID, + "approverIds": approverIDs, + "changelog": "Test changelog", + }, + }) + require.NoError(t, err) +} + +// approveLatestDocumentVersion approves the document's most recent version. +func approveLatestDocumentVersion(t *testing.T, owner *testutil.Client, docID string) { + t.Helper() + + _, err := owner.Do(` + mutation($input: ApproveDocumentVersionInput!) { + approveDocumentVersion(input: $input) { + approvalDecision { id state } + } + } + `, map[string]any{ + "input": map[string]any{ + "documentVersionId": latestDocumentVersionID(t, owner, docID), + }, + }) + require.NoError(t, err) +} + +// publishMajorDocumentVersion publishes the document's draft as a major version +// without approvers and returns the published version ID. +func publishMajorDocumentVersion(t *testing.T, owner *testutil.Client, docID string) string { + t.Helper() + + var result struct { + PublishDocument struct { + DocumentVersion struct { + ID string `json:"id"` + Status string `json:"status"` + } `json:"documentVersion"` + } `json:"publishDocument"` + } + + err := owner.Execute(` + mutation($input: PublishDocumentInput!) { + publishDocument(input: $input) { + documentVersion { id status } + } + } + `, map[string]any{ + "input": map[string]any{ + "minor": false, + "documentId": docID, + "changelog": "Major release", + }, + }, &result) + require.NoError(t, err) + require.Equal(t, "PUBLISHED", result.PublishDocument.DocumentVersion.Status) + + return result.PublishDocument.DocumentVersion.ID +} + +// publishMinorDocumentVersion publishes the document's draft as a minor version +// and returns the published version ID. +func publishMinorDocumentVersion(t *testing.T, owner *testutil.Client, docID string) string { + t.Helper() + + var result struct { + PublishDocument struct { + DocumentVersion struct { + ID string `json:"id"` + Status string `json:"status"` + } `json:"documentVersion"` + } `json:"publishDocument"` + } + + err := owner.Execute(` + mutation($input: PublishDocumentInput!) { + publishDocument(input: $input) { + documentVersion { id status } + } + } + `, map[string]any{ + "input": map[string]any{ + "minor": true, + "documentId": docID, + "changelog": "Minor release", + }, + }, &result) + require.NoError(t, err) + require.Equal(t, "PUBLISHED", result.PublishDocument.DocumentVersion.Status) + + return result.PublishDocument.DocumentVersion.ID +} + +// updateDocumentContent edits the document so a fresh draft is created. +func updateDocumentContent(t *testing.T, owner *testutil.Client, docID, content string) { + t.Helper() + + _, err := owner.Do(` + mutation($input: UpdateDocumentInput!) { + updateDocument(input: $input) { + documentVersion { id } + } + } + `, map[string]any{ + "input": map[string]any{ + "id": docID, + "content": testutil.ProseMirrorTextDoc(content), + }, + }) + require.NoError(t, err) +} + +// requestDocumentSignature requests a signature on the version from the signatory. +func requestDocumentSignature(t *testing.T, owner *testutil.Client, versionID, signatoryID string) { + t.Helper() + + _, err := owner.Do(` + mutation($input: RequestSignatureInput!) { + requestSignature(input: $input) { + documentVersionSignatureEdge { node { id } } + } + } + `, map[string]any{ "input": map[string]any{ "documentVersionId": versionID, + "signatoryId": signatoryID, }, }) require.NoError(t, err) @@ -1707,3 +1809,121 @@ func TestDocumentVersion_ExportPDFSignatures(t *testing.T) { }, ) } + +// TestDocumentVersion_MajorPublishCancelsSignatureRequests verifies that +// publishing a new major version directly (no approvers) cancels the still +// pending signature requests attached to the previous major version. +func TestDocumentVersion_MajorPublishCancelsSignatureRequests(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + signer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + + docID, _ := createTestDocument(t, owner) + + v1ID := publishMajorDocumentVersion(t, owner, docID) + requestDocumentSignature(t, owner, v1ID, signer.GetProfileID().String()) + assertRequestedSignatureCount(t, owner, v1ID, 1) + + // A new major supersedes the previous one, so its pending request is cancelled. + updateDocumentContent(t, owner, docID, "Updated content for v2") + v2ID := publishMajorDocumentVersion(t, owner, docID) + + assertRequestedSignatureCount(t, owner, v1ID, 0) + assertRequestedSignatureCount(t, owner, v2ID, 0) +} + +// TestDocumentVersion_MinorPublishKeepsSignatureRequests verifies that +// publishing a minor version stays within the same major and therefore keeps +// pending signature requests intact. +func TestDocumentVersion_MinorPublishKeepsSignatureRequests(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + signer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + + docID, _ := createTestDocument(t, owner) + + v1ID := publishMajorDocumentVersion(t, owner, docID) + requestDocumentSignature(t, owner, v1ID, signer.GetProfileID().String()) + assertRequestedSignatureCount(t, owner, v1ID, 1) + + // A minor bump keeps the same major; the request must survive. + updateDocumentContent(t, owner, docID, "Updated content for 1.1") + publishMinorDocumentVersion(t, owner, docID) + + assertRequestedSignatureCount(t, owner, v1ID, 1) +} + +// TestDocumentVersion_MajorApprovalPublishCancelsSignatureRequests verifies +// that on the approval path the pending signature requests from the previous +// major are cancelled only once the quorum succeeds and the new major is +// actually published, not while the approval is still pending. +func TestDocumentVersion_MajorApprovalPublishCancelsSignatureRequests(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + signer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + + docID, _ := createTestDocument(t, owner) + approveTestDocument(t, owner, docID) + + v1ID := latestDocumentVersionID(t, owner, docID) + requestDocumentSignature(t, owner, v1ID, signer.GetProfileID().String()) + assertRequestedSignatureCount(t, owner, v1ID, 1) + + // Open a major approval for v2. While the quorum is pending the request + // from the previous major must remain untouched. + updateDocumentContent(t, owner, docID, "Updated content for v2") + requestDocumentApproval(t, owner, docID, []string{getOwnerProfileID(t, owner)}) + assertRequestedSignatureCount(t, owner, v1ID, 1) + + // Once the quorum succeeds and v2.0 is published, the request is cancelled. + approveLatestDocumentVersion(t, owner, docID) + assertRequestedSignatureCount(t, owner, v1ID, 0) +} + +// TestDocumentVersion_RequestSignatureIsIdempotentWithinVersion verifies that +// requesting a signature twice for the same signatory on the same version does +// not create a duplicate signature row. +func TestDocumentVersion_RequestSignatureIsIdempotentWithinVersion(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + signer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + + docID, _ := createTestDocument(t, owner) + signerID := signer.GetProfileID().String() + + v1ID := publishMajorDocumentVersion(t, owner, docID) + requestDocumentSignature(t, owner, v1ID, signerID) + requestDocumentSignature(t, owner, v1ID, signerID) + + assertRequestedSignatureCount(t, owner, v1ID, 1) +} + +// TestDocumentVersion_RequestSignatureDeduplicatesAcrossMinors verifies that +// re-requesting a signature on a newer minor of the same major reuses the +// signatory's existing signature instead of creating a second one, so the +// person is not listed twice in the major's export. +func TestDocumentVersion_RequestSignatureDeduplicatesAcrossMinors(t *testing.T) { + t.Parallel() + owner := testutil.NewClient(t, testutil.RoleOwner) + signer := testutil.NewClientInOrg(t, testutil.RoleViewer, owner) + + docID, _ := createTestDocument(t, owner) + signerID := signer.GetProfileID().String() + + v10ID := publishMajorDocumentVersion(t, owner, docID) + requestDocumentSignature(t, owner, v10ID, signerID) + assertRequestedSignatureCount(t, owner, v10ID, 1) + + // A minor bump stays in the same major; the request stays on v1.0. + updateDocumentContent(t, owner, docID, "Updated content for 1.1") + v11ID := publishMinorDocumentVersion(t, owner, docID) + + // Re-requesting on the newer minor must reuse the existing signature + // instead of inserting a duplicate within the same major. The signature + // query aggregates across every minor of the major, so a duplicate would + // surface as a major-wide REQUESTED count of 2; the fix keeps it at 1. + requestDocumentSignature(t, owner, v11ID, signerID) + + assertRequestedSignatureCount(t, owner, v10ID, 1) + assertRequestedSignatureCount(t, owner, v11ID, 1) +} diff --git a/pkg/coredata/document_version_signature.go b/pkg/coredata/document_version_signature.go index 0d1ff656d..03ca0739d 100644 --- a/pkg/coredata/document_version_signature.go +++ b/pkg/coredata/document_version_signature.go @@ -151,6 +151,86 @@ LIMIT 1 return nil } +// LoadByDocumentMajorAndSignatory loads the signatory's existing signature for +// the whole major that owns documentVersionID, scanning across every minor +// version of that major. A signed signature is preferred over a still pending +// one, then the most recent. It returns ErrResourceNotFound when the signatory +// has no signature anywhere in the major. +func (pvs *DocumentVersionSignature) LoadByDocumentMajorAndSignatory( + ctx context.Context, + conn pg.Querier, + scope Scoper, + documentVersionID gid.GID, + signatory gid.GID, +) error { + q := ` +WITH source_version AS ( + SELECT document_id, major FROM document_versions WHERE id = @document_version_id +), +major_versions AS ( + SELECT dv.id FROM document_versions dv + INNER JOIN source_version sv ON dv.document_id = sv.document_id AND dv.major = sv.major +), +major_signatures AS ( + SELECT + dvs.id, + dvs.organization_id, + dvs.tenant_id, + dvs.document_version_id, + dvs.state, + dvs.signed_by_profile_id, + dvs.signed_at, + dvs.requested_at, + dvs.created_at, + dvs.updated_at + FROM document_version_signatures dvs + INNER JOIN major_versions mv ON dvs.document_version_id = mv.id + WHERE dvs.signed_by_profile_id = @signatory +) +SELECT + id, + organization_id, + document_version_id, + state, + signed_by_profile_id, + signed_at, + requested_at, + created_at, + updated_at +FROM + major_signatures +WHERE + %s +ORDER BY + CASE state WHEN 'SIGNED' THEN 0 ELSE 1 END, + created_at DESC +LIMIT 1 +` + + q = fmt.Sprintf(q, scope.SQLFragment()) + + args := pgx.StrictNamedArgs{"document_version_id": documentVersionID, "signatory": signatory} + maps.Copy(args, scope.SQLArguments()) + + rows, err := conn.Query(ctx, q, args) + if err != nil { + return fmt.Errorf("cannot query document version signature by major: %w", err) + } + + documentVersionSignature, err := pgx.CollectOneRow(rows, pgx.RowToStructByName[DocumentVersionSignature]) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return ErrResourceNotFound + } + + return fmt.Errorf("cannot collect document version signature by major: %w", err) + } + + *pvs = documentVersionSignature + + return nil +} + func (pvs *DocumentVersionSignature) LoadByID( ctx context.Context, conn pg.Querier, @@ -402,6 +482,42 @@ WHERE return nil } +func (pvss *DocumentVersionSignatures) DeleteRequestedByDocumentIDBelowMajor( + ctx context.Context, + conn pg.Tx, + scope Scoper, + documentID gid.GID, + major int, +) error { + q := ` +DELETE FROM document_version_signatures +WHERE + %s + AND state = @state + AND document_version_id IN ( + SELECT id + FROM document_versions + WHERE document_id = @document_id + AND major < @major + ) +` + + q = fmt.Sprintf(q, scope.SQLFragment()) + + args := pgx.StrictNamedArgs{ + "document_id": documentID, + "major": major, + "state": DocumentVersionSignatureStateRequested, + } + maps.Copy(args, scope.SQLArguments()) + + if _, err := conn.Exec(ctx, q, args); err != nil { + return fmt.Errorf("cannot delete requested document version signatures from previous major versions: %w", err) + } + + return nil +} + func (pvss *DocumentVersionSignaturesWithPeople) LoadByDocumentVersionIDWithPeople( ctx context.Context, conn pg.Querier, @@ -433,6 +549,18 @@ signatures_with_people AS ( FROM document_version_signatures dvs INNER JOIN major_versions mv ON dvs.document_version_id = mv.id INNER JOIN iam_membership_profiles p ON dvs.signed_by_profile_id = p.id + WHERE + dvs.state = 'SIGNED' + OR ( + p.state = 'ACTIVE' + AND (p.contract_end_date IS NULL OR p.contract_end_date >= CURRENT_DATE) + AND EXISTS ( + SELECT 1 + FROM iam_memberships m + WHERE m.identity_id = p.identity_id + AND m.organization_id = p.organization_id + ) + ) ) SELECT id, diff --git a/pkg/coredata/document_version_signature_filter.go b/pkg/coredata/document_version_signature_filter.go index 61b673562..275a773a8 100644 --- a/pkg/coredata/document_version_signature_filter.go +++ b/pkg/coredata/document_version_signature_filter.go @@ -62,12 +62,12 @@ func (f *DocumentVersionSignatureFilter) SQLFragment() string { ( @active_contract::boolean = TRUE AND ( - p.contract_end_date IS NULL OR p.contract_end_date > NOW() + p.contract_end_date IS NULL OR p.contract_end_date >= CURRENT_DATE ) ) OR ( @active_contract::boolean = FALSE AND ( - p.contract_end_date IS NOT NULL AND p.contract_end_date <= NOW() + p.contract_end_date IS NOT NULL AND p.contract_end_date < CURRENT_DATE ) ) ) diff --git a/pkg/coredata/migrations/20260529T010239Z.sql b/pkg/coredata/migrations/20260529T010239Z.sql new file mode 100644 index 000000000..67953256e --- /dev/null +++ b/pkg/coredata/migrations/20260529T010239Z.sql @@ -0,0 +1,39 @@ +-- Copyright (c) 2026 Probo Inc . +-- +-- Permission to use, copy, modify, and/or distribute this software for any +-- purpose with or without fee is hereby granted, provided that the above +-- copyright notice and this permission notice appear in all copies. +-- +-- THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH +-- REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +-- AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT, +-- INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +-- LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR +-- OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +-- PERFORMANCE OF THIS SOFTWARE. + +-- A signature applies to a whole major version: minor publishes keep it and the +-- export unions signatures across every minor of the major. A signatory must +-- therefore have at most one signature per (document, major). Historically the +-- request guard was scoped to a single minor version, so re-requesting on a +-- newer minor (or twice on the same version) inserted duplicate rows, making a +-- person appear several times on the exported signature page. +-- +-- Collapse each (document, major, signatory) group to a single signature, +-- keeping the same row the application now prefers: a SIGNED signature over a +-- pending one, then the most recently created. +WITH ranked AS ( + SELECT + dvs.id, + ROW_NUMBER() OVER ( + PARTITION BY dv.document_id, dv.major, dvs.signed_by_profile_id + ORDER BY + CASE dvs.state WHEN 'SIGNED' THEN 0 ELSE 1 END, + dvs.created_at DESC, + dvs.id DESC + ) AS rn + FROM document_version_signatures dvs + INNER JOIN document_versions dv ON dvs.document_version_id = dv.id +) +DELETE FROM document_version_signatures +WHERE id IN (SELECT id FROM ranked WHERE rn > 1); diff --git a/pkg/probo/document_approval_service.go b/pkg/probo/document_approval_service.go index 8ddbba149..308788698 100644 --- a/pkg/probo/document_approval_service.go +++ b/pkg/probo/document_approval_service.go @@ -1005,5 +1005,9 @@ func (s *DocumentApprovalService) publishVersion( return fmt.Errorf("cannot finalize publish: %w", err) } + if err := s.svc.Documents.cancelPreviousMajorSignatureRequestsInTx(ctx, scope, tx, version.DocumentID, version.Major); err != nil { + return fmt.Errorf("cannot cancel signature requests from previous major versions: %w", err) + } + return nil } diff --git a/pkg/probo/document_service.go b/pkg/probo/document_service.go index 801f95405..5ac5e80ac 100644 --- a/pkg/probo/document_service.go +++ b/pkg/probo/document_service.go @@ -985,7 +985,7 @@ func (s *DocumentService) BulkRequestSignatures( } for _, signatoryID := range req.SignatoryIDs { - signature, err := s.createSignatureRequestInTx(ctx, scope, tx, documentVersion.ID, signatoryID, true) + signature, err := s.createSignatureRequestInTx(ctx, scope, tx, documentVersion.ID, signatoryID) if err != nil { return fmt.Errorf("cannot create signature request for document %q and signatory %q: %w", documentID, signatoryID, err) } @@ -1009,7 +1009,6 @@ func (s *DocumentService) createSignatureRequestInTx( tx pg.Tx, documentVersionID gid.GID, signatoryID gid.GID, - ignoreExisting bool, ) (*coredata.DocumentVersionSignature, error) { signatory := &coredata.MembershipProfile{} documentVersion := &coredata.DocumentVersion{} @@ -1022,12 +1021,20 @@ func (s *DocumentService) createSignatureRequestInTx( return nil, fmt.Errorf("cannot load signatory: %w", err) } + // A signature applies to the whole major version: minor publishes keep it + // and the export unions signatures across every minor of the major, so a + // signatory must have at most one signature per major. If one already + // exists anywhere in this major (requested or signed), reuse it instead of + // inserting a duplicate. existingSignature := &coredata.DocumentVersionSignature{} - err := existingSignature.LoadByDocumentVersionIDAndSignatory(ctx, tx, scope, documentVersionID, signatoryID) - if err == nil && ignoreExisting { + err := existingSignature.LoadByDocumentMajorAndSignatory(ctx, tx, scope, documentVersionID, signatoryID) + if err == nil { return existingSignature, nil } + if !errors.Is(err, coredata.ErrResourceNotFound) { + return nil, fmt.Errorf("cannot load existing signature for signatory: %w", err) + } documentVersionSignatureID := gid.New(scope.GetTenantID(), coredata.DocumentVersionSignatureEntityType) now := time.Now() @@ -1088,7 +1095,7 @@ func (s *DocumentService) RequestSignature( var err error - signature, err = s.createSignatureRequestInTx(ctx, scope, tx, req.DocumentVersionID, req.Signatory, false) + signature, err = s.createSignatureRequestInTx(ctx, scope, tx, req.DocumentVersionID, req.Signatory) if err != nil { return fmt.Errorf("cannot create signature request: %w", err) } @@ -2798,9 +2805,32 @@ func (s *DocumentService) publishMajorVersionInTx( return nil, nil, err } + if err := s.cancelPreviousMajorSignatureRequestsInTx(ctx, scope, tx, documentID, documentVersion.Major); err != nil { + return nil, nil, err + } + return document, documentVersion, nil } +// cancelPreviousMajorSignatureRequestsInTx cancels every still-pending +// signature request attached to a prior major version of the document. A new +// major supersedes the signing obligations of older majors, so their +// REQUESTED signatures must not linger. SIGNED signatures are left untouched +// to preserve the audit trail. +func (s *DocumentService) cancelPreviousMajorSignatureRequestsInTx( + ctx context.Context, scope coredata.Scoper, + tx pg.Tx, + documentID gid.GID, + major int, +) error { + signatures := &coredata.DocumentVersionSignatures{} + if err := signatures.DeleteRequestedByDocumentIDBelowMajor(ctx, tx, scope, documentID, major); err != nil { + return fmt.Errorf("cannot cancel signature requests from previous major versions: %w", err) + } + + return nil +} + func (s *DocumentService) publishMinorVersionInTx( ctx context.Context, scope coredata.Scoper, tx pg.Tx, diff --git a/pkg/probo/generated_document_service.go b/pkg/probo/generated_document_service.go index 6956aa242..f8877013e 100644 --- a/pkg/probo/generated_document_service.go +++ b/pkg/probo/generated_document_service.go @@ -3120,5 +3120,11 @@ func (s *GeneratedDocumentService) publishOrRequestApproval( return fmt.Errorf("cannot update document: %w", err) } + if !minor { + if err := s.svc.Documents.cancelPreviousMajorSignatureRequestsInTx(ctx, scope, tx, document.ID, version.Major); err != nil { + return fmt.Errorf("cannot cancel signature requests from previous major versions: %w", err) + } + } + return nil }