[INTPROD-9625] Add support for reaction slack events#112
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for processing Slack reaction events by introducing a new Reaction class and integrating reaction handlers into the bot's event processing.
- Added a Reaction class to encapsulate reaction event data and helper methods.
- Integrated reaction handler registration and processing in the bot and event processor.
- Added new Redis caching functions for fetching messages and bot info to support the reaction functionality.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| omnibot/services/slack/reaction.py | New Reaction class with duplicate reaction attribute handling logic. |
| omnibot/services/slack/bot.py | Added reaction handlers registration in bot initialization. |
| omnibot/services/slack/init.py | Added functions for fetching messages and bot info using Redis caching. |
| omnibot/processor.py | Integrated reaction events processing and reaction callbacks. |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for handling Slack reaction events by introducing a new Reaction class, integrating reaction handlers in the bot configuration, and updating event processing to handle reaction_added and reaction_removed events.
- Introduces a new Reaction class to encapsulate reaction event data.
- Registers reaction handlers in the bot configuration.
- Updates the event processor to process reaction events with appropriate callbacks.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| omnibot/services/slack/reaction.py | New Reaction class with logic for parsing reaction events; duplication in reaction attribute assignment noted. |
| omnibot/services/slack/bot.py | Registers and exposes reaction handlers alongside other event handlers. |
| omnibot/services/slack/init.py | Adds helper functions for fetching messages and bot info used in reaction processing. |
| omnibot/processor.py | Extends event processing to handle reaction_added/reaction_removed events and invoke reaction callbacks. |
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
if event_type == "message" or event_type == "app_mention" looks like it opens up for duplicates
| # Ignore slackbot as it doesn't get classified as a bot user | ||
| elif self.user and self.user == "USLACKBOT": | ||
| logger.debug("ignoring reaction from slackbot", extra=self.event_trace) | ||
| unsupported = True | ||
| if unsupported: | ||
| statsd = stats.get_statsd_client() | ||
| statsd.incr("event.unsupported") | ||
| raise ReactionUnsupportedError() |
There was a problem hiding this comment.
why raising when you say it's ignoring?
There was a problem hiding this comment.
this is the same way as message.py
There was a problem hiding this comment.
are any of these common with message handler base? just override and call base a possibility then?
| ), | ||
| ) | ||
| redis_client = omniredis.get_redis_client() | ||
| hash_key = f"message:{bot.team.name}:{channel}" |
There was a problem hiding this comment.
is key by channel or message? If it's by channel why prefixed as message?
There was a problem hiding this comment.
the hash key is by channel, message just indicates the data type, this pattern is used in other slack functions
There was a problem hiding this comment.
Pull Request Overview
Adds support for processing Slack reaction events by introducing a new Reaction class, reworking message parsing into a shared BaseMessage, and leveraging Redis caching for messages and bot info.
- Introduces
Reactionand updatesprocessor.pyto handlereaction_added/reaction_removed - Refactors
Messageinto a subclass ofBaseMessageand removes redundant code - Adds Redis-backed
get_messageandget_bot_infohelpers inslack/__init__.py
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| omnibot/services/slack/reaction.py | New Reaction class with parsing and unsupported-event checks |
| omnibot/services/slack/message.py | Converted Message to extend BaseMessage, removed old props |
| omnibot/services/slack/base_message.py | New base class for shared event parsing logic |
| omnibot/services/slack/init.py | Added get_message and get_bot_info for Redis caching |
| omnibot/processor.py | Updated dispatcher to route reaction events to new handlers |
omnibot/processor.py
Outdated
| _process_message_message_handlers(message) | ||
| except MessageUnsupportedError: | ||
| pass | ||
| elif event_type == "reaction_added" or event_type == "reaction_removed": |
There was a problem hiding this comment.
wasn't it clearer before with
messages
--> process message
reactions
--> process reactions?
| statsd.incr("event.ignored") | ||
|
|
||
|
|
||
| def _is_message_from_bot(bot: Bot, channel: str, ts: str): |
There was a problem hiding this comment.
comment why this is necessary vs just looking up bot from lookup table of registered handlers
| logger.debug("Event is not a message or reaction type.", extra=event_trace) | ||
| logger.debug(event) |
| def _process_message_event(bot, event_info, event_trace, event_type): | ||
| """ | ||
| Process message or app_mention events. | ||
| """ | ||
| statsd = stats.get_statsd_client() | ||
| try: | ||
| with statsd.timer("process_event"): | ||
| logger.debug( | ||
| f"Processing event: {json.dumps(event_info, indent=2)}", | ||
| extra=event_trace, | ||
| ) | ||
| try: | ||
| message = Message(bot, event_info, event_trace) | ||
| _process_message_message_handlers(message) | ||
| except MessageUnsupportedError: | ||
| pass | ||
| except Exception: | ||
| statsd.incr(f"event.process.failed.{event_type}") | ||
| logger.exception( | ||
| "Could not process message event.", | ||
| exc_info=True, | ||
| extra=event_trace, | ||
| ) | ||
|
|
||
|
|
||
| def _process_reaction_event(bot, event_info, event_trace, event_type): | ||
| """ | ||
| Process reaction_added or reaction_removed events. | ||
| """ | ||
| statsd = stats.get_statsd_client() | ||
| try: | ||
| with statsd.timer("process_event"): | ||
| logger.debug( | ||
| f"Processing event: {json.dumps(event_info, indent=2)}", | ||
| extra=event_trace, | ||
| ) | ||
| try: | ||
| reaction = Reaction(bot, event_info, event_trace) | ||
| _process_reaction_message_handlers(reaction) | ||
| except ReactionUnsupportedError: | ||
| pass | ||
| except Exception: | ||
| statsd.incr(f"event.process.failed.{event_type}") | ||
| logger.exception( | ||
| "Could not process reaction event.", | ||
| exc_info=True, | ||
| extra=event_trace, | ||
| ) |
There was a problem hiding this comment.
maybe better as a decorator
| if not message or "bot_id" not in message: | ||
| logger.warning("Failed to retrieve valid message or 'bot_id' is missing") | ||
| return False | ||
| # There can be multiple bot_ids for the same bot |
| # `bot` is the receiving bot instance handling this reaction event, | ||
| # not the user or bot who sent the reaction. |
There was a problem hiding this comment.
maybe better as docstring?
2 participants
Adds support for reaction slack events.
For now, reaction handlers will only do callbacks if the reaction is on a matching bot slack message.
reactionmatch_typethat will match against regex of emoji names