[INTPROD-9686] Add threaded conversation support #115
Conversation
|
PTAL @grahamnedelka |
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces support for handling thread replies in Slack apps by adding configuration options and updating message processing logic to allow thread replies to be processed when explicitly enabled.
- Add
match_reply: trueconfiguration option to allow handlers to process thread replies - Remove automatic filtering of thread messages and add
parent_user_idproperty toSlackMessage - Update documentation to explain the new thread reply handling configuration
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| omnibot/processor.py | Added logic to skip thread replies unless handler has match_reply: true |
| omnibot/services/slack/message.py | Removed thread filtering, added parent_user_id property and improved thread_ts documentation |
| tests/unit/omnibot/services/slack/message_test.py | Removed test case that expected thread messages to be unsupported |
| docs/root/adding_new_slack_apps.rst | Added documentation for the new match_reply: true configuration option |
| """ | ||
| The user_id of the parent message, if this is a reply to a message. | ||
| """ | ||
| return self._payload["parent_user_id"] |
There was a problem hiding this comment.
The parent_user_id property can return None when parent_user_id is not present in the event, but this isn't documented in the docstring. Consider adding a return type annotation and documenting the None case.
| def thread_ts(self): | ||
| """ | ||
| The timestamp of the parent message, if this is a reply to a message. | ||
| :return: |
There was a problem hiding this comment.
The docstring for thread_ts property is missing return type information and doesn't document that it can return None when not in a thread.
| :return: | |
| :return: The timestamp of the parent message as a string, or None if the message is not part of a thread. | |
| :rtype: Optional[str] |
| super().__init__(bot, event, event_trace, "message") | ||
| self._payload["ts"] = event["ts"] | ||
| self._payload["thread_ts"] = event.get("thread_ts") | ||
| self._payload["parent_user_id"] = event.get("parent_user_id") |
There was a problem hiding this comment.
Why not reference to parent message
message.parent.user.id
There was a problem hiding this comment.
is the user id for the first message in the thread and exists for thread replies but not for regular messages
There was a problem hiding this comment.
feel this needs to go on infradocs
colinsl
left a comment
colinsl
left a comment
There was a problem hiding this comment.
naming convention comments.
This pull request introduces support for handling thread replies in Slack apps by adding new configuration options and updating the message processing logic. The changes ensure that thread replies can be properly matched and processed by handlers, while also improving the overall structure of the
SlackMessageclass.Enhancements for handling thread replies:
docs/root/adding_new_slack_apps.rstto explain that settingmatch_reply: trueis required for apps to handle thread replies._process_message_message_handlersinomnibot/processor.pyto skip processing thread replies unless the handler explicitly specifiesmatch_reply: true.Updates to the
SlackMessageclass:parent_user_idto the_payloaddictionary inomnibot/services/slack/message.pyto capture the user ID of the parent message in thread replies._check_unsupportedthat previously ignored thread messages, allowing them to be processed.parent_user_idand updated thethread_tsproperty inomnibot/services/slack/message.pyto provide clearer access to thread-related metadata.