feat(surveys): adds targeting changes to the survey revision history (#23834)

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

frontend/src/scenes/feature-flags/activityDescriptions.tsx

@@ -260,6 +260,22 @@ export function flagActivityDescriber(logItem: ActivityLogItem, asNotification?:
         return { description: null }
     }
 
+    if (logItem.activity === 'created') {
+        return {
+            description: (
+                <SentenceList
+                    listParts={[<>created a new feature flag:</>]}
+                    prefix={
+                        <>
+                            <strong>{userNameForLogItem(logItem)}</strong>
+                        </>
+                    }
+                    suffix={<> {nameOrLinkToFlag(logItem?.item_id, logItem?.detail.name)}</>}
+                />
+            ),
+        }
+    }
+
     if (logItem.activity == 'updated') {
         let changes: Description[] = []
         let changeSuffix: Description = (

frontend/src/scenes/surveys/surveyActivityDescriber.test.tsx

@@ -0,0 +1,262 @@
+import { render } from '@testing-library/react'
+
+import {
+    LinkSurveyQuestion,
+    MultipleSurveyQuestion,
+    RatingSurveyQuestion,
+    SurveyQuestion,
+    SurveyQuestionBranchingType,
+    SurveyQuestionType,
+} from '~/types'
+
+import {
+    describeBranchingChanges,
+    describeCommonChanges,
+    describeFieldChange,
+    describeLinkChanges,
+    describeMultipleChoiceChanges,
+    describeQuestionChanges,
+    describeRatingChanges,
+} from './surveyActivityDescriber'
+
+const getTextContent = (jsxElement: JSX.Element): string => {
+    const { container } = render(jsxElement)
+    return container.textContent || ''
+}
+
+describe('describeFieldChange', () => {
+    test('sets field with unit', () => {
+        const result = describeFieldChange('wait period', null, 30, 'days')
+        expect(getTextContent(result)).toBe('set wait period to 30 days')
+    })
+
+    test('removes field with unit', () => {
+        const result = describeFieldChange('wait period', 30, null, 'days')
+        expect(getTextContent(result)).toBe('removed wait period (was 30 days)')
+    })
+
+    test('changes field with unit', () => {
+        const result = describeFieldChange('wait period', 30, 60, 'days')
+        expect(getTextContent(result)).toBe('changed wait period from 30 days to 60 days')
+    })
+
+    test('sets field without unit', () => {
+        const result = describeFieldChange('response limit', null, 100)
+        expect(getTextContent(result)).toBe('set response limit to 100')
+    })
+
+    test('removes field without unit', () => {
+        const result = describeFieldChange('response limit', 100, null)
+        expect(getTextContent(result)).toBe('removed response limit (was 100)')
+    })
+
+    test('changes field without unit', () => {
+        const result = describeFieldChange('response limit', 100, 200)
+        expect(getTextContent(result)).toBe('changed response limit from 100 to 200')
+    })
+
+    test('handles undefined before value', () => {
+        const result = describeFieldChange('iteration count', undefined, 5)
+        expect(getTextContent(result)).toBe('set iteration count to 5')
+    })
+
+    test('handles undefined after value', () => {
+        const result = describeFieldChange('iteration count', 5, undefined)
+        expect(getTextContent(result)).toBe('removed iteration count (was 5)')
+    })
+
+    test('handles empty string before value', () => {
+        const result = describeFieldChange('survey title', '', 'New Title')
+        expect(getTextContent(result)).toBe('set survey title to New Title')
+    })
+
+    test('handles empty string after value', () => {
+        const result = describeFieldChange('survey title', 'Old Title', '')
+        expect(getTextContent(result)).toBe('removed survey title (was Old Title)')
+    })
+
+    test('handles both values as empty strings', () => {
+        const result = describeFieldChange('survey title', '', '')
+        expect(getTextContent(result)).toBe('')
+    })
+
+    test('handles before and after as identical', () => {
+        const result = describeFieldChange('response limit', 100, 100)
+        expect(getTextContent(result)).toBe('')
+    })
+
+    test('handles string values with unit', () => {
+        const result = describeFieldChange('response time', 'fast', 'slow', 'seconds')
+        expect(getTextContent(result)).toBe('changed response time from fast seconds to slow seconds')
+    })
+
+    test('handles boolean values', () => {
+        const result = describeFieldChange('is active', false, true)
+        expect(getTextContent(result)).toBe('changed is active from false to true')
+    })
+
+    test('handles null values', () => {
+        const result = describeFieldChange('response limit', null, null)
+        expect(getTextContent(result)).toBe('')
+    })
+})
+
+describe('describeCommonChanges', () => {
+    const before: SurveyQuestion = {
+        question: 'What is your favorite color?',
+        description: 'Choose a color',
+        type: SurveyQuestionType.SingleChoice,
+        optional: false,
+        buttonText: 'Next',
+        choices: ['Red', 'Blue', 'Green'],
+    }
+    const after: SurveyQuestion = {
+        ...before,
+        question: 'What is your favorite animal?',
+        description: 'Choose an animal',
+        optional: true,
+        buttonText: 'Continue',
+    }
+
+    test('describes common changes', () => {
+        const changes = describeCommonChanges(before, after)
+        expect(changes).toHaveLength(4)
+        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(
+            'changed the question description from "Choose a color" to "Choose an animal"'
+        )
+        expect(getTextContent(changes[2])).toBe('made question optional')
+        expect(getTextContent(changes[3])).toBe('changed button text from "Next" to "Continue"')
+    })
+})
+
+describe('describeLinkChanges', () => {
+    const before: LinkSurveyQuestion = {
+        question: 'Visit our website',
+        type: SurveyQuestionType.Link,
+        link: 'http://example.com',
+    }
+    const after: LinkSurveyQuestion = {
+        ...before,
+        link: 'http://example.org',
+    }
+
+    test('describes link changes', () => {
+        const changes = describeLinkChanges([before, after])
+        expect(changes).toHaveLength(1)
+        expect(getTextContent(changes[0])).toBe('updated link from http://example.com to http://example.org')
+    })
+})
+
+describe('describeRatingChanges', () => {
+    const before: RatingSurveyQuestion = {
+        question: 'Rate our service',
+        type: SurveyQuestionType.Rating,
+        display: 'emoji',
+        scale: 5,
+        lowerBoundLabel: 'Poor',
+        upperBoundLabel: 'Excellent',
+    }
+    const after: RatingSurveyQuestion = {
+        ...before,
+        display: 'number',
+        scale: 10,
+        lowerBoundLabel: 'Bad',
+        upperBoundLabel: 'Good',
+    }
+
+    test('describes rating changes', () => {
+        const changes = describeRatingChanges([before, after])
+        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"')
+    })
+})
+
+describe('describeMultipleChoiceChanges', () => {
+    const before: MultipleSurveyQuestion = {
+        question: 'Select your hobbies',
+        type: SurveyQuestionType.MultipleChoice,
+        choices: ['Reading', 'Traveling', 'Cooking'],
+        shuffleOptions: false,
+        hasOpenChoice: false,
+    }
+    const after: MultipleSurveyQuestion = {
+        ...before,
+        choices: ['Reading', 'Cooking', 'Gaming'],
+        shuffleOptions: true,
+        hasOpenChoice: true,
+    }
+
+    test('describes multiple choice changes', () => {
+        const changes = describeMultipleChoiceChanges([before, after])
+        expect(changes).toHaveLength(4)
+        expect(getTextContent(changes[0])).toBe('added choices: Gaming')
+        expect(getTextContent(changes[1])).toBe('removed choices: Traveling')
+        expect(getTextContent(changes[2])).toBe('enabled option shuffling')
+        expect(getTextContent(changes[3])).toBe('added open choice option')
+    })
+})
+
+describe('describeBranchingChanges', () => {
+    const before: MultipleSurveyQuestion = {
+        question: 'Do you like ice cream?',
+        type: SurveyQuestionType.SingleChoice,
+        choices: ['Yes', 'No'],
+        branching: {
+            type: SurveyQuestionBranchingType.NextQuestion,
+        },
+    }
+    const after: MultipleSurveyQuestion = {
+        ...before,
+        branching: {
+            type: SurveyQuestionBranchingType.End,
+        },
+    }
+
+    test('describes branching changes', () => {
+        const changes = describeBranchingChanges(before, after)
+        expect(changes).toHaveLength(1)
+        expect(getTextContent(changes[0])).toBe('updated branching logic')
+    })
+})
+
+describe('describeQuestionChanges', () => {
+    const before: MultipleSurveyQuestion = {
+        question: 'Do you like ice cream?',
+        type: SurveyQuestionType.SingleChoice,
+        description: 'Please answer honestly',
+        optional: false,
+        buttonText: 'Next',
+        choices: ['Yes', 'No'],
+        branching: {
+            type: SurveyQuestionBranchingType.NextQuestion,
+        },
+    }
+    const after: MultipleSurveyQuestion = {
+        question: 'Do you like pizza?',
+        type: SurveyQuestionType.MultipleChoice,
+        description: 'Please answer honestly',
+        optional: true,
+        buttonText: 'Continue',
+        choices: ['Yes', 'No', 'Maybe'],
+        branching: {
+            type: SurveyQuestionBranchingType.End,
+        },
+    }
+    test('describes all changes in a question', () => {
+        const changes = describeQuestionChanges(before, after)
+        expect(changes).toHaveLength(6)
+        expect(getTextContent(changes[0])).toBe(
+            'changed question text from "Do you like ice cream?" to "Do you like pizza?"'
+        )
+        expect(getTextContent(changes[1])).toBe('made question optional')
+        expect(getTextContent(changes[2])).toBe('changed button text from "Next" to "Continue"')
+        expect(getTextContent(changes[3])).toBe('changed question type from single_choice to multiple_choice')
+        expect(getTextContent(changes[4])).toBe('added choices: Maybe')
+        expect(getTextContent(changes[5])).toBe('updated branching logic')
+    })
+})

frontend/src/scenes/surveys/surveyActivityDescriber.tsx

@@ -9,9 +9,21 @@ import {
     userNameForLogItem,
 } from 'lib/components/ActivityLog/humanizeActivity'
 import { Link } from 'lib/lemon-ui/Link'
+import { truncate } from 'lib/utils'
 import { urls } from 'scenes/urls'
+import { match, P } from 'ts-pattern'
 
-import { Survey, SurveyAppearance } from '~/types'
+import {
+    BasicSurveyQuestion,
+    FeatureFlagBasicType,
+    FeatureFlagFilters,
+    LinkSurveyQuestion,
+    MultipleSurveyQuestion,
+    RatingSurveyQuestion,
+    Survey,
+    SurveyAppearance,
+    SurveyQuestionType,
+} from '~/types'
 
 const isEmptyOrUndefined = (value: any): boolean => value === undefined || value === null || value === ''
 
@@ -31,28 +43,92 @@ const surveyActionsMapping: Record<
     string,
     (change?: ActivityChange, logItem?: ActivityLogItem) => ChangeMapping | null
 > = {
-    name: function onName() {
+    name: function onName(change) {
         return {
-            description: [<>changed the name</>],
+            description: [
+                <>
+                    changed the name from <strong>"{change?.before as string}"</strong> to{' '}
+                    <strong>"{change?.after as string}"</strong>
+                </>,
+            ],
         }
     },
-    description: function onDescription() {
+    description: function onDescription(change) {
         return {
-            description: [<>updated the description</>],
+            description: [
+                <>
+                    updated the description from {formatDescription(change?.before as string | null | undefined)} to{' '}
+                    {formatDescription(change?.after as string | null | undefined)}
+                </>,
+            ],
         }
     },
     type: function onType(change) {
         return {
             description: [
                 <>
-                    changed the type to <strong>{change?.after as string}</strong>
+                    changed the type from <strong>{change?.before as string}</strong> to{' '}
+                    <strong>{change?.after as string}</strong>
                 </>,
             ],
         }
     },
-    questions: function onQuestions() {
+    questions: function onQuestions(change?: ActivityChange): ChangeMapping | null {
+        if (!change) {
+            return null
+        }
+
+        const beforeQuestions = change.before as Survey['questions']
+        const afterQuestions = change.after as Survey['questions']
+
+        if (beforeQuestions.length !== afterQuestions.length) {
+            return {
+                description: [
+                    <>
+                        changed the number of questions from <strong>{beforeQuestions.length}</strong> to{' '}
+                        <strong>{afterQuestions.length}</strong>
+                    </>,
+                ],
+            }
+        }
+
+        const questionChanges = afterQuestions
+            .map((afterQ, index) => {
+                const beforeQ = beforeQuestions[index]
+                if (JSON.stringify(beforeQ) !== JSON.stringify(afterQ)) {
+                    return {
+                        index: index + 1,
+                        changes: describeQuestionChanges(beforeQ, afterQ),
+                    }
+                }
+                return null
+            })
+            .filter((item): item is { index: number; changes: JSX.Element[] } => item !== null)
+
+        if (questionChanges.length === 0) {
+            return {
+                description: [<>No changes to questions</>],
+            }
+        }
+
         return {
-            description: [<>updated the questions</>],
+            description: [
+                <>
+                    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>
+                                    ))}
+                                </ul>
+                            </li>
+                        ))}
+                    </ul>
+                </>,
+            ],
         }
     },
     archived: function onArchived(change) {
@@ -106,35 +182,12 @@ const surveyActionsMapping: Record<
             widgetColor: 'widget color',
         }
 
-        const appearanceFields = Object.keys(fieldNameMapping) as (keyof SurveyAppearance)[]
-
-        appearanceFields.forEach((field) => {
-            const before = beforeAppearance?.[field]
-            const after = afterAppearance?.[field]
-            const readableFieldName = fieldNameMapping[field]
-
-            if (!isEmptyOrUndefined(before) || !isEmptyOrUndefined(after)) {
-                if (isEmptyOrUndefined(before) && !isEmptyOrUndefined(after)) {
-                    changes.push(
-                        <>
-                            set {readableFieldName} to <strong>{String(after)}</strong>
-                        </>
-                    )
-                } else if (!isEmptyOrUndefined(before) && isEmptyOrUndefined(after)) {
-                    changes.push(
-                        <>
-                            removed {readableFieldName} (was <strong>{String(before)}</strong>)
-                        </>
-                    )
-                } else if (before !== after) {
-                    changes.push(
-                        <>
-                            changed {readableFieldName} from{' '}
-                            {!isEmptyOrUndefined(before) ? <strong>{String(before)}</strong> : <i>unset</i>} to{' '}
-                            <strong>{String(after)}</strong>
-                        </>
-                    )
-                }
+        Object.entries(fieldNameMapping).forEach(([field, readableFieldName]) => {
+            const before = beforeAppearance?.[field as keyof SurveyAppearance]
+            const after = afterAppearance?.[field as keyof SurveyAppearance]
+            const changeDescription = describeFieldChange(readableFieldName, before, after)
+            if (changeDescription.props.children) {
+                changes.push(changeDescription)
             }
         })
 
@@ -149,160 +202,124 @@ const surveyActionsMapping: Record<
         const afterConditions = change?.after as Survey['conditions']
         const changes: JSX.Element[] = []
 
-        if (!isEmptyOrUndefined(beforeConditions?.url) || !isEmptyOrUndefined(afterConditions?.url)) {
-            if (isEmptyOrUndefined(beforeConditions?.url) && !isEmptyOrUndefined(afterConditions?.url)) {
-                changes.push(
-                    <>
-                        set URL condition to <strong>{afterConditions?.url}</strong>
-                    </>
-                )
-            } else if (!isEmptyOrUndefined(beforeConditions?.url) && isEmptyOrUndefined(afterConditions?.url)) {
-                changes.push(
-                    <>
-                        removed URL condition (was <strong>{beforeConditions?.url}</strong>)
-                    </>
-                )
-            } else if (beforeConditions?.url !== afterConditions?.url) {
-                changes.push(
-                    <>
-                        changed URL condition from{' '}
-                        {!isEmptyOrUndefined(beforeConditions?.url) ? (
-                            <strong>{beforeConditions?.url}</strong>
-                        ) : (
-                            <i>unset</i>
-                        )}{' '}
-                        to <strong>{afterConditions?.url}</strong>
-                    </>
-                )
-            }
-        }
-
-        if (!isEmptyOrUndefined(beforeConditions?.selector) || !isEmptyOrUndefined(afterConditions?.selector)) {
-            if (isEmptyOrUndefined(beforeConditions?.selector) && !isEmptyOrUndefined(afterConditions?.selector)) {
-                changes.push(
-                    <>
-                        set selector to <strong>{afterConditions?.selector}</strong>
-                    </>
-                )
-            } else if (
-                !isEmptyOrUndefined(beforeConditions?.selector) &&
-                isEmptyOrUndefined(afterConditions?.selector)
-            ) {
-                changes.push(
-                    <>
-                        removed selector (was <strong>{beforeConditions?.selector}</strong>)
-                    </>
-                )
-            } else if (beforeConditions?.selector !== afterConditions?.selector) {
-                changes.push(
-                    <>
-                        changed selector from{' '}
-                        {!isEmptyOrUndefined(beforeConditions?.selector) ? (
-                            <strong>{beforeConditions?.selector}</strong>
-                        ) : (
-                            <i>unset</i>
-                        )}{' '}
-                        to <strong>{afterConditions?.selector}</strong>
-                    </>
-                )
-            }
-        }
-
-        if (
-            !isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) ||
-            !isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays)
-        ) {
-            if (
-                isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) &&
-                !isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays)
-            ) {
-                changes.push(
-                    <>
-                        set wait period to <strong>{afterConditions?.seenSurveyWaitPeriodInDays} days</strong>
-                    </>
-                )
-            } else if (
-                !isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) &&
-                isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays)
-            ) {
-                changes.push(
-                    <>
-                        removed wait period (was <strong>{beforeConditions?.seenSurveyWaitPeriodInDays} days</strong>)
-                    </>
-                )
-            } else if (beforeConditions?.seenSurveyWaitPeriodInDays !== afterConditions?.seenSurveyWaitPeriodInDays) {
-                changes.push(
-                    <>
-                        changed wait period from{' '}
-                        {!isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) ? (
-                            <strong>{beforeConditions?.seenSurveyWaitPeriodInDays} days</strong>
-                        ) : (
-                            <i>unset</i>
-                        )}{' '}
-                        to <strong>{afterConditions?.seenSurveyWaitPeriodInDays} days</strong>
-                    </>
-                )
-            }
-        }
-
-        if (!isEmptyOrUndefined(beforeConditions?.urlMatchType) || !isEmptyOrUndefined(afterConditions?.urlMatchType)) {
-            if (beforeConditions?.urlMatchType !== afterConditions?.urlMatchType) {
-                changes.push(
-                    <>
-                        changed URL match type from{' '}
-                        {!isEmptyOrUndefined(beforeConditions?.urlMatchType) ? (
-                            <strong>{beforeConditions?.urlMatchType}</strong>
-                        ) : (
-                            <i>unset</i>
-                        )}{' '}
-                        to <strong>{afterConditions?.urlMatchType}</strong>
-                    </>
-                )
-            }
-        }
+        changes.push(
+            describeFieldChange('URL condition', beforeConditions?.url, afterConditions?.url),
+            describeFieldChange('selector', beforeConditions?.selector, afterConditions?.selector),
+            describeFieldChange(
+                'wait period',
+                beforeConditions?.seenSurveyWaitPeriodInDays,
+                afterConditions?.seenSurveyWaitPeriodInDays,
+                'days'
+            ),
+            describeFieldChange('URL match type', beforeConditions?.urlMatchType, afterConditions?.urlMatchType)
+        )
 
         // Use JSON.stringify for deep comparison of objects
         if (JSON.stringify(beforeConditions?.events) !== JSON.stringify(afterConditions?.events)) {
             changes.push(<>modified event conditions</>)
         }
 
+        return changes.filter((change) => change.props.children).length > 0
+            ? {
+                  description: changes.filter((change) => change.props.children),
+              }
+            : null
+    },
+    responses_limit: function onResponsesLimit(change) {
+        return {
+            description: [describeFieldChange('response limit', change?.before, change?.after, 'responses')].filter(
+                (desc) => desc.props.children
+            ),
+        }
+    },
+    iteration_count: function onIterationCount(change) {
+        return {
+            description: [describeFieldChange('iteration count', change?.before, change?.after)].filter(
+                (desc) => desc.props.children
+            ),
+        }
+    },
+    iteration_frequency_days: function onIterationFrequency(change) {
+        return {
+            description: [describeFieldChange('iteration frequency', change?.before, change?.after, 'days')].filter(
+                (desc) => desc.props.children
+            ),
+        }
+    },
+    targeting_flag: function onTargetingFlag(change) {
+        const beforeFlag = change?.before as FeatureFlagBasicType | null
+        const afterFlag = change?.after as FeatureFlagBasicType | null
+        const changes: Description[] = []
+
+        if (!beforeFlag && afterFlag) {
+            changes.push(
+                <>
+                    added a targeting flag with key <strong>{afterFlag.key}</strong>
+                </>
+            )
+            if (afterFlag.filters?.groups?.length > 0) {
+                changes.push(
+                    <>
+                        set targeting conditions to:
+                        <pre>{JSON.stringify(afterFlag.filters, null, 2)}</pre>
+                    </>
+                )
+            }
+        } else if (beforeFlag && !afterFlag) {
+            changes.push(
+                <>
+                    removed the targeting flag with key <strong>{beforeFlag.key}</strong>
+                </>
+            )
+        } else if (beforeFlag && afterFlag) {
+            if (beforeFlag.key !== afterFlag.key) {
+                changes.push(
+                    <>
+                        changed targeting flag key from <strong>{beforeFlag.key}</strong> to{' '}
+                        <strong>{afterFlag.key}</strong>
+                    </>
+                )
+            }
+        }
+
         return changes.length > 0
             ? {
                   description: changes,
               }
             : null
     },
