diff --git a/frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-edit--light.png b/frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-edit--light.png index 4ca21748c10..01df2b71a3a 100644 Binary files a/frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-edit--light.png and b/frontend/__snapshots__/scenes-app-insights--funnel-top-to-bottom-edit--light.png differ diff --git a/frontend/src/scenes/feature-flags/activityDescriptions.tsx b/frontend/src/scenes/feature-flags/activityDescriptions.tsx index 9027d17cf07..93fec0692b0 100644 --- a/frontend/src/scenes/feature-flags/activityDescriptions.tsx +++ b/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: ( + created a new feature flag:]} + prefix={ + <> + {userNameForLogItem(logItem)} + + } + suffix={<> {nameOrLinkToFlag(logItem?.item_id, logItem?.detail.name)}} + /> + ), + } + } + if (logItem.activity == 'updated') { let changes: Description[] = [] let changeSuffix: Description = ( diff --git a/frontend/src/scenes/surveys/surveyActivityDescriber.test.tsx b/frontend/src/scenes/surveys/surveyActivityDescriber.test.tsx new file mode 100644 index 00000000000..cc9dca12c7a --- /dev/null +++ b/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') + }) +}) diff --git a/frontend/src/scenes/surveys/surveyActivityDescriber.tsx b/frontend/src/scenes/surveys/surveyActivityDescriber.tsx index 3e692fcceec..4a355ca7b2e 100644 --- a/frontend/src/scenes/surveys/surveyActivityDescriber.tsx +++ b/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 "{change?.before as string}" to{' '} + "{change?.after as string}" + , + ], } }, - 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 {change?.after as string} + changed the type from {change?.before as string} to{' '} + {change?.after as string} , ], } }, - 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 {beforeQuestions.length} to{' '} + {afterQuestions.length} + , + ], + } + } + + 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 {questionChanges.length} question{questionChanges.length !== 1 ? 's' : ''}: + + , + ], } }, 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 {String(after)} - - ) - } else if (!isEmptyOrUndefined(before) && isEmptyOrUndefined(after)) { - changes.push( - <> - removed {readableFieldName} (was {String(before)}) - - ) - } else if (before !== after) { - changes.push( - <> - changed {readableFieldName} from{' '} - {!isEmptyOrUndefined(before) ? {String(before)} : unset} to{' '} - {String(after)} - - ) - } + 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 {afterConditions?.url} - - ) - } else if (!isEmptyOrUndefined(beforeConditions?.url) && isEmptyOrUndefined(afterConditions?.url)) { - changes.push( - <> - removed URL condition (was {beforeConditions?.url}) - - ) - } else if (beforeConditions?.url !== afterConditions?.url) { - changes.push( - <> - changed URL condition from{' '} - {!isEmptyOrUndefined(beforeConditions?.url) ? ( - {beforeConditions?.url} - ) : ( - unset - )}{' '} - to {afterConditions?.url} - - ) - } - } - - if (!isEmptyOrUndefined(beforeConditions?.selector) || !isEmptyOrUndefined(afterConditions?.selector)) { - if (isEmptyOrUndefined(beforeConditions?.selector) && !isEmptyOrUndefined(afterConditions?.selector)) { - changes.push( - <> - set selector to {afterConditions?.selector} - - ) - } else if ( - !isEmptyOrUndefined(beforeConditions?.selector) && - isEmptyOrUndefined(afterConditions?.selector) - ) { - changes.push( - <> - removed selector (was {beforeConditions?.selector}) - - ) - } else if (beforeConditions?.selector !== afterConditions?.selector) { - changes.push( - <> - changed selector from{' '} - {!isEmptyOrUndefined(beforeConditions?.selector) ? ( - {beforeConditions?.selector} - ) : ( - unset - )}{' '} - to {afterConditions?.selector} - - ) - } - } - - if ( - !isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) || - !isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays) - ) { - if ( - isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) && - !isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays) - ) { - changes.push( - <> - set wait period to {afterConditions?.seenSurveyWaitPeriodInDays} days - - ) - } else if ( - !isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) && - isEmptyOrUndefined(afterConditions?.seenSurveyWaitPeriodInDays) - ) { - changes.push( - <> - removed wait period (was {beforeConditions?.seenSurveyWaitPeriodInDays} days) - - ) - } else if (beforeConditions?.seenSurveyWaitPeriodInDays !== afterConditions?.seenSurveyWaitPeriodInDays) { - changes.push( - <> - changed wait period from{' '} - {!isEmptyOrUndefined(beforeConditions?.seenSurveyWaitPeriodInDays) ? ( - {beforeConditions?.seenSurveyWaitPeriodInDays} days - ) : ( - unset - )}{' '} - to {afterConditions?.seenSurveyWaitPeriodInDays} days - - ) - } - } - - if (!isEmptyOrUndefined(beforeConditions?.urlMatchType) || !isEmptyOrUndefined(afterConditions?.urlMatchType)) { - if (beforeConditions?.urlMatchType !== afterConditions?.urlMatchType) { - changes.push( - <> - changed URL match type from{' '} - {!isEmptyOrUndefined(beforeConditions?.urlMatchType) ? ( - {beforeConditions?.urlMatchType} - ) : ( - unset - )}{' '} - to {afterConditions?.urlMatchType} - - ) - } - } + 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 {afterFlag.key} + + ) + if (afterFlag.filters?.groups?.length > 0) { + changes.push( + <> + set targeting conditions to: +
{JSON.stringify(afterFlag.filters, null, 2)}
+ + ) + } + } else if (beforeFlag && !afterFlag) { + changes.push( + <> + removed the targeting flag with key {beforeFlag.key} + + ) + } else if (beforeFlag && afterFlag) { + if (beforeFlag.key !== afterFlag.key) { + changes.push( + <> + changed targeting flag key from {beforeFlag.key} to{' '} + {afterFlag.key} + + ) + } + } + 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: +
{JSON.stringify(afterFlag, null, 2)}
+ + ) + } 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: +
{JSON.stringify(beforeFlag, null, 2)}
+ to: +
{JSON.stringify(afterFlag, null, 2)}
+ + ) } } - return { - description: [ - <> - set response limit to {change?.after as number} - , - ], - } - }, - iteration_count: function onIterationCount(change) { - return { - description: [ - <> - changed the iteration count to {change?.after as number} - , - ], - } - }, - iteration_frequency_days: function onIterationFrequency(change) { - return { - description: [ - <> - changed the iteration frequency to {change?.after as number} days - , - ], - } + + 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 {before.type} to {after.type} + , + ] + : [] + + 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 "{before.question}" to "{after.question}" + + ) + } + 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 "{before.buttonText}" to "{after.buttonText}" + + ) + } + return changes +} + +export function describeLinkChanges([before, after]: [LinkSurveyQuestion, LinkSurveyQuestion]): JSX.Element[] { + return before.link !== after.link + ? [ + <> + updated link from {before.link} to {after.link} + , + ] + : [] +} + +export function describeRatingChanges([before, after]: [RatingSurveyQuestion, RatingSurveyQuestion]): JSX.Element[] { + const changes: JSX.Element[] = [] + if (before.display !== after.display) { + changes.push( + <> + changed rating display from {before.display} to {after.display} + + ) + } + if (before.scale !== after.scale) { + changes.push( + <> + changed rating scale from {before.scale} to {after.scale} + + ) + } + if (before.lowerBoundLabel !== after.lowerBoundLabel || before.upperBoundLabel !== after.upperBoundLabel) { + changes.push( + <> + updated rating labels from "{before.lowerBoundLabel}"- + "{before.upperBoundLabel}" to "{after.lowerBoundLabel}"- + "{after.upperBoundLabel}" + + ) + } + 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: {addedChoices.join(', ')} + + ) + } + if (removedChoices.length > 0) { + changes.push( + <> + removed choices: {removedChoices.join(', ')} + + ) + } + } + 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 unset + } + return "{truncate(value, 50)}" +} + +export function describeFieldChange(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{' '} + + {String(after)} + {unit ? ` ${unit}` : ''} + + + ) + } else if (!isEmptyOrUndefined(before) && isEmptyOrUndefined(after)) { + return ( + <> + removed {fieldName} (was{' '} + + {String(before)} + {unit ? ` ${unit}` : ''} + + ) + + ) + } else if (before !== after) { + return ( + <> + changed {fieldName} from{' '} + + {String(before)} + {unit ? ` ${unit}` : ''} + {' '} + to{' '} + + {String(after)} + {unit ? ` ${unit}` : ''} + + + ) + } + return <> +} diff --git a/package.json b/package.json index a37209b6912..7f9acf3c701 100644 --- a/package.json +++ b/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" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a4b2e230c95..33c3d912229 100644 --- a/pnpm-lock.yaml +++ b/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: diff --git a/posthog/api/survey.py b/posthog/api/survey.py index 640ed75aaa3..8c18fe61700 100644 --- a/posthog/api/survey.py +++ b/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): diff --git a/posthog/api/test/test_survey.py b/posthog/api/test/test_survey.py index 05b44a62a14..6fd90dfbfb2 100644 --- a/posthog/api/test/test_survey.py +++ b/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( diff --git a/posthog/models/activity_logging/activity_log.py b/posthog/models/activity_logging/activity_log.py index 377ddaff7dc..1fb332e2e9e 100644 --- a/posthog/models/activity_logging/activity_log.py +++ b/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: