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: slack client to support multiple org #102

Merged
merged 17 commits into from
Jan 26, 2024
Merged

feat: slack client to support multiple org #102

merged 17 commits into from
Jan 26, 2024

Conversation

ekremney
Copy link
Member

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

Thanks for contributing!

Copy link

This PR will trigger a minor release when merged.

Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

i'd propose to split the client-factory part from the actual slack client. this aligns with the principle of single responsibility, ensuring that each client instance is dedicated to a specific target (i.e., a Slack workspace). This redesign would simplify the client interface by removing the need to specify the target for every public method call.

Incomplete example:

class SlackClientFactory {
  constructor(log) {
    this.clients = {};
    this.log = log;
  }

  // createFrom / lambda context handling here...

  getClientForTarget(target) {
    if (!this.clients[target]) {
      const token = this.getTokenForTarget(target);
      if (!token) {
        throw new Error(`No token found for target: ${target}`);
      }
      this.clients[target] = new SlackClient(token, this.log);
    }
    return this.clients[target];
  }

  getTokenForTarget(target) {
    return process.env[`${ENV_PREFIX}${target}`];
  }
}

class SlackClient {
  constructor(token, log) {
    this.client = new WebClient(token);
    this.log = log;
  }

  async postMessage(message, opts = {}) {
    try {
      const result = await this.client.chat.postMessage({
        ...DEFAULT_PARAMS,
        text: message,
        ...opts,
      });

      return {
        channelId: result.channel,
        threadId: result.ts,
      };
    } catch (error) {
      this.log.error('Slack message failed to send');
      throw error;
    }
  }
}

this also benefits security:

  • a client only can deal with one workspace and not accidentally others
  • it is ground work for later adding roles, i.e. clients backed by different scoped slack tokens, for example a privileged client that can create channels, while regular clients only send messages, etc.

packages/spacecat-shared-slack-client/README.md Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/src/index.js Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/src/index.js Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/src/index.js Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/src/index.js Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/src/index.d.ts Outdated Show resolved Hide resolved
@ekremney
Copy link
Member Author

I incorporated all the reviews and added the file upload method. Please re-review

Copy link
Member

@solaris007 solaris007 left a comment

Choose a reason for hiding this comment

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

LGTM, only minor comments

packages/spacecat-shared-slack-client/package.json Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/package.json Outdated Show resolved Hide resolved
packages/spacecat-shared-slack-client/package.json Outdated Show resolved Hide resolved
@solaris007 solaris007 added the enhancement New feature or request label Jan 26, 2024
@ekremney ekremney merged commit a20bce8 into main Jan 26, 2024
6 checks passed
@ekremney ekremney deleted the slack-client branch January 26, 2024 07:52
adobe-bot pushed a commit that referenced this pull request Jan 26, 2024
# @adobe/spacecat-shared-slack-client-v1.0.0 (2024-01-26)

### Features

* slack client to support multiple org ([#102](#102)) ([a20bce8](a20bce8))
@adobe-bot
Copy link

🎉 This PR is included in version @adobe/spacecat-shared-slack-client-v1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants