Skip to content

Improvements#124

Merged
Marfuen merged 10 commits into
mainfrom
mariano/deel-import
Mar 11, 2025
Merged

Improvements#124
Marfuen merged 10 commits into
mainfrom
mariano/deel-import

Conversation

@Marfuen
Copy link
Copy Markdown
Contributor

@Marfuen Marfuen commented Mar 11, 2025

Summary by CodeRabbit

  • New Features

    • Improved progress display on the dashboard by incorporating compliance score data.
    • Updated multilingual interface text for employee labels in English, Spanish, French, Norwegian, and Portuguese to enhance clarity.
    • Introduced a new "status" column in the employee data table, allowing for better tracking of employee statuses.
    • Added functionality for displaying employee statuses with visual representation.
  • Refactor

    • Streamlined the employee creation workflow for smoother processing and more precise error feedback.
    • Enhanced employee status tracking during synchronization.
  • Style

    • Refined the layout of employee details for a more consistent viewing experience.

Marfuen and others added 6 commits March 11, 2025 11:09
- Extract employee creation logic into reusable functions in `lib/db/employee.ts`
- Implement `completeEmployeeCreation` to handle employee, portal user, and task creation
- Enhance Deel sync to support both active and inactive employee processing
- Improve error handling and logging during employee synchronization
- Simplify `createEmployeeAction` by leveraging new database helper functions
- Update `EmployeeDetails` component to display task status more clearly
@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 11, 2025 6:29pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
comp-portal ⬜️ Skipped (Inspect) Mar 11, 2025 6:29pm
web ⬜️ Skipped (Inspect) Mar 11, 2025 6:29pm

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 11, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request updates multiple parts of the codebase. In the dashboard component, unused icon imports are removed, and a new hook for compliance scores is added. The English localization file is updated with a new "status" field, while Spanish, French, Norwegian, and Portuguese locales receive similar updates to include employee status labels. The employee creation action is streamlined by removing duplicate checks and delegating logic to a new function, completeEmployeeCreation, within a newly added employee database module. Additionally, the Deel trigger logic is refactored with improved logging and error handling, and minor formatting adjustments are made in the employee details component.

Changes

File Change Summary
apps/app/src/app/[locale]/.../components/FrameworkProgress.tsx Added import for useComplianceScores; removed unused FileText, CheckCircle, and Cloud imports.
apps/app/src/locales/en.ts, .../es.ts, .../fr.ts, .../no.ts, .../pt.ts Updated to include new status field in the people section for localization.
apps/app/src/actions/people/create-employee-action.ts Refactored to remove individual employee checks; now calls completeEmployeeCreation with enhanced error handling.
apps/app/src/app/[locale]/.../components/EmployeeDetails.tsx Removed several imports; added a Button for task reminders; simplified rendering logic.
apps/app/src/lib/db/employee.ts Modified createOrReactivateEmployee to remove isActive: true property from employee data.
apps/app/src/trigger/deel/index.ts Updated syncDeelEmployees: now imports new DB functions; improved logging, status management, and error catching.
apps/app/src/app/[locale]/.../people/actions/get-employees.ts Removed select clause from db.employee.findMany, returning all fields by default.
apps/app/src/app/[locale]/.../components/EmployeesList.tsx Updated EmployeesListProps interface to include status in columnHeaders.
apps/app/src/components/tables/people/data-table-column-header.tsx Introduced DataTableColumnHeader component for sortable table headers.
apps/app/src/components/tables/people/data-table-header.tsx Added new "status" column for sorting in the data table header.
apps/app/src/components/tables/people/data-table.tsx Updated DataTableProps to include status in columnHeaders and modified rendering logic.
apps/app/src/components/tables/people/employee-status.tsx New file added to manage employee statuses, including EMPLOYEE_STATUS_TYPES and getEmployeeStatusFromBoolean.
apps/app/src/components/tables/people/server-columns.tsx Modified getServerColumnHeaders to include status property in the returned object.

Sequence Diagram(s)

sequenceDiagram
    participant Caller as Action/Trigger
    participant DBModule as "Employee DB Module"
    
    Caller->>DBModule: completeEmployeeCreation({name, email, ...})
    activate DBModule
    DBModule->>DBModule: findEmployeeByEmail(email, orgId)
    DBModule->>DBModule: createOrReactivateEmployee(params)
    DBModule->>DBModule: createOrUpdatePortalUser(params)
    DBModule->>DBModule: createDefaultTasksForEmployee(employeeId)
    deactivate DBModule
    DBModule-->>Caller: Employee record confirmation
Loading

Possibly related PRs

  • Improvements #124: The changes in the main PR are directly related to those in the retrieved PR, as both involve modifications to the same import statements and the addition of the useComplianceScores hook in the FrameworkProgress.tsx file.

Poem

I'm a bunny coding away,
Hopping through changes at break of day,
Updating strings and streamlining flow,
Employee tasks in a neat new row,
With each bug I nibble, my joy does grow 🐰✨!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7860aa and 75e27ee.

⛔ Files ignored due to path filters (4)
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
📒 Files selected for processing (19)
  • apps/app/src/app/[locale]/(app)/(dashboard)/(home)/components/FrameworkProgress.tsx (0 hunks)
  • apps/app/src/app/[locale]/(app)/(dashboard)/(home)/components/FrameworkProgress.tsx (2 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)
  • apps/app/src/trigger/deel/index.ts (5 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)
  • 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)
  • 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)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (7)
apps/app/src/actions/people/create-employee-action.ts (2)

30-36: Validate additional fields before calling completeEmployeeCreation.

Though this call is correct, consider adding validations or sanitation for parameters such as department and externalEmployeeId if they come from user input or external sources, preventing data inconsistencies.


58-59: Leverage more descriptive errors and monitor error frequency.

While returning the error message improves readability, consider logging and categorizing error types to easily track and handle recurring issues in production (e.g., user input errors vs. database constraints).

apps/app/src/trigger/deel/index.ts (4)

194-201: Consider a configurable integration run interval.

Hardcoding a 24-hour skip window works but may limit certain use cases requiring more frequent syncs. Allowing administrators to configure or override the interval could increase flexibility.

Also applies to: 203-213


274-277: Logging inactive employees is beneficial — consider proactive handling.

Listing inactive employees is helpful for debugging. In addition, consider adding further logic, automated notifications, or separate processing steps tailored to reactivating or archiving those records.

Also applies to: 279-279


339-342: Use consistent error handling for findEmployeeByEmail.

If a database error occurs here, ensure it's captured in a try-catch block or appropriately handled so as not to stall the entire sync.


357-358: Track changes of isActive status in a dedicated table or log.

While setting isActive is straightforward, it may be beneficial to record when and why status changes occur, aiding in auditing and troubleshooting.

Also applies to: 360-362

apps/app/src/lib/db/employee.ts (1)

84-123: Portal user upsert approach.

This pattern effectively ensures that a portal login is created or updated as part of the employee life cycle. Just verify that the same email is not shared across different user accounts to avoid overwriting references inadvertently.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d6640ed and 2e7a890.

⛔ Files ignored due to path filters (2)
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • apps/app/src/app/[locale]/(app)/(dashboard)/(home)/components/FrameworkProgress.tsx (0 hunks)
  • apps/app/src/app/[locale]/(app)/(dashboard)/(home)/components/FrameworkProgress.tsx (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)
  • apps/app/src/actions/people/create-employee-action.ts (3 hunks)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx (1 hunks)
  • apps/app/src/lib/db/employee.ts (1 hunks)
  • apps/app/src/trigger/deel/index.ts (5 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)
🔇 Additional comments (15)
apps/app/src/locales/es.ts (1)

766-766: Label enhancement looks good!

The change from "Nombre" to "Nombre del empleado" provides better context for users by specifying that this refers to an employee's name rather than just any name.

apps/app/src/locales/no.ts (1)

766-766: Label clarification is appropriate!

The translation update from "Navn" to "Ansattnavn" correctly aligns with the changes in other localization files, making it clearer that this field refers specifically to an employee's name.

apps/app/src/locales/fr.ts (1)

766-766: Good translation improvement!

Changing from "Nom" to "Nom de l'employé" provides better context for French-speaking users, making it explicit that this field refers to an employee's name rather than just any name.

apps/app/src/locales/pt.ts (1)

766-766: Translation enhancement is appropriate!

The change from "Nome" to "Nome do Funcionário" improves clarity for Portuguese-speaking users by explicitly indicating that the field refers to an employee's name.

apps/app/src/locales/en.ts (1)

1-932: Complete replacement of English localization file looks well-structured.

The file is well-organized with clear hierarchical sections for different parts of the application. The structure follows best practices with logical grouping of related strings.

apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx (6)

17-21: Consistent indentation change applied.

The indentation has been consistently updated from 2-space to 4-space formatting throughout the import statements.


23-24: Consistent indentation change applied to props interface.

The indentation has been updated to maintain the 4-space formatting standard.


27-29: Consistent indentation change applied to hook usage.

The indentation has been updated to maintain the 4-space formatting standard.


30-46: Consistent indentation change applied to error handling.

The error handling logic maintains its functionality while being reformatted with 4-space indentation.


48-70: Consistent indentation change applied to loading state.

The loading state rendering maintains its functionality while being reformatted with 4-space indentation.


72-131: Consistent indentation change applied to main component rendering.

The component rendering logic maintains its functionality while being reformatted with 4-space indentation.

apps/app/src/actions/people/create-employee-action.ts (1)

7-7: Good modular approach by importing completeEmployeeCreation.

This refactor neatly encapsulates the employee creation flow into a single reusable function, enhancing maintainability and making the code more intuitive.

apps/app/src/trigger/deel/index.ts (1)

287-290: Validate active status transitions.

Setting isActive based on Deel data is fine, but if a previously active employee is identified as inactive, confirm whether that triggers de-provisioning or related tasks to maintain data consistency.

Also applies to: 293-293

apps/app/src/lib/db/employee.ts (2)

17-32: Indexing best practices.

Relying on email_organizationId compound uniqueness is good. Ensure an appropriate database index is in place to maintain performance for large organizations.


34-82: Validate reactivation logic in multi-tenant scenarios.

When reactivating an employee, confirm that the original record truly belongs to the same organization, especially if data from external sources (like Deel) might inadvertently match an email from a different tenant in the same system.

Comment on lines +372 to +380
try {
// Create new employee using the reusable function
const newEmployee = await completeEmployeeCreation({
name,
email,
department,
isActive: true,
externalEmployeeId: deelEmployee.id,
organizationId: integration.organization.id,
},
});
logger.info(
`Successfully created new employee: ${name} with ID: ${newEmployee.id}`
);
processedEmployees.push({ id: newEmployee.id, action: "created" });
externalEmployeeId: deelEmployee.id,
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Sustain batch creation resilience.

The try-catch approach for each employee creation is good at preventing the entire sync from failing when one record is problematic. However, consider adding a retry mechanism or more detailed error handling for common failures (e.g., duplicate external IDs). This ensures robust and fault-tolerant imports from Deel.

Also applies to: 382-393, 395-402, 404-408

Comment on lines +4 to +15
const DEFAULT_TASKS = [
{
code: "POLICY-ACCEPT",
name: "Policy Acceptance",
description: "Review and accept company policies",
},
{
code: "DEVICE-SECURITY",
name: "Device Security",
description: "Complete device security checklist and configuration",
},
] as const;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid potential duplicate tasks in concurrent runs.

If completeEmployeeCreation is called multiple times in parallel for the same employee, repeated calls to createDefaultTasksForEmployee may lead to duplicate tasks unless guarded by unique constraints. Consider transactional logic or unique checks around assignment.

Comment on lines +125 to +158
/**
* Create default tasks for an employee
*/
export async function createDefaultTasksForEmployee(
employeeId: string
): Promise<void> {
// Create or get the required task definitions first and store their IDs
const requiredTasks = await Promise.all(
DEFAULT_TASKS.map(async (task) => {
return db.employeeRequiredTask.upsert({
where: { code: task.code },
create: {
code: task.code,
name: task.name,
description: task.description,
},
update: {},
});
})
);

// Now create the employee tasks using the actual task IDs
await Promise.all(
requiredTasks.map(async (task) => {
return db.employeeTask.create({
data: {
employeeId: employeeId,
requiredTaskId: task.id,
status: "assigned",
},
});
})
);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Prevent conflicting tasks.

employeeTask.create() can lead to multiple tasks of the same code if createDefaultTasksForEmployee is called repeatedly for the same employee. Consider adding a check for existing tasks or adopting an upsert pattern.

Comment on lines +160 to +185
/**
* Complete employee creation by handling all steps:
* 1. Create/reactivate the employee
* 2. Create/update portal user
* 3. Create default tasks
*/
export async function completeEmployeeCreation(params: {
name: string;
email: string;
department: Departments;
organizationId: string;
externalEmployeeId?: string;
}): Promise<Employee> {
const employee = await createOrReactivateEmployee(params);

await createOrUpdatePortalUser({
employeeId: employee.id,
name: params.name,
email: params.email,
organizationId: params.organizationId,
});

await createDefaultTasksForEmployee(employee.id);

return employee;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Single transaction for the entire creation flow.

completeEmployeeCreation runs multiple DB operations which could partially succeed if an error occurs mid-process. Wrapping these steps in a transaction preserves consistency, preventing partial data writes.

…nality

- Introduce employee status column in the People table
- Create `EmployeeStatus` component for consistent status display
- Add sorting and column header functionality for status column
- Update server-side column headers and translations
- Refactor data table to support dynamic column rendering
- Remove `isActive` from employee creation logic
- Improve table layout and responsiveness
@vercel vercel Bot temporarily deployed to Preview – comp-portal March 11, 2025 18:23 Inactive
@vercel vercel Bot temporarily deployed to Preview – web March 11, 2025 18:23 Inactive
@vercel vercel Bot temporarily deployed to Preview – web March 11, 2025 18:24 Inactive
@vercel vercel Bot temporarily deployed to Preview – comp-portal March 11, 2025 18:24 Inactive
@vercel vercel Bot temporarily deployed to Preview – app March 11, 2025 18:25 Inactive
@vercel vercel Bot temporarily deployed to Preview – comp-portal March 11, 2025 18:25 Inactive
@vercel vercel Bot temporarily deployed to Preview – web March 11, 2025 18:25 Inactive
@Marfuen Marfuen merged commit 4b46a62 into main Mar 11, 2025
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
apps/app/src/lib/db/employee.ts (3)

17-32: Add validation for email and organizationId parameters.

The function could benefit from input validation to ensure that the email and organizationId parameters are not empty or malformed before executing the database query.

export async function findEmployeeByEmail(
  email: string,
  organizationId: string
): Promise<Employee | null> {
+  if (!email || !email.trim()) {
+    throw new Error("Email is required");
+  }
+  if (!organizationId || !organizationId.trim()) {
+    throw new Error("Organization ID is required");
+  }
  return db.employee.findUnique({
    where: {
      email_organizationId: {
        email,
        organizationId,
      },
    },
  });
}

34-82: Add input validation and enhance error messages.

The function currently lacks input validation for parameters. Also, the error message could be more specific about which email is causing the conflict.

export async function createOrReactivateEmployee(params: {
  name: string;
  email: string;
  department: Departments;
  organizationId: string;
  externalEmployeeId?: string;
}): Promise<Employee> {
  const { name, email, department, organizationId, externalEmployeeId } =
    params;

+  // Validate input parameters
+  if (!name || !name.trim()) {
+    throw new Error("Employee name is required");
+  }
+  if (!email || !email.trim()) {
+    throw new Error("Employee email is required");
+  }
+  if (!department) {
+    throw new Error("Department is required");
+  }
+  if (!organizationId || !organizationId.trim()) {
+    throw new Error("Organization ID is required");
+  }

  // First check if an employee exists
  const existingEmployee = await findEmployeeByEmail(email, organizationId);

  if (existingEmployee) {
    if (existingEmployee.isActive) {
      throw new Error(
-        "An employee with this email already exists in your organization"
+        `An employee with email "${email}" already exists in your organization`
      );
    }

    // Reactivate the existing employee
    return db.employee.update({
      where: { id: existingEmployee.id },
      data: {
        name,
        department,
        isActive: true,
        externalEmployeeId,
        organizationId,
        updatedAt: new Date(),
      },
    });
  }

  // Create a new employee
  return db.employee.create({
    data: {
      name,
      email,
      department,
      organizationId,
      isActive: true,
      externalEmployeeId,
    },
  });
}

84-123: Consider handling email verification in the update case.

When creating a new portal user, the emailVerified field is set to false, but this field is not updated when updating an existing user. This could lead to inconsistent behavior if the email is changed.

update: {
  updatedAt: new Date(),
  name,
  email,
  organizationId,
+  // Reset email verification if the email has changed
+  emailVerified: {
+    set: db.portalUser.findUnique({
+      where: { email },
+      select: { email: true }
+    }).then(user => user?.email === email)
+  },
  employees: {
    connect: {
      id: employeeId,
    },
  },
},
apps/app/src/components/tables/people/data-table-column-header.tsx (1)

1-28: Well-designed reusable component

This is a clean, well-designed generic component for table headers with sorting functionality. The component:

  1. Has clear TypeScript typing
  2. Follows single responsibility principle
  3. Provides good reusability across table columns
  4. Has consistent styling defaults

Consider adding aria-label or aria-sort attributes to improve accessibility for screen readers.

 <Button
   variant="ghost"
   onClick={() => column.toggleSorting(column.getIsSorted() === "asc")}
+  aria-sort={column.getIsSorted() || "none"}
+  aria-label={`Sort by ${title} ${column.getIsSorted() === "asc" ? "descending" : column.getIsSorted() === "desc" ? "unsorted" : "ascending"}`}
 >
   {title}
 </Button>
apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx (1)

33-35: Remove console.log statement before production

Debug logging statements should be removed before code is pushed to production.

-	console.log({
-		employees,
-	});
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2e7a890 and b7860aa.

⛔ Files ignored due to path filters (3)
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
  • apps/app/languine.lock is excluded by !**/*.lock
📒 Files selected for processing (26)
  • 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)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx (1 hunks)
  • apps/app/src/lib/db/employee.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)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx (3 hunks)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/actions/get-employees.ts (0 hunks)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx (2 hunks)
  • apps/app/src/components/status.tsx (1 hunks)
  • apps/app/src/components/tables/people/data-table-column-header.tsx (1 hunks)
  • apps/app/src/components/tables/people/data-table-header.tsx (1 hunks)
  • apps/app/src/components/tables/people/data-table.tsx (1 hunks)
  • apps/app/src/components/tables/people/employee-status.tsx (1 hunks)
  • apps/app/src/components/tables/people/server-columns.tsx (1 hunks)
  • apps/app/src/lib/db/employee.ts (0 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)
💤 Files with no reviewable changes (1)
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/actions/get-employees.ts
✅ Files skipped from review due to trivial changes (1)
  • apps/app/src/components/status.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
  • apps/app/src/lib/db/employee.ts
  • apps/app/src/locales/pt.ts
  • apps/app/src/locales/es.ts
  • apps/app/src/locales/no.ts
  • apps/app/src/locales/no.ts
  • apps/app/src/locales/fr.ts
  • apps/app/src/locales/es.ts
  • apps/app/src/locales/pt.ts
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx
  • apps/app/src/app/[locale]/(app)/(dashboard)/people/[employeeId]/components/EmployeeDetails.tsx
  • apps/app/src/locales/fr.ts
🔇 Additional comments (18)
apps/app/src/lib/db/employee.ts (2)

125-158: Prevent duplicate tasks for employees.

This function can create duplicate tasks if called multiple times for the same employee, as it doesn't check if the tasks already exist.

Consider using an upsert pattern or checking for existing tasks before creating new ones:

export async function createDefaultTasksForEmployee(
  employeeId: string
): Promise<void> {
  // Create or get the required task definitions first and store their IDs
  const requiredTasks = await Promise.all(
    DEFAULT_TASKS.map(async (task) => {
      return db.employeeRequiredTask.upsert({
        where: { code: task.code },
        create: {
          code: task.code,
          name: task.name,
          description: task.description,
        },
        update: {},
      });
    })
  );

  // Now create the employee tasks using the actual task IDs
  await Promise.all(
    requiredTasks.map(async (task) => {
-      return db.employeeTask.create({
-        data: {
-          employeeId: employeeId,
-          requiredTaskId: task.id,
-          status: "assigned",
-        },
-      });
+      return db.employeeTask.upsert({
+        where: {
+          employeeId_requiredTaskId: {
+            employeeId: employeeId,
+            requiredTaskId: task.id,
+          },
+        },
+        create: {
+          employeeId: employeeId,
+          requiredTaskId: task.id,
+          status: "assigned",
+        },
+        update: {}, // Keep existing data if the task already exists
+      });
    })
  );
}

160-185: Use a transaction for the employee creation process.

The current implementation runs multiple database operations without a transaction. If any operation fails midway, it could leave the database in an inconsistent state.

Wrap all operations in a transaction to ensure atomicity:

export async function completeEmployeeCreation(params: {
  name: string;
  email: string;
  department: Departments;
  organizationId: string;
  externalEmployeeId?: string;
}): Promise<Employee> {
+  return db.$transaction(async (tx) => {
+    // Use the transaction client for all database operations
+    const employee = await createOrReactivateEmployee({
+      ...params,
+      tx, // Pass transaction to other functions
+    });
+
+    await createOrUpdatePortalUser({
+      employeeId: employee.id,
+      name: params.name,
+      email: params.email,
+      organizationId: params.organizationId,
+      tx,
+    });
+
+    await createDefaultTasksForEmployee(employee.id, tx);
+
+    return employee;
+  });
-  const employee = await createOrReactivateEmployee(params);
-
-  await createOrUpdatePortalUser({
-    employeeId: employee.id,
-    name: params.name,
-    email: params.email,
-    organizationId: params.organizationId,
-  });
-
-  await createDefaultTasksForEmployee(employee.id);
-
-  return employee;
}

Note: You would need to modify the other functions to accept and use the transaction client as well.

apps/app/src/locales/pt.ts (1)

769-770: Consistent localization addition for employee table columns.

The addition of the "externalId" and "status" fields to the Portuguese localization file is consistent with the changes in other locales, providing proper translations for the employee table columns.

apps/app/src/locales/es.ts (1)

769-770: Appropriate Spanish translations for employee table headers.

The Spanish translations for "externalId" ("ID Externo") and "status" ("Estado") are correctly implemented, maintaining consistency with the other language files while using proper Spanish terminology.

apps/app/src/locales/fr.ts (1)

769-770: French localization correctly implemented for employee status.

The French translations for "externalId" ("ID externe") and "status" ("Statut") are appropriately added, ensuring the employee table can be properly displayed in French.

apps/app/src/locales/no.ts (1)

769-770: Norwegian translations properly added for employee table fields.

The Norwegian translations for "externalId" ("Ekstern ID") and "status" ("Status") are correctly implemented, completing the localization updates across all supported languages.

apps/app/src/components/tables/people/server-columns.tsx (1)

3-12: LGTM - Status field successfully added to column headers

The addition of the status field to the returned column headers is clean and follows the established pattern. This properly connects with the localization updates seen elsewhere in the PR.

apps/app/src/components/tables/people/data-table-header.tsx (1)

105-119: LGTM - Status column implementation

The status column implementation correctly follows the same pattern as existing columns, including proper sorting functionality and visibility control.

apps/app/src/locales/en.ts (2)

607-613: LGTM - Added status field to people table localization

The addition of the status field to the people table localization is consistent with the component changes seen in server-columns.tsx and data-table-header.tsx.


51-68: LGTM - Comprehensive status labels

Good job on providing a complete set of status values including "active" and "inactive" that will be used for the new employee status column.

apps/app/src/components/tables/people/data-table.tsx (4)

24-38: Well-organized imports for the new status feature

The imports are neatly organized, bringing in all the necessary components for implementing the new status column including the DataTableColumnHeader, Badge, and the custom EmployeeStatus components.


41-46: Properly updated interface to include status field

The DataTableProps interface has been correctly updated to include the new 'status' field in the columnHeaders object, maintaining type safety throughout the component.


106-123: Good implementation of the status column

The status column is well-implemented with proper sorting capabilities and clear rendering logic. Using the getEmployeeStatusFromBoolean utility function helps maintain consistency in status representation.


175-177: Consistent styling for the status column

The cell className assignment logic has been extended to include styling for the status column, maintaining consistency with the table's design pattern.

apps/app/src/components/tables/people/employee-status.tsx (3)

4-12: Well-defined employee status types and colors

Good job defining the employee status types as constants and exporting the type. The color mapping is also well-structured and provides a clear visual indication for each status.


18-30: Clean and reusable EmployeeStatus component

The EmployeeStatus component is well-implemented, with proper JSDoc comments and using the i18n hook for localization. It maintains a consistent styling approach that matches other status components in the system.


35-39: Simple and effective utility function

The getEmployeeStatusFromBoolean function is straightforward and follows the single responsibility principle. It provides a clean way to convert boolean values to status types.

apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx (1)

16-21: Interface updated to include status field

The EmployeesListProps interface has been properly updated to include the 'status' field in the columnHeaders object, maintaining consistency with the DataTableProps interface.

Comment on lines +33 to +34
DropdownMenuTrigger,
} from "@bubba/ui/dropdown-menu";
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Remove console.log for debugging in EmployeesList

There's a console.log statement in EmployeesList.tsx (lines 33-35) that should be removed before merging to production.


🏁 Script executed:

#!/bin/bash
# Check for console.log in EmployeesList.tsx
rg "console.log" "apps/app/src/app/\[locale\]/\(app\)/\(dashboard\)/people/components/EmployeesList.tsx"

Length of output: 230


Below is a new shell script to search the repository for any file named “EmployeesList.tsx” and then check whether it contains any console.log statements. This will help verify whether the debugging output mentioned in the review comment actually exists in the codebase—since the originally provided path did not match any file.

Please run the following script:

Once you run this script, please verify whether any EmployeesList.tsx file returns a hit for console.log. If such debugging code is found, it should be removed before merging to production. If no file is found or no console.log is present, you can disregard the debug removal comment.


🏁 Script executed:

#!/bin/bash
# Locate all EmployeesList.tsx files in the repository
echo "Searching for EmployeesList.tsx files in the repository..."
fd "EmployeesList.tsx" > /tmp/employees_list_files.txt

# Check each found file for console.log occurrences
echo "Checking for console.log statements in the located EmployeesList.tsx files..."
while IFS= read -r file; do
  echo "Processing $file:"
  grep -n "console.log" "$file" || echo "  No console.log found in $file."
done < /tmp/employees_list_files.txt

Length of output: 795


Action Required: Remove Debugging Console.log Statement from EmployeesList.tsx

The file apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx contains a console.log statement (detected at line 33) that should be removed before production merge.

  • Action: Remove the debugging console.log from the file, ensuring it’s completely omitted from production code.
  • File: apps/app/src/app/[locale]/(app)/(dashboard)/people/components/EmployeesList.tsx
  • Location: At least line 33 (as detected)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants