Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(api): prebuilt endpoints #2642

Merged
merged 8 commits into from
Aug 29, 2024
Merged

feat(api): prebuilt endpoints #2642

merged 8 commits into from
Aug 29, 2024

Conversation

bodinsamuel
Copy link
Collaborator

@bodinsamuel bodinsamuel commented Aug 27, 2024

Describe your changes

Contributes to https://linear.app/nango/issue/NAN-1537/script-v2-integrate-ui

  • Feat: New dedicated file for Template deploy
  • Feat: simplify payload body for deploy and upgrade to only specify what we need and get the actual data from the backend to make sure it's the correct data
  • Fix: reverse API path for template from /deploy|upgrade/pre-built -> /pre-build/deploy|upgrade just for the sake of REST logic
  • Fix: getNangoConfigIdAndLocationFromId was not multi-tenant safe (any id could be passed)
  • Fix: missing track_deletes on template deploy
  • Fix: removed unused FlowCreate.tsx

🧪 Test

  • Create Exact Online integration
  • Deploy any template (action / sync)

Summary by CodeRabbit

  • New Features

    • Introduced tracking for deletions in synchronization configurations.
    • Added new endpoints for deploying and upgrading pre-built flows.
  • Bug Fixes

    • Enhanced error handling across various API endpoints for better clarity and specificity.
  • Refactor

    • Streamlined data structures and API interactions for flow management.
    • Removed deprecated interfaces and properties to simplify the codebase.
  • Documentation

    • Updated API endpoint paths and request body structures for clarity.
  • Tests

    • Modified test cases to reflect the addition of the track_deletes property.

@bodinsamuel bodinsamuel self-assigned this Aug 27, 2024
@@ -414,18 +377,14 @@ export async function deployPreBuilt({
let nango_config_id: number;
let provider_config_key: string;

if (nangoYamlBody) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It felt unused/impossible let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, don't see any case for this.

Copy link
Member

@khaliqgant khaliqgant Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at this again and I think this PR breaks the admin deploy command https://github.com/NangoHQ/nango/blob/master/packages/cli/lib/services/deploy.service.ts#L79-L99

It is not really used anymore (internal only) so maybe we should remove that functionality completely?

Copy link

linear bot commented Aug 27, 2024

@bodinsamuel bodinsamuel marked this pull request as ready for review August 27, 2024 23:21
};

const reEnableFlow = async (flow: ExtendedFlow): Promise<boolean> => {
const reEnableFlow = async (flow: any): Promise<boolean> => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this any is temporary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed, I should have added a comment.

Copy link
Member

@khaliqgant khaliqgant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non blocking inline question about usage of any

Copy link

coderabbitai bot commented Aug 29, 2024

Walkthrough

The changes involve significant modifications across various components, primarily focusing on the deployment and management of pre-built flows. Key updates include the introduction of new properties for tracking deletions, the removal of certain methods and interfaces, and the restructuring of API endpoints. Additionally, several files have been refactored to streamline functionality and improve type definitions, enhancing the overall organization of the codebase.

Changes

Files Change Summary
packages/cli/lib/services/deploy.service.ts Added track_deletes property set to false in the postData array object.
packages/server/lib/controllers/flow.controller.ts Removed deployPreBuiltFlow method; updated imports and modified downloadFlow to use getSyncConfigById.
packages/server/lib/controllers/onboarding.controller.ts Added conditional track_deletes property for GitHub integration configurations; removed nangoYamlBody parameter from deployPreBuiltSyncConfig call.
packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts Introduced a new endpoint for handling post-deployment actions with input validation using Zod.
packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts Introduced a new endpoint for handling upgrade requests with input validation using Zod.
packages/server/lib/routes.ts Updated import paths and modified routing endpoints for deploying and upgrading pre-built flows.
packages/server/lib/utils/tests.ts Modified the isError function's type assertion to simplify error handling.
packages/shared/lib/services/flow.service.ts Added getFlowByIntegrationAndName method for retrieving flows based on integration details.
packages/shared/lib/services/sync/config/config.service.ts Renamed getNangoConfigIdAndLocationFromId to getSyncConfigById with updated parameters and SQL query.
packages/shared/lib/services/sync/config/deploy.service.ts Streamlined parameters and internal logic for upgradePreBuilt and deployPreBuilt functions.
packages/shared/lib/services/sync/config/deploy.service.unit.test.ts Added track_deletes property set to true in test cases for sync configuration.
packages/types/lib/api.ts Modified ApiError interface to broaden type flexibility for error handling.
packages/types/lib/deploy/incomingFlow.ts Updated interfaces to require track_deletes and providerConfigKey, removed optional properties, and eliminated IncomingFlowConfigUpgrade.
packages/types/lib/flow/http.api.ts Renamed UpgradePreBuiltFlow to PutUpgradePreBuiltFlow and introduced a new PostPreBuiltDeploy type for the updated API structure.
packages/webapp/src/pages/Integration/FlowPage.tsx Updated upgradeFlow object to use PutUpgradePreBuiltFlow['Body'], reflecting changes in the data model and API endpoint.
packages/webapp/src/pages/Integration/Scripts.tsx Removed rawName prop from component instances, affecting data interaction with endpoints.
packages/webapp/src/pages/Integration/components/EnableDisableSync.tsx Removed ExtendedPreBuiltFlow interface and simplified data handling in various functions.
packages/webapp/src/types.ts Removed PreBuiltFlow interface, indicating a significant change in the data model for pre-built flows.
packages/webapp/src/utils/api.tsx Updated useCreateFlow function to accept a single object of type PostPreBuiltDeploy['Body'], reflecting changes in API interaction.

Poem

🐇 In fields of code, where changes bloom,
New paths emerge, dispelling gloom.
With track_deletes, we hop along,
Simplified flows, where we belong.
So let us cheer, with joyful grace,
For every change, a brighter space! 🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Outside diff range, codebase verification and nitpick comments (1)
packages/types/lib/deploy/incomingFlow.ts (1)

49-49: Ensure public_route is included in IncomingPreBuiltFlowConfig instances.

The public_route field is expected in the IncomingPreBuiltFlowConfig interface. It appears to be missing in the onboarding.controller.ts file. Please verify and update the instances to include this field.

  • File: packages/server/lib/controllers/onboarding.controller.ts
    • Line: const config: IncomingPreBuiltFlowConfig[] = [...] (Ensure public_route is included)
Analysis chain

Verify the usage of IncomingPreBuiltFlowConfig.

Ensure that all instances of IncomingPreBuiltFlowConfig are updated to include the public_route field.

Run the following script to verify the usage:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all instances of `IncomingPreBuiltFlowConfig` include the `public_route` field.

# Test: Search for the interface usage. Expect: Only occurrences with the `public_route` field.
rg --type typescript -A 5 $'IncomingPreBuiltFlowConfig'

Length of output: 89


Script:

#!/bin/bash
# Description: Verify all instances of `IncomingPreBuiltFlowConfig` include the `public_route` field.

# Test: Search for the interface usage. Expect: Only occurrences with the `public_route` field.
rg --type ts -A 5 $'IncomingPreBuiltFlowConfig'

Length of output: 4834

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 356da13 and a5f7960.

Files ignored due to path filters (1)
  • packages/cli/lib/services/__snapshots__/deploy.service.unit.cli-test.ts.snap is excluded by !**/*.snap
Files selected for processing (19)
  • packages/cli/lib/services/deploy.service.ts (1 hunks)
  • packages/server/lib/controllers/flow.controller.ts (4 hunks)
  • packages/server/lib/controllers/onboarding.controller.ts (2 hunks)
  • packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts (1 hunks)
  • packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts (1 hunks)
  • packages/server/lib/routes.ts (3 hunks)
  • packages/server/lib/utils/tests.ts (1 hunks)
  • packages/shared/lib/services/flow.service.ts (2 hunks)
  • packages/shared/lib/services/sync/config/config.service.ts (1 hunks)
  • packages/shared/lib/services/sync/config/deploy.service.ts (10 hunks)
  • packages/shared/lib/services/sync/config/deploy.service.unit.test.ts (2 hunks)
  • packages/types/lib/api.ts (2 hunks)
  • packages/types/lib/deploy/incomingFlow.ts (1 hunks)
  • packages/types/lib/flow/http.api.ts (1 hunks)
  • packages/webapp/src/pages/Integration/FlowPage.tsx (3 hunks)
  • packages/webapp/src/pages/Integration/Scripts.tsx (2 hunks)
  • packages/webapp/src/pages/Integration/components/EnableDisableSync.tsx (4 hunks)
  • packages/webapp/src/types.ts (1 hunks)
  • packages/webapp/src/utils/api.tsx (2 hunks)
Additional context used
Biome
packages/shared/lib/services/sync/config/deploy.service.ts

[error] 274-274: Avoid the delete operator which can impact performance.

Unsafe fix: Use an undefined assignment instead.

(lint/performance/noDelete)

Additional comments not posted (34)
packages/types/lib/deploy/incomingFlow.ts (1)

49-49: Verify the usage of IncomingFlowConfig.

Ensure that all instances of IncomingFlowConfig are updated to include the providerConfigKey field.

Run the following script to verify the usage:

Verification successful

Verification Successful: providerConfigKey is included in IncomingFlowConfig instances.

The providerConfigKey field is present in the instances of IncomingFlowConfig across the codebase, confirming that the updates have been applied correctly.

  • Instances of IncomingFlowConfig include the providerConfigKey field as expected.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all instances of `IncomingFlowConfig` include the `providerConfigKey` field.

# Test: Search for the interface usage. Expect: Only occurrences with the `providerConfigKey` field.
rg --type typescript -A 5 $'IncomingFlowConfig'

Length of output: 81


Script:

#!/bin/bash
# Description: Find all TypeScript files and search for `IncomingFlowConfig` usage to verify the inclusion of `providerConfigKey`.

# Find all TypeScript files and search for `IncomingFlowConfig` usage
fd --extension ts --exec rg 'IncomingFlowConfig' -A 5

Length of output: 5643

packages/types/lib/api.ts (2)

1-1: Verify the usage of ApiError.

Ensure that the TErrors type is used consistently across the codebase.

Run the following script to verify the usage:

Verification successful

Consistent usage of ApiError with TErrors type verified.

The TErrors type in the ApiError interface is used consistently across the codebase, leveraging the flexibility provided by changing it to any. No inconsistencies were found.

  • Instances of ApiError are used with various error codes and types, such as ValidationError[], indicating effective utilization of the flexibility.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistent usage of `ApiError` with the `TErrors` type.

# Test: Search for the interface usage. Expect: Consistent usage of `TErrors` type.
rg --type typescript -A 5 $'ApiError'

Length of output: 71


Script:

#!/bin/bash
# Description: Verify the consistent usage of `ApiError` with the `TErrors` type.

# Test: Search for the interface usage. Expect: Consistent usage of `TErrors` type.
rg -A 5 $'ApiError'

Length of output: 35227


75-75: Verify the usage of Endpoint.

Ensure that the new type condition for the Errors property is used consistently across the codebase.

Run the following script to verify the usage:

Verification successful

The new type condition for the Errors property is consistently applied.

The usage of the Errors property across the codebase aligns with the new type condition, confirming its consistent application. No further action is required.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the consistent usage of `Endpoint` with the new type condition for the `Errors` property.

# Test: Search for the interface usage. Expect: Consistent usage of the new type condition.
rg --type typescript -A 5 $'Endpoint'

Length of output: 71


Script:

#!/bin/bash
# Description: Verify the consistent usage of `Endpoint` with the new type condition for the `Errors` property.

# Test: Search for the interface usage. Expect: Consistent usage of the new type condition.
rg --type ts -A 5 $'Endpoint'

Length of output: 131217


Script:

#!/bin/bash
# Description: Verify the consistent usage of the `Errors` property within the `Endpoint` interface.

# Test: Search for the `Errors` property usage in the context of `Endpoint`.
rg --type ts -A 5 $'Errors'

Length of output: 34231

packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts (2)

1-7: LGTM!

The import statements are relevant and necessary for the file's functionality.


9-19: LGTM!

The validation schema is well-defined and ensures that the request body is correctly validated.

packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts (2)

1-8: LGTM!

The import statements are relevant and necessary for the file's functionality.


10-17: LGTM!

The validation schema is well-defined and ensures that the request body is correctly validated.

packages/server/lib/utils/tests.ts (1)

68-70: LGTM!

The modification simplifies the type assertion and aligns with the expected structure of the error response.

packages/webapp/src/types.ts (1)

Line range hint 1-1: LGTM! But verify the impact of the removal on the codebase.

The removal of the PreBuiltFlow interface suggests that the functionality related to pre-built flows has been deprecated or refactored. Ensure that all components or services that relied on this interface are updated to accommodate this change.

The code changes are approved.

Run the following script to verify the impact of the removal on the codebase:

packages/shared/lib/services/flow.service.ts (1)

68-77: LGTM! But verify the correctness of the method.

The method is correctly implemented. However, ensure that the method retrieves the correct flow based on the provided integration provider and script name.

The code changes are approved.

Run the following script to verify the correctness of the method:

Verification successful

The getFlowByIntegrationAndName method is correctly implemented and used.

The method is used in putUpgrade.ts and postDeploy.ts to retrieve flows based on the provided parameters. The error handling for unknown flows is appropriate, confirming that the method functions as intended.

  • Files using the method:
    • packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts
    • packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of the `getFlowByIntegrationAndName` method.

# Test: Search for the usage of the `getFlowByIntegrationAndName` method. Expect: Correct flow retrieval.
rg --type javascript 'getFlowByIntegrationAndName'

Length of output: 85


Script:

#!/bin/bash
# Description: Verify the correctness of the `getFlowByIntegrationAndName` method.

# Test: Search for the usage of the `getFlowByIntegrationAndName` method. Expect: Correct flow retrieval.
rg --type typescript 'getFlowByIntegrationAndName'

Length of output: 85


Script:

#!/bin/bash
# Description: Verify the correctness of the `getFlowByIntegrationAndName` method.

# Test: Search for the usage of the `getFlowByIntegrationAndName` method. Expect: Correct flow retrieval.
rg --type ts 'getFlowByIntegrationAndName'

Length of output: 619


Script:

#!/bin/bash
# Extract relevant lines from putUpgrade.ts to analyze the usage of getFlowByIntegrationAndName
rg --type ts -A 5 'getFlowByIntegrationAndName' packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts

# Extract relevant lines from postDeploy.ts to analyze the usage of getFlowByIntegrationAndName
rg --type ts -A 5 'getFlowByIntegrationAndName' packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts

Length of output: 701

packages/server/lib/controllers/flow.controller.ts (3)

15-16: LGTM!

The updated import statements reflect a shift in the method used to retrieve configuration data, specifically altering the way configurations are fetched by utilizing the new method that likely aligns with the updated data structure or API.

The code changes are approved.


98-98: LGTM!

The modification of the downloadFlow method to utilize getSyncConfigById suggests a reorganization of how flow configurations are accessed, emphasizing the new method's role in fetching configurations based on the environment ID and a specific ID.

The code changes are approved.


Line range hint 1-1: LGTM! But verify the impact of the removal on the codebase.

The removal of the deployPreBuiltFlow method suggests a potential shift in how pre-built flows are managed. Ensure that the responsibility for deploying pre-built flows is correctly delegated to another part of the application or refactored.

The code changes are approved.

Run the following script to verify the impact of the removal on the codebase:

Verification successful

Removal of deployPreBuiltFlow Verified: The deployPreBuiltFlow method has been successfully removed with no remaining references in the codebase. This confirms that the removal does not negatively impact other parts of the application.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the removal of the `deployPreBuiltFlow` method on the codebase.

# Test: Search for the usage of the `deployPreBuiltFlow` method. Expect: No occurrences.
rg --type javascript 'deployPreBuiltFlow'

Length of output: 76


Script:

#!/bin/bash
# Description: Verify the impact of the removal of the `deployPreBuiltFlow` method on the codebase.

# Test: Search for the usage of the `deployPreBuiltFlow` method. Expect: No occurrences.
rg 'deployPreBuiltFlow'

Length of output: 23

packages/shared/lib/services/sync/config/deploy.service.unit.test.ts (2)

71-72: LGTM!

The addition of the track_deletes property enhances the functionality by enabling tracking of deleted models.


110-111: LGTM!

The addition of the track_deletes property enhances the functionality by enabling tracking of deleted models.

packages/webapp/src/pages/Integration/components/EnableDisableSync.tsx (6)

11-11: LGTM!

The import of the PostPreBuiltDeploy type is necessary for the changes in the function signatures.


69-76: LGTM!

The change in the function signature reflects a shift in the expected input structure for creating new flows.


Line range hint 79-90: LGTM!

The change in the function signature indicates a reduction in type specificity.


Line range hint 93-114: LGTM!

The removal of the _model_schema parameter simplifies the function implementation.


147-167: LGTM!

The function onEnableSync correctly uses the updated createNewFlow and reEnableFlow functions.


Line range hint 24-165: LGTM!

The EnableDisableSync component correctly uses the updated FlowProps interface.

packages/server/lib/controllers/onboarding.controller.ts (3)

191-192: Addition of track_deletes property for sync configuration.

The track_deletes property is conditionally set based on githubDemoSync.track_deletes. This enhances the configuration by allowing the tracking of deletions.

The code changes are approved.


205-206: Addition of track_deletes property for action configuration.

The track_deletes property is conditionally set based on githubDemoAction.track_deletes. This enhances the configuration by allowing the tracking of deletions.

The code changes are approved.


210-210: Removal of nangoYamlBody parameter.

The call to deployPreBuiltSyncConfig has been updated to remove the nangoYamlBody parameter, indicating a potential simplification or change in the deployment process.

The code changes are approved.

packages/cli/lib/services/deploy.service.ts (1)

407-408: Addition of track_deletes property to action configuration.

The track_deletes property is added to the action configuration and is set to false. This indicates an enhancement in the service's capability to manage or log deletion actions.

The code changes are approved.

packages/server/lib/routes.ts (2)

51-51: Update import path for putUpgradePreBuilt.

The import path for putUpgradePreBuilt has been updated, suggesting a reorganization of the file structure for better clarity or modularity.

The code changes are approved.


273-274: Update routing endpoints for deploying and upgrading pre-built flows.

The routing endpoints have been updated to reflect the new structure, potentially improving the logical grouping of related functionalities under the pre-built category.

The code changes are approved.

packages/webapp/src/utils/api.tsx (1)

820-824: LGTM! But verify the function usage in the codebase.

The function is correctly implemented with the new parameter type and endpoint URL. However, ensure that all function calls to useCreateFlow match the new signature.

The code changes are approved.

Run the following script to verify the function usage:

packages/shared/lib/services/sync/config/config.service.ts (1)

647-658: LGTM! But verify the function usage in the codebase.

The function is correctly implemented with the new parameters and SQL query. However, ensure that all function calls to getSyncConfigById match the new signature.

The code changes are approved.

Run the following script to verify the function usage:

Verification successful

Function usage is consistent with the new implementation.

The function getSyncConfigById is correctly used with the new parameters across the codebase. No discrepancies were found in the function calls.

  • flow.controller.ts: Correct usage with environmentId and id.
  • putUpgrade.ts: Correct usage with environmentId and id.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `getSyncConfigById` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts $'getSyncConfigById'

Length of output: 737

packages/shared/lib/services/sync/config/deploy.service.ts (2)

Line range hint 220-336: LGTM! But verify the function usage in the codebase.

The function is correctly implemented with the new parameters and optimized internal logic. However, ensure that all function calls to upgradePreBuilt match the new signature.

The code changes are approved.

Run the following script to verify the function usage:

Verification successful

Function Usage Verified:

The upgradePrebuiltFlow function is correctly called with the new parameters in putUpgrade.ts, matching the updated signature. No issues were found with the function usage in the codebase.

  • File: packages/server/lib/controllers/v1/flow/preBuilt/putUpgrade.ts
  • Function Call: Correct parameters are used.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `upgradePreBuilt` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts $'upgradePreBuilt'

Length of output: 313


Script:

#!/bin/bash
# Description: Check how `upgradePrebuiltFlow` is used in `putUpgrade.ts`.

# Test: Search for the usage of `upgradePrebuiltFlow` in the file.
rg --type ts 'upgradePrebuiltFlow' -A 5

Length of output: 1185


Line range hint 364-536: LGTM! But verify the function usage in the codebase.

The function is correctly implemented with the new parameters and consolidated logic for handling public templates. However, ensure that all function calls to deployPreBuilt match the new signature.

The code changes are approved.

Run the following script to verify the function usage:

Verification successful

Function Usage Verified: deployPreBuilt

The function deployPreBuilt is used correctly with its new signature across the codebase. All instances match the expected parameters.

  • Files Verified:
    • packages/server/lib/controllers/onboarding.controller.ts
    • packages/server/lib/controllers/flow.controller.ts
    • packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all function calls to `deployPreBuilt` match the new signature.

# Test: Search for the function usage. Expect: Only occurrences of the new signature.
rg --type ts $'deployPreBuilt'

Length of output: 922


Script:

#!/bin/bash
# Extract lines where `deployPreBuilt` is called in the identified files to verify the new signature.

# Check usage in onboarding.controller.ts
rg --type ts --context 5 'deployPreBuiltSyncConfig' packages/server/lib/controllers/onboarding.controller.ts

# Check usage in flow.controller.ts
rg --type ts --context 5 'deployPreBuiltSyncConfig' packages/server/lib/controllers/flow.controller.ts

# Check usage in postDeploy.ts
rg --type ts --context 5 'deployPreBuilt' packages/server/lib/controllers/v1/flow/preBuilt/postDeploy.ts

Length of output: 2641

packages/webapp/src/pages/Integration/FlowPage.tsx (3)

26-26: LGTM!

The import statement is correct and aligns with the changes in the file.


267-274: LGTM!

The changes to the upgradeFlow object are correct and align with the new data model.


277-277: LGTM!

The change in the API endpoint aligns with the new RESTful principles mentioned in the PR summary.

Comment on lines +4 to +16
export type PutUpgradePreBuiltFlow = Endpoint<{
Method: 'PUT';
Path: '/api/v1/flow/upgrade/pre-built';
Body: IncomingFlowConfigUpgrade;
Error: ApiError<'upgrade_failed'>;
Path: '/api/v1/flow/pre-built/upgrade';
Body: {
id: number;
provider: string;
scriptName: string;
type: ScriptTypeLiteral;
upgradeVersion: string;
lastDeployed: string;
providerConfigKey: string;
};
Error: ApiError<'upgrade_failed'> | ApiError<'unknown_provider'> | ApiError<'unknown_sync_config'> | ApiError<'unknown_flow'> | ApiError<'invalid_version'>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating the lastDeployed field.

Ensure that the lastDeployed field follows a proper date format to avoid potential issues with date handling.

Apply this diff to add a validation comment:

 Body: {
     id: number;
     provider: string;
     scriptName: string;
     type: ScriptTypeLiteral;
     upgradeVersion: string;
-    lastDeployed: string;
+    lastDeployed: string; // Ensure this follows a proper date format
     providerConfigKey: string;
 };
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.

Suggested change
export type PutUpgradePreBuiltFlow = Endpoint<{
Method: 'PUT';
Path: '/api/v1/flow/upgrade/pre-built';
Body: IncomingFlowConfigUpgrade;
Error: ApiError<'upgrade_failed'>;
Path: '/api/v1/flow/pre-built/upgrade';
Body: {
id: number;
provider: string;
scriptName: string;
type: ScriptTypeLiteral;
upgradeVersion: string;
lastDeployed: string;
providerConfigKey: string;
};
Error: ApiError<'upgrade_failed'> | ApiError<'unknown_provider'> | ApiError<'unknown_sync_config'> | ApiError<'unknown_flow'> | ApiError<'invalid_version'>;
export type PutUpgradePreBuiltFlow = Endpoint<{
Method: 'PUT';
Path: '/api/v1/flow/pre-built/upgrade';
Body: {
id: number;
provider: string;
scriptName: string;
type: ScriptTypeLiteral;
upgradeVersion: string;
lastDeployed: string; // Ensure this follows a proper date format
providerConfigKey: string;
};
Error: ApiError<'upgrade_failed'> | ApiError<'unknown_provider'> | ApiError<'unknown_sync_config'> | ApiError<'unknown_flow'> | ApiError<'invalid_version'>;

Comment on lines +22 to +37
export type PostPreBuiltDeploy = Endpoint<{
Method: 'POST';
Path: '/api/v1/flow/pre-built/deploy';
Body: {
provider: string;
providerConfigKey: string;
scriptName: string;
type: ScriptTypeLiteral;
};
Error: ApiError<'unknown_provider'> | ApiError<'resource_capped'> | ApiError<'failed_to_deploy', Error[]> | ApiError<'unknown_flow'>;
Success: {
data: {
id: number;
};
};
}>;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider validating the providerConfigKey field.

Ensure that the providerConfigKey field follows a proper format to avoid potential issues with configuration handling.

Apply this diff to add a validation comment:

 Body: {
     provider: string;
     providerConfigKey: string;
     scriptName: string;
     type: ScriptTypeLiteral;
 };
 Error: ApiError<'unknown_provider'> | ApiError<'resource_capped'> | ApiError<'failed_to_deploy', Error[]> | ApiError<'unknown_flow'>;
 Success: {
     data: {
         id: number;
     };
 };
+// Ensure `providerConfigKey` follows a proper format
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.

Suggested change
export type PostPreBuiltDeploy = Endpoint<{
Method: 'POST';
Path: '/api/v1/flow/pre-built/deploy';
Body: {
provider: string;
providerConfigKey: string;
scriptName: string;
type: ScriptTypeLiteral;
};
Error: ApiError<'unknown_provider'> | ApiError<'resource_capped'> | ApiError<'failed_to_deploy', Error[]> | ApiError<'unknown_flow'>;
Success: {
data: {
id: number;
};
};
}>;
export type PostPreBuiltDeploy = Endpoint<{
Method: 'POST';
Path: '/api/v1/flow/pre-built/deploy';
Body: {
provider: string;
providerConfigKey: string;
scriptName: string;
type: ScriptTypeLiteral;
};
Error: ApiError<'unknown_provider'> | ApiError<'resource_capped'> | ApiError<'failed_to_deploy', Error[]> | ApiError<'unknown_flow'>;
Success: {
data: {
id: number;
};
};
// Ensure `providerConfigKey` follows a proper format
}>;

Comment on lines +42 to +43
track_deletes: boolean;
providerConfigKey: string;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the track_deletes field.

Add documentation to explain the purpose of the track_deletes field.

Apply this diff to add documentation:

 track_deletes: boolean;
+// Indicates whether deletes are tracked
 providerConfigKey: string;
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.

Suggested change
track_deletes: boolean;
providerConfigKey: string;
track_deletes: boolean;
// Indicates whether deletes are tracked
providerConfigKey: string;

Comment on lines +21 to +79
export const putUpgradePreBuilt = asyncWrapper<PutUpgradePreBuiltFlow>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
return;
}

const val = validation.safeParse(req.body);
if (!val.success) {
res.status(400).send({
error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) }
});
return;
}

const body: PutUpgradePreBuiltFlow['Body'] = val.data;
const { environment, account } = res.locals;

const syncConfig = await getSyncConfigById(environment.id, body.id);
if (!syncConfig) {
res.status(400).send({
error: { code: 'unknown_sync_config' }
});
return;
}

const config = await configService.getProviderConfig(body.providerConfigKey, environment.id);
if (!config) {
res.status(400).send({ error: { code: 'unknown_provider' } });
return;
}

const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) {
res.status(400).send({ error: { code: 'unknown_flow' } });
return;
}

if (flow.version !== body.upgradeVersion) {
res.status(400).send({ error: { code: 'invalid_version' } });
return;
}

const result = await upgradePrebuiltFlow({
environment,
account,
config,
syncConfig,
flow,
logContextGetter
});

if (result.isOk()) {
res.send({ success: true });
return;
}

res.status(400).send({ error: { code: 'upgrade_failed', message: result.error.message } });
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring for improved readability and error handling.

The function is well-structured and handles various scenarios effectively. However, consider the following improvements:

  • Refactor the function to reduce complexity and improve readability.
  • Use a centralized error handling mechanism to handle errors consistently.

Example refactor:

export const putUpgradePreBuilt = asyncWrapper<PutUpgradePreBuiltFlow>(async (req, res) => {
    const emptyQuery = requireEmptyQuery(req, { withEnv: true });
    if (emptyQuery) {
        return res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
    }

    const val = validation.safeParse(req.body);
    if (!val.success) {
        return res.status(400).send({ error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) } });
    }

    const body: PutUpgradePreBuiltFlow['Body'] = val.data;
    const { environment, account } = res.locals;

    try {
        const syncConfig = await getSyncConfigById(environment.id, body.id);
        if (!syncConfig) throw new Error('unknown_sync_config');

        const config = await configService.getProviderConfig(body.providerConfigKey, environment.id);
        if (!config) throw new Error('unknown_provider');

        const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
        if (!flow) throw new Error('unknown_flow');

        if (flow.version !== body.upgradeVersion) throw new Error('invalid_version');

        const result = await upgradePrebuiltFlow({ environment, account, config, syncConfig, flow, logContextGetter });
        if (!result.isOk()) throw new Error(result.error.message);

        res.send({ success: true });
    } catch (error) {
        res.status(400).send({ error: { code: error.message } });
    }
});
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.

Suggested change
export const putUpgradePreBuilt = asyncWrapper<PutUpgradePreBuiltFlow>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
return;
}
const val = validation.safeParse(req.body);
if (!val.success) {
res.status(400).send({
error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) }
});
return;
}
const body: PutUpgradePreBuiltFlow['Body'] = val.data;
const { environment, account } = res.locals;
const syncConfig = await getSyncConfigById(environment.id, body.id);
if (!syncConfig) {
res.status(400).send({
error: { code: 'unknown_sync_config' }
});
return;
}
const config = await configService.getProviderConfig(body.providerConfigKey, environment.id);
if (!config) {
res.status(400).send({ error: { code: 'unknown_provider' } });
return;
}
const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) {
res.status(400).send({ error: { code: 'unknown_flow' } });
return;
}
if (flow.version !== body.upgradeVersion) {
res.status(400).send({ error: { code: 'invalid_version' } });
return;
}
const result = await upgradePrebuiltFlow({
environment,
account,
config,
syncConfig,
flow,
logContextGetter
});
if (result.isOk()) {
res.send({ success: true });
return;
}
res.status(400).send({ error: { code: 'upgrade_failed', message: result.error.message } });
});
export const putUpgradePreBuilt = asyncWrapper<PutUpgradePreBuiltFlow>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
return res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
}
const val = validation.safeParse(req.body);
if (!val.success) {
return res.status(400).send({ error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) } });
}
const body: PutUpgradePreBuiltFlow['Body'] = val.data;
const { environment, account } = res.locals;
try {
const syncConfig = await getSyncConfigById(environment.id, body.id);
if (!syncConfig) throw new Error('unknown_sync_config');
const config = await configService.getProviderConfig(body.providerConfigKey, environment.id);
if (!config) throw new Error('unknown_provider');
const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) throw new Error('unknown_flow');
if (flow.version !== body.upgradeVersion) throw new Error('invalid_version');
const result = await upgradePrebuiltFlow({ environment, account, config, syncConfig, flow, logContextGetter });
if (!result.isOk()) throw new Error(result.error.message);
res.send({ success: true });
} catch (error) {
res.status(400).send({ error: { code: error.message } });
}
});

Comment on lines +21 to +89
export const postPreBuiltDeploy = asyncWrapper<PostPreBuiltDeploy>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
return;
}

const val = validation.safeParse(req.body);
if (!val.success) {
res.status(400).send({
error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) }
});
return;
}

const body: PostPreBuiltDeploy['Body'] = val.data;

const { environment, account } = res.locals;
const environmentId = environment.id;

const config = await configService.getConfigIdByProviderConfigKey(body.providerConfigKey, environmentId);
if (!config) {
res.status(400).send({ error: { code: 'unknown_provider' } });
return;
}

if (account.is_capped) {
const isCapped = await connectionService.shouldCapUsage({ providerConfigKey: body.providerConfigKey, environmentId, type: 'activate' });
if (isCapped) {
res.status(400).send({ error: { code: 'resource_capped' } });
return;
}
}

const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) {
res.status(400).send({ error: { code: 'unknown_flow' } });
return;
}

const { success, error, response } = await deployPreBuilt({
environment,
account,
configs: [
{
...flow,
public_route: body.provider,
provider: body.provider,
providerConfigKey: body.providerConfigKey,
model_schema: flow.models as unknown as any,
is_public: true,
type: flow.type!,
models: flow.returns,
track_deletes: flow.track_deletes === true,
metadata: { description: flow.description, scopes: flow.scopes }
}
],
logContextGetter,
orchestrator
});

if (!success || response === null) {
res.status(503).send({ error: { code: 'failed_to_deploy', errors: [error!] } });
return;
}

await syncManager.triggerIfConnectionsExist(response.result, environmentId, logContextGetter, orchestrator);

res.status(201).send({ data: { id: response.result[0]!.id! } });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider refactoring for improved readability and error handling.

The function is well-structured and handles various scenarios effectively. However, consider the following improvements:

  • Refactor the function to reduce complexity and improve readability.
  • Use a centralized error handling mechanism to handle errors consistently.

Example refactor:

export const postPreBuiltDeploy = asyncWrapper<PostPreBuiltDeploy>(async (req, res) => {
    const emptyQuery = requireEmptyQuery(req, { withEnv: true });
    if (emptyQuery) {
        return res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
    }

    const val = validation.safeParse(req.body);
    if (!val.success) {
        return res.status(400).send({ error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) } });
    }

    const body: PostPreBuiltDeploy['Body'] = val.data;
    const { environment, account } = res.locals;
    const environmentId = environment.id;

    try {
        const config = await configService.getConfigIdByProviderConfigKey(body.providerConfigKey, environmentId);
        if (!config) throw new Error('unknown_provider');

        if (account.is_capped) {
            const isCapped = await connectionService.shouldCapUsage({ providerConfigKey: body.providerConfigKey, environmentId, type: 'activate' });
            if (isCapped) throw new Error('resource_capped');
        }

        const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
        if (!flow) throw new Error('unknown_flow');

        const { success, error, response } = await deployPreBuilt({
            environment,
            account,
            configs: [
                {
                    ...flow,
                    public_route: body.provider,
                    provider: body.provider,
                    providerConfigKey: body.providerConfigKey,
                    model_schema: flow.models as unknown as any,
                    is_public: true,
                    type: flow.type!,
                    models: flow.returns,
                    track_deletes: flow.track_deletes === true,
                    metadata: { description: flow.description, scopes: flow.scopes }
                }
            ],
            logContextGetter,
            orchestrator
        });

        if (!success || response === null) throw new Error(error!.message);

        await syncManager.triggerIfConnectionsExist(response.result, environmentId, logContextGetter, orchestrator);

        res.status(201).send({ data: { id: response.result[0]!.id! } });
    } catch (error) {
        res.status(400).send({ error: { code: error.message } });
    }
});
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.

Suggested change
export const postPreBuiltDeploy = asyncWrapper<PostPreBuiltDeploy>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
return;
}
const val = validation.safeParse(req.body);
if (!val.success) {
res.status(400).send({
error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) }
});
return;
}
const body: PostPreBuiltDeploy['Body'] = val.data;
const { environment, account } = res.locals;
const environmentId = environment.id;
const config = await configService.getConfigIdByProviderConfigKey(body.providerConfigKey, environmentId);
if (!config) {
res.status(400).send({ error: { code: 'unknown_provider' } });
return;
}
if (account.is_capped) {
const isCapped = await connectionService.shouldCapUsage({ providerConfigKey: body.providerConfigKey, environmentId, type: 'activate' });
if (isCapped) {
res.status(400).send({ error: { code: 'resource_capped' } });
return;
}
}
const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) {
res.status(400).send({ error: { code: 'unknown_flow' } });
return;
}
const { success, error, response } = await deployPreBuilt({
environment,
account,
configs: [
{
...flow,
public_route: body.provider,
provider: body.provider,
providerConfigKey: body.providerConfigKey,
model_schema: flow.models as unknown as any,
is_public: true,
type: flow.type!,
models: flow.returns,
track_deletes: flow.track_deletes === true,
metadata: { description: flow.description, scopes: flow.scopes }
}
],
logContextGetter,
orchestrator
});
if (!success || response === null) {
res.status(503).send({ error: { code: 'failed_to_deploy', errors: [error!] } });
return;
}
await syncManager.triggerIfConnectionsExist(response.result, environmentId, logContextGetter, orchestrator);
res.status(201).send({ data: { id: response.result[0]!.id! } });
export const postPreBuiltDeploy = asyncWrapper<PostPreBuiltDeploy>(async (req, res) => {
const emptyQuery = requireEmptyQuery(req, { withEnv: true });
if (emptyQuery) {
return res.status(400).send({ error: { code: 'invalid_query_params', errors: zodErrorToHTTP(emptyQuery.error) } });
}
const val = validation.safeParse(req.body);
if (!val.success) {
return res.status(400).send({ error: { code: 'invalid_body', errors: zodErrorToHTTP(val.error) } });
}
const body: PostPreBuiltDeploy['Body'] = val.data;
const { environment, account } = res.locals;
const environmentId = environment.id;
try {
const config = await configService.getConfigIdByProviderConfigKey(body.providerConfigKey, environmentId);
if (!config) throw new Error('unknown_provider');
if (account.is_capped) {
const isCapped = await connectionService.shouldCapUsage({ providerConfigKey: body.providerConfigKey, environmentId, type: 'activate' });
if (isCapped) throw new Error('resource_capped');
}
const flow = flowService.getFlowByIntegrationAndName({ provider: body.provider, type: body.type, scriptName: body.scriptName });
if (!flow) throw new Error('unknown_flow');
const { success, error, response } = await deployPreBuilt({
environment,
account,
configs: [
{
...flow,
public_route: body.provider,
provider: body.provider,
providerConfigKey: body.providerConfigKey,
model_schema: flow.models as unknown as any,
is_public: true,
type: flow.type!,
models: flow.returns,
track_deletes: flow.track_deletes === true,
metadata: { description: flow.description, scopes: flow.scopes }
}
],
logContextGetter,
orchestrator
});
if (!success || response === null) throw new Error(error!.message);
await syncManager.triggerIfConnectionsExist(response.result, environmentId, logContextGetter, orchestrator);
res.status(201).send({ data: { id: response.result[0]!.id! } });
} catch (error) {
res.status(400).send({ error: { code: error.message } });
}
});

@bodinsamuel bodinsamuel merged commit c530338 into master Aug 29, 2024
23 checks passed
@bodinsamuel bodinsamuel deleted the feat/api-prebuilt branch August 29, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants