-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: move tracking of important events #25152
Conversation
Size Change: -842 B (-0.01%) Total Size: 9.21 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
…ostHog/posthog into chore/move-tracking-to-backend
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated4 snapshot changes in total. 0 added, 4 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
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.
Nice work, I know this was a lot of work. Added a few questions but overall looks good.
|
||
if "viewed" in serializer.validated_data and not request.user.is_anonymous: | ||
recording.check_viewed_for_user(user, save_viewed=True) | ||
# TODO: make sure this has all the props we want |
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.
Should this todo stay?
"start_date": instance.start_date, | ||
"end_date": instance.end_date, | ||
} | ||
if before_update.start_date is None and instance.start_date is not 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.
Clever way of doing this
@@ -344,6 +356,8 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Insight: | |||
request = self.context["request"] | |||
tags = validated_data.pop("tags", None) # tags are created separately as global tag relationships | |||
team_id = self.context["team_id"] | |||
current_url = request.headers.get("Referer") |
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 we sure this Referer
property is the way to get the current URL? Will proxies / other things affect this?
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.
I'm not sure. I don't think proxies will matter much because we don't proxy our API urls. If you have a better suggestion then I'm all ears. I'll also ask in dev.
📸 UI snapshots have been updated5 snapshot changes in total. 0 added, 5 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
…ostHog/posthog into chore/move-tracking-to-backend
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.
everything looks fine in the feature success domain!
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Problem
Our product intelligence sometimes gets blocked. It will be more reliable on the server.
Move the following to the server (or make sure they are there):
Changes
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Does this work well for both Cloud and self-hosted?
How did you test this code?