feat(surveys): adds targeting changes to the survey revision history #23834
Conversation
…bvious way. And I need to add the UI for pulling data from featureflag serialization
| @@ -260,6 +260,22 @@ export function flagActivityDescriber(logItem: ActivityLogItem, asNotification?: | |||
| return { description: null } | |||
| } | |||
|
|
|||
| if (logItem.activity === 'created') { | |||
There was a problem hiding this comment.
drive by fix – I noticed that we never actually visualize (but we do log) creation events in the feature flag history! Here's what it look like now
| updated <strong>{questionChanges.length}</strong> question{questionChanges.length !== 1 ? 's' : ''}: | ||
| <ul className="bullet-list"> | ||
| {questionChanges.map(({ index, changes }) => ( | ||
| <li key={index}> | ||
| Question {index}: | ||
| <ul className="bullet-list"> | ||
| {changes.map((changeItem, changeIndex) => ( | ||
| <li key={changeIndex}>{changeItem}</li> |
There was a problem hiding this comment.
this gives us a UI like this, which I'm pretty happy with (although the "Question 1" bit seems bit clunky, I just wanted to let them know which question was actually changed!)
|
|
||
| // Use JSON.stringify for deep comparison of objects | ||
| if (JSON.stringify(beforeConditions?.events) !== JSON.stringify(afterConditions?.events)) { | ||
| changes.push(<>modified event conditions</>) |
There was a problem hiding this comment.
elected to not go into details about the event condition changes, just letting users know that they were changed at all.
| ] | ||
| : [] | ||
|
|
||
| const specificChanges = match([before, after] as const) |
There was a problem hiding this comment.
some cool shit in here with ts-pattern that lets me match a tuple of types with a very nice combinator-style developer experience.
| return [...commonChanges, ...typeChangeDescription, ...specificChanges, ...describeBranchingChanges(before, after)] | ||
| } | ||
|
|
||
| export function describeCommonChanges(before: SurveyQuestion, after: SurveyQuestion): JSX.Element[] { |
There was a problem hiding this comment.
I still have functions that are comparing outcomes conditionally, but now that they can be flattened by the .with() combinator, this whole chunk of code gets easier to read.
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
…tHog/posthog into feat/add-targeting-to-survey-history
|
Size Change: 0 B Total Size: 1.07 MB ℹ️ View Unchanged
|
| expect(getTextContent(changes[0])).toBe( | ||
| 'changed question text from "What is your favorite color?" to "What is your favorite animal?"' | ||
| ) | ||
| expect(getTextContent(changes[1])).toBe('updated description') |
There was a problem hiding this comment.
why not also include the before/after description, like with the question change above?
There was a problem hiding this comment.
overlooked it; great feedback. Will add.
| expect(changes).toHaveLength(3) | ||
| expect(getTextContent(changes[0])).toBe('changed rating display from emoji to number') | ||
| expect(getTextContent(changes[1])).toBe('changed rating scale from 5 to 10') | ||
| expect(getTextContent(changes[2])).toBe('updated rating labels from Poor-Excellent to Bad-Good') |
There was a problem hiding this comment.
Multi-word labels will be rendered like this ...from Very bad-Very good to Quite bad-Quite good
I'd suggest double quotes here: ...from "Very bad"-"Very good" to "Quite bad"-"Quite good"
| if (value.length <= 50) { | ||
| return <strong>"{value}"</strong> | ||
| } | ||
| return <strong>"{value.slice(0, 50)}..."</strong> |
There was a problem hiding this comment.
nit: you can use this handy utility function
posthog/frontend/src/lib/utils.tsx
Line 640 in 86ac919
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 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. |
…tHog/posthog into feat/add-targeting-to-survey-history
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 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. |
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 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. |
…23834) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
…ostHog#23834) Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Closes #23725
A few interesting things that I did (things worth looking at in greater depth IMO)
surveys.pyto handle manually inserting changelog entries whenever we make changes to the survey targeting (this has to be done manually because the existingchanges_betweenabstraction only supports calculating changes within an existing model, and doesn't support foreign key relationships. I wrote a test for this new behavior.surveyActivityDescriberto handle displaying changes to survey components in a easy-to-digest way. Outside of just writing a lot of code, these changes included:activity_logchanges_betweenmethod to prevent it from failing when it tried to look up objects that didn't exist. That's by design!Changes
Feel free to pull this to your local branch and add some targeting behavior yourself, here's what my survey history looks like after a bunch of manual testing
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
surveyActivityDescriber