-
Notifications
You must be signed in to change notification settings - Fork 64
feat: iOS surveys use the new response question id format #383
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
Conversation
/// Sends a `survey dismissed` event to PostHog instance | ||
private func sendSurveyDismissedEvent(survey: PostHogSurvey) { | ||
let additionalProperties: [String: Any] = [ | ||
"$survey_questions": survey.questions.map(\.question), |
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.
survey dismissed should not have $survey_questions
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.
in the posthog-js, it also includes the $survey_questions
property.
it's useful for getting all responses up until when the user dismissed it
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.
mmm i dont follow because our APIs only query for survey sent events when getting responses, how is that useful?
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.
if I recall correctly, I copied over from what posthog-js ws doing, thinking that it's part of the partial response feature (was wip back then if I remember correctly), but good point on APIs above. Let's wait for @lucasheriques to comment. If we are not using this anywhere, we can drop from all sdks?
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 think makes sense, we can drop it. When we implement partial responses for mobile SDKs (sometime in the future) we'll have access to all responses regardless. Will remove from the JS sdk too
@ioannisj we will need to check if this breaks with Flutter or not due to the missing |
@ioannisj i have found no tests for |
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.
code looks good, pr is still in draft though
but as mentioned, I'd also include the $survey_questions in the dismissed event, so there's a way to get the responses in case someone answers 1 question (in a 3 question survey) and then dismisses it
TIL https://github.com/PostHog/posthog-js/blob/f293275d1379327851e3c2311be59757c030c590/packages/browser/src/extensions/surveys/surveys-extension-utils.tsx#L453 |
Since we haven't touched the public APIs, I think we should be okay. We grab the survey id on native layer when we get a survey response. Need to run out in a bit for a while, I'll test as soon as I'm back though just to be sure
No, sadly I forgot to add unit tests on the events back then |
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.
Left a couple of minor comments on public APIs. Also, since we are missing some unit tests on survey_sent event, would be cool to add them. I'm positive this won't break Flutter but I'll test soon
@ioannisj would you like to add a test and finish this since our hackathon started and wont have more time this week? |
@ioannisj can we get this out? |
yeap on it, missed your previous comment |
💡 Motivation and Context
follow up PostHog/posthog-js#2352
Closes #323
💚 How did you test it?
📝 Checklist