Add statuses and keep track of policies across controls - #64
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces several new server‐side actions and hooks for handling organization control requirements and progress. New functions are added to create control requirements based on user frameworks, and endpoints are provided to fetch progress and detailed requirement data. The UI components have been enhanced to display control statuses and additional requirement details. Additionally, status representations, localization entries, database migration scripts, and schema updates have been modified to support the new data structures and relationships. Package dependencies were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FrameworkAction as selectFrameworksAction
participant OrgCtrlReq as createOrganizationControlRequirements
participant DB
User->>FrameworkAction: Initiates framework selection
FrameworkAction->>DB: Create frameworks and policies
FrameworkAction->>OrgCtrlReq: Call to create control requirements
OrgCtrlReq->>DB: Insert organization control requirements
DB-->>OrgCtrlReq: Confirmation
OrgCtrlReq-->>FrameworkAction: Requirements created
FrameworkAction-->>User: Completion response
sequenceDiagram
participant UI as SingleControl Component
participant Hook1 as useOrganizationControlProgress
participant Hook2 as useOrganizationControlRequirements
participant Server as Server Actions
participant DB
UI->>Hook1: Request control progress (controlId)
Hook1->>Server: Call getOrganizationControlProgress
Server->>DB: Query progress data
DB-->>Server: Return progress data
Server-->>Hook1: Return progress result
Hook1-->>UI: Provide progress details
UI->>Hook2: Request control requirements (controlId)
Hook2->>Server: Call getOrganizationControlRequirements
Server->>DB: Query requirement data
DB-->>Server: Return requirement details
Server-->>Hook2: Return requirement result
Hook2-->>UI: Provide requirement details
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (24)
apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Actions/getOrganizationControlsProgress.ts (6)
7-18: Consider adding JSDoc/TSDoc for theControlProgressinterface.This interface is an excellent abstraction for reporting control completion progress. Documenting its properties with descriptions (e.g., what "byType" tracks or how "progress" is derived) helps ensure clarity and easier maintenance.
20-28: Validate the input size.The schema ensures
controlIdsis an array of strings, but there is no check on array length. If the client sends an extremely large array, the query might become expensive or even exceed parameter limits. Consider adding a length constraint or chunking to handle large volumes gracefully..schema(z.object({ - controlIds: z.array(z.string()) + controlIds: z.array(z.string()).max(1000, "Too many IDs. Please limit to 1000.") }))
33-37: Improve authorization error reporting.This block correctly checks for
user.organizationIdbut simply returns a string for the error. Returning structured error data or throwing a dedicated error object might be more consistent with the rest of your codebase. You could also consider logging the user's ID for diagnostics.
39-49: Consider handling large result sets.Retrieving many organization control requirements in one query could affect performance. If there's a chance the list is long, consider pagination, filtering, or batched processing to reduce load on the database.
64-101: Consider future extensibility for requirement completion logic.Different requirement types (policy, file, evidence, etc.) are handled with a switch statement. If new types are introduced later, you’ll need to extend this logic. Centralizing or delegating each type’s "completion" condition—for instance, via a pluggable strategy object—can make future maintenance simpler.
116-122: Refine error handling.Using
console.erroris fine for immediate debugging, but you may want to include more structured logging (e.g., using a logger library) to provide better observability. Consider returning consistent error objects or status codes to your consumers for clarity.apps/app/src/components/tables/frameworks/columns.tsx (4)
22-34: Document the additional properties inOrganizationControlType.The added fields
name,id,category, andrequirementsexpand the data model significantly. Adding inline comments or a dedicated docstring clarifies their purpose, possible values, and behaviors for collaborators.
37-59: Avoid duplicating completion logic in multiple places.
getControlStatuscomputes total vs. completed requirements to return a status. Later in the file, lines 103-115 repeat similar logic to derive completed vs. total. Returning both the status and some numeric details fromgetControlStatuswould help centralize logic and reduce duplication.-function getControlStatus(requirements: OrganizationControlType["requirements"]): StatusType { +function getControlStatus(requirements: OrganizationControlType["requirements"]): + { status: StatusType; completed: number; total: number } { if (!requirements || requirements.length === 0) { - return "not_started"; + return { status: "not_started", completed: 0, total: 0 }; } ... return { - "in_progress"; + status: "in_progress", + completed: completedRequirements, + total: totalRequirements }; }
100-115: Consolidate the requirement filtering logic.You are repeating the condition to filter completed requirements. Considering you already extracted a helper above (
getControlStatus), you can unify this approach. It prevents any mismatch if you add or modify requirement types later.
118-141: Consider localizing the tooltip content."Progress:", "Completed:", and "requirements" are currently hardcoded strings, which might not be localized for users in other locales. If you have multi-language support, wrapping them in translation calls provides a more consistent experience.
apps/app/src/components/frameworks/framework-status.tsx (1)
16-17: Map all status types inSTATUS_COLORScarefully.
draftandpublishedare excluded fromStatusTypeusingExclude, but if they're ever reintroduced or used in a similar color map, it could cause confusion. Keep an eye on this to avoid any unhandled status scenario in the future.apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/hooks/useOrganizationControlProgress.ts (2)
13-13: Consider destructuring nested data property.The return statement accesses a nested
dataproperty twice (result.data.data). This could be simplified for better readability.- return result.data.data; + const { data: { data: progressData } } = result; + return progressData;
9-11: Consider more specific error message.The error message could be more descriptive by including the
controlIdin the error message for better debugging.- throw new Error("Failed to fetch control progress"); + throw new Error(`Failed to fetch control progress for controlId: ${controlId}`);apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/hooks/useOrganizationControlsProgress.ts (2)
32-38: Consider memoizing the transformed data.The data transformation using reduce runs on every render. Consider using useMemo to optimize performance.
+import { useMemo } from 'react'; export function useOrganizationControlsProgress(controlIds: string[]) { const { data, error, isLoading, mutate } = useSWR<ControlProgress[]>( controlIds.length > 0 ? ["organization-controls-progress", controlIds] : null, () => fetchOrganizationControlsProgress(controlIds), { revalidateOnFocus: false, revalidateOnReconnect: false, } ); + const transformedData = useMemo(() => + data?.reduce( + (acc, curr) => { + acc[curr.controlId] = curr; + return acc; + }, + {} as Record<string, ControlProgress> + ), + [data] + ); return { - data: data?.reduce( - (acc, curr) => { - acc[curr.controlId] = curr; - return acc; - }, - {} as Record<string, ControlProgress> - ), + data: transformedData, isLoading, error, mutate, }; }
12-14: Consider more specific error message.The error message could be more descriptive by including the controlIds in the error message for better debugging.
- throw new Error("Failed to fetch controls progress"); + throw new Error(`Failed to fetch controls progress for controlIds: ${controlIds.join(', ')}`);apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policy.ts (1)
53-56: Update error message to match function's purpose.The error message "Failed to fetch policy statistics" is misleading as this function fetches a policy, not policy statistics.
Apply this diff to improve the error message:
return { success: false, - error: "Failed to fetch policy statistics", + error: "Failed to fetch policy", };apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Actions/getOrganizationControlRequirements.ts (1)
4-8: Remove unused type imports.The types
ControlRequirement,OrganizationControlRequirement, andPolicyare imported but never used.Apply this diff to remove unused imports:
import type { - ControlRequirement, - OrganizationControlRequirement, - Policy, } from "@bubba/db";apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/actions/publish-policy.ts (1)
37-45: Simplify policy query.The
selectclause is unnecessary since we only need theid. Also, the non-null assertion onorganizationIdis redundant due to the prior check.Apply this diff to simplify the query:
const policy = await db.organizationPolicy.findFirst({ where: { policyId: id, - organizationId: user.organizationId!, + organizationId: user.organizationId, - }, - select: { - id: true, }, });apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Actions/getOrganizationControlProgress.ts (1)
67-81: Consider refactoring the completion check logic for better maintainability.The switch statement could be simplified and the "published" status could be defined as a constant.
+const POLICY_PUBLISHED_STATUS = "published"; +const COMPLETION_CRITERIA = { + policy: (req) => req.organizationPolicy?.status === POLICY_PUBLISHED_STATUS, + file: (req) => !!req.fileUrl, + evidence: (req) => !!req.content, + default: (req) => req.published +}; + let isCompleted = false; -switch (requirement.type) { - case "policy": - isCompleted = - requirement.organizationPolicy?.status === "published"; - break; - case "file": - isCompleted = !!requirement.fileUrl; - break; - case "evidence": - isCompleted = !!requirement.content; - break; - default: - isCompleted = requirement.published; -} +isCompleted = (COMPLETION_CRITERIA[requirement.type] || COMPLETION_CRITERIA.default)(requirement);apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/SingleControl.tsx (3)
29-34: Improve progress status calculation.The current implementation can be simplified using a more concise approach.
- const progressStatus = - controlProgress?.progress?.completed > 0 - ? "in_progress" - : controlProgress?.progress?.completed === 0 - ? "not_started" - : "completed"; + const progressStatus = controlProgress?.progress?.completed === 0 + ? "not_started" + : controlProgress?.progress?.completed === controlProgress?.progress?.total + ? "completed" + : "in_progress";
79-82: Improve URL generation for requirements.Consider extracting the URL generation logic into a separate function for better maintainability.
- const url = - requirement.type === "policy" - ? `/policies/${requirement.organizationPolicy?.policy?.id}` - : "_blank"; + const getRequirementUrl = (requirement: any) => { + if (requirement.type === "policy" && requirement.organizationPolicy?.policy?.id) { + return `/policies/${requirement.organizationPolicy.policy.id}`; + } + return "_blank"; + }; + const url = getRequirementUrl(requirement);
84-87: Improve completion status check.Consider extracting the completion status check into a separate function for better readability and reusability.
- const isCompleted = - requirement.type === "policy" - ? requirement.organizationPolicy?.status === "published" - : false; + const isRequirementCompleted = (requirement: any) => { + if (requirement.type === "policy") { + return requirement.organizationPolicy?.status === "published"; + } + return false; + }; + const isCompleted = isRequirementCompleted(requirement);packages/db/prisma/migrations/20250218014206_add_name_column/migration.sql (1)
1-2: Consider making the name column required.The
namecolumn seems like an essential field for ControlRequirement. Consider making it NOT NULL with a default value to ensure data integrity.-ALTER TABLE "ControlRequirement" ADD COLUMN "name" TEXT; +ALTER TABLE "ControlRequirement" ADD COLUMN "name" TEXT NOT NULL DEFAULT 'Unnamed Requirement';packages/db/prisma/migrations/20250218002311_add_missing_columns_to_org_control_req/migration.sql (1)
1-7: Migration Header and Warning Comments
The header comments appropriately warn that adding non-nullable columns (typeandupdatedAt) without a default value can cause issues if the table already contains data. Please ensure that either the table is empty or a suitable migration strategy (such as a temporary default value and a subsequent update) is in place.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
apps/app/languine.lockis excluded by!**/*.lockbun.lockbis excluded by!**/bun.lockbyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
apps/app/src/actions/framework/select-frameworks-action.ts(4 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Actions/getOrganizationControlProgress.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Actions/getOrganizationControlRequirements.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Components/SingleControl.tsx(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/hooks/useOrganizationControlProgress.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/hooks/useOrganizationControlRequirements.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Actions/getOrganizationCategories.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Actions/getOrganizationControlsProgress.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Components/FrameworkControls.tsx(3 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/hooks/useOrganizationCategories.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/hooks/useOrganizationControlsProgress.ts(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/actions/publish-policy.ts(2 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/page.tsx(1 hunks)apps/app/src/app/[locale]/(app)/(dashboard)/policies/actions/get-policy.ts(1 hunks)apps/app/src/components/frameworks/framework-status.tsx(2 hunks)apps/app/src/components/tables/frameworks/columns.tsx(2 hunks)apps/app/src/components/tables/frameworks/data-table-header.tsx(2 hunks)apps/app/src/env.mjs(2 hunks)apps/app/src/locales/en.ts(1 hunks)apps/app/src/locales/es.ts(1 hunks)apps/app/src/locales/fr.ts(1 hunks)apps/app/src/locales/no.ts(1 hunks)apps/app/src/locales/pt.ts(1 hunks)packages/data/controls/soc2.json(28 hunks)packages/db/prisma/migrations/20250217233112_add_organization_control_requirement_table/migration.sql(1 hunks)packages/db/prisma/migrations/20250218002311_add_missing_columns_to_org_control_req/migration.sql(1 hunks)packages/db/prisma/migrations/20250218002808_removed_policy_id_column/migration.sql(1 hunks)packages/db/prisma/migrations/20250218014206_add_name_column/migration.sql(1 hunks)packages/db/prisma/migrations/20250218202303_add_organization_policy_to_org_control_req/migration.sql(1 hunks)packages/db/prisma/schema.prisma(4 hunks)packages/db/prisma/seed.ts(3 hunks)packages/db/prisma/seedTypes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/app/src/app/[locale]/(app)/(dashboard)/policies/[id]/page.tsx
- packages/db/prisma/migrations/20250218002808_removed_policy_id_column/migration.sql
🔇 Additional comments (23)
apps/app/src/components/tables/frameworks/columns.tsx (1)
85-94: Verify correct i18n usage for the Category label.You're using
t("risk.vendor.table.category")for the header but referencingrow.original.categoryfor the data. Confirm that the same localized text is appropriate here, or rename the i18n key for clarity and consistency.apps/app/src/components/frameworks/framework-status.tsx (1)
5-6: Check references to removed statuses.You replaced
"compliant"and"non_compliant"with"completed"and"in_progress"inSTATUS_TYPES. Ensure you’ve removed or updated references to the old statuses (compliant/non_compliant) across the codebase (e.g., older migrations, older components).apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Components/FrameworkControls.tsx (1)
26-41: Well-implemented memoization and data transformation!The use of
useMemofor flattening and transforming the controls array is a good optimization that prevents unnecessary recalculations. The transformation logic is clean and preserves all necessary data fields.apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Actions/getOrganizationCategories.ts (1)
58-66: Well-structured database query with proper policy status inclusion!The enhanced query properly includes organization policy status while maintaining a clean and type-safe structure.
apps/app/src/env.mjs (1)
23-23:Details
❓ Verification inconclusive
Verify NODE_ENV configuration in deployment environments.
While the NODE_ENV configuration is correctly added, ensure it's properly set in all deployment environments as it often affects various runtime behaviors.
Also applies to: 55-55
🏁 Script executed:
#!/bin/bash # Check if NODE_ENV is configured in deployment files rg -g '*.{json,yaml,yml}' 'NODE_ENV'Length of output: 36
Action Required: Please verify that the NODE_ENV variable is correctly set in your deployment configuration files.
- In
apps/app/src/env.mjs, at lines 23 and 55, NODE_ENV is defined as an optional string.- The initial search in deployment-related files (JSON/YAML/YML) did not yield any instances of NODE_ENV. This might indicate that your deployment manifests (e.g., Docker Compose files, Kubernetes configurations, or other environment configuration files) might not explicitly set NODE_ENV.
- Next Step: Manually inspect your deployment configuration to ensure NODE_ENV is provided where necessary since it affects runtime behavior.
apps/app/src/app/[locale]/(app)/(dashboard)/controls/[id]/Actions/getOrganizationControlProgress.ts (1)
48-93: Well-implemented progress tracking with type-specific handling!The progress calculation is robust with proper handling of edge cases and type-specific completion criteria.
apps/app/src/components/tables/frameworks/data-table-header.tsx (1)
75-91: LGTM! The new category column is well-implemented.The new category column follows the same pattern as existing columns, with proper sorting functionality and visibility control.
packages/db/prisma/seed.ts (2)
32-33: LGTM! Cleanup for organizationControlRequirement is properly added.The cleanup is correctly added within the development environment check.
280-281: LGTM! The name field is properly handled in upsert operations.The
namefield is correctly added to both create and update operations for control requirements.Also applies to: 289-290
apps/app/src/locales/no.ts (1)
510-513: LGTM!The Norwegian translations for the new statuses are accurate and maintain consistency with other language files.
apps/app/src/locales/pt.ts (1)
510-513: LGTM!The Portuguese translations for the new statuses are accurate and maintain consistency with other language files.
apps/app/src/locales/es.ts (1)
510-513: LGTM!The Spanish translations for the new statuses are accurate and maintain consistency with other language files.
apps/app/src/locales/fr.ts (1)
510-512: LGTM! French translations are accurate.The new status translations "Terminé" for "completed" and "En cours" for "in_progress" are correctly implemented in French.
packages/db/prisma/migrations/20250218202303_add_organization_policy_to_org_control_req/migration.sql (1)
1-5: LGTM! Well-structured foreign key relationship.The migration correctly sets up the foreign key relationship with appropriate cascade behavior for maintaining referential integrity.
packages/db/prisma/migrations/20250217233112_add_organization_control_requirement_table/migration.sql (1)
1-17: LGTM! Well-designed table structure with proper constraints.The migration:
- Creates a robust table structure with appropriate primary key
- Prevents duplicates with composite unique index
- Maintains referential integrity with cascading foreign keys
packages/db/prisma/migrations/20250218002311_add_missing_columns_to_org_control_req/migration.sql (2)
8-16: ALTER TABLE: Adding New Columns
The ALTER TABLE statement is correctly formulated to add the new columns (content,createdAt,description,fileUrl,policyId,published,type, andupdatedAt) to theOrganizationControlRequirementtable. However, note that the columnstypeandupdatedAtare added as non-nullable without defaults. Double-check that this aligns with the data migration plan if existing records are present.
18-20: Foreign Key Constraint Addition
The foreign key onpolicyIdis added withON DELETE SET NULLandON UPDATE CASCADE, which fits the expected behavior. Confirm that the data types forpolicyIdin both tables match and that this change is consistent with the overall data model expectations.packages/db/prisma/schema.prisma (5)
7-10: Datasource Configuration
The datasource block now uses PostgreSQL with the database URL sourced from an environment variable. This update adheres to common best practices for configuration management.
287-293: OrganizationControl Model Updates
New relations added to theOrganizationControlmodel—including optional fields fororganizationFrameworkIdandorganizationCategoryIdas well as the relation toOrganizationControlRequirement—are well defined. Please review that these relationships accurately support the intended business logic.
920-934: ControlRequirement Model Enhancements
TheControlRequirementmodel now includescreatedAt(with a default value),updatedAt(managed by Prisma’s@updatedAt), and a relation toOrganizationControlRequirement. These enhancements improve auditability and data integrity. No issues were found.
936-958: OrganizationControlRequirement Model Definition
The new model forOrganizationControlRequirementis comprehensively defined with fields corresponding directly to the migration changes. The inclusion of non-nullable fields (liketype) alongside relations, and the unique constraint on[organizationControlId, controlRequirementId], provides a robust structure. Just ensure that creation logic in the application handles these non-nullable fields properly.
966-984: OrganizationPolicy Model Enhancements
TheOrganizationPolicymodel now includes acontentfield (as an array of JSON objects) and a relation toOrganizationControlRequirement. These changes support the new functionality for tracking and associating policies with controls. It would be beneficial to verify compatibility with existing queries and application logic.packages/data/controls/soc2.json (1)
1-1254: Comprehensive Update of SOC2 Controls Data
The JSON file has been significantly updated across multiple control categories (e.g., CC1.x, CC2.x, CC3.x, etc.) by explicitly adding a"name"property to each requirement object. This addition improves clarity, consistency, and maintainability in the control definitions and ensures that policies and procedures are clearly labeled. Please verify that the new names (such as "Corporate Governance Policy", "Board Oversight Procedures", etc.) align with your business terminology and map correctly to the corresponding database records in your application.
| if (!result) { | ||
| throw new Error("Failed to fetch frameworks"); | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message.
The error message references "frameworks" instead of "categories".
- throw new Error("Failed to fetch frameworks");
+ throw new Error(`Failed to fetch categories for frameworkId: ${frameworkId}`);📝 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.
| if (!result) { | |
| throw new Error("Failed to fetch frameworks"); | |
| } | |
| if (!result) { | |
| - throw new Error("Failed to fetch frameworks"); | |
| + throw new Error(`Failed to fetch categories for frameworkId: ${frameworkId}`); | |
| } |
| if (!result) { | ||
| throw new Error("Failed to fetch control"); | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message.
The error message references "control" instead of "control requirements" and could include the controlId.
- throw new Error("Failed to fetch control");
+ throw new Error(`Failed to fetch control requirements for controlId: ${controlId}`);📝 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.
| if (!result) { | |
| throw new Error("Failed to fetch control"); | |
| } | |
| if (!result) { | |
| throw new Error(`Failed to fetch control requirements for controlId: ${controlId}`); | |
| } |
| type: string; | ||
| description: string; | ||
| policyId?: string; | ||
| name?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add name property to ControlRequirement interface for consistency.
The name property has been added to the Requirement interface but is missing in the ControlRequirement interface. Since both interfaces represent similar entities, they should maintain consistency in their properties.
Apply this diff to add the name property to the ControlRequirement interface:
export interface ControlRequirement {
id: string;
type: string;
description: string;
policyId?: string;
+ name?: string;
}Also applies to: 32-37
| return { | ||
| error: "Not authorized - no organization found", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize error response structure.
The error response structure is inconsistent with other functions in the codebase which use { success: boolean, error?: string }. Also, the error message in the catch block is incorrect as it mentions "organization control" instead of "organization control requirements".
Apply this diff to standardize error responses:
return {
+ success: false,
error: "Not authorized - no organization found",
};
// ... and in catch block:
return {
+ success: false,
- error: "Failed to fetch organization control",
+ error: "Failed to fetch organization control requirements",
};Also applies to: 58-62
| const createOrganizationControlRequirements = async ( | ||
| user: User, | ||
| organizationFrameworkIds: string[] | ||
| ) => { | ||
| if (!user.organizationId) { | ||
| throw new Error("Not authorized - no organization found"); | ||
| } | ||
|
|
||
| const controls = await db.organizationControl.findMany({ | ||
| where: { | ||
| organizationId: user.organizationId!, | ||
| organizationFrameworkId: { | ||
| in: organizationFrameworkIds, | ||
| }, | ||
| }, | ||
| include: { | ||
| control: true, | ||
| }, | ||
| }); | ||
|
|
||
| // Create control requirements for each control | ||
| const controlRequirements = await db.controlRequirement.findMany({ | ||
| where: { | ||
| controlId: { in: controls.map((control) => control.controlId) }, | ||
| }, | ||
| include: { | ||
| policy: true, // Include the policy to get its ID | ||
| }, | ||
| }); | ||
|
|
||
| // Get all organization policies for this organization | ||
| const organizationPolicies = await db.organizationPolicy.findMany({ | ||
| where: { | ||
| organizationId: user.organizationId, | ||
| }, | ||
| }); | ||
|
|
||
| for (const control of controls) { | ||
| const requirements = controlRequirements.filter( | ||
| (req) => req.controlId === control.controlId | ||
| ); | ||
|
|
||
| await db.organizationControlRequirement.createMany({ | ||
| data: requirements.map((requirement) => { | ||
| // Find the corresponding organization policy if this is a policy requirement | ||
| const policyId = | ||
| requirement.type === "policy" ? requirement.policy?.id : null; | ||
| const organizationPolicy = policyId | ||
| ? organizationPolicies.find((op) => op.policyId === policyId) | ||
| : null; | ||
|
|
||
| return { | ||
| organizationControlId: control.id, | ||
| controlRequirementId: requirement.id, | ||
| type: requirement.type, | ||
| description: requirement.description, | ||
| organizationPolicyId: organizationPolicy?.id || null, | ||
| }; | ||
| }), | ||
| }); | ||
| } | ||
|
|
||
| return controlRequirements; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and type safety.
Consider adding error handling for database operations and improving type safety.
const createOrganizationControlRequirements = async (
user: User,
organizationFrameworkIds: string[]
) => {
if (!user.organizationId) {
throw new Error("Not authorized - no organization found");
}
+ try {
const controls = await db.organizationControl.findMany({
where: {
organizationId: user.organizationId!,
organizationFrameworkId: {
in: organizationFrameworkIds,
},
},
include: {
control: true,
},
});
+ if (controls.length === 0) {
+ console.warn("No controls found for the given organization frameworks");
+ return [];
+ }
const controlRequirements = await db.controlRequirement.findMany({
where: {
controlId: { in: controls.map((control) => control.controlId) },
},
include: {
policy: true,
},
});
+ if (controlRequirements.length === 0) {
+ console.warn("No control requirements found for the given controls");
+ return [];
+ }
const organizationPolicies = await db.organizationPolicy.findMany({
where: {
organizationId: user.organizationId,
},
});
for (const control of controls) {
const requirements = controlRequirements.filter(
(req) => req.controlId === control.controlId
);
+ if (requirements.length === 0) {
+ console.warn(`No requirements found for control ${control.controlId}`);
+ continue;
+ }
await db.organizationControlRequirement.createMany({
data: requirements.map((requirement) => {
const policyId =
requirement.type === "policy" ? requirement.policy?.id : null;
const organizationPolicy = policyId
? organizationPolicies.find((op) => op.policyId === policyId)
: null;
return {
organizationControlId: control.id,
controlRequirementId: requirement.id,
type: requirement.type,
description: requirement.description,
organizationPolicyId: organizationPolicy?.id || null,
};
}),
});
}
return controlRequirements;
+ } catch (error) {
+ console.error("Error creating organization control requirements:", error);
+ throw new Error("Failed to create organization control requirements");
+ }
};📝 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.
| const createOrganizationControlRequirements = async ( | |
| user: User, | |
| organizationFrameworkIds: string[] | |
| ) => { | |
| if (!user.organizationId) { | |
| throw new Error("Not authorized - no organization found"); | |
| } | |
| const controls = await db.organizationControl.findMany({ | |
| where: { | |
| organizationId: user.organizationId!, | |
| organizationFrameworkId: { | |
| in: organizationFrameworkIds, | |
| }, | |
| }, | |
| include: { | |
| control: true, | |
| }, | |
| }); | |
| // Create control requirements for each control | |
| const controlRequirements = await db.controlRequirement.findMany({ | |
| where: { | |
| controlId: { in: controls.map((control) => control.controlId) }, | |
| }, | |
| include: { | |
| policy: true, // Include the policy to get its ID | |
| }, | |
| }); | |
| // Get all organization policies for this organization | |
| const organizationPolicies = await db.organizationPolicy.findMany({ | |
| where: { | |
| organizationId: user.organizationId, | |
| }, | |
| }); | |
| for (const control of controls) { | |
| const requirements = controlRequirements.filter( | |
| (req) => req.controlId === control.controlId | |
| ); | |
| await db.organizationControlRequirement.createMany({ | |
| data: requirements.map((requirement) => { | |
| // Find the corresponding organization policy if this is a policy requirement | |
| const policyId = | |
| requirement.type === "policy" ? requirement.policy?.id : null; | |
| const organizationPolicy = policyId | |
| ? organizationPolicies.find((op) => op.policyId === policyId) | |
| : null; | |
| return { | |
| organizationControlId: control.id, | |
| controlRequirementId: requirement.id, | |
| type: requirement.type, | |
| description: requirement.description, | |
| organizationPolicyId: organizationPolicy?.id || null, | |
| }; | |
| }), | |
| }); | |
| } | |
| return controlRequirements; | |
| }; | |
| const createOrganizationControlRequirements = async ( | |
| user: User, | |
| organizationFrameworkIds: string[] | |
| ) => { | |
| if (!user.organizationId) { | |
| throw new Error("Not authorized - no organization found"); | |
| } | |
| try { | |
| const controls = await db.organizationControl.findMany({ | |
| where: { | |
| organizationId: user.organizationId!, | |
| organizationFrameworkId: { | |
| in: organizationFrameworkIds, | |
| }, | |
| }, | |
| include: { | |
| control: true, | |
| }, | |
| }); | |
| if (controls.length === 0) { | |
| console.warn("No controls found for the given organization frameworks"); | |
| return []; | |
| } | |
| const controlRequirements = await db.controlRequirement.findMany({ | |
| where: { | |
| controlId: { in: controls.map((control) => control.controlId) }, | |
| }, | |
| include: { | |
| policy: true, // Include the policy to get its ID | |
| }, | |
| }); | |
| if (controlRequirements.length === 0) { | |
| console.warn("No control requirements found for the given controls"); | |
| return []; | |
| } | |
| const organizationPolicies = await db.organizationPolicy.findMany({ | |
| where: { | |
| organizationId: user.organizationId, | |
| }, | |
| }); | |
| for (const control of controls) { | |
| const requirements = controlRequirements.filter( | |
| (req) => req.controlId === control.controlId | |
| ); | |
| if (requirements.length === 0) { | |
| console.warn(`No requirements found for control ${control.controlId}`); | |
| continue; | |
| } | |
| await db.organizationControlRequirement.createMany({ | |
| data: requirements.map((requirement) => { | |
| const policyId = | |
| requirement.type === "policy" ? requirement.policy?.id : null; | |
| const organizationPolicy = policyId | |
| ? organizationPolicies.find((op) => op.policyId === policyId) | |
| : null; | |
| return { | |
| organizationControlId: control.id, | |
| controlRequirementId: requirement.id, | |
| type: requirement.type, | |
| description: requirement.description, | |
| organizationPolicyId: organizationPolicy?.id || null, | |
| }; | |
| }), | |
| }); | |
| } | |
| return controlRequirements; | |
| } catch (error) { | |
| console.error("Error creating organization control requirements:", error); | |
| throw new Error("Failed to create organization control requirements"); | |
| } | |
| }; |
| completed: "Completed", | ||
| in_progress: "In Progress", | ||
| not_started: "Not Started", |
There was a problem hiding this comment.
Ensure consistent status options across all languages.
The English translations are missing the "non_compliant" status that exists in other language files (es.ts, no.ts, pt.ts). This inconsistency could lead to missing translations or undefined behavior.
Apply this diff to maintain consistency with other language files:
statuses: {
completed: "Completed",
in_progress: "In Progress",
+ non_compliant: "Non Compliant",
not_started: "Not Started",
}📝 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.
| completed: "Completed", | |
| in_progress: "In Progress", | |
| not_started: "Not Started", | |
| completed: "Completed", | |
| in_progress: "In Progress", | |
| non_compliant: "Non Compliant", | |
| not_started: "Not Started", |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Components/FrameworkControls.tsx (1)
24-39: Consider improving type safety.The type assertion could be replaced with proper type inference, and the requirements field might need null safety handling.
Consider this improvement:
- const allControls = useMemo(() => { + const allControls = useMemo<OrganizationControlType[]>(() => { if (!organizationCategories) return []; return organizationCategories.flatMap((category) => category.organizationControl.map((control) => ({ code: control.control.code, description: control.control.description, name: control.control.name, status: control.status, id: control.id, frameworkId, category: category.name, - requirements: control.OrganizationControlRequirement, + requirements: control.OrganizationControlRequirement ?? [], })) ); - }, [organizationCategories, frameworkId]) as OrganizationControlType[]; + }, [organizationCategories, frameworkId]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
bun.lockis excluded by!**/*.lockbun.lockbis excluded by!**/bun.lockbyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Components/FrameworkControls.tsx(3 hunks)packages/db/prisma/seed.js(2 hunks)
🔇 Additional comments (5)
apps/app/src/app/[locale]/(app)/(dashboard)/frameworks/[frameworkId]/Components/FrameworkControls.tsx (3)
13-14: LGTM! Necessary imports added correctly.The new imports support the memoization optimization and type safety improvements.
24-39: LGTM! Efficient memoization of control data.The implementation correctly prevents unnecessary recomputations and handles the null case appropriately.
52-52: LGTM! Clean DataTable implementation.The simplified DataTable usage with memoized data improves component clarity and performance.
packages/db/prisma/seed.js (2)
25-25: LGTM! Proper placement of the new deletion operation.The deletion of organization control requirements is correctly positioned after related tables, maintaining referential integrity during cleanup.
208-208:Details
❓ Verification inconclusive
Consider adding validation for the name field.
While the addition of the name field is correct, consider validating it before use to prevent potential issues with null or invalid values.
Also applies to: 216-216
🏁 Script executed:
#!/bin/bash # Description: Check if any control requirements in the data files have missing or invalid names # Search for control requirements without a name field or with invalid names echo "Checking control requirements in data files..." fd . "data/controls" -e json -x jq -r '.[] | select(.requirements != null) | .requirements[] | select(.name == null or .name == "" or .name == "undefined") | "Control requirement \(.id) has missing or invalid name"'Length of output: 419
Action Required: Validate the 'name' field and verify data source paths
- The addition of
name: requirement.nameto both create and update operations correctly aligns with the schema changes.- However, there is no validation in place to ensure that the name field is neither null nor invalid.
- The provided shell script attempt failed because the search path
"data/controls"is not a directory. Please verify whether the control requirements data exists at a different path or update the script accordingly.- Consider adding validation logic before the upsert operations to prevent potential issues from missing or invalid names.
This branch had an error being deployed
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores