refactor: update approval record schema and related functions #530
Conversation
- Changed the approval record schema from `policyRuleAnyApprovalRecord` to `deploymentVersionApprovalRecord`. - Removed unused base approval record definitions and related validation fields. - Updated approval record retrieval functions to use the new schema and improved parameter handling for role and user approvals. - Cleaned up imports and adjusted related types for consistency.
WalkthroughThis change consolidates approval record management by removing the old Changes
Sequence Diagram(s)sequenceDiagram
participant API
participant DB
participant RuleEngine
API->>DB: Insert into deploymentVersionApprovalRecord (on approval)
RuleEngine->>DB: Query deploymentVersionApprovalRecord for approvals
DB-->>API: Return approval record
DB-->>RuleEngine: Return approval records
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/db/src/schema/rules/approval-record.ts (2)
36-38:approvedAtdefaulting toNULLis redundantIn Postgres, the absence of a default already yields
NULL. Dropping the explicitsql\NULL`` simplifies the DDL and avoids surprising diffs when generating migrations.
52-55: Consider a dedicated index ondeploymentVersionIdfor query speedAll read paths shown in
version-approval-rule.tsfilter bydeploymentVersionIdonly.
A composite UNIQUE index (deploymentVersionId, userId) cannot be used by Postgres for single-column look-ups, so large installations could see a full-index scan.uniqueDeploymentVersionIdUserId: uniqueIndex( "unique_deployment_version_id_user_id", ).on(t.deploymentVersionId, t.userId), + + // Speeds up WHERE ... deployment_version_id IN (...) + byDeploymentVersionId: index("idx_approval_record_deployment_version_id").on( + t.deploymentVersionId, + ),packages/rule-engine/src/rules/version-approval-rule.ts (2)
42-55: Quadratic filtering – convert to a map for O(n) look-ups
records.filter(…)insidecandidates.filteryields O(n²) behaviour.
Building aMap<deploymentVersionId, ApprovalRecord[]>once avoids repeated scans.- const allowedCandidates = candidates.filter((release) => { - const records = approvalRecords.filter( - (r) => r.deploymentVersionId === release.id, - ); + const recordsByVersion = _.groupBy( + approvalRecords, + (r) => r.deploymentVersionId, + ); + + const allowedCandidates = candidates.filter((release) => { + const records = recordsByVersion[release.id] ?? [];
114-125: Duplicate logic – reusegetAnyApprovalRecords
getUserApprovalRecordsFuncrepeats theinArray + userIdfilter already possible by composinggetAnyApprovalRecordsand an in-memory filter. Extracting a shared low-level fetch keeps DB logic in one place and simplifies testability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/api/src/router/deployment-version-checks/approvals.ts(1 hunks)packages/db/src/schema/rbac.ts(1 hunks)packages/db/src/schema/rules/approval-any.ts(1 hunks)packages/db/src/schema/rules/approval-base.ts(0 hunks)packages/db/src/schema/rules/approval-record.ts(1 hunks)packages/db/src/schema/rules/approval-role.ts(0 hunks)packages/db/src/schema/rules/approval-user.ts(0 hunks)packages/db/src/schema/rules/index.ts(1 hunks)packages/rule-engine/src/manager/version-manager-rules.ts(3 hunks)packages/rule-engine/src/rules/version-approval-rule.ts(4 hunks)
💤 Files with no reviewable changes (3)
- packages/db/src/schema/rules/approval-user.ts
- packages/db/src/schema/rules/approval-base.ts
- packages/db/src/schema/rules/approval-role.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/api/src/router/deployment-version-checks/approvals.tspackages/db/src/schema/rules/index.tspackages/db/src/schema/rbac.tspackages/rule-engine/src/manager/version-manager-rules.tspackages/db/src/schema/rules/approval-any.tspackages/rule-engine/src/rules/version-approval-rule.tspackages/db/src/schema/rules/approval-record.ts
🧬 Code Graph Analysis (3)
packages/rule-engine/src/manager/version-manager-rules.ts (1)
packages/rule-engine/src/rules/version-approval-rule.ts (3)
VersionApprovalRule(32-72)getRoleApprovalRecordsFunc(85-112)getUserApprovalRecordsFunc(114-125)
packages/rule-engine/src/rules/version-approval-rule.ts (3)
packages/rule-engine/src/types.ts (1)
RuleEngineContext(23-28)packages/db/src/schema/release.ts (1)
release(113-124)packages/db/src/client.ts (1)
db(15-15)
packages/db/src/schema/rules/approval-record.ts (1)
packages/db/src/schema/auth.ts (1)
user(27-44)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
packages/db/src/schema/rules/index.ts (1)
9-9: Export of new approval record module.This export adds the new unified schema for deployment version approval records, aligning with the refactoring to consolidate approval record management into a single table and schema.
packages/api/src/router/deployment-version-checks/approvals.ts (1)
87-87: Updated insert target to use the new unified approval record table.The mutation now correctly inserts records into the new
deploymentVersionApprovalRecordtable instead of the previous approval record table, consistent with the schema consolidation.packages/db/src/schema/rbac.ts (1)
34-37: Addition of EntityTypeEnum for strongly typed entity references.The new enum provides a strongly typed alternative to the string literal values, improving type safety when filtering approval records by entity types in the rule engine.
packages/rule-engine/src/manager/version-manager-rules.ts (3)
6-7: Updated imports to use the new approval record retrieval functions.These imports correctly reference the renamed higher-order functions for role and user approval record retrieval.
40-44: Updated role approval rule implementation to use the new function-returning paradigm.The code now correctly destructures the input object to get
roleIdandrequiredApprovalsCount, and uses the higher-order functiongetRoleApprovalRecordsFuncto create a specialized approval record retrieval function for the specific role.
53-57: Updated user approval rule implementation to use the new function-returning paradigm.Similar to the role approval changes, this code now correctly destructures the input object to get
userIdand uses the higher-order functiongetUserApprovalRecordsFuncto create a specialized approval record retrieval function for the specific user.packages/db/src/schema/rules/approval-any.ts (1)
12-19: Looks correct – table definition and validation in sync
requiredApprovalsCountis constrained both at the DB level (.notNull().default(1)) and at the Zod level (.min(1).default(1)), giving good defence-in-depth.
No further issues spotted in this segment.packages/rule-engine/src/rules/version-approval-rule.ts (1)
85-112: Strengthen join condition & param naming
- The parameter is called
versionIdsbut elsewhere the refactor standardises ondeploymentVersionIds; keep naming consistent.- Joining
entityRole.entityIdtouserIdis correct, but adding an expliciteq(entityRole.workspaceId, context.environment.workspaceId)(if available in your schema) prevents cross-workspace leakage.
| // Base validation fields for approval records | ||
| export const baseApprovalRecordValidationFields = { | ||
| deploymentVersionId: z.string().uuid(), | ||
| userId: z.string().uuid(), | ||
| status: z.enum(approvalStatus.enumValues), | ||
| reason: z.string().optional(), | ||
| }; |
There was a problem hiding this comment.
Validation schema allows undefined but not null for reason
DB column reason is nullable; inserting null directly will violate Zod.
- reason: z.string().optional(),
+ reason: z.string().nullable().optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Base validation fields for approval records | |
| export const baseApprovalRecordValidationFields = { | |
| deploymentVersionId: z.string().uuid(), | |
| userId: z.string().uuid(), | |
| status: z.enum(approvalStatus.enumValues), | |
| reason: z.string().optional(), | |
| }; | |
| // Base validation fields for approval records | |
| export const baseApprovalRecordValidationFields = { | |
| deploymentVersionId: z.string().uuid(), | |
| userId: z.string().uuid(), | |
| status: z.enum(approvalStatus.enumValues), | |
| reason: z.string().nullable().optional(), | |
| }; |
| deploymentVersionId: uuid("deployment_version_id").notNull(), | ||
|
|
||
| // User who performed the approval/rejection action | ||
| userId: uuid("user_id") | ||
| .references(() => user.id) | ||
| .notNull(), | ||
|
|
There was a problem hiding this comment.
Missing FK to the deployment-version table may break referential integrity
deploymentVersionId is declared as a bare UUID. Without a .references(() => deploymentVersion.id …) clause the DB won’t enforce that the referenced deployment version actually exists, and cascaded deletes/updates are impossible.
- deploymentVersionId: uuid("deployment_version_id").notNull(),
+ deploymentVersionId: uuid("deployment_version_id")
+ .notNull()
+ .references(() => deploymentVersion.id, { onDelete: "cascade" }),(you’ll need to import deploymentVersion from its schema module at the top).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deploymentVersionId: uuid("deployment_version_id").notNull(), | |
| // User who performed the approval/rejection action | |
| userId: uuid("user_id") | |
| .references(() => user.id) | |
| .notNull(), | |
| deploymentVersionId: uuid("deployment_version_id") | |
| .notNull() | |
| .references(() => deploymentVersion.id, { onDelete: "cascade" }), | |
| // User who performed the approval/rejection action | |
| userId: uuid("user_id") | |
| .references(() => user.id) | |
| .notNull(), |
| deploymentVersionId: string; | ||
| status: "approved" | "rejected"; | ||
| userId: string; | ||
| reason: string | null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing the built-in Record utility type
Using the name Record hides the standard TS Record<K,V> helper and can trip up IDE auto-completion.
-type Record = {
+type ApprovalRecord = {
deploymentVersionId: string;
status: "approved" | "rejected";
userId: string;
reason: string | null;
};(remember to adjust other references).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deploymentVersionId: string; | |
| status: "approved" | "rejected"; | |
| userId: string; | |
| reason: string | null; | |
| }; | |
| type ApprovalRecord = { | |
| deploymentVersionId: string; | |
| status: "approved" | "rejected"; | |
| userId: string; | |
| reason: string | null; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/db/src/schema/rules/rule-relations.ts (1)
71-83: Consider explicit cascade rules for referential integrityWhen defining the
deploymentVersionApprovalRecordrelations you may want to specifyonDelete/onUpdatebehaviour so that dangling approval-records don’t linger if a deployment version or user is removed.deploymentVersion: one(deploymentVersion, { fields: [deploymentVersionApprovalRecord.deploymentVersionId], references: [deploymentVersion.id], + onDelete: "cascade", }), user: one(user, { fields: [deploymentVersionApprovalRecord.userId], references: [user.id], + onDelete: "cascade", }),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/db/src/schema/rules/rule-relations.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/rules/rule-relations.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (2)
packages/db/src/schema/rules/rule-relations.ts (2)
13-16: Verify downstream renaming touser.approvalRecords
userApprovalRelationsnow exposes a singleapprovalRecordscollection that points todeploymentVersionApprovalRecord.
Please confirm every caller that previously accesseduser.[any|role|user]ApprovalRecords(or similar) has been migrated to the new property; otherwise those look-ups will fail at runtime.
6-9:✅ Verification successful
Confirm obsolete schema imports haven’t crept back in
The refactor description states that the
policyRule*ApprovalRecordtables were removed, yet we still importpolicyRuleAnyApproval,policyRuleRoleApproval, andpolicyRuleUserApproval.
If these modules were also meant to be retired (or renamed), the build will break once the files disappear. Please double-check that the three imports – and the relation blocks that use them – really are still needed.
🏁 Script executed:
#!/bin/bash # List any remaining approval-record-style schema files – output should be empty. fd -a "policyRule*ApprovalRecord" packages/db/src/schema || true # Confirm the three approval schema files still exist. fd -a "approval-{any,role,user}.ts" packages/db/src/schema/rulesLength of output: 449
Let’s re-run with glob patterns to accurately check for both the removed records and the existing rule files:
🏁 Script executed:
#!/bin/bash # 1) Look for any leftover ApprovalRecord schema files (should be none) fd --glob 'policyRule*ApprovalRecord.ts' packages/db/src/schema || echo "No ApprovalRecord files found." # 2) Verify the three approval-rule modules still exist fd --glob 'approval-any.ts' packages/db/src/schema/rules || echo "approval-any.ts missing!" fd --glob 'approval-role.ts' packages/db/src/schema/rules || echo "approval-role.ts missing!" fd --glob 'approval-user.ts' packages/db/src/schema/rules || echo "approval-user.ts missing!"Length of output: 375
No obsolete schema imports detected
All
policyRule*ApprovalRecord.tsfiles have been removed, and the three imported modules still exist:
- packages/db/src/schema/rules/approval-any.ts
- packages/db/src/schema/rules/approval-role.ts
- packages/db/src/schema/rules/approval-user.ts
The imports in
packages/db/src/schema/rules/rule-relations.tsare valid and required.
1 participant
Summary by CodeRabbit