[Fix] Seed process not working #8422
Conversation
WalkthroughThe changes in this pull request involve modifications to several functions across different files to enhance SQL query handling and improve database operations. Key updates include the addition of MySQL-specific syntax handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant Database
User->>API: Request to create default KPI
API->>Database: Call insertRandomGoalKpiInBatches with batchSize
Database-->>API: Return inserted KPIs
API-->>User: Respond with success
User->>API: Request to create organization projects
API->>Database: Call createDefaultOrganizationProjects
Database-->>API: Return created projects
API-->>User: Respond with success
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Outside diff range and nitpick comments (6)
packages/core/src/core/utils.ts (2)
Line range hint
720-732: RefactorreplacePlaceholdersfor improved clarity and maintainabilityWhile the function works as intended, its structure could be improved for better readability and maintainability. Consider the following suggestions:
- Add a function-level comment explaining its purpose and parameters.
- Use a switch statement instead of multiple if conditions for different database types.
- Separate the placeholder replacement logic from the MySQL-specific quoting logic.
Here's a suggested refactoring:
/** * Adjusts SQL query placeholders and quoting based on the database type. * @param query - The original SQL query * @param dbType - The type of the database * @returns The adjusted SQL query */ export function replacePlaceholders(query: string, dbType: DatabaseTypeEnum): string { switch (dbType) { case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: return query.replace(/\$\d+/g, '?'); case DatabaseTypeEnum.mysql: return query.replace(/\$\d+/g, '?').replace(/"/g, '`'); default: return query; } }This refactoring improves readability and makes it easier to add support for additional database types in the future.
Line range hint
720-732: Consider broader refactoring for database-type handling consistencyThe
replacePlaceholdersfunction is part of a larger set of database-related utility functions in this file. To improve overall code consistency and maintainability, consider the following suggestions:
- Create a common pattern for handling different database types across all relevant functions.
- Consider extracting database-specific logic into separate helper functions or a dedicated module.
- Ensure consistent error handling and edge case management across all database-related functions.
Here's an example of how you might start this refactoring:
- Create a new file
database-utils.tswith specific helpers for each database type.- In
utils.ts, use these helpers to simplify the logic:import { adjustQueryForSQLite, adjustQueryForMySQL } from './database-utils'; export function replacePlaceholders(query: string, dbType: DatabaseTypeEnum): string { switch (dbType) { case DatabaseTypeEnum.sqlite: case DatabaseTypeEnum.betterSqlite3: return adjustQueryForSQLite(query); case DatabaseTypeEnum.mysql: return adjustQueryForMySQL(query); default: return query; } }This approach would make it easier to maintain database-specific logic and ensure consistency across the codebase.
packages/core/src/goal-kpi/goal-kpi.seed.ts (2)
31-31: Consider makingbatchSizea constant or configurationThe hardcoded value
100forbatchSizecould be defined as a constant or retrieved from a configuration to enhance maintainability and readability.Apply this diff to define
BATCH_SIZEas a constant:+const BATCH_SIZE = 100; ... -return insertRandomGoalKpiInBatches(dataSource, goalKpis, 100); +return insertRandomGoalKpiInBatches(dataSource, goalKpis, BATCH_SIZE);
34-50: Rename function for clarityThe function name
insertRandomGoalKpiInBatchesmight be misleading since it doesn't generate randomGoalKPIs but inserts provided ones. Consider renaming it toinsertGoalKpisInBatchesto improve clarity.Apply this diff to rename the function:
-const insertRandomGoalKpiInBatches = async ( +const insertGoalKpisInBatches = async ( dataSource: DataSource, goalKpis: GoalKPI[], batchSize: number ): Promise<GoalKPI[]> => { ... } ... -return insertRandomGoalKpiInBatches(dataSource, goalKpis, 100); +return insertGoalKpisInBatches(dataSource, goalKpis, 100);packages/core/src/organization-project/organization-project.seed.ts (2)
Line range hint
235-238: Ensure consistent function declaration styleThe
seedProjectMembersCountfunction has been changed from an exported constant assigned to an anonymous function to an exported named async function. For consistency and readability, consider using the same function declaration style throughout the codebase. Most functions in this file are declared usingexport const functionName = async () => {}syntax.
Line range hint
269-301: Consider using QueryBuilder to construct database-agnostic queriesUsing raw SQL queries with database-specific syntax can introduce maintenance challenges and increase the potential for errors. Consider utilizing TypeORM's
QueryBuilderto construct the update statements. This approach abstracts away database-specific syntax differences, enhances readability, and improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/core/src/core/utils.ts (1 hunks)
- packages/core/src/goal-kpi/goal-kpi.seed.ts (1 hunks)
- packages/core/src/organization-project/organization-project.seed.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/core/src/core/utils.ts (2)
Line range hint
1-732: Overall assessment: Positive changes with room for further improvementsThe changes to the
replacePlaceholdersfunction improve MySQL compatibility, which is a valuable addition. However, this review has identified several areas for potential improvement:
- The immediate changes could be refactored for better clarity and maintainability.
- There's an opportunity for a broader refactoring to improve consistency in handling different database types across the file.
- Consider extracting database-specific logic into separate helper functions or a dedicated module.
These improvements would enhance the overall quality, maintainability, and extensibility of the codebase. While the current changes are approved, I recommend considering these suggestions for future iterations.
728-732: Enhance MySQL compatibility with backtick quotingThe addition of MySQL-specific handling for identifier quoting is a good improvement. It ensures that column and table names are properly escaped in MySQL queries, preventing potential syntax errors.
However, there are a couple of points to consider:
- The condition
[DatabaseTypeEnum.mysql].includes(dbType)can be simplified todbType === DatabaseTypeEnum.mysql.- This change might affect existing queries that rely on double quotes for string literals.
To ensure this change doesn't introduce unintended side effects, please run the following verification script:
This script will help identify any potential conflicts or issues that might arise from this change across the codebase.
packages/core/src/organization-project/organization-project.seed.ts (1)
272-285:⚠️ Potential issueVerify the database type comparison for accurate condition checking
In the condition
if (dataSource.options.type === DatabaseTypeEnum.mysql), ensure thatdataSource.options.typeandDatabaseTypeEnum.mysqlare of compatible types and values. TypeORM'sdataSource.options.typeis typically a string (e.g.,'mysql'), so confirm thatDatabaseTypeEnum.mysqlmatches this string value to prevent incorrect query execution.To verify the values, you can log them:
console.log('dataSource.options.type:', dataSource.options.type); console.log('DatabaseTypeEnum.mysql:', DatabaseTypeEnum.mysql);Alternatively, run the following script to check the value of
DatabaseTypeEnum.mysql:
| @@ -27,12 +27,25 @@ export const createDefaultGoalKpi = async ( | |||
| goalKpis.push(goalKpi); | |||
| }); | |||
| }); | |||
| return await insertRandomGoalKpi(dataSource, goalKpis); | |||
| // Insert in batches to prevent deadlocks | |||
| return await insertRandomGoalKpiInBatches(dataSource, goalKpis, 100); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary await in return statement
In an async function, using return await is redundant because the returned promise will be awaited automatically. You can simplify the code by removing the await.
Apply this diff to simplify the return statement:
-return await insertRandomGoalKpiInBatches(dataSource, goalKpis, 100);
+return insertRandomGoalKpiInBatches(dataSource, goalKpis, 100);📝 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.
| return await insertRandomGoalKpiInBatches(dataSource, goalKpis, 100); | |
| return insertRandomGoalKpiInBatches(dataSource, goalKpis, 100); |
| await dataSource.transaction(async (manager) => { | ||
| await manager.save(batch); | ||
| }); | ||
| insertedGoalKpis.push(...batch); |
There was a problem hiding this comment.
Use returned entities from save method to capture generated values
When saving entities using manager.save(batch), the method returns the saved entities, including any generated values like IDs or timestamps. Currently, you're adding the original batch to insertedGoalKpis, which might miss these updates. Capture the returned entities instead.
Apply this diff to collect the saved entities:
await dataSource.transaction(async (manager) => {
- await manager.save(batch);
+ const savedBatch = await manager.save(batch);
+ insertedGoalKpis.push(...savedBatch);
});
-insertedGoalKpis.push(...batch);📝 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.
| await dataSource.transaction(async (manager) => { | |
| await manager.save(batch); | |
| }); | |
| insertedGoalKpis.push(...batch); | |
| await dataSource.transaction(async (manager) => { | |
| const savedBatch = await manager.save(batch); | |
| insertedGoalKpis.push(...savedBatch); | |
| }); |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 088315d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution
Sent with 💌 from NxCloud. |
* feat: #8386 global logging API middleware * fix: #8386 save API response into DB * fix: store missing user id to the tabele * fix: favorite entity type use global entity enum * fix: #8386 api call log module middleware routes * fix: improvement suggested by CodeRabbit * fix: #8386 recursively process nested objects and arrays * fix: updated logger * feat(ActivityLog): Optimize beforeEntityCreate and beforeEntityUpdate methods - Centralized serialization logic for SQLite databases into a single method `serializeDataForSQLite`. - Refactored `beforeEntityCreate` and `beforeEntityUpdate` methods to delegate serialization logic to the new method. * fix(cspell): typo spelling :-) * fix: no need to store empty object into DB * fix(api-call-log): correct middleware configuration for time tracking routes - Replaced the unsupported regular expression pattern in route paths with the correct wildcard pattern ('/timesheet/*') in the `ApiCallLogMiddleware`. * feat: login components to handle workspace login and navigation * fix: rename `_errorHandlingService` to `errorHandlingService` and update type annotations in `NgxLoginComponent` constructor and `tap` callback. * resolves #8379 by fixing late desktop theme listener setup * fix: login components to handle workspace navigation and state * fix: #8386 updated call log middleware * fix: #8386 added http method transformer pipe * fix: ensure robust extraction of token from authorization header * fix: optimize code based on coderabbitai suggestion * fix: apply filters on logging API * refactor: time tracker component to combine conditions for retrieving remote log and handle API request * feat: add two function calls to updateTaskStatus and updateOrganizationTeamEmployee on silent restart * fix: improve ActorTypeTransformerPipe for integer storage * feat: add missing `version` property to timer objects in SequenceQueue and TimeTrackerComponent. * fix: optimize code based on coderabbitai suggestion * fix: optimize code based on coderabbitai suggestion * [Fix] View and Resource Links Activity Logs (#8409) * fix: organization sprint update activity log * feat: task view activity log * feat: resource link activity log * fix: removed unecessary js-docs * fix: use global base entity enum * fix: CodeRabbit suggestions * fix: CodeRabbit suggestions --------- Co-authored-by: Ruslan Konviser <evereq@gmail.com> * [Feat] Selector update persistence electron side (#8421) * feat: create electron selector service * feat: electronService with selectorElectronService in client-selector, note, project-selector, task-selector, and team-selector components * fix: rename 'refresh' method to 'clear' in client-selector, project-selector, task-selector, and team-selector components * feat: add conditional check for `this.useStore` in `clear()` method of ClientSelector, ProjectSelector, TaskSelector, and TeamSelector components. * [Fix] Seed process not working (#8422) * [Fix] Seed process not working * [Fix] Seed process not working * chore: windows builds --------- Co-authored-by: Rahul R. <rahulrathore576@gmail.com> Co-authored-by: GloireMutaliko21 <mufunyig@gmail.com> Co-authored-by: Kifungo A <45813955+adkif@users.noreply.github.com> Co-authored-by: Gloire Mutaliko (Salva) <86450367+GloireMutaliko21@users.noreply.github.com> Co-authored-by: samuel mbabhazi <111171386+samuelmbabhazi@users.noreply.github.com>
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation