Create migration script for reporting #1316
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant FileModel
participant Database
User->>API: Upload file with source = "Report"
API->>FileModel: Create new File entry (source: "Report")
FileModel->>Database: Insert file record (source: "Report")
Database-->>FileModel: Confirmation
FileModel-->>API: File created
API-->>User: Success response
Assessment against linked issues
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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: 1
🧹 Nitpick comments (3)
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js (3)
1-1: Remove redundant 'use strict' directiveJavaScript modules are automatically in strict mode, so this directive is unnecessary.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
8-11: Consider adding a check if the enum value already existsTo make the migration more robust, consider checking if the enum value already exists before attempting to add it. This prevents errors when running the migration multiple times.
await queryInterface.sequelize.query( - `ALTER TYPE enum_files_source ADD VALUE 'Report';`, + `DO $$ + BEGIN + IF NOT EXISTS (SELECT 1 FROM pg_enum WHERE enumlabel = 'Report' AND + enumtypid = (SELECT oid FROM pg_type WHERE typname = 'enum_files_source')) THEN + ALTER TYPE enum_files_source ADD VALUE 'Report'; + END IF; + END $$;`, { transaction } );
19-21: Consider implementing a proper 'down' methodWhile removing enum values in PostgreSQL is complex and your comment is valid, it's good practice to have a complete migration. For a proper 'down' method, you could create a workaround to handle this limitation.
async down(queryInterface, Sequelize) { - // The enum value won't cause any issues if it is not removed + // PostgreSQL doesn't support removing enum values directly + // We would need to: + // 1. Create a new enum type without the 'Report' value + // 2. Update the column to use the new type + // 3. Drop the old type + // This is complex and potentially risky, so we're leaving it as is }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js(1 hunks)Servers/models/file.model.ts(3 hunks)Servers/utils/fileUpload.utils.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (2)
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js (1)
5-17: LGTM! Good use of transactions for safe migrationsThe migration correctly uses a transaction to ensure atomic changes, with proper error handling and rollback in case of failure. This is a best practice for database migrations.
Servers/models/file.model.ts (1)
11-11: LGTM! All type definitions and model configurations properly updatedThe changes consistently add "Report" as a valid source value across all relevant places:
- The
Fileinterface type definition- The
FileTypeinterface type definition- The Sequelize model's ENUM column definition and type annotation
This thorough approach ensures type safety and database consistency.
Also applies to: 21-21, 63-65
| @@ -10,7 +10,7 @@ export const uploadFile = async ( | |||
| file: UploadedFile, | |||
| user_id: number, | |||
| project_framework_id: number, | |||
| source: "Assessment tracker group" | "Compliance tracker group", | |||
| source: "Assessment tracker group" | "Compliance tracker group" | "Report", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Type definition correctly updated for 'Report' source
The type definition for the source parameter has been appropriately expanded to include the new "Report" option, maintaining consistency with the database migration and model changes.
Consider whether any downstream consumers of this function need to be updated to handle the new source type. If the source type affects file handling behavior elsewhere in the application, those areas might need updates as well.
🏁 Script executed:
#!/bin/bash
# Check for any other references to the source field that might need updating
rg -A 1 -B 1 "source: (\"Assessment tracker group\" \| \"Compliance tracker group\")" --type ts --glob "!Servers/utils/fileUpload.utils.ts" --glob "!Servers/models/file.model.ts"Length of output: 176
🏁 Script executed:
#!/bin/bash
# Search for any mentions of the source literal values in TypeScript files
rg "Assessment tracker group" --type ts
rg "Compliance tracker group" --type ts
# Inspect the 'source' property definition in the file model to ensure it's updated
rg "source" -A2 -B2 Servers/models/file.model.tsLength of output: 2570
Ensure ‘Report’ source is handled downstream
The type definition and model enum have been correctly expanded to include "Report", but a search across the codebase did not uncover any calls to uploadFile(..., "Report"):
- Controllers (
reporting.ctrl.ts,file.ctrl.ts) still only use"Assessment tracker group" subControl.utils.tsonly uses"Compliance tracker group"- No other TS files pass
"Report"touploadFile
Please add or update any controllers, services, or UI components that should upload files with the "Report" source to ensure this new option is actually used.
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js
Show resolved
Hide resolved
Servers/database/migrations/20250508061148-update-enum_files_source-reporting.js
Show resolved
Hide resolved
Describe your changes
Reportto file source enumCC: @eieimon
Write your issue number after "Fixes "
Fixes #1313
Please ensure all items are checked off before requesting a review:
Summary by CodeRabbit