-
Notifications
You must be signed in to change notification settings - Fork 164
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
Conversation
I am interested in any thoughts on this pull request. I understand what the original issue was asking for but I'm wondering if |
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 |
There was a problem hiding this 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 😂
Awesome. This was so few lines I thought I was missing something. Glad to see it get approved soon : } |
8163882
to
bd47d8e
Compare
There was a problem hiding this 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!
Description ✏️
Closes #535
Type of Change 🐞
Checklist ✅