-
Notifications
You must be signed in to change notification settings - Fork 138
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: add property so we can check if a client is using a proxy #1084
Conversation
Size Change: +87.6 kB (+10%) Total Size: 941 kB
ℹ️ View Unchanged
|
@pauldambra I'd love your thoughts on this PR and implementation. Another route would be to add an |
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.
Sweet - you will want to add the bump patch
label so that when merged it automatically publishes.
You might get tripped up by tests here as a bunch are really badly setup so are probably throwing due to api_host not being set (too much mocking...)
Sounds good, I'll add a few more tests and test things before planning the patch. |
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.
Pushed a change to fix the tests as it might not have been super obvious how to fix it.
Just needs a test for this logic now and its good
@benjackwhite should this property be added to the posthog in the taxonomy and property definitions? |
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.
@zlwaterfield yeah would be good to add to the list but not urgent
Changes
We are trying to determine whether or not a user is sending a events via a proxy in the dashboard so we can customize the experience for them. This will allow us to determine that.
Checklist