rohit/probo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Scope document signatures to the major version

Browse Source 
A signature applies to a whole major: minor publishes keep it and the
export unions signatures across every minor of the major. The request
guard was scoped to a single minor, so re-requesting on a newer minor
(or twice on the same version) inserted duplicate rows and a signatory
appeared several times on the exported signature page.

Deduplicate by loading any existing signature across the major before
inserting, cancel still-pending requests from prior majors when a new
major is published, and restrict the export to active signatories
(comparing contract end dates against the current date). A migration
collapses the duplicate rows already in the table, preferring a signed
row over a pending one and then the most recent.

Signed-off-by: Bryan Frimin <bryan@probo.com>
This commit is contained in:
Bryan Frimin
2026-05-28 18:12:32 -07:00
parent 4efba328a6
commit 1a8ca29264
7 changed files with 489 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files

330
e2e/console/document_version_test.go
View File

@@ -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)
}

128
pkg/coredata/document_version_signature.go
View File

@@ -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,

4
pkg/coredata/document_version_signature_filter.go
View File

@@ -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
)
)
)

39
pkg/coredata/migrations/20260529T010239Z.sql Normal file
View File

@@ -0,0 +1,39 @@
-- Copyright (c) 2026 Probo Inc <hello@getprobo.com>.
--
-- 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);

4
pkg/probo/document_approval_service.go
View File

@@ -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
}

40
pkg/probo/document_service.go
View File

@@ -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,

6
pkg/probo/generated_document_service.go
View File

@@ -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
}