Experiment Session Filter #1260
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
Additional details and impacted files📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
stephherbers
force-pushed
the
smh/sessions-filter
branch
from
c4bba25 to
1f076db
Compare
There was a problem hiding this comment.
This is looking great @stephherbers. Some comments but nothing major.
| }, | ||
| }); | ||
|
|
||
| showTabContent('allsessions'); |
There was a problem hiding this comment.
Why did this need to be added? Also it won't work if the user doesn't have 'view_chat' permissions
There was a problem hiding this comment.
right this is a weird permissions thing-- without this there, when a user navigates to the experiment page just the tabs are shown. Then to see the sessions the user has to click a different tab (like events) and then click sessions again to see the sessions
There was a problem hiding this comment.
Hmm, I think somewhere the tab is being hidden unexpectedly rather. It should be showing up by default, since the allsessions tab is checked by default
templates/experiments/components/archived_experiment_details.html
Outdated
Show resolved
Hide resolved
| from apps.chat.models import Chat, ChatMessage | ||
|
|
||
|
|
||
| def build_participant_filter(operator, value): |
There was a problem hiding this comment.
Just a style note (no need to take action at this point):
These could be made generic by extracting the field name:
def build_string_filter(field_name, operator, value):
...
participant_filter = build_string_filter("participant__identifier", operator, value)
templates/experiments/components/archived_experiment_details.html
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
One comment on the filter query for tags and then I did some testing locally and put together a QA doc: https://docs.google.com/document/d/1u5BHH4bKjggG3XZMk_-Y7oke6ft6nlxZb-J3cuE46Uk/edit?tab=t.0
It's mostly small stuff but I think it'd be good to get the 'MUST' section done before merging.
apps/experiments/filters.py
Outdated
| ) | ||
| .filter(matching_tag_count=len(selected_tags)) | ||
| .values_list("id", flat=True) | ||
| ) |
There was a problem hiding this comment.
I think a simpler way of doing this is to use an exists query:
content_type = ContentType.objects.get_for_model(Chat)
conditions = Q()
for tag in selected_tags:
conditions &= Exists(
CustomTaggedItem.objects.filter(
object_id=OuterRef("id"),
content_type_id=content_type,
tag__name=name,
)
)
return conditions
| if tags_query := self.request.GET.get("tags"): | ||
| tags = tags_query.split("&") | ||
| query_set = query_set.filter(chat__tags__name__in=tags).distinct() | ||
| for i in range(30): # arbitrary number higher than any # of filters we'd expect |
There was a problem hiding this comment.
we could also do a while loop here. Something like
idx = 0
while True:
...
idx += 1There was a problem hiding this comment.
It was originally a while loop. I suggested changing it to a for loop to avoid any chance of runaway
There was a problem hiding this comment.
what about doing
while filter_column and filter_operator and filter_value:
...
Description
A new and much improved filter ticket
Design doc here
Using django-filter turned out to be a bit more complicated than I thought- I still think it's worth looking into, but not worth blocking the rollout of this feature.
I still would like to work on getting the column types in the html templated so this can be used elsewhere in the codebase, but that can be a followup ticket as well
User Impact
It will make querying the data much easier and granular and hopefully they will not have to export the data just to filter it.
Also the filters are in the url so users can copy and send the link to others with the filters applied which I bet will come in handy
Demo
Loom video
Docs and Changelog
changelog yes indeed