-
-
Notifications
You must be signed in to change notification settings - Fork 151
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
4682 Updated cl_send_alerts to support V2 webhooks while maintaining compatibility with V1 webhooks #4705
base: 4657-allow-users-to-select-webhooks-version
Are you sure you want to change the base?
Conversation
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.
Looks right to me after a quick skim, thanks Alberto! Onward to Eduardo for full review!
Process notes (@flooie and @s-taube, you might want to perk your ears, so we're all on the same page):
I think this should work, but I'm still experimenting. Definitely curious what we think. |
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.
@albertisfu LGTM, just one question.
cl/lib/elasticsearch_utils.py
Outdated
for result in main_results: | ||
child_result_objects = [] | ||
if hasattr(result, "child_docs"): | ||
for child_doc in result.child_docs: | ||
child_result_objects.append(child_doc.to_dict()) | ||
result["child_docs"] = child_result_objects |
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 used this loop to combine child documents from nested queries into the parent. I noticed you removed the loop in a different file and used the new set_results_child_docs
method instead. In this case, we're simply removing the logic. Won't this change impact the send_recap_alerts
command? We're accessing the child_doc
property as a dictionary key in the process_alert_hits
helper function.
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.
Good question! I removed this code because it was redundant. The do_es_sweep_alert_query
method already calls limit_inner_hits
, which sets the child_docs
key in the results. Therefore, this code was no longer necessary. I realized that we only need to perform this conversion to use a defaultdict
on child_docs
when the results are an AttrDict
instance, and they are going to be serialized for rendering an API response. That’s why it’s not required here.
This observation led me to realize that set_child_docs_and_score
was more complex than it needed to be. If the results are already a dictionary, there’s no need to convert them again. As a result, I refactored that method and also moved the logic for assigning the BM25
score here.
…tweak-cl-send-alerts-to-support-v2-webhooks
- Removed score from Opinion Search Alert webhooks
Thanks, @ERosendo! I’ve updated the branch and resolved the merge conflicts.
|
As planned in #4682, this PR updates the
cl_send_alerts
command to support V2 of the Opinion Search Alerts Webhooks.The following changes have been applied:
Below are examples of the updated Opinion Search Alert Email and Webhooks:
Webhook V1
Webhook V2
Let me know what do you think.