Skip to content

feat: notify admins when a thread has 100 messages 🚨 #605

Merged
ramiAbdou merged 3 commits intocolorstackorg:mainfrom
ciaracade:slackThreadAlert
Dec 7, 2024
Merged

feat: notify admins when a thread has 100 messages 🚨 #605
ramiAbdou merged 3 commits intocolorstackorg:mainfrom
ciaracade:slackThreadAlert

Conversation

@ciaracade
Copy link
Contributor

@ciaracade ciaracade commented Nov 6, 2024

Description ✏️

Closes #535

  • Implemented a function that counts the number of replies a thread has
  • Checks if the number of messages in the thread is exactly 100
  • Sends a notification to the internal Slack with a message

🚨 Uh-oh! Thread #0000000 has gone over 💯 replies!
Better see what's going on. 👀

Type of Change 🐞

  • Feature - A non-breaking change that adds functionality.

Checklist ✅

  • I have done a self-review of my code.
  • I have manually tested my code (if applicable).
  • I have added/updated any relevant documentation (if applicable).

@ciaracade
Copy link
Contributor Author

ciaracade commented Nov 6, 2024

I am interested in any thoughts on this pull request. I understand what the original issue was asking for but I'm wondering if notify-busy-slack-thread.ts should have been split into two files: the 100-count query and a function located in packages/core/src/modules/slack/usecases that sends a message to the internal Slack and used in add-slack-message.ts

@tomas-salgado tomas-salgado changed the title feat: Add notify-busy-slack-thread.ts feat: notify admins when a thread has 100 messages 🚨 Dec 1, 2024
@tomas-salgado
Copy link
Collaborator

I am interested in any thoughts on this pull request. I understand what the original issue was asking for but I'm wondering if notify-busy-slack-thread.ts should have been split into two files: the 100-count query and a function located in packages/core/src/modules/slack/usecases that sends a message to the internal Slack and used in add-slack-message.ts

I think in this case it is fine as it is, because the logic is not too long so it's reasonable to have all of it together

@tomas-salgado tomas-salgado added the Bloomberg This issue is reserved for the Bloomberg Mentorship Program label Dec 1, 2024
Copy link
Collaborator

@tomas-salgado tomas-salgado left a comment

Choose a reason for hiding this comment

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

LGTM @ciaracade! I just made a minor fix by changing the order of the imports to fix the lint error, but this looks good. Curious to see how often this ends up sending a message 😂

@tomas-salgado tomas-salgado added the Ready ✅ This PR is ready for a final review. label Dec 1, 2024
@ciaracade
Copy link
Contributor Author

Awesome. This was so few lines I thought I was missing something. Glad to see it get approved soon : }

Copy link
Member

@ramiAbdou ramiAbdou left a comment

Choose a reason for hiding this comment

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

@ciaracade @tomas-salgado I modified this slightly to use the reply_count that comes from the Slack webhook when a reply is sent, this saves us from having to do a DB query every time a message gets sent in the Slack. See the docs here!

@ramiAbdou ramiAbdou merged commit ee5e738 into colorstackorg:main Dec 7, 2024
@ciaracade ciaracade deleted the slackThreadAlert branch December 10, 2024 03:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bloomberg This issue is reserved for the Bloomberg Mentorship Program Ready ✅ This PR is ready for a final review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notify Internal Slack when a thread gets 100+ replies 😅

3 participants