-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: query-ify the matching_events API #26916
Conversation
@@ -69,7 +69,6 @@ export function RecordingRow({ recording }: RecordingRowProps): JSX.Element { | |||
onClick={() => { | |||
openSessionPlayer({ | |||
id: recording.id, | |||
matching_events: recording.matching_events, |
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.
these are never present here, so we can delete this
const response = await api.recordings.getMatchingEvents(params) | ||
} | ||
if (values.listAPIAsQuery) { | ||
params.as_query = true |
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.
switch to the new query code (or not)
raise exceptions.ValidationError( | ||
"Must specify exactly one session_id", | ||
) | ||
if use_query_type: |
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.
a little duplication is ok since we're going to delete one branch pretty soon
] | ||
|
||
response = self.client.get( | ||
f"/api/projects/{self.team.id}/session_recordings/matching_events?{'&'.join(query_params)}&as_query=true" |
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.
prove it works
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
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.
Extremely low context, but I trust the feature flag
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This API still depends on the old shaped filters which I want to delete
So, shift it to not using those... (behind a flag)