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: notify admins when a thread has 100 messages 🚨 #605

Merged
merged 3 commits into from
Dec 7, 2024

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
1 check passed
@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