Feature/bulk progress creation #2426
Conversation
Summary by CodeRabbit
WalkthroughA bulk creation feature for progress records was introduced. This includes a new controller, model method, validation middleware, and a dedicated POST route for handling multiple progress records in a single request. The implementation ensures validation, error handling, and batch database operations, while also updating exports and route definitions as needed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant AuthMiddleware
participant BulkValidation
participant ProgressController
participant ProgressModel
participant Firestore
Client->>Router: POST /progresses/bulk (records[])
Router->>AuthMiddleware: Authenticate user
AuthMiddleware->>BulkValidation: Validate records array
BulkValidation->>ProgressController: Pass validated request
ProgressController->>ProgressModel: createBulkProgressDocuments(records)
ProgressModel->>Firestore: Batch write (valid records)
ProgressModel-->>ProgressController: {successes, failures}
ProgressController-->>Client: Response with summary
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
-
controllers/progresses.js(2 hunks) -
middlewares/validators/progresses.js(3 hunks) -
models/progresses.js(1 hunks) -
routes/progresses.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
models/progresses.js
[error] 159-159: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 159-159: Delete ··
(prettier/prettier)
[error] 164-164: Insert ,
(prettier/prettier)
[error] 166-166: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 166-166: Delete ··
(prettier/prettier)
[error] 171-171: 'userId' is assigned a value but never used.
(no-unused-vars)
[error] 172-172: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 172-172: Delete ········
(prettier/prettier)
[error] 177-177: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 177-177: Delete ········
(prettier/prettier)
[error] 181-181: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 181-181: Delete ········
(prettier/prettier)
[error] 185-185: Replace ·?·null·:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`` with ⏎············?·null⏎············:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`,`
(prettier/prettier)
[error] 191-191: Insert ,
(prettier/prettier)
[error] 196-196: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 196-196: Delete ··
(prettier/prettier)
[error] 203-203: Insert ,
(prettier/prettier)
[error] 207-207: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 207-207: Delete ·
(prettier/prettier)
[error] 208-208: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 208-208: Delete ·
(prettier/prettier)
[error] 209-209: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 209-209: Delete ·
(prettier/prettier)
[error] 210-210: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 210-210: Replace · with ,
(prettier/prettier)
[error] 212-212: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 212-212: Delete ······
(prettier/prettier)
[error] 215-215: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 215-215: Delete ······
(prettier/prettier)
[error] 219-219: Insert ,
(prettier/prettier)
[error] 223-223: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 223-223: Delete ··
(prettier/prettier)
[error] 228-228: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 228-228: Delete ··
(prettier/prettier)
controllers/progresses.js
[error] 223-225: Delete ⏎⏎
(prettier/prettier)
[error] 286-286: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 286-286: Delete ··
(prettier/prettier)
[error] 289-289: Replace record with (record)
(prettier/prettier)
[error] 291-291: Insert ,
(prettier/prettier)
[error] 293-293: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 293-293: Delete ····
(prettier/prettier)
[error] 295-295: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 295-295: Delete ····
(prettier/prettier)
[error] 302-302: Insert ,
(prettier/prettier)
[error] 303-303: Insert ,
(prettier/prettier)
[error] 313-313: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 313-313: Delete ·
(prettier/prettier)
[error] 314-314: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 314-314: Delete ·
(prettier/prettier)
[error] 315-315: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 315-315: Delete ·
(prettier/prettier)
[error] 316-316: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 316-316: Delete ·
(prettier/prettier)
[error] 318-318: Insert ,
(prettier/prettier)
middlewares/validators/progresses.js
[error] 39-39: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 39-39: Delete ··
(prettier/prettier)
[error] 46-46: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 46-46: Delete ··
(prettier/prettier)
[error] 161-161: 'baseProgressSchema' is assigned a value but never used.
(no-unused-vars)
[error] 162-162: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 162-162: Delete ··
(prettier/prettier)
[error] 193-193: Replace .string().trim() with ⏎··············.string()⏎··············.trim()⏎··············
(prettier/prettier)
[error] 194-194: Insert ··
(prettier/prettier)
[error] 195-195: Replace ·············· with ················
(prettier/prettier)
[error] 196-196: Insert ··
(prettier/prettier)
[error] 197-197: Replace ················ with ··················
(prettier/prettier)
[error] 198-198: Insert ··
(prettier/prettier)
[error] 199-199: Replace ·············· with ················
(prettier/prettier)
[error] 200-200: Insert ··
(prettier/prettier)
[error] 201-201: Insert ··
(prettier/prettier)
[error] 202-202: Insert ··
(prettier/prettier)
🪛 Biome (1.9.4)
middlewares/validators/progresses.js
[error] 195-195: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (2)
routes/progresses.ts (1)
21-22: Well-structured new route for bulk creation.The implementation of a dedicated route for bulk progress creation follows best practices by reusing authentication and adding specific validation middleware. The clear comment above the route improves maintainability.
middlewares/validators/progresses.js (1)
4-35: Great refactoring to create a reusable schema.Extracting the common validation logic into a reusable function improves code maintainability and reduces duplication. This is a good pattern.
| /** | ||
| * Creates multiple progress documents in bulk. | ||
| * @param {Object} req - The HTTP request object. | ||
| * @param {Object} req.body - The request body containing an array of progress records. | ||
| * @param {Array<ProgressRequestBody>} req.body.records - Array of progress records to create. | ||
| * @param {Object} res - The HTTP response object. | ||
| * @returns {Promise<void>} A Promise that resolves when the response is sent. | ||
| */ | ||
| const createBulkProgress = async (req, res) => { | ||
| if (req.userData.roles.archived) { | ||
| return res.boom.forbidden(UNAUTHORIZED_WRITE); | ||
| } | ||
|
|
||
| const { records } = req.body; | ||
|
|
||
| try { | ||
| // Add userId to each record | ||
| const recordsWithUserId = records.map(record => ({ | ||
| ...record, | ||
| userId: req.userData.id | ||
| })); | ||
|
|
||
| const result = await progressesModel.createBulkProgressDocuments(recordsWithUserId); | ||
|
|
||
| return res.status(201).json({ | ||
| message: `Successfully created ${result.successCount} progress records`, | ||
| data: { | ||
| successCount: result.successCount, | ||
| failureCount: result.failureCount, | ||
| successfulRecords: result.successfulRecords, | ||
| failedRecords: result.failedRecords | ||
| } | ||
| }); | ||
| } catch (error) { | ||
| logger.error(`Error in bulk progress creation: ${error.message}`); | ||
| return res.status(500).json({ | ||
| message: INTERNAL_SERVER_ERROR_MESSAGE, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix formatting issues and enhance error handling.
The bulk progress creation controller logic is well-implemented, but has formatting issues and limited error handling.
Apply these formatting fixes:
const createBulkProgress = async (req, res) => {
if (req.userData.roles.archived) {
return res.boom.forbidden(UNAUTHORIZED_WRITE);
}
const { records } = req.body;
-
+
try {
// Add userId to each record
- const recordsWithUserId = records.map(record => ({
+ const recordsWithUserId = records.map((record) => ({
...record,
- userId: req.userData.id
+ userId: req.userData.id,
}));
-
+
const result = await progressesModel.createBulkProgressDocuments(recordsWithUserId);
-
+
return res.status(201).json({
message: `Successfully created ${result.successCount} progress records`,
data: {
successCount: result.successCount,
failureCount: result.failureCount,
successfulRecords: result.successfulRecords,
- failedRecords: result.failedRecords
+ failedRecords: result.failedRecords,
}
});
} catch (error) {
logger.error(`Error in bulk progress creation: ${error.message}`);
return res.status(500).json({
message: INTERNAL_SERVER_ERROR_MESSAGE,
});
}
};Consider enhancing error handling to differentiate between server errors and client errors (like validation failures that passed middleware but failed at model level).
📝 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.
| /** | |
| * Creates multiple progress documents in bulk. | |
| * @param {Object} req - The HTTP request object. | |
| * @param {Object} req.body - The request body containing an array of progress records. | |
| * @param {Array<ProgressRequestBody>} req.body.records - Array of progress records to create. | |
| * @param {Object} res - The HTTP response object. | |
| * @returns {Promise<void>} A Promise that resolves when the response is sent. | |
| */ | |
| const createBulkProgress = async (req, res) => { | |
| if (req.userData.roles.archived) { | |
| return res.boom.forbidden(UNAUTHORIZED_WRITE); | |
| } | |
| const { records } = req.body; | |
| try { | |
| // Add userId to each record | |
| const recordsWithUserId = records.map(record => ({ | |
| ...record, | |
| userId: req.userData.id | |
| })); | |
| const result = await progressesModel.createBulkProgressDocuments(recordsWithUserId); | |
| return res.status(201).json({ | |
| message: `Successfully created ${result.successCount} progress records`, | |
| data: { | |
| successCount: result.successCount, | |
| failureCount: result.failureCount, | |
| successfulRecords: result.successfulRecords, | |
| failedRecords: result.failedRecords | |
| } | |
| }); | |
| } catch (error) { | |
| logger.error(`Error in bulk progress creation: ${error.message}`); | |
| return res.status(500).json({ | |
| message: INTERNAL_SERVER_ERROR_MESSAGE, | |
| }); | |
| } | |
| }; | |
| const createBulkProgress = async (req, res) => { | |
| if (req.userData.roles.archived) { | |
| return res.boom.forbidden(UNAUTHORIZED_WRITE); | |
| } | |
| const { records } = req.body; | |
| try { | |
| // Add userId to each record | |
| const recordsWithUserId = records.map((record) => ({ | |
| ...record, | |
| userId: req.userData.id, | |
| })); | |
| const result = await progressesModel.createBulkProgressDocuments(recordsWithUserId); | |
| return res.status(201).json({ | |
| message: `Successfully created ${result.successCount} progress records`, | |
| data: { | |
| successCount: result.successCount, | |
| failureCount: result.failureCount, | |
| successfulRecords: result.successfulRecords, | |
| failedRecords: result.failedRecords, | |
| }, | |
| }); | |
| } catch (error) { | |
| logger.error(`Error in bulk progress creation: ${error.message}`); | |
| return res.status(500).json({ | |
| message: INTERNAL_SERVER_ERROR_MESSAGE, | |
| }); | |
| } | |
| }; |
🧰 Tools
🪛 ESLint
[error] 286-286: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 286-286: Delete ··
(prettier/prettier)
[error] 289-289: Replace record with (record)
(prettier/prettier)
[error] 291-291: Insert ,
(prettier/prettier)
[error] 293-293: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 293-293: Delete ····
(prettier/prettier)
[error] 295-295: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 295-295: Delete ····
(prettier/prettier)
[error] 302-302: Insert ,
(prettier/prettier)
[error] 303-303: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In controllers/progresses.js around lines 272 to 311, fix inconsistent
indentation and spacing to improve code readability. Enhance error handling by
checking if the caught error is a validation or client error and respond with a
400 status and detailed message; otherwise, respond with a 500 status for server
errors. Ensure the error logging captures full error details for debugging.
| module.exports = { | ||
| createProgress, | ||
| getProgress, | ||
| getProgressRangeData, | ||
| getProgressBydDateController, | ||
| createBulkProgress | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix trailing whitespace in module exports.
The module exports have trailing whitespace issues.
Apply this fix:
-module.exports = {
- createProgress,
- getProgress,
- getProgressRangeData,
+module.exports = {
+ createProgress,
+ getProgress,
+ getProgressRangeData,
getProgressBydDateController,
- createBulkProgress
+ createBulkProgress,
};📝 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.
| module.exports = { | |
| createProgress, | |
| getProgress, | |
| getProgressRangeData, | |
| getProgressBydDateController, | |
| createBulkProgress | |
| }; | |
| module.exports = { | |
| createProgress, | |
| getProgress, | |
| getProgressRangeData, | |
| getProgressBydDateController, | |
| createBulkProgress, | |
| }; |
🧰 Tools
🪛 ESLint
[error] 313-313: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 313-313: Delete ·
(prettier/prettier)
[error] 314-314: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 314-314: Delete ·
(prettier/prettier)
[error] 315-315: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 315-315: Delete ·
(prettier/prettier)
[error] 316-316: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 316-316: Delete ·
(prettier/prettier)
[error] 318-318: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In controllers/progresses.js around lines 313 to 319, remove any trailing
whitespace characters from the module.exports object to ensure clean and
consistent code formatting. Check each line within the export block and delete
spaces or tabs after the last character on the line.
| /** | ||
| * Creates multiple progress documents in a batch operation. | ||
| * @param {Array<Object>} progressDataArray - Array of progress data objects to create. | ||
| * @returns {Promise<Object>} A Promise that resolves with the result of the batch operation, | ||
| * including counts of successful and failed operations and details of each. | ||
| */ | ||
| const createBulkProgressDocuments = async (progressDataArray) => { | ||
| const batch = fireStore.batch(); | ||
| const createdAtTimestamp = new Date().getTime(); | ||
| const progressDateTimestamp = getProgressDateTimestamp(); | ||
|
|
||
| const result = { | ||
| successCount: 0, | ||
| failureCount: 0, | ||
| successfulRecords: [], | ||
| failedRecords: [] | ||
| }; | ||
|
|
||
| // First, check for existing progress documents for the current day | ||
| const existingProgressChecks = await Promise.all( | ||
| progressDataArray.map(async (progressData) => { | ||
| try { | ||
| const { type, taskId, userId } = progressData; | ||
|
|
||
| // Validate task exists if taskId is provided | ||
| if (taskId) { | ||
| await assertTaskExists(taskId); | ||
| } | ||
|
|
||
| // Check if progress already exists for today | ||
| const query = buildQueryForPostingProgress(progressData); | ||
| const existingDocumentSnapshot = await query.where("date", "==", progressDateTimestamp).get(); | ||
|
|
||
| return { | ||
| progressData, | ||
| exists: !existingDocumentSnapshot.empty, | ||
| error: existingDocumentSnapshot.empty ? null : `${type.charAt(0).toUpperCase() + type.slice(1)} ${PROGRESS_ALREADY_CREATED}` | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| progressData, | ||
| exists: false, | ||
| error: error.message | ||
| }; | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| // Process records that don't have existing progress for today | ||
| existingProgressChecks.forEach((check) => { | ||
| if (check.error) { | ||
| result.failureCount++; | ||
| result.failedRecords.push({ | ||
| record: check.progressData, | ||
| error: check.error | ||
| }); | ||
| } else { | ||
| // Add to batch | ||
| const progressDocumentData = { | ||
| ...check.progressData, | ||
| createdAt: createdAtTimestamp, | ||
| date: progressDateTimestamp | ||
| }; | ||
|
|
||
| const docRef = progressesCollection.doc(); | ||
| batch.set(docRef, progressDocumentData); | ||
|
|
||
| result.successCount++; | ||
| result.successfulRecords.push({ | ||
| id: docRef.id, | ||
| ...progressDocumentData | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // Commit the batch if there are any successful records | ||
| if (result.successCount > 0) { | ||
| await batch.commit(); | ||
| } | ||
|
|
||
| return result; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider breaking down the lengthy function.
The createBulkProgressDocuments function is quite long and handles multiple concerns. Consider breaking it down into smaller helper functions for better readability and maintainability.
You could extract the validation logic and batch creation into separate helper functions:
/**
* Validates a single progress record
* @param {Object} progressData - Progress data to validate
* @returns {Promise<Object>} - Validation result with error if any
*/
const validateProgressRecord = async (progressData) => {
try {
const { type, taskId } = progressData;
// Validate task exists if taskId is provided
if (taskId) {
await assertTaskExists(taskId);
}
// Check if progress already exists for today
const query = buildQueryForPostingProgress(progressData);
const progressDateTimestamp = getProgressDateTimestamp();
const existingDocumentSnapshot = await query.where("date", "==", progressDateTimestamp).get();
return {
progressData,
exists: !existingDocumentSnapshot.empty,
error: existingDocumentSnapshot.empty ? null : `${type.charAt(0).toUpperCase() + type.slice(1)} ${PROGRESS_ALREADY_CREATED}`
};
} catch (error) {
return {
progressData,
exists: false,
error: error.message
};
}
};
/**
* Creates multiple progress documents in a batch operation.
* @param {Array<Object>} progressDataArray - Array of progress data objects to create.
* @returns {Promise<Object>} A Promise that resolves with the result of the batch operation.
*/
const createBulkProgressDocuments = async (progressDataArray) => {
// ... Rest of the function with calls to the helper methods
}🧰 Tools
🪛 ESLint
[error] 159-159: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 159-159: Delete ··
(prettier/prettier)
[error] 164-164: Insert ,
(prettier/prettier)
[error] 166-166: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 166-166: Delete ··
(prettier/prettier)
[error] 171-171: 'userId' is assigned a value but never used.
(no-unused-vars)
[error] 172-172: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 172-172: Delete ········
(prettier/prettier)
[error] 177-177: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 177-177: Delete ········
(prettier/prettier)
[error] 181-181: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 181-181: Delete ········
(prettier/prettier)
[error] 185-185: Replace ·?·null·:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`` with ⏎············?·null⏎············:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`,`
(prettier/prettier)
[error] 191-191: Insert ,
(prettier/prettier)
[error] 196-196: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 196-196: Delete ··
(prettier/prettier)
[error] 203-203: Insert ,
(prettier/prettier)
[error] 207-207: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 207-207: Delete ·
(prettier/prettier)
[error] 208-208: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 208-208: Delete ·
(prettier/prettier)
[error] 209-209: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 209-209: Delete ·
(prettier/prettier)
[error] 210-210: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 210-210: Replace · with ,
(prettier/prettier)
[error] 212-212: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 212-212: Delete ······
(prettier/prettier)
[error] 215-215: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 215-215: Delete ······
(prettier/prettier)
[error] 219-219: Insert ,
(prettier/prettier)
[error] 223-223: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 223-223: Delete ··
(prettier/prettier)
[error] 228-228: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 228-228: Delete ··
(prettier/prettier)
🤖 Prompt for AI Agents
In models/progresses.js from lines 149 to 230, the createBulkProgressDocuments
function is too long and mixes validation and batch creation logic. Refactor by
extracting the validation logic into a separate async helper function, e.g.,
validateProgressRecord, which checks task existence and existing progress for
the day, returning validation results with errors if any. Then simplify
createBulkProgressDocuments to call this helper for each record, handle results,
and perform batch writes accordingly. This will improve readability and
maintainability.
| // First, check for existing progress documents for the current day | ||
| const existingProgressChecks = await Promise.all( | ||
| progressDataArray.map(async (progressData) => { | ||
| try { | ||
| const { type, taskId, userId } = progressData; | ||
|
|
||
| // Validate task exists if taskId is provided | ||
| if (taskId) { | ||
| await assertTaskExists(taskId); | ||
| } | ||
|
|
||
| // Check if progress already exists for today | ||
| const query = buildQueryForPostingProgress(progressData); | ||
| const existingDocumentSnapshot = await query.where("date", "==", progressDateTimestamp).get(); | ||
|
|
||
| return { | ||
| progressData, | ||
| exists: !existingDocumentSnapshot.empty, | ||
| error: existingDocumentSnapshot.empty ? null : `${type.charAt(0).toUpperCase() + type.slice(1)} ${PROGRESS_ALREADY_CREATED}` | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| progressData, | ||
| exists: false, | ||
| error: error.message | ||
| }; | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider transaction for atomicity.
The current implementation checks for existing progress documents and validates tasks serially, which is good but could lead to race conditions if multiple requests try to create progress for the same user/task on the same day.
Consider using Firestore transactions instead of just batches to ensure atomicity and prevent race conditions. Transactions would allow you to check for existing documents and create new ones in a single atomic operation.
🧰 Tools
🪛 ESLint
[error] 171-171: 'userId' is assigned a value but never used.
(no-unused-vars)
[error] 172-172: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 172-172: Delete ········
(prettier/prettier)
[error] 177-177: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 177-177: Delete ········
(prettier/prettier)
[error] 181-181: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 181-181: Delete ········
(prettier/prettier)
[error] 185-185: Replace ·?·null·:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`` with ⏎············?·null⏎············:·${type.charAt(0).toUpperCase()·+·type.slice(1)}·${PROGRESS_ALREADY_CREATED}`,`
(prettier/prettier)
[error] 191-191: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In models/progresses.js around lines 167 to 195, the current code checks for
existing progress documents and validates tasks outside of a transaction, which
can cause race conditions. Refactor this logic to use Firestore transactions
that atomically check for existing progress and create new documents within the
same transaction. This ensures that the existence check and creation happen as a
single atomic operation, preventing concurrent requests from creating duplicate
progress entries.
| result.successfulRecords.push({ | ||
| id: docRef.id, | ||
| ...progressDocumentData | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix object formatting in successfulRecords.
The object being pushed to successfulRecords is missing a trailing comma.
Apply this fix:
result.successfulRecords.push({
id: docRef.id,
- ...progressDocumentData
+ ...progressDocumentData,
});📝 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.
| result.successfulRecords.push({ | |
| id: docRef.id, | |
| ...progressDocumentData | |
| }); | |
| result.successfulRecords.push({ | |
| id: docRef.id, | |
| ...progressDocumentData, | |
| }); |
🧰 Tools
🪛 ESLint
[error] 219-219: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In models/progresses.js around lines 217 to 220, the object pushed to
successfulRecords is missing a trailing comma after the last property. Add a
trailing comma after the spread operator line to fix the object formatting.
| const createBulkProgressDocuments = async (progressDataArray) => { | ||
| const batch = fireStore.batch(); | ||
| const createdAtTimestamp = new Date().getTime(); | ||
| const progressDateTimestamp = getProgressDateTimestamp(); | ||
|
|
||
| const result = { | ||
| successCount: 0, | ||
| failureCount: 0, | ||
| successfulRecords: [], | ||
| failedRecords: [] | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix result object formatting.
The result object is missing a trailing comma after successfulRecords.
Apply this fix:
const result = {
successCount: 0,
failureCount: 0,
- successfulRecords: [],
+ successfulRecords: [],
failedRecords: []
};Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 ESLint
[error] 159-159: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 159-159: Delete ··
(prettier/prettier)
[error] 164-164: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In models/progresses.js around lines 155 to 165, the result object declaration
is missing a trailing comma after the successfulRecords property. Add a comma
after successfulRecords to properly format the object and avoid syntax errors.
| const progressDocumentData = { | ||
| ...check.progressData, | ||
| createdAt: createdAtTimestamp, | ||
| date: progressDateTimestamp | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix formatting in document data object.
The progress document data object has formatting issues.
Apply this fix:
- const progressDocumentData = {
- ...check.progressData,
- createdAt: createdAtTimestamp,
- date: progressDateTimestamp
+ const progressDocumentData = {
+ ...check.progressData,
+ createdAt: createdAtTimestamp,
+ date: progressDateTimestamp,
};📝 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 progressDocumentData = { | |
| ...check.progressData, | |
| createdAt: createdAtTimestamp, | |
| date: progressDateTimestamp | |
| }; | |
| const progressDocumentData = { | |
| ...check.progressData, | |
| createdAt: createdAtTimestamp, | |
| date: progressDateTimestamp, | |
| }; |
🧰 Tools
🪛 ESLint
[error] 207-207: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 207-207: Delete ·
(prettier/prettier)
[error] 208-208: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 208-208: Delete ·
(prettier/prettier)
[error] 209-209: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 209-209: Delete ·
(prettier/prettier)
[error] 210-210: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 210-210: Replace · with ,
(prettier/prettier)
🤖 Prompt for AI Agents
In models/progresses.js around lines 207 to 211, the progressDocumentData object
has inconsistent or unclear formatting. Reformat the object to have each
key-value pair on its own line with proper indentation and consistent spacing,
improving readability and maintaining code style standards.
| const validateBulkCreateProgressRecords = async (req, res, next) => { | ||
| const baseProgressSchema = createProgressRecordSchema(); | ||
|
|
||
| const bulkSchema = joi | ||
| .object() | ||
| .keys({ | ||
| records: joi | ||
| .array() | ||
| .min(1) | ||
| .max(50) | ||
| .items( | ||
| joi.object().keys({ | ||
| type: joi | ||
| .string() | ||
| .trim() | ||
| .valid(...VALID_PROGRESS_TYPES) | ||
| .required() | ||
| .messages({ | ||
| "any.required": "Required field 'type' is missing.", | ||
| "any.only": "Type field is restricted to either 'user' or 'task'.", | ||
| }), | ||
| completed: joi.string().trim().required().messages({ | ||
| "any.required": "Required field 'completed' is missing.", | ||
| "string.trim": "completed must not have leading or trailing whitespace", | ||
| }), | ||
| planned: joi.string().trim().required().messages({ | ||
| "any.required": "Required field 'planned' is missing.", | ||
| "string.trim": "planned must not have leading or trailing whitespace", | ||
| }), | ||
| blockers: joi.string().trim().allow("").required().messages({ | ||
| "any.required": "Required field 'blockers' is missing.", | ||
| "string.trim": "blockers must not have leading or trailing whitespace", | ||
| }), | ||
| taskId: joi.string().trim().when("type", { | ||
| is: "task", | ||
| then: joi.required().messages({ | ||
| "any.required": "Required field 'taskId' is missing for task type.", | ||
| "string.trim": "taskId must not have leading or trailing whitespace", | ||
| }), | ||
| otherwise: joi.forbidden().messages({ | ||
| "any.unknown": "taskId should not be provided for user type.", | ||
| }), | ||
| }), | ||
| }) | ||
| ) | ||
| .required() | ||
| .messages({ | ||
| "array.min": "At least one progress record is required.", | ||
| "array.max": "Maximum of 50 progress records can be created at once.", | ||
| "any.required": "Progress records array is required.", | ||
| }), | ||
| }) | ||
| .messages({ "object.unknown": "Invalid field provided." }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider reusing baseProgressSchema in bulk validation.
The baseProgressSchema is created but not used, and instead the validation rules are duplicated.
The validator creates baseProgressSchema but doesn't use it, instead duplicating the validation rules in the bulk schema. Consider reusing the schema to avoid duplication:
const validateBulkCreateProgressRecords = async (req, res, next) => {
const baseProgressSchema = createProgressRecordSchema();
const bulkSchema = joi
.object()
.keys({
records: joi
.array()
.min(1)
.max(50)
.items(
joi.object().keys({
- type: joi
- .string()
- .trim()
- .valid(...VALID_PROGRESS_TYPES)
- .required()
- .messages({
- "any.required": "Required field 'type' is missing.",
- "any.only": "Type field is restricted to either 'user' or 'task'.",
- }),
- completed: joi.string().trim().required().messages({
- "any.required": "Required field 'completed' is missing.",
- "string.trim": "completed must not have leading or trailing whitespace",
- }),
- planned: joi.string().trim().required().messages({
- "any.required": "Required field 'planned' is missing.",
- "string.trim": "planned must not have leading or trailing whitespace",
- }),
- blockers: joi.string().trim().allow("").required().messages({
- "any.required": "Required field 'blockers' is missing.",
- "string.trim": "blockers must not have leading or trailing whitespace",
- }),
+ ...baseProgressSchema.extract(),
taskId: joi.string().trim().when("type", {
is: "task",
then: joi.required().messages({
"any.required": "Required field 'taskId' is missing for task type.",
"string.trim": "taskId must not have leading or trailing whitespace",
}),
otherwise: joi.forbidden().messages({
"any.unknown": "taskId should not be provided for user type.",
}),
}),
})
)
.required()
.messages({
"array.min": "At least one progress record is required.",
"array.max": "Maximum of 50 progress records can be created at once.",
"any.required": "Progress records array is required.",
}),
})
.messages({ "object.unknown": "Invalid field provided." });Note: If Joi's .extract() method isn't available in your version, you might need to use another approach to reuse the schema keys.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 195-195: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 ESLint
[error] 161-161: 'baseProgressSchema' is assigned a value but never used.
(no-unused-vars)
[error] 162-162: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 162-162: Delete ··
(prettier/prettier)
[error] 193-193: Replace .string().trim() with ⏎··············.string()⏎··············.trim()⏎··············
(prettier/prettier)
[error] 194-194: Insert ··
(prettier/prettier)
[error] 195-195: Replace ·············· with ················
(prettier/prettier)
[error] 196-196: Insert ··
(prettier/prettier)
[error] 197-197: Replace ················ with ··················
(prettier/prettier)
[error] 198-198: Insert ··
(prettier/prettier)
[error] 199-199: Replace ·············· with ················
(prettier/prettier)
[error] 200-200: Insert ··
(prettier/prettier)
[error] 201-201: Insert ··
(prettier/prettier)
[error] 202-202: Insert ··
(prettier/prettier)
🤖 Prompt for AI Agents
In middlewares/validators/progresses.js between lines 160 and 212, the
baseProgressSchema is defined but not used in the bulk validation schema,
leading to duplicated validation rules. Refactor the bulkSchema to reuse
baseProgressSchema by referencing its keys or structure within the records array
items, avoiding duplication. If Joi's .extract() method is unavailable, manually
incorporate the keys from baseProgressSchema into the bulkSchema's records items
to maintain consistency and reduce redundancy.
| taskId: joi.string().trim().when("type", { | ||
| is: "task", | ||
| then: joi.required().messages({ | ||
| "any.required": "Required field 'taskId' is missing for task type.", | ||
| "string.trim": "taskId must not have leading or trailing whitespace", | ||
| }), | ||
| otherwise: joi.forbidden().messages({ | ||
| "any.unknown": "taskId should not be provided for user type.", | ||
| }), | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix Biome warning about 'then' property.
The static analysis tool Biome is warning about using 'then' as a property name, which could be confused with Promise's then method.
Consider renaming or refactoring the conditional validation to avoid the Biome warning. You could use Joi's when with a callback function instead:
- taskId: joi.string().trim().when("type", {
- is: "task",
- then: joi.required().messages({
- "any.required": "Required field 'taskId' is missing for task type.",
- "string.trim": "taskId must not have leading or trailing whitespace",
- }),
- otherwise: joi.forbidden().messages({
- "any.unknown": "taskId should not be provided for user type.",
- }),
- }),
+ taskId: joi.string().trim().when("type", (value, helpers) => {
+ return value === "task"
+ ? joi.required().messages({
+ "any.required": "Required field 'taskId' is missing for task type.",
+ "string.trim": "taskId must not have leading or trailing whitespace",
+ })
+ : joi.forbidden().messages({
+ "any.unknown": "taskId should not be provided for user type.",
+ });
+ }),Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 195-195: Do not add then to an object.
(lint/suspicious/noThenProperty)
🪛 ESLint
[error] 193-193: Replace .string().trim() with ⏎··············.string()⏎··············.trim()⏎··············
(prettier/prettier)
[error] 194-194: Insert ··
(prettier/prettier)
[error] 195-195: Replace ·············· with ················
(prettier/prettier)
[error] 196-196: Insert ··
(prettier/prettier)
[error] 197-197: Replace ················ with ··················
(prettier/prettier)
[error] 198-198: Insert ··
(prettier/prettier)
[error] 199-199: Replace ·············· with ················
(prettier/prettier)
[error] 200-200: Insert ··
(prettier/prettier)
[error] 201-201: Insert ··
(prettier/prettier)
[error] 202-202: Insert ··
(prettier/prettier)
🤖 Prompt for AI Agents
In middlewares/validators/progresses.js around lines 193 to 202, the use of
'then' as a property in the Joi conditional validation triggers a Biome warning
due to potential confusion with Promise's then method. Refactor the Joi.when
condition to use a callback function instead of the object with 'is' and 'then'
properties. Inside the callback, return the appropriate Joi schema for the
'taskId' field based on the 'type' value, ensuring the validation logic and
custom messages remain the same while avoiding the 'then' property usage.
|
@sam22kalvium provide detailed description and link to the issue this solves |
Date:
Developer Name:
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes