-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Ephemeral slack messages as responses to user questions #4142
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -161,27 +161,33 @@ class SeedPresaveDocument(BaseModel): | |||
url="https://docs.onyx.app/more/use_cases/overview", | |||
title=overview_title, | |||
content=overview, | |||
title_embedding=model.encode(f"search_document: {overview_title}"), | |||
content_embedding=model.encode(f"search_document: {overview_title}\n{overview}"), | |||
title_embedding=list(model.encode(f"search_document: {overview_title}")), |
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.
are these just typing changes?
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.
Yes. For anopther PR I need to upgrade HF version, which apparently changes the type here, so 'list' was needed to make it work with my local version.
As that is another PR though, I will remove.
@@ -72,6 +72,11 @@ def make_slack_api_rate_limited( | |||
@wraps(call) | |||
def rate_limited_call(**kwargs: Any) -> SlackResponse: | |||
last_exception = None | |||
|
|||
if "thread_ts" in kwargs: |
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.
can we add a comment as to why we're doing this? Also, are we sure this should be the desired logic for all slack calls?
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.
Was actually not necessary. Sending thread_ts as None is equivalent to removing that key.
Code snippet removed.
backend/onyx/onyxbot/slack/blocks.py
Outdated
channel_conf: ChannelConfig | None = None, | ||
feedback_reminder_id: str | None = None, | ||
) -> Block: | ||
if not chat_message_id: |
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.
we shouldn't need to have these checks here—we can just rely on mypy / typing to handle this.
In other words, we could just have the typing as:
chat_message_id: int
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.
Agreed. Done.
backend/onyx/onyxbot/slack/blocks.py
Outdated
if not chat_message_id: | ||
raise ValueError("Chat message id is required to change the ephemeral message") | ||
|
||
if message_info is None: |
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.
same as the ^ for these 2
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.
Done
backend/onyx/onyxbot/slack/utils.py
Outdated
def build_publish_ephemeral_message_id( | ||
original_question_ts: str | None = None, | ||
) -> str: | ||
if original_question_ts is None: |
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.
same comment here
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.
done
if action_id == SHOW_EVERYONE_ACTION_ID: | ||
# Convert to non-ephemeral message in thread | ||
try: | ||
webhook.send( |
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.
what does this one specifically do? Would be nice to add a comment here
# The additional data required that was added to buttons. | ||
# Specifically, this contains the message_info, channel_conf information | ||
# and some additional attributes. | ||
value_dict = json.loads(req.payload["actions"][0]["value"]) |
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.
for future reference / a small enhancement: would prefer to get things into the "typed world" as soon as possible. In this case, that would mean converting this into a pydantic model / a typed dict.
todos: - search using private docs - review
- Updated frontend descriptions - removal of 'I need help'-type options - proper handling of response group - no response group + ephemeral
- always check ACL - if not ephemeral, only use public docs (user = None) - changed warning in slack bot - shortened 'share with everyone' button
3ce1225
to
a14060b
Compare
Description
We want to enable our customers to configure an Onyx bot for a channel such that the bot is able to respond as an ephemeral message to the user who is asking the question. That provides us with the ability to leverage documents for the search that a user has private access to, not just documents that are public to everyone.
See details in the current design doc.
Ticket: https://linear.app/danswer/issue/DAN-1489/enable-ephemeral-replies-by-answer-bot-in-group-channels
How Has This Been Tested?
Locally:
Key test cases:
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.