From 9a19bcc12caca4eb36b6e206546d5ce4ce692468 Mon Sep 17 00:00:00 2001 From: weichen Date: Sun, 13 Sep 2026 22:29:53 +0000 Subject: [PATCH] fix(docduty): revoke inactive slots and explicit duty changes Empty assigned heal now clears only exited or non-member reviewer/approver holders and never blanks author. Explicit reconcile uses the pre-update duty plus a project advisory lock so viewer demotion and transfers are not left to generic heal. Co-authored-by: Cursor --- internal/docduty/resolve.go | 26 ++++-- internal/docduty/resolve_test.go | 30 ++++++- internal/docduty/revoke.go | 113 +++++++++++++++++++++++ internal/docduty/revoke_test.go | 62 +++++++++++++ internal/docduty/store_pg_test.go | 144 ++++++++++++++++++++++++++++++ 5 files changed, 365 insertions(+), 10 deletions(-) create mode 100644 internal/docduty/revoke.go create mode 100644 internal/docduty/revoke_test.go diff --git a/internal/docduty/resolve.go b/internal/docduty/resolve.go index d994cad8..bdb02151 100644 --- a/internal/docduty/resolve.go +++ b/internal/docduty/resolve.go @@ -141,32 +141,40 @@ func DutyMissing(currentID int64, memberIDs map[int64]struct{}) bool { return !ok } -func healSlot(current, assigned Holder, memberIDs map[int64]struct{}) (Holder, bool) { +func healSlot(current, assigned Holder, memberIDs map[int64]struct{}, clearInactive bool) (Holder, bool) { if assigned.UserID > 0 { if current.UserID == assigned.UserID && current.Name == assigned.Name { return current, false } return assigned, true } - // assigned 为空：不臆造、不清空。在任成员原样保留；占位/非成员也不删除。 - _ = memberIDs - return current, false + if current.UserID <= 0 { + return current, false + } + if _, ok := memberIDs[current.UserID]; ok { + return current, false + } + if !clearInactive { + return current, false + } + return Holder{}, true } -// HealBinding 以项目 document_duty 为权威。assigned 非零则对齐（错 ID 或同 ID 旧名称）； -// assigned 为空才保留当前值，避免臆造或清空。 +// HealBinding 以项目 document_duty 为权威。assigned 非零则对齐（错 ID 或同 ID 旧名称）。 +// assigned 为空时：审核/批准仅清空已退出或非成员的失效 holder；在任成员保留以兼容未初始化 duty 的旧项目。 +// 编制槽位即使 assigned 为空也不清空，避免无意去掉 owner/author。 func HealBinding(current, assigned Binding, memberIDs map[int64]struct{}) (Binding, bool) { out := current changed := false - if next, ok := healSlot(current.Author, assigned.Author, memberIDs); ok { + if next, ok := healSlot(current.Author, assigned.Author, memberIDs, false); ok { out.Author = next changed = true } - if next, ok := healSlot(current.Reviewer, assigned.Reviewer, memberIDs); ok { + if next, ok := healSlot(current.Reviewer, assigned.Reviewer, memberIDs, true); ok { out.Reviewer = next changed = true } - if next, ok := healSlot(current.Approver, assigned.Approver, memberIDs); ok { + if next, ok := healSlot(current.Approver, assigned.Approver, memberIDs, true); ok { out.Approver = next changed = true } diff --git a/internal/docduty/resolve_test.go b/internal/docduty/resolve_test.go index b24d8ab3..8341d0eb 100644 --- a/internal/docduty/resolve_test.go +++ b/internal/docduty/resolve_test.go @@ -153,13 +153,41 @@ func TestHealBinding_EmptyAssignedKeepsActiveMember(t *testing.T) { } got, changed := HealBinding(current, Binding{}, memberIDs) if changed { - t.Fatalf("empty assigned must not invent or clear: %+v", got) + t.Fatalf("empty assigned must not invent or clear active members: %+v", got) } if got != current { t.Fatalf("empty assigned must keep current: %+v", got) } } +func TestHealBinding_EmptyAssignedClearsInactiveHolder(t *testing.T) { + memberIDs := map[int64]struct{}{170: {}} + current := Binding{ + Author: Holder{UserID: 170, Name: "编制人A"}, + Reviewer: Holder{UserID: 171, Name: "已退出审核人"}, + Approver: Holder{UserID: 2, Name: "审核负责人"}, + } + got, changed := HealBinding(current, Binding{}, memberIDs) + if !changed { + t.Fatal("inactive reviewer/approver must be cleared when assigned is empty") + } + if got.Reviewer.UserID != 0 || got.Approver.UserID != 0 { + t.Fatalf("want cleared review/approve slots, got %+v", got) + } + if got.Author.UserID != 170 { + t.Fatalf("author must stay: %+v", got.Author) + } +} + +func TestHealBinding_EmptyAssignedDoesNotClearAuthor(t *testing.T) { + memberIDs := map[int64]struct{}{} + current := Binding{Author: Holder{UserID: 170, Name: "编制人A"}} + got, changed := HealBinding(current, Binding{}, memberIDs) + if changed || got.Author.UserID != 170 { + t.Fatalf("author must not be cleared when assigned is empty: changed=%v got=%+v", changed, got) + } +} + func TestHealBinding_ZeroDutyBackfill(t *testing.T) { members, creator := sampleMembers() assigned := Resolve(members, creator, RequestDuties{}) diff --git a/internal/docduty/revoke.go b/internal/docduty/revoke.go new file mode 100644 index 00000000..fe8527d2 --- /dev/null +++ b/internal/docduty/revoke.go @@ -0,0 +1,113 @@ +package docduty + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5/pgxpool" +) + +// MemberDutyChange 成员职责/在任状态变更，供显式撤销文档槽位。 +type MemberDutyChange struct { + UserID int64 + PreviousDuty string + NextDuty string + Exited bool + PriorHolderIDs map[string]int64 +} + +// SlotRevocation 从受控文档上撤销某用户的审核/批准槽位；永不针对 author。 +type SlotRevocation struct { + UserID int64 + Duty string +} + +// PriorDutyHolders 更新前在任成员的职责持有者。 +func PriorDutyHolders(members []Member) map[string]int64 { + out := make(map[string]int64, 3) + for _, m := range members { + if !isActiveMember(m) { + continue + } + switch m.DocumentDuty { + case DutyAuthor, DutyReviewer, DutyApprover: + out[m.DocumentDuty] = m.UserID + } + } + return out +} + +// PlanSlotRevocations 按更新前 duty 显式计算要撤销的审核/批准槽位。 +// reviewer/approver → viewer/其他职责、职责转移、退出成员都会产生撤销；author 永不进入结果。 +func PlanSlotRevocations(change MemberDutyChange) []SlotRevocation { + seen := make(map[string]struct{}, 4) + out := make([]SlotRevocation, 0, 2) + add := func(userID int64, duty string) { + if userID <= 0 || (duty != DutyReviewer && duty != DutyApprover) { + return + } + key := fmt.Sprintf("%d:%s", userID, duty) + if _, ok := seen[key]; ok { + return + } + seen[key] = struct{}{} + out = append(out, SlotRevocation{UserID: userID, Duty: duty}) + } + if change.Exited || change.PreviousDuty != change.NextDuty { + add(change.UserID, change.PreviousDuty) + } + if change.NextDuty == DutyReviewer || change.NextDuty == DutyApprover { + if prior := change.PriorHolderIDs[change.NextDuty]; prior > 0 && prior != change.UserID { + add(prior, change.NextDuty) + } + } + return out +} + +// ReconcileMemberDuty 在项目级 advisory lock 下按更新前 duty 清空对应槽位，再 heal 新职责。 +func ReconcileMemberDuty(ctx context.Context, pool *pgxpool.Pool, projectID int64, change MemberDutyChange) error { + if pool == nil || projectID <= 0 { + return nil + } + tx, err := pool.Begin(ctx) + if err != nil { + return fmt.Errorf("docduty: begin reconcile: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + if _, err := tx.Exec(ctx, `SELECT pg_advisory_xact_lock($1)`, projectID); err != nil { + return fmt.Errorf("docduty: lock project %d: %w", projectID, err) + } + for _, rev := range PlanSlotRevocations(change) { + q, ok := clearDutySlotSQL(rev.Duty) + if !ok { + continue + } + if _, err := tx.Exec(ctx, q, projectID, rev.UserID); err != nil { + return fmt.Errorf("docduty: clear %s for user %d: %w", rev.Duty, rev.UserID, err) + } + } + if err := tx.Commit(ctx); err != nil { + return fmt.Errorf("docduty: commit reconcile: %w", err) + } + _, _, err = HealProjectDocuments(ctx, pool, projectID) + return err +} + +func clearDutySlotSQL(duty string) (string, bool) { + switch duty { + case DutyReviewer: + return ` + UPDATE document_records + SET reviewer_id = NULL, reviewer_name = '', updated_at = now() + WHERE project_id = $1 AND COALESCE(task_key, '') <> '' AND reviewer_id = $2 + `, true + case DutyApprover: + return ` + UPDATE document_records + SET approver_id = NULL, approver_name = '', updated_at = now() + WHERE project_id = $1 AND COALESCE(task_key, '') <> '' AND approver_id = $2 + `, true + default: + return "", false + } +} diff --git a/internal/docduty/revoke_test.go b/internal/docduty/revoke_test.go new file mode 100644 index 00000000..916fffeb --- /dev/null +++ b/internal/docduty/revoke_test.go @@ -0,0 +1,62 @@ +package docduty + +import "testing" + +func TestPlanSlotRevocations_ReviewerToViewer(t *testing.T) { + got := PlanSlotRevocations(MemberDutyChange{ + UserID: 171, PreviousDuty: DutyReviewer, NextDuty: DutyNone, + }) + if len(got) != 1 || got[0].UserID != 171 || got[0].Duty != DutyReviewer { + t.Fatalf("reviewer→viewer: %+v", got) + } +} + +func TestPlanSlotRevocations_DutyTransfer(t *testing.T) { + got := PlanSlotRevocations(MemberDutyChange{ + UserID: 99, PreviousDuty: DutyNone, NextDuty: DutyReviewer, + PriorHolderIDs: map[string]int64{DutyReviewer: 171}, + }) + if len(got) != 1 || got[0].UserID != 171 || got[0].Duty != DutyReviewer { + t.Fatalf("transfer must revoke prior reviewer: %+v", got) + } +} + +func TestPlanSlotRevocations_ExitedMember(t *testing.T) { + got := PlanSlotRevocations(MemberDutyChange{ + UserID: 171, PreviousDuty: DutyReviewer, NextDuty: DutyReviewer, Exited: true, + }) + if len(got) != 1 || got[0].Duty != DutyReviewer { + t.Fatalf("exited reviewer must revoke: %+v", got) + } +} + +func TestPlanSlotRevocations_IdempotentEmpty(t *testing.T) { + got := PlanSlotRevocations(MemberDutyChange{UserID: 171, PreviousDuty: DutyNone, NextDuty: DutyNone}) + if len(got) != 0 { + t.Fatalf("repeat empty change must be no-op: %+v", got) + } +} + +func TestPlanSlotRevocations_NeverRevokesAuthor(t *testing.T) { + got := PlanSlotRevocations(MemberDutyChange{ + UserID: 170, PreviousDuty: DutyAuthor, NextDuty: DutyNone, + PriorHolderIDs: map[string]int64{DutyAuthor: 170}, + }) + if len(got) != 0 { + t.Fatalf("author must not be revoked: %+v", got) + } +} + +func TestPriorDutyHolders_IgnoresExited(t *testing.T) { + holders := PriorDutyHolders([]Member{ + {UserID: 170, Status: StatusActive, DocumentDuty: DutyAuthor}, + {UserID: 171, Status: "exited", DocumentDuty: DutyReviewer}, + {UserID: 4, Status: StatusActive, DocumentDuty: DutyApprover}, + }) + if holders[DutyAuthor] != 170 || holders[DutyApprover] != 4 { + t.Fatalf("holders=%v", holders) + } + if _, ok := holders[DutyReviewer]; ok { + t.Fatal("exited reviewer must not be a prior holder") + } +} diff --git a/internal/docduty/store_pg_test.go b/internal/docduty/store_pg_test.go index 0b8cb796..9cf451c2 100644 --- a/internal/docduty/store_pg_test.go +++ b/internal/docduty/store_pg_test.go @@ -163,6 +163,150 @@ func TestHealBinding_EmptyAssignedKeepsMember_PG(t *testing.T) { assertDocApprover(t, ctx, pool, docID, memberID) } +func TestHealBinding_EmptyAssignedClearsExited_PG(t *testing.T) { + ctx, _, pool := openDutyTestPool(t) + suffix := time.Now().UnixNano() + authorID := insertDutyUser(t, ctx, pool, suffix, "ex_author") + exitedID := insertDutyUser(t, ctx, pool, suffix, "ex_reviewer") + + var projectID int64 + if err := pool.QueryRow(ctx, ` + INSERT INTO projects (code, name, status, creator_user_id) + VALUES ($1, $2, 'active', $3) + RETURNING id + `, fmt.Sprintf("duty-exited-%d", suffix), "clear exited", authorID).Scan(&projectID); err != nil { + t.Fatalf("insert project: %v", err) + } + t.Cleanup(func() { + _, _ = pool.Exec(context.Background(), `DELETE FROM document_records WHERE project_id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM project_members WHERE project_id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM projects WHERE id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM users WHERE id IN ($1,$2)`, authorID, exitedID) + }) + if _, err := pool.Exec(ctx, ` + INSERT INTO project_members (project_id, user_id, project_role, document_duty, status, joined_at, updated_at) + VALUES ($1, $2, 'owner', 'author', 'active', now(), now()), + ($1, $3, 'member', '', 'exited', now(), now()) + `, projectID, authorID, exitedID); err != nil { + t.Fatalf("insert members: %v", err) + } + docID := fmt.Sprintf("doc_proj%d_intake", projectID) + insertDutyDoc(t, ctx, pool, projectID, docID, "intake", authorID, exitedID, 0) + if _, err := pool.Exec(ctx, `UPDATE document_records SET author_name = $2 WHERE doc_id = $1`, docID, "用户ex_author"); err != nil { + t.Fatal(err) + } + + _, n, err := HealProjectDocuments(ctx, pool, projectID) + if err != nil { + t.Fatal(err) + } + if n == 0 { + t.Fatal("exited reviewer must be cleared when assigned is empty") + } + assertDocReviewer(t, ctx, pool, docID, 0) + assertDocAuthorName(t, ctx, pool, docID, "用户ex_author") +} + +func TestReconcileMemberDuty_ReviewerToViewerAndTransfer_PG(t *testing.T) { + ctx, _, pool := openDutyTestPool(t) + suffix := time.Now().UnixNano() + authorID := insertDutyUser(t, ctx, pool, suffix, "rec_author") + reviewerA := insertDutyUser(t, ctx, pool, suffix, "rec_ra") + reviewerB := insertDutyUser(t, ctx, pool, suffix, "rec_rb") + + var projectID int64 + if err := pool.QueryRow(ctx, ` + INSERT INTO projects (code, name, status, creator_user_id) + VALUES ($1, $2, 'active', $3) + RETURNING id + `, fmt.Sprintf("duty-rec-%d", suffix), "explicit reconcile", authorID).Scan(&projectID); err != nil { + t.Fatalf("insert project: %v", err) + } + t.Cleanup(func() { + _, _ = pool.Exec(context.Background(), `DELETE FROM document_records WHERE project_id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM project_members WHERE project_id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM projects WHERE id = $1`, projectID) + _, _ = pool.Exec(context.Background(), `DELETE FROM users WHERE id IN ($1,$2,$3)`, authorID, reviewerA, reviewerB) + }) + if _, err := pool.Exec(ctx, ` + INSERT INTO project_members (project_id, user_id, project_role, document_duty, status, joined_at, updated_at) + VALUES ($1, $2, 'owner', 'author', 'active', now(), now()), + ($1, $3, 'member', 'reviewer', 'active', now(), now()), + ($1, $4, 'member', '', 'active', now(), now()) + `, projectID, authorID, reviewerA, reviewerB); err != nil { + t.Fatalf("insert members: %v", err) + } + docID := fmt.Sprintf("doc_proj%d_intake", projectID) + insertDutyDoc(t, ctx, pool, projectID, docID, "intake", authorID, reviewerA, 0) + + if _, err := pool.Exec(ctx, ` + UPDATE project_members SET document_duty = '' WHERE project_id = $1 AND user_id = $2 + `, projectID, reviewerA); err != nil { + t.Fatal(err) + } + if err := ReconcileMemberDuty(ctx, pool, projectID, MemberDutyChange{ + UserID: reviewerA, PreviousDuty: DutyReviewer, NextDuty: DutyNone, + }); err != nil { + t.Fatal(err) + } + assertDocReviewer(t, ctx, pool, docID, 0) + var authorGot int64 + if err := pool.QueryRow(ctx, `SELECT COALESCE(author_id, 0) FROM document_records WHERE doc_id = $1`, docID).Scan(&authorGot); err != nil { + t.Fatal(err) + } + if authorGot != authorID { + t.Fatalf("author must stay %d, got %d", authorID, authorGot) + } + + if _, err := pool.Exec(ctx, ` + UPDATE project_members SET document_duty = '' WHERE project_id = $1 AND user_id = $2 + `, projectID, reviewerA); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, ` + UPDATE project_members SET document_duty = 'reviewer' WHERE project_id = $1 AND user_id = $2 + `, projectID, reviewerB); err != nil { + t.Fatal(err) + } + if _, err := pool.Exec(ctx, ` + UPDATE document_records SET reviewer_id = $2, reviewer_name = 'A' WHERE doc_id = $1 + `, docID, reviewerA); err != nil { + t.Fatal(err) + } + if err := ReconcileMemberDuty(ctx, pool, projectID, MemberDutyChange{ + UserID: reviewerB, PreviousDuty: DutyNone, NextDuty: DutyReviewer, + PriorHolderIDs: map[string]int64{DutyReviewer: reviewerA}, + }); err != nil { + t.Fatal(err) + } + assertDocReviewer(t, ctx, pool, docID, reviewerB) + + if err := ReconcileMemberDuty(ctx, pool, projectID, MemberDutyChange{ + UserID: reviewerB, PreviousDuty: DutyReviewer, NextDuty: DutyReviewer, + }); err != nil { + t.Fatal(err) + } + assertDocReviewer(t, ctx, pool, docID, reviewerB) + + if _, err := pool.Exec(ctx, ` + UPDATE project_members SET status = 'exited', document_duty = 'reviewer' WHERE project_id = $1 AND user_id = $2 + `, projectID, reviewerB); err != nil { + t.Fatal(err) + } + if err := ReconcileMemberDuty(ctx, pool, projectID, MemberDutyChange{ + UserID: reviewerB, PreviousDuty: DutyReviewer, NextDuty: DutyReviewer, Exited: true, + }); err != nil { + t.Fatal(err) + } + assertDocReviewer(t, ctx, pool, docID, 0) + if err := ReconcileMemberDuty(ctx, pool, projectID, MemberDutyChange{ + UserID: reviewerB, PreviousDuty: DutyReviewer, NextDuty: DutyReviewer, Exited: true, + }); err != nil { + t.Fatal(err) + } + assertDocReviewer(t, ctx, pool, docID, 0) +} + func TestOwnerAuthorBackfill_MultiOwnerDeterministic_PG(t *testing.T) { ctx, _, pool := openDutyTestPool(t) suffix := time.Now().UnixNano()