fix: Refactor Update Metadata #2681
Conversation
WalkthroughThe changes involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Database
Client->>Server: Send updateMetadata request
Server->>Server: Parse request body
Server->>Database: Get connections by IDs
Database-->>Server: Return connections
alt Unknown connections
Server->>Server: Construct error message
end
Server-->>Client: Return response (success/error)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
komyg
force-pushed
the
fix/refactor-update-metadata
branch
from
37453e9
to
ad36f8c
Compare
![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
Outside diff range, codebase verification and nitpick comments (1)
packages/server/lib/controllers/connection/updateMetadata.ts (1)
56-70: LGTM with a minor suggestion!The
updateByConnectionIdfunction looks good. It improves the user experience by providing clearer feedback for unknown connection IDs.Consider extracting the error response for unknown connection IDs into a separate function to further improve code clarity and reusability. For example:
function sendUnknownConnectionError(res: Response, unknownIds: string[], providerConfigKey: string) { res.status(404).send({ error: { code: 'unknown_connection', message: `Connection with connection ids: ${unknownIds.join(', ')} and provider config key ${providerConfigKey} not found. Please make sure the connection exists in the Nango dashboard. No actions were taken on any of the connections as a result of this failure.` } }); }Then, you can use it in
updateByConnectionIdlike this:if (storedConnections.length !== connectionIds.length) { const unknownIds = connectionIds.filter((connectionId) => !storedConnections.find((conn) => conn.connection_id === connectionId)); sendUnknownConnectionError(res, unknownIds, providerConfigKey); return; }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/server/lib/controllers/connection/updateMetadata.integration.test.ts (2 hunks)
- packages/server/lib/controllers/connection/updateMetadata.ts (2 hunks)
- packages/shared/lib/services/connection.service.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/server/lib/controllers/connection/updateMetadata.integration.test.ts
Additional comments not posted (5)
packages/server/lib/controllers/connection/updateMetadata.ts (3)
Line range hint
16-36: LGTM!The refactoring of
updateMetadatalooks good. Breaking down the function into smaller, reusable parts improves code clarity and maintainability. The error handling has also been improved to consolidate responses for unknown connection IDs.
38-46: LGTM!The
parseBodyfunction looks good. It improves code clarity by centralizing the extraction of parameters from the request body.
48-54: LGTM!The
bodyParamToArrayfunction looks good. It ensures consistent handling of both single and multiple connection IDs.packages/shared/lib/services/connection.service.ts (2)
488-500: LGTM!The new
getConnectionsByConnectionIdsmethod looks good:
- It correctly queries the
_nango_connectionstable based on the provided parameters.- It handles the case when no results are found by returning an empty array.
- It follows best practices and has clear naming.
The code changes are approved.
Line range hint
709-715: LGTM!The changes to the
updateMetadatamethod look good:
- The updated method signature enhances flexibility by accepting both
Connection[]andStoredConnection[].- The method implementation remains correct and maintains its original functionality.
- It follows best practices by using a database transaction and merging metadata correctly.
The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
packages/server/lib/controllers/connection/connectionId/metadata/patchMetadata.ts (1)
57-71: LGTM with a minor suggestion!The code changes are approved. The function is well-structured and handles the case when any of the connection IDs are not found.
Consider extracting the error response logic into a separate function to improve code readability and reusability. For example:
function sendUnknownConnectionError(res: Response, unknownIds: string[], providerConfigKey: string) { res.status(404).send({ error: { code: 'unknown_connection', message: `Connection with connection ids: ${unknownIds.join(', ')} and provider config key ${providerConfigKey} not found. Please make sure the connection exists in the Nango dashboard. No actions were taken on any of the connections as a result of this failure.` } }); }Then, update the
updateByConnectionIdfunction to use the new function:async function updateByConnectionId(res: Response, connectionIds: string[], providerConfigKey: string, environmentId: number, metadata: Metadata) { const storedConnections = await connectionService.getConnectionsByConnectionIds(connectionIds, providerConfigKey, environmentId); if (storedConnections.length !== connectionIds.length) { const unknownIds = connectionIds.filter((connectionId) => !storedConnections.find((conn) => conn.connection_id === connectionId)); - res.status(404).send({ - error: { - code: 'unknown_connection', - message: `Connection with connection ids: ${unknownIds.join(', ')} and provider config key ${providerConfigKey} not found. Please make sure the connection exists in the Nango dashboard. No actions were taken on any of the connections as a result of this failure.` - } - }); + sendUnknownConnectionError(res, unknownIds, providerConfigKey); return; } await connectionService.updateMetadata(storedConnections, metadata); }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- packages/server/lib/controllers/connection/connectionId/metadata/patchMetadata.integration.test.ts (2 hunks)
- packages/server/lib/controllers/connection/connectionId/metadata/patchMetadata.ts (2 hunks)
- packages/shared/lib/services/connection.service.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/server/lib/controllers/connection/connectionId/metadata/patchMetadata.integration.test.ts
Files skipped from review as they are similar to previous changes (1)
- packages/shared/lib/services/connection.service.ts
Additional comments not posted (3)
packages/server/lib/controllers/connection/connectionId/metadata/patchMetadata.ts (3)
Line range hint
17-37: LGTM!The code changes are approved. The refactoring enhances code clarity and modularity by using helper functions for parsing the request body and updating metadata.
39-47: LGTM!The code changes are approved. The function improves code clarity by structuring the request body and ensures that
connectionIdsis always an array using thebodyParamToArrayhelper function.
49-55: LGTM!The code changes are approved. The function correctly converts a string or an array of strings to an array of strings and handles the case when the input is undefined.
Describe your changes
Refactor the PatchMetadata file to:
Issue ticket number and link
Checklist before requesting a review (skip if just adding/editing APIs & templates)
Summary by CodeRabbit
New Features
updateMetadatafunction to better manage connection IDs and error responses.Bug Fixes
Refactor
updateMetadatafunction for better clarity and maintainability.