diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index dde01266e3b8..f8dee3a87451 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -20,6 +20,10 @@ export interface GitHubPagedResponse extends PagedResponse { } export type FlagsmithValue = string | number | boolean | null +// A value shown in a diff. JSON fields, e.g. segment `rules_data`, arrive as +// arrays or objects. +export type DiffValue = FlagsmithValue | unknown[] | Record + export type EnvironmentMetrics = { id: number name: string @@ -565,8 +569,8 @@ export type AuditLogItem = { export type AuditLogDetail = AuditLogItem & { change_details: { field: string - old: FlagsmithValue - new: FlagsmithValue + old: DiffValue + new: DiffValue }[] } export type PaymentMethod = 'CHARGEBEE' | 'XERO' | 'AWS_MARKETPLACE' diff --git a/frontend/common/utils/__tests__/utils.test.ts b/frontend/common/utils/__tests__/utils.test.ts index ff9527edab7f..810d83d20db6 100644 --- a/frontend/common/utils/__tests__/utils.test.ts +++ b/frontend/common/utils/__tests__/utils.test.ts @@ -120,3 +120,34 @@ describe('getPlanPermission', () => { }, ) }) + +describe('sanitiseDiffString', () => { + it.each([ + [null, ''], + [undefined, ''], + ['', '""'], + ['small', 'small'], + [0, '0'], + [false, 'false'], + ])('formats %p as %p', (value, expected) => { + expect(Utils.sanitiseDiffString(value)).toBe(expected) + }) + + it('formats an array of objects as indented JSON', () => { + const rulesData = [ + { + conditions: [{ operator: 'EQUAL', property: 'age', value: '19' }], + rules: [], + type: 'ALL', + }, + ] + + expect(Utils.sanitiseDiffString(rulesData)).toBe( + JSON.stringify(rulesData, null, 2), + ) + }) + + it('formats an object as indented JSON', () => { + expect(Utils.sanitiseDiffString({ a: 1 })).toBe('{\n "a": 1\n}') + }) +}) diff --git a/frontend/common/utils/utils.tsx b/frontend/common/utils/utils.tsx index 1609b6ddf716..bbab9e0cabe2 100644 --- a/frontend/common/utils/utils.tsx +++ b/frontend/common/utils/utils.tsx @@ -3,6 +3,7 @@ import ProjectStore from 'common/stores/project-store' import Project from 'common/project' import { ContentType, + DiffValue, FeatureState, FlagsmithValue, MultivariateFeatureStateValue, @@ -721,10 +722,16 @@ const Utils = Object.assign({}, BaseUtils, { ) }, - sanitiseDiffString: (value: FlagsmithValue) => { + sanitiseDiffString: (value: DiffValue | undefined): string => { if (value === undefined || value === null) { return '' } + if (value === '') { + return '""' + } + if (typeof value === 'object') { + return JSON.stringify(value, null, 2) + } return `${value}` }, diff --git a/frontend/e2e/helpers/e2e-helpers.playwright.ts b/frontend/e2e/helpers/e2e-helpers.playwright.ts index 3f2d5e132c95..24e276d4e163 100644 --- a/frontend/e2e/helpers/e2e-helpers.playwright.ts +++ b/frontend/e2e/helpers/e2e-helpers.playwright.ts @@ -817,17 +817,45 @@ export class E2EHelpers { // Use .first() to handle cases where multiple comparison modals might exist in DOM await expect(this.page.locator(byId('old-enabled')).first()).toHaveText(`${oldEnabled}`); await expect(this.page.locator(byId('new-enabled')).first()).toHaveText(`${newEnabled}`); - if (oldValue !== undefined) { - const expectedOldValue = oldValue === null ? '' : `${oldValue}`; - await expect(this.page.locator(byId('old-value')).first()).toHaveText(expectedOldValue); - } - if (newValue !== undefined) { - const expectedNewValue = newValue === null ? '' : `${newValue}`; - await expect(this.page.locator(byId('new-value')).first()).toHaveText(expectedNewValue); + if (oldValue !== undefined && newValue !== undefined) { + await this.assertDiffString( + // The value diff can sit in an inactive tab, e.g. when only variations changed. + this.page + .getByRole('group', { includeHidden: true, name: 'Value diff' }) + .first(), + oldValue, + newValue, + ); } await this.closeModal(); } + // Assert what a DiffString renders. Changed values are shown by + // react-diff-viewer as a removed (`-`) line and an added (`+`) line. + async assertDiffString(diff: Locator, oldValue: any, newValue: any) { + const toText = (value: any) => (value === null ? '' : `${value}`); + const oldText = toText(oldValue); + const newText = toText(newValue); + + if (oldText === newText) { + await expect(diff).toHaveText(oldText === '' ? 'No Value.' : oldText); + return; + } + + const removedLines = diff.locator('tr:has(td pre:text-is("-")) td:last-child'); + const addedLines = diff.locator('tr:has(td pre:text-is("+")) td:last-child'); + if (oldText === '') { + await expect(removedLines).toHaveCount(0); + } else { + await expect(removedLines).toHaveText([oldText]); + } + if (newText === '') { + await expect(addedLines).toHaveCount(0); + } else { + await expect(addedLines).toHaveText([newText]); + } + } + // Assert number of versions async assertNumberOfVersions(featureName: string, versions: number) { await this.goToFeatureVersions(featureName); diff --git a/frontend/e2e/tests/segment-test.pw.ts b/frontend/e2e/tests/segment-test.pw.ts index 810f7ea72d00..cccb3555d525 100644 --- a/frontend/e2e/tests/segment-test.pw.ts +++ b/frontend/e2e/tests/segment-test.pw.ts @@ -475,3 +475,45 @@ test('Segment test 4 - Create ANY rule type segment and verify match changes whe await deleteTrait('age_any') await deleteTrait('team') }) + +test('Segment test 5 - Audit log shows segment rule changes @enterprise', async ({ page }) => { + const { + click, + createSegment, + deleteSegment, + gotoProject, + gotoSegments, + login, + navigateToSegment, + setSegmentRule, + waitForElementVisible, + waitForToast, + } = createHelpers(page) + const segmentName = 'audit_log_segment' + + log('Login') + await login(E2E_USER, PASSWORD) + await gotoProject(E2E_SEGMENT_PROJECT_3) + await waitForElementVisible(byId('features-page')) + + log('Create and update segment') + await gotoSegments() + await createSegment(segmentName, [{ name: 'age', operator: 'EQUAL', value: 18 }]) + await navigateToSegment(segmentName) + await setSegmentRule(0, 0, 'age', 'EQUAL', 19) + await click(byId('update-segment')) + await waitForToast() + + log('Check audit log shows the segment rule change') + await click(byId('audit-log-link')) + await click(`text="Segment updated: ${segmentName}"`) + const rulesDataDiff = page + .locator('.list-item', { hasText: 'Rules data' }) + .getByRole('group', { name: 'Value diff' }) + await expect(rulesDataDiff).toContainText('19') + await expect(rulesDataDiff).not.toContainText('[object Object]') + + log('Clean up segment') + await gotoSegments() + await deleteSegment(segmentName) +}) diff --git a/frontend/web/components/diff/DiffString.tsx b/frontend/web/components/diff/DiffString.tsx index 0042b10d60d6..eefe3281768d 100644 --- a/frontend/web/components/diff/DiffString.tsx +++ b/frontend/web/components/diff/DiffString.tsx @@ -2,39 +2,24 @@ import React, { FC } from 'react' import Diff, { DiffMethod } from 'react-diff-viewer-continued' import Prism from 'prismjs' import 'prismjs/components/prism-json' -import { FlagsmithValue } from 'common/types/responses' +import { DiffValue } from 'common/types/responses' +import Utils from 'common/utils/utils' import DiffRow from './DiffRow' type DiffType = { - oldValue: FlagsmithValue - newValue: FlagsmithValue + oldValue: DiffValue + newValue: DiffValue compareMethod?: DiffMethod } -const sanitiseDiffString = (value: FlagsmithValue) => { - if (value === undefined || value === null) { - return '' - } - if (value === '') { - return '""' - } - return `${value}` -} -const DiffString: FC = ({ - compareMethod = DiffMethod.WORDS_WITH_SPACE, - newValue, - oldValue, -}) => { - if (E2E) { - return ( - <> -
{oldValue}
-
{newValue}
- - ) - } - - if (oldValue === newValue) { +const renderDiff = ( + oldValue: DiffValue, + newValue: DiffValue, + compareMethod: DiffMethod, +) => { + const oldText = Utils.sanitiseDiffString(oldValue) + const newText = Utils.sanitiseDiffString(newValue) + if (oldText === newText) { if (oldValue === null || oldValue === '') { return
No Value.
} @@ -43,11 +28,7 @@ const DiffString: FC = ({
       
@@ -55,12 +36,10 @@ const DiffString: FC = ({
   }
   return (
      (
         
 = ({
   )
 }
 
+const DiffString: FC = ({
+  compareMethod = DiffMethod.WORDS_WITH_SPACE,
+  newValue,
+  oldValue,
+}) => (
+  
+ {renderDiff(oldValue, newValue, compareMethod)} +
+) + export default DiffString