fix(Diff display): Segment rules diff showing up as [Object object]

frontend/common/types/responses.ts

@@ -20,6 +20,10 @@ export interface GitHubPagedResponse<T> extends PagedResponse<T> {
 }
 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<string, unknown>
+
 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'

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}')
+  })
+})

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}`
   },
 

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);

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)
+})

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<DiffType> = ({
-  compareMethod = DiffMethod.WORDS_WITH_SPACE,
-  newValue,
-  oldValue,
-}) => {
-  if (E2E) {
-    return (
-      <>
-        <div data-test={'old-value'}>{oldValue}</div>
-        <div data-test={'new-value'}>{newValue}</div>
-      </>
-    )
-  }
-
-  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 <div className='text-muted'>No Value.</div>
     }
@@ -43,24 +28,18 @@ const DiffString: FC<DiffType> = ({
         <pre
           className='d-inline mb-0'
           dangerouslySetInnerHTML={{
-            __html: Prism.highlight(
-              sanitiseDiffString(newValue),
-              Prism.languages.json,
-              'json',
-            ),
+            __html: Prism.highlight(newText, Prism.languages.json, 'json'),
           }}
         />
       </DiffRow>
     )
   }
   return (
     <Diff
-      oldValue={sanitiseDiffString(oldValue)}
-      newValue={sanitiseDiffString(newValue)}
+      oldValue={oldText}
+      newValue={newText}
       compareMethod={compareMethod}
-      hideLineNumbers={
-        !(`${oldValue}`.includes?.('\n') || `${newValue}`.includes?.('\n'))
-      }
+      hideLineNumbers={!(oldText.includes('\n') || newText.includes('\n'))}
       renderContent={(str) => (
         <pre
           className='d-inline'
@@ -74,4 +53,14 @@ const DiffString: FC<DiffType> = ({
   )
 }
 
+const DiffString: FC<DiffType> = ({
+  compareMethod = DiffMethod.WORDS_WITH_SPACE,
+  newValue,
+  oldValue,
+}) => (
+  <div role='group' aria-label='Value diff'>
+    {renderDiff(oldValue, newValue, compareMethod)}
+  </div>
+)
+
 export default DiffString