chore: use question id instead of question index for survey queries #29226
Conversation
lucasheriques
changed the base branch from
master
to
chore/add-question-id-to-surveys
|
Size Change: 0 B Total Size: 9.73 MB ℹ️ View Unchanged
|
| @@ -147,7 +148,42 @@ export interface SurveyDateRange { | |||
| date_to: string | null | |||
| } | |||
|
|
|||
| const getResponseField = (i: number): string => (i === 0 ? '$survey_response' : `$survey_response_${i}`) | |||
| // New function that supports both index-based and ID-based approaches | |||
There was a problem hiding this comment.
Are all methods here used elsewhere? otherwise we should not default to export
There was a problem hiding this comment.
need export to do tests; i can also do a hard copy of them on the test file if preferable though
There was a problem hiding this comment.
no hard copying isn't ideal.
I usually would test the 1st method to be called and just assert the final result; your method input has to allow running into all code paths with the inner methods as well of course, it's more like an integration test that tests multiple methods rather than every single method, I usually find this approach better, and a must on SDKs since you cant expose everything otherwise everything is a public API, here is different but I think it still makes sense if easy to apply the same concept
| arrayJoin(JSONExtractArrayRaw(properties, '${fields[1] || ''}')), | ||
| arrayJoin(JSONExtractArrayRaw(properties, '${fields[0]}')) | ||
| )` | ||
| } | ||
|
|
||
| function duplicateExistingSurvey(survey: Survey | NewSurvey): Partial<Survey> { |
There was a problem hiding this comment.
I guess not in the scope of this PR but when duplicating an existing survey, should we clear question ids?
There was a problem hiding this comment.
its not super important because questions are just a JSON array and not its own database column but I'd do it anyway
…on-id-instead-of-question-index-for-survey-queries
lucasheriques
changed the title
|
Not sure if it's gonna be on this PR (likely a new one), but remember to re-enable the questions reordering drag & drop. |
📸 UI snapshots have been updated3 snapshot changes in total. 0 added, 3 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Problem
uses the new field question id to make survey queries instead
relates to #27200
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?