Fixed bugs to ensure proper flow of prompts and remove repetitive prompts #2845
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
setup.ts (3)
226-226: Enhance error handling consistency.While the error handling improvements are good, consider adding more specific error information to help with troubleshooting.
- throw new Error("Could not connect to database to check for data"); + const error = err instanceof Error ? err.message : String(err); + throw new Error(`Failed to connect to database: ${error}`);Also applies to: 242-254
Line range hint
1276-1323: Consider extracting MongoDB connection retry logic.The MongoDB connection retry logic could be extracted into a separate function for better maintainability.
+async function waitForMongoDB(url: string, maxRetries: number = 30): Promise<void> { + let isConnected = false; + let retryCount = 0; + while (!isConnected) { + if (retryCount >= maxRetries) { + throw new Error(`Timed out waiting for MongoDB to be ready after ${maxRetries} seconds`); + } + try { + const client = new MongoClient(url); + await client.connect(); + await client.db().command({ ping: 1 }); + client.close(); + isConnected = true; + console.log("MongoDB is ready!"); + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + console.log( + `Waiting for MongoDB to be ready... Retry ${retryCount + 1}/${maxRetries}. Details: ${error}` + ); + await new Promise((resolve) => setTimeout(resolve, 1000)); + retryCount++; + } + } +}
1332-1335: Enhance setup completion message.Consider providing more specific information about what was successfully set up.
- "\nCongratulations! Talawa API has been successfully setup! 🥂🎉", + "\nCongratulations! Talawa API has been successfully set up! 🥂🎉\n" + + "- Database configuration: ✅\n" + + `- Default data imported: ✅\n` + + `- Sample data imported: ${shouldImportSampleData ? "✅" : "❌"}\n` + + "- Docker containers: " + (isDockerInstallation ? "✅" : "❌"),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
setup.ts(13 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check for linting, formatting, and type errors
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
setup.ts (3)
5-5: Import changes align with ES modules transition.The addition of
dirnameandfileURLToPathimports properly handles path resolution in ES modules, addressing the breaking change mentioned in the PR objectives.Also applies to: 34-34
145-149: Improved log path resolution and file management.The changes correctly handle path resolution for ES modules and improve log file management by:
- Using
dirname(fileURLToPath(import.meta.url))for proper path resolution- Creating logs directory if it doesn't exist
Also applies to: 159-159
Line range hint
1233-1275: Improved data import flow and user prompts.The changes provide clearer prompts and better separation between default and sample data import operations.
palisadoes
added
the
ignore-sensitive-files-pr
Yes I'm writing tests now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
setup.ts (1)
Line range hint
1233-1275: Add error handling for data import operations.The database setup flow has been improved, but consider adding:
- Error handling for failed data imports
- Rollback mechanism for partial import failures
- Progress indicators for long-running operations
if (shouldOverwriteData) { await wipeExistingData(process.env.MONGO_DB_URL); - await importDefaultData(); + try { + console.log("Importing default data..."); + await importDefaultData(); + console.log("Default data imported successfully."); + } catch (error) { + console.error("Failed to import default data:", error); + const { shouldRetry } = await inquirer.prompt({ + type: "confirm", + name: "shouldRetry", + message: "Would you like to retry importing default data?", + default: true, + }); + if (shouldRetry) { + await importDefaultData(); + } else { + throw new Error("Setup cannot continue without default data"); + } + }
🧹 Nitpick comments (5)
tests/setup/transactionLog.spec.ts (2)
6-16: Update test documentation to match the actual test cases.The documentation comments describe test cases for
askForSuperAdminEmailandsuperAdminfunctions, but this file is testing theaskForTransactionLogPathfunction. Please update the documentation to reflect the actual test cases in this file.
33-45: Consider adding assertions for prompt calls and error handling.While the test effectively verifies the retry behavior, consider adding assertions to:
- Verify that
inquirer.promptwas called exactly twice- Verify error messages shown to users match expected values
Example enhancement:
const result = await askForTransactionLogPath(); expect(result).toEqual(testPath); +expect(inquirer.prompt).toHaveBeenCalledTimes(2); +expect(console.error).toHaveBeenCalledWith( + "Invalid path or file does not exist. Please enter a valid file path." +);setup.ts (3)
Line range hint
172-202: Enhance user feedback in the prompt.Consider the following improvements:
- The default path shown in the prompt message should match the actual default path used in
transactionLogPath- Error messages could be more specific about permission issues
{ type: "input", name: "logPath", message: "Enter absolute path of log file:", - default: null, + default: path.join(process.cwd(), "logs", "transaction.log"), }, if (logPath && fs.existsSync(logPath)) { try { fs.accessSync(logPath, fs.constants.R_OK | fs.constants.W_OK); isValidPath = true; } catch { console.error( - "The file is not readable/writable. Please enter a valid file path.", + `The file exists but ${!fs.accessSync(logPath, fs.constants.R_OK) ? "is not readable" : "is not writable"}. Please ensure you have proper permissions.`, ); }
Line range hint
1276-1331: Enhance Docker setup reliability.Consider the following improvements:
- Move timeout values to constants
- Add health checks for Redis and MinIO services
- Add retry mechanism for Docker compose failures
+const MONGODB_READY_TIMEOUT_SECONDS = 30; +const MONGODB_CHECK_INTERVAL_MS = 1000; + if (shouldStartDockerContainers) { console.log("Starting docker container..."); try { await runDockerComposeWithLogs(); console.log("Docker containers have been built successfully!"); - // Wait for mongoDB to be ready - console.log("Waiting for mongoDB to be ready..."); + // Wait for services to be ready + console.log("Waiting for services to be ready..."); let isConnected = false; - const maxRetries = 30; // 30 seconds timeout + const maxRetries = MONGODB_READY_TIMEOUT_SECONDS; let retryCount = 0; + + // Check MongoDB readiness while (!isConnected) { if (retryCount >= maxRetries) { throw new Error( - "Timed out waiting for MongoDB to be ready after 30 seconds", + `Timed out waiting for MongoDB to be ready after ${MONGODB_READY_TIMEOUT_SECONDS} seconds`, ); } try { const client = new MongoClient(process.env.MONGO_DB_URL as string); await client.connect(); await client.db().command({ ping: 1 }); client.close(); isConnected = true; console.log("MongoDB is ready!"); } catch (err) { const error = err instanceof Error ? err.message : String(err); console.log( `Waiting for MongoDB to be ready... Retry ${retryCount + 1}/${maxRetries}. Details: ${error}`, ); - await new Promise((resolve) => setTimeout(resolve, 1000)); + await new Promise((resolve) => setTimeout(resolve, MONGODB_CHECK_INTERVAL_MS)); retryCount++; } } + + // Check Redis readiness + console.log("Checking Redis connection..."); + await checkRedisConnection(`redis://${process.env.REDIS_HOST}:${process.env.REDIS_PORT}`); + console.log("Redis is ready!"); + + // Check MinIO readiness + console.log("Checking MinIO connection..."); + // Add MinIO health check here + console.log("MinIO is ready!");
1332-1335: Enhance the success message with setup details.Consider adding more information to help users get started:
- console.log( - "\nCongratulations! Talawa API has been successfully setup! 🥂🎉", - ); + console.log(` + \nCongratulations! Talawa API has been successfully setup! 🥂🎉 + + Your setup details: + - API running on port: ${process.env.SERVER_PORT} + - MongoDB URL: ${process.env.MONGO_DB_URL} + - Redis host: ${process.env.REDIS_HOST} + - MinIO endpoint: ${process.env.MINIO_ENDPOINT} + + To start the server, run: npm start + For more information, visit: https://github.com/PalisadoesFoundation/talawa-api + `);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts(15 hunks)tests/setup/transactionLog.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check for linting, formatting, and type errors
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (2)
tests/setup/transactionLog.spec.ts (1)
22-31: LGTM! Well-structured test case.The test effectively covers the success scenario with proper mocking of dependencies and validation of the result.
setup.ts (1)
Line range hint
143-171: LGTM! Proper ES modules path resolution implementation.The function correctly handles the transition from CommonJS to ES modules by using
fileURLToPathanddirnamefor path resolution while maintaining the existing functionality.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2845 +/- ##
========================================
Coverage 97.68% 97.68%
========================================
Files 364 364
Lines 18563 18563
Branches 2677 2677
========================================
Hits 18133 18133
Misses 425 425
Partials 5 5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
setup.ts (2)
Line range hint
143-170: Add write permission validation for log directoryThe function should validate write permissions for the log directory before setting it as the log path.
export function transactionLogPath(logPath: string | null): void { const config = dotenv.parse(fs.readFileSync(".env")); const currDir = dirname(fileURLToPath(import.meta.url)); config.LOG = "true"; if (!logPath) { const defaultLogPath = path.resolve(currDir, "logs"); const defaultLogFile = path.join(defaultLogPath, "transaction.log"); + + // Validate write permissions + try { + if (!fs.existsSync(defaultLogPath)) { + fs.mkdirSync(defaultLogPath, { recursive: true }); + } + // Test write permissions + fs.accessSync(defaultLogPath, fs.constants.W_OK); + } catch (error) { + throw new Error(`No write permission for log directory: ${defaultLogPath}`); + } - if (!fs.existsSync(defaultLogPath)) { - console.log("Creating logs/transaction.log file..."); - fs.mkdirSync(defaultLogPath); - }
Line range hint
1037-1365: Consider breaking down the main functionThe main function is quite long and handles many responsibilities. Consider breaking it down into smaller, focused functions for better maintainability.
Example refactor:
async function configureTokens(): Promise<void> { let accessToken: string | null = ""; let refreshToken: string | null = ""; // ... token configuration logic ... await accessAndRefreshTokens(accessToken, refreshToken); } async function configureLogging(): Promise<void> { const { shouldLog } = await inquirer.prompt({ type: "confirm", name: "shouldLog", message: "Would you like to enable logging for the database transactions?", default: true, }); // ... logging configuration logic ... } async function main(): Promise<void> { console.log("Welcome to the Talawa API setup! 🚀"); await initializeEnvironment(); await configureTokens(); await configureLogging(); // ... other configuration steps ... }
🧹 Nitpick comments (6)
tests/setup/dataImportFlow.spec.ts (3)
17-52: Test description could be more preciseThe test description "should import sample data if the database is empty and user opts to import" doesn't fully reflect what's being tested. Consider renaming to clarify that it tests the flow where sample data is imported instead of default data.
- it("should import sample data if the database is empty and user opts to import", async () => { + it("should import sample data (not default data) when database is empty and user chooses to import sample data", async () => {
17-52: Extract common mock setup to beforeEachThe mock setup code is duplicated across test cases. Consider extracting it to the
beforeEachblock to improve maintainability and reduce duplication.describe("Data Importation Without Docker", () => { const mockEnv = { ...process.env }; // Backup the environment variables + let checkDbMock: vi.Mock; + let wipeExistingDataMock: vi.Mock; + let importDataMock: vi.Mock; + let importDefaultDataMock: vi.Mock; beforeEach(() => { process.env.MONGO_DB_URL = "mongodb://localhost:27017/sample-table"; vi.resetAllMocks(); + + checkDbMock = vi + .fn() + .mockImplementation(async (): Promise<boolean> => { + return true; + }); + + wipeExistingDataMock = vi + .fn() + .mockImplementation(async (): Promise<void> => { + return Promise.resolve(); + }); + + importDataMock = vi + .fn() + .mockImplementation(async (): Promise<void> => { + return Promise.resolve(); + }); + + importDefaultDataMock = vi + .fn() + .mockImplementation(async (): Promise<void> => { + return Promise.resolve(); + }); });Also applies to: 54-89, 91-126, 128-163, 165-200
252-287: Enhance error scenario testingThe error scenario test doesn't validate the actual error message. Consider adding assertions to verify the error details.
it("should terminate execution if error is encountered while starting containers", async () => { + const errorMessage = "Error starting containers"; const runDockerComposeMock = vi .fn() .mockImplementation(async (): Promise<void> => { - throw new Error("Error starting containers"); + throw new Error(errorMessage); }); // ... other mocks ... - await dataImportWithDocker( + await expect(dataImportWithDocker( runDockerComposeMock, importDefaultDataMock, importDataMock, connectDatabaseMock, - ); + )).rejects.toThrow(errorMessage);setup.ts (3)
Line range hint
1-34: Consider organizing imports by categoryWhile not critical, organizing imports into categories (built-in modules, external dependencies, internal modules) would improve readability.
+// Built-in Node.js modules import * as cryptolib from "crypto"; -import dotenv from "dotenv"; import fs from "fs"; -import inquirer from "inquirer"; import path, { dirname } from "path"; import type { ExecException } from "child_process"; import { exec, spawn, execSync } from "child_process"; +import { fileURLToPath } from "url"; + +// External dependencies +import dotenv from "dotenv"; +import inquirer from "inquirer"; import { MongoClient } from "mongodb"; + +// Internal modules import { MAXIMUM_IMAGE_SIZE_LIMIT_KB } from "@constants"; -import { - askForMongoDBUrl, - checkConnection, - checkExistingMongoDB, -} from "@setup/MongoDB"; +// ... rest of internal imports
971-999: Make database connection retry configuration customizableThe retry logic uses hardcoded values. Consider making these configurable through environment variables.
async function connectToDatabase(): Promise<void> { console.log("Waiting for mongoDB to be ready..."); let isConnected = false; - const maxRetries = 30; // 30 seconds timeout + const maxRetries = Number(process.env.MONGODB_MAX_RETRIES) || 30; + const retryInterval = Number(process.env.MONGODB_RETRY_INTERVAL) || 1000; // milliseconds let retryCount = 0; while (!isConnected) { if (retryCount >= maxRetries) { throw new Error( - "Timed out waiting for MongoDB to be ready after 30 seconds", + `Timed out waiting for MongoDB to be ready after ${maxRetries * retryInterval / 1000} seconds`, ); } // ... rest of the function - await new Promise((resolve) => setTimeout(resolve, 1000)); + await new Promise((resolve) => setTimeout(resolve, retryInterval));
Line range hint
471-488: Improve Docker Compose command detectionThe function could be more robust in detecting Docker Compose versions and handling errors.
export function getDockerComposeCommand(): { command: string; args: string[] } { let dockerComposeCmd = "docker-compose"; // Default to v1 let args = ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"]; try { - // Test if 'docker compose' works (v2) - execSync("docker compose version", { stdio: "ignore" }); + // Check for Docker Compose v2 + const result = execSync("docker compose version", { encoding: 'utf8' }); + if (result.toLowerCase().includes('v2.')) { + console.log("Using Docker Compose v2"); + dockerComposeCmd = "docker"; + args = ["compose", ...args]; + } else { + console.log("Using Docker Compose v1"); + } - dockerComposeCmd = "docker"; - args = ["compose", ...args]; // Prefix 'compose' for v2 } catch (error) { - console.log(error); + console.log("Docker Compose v2 not detected, falling back to v1"); dockerComposeCmd = process.platform === "win32" ? "docker-compose.exe" : "docker-compose"; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts(14 hunks)tests/setup/dataImportFlow.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
tests/setup/dataImportFlow.spec.ts (1)
Line range hint
1021-1027: Inconsistent default values in promptsThe default value for importing sample data differs between Docker and non-Docker flows:
- Docker flow:
default: true- Non-Docker flow:
default: falseThis inconsistency might confuse users. Consider aligning the defaults or documenting the reason for the difference.
Also applies to: 385-386
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
tests/setup/dataImportFlow.spec.ts (4)
Line range hint
918-975: Refactor data import function for better separation of concerns.The
dataImportWithoutDockerfunction handles too many responsibilities. Consider splitting it into smaller, focused functions.
- Extract user interaction logic:
async function promptForDataImport(isEmptyDb: boolean): Promise<{ shouldOverwrite: boolean; shouldImportSample: boolean; }> { if (!isEmptyDb) { const { shouldOverwriteData } = await inquirer.prompt({ type: "confirm", name: "shouldOverwriteData", message: "Do you want to delete the existing data and import required default Data?", default: false, }); if (!shouldOverwriteData) { return { shouldOverwrite: false, shouldImportSample: false }; } } const { shouldImportSample } = await inquirer.prompt({ type: "confirm", name: "shouldImportSample", message: "Do you want to import Talawa sample data for testing?", default: false, }); return { shouldOverwrite: !isEmptyDb, shouldImportSample }; }
- Add better error handling:
} catch (error) { + if (error instanceof Error) { + if (error.message.includes("ECONNREFUSED")) { + throw new Error("Database connection refused. Please check if the database is running."); + } + if (error.message.includes("Authentication failed")) { + throw new Error("Database authentication failed. Please check your credentials."); + } + } throw new Error( `Failed to check database: ${error instanceof Error ? error.message : String(error)}`, ); }
Line range hint
977-1005: Enhance database connection handling.The connection retry logic could be more robust and configurable.
- Extract retry configuration:
interface RetryConfig { maxRetries: number; retryDelay: number; timeout: number; } const DEFAULT_RETRY_CONFIG: RetryConfig = { maxRetries: 30, retryDelay: 1000, timeout: 30000, }; async function connectToDatabase(config: RetryConfig = DEFAULT_RETRY_CONFIG): Promise<void> { console.log("Waiting for mongoDB to be ready..."); let client: MongoClient | null = null; try { await withRetry(async () => { client = new MongoClient(process.env.MONGO_DB_URL as string); await client.connect(); await client.db().command({ ping: 1 }); console.log("MongoDB is ready!"); }, config); } catch (err) { throw new Error(`Database connection failed: ${err instanceof Error ? err.message : String(err)}`); } finally { if (client) { await client.close(); } } }
- Add helper for retry logic:
async function withRetry<T>( operation: () => Promise<T>, config: RetryConfig ): Promise<T> { let lastError: Error | null = null; for (let i = 0; i < config.maxRetries; i++) { try { return await operation(); } catch (err) { lastError = err instanceof Error ? err : new Error(String(err)); console.log( `Retry ${i + 1}/${config.maxRetries}. Error: ${lastError.message}` ); await new Promise(resolve => setTimeout(resolve, config.retryDelay)); } } throw new Error( `Operation failed after ${config.maxRetries} retries. Last error: ${lastError?.message}` ); }
Line range hint
471-490: Improve Docker command detection and error handling.The Docker command detection could be more robust and include better error handling.
export function getDockerComposeCommand(): { command: string; args: string[] } { + // Check if Docker CLI is available first + try { + execSync("docker --version", { stdio: "ignore" }); + } catch (error) { + throw new Error( + "Docker CLI not found. Please ensure Docker is installed correctly." + ); + } + let dockerComposeCmd = "docker-compose"; // Default to v1 let args = ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"]; try { // Test if 'docker compose' works (v2) execSync("docker compose version", { stdio: "ignore" }); dockerComposeCmd = "docker"; args = ["compose", ...args]; // Prefix 'compose' for v2 } catch (error) { - console.log(error); + console.log("Docker Compose v2 not detected, falling back to v1"); dockerComposeCmd = process.platform === "win32" ? "docker-compose.exe" : "docker-compose"; } return { command: dockerComposeCmd, args }; }
Line range hint
1007-1042: Add cleanup handling for Docker operations.The Docker import function should handle cleanup in case of failures.
export async function dataImportWithDocker( runDockerComposeWithLogs: () => Promise<void>, importDefaultData: () => Promise<void>, importData: () => Promise<void>, connectToDatabase: () => Promise<void>, ): Promise<void> { + let containersStarted = false; try { const { shouldStartDockerContainers } = await inquirer.prompt({ type: "confirm", name: "shouldStartDockerContainers", message: "Do you want to start the Docker containers now?", default: true, }); if (shouldStartDockerContainers) { console.log("Starting docker container..."); await runDockerComposeWithLogs(); + containersStarted = true; // ... rest of the code ... } } catch (err) { + if (containersStarted) { + console.log("Error occurred. Cleaning up containers..."); + try { + const { command, args } = getDockerComposeCommand(); + await new Promise((resolve, reject) => { + const cleanup = spawn(command, [...args.slice(0, 2), "down"]); + cleanup.on("error", reject); + cleanup.on("close", resolve); + }); + } catch (cleanupError) { + console.error("Failed to cleanup containers:", cleanupError); + } + } throw err; } }
🧹 Nitpick comments (7)
tests/setup/dataImportFlow.spec.ts (5)
6-6: Enhance environment variable backup.Consider using a more type-safe approach for environment variable backup.
- const mockEnv = { ...process.env }; // Backup the environment variables + const mockEnv: NodeJS.ProcessEnv = { ...process.env }; // Backup the environment variables
17-52: Improve test case naming and assertions.The test case name could be more descriptive of the exact scenario being tested. Also, consider adding assertions for the exact parameters passed to the mocked functions.
- it("should import sample data if the database is empty and user opts to import", async () => { + it("should import sample data without wiping when database is empty and user confirms import", async () => { // ... existing code ... expect(checkDbMock).toBeCalled(); + expect(checkDbMock).toHaveBeenCalledWith(process.env.MONGO_DB_URL); expect(wipeExistingDataMock).not.toBeCalled(); expect(importDefaultDataMock).not.toBeCalled(); expect(importDataMock).toBeCalled();
202-238: Add validation for error message content.The error handling test should validate the exact error message format.
await expect( dataImportWithoutDocker( checkDbMock, wipeExistingDataMock, importDefaultDataMock, importDataMock, ), - ).rejects.toThrow(errorMessage); + ).rejects.toThrow(/^Failed to check database: Database connection failed$/);
1-436: Add test coverage for additional edge cases.Consider adding test cases for the following scenarios:
- Network connectivity issues during database operations
- Permission errors when accessing Docker daemon
- Invalid data format during import
Example test case for network issues:
it("should handle network connectivity issues during database operations", async () => { const connectDatabaseMock = vi .fn() .mockRejectedValue(new Error("ECONNREFUSED")); vi.spyOn(inquirer, "prompt").mockResolvedValueOnce({ shouldStartDockerContainers: true, }); await expect( dataImportWithDocker( vi.fn(), vi.fn(), vi.fn(), connectDatabaseMock, ) ).rejects.toThrow(/ECONNREFUSED/); });
5-34: Organize imports for better maintainability.The imports could be better organized by grouping them into:
- Node.js built-in modules
- External dependencies
- Internal modules
-import path, { dirname } from "path"; -import type { ExecException } from "child_process"; -import { exec, spawn, execSync } from "child_process"; -import { MongoClient } from "mongodb"; +// Node.js built-in modules +import { dirname } from "path"; +import path from "path"; +import type { ExecException } from "child_process"; +import { exec, execSync, spawn } from "child_process"; +import { fileURLToPath } from "url"; +// External dependencies +import { MongoClient } from "mongodb"; +import dotenv from "dotenv"; +import inquirer from "inquirer"; +// Internal modulessetup.ts (2)
977-1005: Enhance retry mechanism with exponential backoff.While the connection handling is good, consider implementing exponential backoff for retries to prevent overwhelming the database server.
async function connectToDatabase(): Promise<void> { console.log("Waiting for mongoDB to be ready..."); let isConnected = false; const maxRetries = 30; let retryCount = 0; + const baseDelay = 1000; // 1 second while (!isConnected) { if (retryCount >= maxRetries) { throw new Error( "Timed out waiting for MongoDB to be ready after 30 seconds", ); } try { const client = new MongoClient(process.env.MONGO_DB_URL as string); await client.connect(); await client.db().command({ ping: 1 }); client.close(); isConnected = true; console.log("MongoDB is ready!"); } catch (err) { const error = err instanceof Error ? err.message : String(err); console.log( `Waiting for MongoDB to be ready... Retry ${retryCount + 1}/${maxRetries}. Details: ${error}`, ); - await new Promise((resolve) => setTimeout(resolve, 1000)); + const delay = Math.min(baseDelay * Math.pow(2, retryCount), 10000); // Cap at 10 seconds + await new Promise((resolve) => setTimeout(resolve, delay)); retryCount++; } } return Promise.resolve(); }
918-975: Extract common prompt logic to reduce duplication.Both data import functions have similar prompt handling code. Consider extracting the common prompt logic into a shared function.
async function promptForSampleDataImport(message: string = "Do you want to import Talawa sample data for testing and evaluation purposes?"): Promise<boolean> { const { shouldImportSampleData } = await inquirer.prompt({ type: "confirm", name: "shouldImportSampleData", message, default: false, }); return shouldImportSampleData; }This would simplify both functions and make future changes easier to maintain.
Also applies to: 1007-1042
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts(14 hunks)tests/setup/dataImportFlow.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (3)
setup.ts (3)
5-5: LGTM! Proper ES modules path handling.The changes correctly handle the transition from CommonJS to ES modules by using
dirnameandfileURLToPathinstead of__dirname.Also applies to: 34-34
924-935: Improved error handling with descriptive messages.The error handling has been enhanced with:
- Proper error propagation instead of console.log
- Detailed error messages
- Type-safe error handling using instanceof checks
1355-1367: LGTM! Clear separation of Docker and non-Docker flows.The main function now has a clearer separation of concerns between Docker and non-Docker environments, making the code more maintainable.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/setup/dataImportFlow.spec.ts (2)
17-239: Refactor duplicated mock setups in 'Data Importation Without Docker' testsThe mock implementations for
checkDbMock,wipeExistingDataMock,importDataMock, andimportDefaultDataMockare repeated across multiple tests. To improve maintainability and reduce redundancy, consider refactoring these mocks into shared variables initialized in thebeforeEachblock.Example refactoring:
let checkDbMock: ReturnType<typeof vi.fn>; let wipeExistingDataMock: ReturnType<typeof vi.fn>; let importDataMock: ReturnType<typeof vi.fn>; let importDefaultDataMock: ReturnType<typeof vi.fn>; beforeEach(() => { checkDbMock = vi.fn().mockResolvedValue(true); wipeExistingDataMock = vi.fn().mockResolvedValue(); importDataMock = vi.fn().mockResolvedValue(); importDefaultDataMock = vi.fn().mockResolvedValue(); vi.resetAllMocks(); });You can then adjust the return values or behaviors within individual tests as needed.
241-464: Refactor duplicated mock setups in 'Data Importation With Docker' testsSimilar to the previous suggestion, the mock implementations for
runDockerComposeMock,importDataMock,importDefaultDataMock, andconnectDatabaseMockare repeated across multiple tests. Refactoring them into shared variables initialized in thebeforeEachblock would enhance readability and maintainability.Example refactoring:
let runDockerComposeMock: ReturnType<typeof vi.fn>; let importDataMock: ReturnType<typeof vi.fn>; let importDefaultDataMock: ReturnType<typeof vi.fn>; let connectDatabaseMock: ReturnType<typeof vi.fn>; beforeEach(() => { runDockerComposeMock = vi.fn().mockResolvedValue(); importDataMock = vi.fn().mockResolvedValue(); importDefaultDataMock = vi.fn().mockResolvedValue(); connectDatabaseMock = vi.fn().mockResolvedValue(); vi.resetAllMocks(); });Override specific behaviors in individual tests when necessary.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/setup/dataImportFlow.spec.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Testing Application (22.x)
🔇 Additional comments (1)
tests/setup/dataImportFlow.spec.ts (1)
202-239: Excellent work incorporating the database connection failure testThe test case for handling database connection failures is well implemented and enhances the robustness of the test suite. It correctly mocks the rejection and asserts that the error is thrown, ensuring that the error handling logic works as expected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/setup/dataImportFlow.spec.ts (4)
5-15: Consider improving test environment setup.The test setup could be enhanced in the following ways:
- Extract the MongoDB URL to a constant or configuration file
- Be more selective about which environment variables to backup
+const TEST_MONGODB_URL = "mongodb://localhost:27017/sample-table"; +const REQUIRED_ENV_VARS = ['MONGO_DB_URL']; describe("Data Importation Without Docker", () => { - const mockEnv = { ...process.env }; + const mockEnv = REQUIRED_ENV_VARS.reduce((acc, key) => ({ + ...acc, + [key]: process.env[key] + }), {}); beforeEach(() => { - process.env.MONGO_DB_URL = "mongodb://localhost:27017/sample-table"; + process.env.MONGO_DB_URL = TEST_MONGODB_URL; vi.resetAllMocks(); });Also applies to: 241-251
17-238: Reduce test setup duplication and improve error handling tests.Consider the following improvements:
- Extract common mock setup to a helper function
- Use a specific error type for database connection failures
// Add these helper functions at the beginning of the test suite function createMocks() { return { checkDbMock: vi.fn().mockResolvedValue(true), wipeExistingDataMock: vi.fn().mockResolvedValue(undefined), importDataMock: vi.fn().mockResolvedValue(undefined), importDefaultDataMock: vi.fn().mockResolvedValue(undefined), }; } class DatabaseConnectionError extends Error { constructor(message: string) { super(message); this.name = 'DatabaseConnectionError'; } }Then use them in your tests:
it("should handle database connection failure gracefully", async () => { - const checkDbMock = vi.fn()... - const wipeExistingDataMock = vi.fn()... - const importDataMock = vi.fn()... - const importDefaultDataMock = vi.fn()... + const { + checkDbMock, + wipeExistingDataMock, + importDataMock, + importDefaultDataMock + } = createMocks(); const errorMessage = "Database connection failed"; - checkDbMock.mockRejectedValueOnce(new Error(errorMessage)); + checkDbMock.mockRejectedValueOnce(new DatabaseConnectionError(errorMessage));
438-463: Improve Docker timeout test reliability and assertions.The timeout test could be enhanced by:
- Using a constant for the timeout value
- Adding an explicit assertion for the error message
+const DOCKER_STARTUP_TIMEOUT = 3000; it("should handle Docker container startup timeout", async () => { const runDockerComposeMock = vi.fn().mockImplementation(async () => { return new Promise((_, reject) => { setTimeout(() => { reject(new Error("Docker compose operation timed out")); - }, 3000); + }, DOCKER_STARTUP_TIMEOUT); }); }); const importDataMock = vi.fn(); const importDefaultDataMock = vi.fn(); const connectDatabaseMock = vi.fn(); vi.spyOn(inquirer, "prompt").mockResolvedValueOnce({ shouldStartDockerContainers: true, }); - await dataImportWithDocker( + await expect(dataImportWithDocker( runDockerComposeMock, importDefaultDataMock, importDataMock, connectDatabaseMock, - ); + )).rejects.toThrow("Docker compose operation timed out"); expect(runDockerComposeMock).toBeCalled(); expect(connectDatabaseMock).not.toBeCalled(); });
1-464: Add test for MongoDB URL configuration.Consider adding a test case to verify that the correct MongoDB URL is being used. This ensures that the connection string is properly configured and passed to the database connection functions.
Example test to add:
it("should use the correct MongoDB URL from environment", async () => { const TEST_URL = "mongodb://test:27017/db"; process.env.MONGO_DB_URL = TEST_URL; const { checkDbMock } = createMocks(); await dataImportWithoutDocker( checkDbMock, vi.fn(), vi.fn(), vi.fn(), ); expect(checkDbMock).toHaveBeenCalledWith(TEST_URL); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/setup/dataImportFlow.spec.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/setup/dataImportFlow.spec.ts (1)
Learnt from: Aad1tya27
PR: PalisadoesFoundation/talawa-api#2845
File: tests/setup/dataImportFlow.spec.ts:113-114
Timestamp: 2025-01-14T00:46:56.756Z
Learning: In talawa-api's setup.ts, two different prompt keys are used for sample data import:
1. `importSampleData` - Used when asking to import sample data after overwriting existing data
2. `shouldImportSampleData` - Used when asking to import sample data into an empty database
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check for linting, formatting, and type errors
- GitHub Check: Analyse Code With CodeQL (typescript)
|
Your screen shots don't show the user being offered the opportunity to install sample data after saying No to the default data. |
There is no option to say No to the default data(which is just the organizations). There is only option to reject Sample Data, and on doing so default data will be automatically imported. |
|
If Docker is not Used: Case 2: No Collections Found, Accepted Sample Data => Sample Data is loaded Case 3: Collections Found, Data not overwritten => No change Case 4: Collections Found, Data overwritten => option between loading sample data or default data |
|
@palisadoes If you want to see screenshots of Docker installation I can share them too, and if any other change is required let me know. |
|
The logic must be:
Other comments: The questions are too wordy. Instead of the prompt : Instead of the prompt : |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
tests/setup/dataImportFlow.spec.ts (4)
203-203: Fix typo in test descriptionThere's an extra space in "should do no-op".
- it("should do no-op if db not empty and user imports neither default nor sample data", async () => { + it("should do no-op if db not empty and user imports neither default nor sample data", async () => {
278-314: Enhance database connection failure testThe test verifies basic error handling but could be improved to test different types of database failures.
Consider adding test cases for:
- Network timeout
- Authentication failure
- Invalid database URL
Example:
it("should handle database authentication failure", async () => { const errorMessage = "Authentication failed"; checkDbMock.mockRejectedValueOnce(new Error(errorMessage)); await expect( dataImportWithoutDocker( checkDbMock, wipeExistingDataMock, importDefaultDataMock, importDataMock, ), ).rejects.toThrow(errorMessage); });
366-401: Improve error handling test for Docker container startupThe test doesn't verify that the error message is propagated correctly.
Add assertion to verify the error message:
it("should terminate execution if error is encountered while starting containers", async () => { + const errorMessage = "Error starting containers"; const runDockerComposeMock = vi .fn() .mockImplementation(async (): Promise<void> => { - throw new Error("Error starting containers"); + throw new Error(errorMessage); }); // ... other mocks ... - await dataImportWithDocker( + await expect(dataImportWithDocker( runDockerComposeMock, importDefaultDataMock, importDataMock, connectDatabaseMock, - ); + )).rejects.toThrow(errorMessage); expect(runDockerComposeMock).toBeCalled(); expect(connectDatabaseMock).not.toBeCalled(); expect(importDefaultDataMock).not.toBeCalled(); expect(importDataMock).not.toBeCalled(); });
553-578: Improve Docker timeout testThe test could be enhanced to:
- Use a constant for the timeout value
- Verify the exact error message
- Add assertion for the rejection
+const MOCK_TIMEOUT = 3000; + it("should handle Docker container startup timeout", async () => { + const errorMessage = "Docker compose operation timed out"; const runDockerComposeMock = vi.fn().mockImplementation(async () => { return new Promise((_, reject) => { setTimeout(() => { - reject(new Error("Docker compose operation timed out")); + reject(new Error(errorMessage)); - }, 3000); + }, MOCK_TIMEOUT); }); }); // ... other mocks ... - await dataImportWithDocker( + await expect(dataImportWithDocker( runDockerComposeMock, importDefaultDataMock, importDataMock, connectDatabaseMock, - ); + )).rejects.toThrow(errorMessage); expect(runDockerComposeMock).toBeCalled(); expect(connectDatabaseMock).not.toBeCalled(); });setup.ts (3)
918-992: Enhance error handling in dataImportWithoutDockerWhile the error handling is improved, the function could benefit from more specific error types and consistent error handling.
Consider creating custom error types and handling operation-specific errors:
class DatabaseConnectionError extends Error { constructor(message: string) { super(message); this.name = 'DatabaseConnectionError'; } } class DataImportError extends Error { constructor(message: string) { super(message); this.name = 'DataImportError'; } } export async function dataImportWithoutDocker(...): Promise<void> { if (!process.env.MONGO_DB_URL) { throw new DatabaseConnectionError("MongoDB URL is not configured"); } try { isDbEmpty = await checkDb(process.env.MONGO_DB_URL); } catch (error) { throw new DatabaseConnectionError( `Failed to check database: ${error instanceof Error ? error.message : String(error)}` ); } try { // ... rest of the function ... } catch (error) { throw new DataImportError( `Failed to import data: ${error instanceof Error ? error.message : String(error)}` ); } }
994-1022: Make database connection retry parameters configurableThe retry parameters are hardcoded. Consider making them configurable through environment variables.
+const DEFAULT_MAX_RETRIES = 30; +const DEFAULT_RETRY_INTERVAL_MS = 1000; async function connectToDatabase(): Promise<void> { console.log("Waiting for mongoDB to be ready..."); let isConnected = false; - const maxRetries = 30; // 30 seconds timeout + const maxRetries = Number(process.env.MONGODB_MAX_RETRIES) || DEFAULT_MAX_RETRIES; + const retryInterval = Number(process.env.MONGODB_RETRY_INTERVAL_MS) || DEFAULT_RETRY_INTERVAL_MS; let retryCount = 0; while (!isConnected) { if (retryCount >= maxRetries) { throw new Error( - "Timed out waiting for MongoDB to be ready after 30 seconds" + `Timed out waiting for MongoDB to be ready after ${maxRetries * retryInterval / 1000} seconds` ); } // ... rest of the function ... - await new Promise((resolve) => setTimeout(resolve, 1000)); + await new Promise((resolve) => setTimeout(resolve, retryInterval)); } } return Promise.resolve(); }
1024-1069: Improve error handling and logging in dataImportWithDockerThe function could benefit from more detailed error handling and logging.
export async function dataImportWithDocker( runDockerComposeWithLogs: () => Promise<void>, importDefaultData: () => Promise<void>, importData: () => Promise<void>, connectToDatabase: () => Promise<void>, ): Promise<void> { // ... rest of the function ... if (shouldStartDockerContainers) { console.log("Starting docker container..."); try { await runDockerComposeWithLogs(); console.log("Docker containers have been built successfully!"); await connectToDatabase(); // ... rest of the function ... } catch (err) { - console.log("Some error occurred: " + err); + const error = err instanceof Error ? err : new Error(String(err)); + console.error("Failed to setup Docker environment:", { + error: error.message, + stack: error.stack, + phase: "docker_setup", + }); + throw error; } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts(14 hunks)tests/setup/dataImportFlow.spec.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/setup/dataImportFlow.spec.ts (1)
Learnt from: Aad1tya27
PR: PalisadoesFoundation/talawa-api#2845
File: tests/setup/dataImportFlow.spec.ts:113-114
Timestamp: 2025-01-14T00:46:56.756Z
Learning: In talawa-api's setup.ts, two different prompt keys are used for sample data import:
1. `importSampleData` - Used when asking to import sample data after overwriting existing data
2. `shouldImportSampleData` - Used when asking to import sample data into an empty database
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
setup.ts (1)
Line range hint
143-171: LGTM! Path handling updated for ES modulesThe changes correctly update the path handling to use
fileURLToPathanddirnamefor ES modules compatibility, replacing the CommonJS__dirname.
|
@palisadoes I've corrected the flow of prompts and changed the tests accordingly. Kindly re-review it. For some reason I'm failing the Validate Code Rabbit check, even though it approved my changes. |
palisadoes
merged commit 9937d25
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Bug in setup.ts
Issue Number:
Fixes #2829
Snapshots/Videos:
If relevant, did you update the documentation?
No
Does this PR introduce a breaking change?
Yeah, there was a breaking change related to __dirname, which was caused due CommonJS to ES.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
Have you read the contributing guide?
Summary by CodeRabbit