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): Add slack integration #524

Closed

Conversation

adityaRajGit
Copy link

@adityaRajGit adityaRajGit commented Nov 10, 2024

User description

Fixes #124

Description

This PR adds slack integration.

Dependencies

Installed slackapi/bolt-js package for slack integration

Future Improvements

NA

Mentions

@rajdip-b

Screenshots of relevant screens

NA

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Enhancement, Tests


Description

  • Implemented Slack integration with event handling capabilities, allowing various events to be emitted to Slack.
  • Added unit tests to ensure the correct instantiation and functionality of the Slack integration.
  • Updated the integration factory to include the new Slack integration.
  • Defined a new metadata interface for Slack integration to manage required parameters.
  • Added @slack/bolt as a dependency to support Slack API interactions.
  • Updated package lock files to reflect new dependencies and configurations.

Changes walkthrough 📝

Relevant files
Enhancement
3 files
slack.integration.ts
Implement Slack integration with event handling                   

apps/api/src/integration/plugins/slack/slack.integration.ts

  • Implemented Slack integration class with event handling.
  • Added methods for permitted events and required metadata.
  • Integrated Slack API to emit events.
  • Included error handling for Slack event emission.
  • +125/-0 
    integration.factory.ts
    Update integration factory to include Slack                           

    apps/api/src/integration/plugins/factory/integration.factory.ts

  • Added Slack integration to the integration factory.
  • Updated switch case to include Slack integration.
  • +3/-0     
    integration.types.ts
    Define metadata interface for Slack integration                   

    apps/api/src/integration/integration.types.ts

  • Defined SlackIntegrationMetadata interface.
  • Added metadata fields for Slack integration.
  • +6/-0     
    Tests
    1 files
    slack.integration.spec.ts
    Add unit tests for Slack integration                                         

    apps/api/src/integration/plugins/slack/slack.integration.spec.ts

  • Added unit tests for Slack integration.
  • Tested Slack integration instantiation and event permissions.
  • Verified required metadata parameters.
  • +30/-0   
    Dependencies
    4 files
    package.json
    Add Slack Bolt package dependency                                               

    apps/api/package.json

  • Added @slack/bolt package as a dependency.
  • Updated package dependencies for Slack integration.
  • +2/-0     
    package-lock.json
    Update package lock with new dependencies                               

    packages/schema/package-lock.json

  • Added zod as a dependency.
  • Updated package lock with new dependencies.
  • +18/-1   
    package-lock.json
    Update ESLint config package lock                                               

    packages/eslint-config-custom/package-lock.json

  • Updated package lock with custom ESLint config.
  • Linked local dependencies.
  • +7/-0     
    package-lock.json
    Update tsconfig package lock                                                         

    packages/tsconfig/package-lock.json

  • Updated package lock with local tsconfig dependency.
  • Linked local dependencies.
  • +8/-1     
    Additional files (token-limit)
    2 files
    pnpm-lock.yaml
    ...                                                                                                           

    pnpm-lock.yaml

    ...

    +610/-146
    package-lock.json
    ...                                                                                                           

    apps/api/package-lock.json

    ...

    +2361/-89

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The Slack integration requires sensitive credentials (botToken and signingSecret) which should be properly secured and not logged. The current implementation logs error messages that could potentially expose these credentials in stack traces.

    ⚡ Recommended focus areas for review

    Duplicate Events
    The getPermittedEvents() method contains duplicate event types: INTEGRATION_ADDED, INTEGRATION_UPDATED, and INTEGRATION_DELETED are listed twice in the set

    Error Handling
    The error handling in emitEvent() could be improved by adding more specific error types and handling different failure scenarios separately

    Resource Management
    The Slack App instance is stored as a class member but there's no cleanup mechanism when the integration is destroyed or reconnected

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Remove circular self-dependency to prevent installation and deployment issues

    Remove the circular self-dependency "api": "file:" as it creates a recursive
    dependency that could cause issues during installation and deployment.

    apps/api/package.json [37-40]

    -"@slack/bolt": "^3.13.1",
    +"@slack/bolt": "^3.13.1", 
     "@socket.io/redis-adapter": "^8.3.0",
     "@supabase/supabase-js": "^2.39.6",
    -"api": "file:",
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Circular self-dependencies can cause serious issues during package installation and deployment. Removing this problematic dependency is critical for preventing potential build failures and dependency resolution problems.

    9
    Remove duplicate entries from enumerated set of values

    Remove duplicate event types from the permitted events set to avoid redundancy and
    potential confusion.

    apps/api/src/integration/plugins/slack/slack.integration.ts [18-48]

     return new Set([
         EventType.INTEGRATION_ADDED,
    -    // ... other events ...
    -    EventType.INTEGRATION_ADDED,
         EventType.INTEGRATION_UPDATED,
    -    EventType.INTEGRATION_DELETED
    +    EventType.INTEGRATION_DELETED,
    +    // ... other unique events ...
     ])
    Suggestion importance[1-10]: 8

    Why: The permitted events set contains duplicate entries for INTEGRATION_ADDED, INTEGRATION_UPDATED, and INTEGRATION_DELETED, which could cause confusion and indicates a clear oversight that should be fixed.

    8
    Possible bug
    Add input validation to prevent runtime errors from missing required parameters

    Validate metadata parameters before attempting to emit events to prevent runtime
    errors.

    apps/api/src/integration/plugins/slack/slack.integration.ts [55-58]

     async emitEvent(
         data: IntegrationEventData,
         metadata: SlackIntegrationMetadata
     ) : Promise<void> {
    +    if (!metadata.botToken || !metadata.signingSecret || !metadata.channelId) {
    +        throw new Error('Missing required Slack metadata parameters');
    +    }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding validation for required metadata parameters is crucial to prevent runtime errors and provide clear error messages, significantly improving the robustness of the integration.

    9
    Best practice
    Initialize external service connections during object construction rather than on each method call

    Initialize the Slack app once in the constructor instead of creating a new instance
    on every event emission. This avoids potential memory leaks and improves
    performance.

    apps/api/src/integration/plugins/slack/slack.integration.ts [61-67]

    -if(!this.app)
    -{
    -  this.app = new App({
    -      token: metadata.botToken,
    -      signingSecret: metadata.signingSecret
    -  })
    +constructor(metadata?: SlackIntegrationMetadata) {
    +    super(IntegrationType.SLACK);
    +    if (metadata) {
    +        this.app = new App({
    +            token: metadata.botToken,
    +            signingSecret: metadata.signingSecret
    +        });
    +    }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Moving the Slack app initialization to the constructor would improve performance and prevent potential memory leaks. However, the suggested implementation requires metadata in constructor which may not always be available at construction time.

    7

    💡 Need additional feedback ? start a PR chat

    @rajdip-b rajdip-b changed the title feat(api) : added slack integration and unit test cases feat(api) : Add slack integration Nov 10, 2024
    @rajdip-b rajdip-b changed the title feat(api) : Add slack integration feat(api): Add slack integration Nov 10, 2024
    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

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

    I see a lot of file: dependency. can you explain what that is?

    "@socket.io/redis-adapter": "^8.3.0",
    "@supabase/supabase-js": "^2.39.6",
    "api": "file:",
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Whats this?

    Copy link
    Author

    Choose a reason for hiding this comment

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

    Sorry , I did not intend to add all of the package.json changes in the final commit. It has no significance whatsoever I was just testing/exploring.

    Copy link
    Member

    Choose a reason for hiding this comment

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

    Okay, but you should remove it in your next commit :)

    "typescript": "^4.5.3"
    },
    "dependencies": {
    "eslint-config-custom": "file:"
    Copy link
    Member

    Choose a reason for hiding this comment

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

    Not sure why you included this

    @adityaRajGit adityaRajGit deleted the slack-integration branch November 11, 2024 11:51
    @adityaRajGit adityaRajGit restored the slack-integration branch November 11, 2024 11:51
    @adityaRajGit adityaRajGit deleted the slack-integration branch November 11, 2024 12:27
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    Integration: Add Slack
    2 participants