-    responses_limit: function onResponsesLimit(change) {
-        if (isEmptyOrUndefined(change?.after)) {
-            return {
-                description: [<>removed response limit</>],
+    targeting_flag_filters: function onTargetingFlagFilter(change) {
+        const beforeFlag = change?.before as FeatureFlagFilters | null
+        const afterFlag = change?.after as FeatureFlagFilters | null
+        const changes: Description[] = []
+
+        if (!beforeFlag && afterFlag) {
+            changes.push(
+                <>
+                    added targeting flag filter with the following conditions:
+                    <pre>{JSON.stringify(afterFlag, null, 2)}</pre>
+                </>
+            )
+        } else if (beforeFlag && !afterFlag) {
+            changes.push(<>removed targeting flag filter</>)
+        } else if (beforeFlag && afterFlag) {
+            if (JSON.stringify(beforeFlag) !== JSON.stringify(afterFlag)) {
+                changes.push(
+                    <>
+                        changed targeting conditions from:
+                        <pre>{JSON.stringify(beforeFlag, null, 2)}</pre>
+                        to:
+                        <pre>{JSON.stringify(afterFlag, null, 2)}</pre>
+                    </>
+                )
             }
         }
-        return {
-            description: [
-                <>
-                    set response limit to <strong>{change?.after as number}</strong>
-                </>,
-            ],
-        }
-    },
-    iteration_count: function onIterationCount(change) {
-        return {
-            description: [
-                <>
-                    changed the iteration count to <strong>{change?.after as number}</strong>
-                </>,
-            ],
-        }
-    },
-    iteration_frequency_days: function onIterationFrequency(change) {
-        return {
-            description: [
-                <>
-                    changed the iteration frequency to <strong>{change?.after as number} days</strong>
-                </>,
-            ],
-        }
+
+        return changes.length > 0
+            ? {
+                  description: changes,
+              }
+            : null
     },
 }
 
@@ -346,7 +363,8 @@ export function surveyActivityDescriber(logItem: ActivityLogItem, asNotification
             const possibleLogItem = surveyActionsMapping[change.field]?.(change, logItem)
             if (possibleLogItem?.description) {
                 if (Array.isArray(possibleLogItem.description) && possibleLogItem.description.length > 1) {
-                    // This is for conditions, which may have multiple changes
+                    // This is for the conditions section, which may have multiple changes.
+                    // Probably could be refactored into a separate handler like some of the other fields
                     changes.push(
                         ...possibleLogItem.description.map((desc) => ({
                             field: 'conditions',
@@ -390,17 +408,20 @@ export function surveyActivityDescriber(logItem: ActivityLogItem, asNotification
 
     return defaultDescriber(logItem, asNotification, surveyLink)
 }
-function getPreposition(field: string): string {
+
+export function getPreposition(field: string): string {
     switch (field) {
-        case 'name':
-        case 'description':
         case 'questions':
         case 'appearance':
         case 'type':
             return 'of'
+        case 'name':
+        case 'description':
         case 'responses_limit':
         case 'iteration_count':
         case 'iteration_frequency_days':
+        case 'targeting_flag_filters':
+        case 'targeting_flag':
             return 'for'
         case 'archived':
         case 'start_date':
@@ -410,3 +431,189 @@ function getPreposition(field: string): string {
             return 'of'
     }
 }
+
+type SurveyQuestion = BasicSurveyQuestion | LinkSurveyQuestion | RatingSurveyQuestion | MultipleSurveyQuestion
+
+export function describeQuestionChanges(before: SurveyQuestion, after: SurveyQuestion): JSX.Element[] {
+    const commonChanges = describeCommonChanges(before, after)
+    const typeChangeDescription =
+        before.type !== after.type
+            ? [
+                  <>
+                      changed question type from <strong>{before.type}</strong> to <strong>{after.type}</strong>
+                  </>,
+              ]
+            : []
+
+    const specificChanges = match([before, after])
+        .with([{ type: SurveyQuestionType.Link }, { type: SurveyQuestionType.Link }], describeLinkChanges)
+        .with([{ type: SurveyQuestionType.Rating }, { type: SurveyQuestionType.Rating }], describeRatingChanges)
+        .with(
+            [
+                { type: P.union(SurveyQuestionType.SingleChoice, SurveyQuestionType.MultipleChoice) },
+                { type: P.union(SurveyQuestionType.SingleChoice, SurveyQuestionType.MultipleChoice) },
+            ],
+            describeMultipleChoiceChanges
+        )
+        .otherwise(() => [])
+
+    return [...commonChanges, ...typeChangeDescription, ...specificChanges, ...describeBranchingChanges(before, after)]
+}
+
+export function describeCommonChanges(before: SurveyQuestion, after: SurveyQuestion): JSX.Element[] {
+    const changes: JSX.Element[] = []
+    if (before.question !== after.question) {
+        changes.push(
+            <>
+                changed question text from "<strong>{before.question}</strong>" to "<strong>{after.question}</strong>"
+            </>
+        )
+    }
+    if (before.description !== after.description) {
+        changes.push(
+            <>
+                changed the question description from {formatDescription(before.description)} to{' '}
+                {formatDescription(after.description)}
+            </>
+        )
+    }
+    if (before.optional !== after.optional) {
+        changes.push(<>{after.optional ? 'made question optional' : 'made question required'}</>)
+    }
+    if (before.buttonText !== after.buttonText) {
+        changes.push(
+            <>
+                changed button text from "<strong>{before.buttonText}</strong>" to "<strong>{after.buttonText}</strong>"
+            </>
+        )
+    }
+    return changes
+}
+
+export function describeLinkChanges([before, after]: [LinkSurveyQuestion, LinkSurveyQuestion]): JSX.Element[] {
+    return before.link !== after.link
+        ? [
+              <>
+                  updated link from <strong>{before.link}</strong> to <strong>{after.link}</strong>
+              </>,
+          ]
+        : []
+}
+
+export function describeRatingChanges([before, after]: [RatingSurveyQuestion, RatingSurveyQuestion]): JSX.Element[] {
+    const changes: JSX.Element[] = []
+    if (before.display !== after.display) {
+        changes.push(
+            <>
+                changed rating display from <strong>{before.display}</strong> to <strong>{after.display}</strong>
+            </>
+        )
+    }
+    if (before.scale !== after.scale) {
+        changes.push(
+            <>
+                changed rating scale from <strong>{before.scale}</strong> to <strong>{after.scale}</strong>
+            </>
+        )
+    }
+    if (before.lowerBoundLabel !== after.lowerBoundLabel || before.upperBoundLabel !== after.upperBoundLabel) {
+        changes.push(
+            <>
+                updated rating labels from <strong>"{before.lowerBoundLabel}"</strong>-
+                <strong>"{before.upperBoundLabel}"</strong> to <strong>"{after.lowerBoundLabel}"</strong>-
+                <strong>"{after.upperBoundLabel}"</strong>
+            </>
+        )
+    }
+    return changes
+}
+
+export function describeMultipleChoiceChanges([before, after]: [
+    MultipleSurveyQuestion,
+    MultipleSurveyQuestion
+]): JSX.Element[] {
+    const changes: JSX.Element[] = []
+    if (JSON.stringify(before.choices) !== JSON.stringify(after.choices)) {
+        const addedChoices = after.choices.filter((c) => !before.choices.includes(c))
+        const removedChoices = before.choices.filter((c) => !after.choices.includes(c))
+        if (addedChoices.length > 0) {
+            changes.push(
+                <>
+                    added choices: <strong>{addedChoices.join(', ')}</strong>
+                </>
+            )
+        }
+        if (removedChoices.length > 0) {
+            changes.push(
+                <>
+                    removed choices: <strong>{removedChoices.join(', ')}</strong>
+                </>
+            )
+        }
+    }
+    if (before.shuffleOptions !== after.shuffleOptions) {
+        changes.push(<>{after.shuffleOptions ? 'enabled' : 'disabled'} option shuffling</>)
+    }
+    if (before.hasOpenChoice !== after.hasOpenChoice) {
+        changes.push(<>{after.hasOpenChoice ? 'added' : 'removed'} open choice option</>)
+    }
+    return changes
+}
+
+export function describeBranchingChanges(before: SurveyQuestion, after: SurveyQuestion): JSX.Element[] {
+    if (JSON.stringify(before.branching) !== JSON.stringify(after.branching)) {
+        return [<>updated branching logic</>]
+    }
+    return []
+}
+
+export const formatDescription = (value: string | null | undefined): JSX.Element => {
+    if (value === undefined || value === null || value === '') {
+        return <i>unset</i>
+    }
+    return <strong>"{truncate(value, 50)}"</strong>
+}
+
+export function describeFieldChange<T>(fieldName: string, before: T, after: T, unit?: string): JSX.Element {
+    if (isEmptyOrUndefined(before) && isEmptyOrUndefined(after)) {
+        return <></>
+    }
+    if (isEmptyOrUndefined(before) && !isEmptyOrUndefined(after)) {
+        return (
+            <>
+                set {fieldName} to{' '}
+                <strong>
+                    {String(after)}
+                    {unit ? ` ${unit}` : ''}
+                </strong>
+            </>
+        )
+    } else if (!isEmptyOrUndefined(before) && isEmptyOrUndefined(after)) {
+        return (
+            <>
+                removed {fieldName} (was{' '}
+                <strong>
+                    {String(before)}
+                    {unit ? ` ${unit}` : ''}
+                </strong>
+                )
+            </>
+        )
+    } else if (before !== after) {
+        return (
+            <>
+                changed {fieldName} from{' '}
+                <strong>
+                    {String(before)}
+                    {unit ? ` ${unit}` : ''}
+                </strong>{' '}
+                to{' '}
+                <strong>
+                    {String(after)}
+                    {unit ? ` ${unit}` : ''}
+                </strong>
+            </>
+        )
+    }
+    return <></>
+}

package.json

@@ -174,6 +174,7 @@
         "sass": "^1.26.2",
         "tailwind-merge": "^2.2.2",
         "tailwindcss": "^3.4.0",
+        "ts-pattern": "4.3",
         "use-debounce": "^9.0.3",
         "use-resize-observer": "^8.0.0",
         "zxcvbn": "^4.4.2"

pnpm-lock.yaml

@@ -343,6 +343,9 @@ dependencies:
   tailwindcss:
     specifier: ^3.4.0
     version: 3.4.0(ts-node@10.9.1)
+  ts-pattern:
+    specifier: '4.3'
+    version: 4.3.0
   use-debounce:
     specifier: ^9.0.3
     version: 9.0.3(react@18.2.0)
@@ -20998,6 +21001,10 @@ packages:
       v8-compile-cache-lib: 3.0.1
       yn: 3.1.1
 
+  /ts-pattern@4.3.0:
+    resolution: {integrity: sha512-pefrkcd4lmIVR0LA49Imjf9DYLK8vtWhqBPA3Ya1ir8xCW0O2yjL9dsCVvI7pCodLC5q7smNpEtDR2yVulQxOg==}
+    dev: false
+
   /tsconfig-paths@3.14.2:
     resolution: {integrity: sha512-o/9iXgCYc5L/JxCHPe3Hvh8Q/2xm5Z+p18PESBU6Ff33695QnCHBEjcytY2q19ua7Mbl/DavtBOLq+oG0RCL+g==}
     dependencies:

posthog/api/survey.py

@@ -26,7 +26,7 @@ from posthog.client import sync_execute
 from posthog.models import Action
 from posthog.constants import AvailableFeature
 from posthog.exceptions import generate_exception_response
-from posthog.models.activity_logging.activity_log import changes_between, load_activity, log_activity, Detail
+from posthog.models.activity_logging.activity_log import Change, changes_between, load_activity, log_activity, Detail
 from posthog.models.activity_logging.activity_page import activity_page_response
 from posthog.models.feature_flag.feature_flag import FeatureFlag
 from posthog.models.feedback.survey import Survey
@@ -317,8 +317,15 @@ class SurveySerializerCreateUpdateOnly(serializers.ModelSerializer):
 
     def update(self, instance: Survey, validated_data):
         before_update = Survey.objects.get(pk=instance.pk)
+        changes = []
         if validated_data.get("remove_targeting_flag"):
             if instance.targeting_flag:
+                # Manually delete the flag and log the change
+                # The `changes_between` method won't catch this because the flag (and underlying ForeignKey relationship)
+                # will have been deleted by the time the `changes_between` method is called, so we need to log the change manually
+                changes.append(
+                    Change(type="Survey", field="targeting_flag", action="deleted", before=instance.targeting_flag)
+                )
                 instance.targeting_flag.delete()
                 validated_data["targeting_flag_id"] = None
             validated_data.pop("remove_targeting_flag")
@@ -331,15 +338,34 @@ class SurveySerializerCreateUpdateOnly(serializers.ModelSerializer):
             new_filters = validated_data["targeting_flag_filters"]
             if instance.targeting_flag:
                 existing_targeting_flag = instance.targeting_flag
+                existing_targeting_flag_filters = existing_targeting_flag.filters
                 serialized_data_filters = {
-                    **existing_targeting_flag.filters,
+                    **existing_targeting_flag_filters,
                     **new_filters,
                 }
+                # Log the existing filter change
+                # The `changes_between` method won't catch this because the flag (and underlying ForeignKey relationship)
+                # will have been deleted by the time the `changes_between` method is called, so we need to log the change manually
+                changes.append(
+                    Change(
+                        type="Survey",
+                        field="targeting_flag_filters",
+                        action="changed",
+                        before=existing_targeting_flag_filters,
+                        after=new_filters,
+                    )
+                )
                 self._create_or_update_targeting_flag(instance.targeting_flag, serialized_data_filters)
             else:
                 new_flag = self._create_or_update_targeting_flag(
                     None, new_filters, instance.name, bool(instance.start_date)
                 )
+                # Log the new filter change
+                # The `changes_between` method won't catch this because the flag (and underlying ForeignKey relationship)
+                # will have been deleted by the time the `changes_between` method is called, so we need to log the change manually
+                changes.append(
+                    Change(type="Survey", field="targeting_flag_filters", action="created", after=new_filters)
+                )
                 validated_data["targeting_flag_id"] = new_flag.id
             validated_data.pop("targeting_flag_filters")
 
@@ -355,7 +381,7 @@ class SurveySerializerCreateUpdateOnly(serializers.ModelSerializer):
         iteration_count = validated_data.get("iteration_count")
         if instance.current_iteration is not None and instance.current_iteration > iteration_count > 0:
             raise serializers.ValidationError(
-                f"Cannot change survey recurrence to {validated_data.get('iteration_count')}, should be at least {instance.current_iteration}"
+                f"Cannot change survey recurrence to {iteration_count}, should be at least {instance.current_iteration}"
             )
 
         instance.iteration_count = iteration_count
@@ -363,10 +389,15 @@ class SurveySerializerCreateUpdateOnly(serializers.ModelSerializer):
 
         instance = super().update(instance, validated_data)
 
-        self._add_user_survey_interacted_filters(instance, end_date)
-        self._associate_actions(instance, validated_data.get("conditions"))
         team = Team.objects.get(id=self.context["team_id"])
-        changes = changes_between("Survey", previous=before_update, current=instance)
+        # `changes_between` will not catch changes to the ForeignKey relationships
+        # so it's useful for any changes to the Survey model itself, but not for the related models
+        non_foreign_table_relation_changes = changes_between(
+            "Survey",
+            previous=before_update,
+            current=instance,
+        )
+        changes.extend(non_foreign_table_relation_changes)
         log_activity(
             organization_id=team.organization_id,
             team_id=self.context["team_id"],
@@ -377,6 +408,9 @@ class SurveySerializerCreateUpdateOnly(serializers.ModelSerializer):
             activity="updated",
             detail=Detail(changes=changes, name=instance.name),
         )
+
+        self._add_user_survey_interacted_filters(instance, end_date)
+        self._associate_actions(instance, validated_data.get("conditions"))
         return instance
 
     def _associate_actions(self, instance: Survey, conditions):

posthog/api/test/test_survey.py

@@ -1,10 +1,12 @@
 import re
 from datetime import datetime, timedelta
+from typing import Any
 from unittest.mock import ANY
 
 import pytest
 from django.core.cache import cache
 from django.test.client import Client
+
 from freezegun.api import freeze_time
 from posthog.api.survey import nh3_clean_with_allow_list
 from posthog.models.cohort.cohort import Cohort
@@ -1169,6 +1171,70 @@ class TestSurvey(APIBaseTest):
         self.team.refresh_from_db()
         assert self.team.surveys_opt_in is False
 
+    @freeze_time("2023-05-01 12:00:00")
+    def test_update_survey_targeting_flag_filters_records_activity(self):
+        linked_flag = FeatureFlag.objects.create(team=self.team, key="linked-flag", created_by=self.user)
+        targeting_flag = FeatureFlag.objects.create(team=self.team, key="targeting-flag", created_by=self.user)
+        internal_targeting_flag = FeatureFlag.objects.create(
+            team=self.team, key="custom-targeting-flag", created_by=self.user
+        )
+
+        survey_with_flags = Survey.objects.create(
+            team=self.team,
+            created_by=self.user,
+            name="Survey 2",
+            type="popover",
+            linked_flag=linked_flag,
+            targeting_flag=targeting_flag,
+            internal_targeting_flag=internal_targeting_flag,
+            questions=[{"type": "open", "question": "What's a hedgehog?"}],
+        )
+
+        new_filters: dict[str, Any] = {
+            "targeting_flag_filters": {
+                "groups": [
+                    {"variant": None, "properties": [], "rollout_percentage": 69},
+                    {"variant": None, "properties": [], "rollout_percentage": 75},
+                ],
+                "payloads": {},
+                "multivariate": None,
+            }
+        }
+
+        response = self.client.patch(
+            f"/api/projects/{self.team.id}/surveys/{survey_with_flags.id}/",
+            data={"targeting_flag_filters": new_filters},
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_200_OK)
+
+        expected_activity_log = [
+            {
+                "user": {"first_name": self.user.first_name, "email": self.user.email},
+                "activity": "updated",
+                "scope": "Survey",
+                "item_id": str(survey_with_flags.id),
+                "detail": {
+                    "changes": [
+                        {
+                            "type": "Survey",
+                            "action": "changed",
+                            "field": "targeting_flag_filters",
+                            "before": {},
+                            "after": new_filters,
+                        },
+                    ],
+                    "trigger": None,
+                    "name": "Survey 2",
+                    "short_id": None,
+                    "type": None,
+                },
+                "created_at": "2023-05-01T12:00:00Z",
+            }
+        ]
+
+        self._assert_survey_activity(expected_activity_log)
+
     @freeze_time("2023-05-01 12:00:00")
     def test_create_survey_records_activity(self):
         response = self.client.post(

posthog/models/activity_logging/activity_log.py

@@ -6,12 +6,15 @@ from typing import Any, Literal, Optional, Union
 
 import structlog
 from django.core.paginator import Paginator
+from django.core.exceptions import ObjectDoesNotExist
+
 from django.db import models
 from django.utils import timezone
 from django.conf import settings
 
 from posthog.models.dashboard import Dashboard
 from posthog.models.dashboard_tile import DashboardTile
+from posthog.models.feature_flag.feature_flag import FeatureFlag
 from posthog.models.user import User
 from posthog.models.utils import UUIDT, UUIDModel
 
@@ -84,6 +87,16 @@ class ActivityDetailEncoder(json.JSONEncoder):
         if isinstance(obj, Decimal):
             # more precision than we'll need but avoids rounding too unnecessarily
             return format(obj, ".6f").rstrip("0").rstrip(".")
+        if isinstance(obj, FeatureFlag):
+            return {
+                "id": obj.id,
+                "key": obj.key,
+                "name": obj.name,
+                "filters": obj.filters,
+                "team_id": obj.team_id,
+                "deleted": obj.deleted,
+                "active": obj.active,
+            }
 
         return json.JSONEncoder.default(self, obj)
 
@@ -206,9 +219,6 @@ field_exclusions: dict[ActivityScope, list[str]] = {
         "property_type_format",
     ],
     "Team": ["uuid", "updated_at", "api_token", "created_at", "id"],
-    # TODO: Don't try and track changes to survey targeting, we will support
-    # this with https://github.com/PostHog/posthog/issues/23725
-    "Survey": ["targeting_flag", "linked_flag", "internal_targeting_flag"],
 }
 
 
@@ -236,18 +246,36 @@ def _read_through_relation(relation: models.Manager) -> list[Union[dict, str]]:
     return described_models
 
 
+def safely_get_field_value(instance: models.Model | None, field: str):
+    """Helper function to get the value of a field, handling related objects and exceptions."""
+    if instance is None:
+        return None
+    try:
+        value = getattr(instance, field, None)
+        if isinstance(value, models.Manager):
+            value = _read_through_relation(value)
+    # If the field is a related field and the related object has been deleted, this will raise an ObjectDoesNotExist
+    # exception. We catch this exception and return None, since the related object has been deleted, and we
+    # don't need any additional information about it other than the fact that it was deleted.
+    except ObjectDoesNotExist:
+        value = None
+    return value
+
+
 def changes_between(
     model_type: ActivityScope,
     previous: Optional[models.Model],
     current: Optional[models.Model],
 ) -> list[Change]:
     """
-    Identifies changes between two models by comparing fields
+    Identifies changes between two models by comparing fields.
+    Note that this method only really works for models that have a single instance
+    and not for models that have a many-to-many relationship with another model.
     """
     changes: list[Change] = []
 
     if previous is None and current is None:
-        # there are no changes between two things that don't exist
+        # There are no changes between two things that don't exist.
         return changes
 
     if previous is not None:
@@ -256,23 +284,18 @@ def changes_between(
         filtered_fields = [f.name for f in fields if f.name not in excluded_fields]
 
         for field in filtered_fields:
-            left = getattr(previous, field, None)
-            if isinstance(left, models.Manager):
-                left = _read_through_relation(left)
-
-            right = getattr(current, field, None)
-            if isinstance(right, models.Manager):
-                right = _read_through_relation(right)
+            left = safely_get_field_value(previous, field)
+            right = safely_get_field_value(current, field)
 
             if field == "tagged_items":
-                field = "tags"  # or the UI needs to be coupled to this internal backend naming
+                field = "tags"  # Or the UI needs to be coupled to this internal backend naming.
 
             if field == "dashboards" and "dashboard_tiles" in filtered_fields:
-                # only process dashboard_tiles when it is present. It supersedes dashboards
+                # Only process dashboard_tiles when it is present. It supersedes dashboards.
                 continue
 
             if model_type == "Insight" and field == "dashboard_tiles":
-                # the api exposes this as dashboards and that's what the activity describers expect
+                # The API exposes this as dashboards and that's what the activity describers expect.
                 field = "dashboards"
 
             if left is None and right is not None: