diff --git a/frontend/common/utils/__tests__/copyToClipboard.test.ts b/frontend/common/utils/__tests__/copyToClipboard.test.ts new file mode 100644 index 000000000000..f4d77c6ff72b --- /dev/null +++ b/frontend/common/utils/__tests__/copyToClipboard.test.ts @@ -0,0 +1,47 @@ +import { copyToClipboard } from 'common/utils/copyToClipboard' + +const writeText = jest.fn() +const toast = jest.fn() + +beforeEach(() => { + writeText.mockReset().mockResolvedValue(undefined) + toast.mockReset() + ;(global as any).toast = toast + Object.defineProperty(global, 'navigator', { + configurable: true, + value: { clipboard: { writeText } }, + writable: true, + }) +}) + +describe('copyToClipboard', () => { + it('writes the value and toasts the default success message', async () => { + await copyToClipboard('DEFAULT_VALUE') + + expect(writeText).toHaveBeenCalledWith('DEFAULT_VALUE') + expect(toast).toHaveBeenCalledWith('Copied to clipboard') + }) + + it('toasts a caller-supplied success message instead', async () => { + await copyToClipboard('prompt', 'Cleanup prompt copied to clipboard') + + expect(toast).toHaveBeenCalledWith('Cleanup prompt copied to clipboard') + }) + + it('toasts the failure and rethrows when the write is rejected', async () => { + const error = new Error('denied') + writeText.mockRejectedValue(error) + + await expect(copyToClipboard('value')).rejects.toThrow(error) + expect(toast).toHaveBeenCalledWith('Failed to copy to clipboard') + }) + + it('toasts a caller-supplied failure message instead', async () => { + writeText.mockRejectedValue(new Error('denied')) + + await expect( + copyToClipboard('value', undefined, 'Could not copy the value'), + ).rejects.toThrow() + expect(toast).toHaveBeenCalledWith('Could not copy the value') + }) +}) diff --git a/frontend/common/utils/copyToClipboard.ts b/frontend/common/utils/copyToClipboard.ts new file mode 100644 index 000000000000..7a9bf4f96591 --- /dev/null +++ b/frontend/common/utils/copyToClipboard.ts @@ -0,0 +1,21 @@ +/** + * Write `value` to the clipboard and toast the outcome. + * + * Rethrows after toasting so callers that need to react to a failure can, + * but the toast means most callers do not have to. + */ +export const copyToClipboard = async ( + value: string, + successMessage?: string, + errorMessage?: string, +) => { + try { + await navigator.clipboard.writeText(value) + toast(successMessage ?? 'Copied to clipboard') + } catch (error) { + toast(errorMessage ?? 'Failed to copy to clipboard') + throw error + } +} + +export default copyToClipboard diff --git a/frontend/common/utils/utils.tsx b/frontend/common/utils/utils.tsx index 2c545577bbdf..a7718160284e 100644 --- a/frontend/common/utils/utils.tsx +++ b/frontend/common/utils/utils.tsx @@ -71,6 +71,7 @@ export const planNames = { startup: 'Startup', } import BaseUtils from './base/_utils' +import { copyToClipboard } from './copyToClipboard' const Utils = Object.assign({}, BaseUtils, { appendImage: (src: string) => { const img = document.createElement('img') @@ -148,19 +149,7 @@ const Utils = Object.assign({}, BaseUtils, { return res }, - copyToClipboard: async ( - value: string, - successMessage?: string, - errorMessage?: string, - ) => { - try { - await navigator.clipboard.writeText(value) - toast(successMessage ?? 'Copied to clipboard') - } catch (error) { - toast(errorMessage ?? 'Failed to copy to clipboard') - throw error - } - }, + copyToClipboard, displayLimitAlert(type: string, percentage: number | undefined) { const envOrProject = diff --git a/frontend/documentation/components/ValueEditor.stories.tsx b/frontend/documentation/components/ValueEditor.stories.tsx index f58af691f279..483d7f62c8c6 100644 --- a/frontend/documentation/components/ValueEditor.stories.tsx +++ b/frontend/documentation/components/ValueEditor.stories.tsx @@ -1,9 +1,9 @@ -import React, { useState } from 'react' +import React, { useEffect, useState } from 'react' import type { Meta, StoryObj } from 'storybook' -import ValueEditor from 'components/ValueEditor' -import FieldLabel from 'components/base/forms/FieldLabel' import Constants from 'common/constants' +import ValueEditor, { ValueEditorProps } from 'components/ValueEditor' +import ControlWeightChip from 'components/mv/ControlWeightChip' const meta: Meta = { parameters: { chromatic: { disableSnapshot: false } }, @@ -13,23 +13,27 @@ export default meta type Story = StoryObj -const DEFAULT_TOOLTIP = Constants.strings.REMOTE_CONFIG_DESCRIPTION +// The real props, so a story cannot drift from the component's contract. +type InteractiveProps = Omit & { + initialValue?: string + width?: number +} const Interactive = ({ initialValue = '', - label, - tooltip = DEFAULT_TOOLTIP, + width = 640, ...props -}: Record) => { +}: InteractiveProps) => { const [value, setValue] = useState(initialValue) return ( -
- {label && {label}} +
) } +// Empty state. The "Enter a value..." text is not a real ::placeholder — it is +// rendered into the contenteditable and styled by `code.txt.empty`. export const Default: Story = { render: () => , } @@ -53,23 +57,49 @@ export const Json: Story = { render: () => ( ), } +// A value that arrives after mount, the way a loaded feature does. Detection +// has to wait for it: a mount-only check left JSON rendering as .txt. +const LateLoading = () => { + const [value, setValue] = useState('') + useEffect(() => { + const timer = setTimeout(() => setValue('{ "colour": "blue" }'), 150) + return () => clearTimeout(timer) + }, []) + return ( +
+ +
+ ) +} + +export const ValueArrivesAfterMount: Story = { + render: () => , +} + +// The danger tone, which no other story shows. The format has to be chosen +// rather than pinned: a pinned editor has no format row to render the warning +// against. export const InvalidJson: Story = { - render: () => ( - - ), + play: async ({ canvasElement }: { canvasElement: HTMLElement }) => { + const buttons = canvasElement.querySelectorAll('.select-language button') + const json = Array.from(buttons).find( + (button) => button.textContent?.trim() === '.json', + ) + ;(json as HTMLButtonElement | undefined)?.click() + }, + render: () => , } export const CodeMedium: Story = { render: () => ( @@ -82,13 +112,43 @@ export const Disabled: Story = { ), } -export const OnlyOneLang: Story = { +// A pinned format hides the row, since there is nothing to switch to. This is +// what the SAML IdP metadata field renders. +export const XmlOnly: Story = { render: () => ( '} /> ), } + +// The multivariate control value carries a weight chip and a tooltip, so it is +// the widest label this component gets. Label and format buttons share one flex +// row, so they compress rather than overlap. +const controlWeight = + +export const BadgeLabel: Story = { + render: () => ( + + ), +} + +// The same label at the narrowest width the drawer reaches. +export const BadgeLabelNarrow: Story = { + render: () => ( + + ), +} diff --git a/frontend/e2e/helpers/e2e-helpers.playwright.ts b/frontend/e2e/helpers/e2e-helpers.playwright.ts index 85ded873745a..df5e4bb9b402 100644 --- a/frontend/e2e/helpers/e2e-helpers.playwright.ts +++ b/frontend/e2e/helpers/e2e-helpers.playwright.ts @@ -1,4 +1,7 @@ -import { Page, expect } from '@playwright/test'; +import { Locator, Page, expect } from '@playwright/test'; + +// A CSS/data-test string, or a Locator built from role and accessible name. +type SelectorOrLocator = string | Locator; import { LONG_TIMEOUT, SHORT_TIMEOUT, byId, log, logUsingLastSection, getFlagsmith } from './utils.playwright'; // Re-export for backwards compatibility @@ -18,13 +21,36 @@ export type Rule = { export class E2EHelpers { constructor(private page: Page) {} + // The value editors are selected by role and accessible name rather than a + // data-test. The label reads "Control Value" once the feature has variations, + // hence the alternation; the weight chip is a labelAfter sibling, so it stays + // out of the accessible name. + featureValueField(): Locator { + return this.page + .locator('#create-feature-modal') + .getByRole('textbox', { name: /^(Value|Control Value)$/ }); + } + + variationValueField(index: number): Locator { + return this.page.getByRole('textbox', { name: 'Variation Value' }).nth(index); + } + + // The override's own label is "Value", or "Segment Control Value" once the + // feature has variations. Anchored, because getByRole matches the name as a + // substring and the row also holds read-only "Variation Value" editors. + segmentOverrideValueField(index: number): Locator { + return this.page + .locator(byId(`segment-override-${index}`)) + .getByRole('textbox', { name: /^(Value|Segment Control Value)$/ }); + } + async isElementExists(selector: string): Promise { return await this.page.locator(byId(selector)).count() > 0; } - async setText(selector: string, text: string) { + async setText(selector: SelectorOrLocator, text: string) { logUsingLastSection(`Set text ${selector} : ${text}`); - const element = this.page.locator(selector).first(); + const element = typeof selector === 'string' ? this.page.locator(selector).first() : selector; await element.waitFor({ state: 'visible', timeout: LONG_TIMEOUT }); await element.clear(); if (text) { @@ -32,9 +58,10 @@ export class E2EHelpers { } } - async waitForElementVisible(selector: string, timeout: number = LONG_TIMEOUT) { + async waitForElementVisible(selector: SelectorOrLocator, timeout: number = LONG_TIMEOUT) { logUsingLastSection(`Waiting element visible ${selector}`); - await this.page.locator(selector).first().waitFor({ + const element = typeof selector === 'string' ? this.page.locator(selector).first() : selector; + await element.waitFor({ state: 'visible', timeout }); @@ -269,7 +296,7 @@ export class E2EHelpers { await featureRow.waitFor({ state: 'visible', timeout: LONG_TIMEOUT }); await featureRow.dispatchEvent('click'); await this.waitForElementVisible('#create-feature-modal'); - await this.waitForElementVisible(byId('featureValue')); + await this.waitForElementVisible(this.featureValueField()); } // Create a feature @@ -296,7 +323,7 @@ export class E2EHelpers { await this.gotoFeatures(); await this.click('#show-create-feature-btn'); await this.setText(byId('featureID'), name); - await this.setText(byId('featureValue'), `${value}`); + await this.setText(this.featureValueField(), `${value}`); await this.setText(byId('featureDesc'), description); if (!defaultOff) { await this.click(byId('toggle-feature-button')); @@ -305,7 +332,7 @@ export class E2EHelpers { const v = mvs[i]; await this.click(byId('add-variation')); await this.page.waitForTimeout(200); - await this.setText(byId(`featureVariationValue${i}`), v.value); + await this.setText(this.variationValueField(i), v.value); await this.setText(byId(`featureVariationWeight${v.value}`), `${v.weight}`); await this.page.waitForTimeout(100); } @@ -588,7 +615,7 @@ export class E2EHelpers { await this.click(byId('segment_overrides')); } await this.click(dropdownSelector); - await this.waitForElementVisible(byId(`segment-override-value-${index}`)); + await this.waitForElementVisible(this.segmentOverrideValueField(index)); } // Add segment override for boolean flags @@ -611,7 +638,7 @@ export class E2EHelpers { // Add segment override for remote configs async addSegmentOverrideConfig(index: number, value: string | number | boolean, selectionIndex: number = 0) { await this.openSegmentOverride(index, selectionIndex); - await this.setText(byId(`segment-override-value-${index}`), `${value}`); + await this.setText(this.segmentOverrideValueField(index), `${value}`); await this.click(byId(`segment-override-toggle-${index}`)); } @@ -631,7 +658,7 @@ export class E2EHelpers { await featureRow.dispatchEvent('click'); await this.waitForElementVisible(byId('update-feature-btn')); if (value !== '') { - await this.setText(byId('featureValue'), `${value}`); + await this.setText(this.featureValueField(), `${value}`); } if (mvs.length > 0) { await this.page.waitForTimeout(500); diff --git a/frontend/e2e/tests/change-request-test.pw.ts b/frontend/e2e/tests/change-request-test.pw.ts index f1b681acb30c..a4274d3b44b4 100644 --- a/frontend/e2e/tests/change-request-test.pw.ts +++ b/frontend/e2e/tests/change-request-test.pw.ts @@ -12,10 +12,10 @@ test.describe('Change Request Tests', () => { page, }, testInfo) => { const { + featureValueField, assertChangeRequestCount, approveChangeRequest, - assertInputValue, - closeModal, + closeModal, createChangeRequest, createEnvironment, createRemoteConfig, @@ -73,7 +73,7 @@ test.describe('Change Request Tests', () => { log('Create change request by editing feature value') await gotoFeatures() await gotoFeature(featureName) - await setText(byId('featureValue'), 'updated_value') + await setText(featureValueField(), 'updated_value') await createChangeRequest( 'Update feature value', @@ -126,7 +126,7 @@ test.describe('Change Request Tests', () => { await page.reload({ waitUntil: 'domcontentloaded' }) await waitForElementVisible('#show-create-feature-btn') await gotoFeature(featureName) - await expect(page.locator(byId('featureValue'))).toHaveValue('updated_value', { timeout: 15000 }) + await expect(featureValueField()).toHaveText('updated_value', { timeout: 15000 }) await closeModal() log('Verify value via API') diff --git a/frontend/e2e/tests/flag-tests.pw.ts b/frontend/e2e/tests/flag-tests.pw.ts index c349137c0fb3..349fc63bd239 100644 --- a/frontend/e2e/tests/flag-tests.pw.ts +++ b/frontend/e2e/tests/flag-tests.pw.ts @@ -115,6 +115,48 @@ test.describe('Flag Tests', () => { await deleteFeature('header_enabled') }); + test('Value editor validates the value against the chosen format @oss', async ({ + page, + }) => { + const { + click, + closeModal, + featureValueField, + gotoProject, + login, + waitForElementVisible, + } = createHelpers(page) + + log('Login') + await login(E2E_USER, PASSWORD) + await gotoProject(E2E_TEST_PROJECT) + await waitForElementVisible(byId('features-page')) + + log('Open the create feature drawer') + await click('#show-create-feature-btn') + await waitForElementVisible(featureValueField()) + + const formats = page.getByRole('group', { name: 'Value format' }) + const validationError = page.locator('#language-validation-error') + + log('Malformed XML reports an error against the .xml format') + await featureValueField().fill('') + await formats.getByRole('button', { name: '.xml' }).click() + await expect(validationError).toBeVisible() + + log('A well-formed document clears it') + await featureValueField().fill('value') + await expect(validationError).toHaveCount(0) + + log('Invalid JSON reports against .json, valid JSON clears it') + await formats.getByRole('button', { name: '.json' }).click() + await expect(validationError).toBeVisible() + await featureValueField().fill('{ "colour": "blue" }') + await expect(validationError).toHaveCount(0) + + await closeModal() + }) + test('Feature flags can have tags added and be archived @oss', async ({ page }, testInfo) => { const { addTagToFeature, diff --git a/frontend/e2e/tests/mv-options-tests.pw.ts b/frontend/e2e/tests/mv-options-tests.pw.ts index 6cd540120f9f..c8d9cf718d8f 100644 --- a/frontend/e2e/tests/mv-options-tests.pw.ts +++ b/frontend/e2e/tests/mv-options-tests.pw.ts @@ -22,6 +22,7 @@ const variantCards = (page: Page) => page.locator('#create-feature-modal .varian test.describe('Multivariate Options', () => { test('Repeated saves keep the variant set stable @oss', async ({ page }) => { const { + variationValueField, closeModal, createRemoteConfig, editRemoteConfig, @@ -62,6 +63,7 @@ test.describe('Multivariate Options', () => { test('Variants can be added and removed in a single save @oss', async ({ page }) => { const { + variationValueField, click, closeModal, createRemoteConfig, @@ -90,7 +92,7 @@ test.describe('Multivariate Options', () => { await expect(variantCards(page)).toHaveCount(1); await click(byId('add-variation')); await page.waitForTimeout(200); - await setText(byId('featureVariationValue1'), 'added'); + await setText(variationValueField(1), 'added'); await page.waitForTimeout(500); await click(byId('update-feature-btn')); await waitForToast(); diff --git a/frontend/e2e/tests/segment-test.pw.ts b/frontend/e2e/tests/segment-test.pw.ts index e255a7aef2e8..810f7ea72d00 100644 --- a/frontend/e2e/tests/segment-test.pw.ts +++ b/frontend/e2e/tests/segment-test.pw.ts @@ -312,6 +312,7 @@ test('Segment test 2 - Test segment priority and overrides @oss', async ({ page test('Segment test 3 - Test user-specific feature overrides @oss', async ({ page }, testInfo) => { const { + featureValueField, assertUserFeatureValue, click, clickUserFeature, @@ -350,7 +351,7 @@ test('Segment test 3 - Test user-specific feature overrides @oss', async ({ page log('Edit flag for user') await clickUserFeature(REMOTE_CONFIG_FEATURE) - await setText(byId('featureValue'), 'small') + await setText(featureValueField(), 'small') await click('#update-feature-btn') await waitAndRefresh() // wait and refresh to avoid issues with data sync from UK -> US in github workflows await assertUserFeatureValue(REMOTE_CONFIG_FEATURE, '"small"') diff --git a/frontend/e2e/tests/versioning-tests.pw.ts b/frontend/e2e/tests/versioning-tests.pw.ts index f512c5c5553d..c266b6465447 100644 --- a/frontend/e2e/tests/versioning-tests.pw.ts +++ b/frontend/e2e/tests/versioning-tests.pw.ts @@ -10,6 +10,7 @@ import { E2E_USER, PASSWORD } from '../config'; test('Versioning tests - Create, edit, and compare feature versions @oss', async ({ page }, testInfo) => { const { + variationValueField, assertNumberOfVersions, click, closeModal, @@ -100,7 +101,7 @@ test('Versioning tests - Create, edit, and compare feature versions @oss', async await expect(page.locator(byId('featureVariationKey0'))).toHaveText('primary') await click(byId('add-variation')) await page.waitForTimeout(200) - await setText(byId('featureVariationValue2'), 'huge') + await setText(variationValueField(2), 'huge') await page.waitForTimeout(500) await click(byId('update-feature-btn')) await waitForToast() diff --git a/frontend/web/components/Highlight.js b/frontend/web/components/Highlight.js index d8370a1a9219..bf92943f7ae6 100644 --- a/frontend/web/components/Highlight.js +++ b/frontend/web/components/Highlight.js @@ -85,6 +85,15 @@ class Highlight extends React.Component { if (nextState.expandable !== this.state.expandable) return true if (nextState.expanded !== this.state.expanded) return true if (nextProps['data-test'] !== this.props['data-test']) return true + // Without these a value editor that turns read-only keeps its old + // contentEditable and never gains aria-readonly, because the text and + // className are unchanged. + if (nextProps.disabled !== this.props.disabled) return true + if (nextProps.role !== this.props.role) return true + if (nextProps['aria-readonly'] !== this.props['aria-readonly']) return true + if (nextProps['aria-labelledby'] !== this.props['aria-labelledby']) + return true + if (!nextProps.onChange !== !this.props.onChange) return true return this.state.value.__html !== `${nextProps.children}` } @@ -156,6 +165,13 @@ class Highlight extends React.Component { { this.setState({ changed: true }) - setValue( - Utils.getTypedValue( - Utils.safeParseEventValue(controlValue), - ), - ) + setValue(Utils.getTypedValue(controlValue)) }} canCopyValue={ permission && @@ -278,42 +275,34 @@ const SegmentOverrideInner = class Override extends React.Component { {showValue ? ( <>
- { + : (newValue) => { this.setState({ changed: true }) - setValue( - Utils.getTypedValue(Utils.safeParseEventValue(e)), - ) + setValue(Utils.getTypedValue(newValue)) } } - placeholder="Value e.g. 'big' " />
) : (
- } value={v.value} - data-test={`segment-override-value-${index}`} - placeholder="Value e.g. 'big' " disabled={readOnly} onChange={ readOnly ? null - : (e) => { + : (newValue) => { this.setState({ changed: true }) - setValue( - Utils.getTypedValue(Utils.safeParseEventValue(e)), - ) + setValue(Utils.getTypedValue(newValue)) } } /> diff --git a/frontend/web/components/ValueEditor.js b/frontend/web/components/ValueEditor.js deleted file mode 100644 index 038eb1656d31..000000000000 --- a/frontend/web/components/ValueEditor.js +++ /dev/null @@ -1,251 +0,0 @@ -import React, { Component } from 'react' -import cx from 'classnames' -import Highlight from './Highlight' -import { Clipboard } from 'polyfill-react-native' -import Icon from './icons/Icon' -import BareButton from './base/forms/BareButton' - -import toml from 'toml' -import yaml from 'yaml' - -function xmlIsInvalid(xmlStr) { - const parser = new DOMParser() - const dom = parser.parseFromString(xmlStr, 'application/xml') - for (const element of Array.from(dom.querySelectorAll('parsererror'))) { - if (element instanceof HTMLElement) { - // Found the error. - return element.innerText - } - } - // No errors found. - return false -} - -class Validation extends Component { - constructor(props) { - super(props) - this.state = {} - this.validateLanguage(this.props.language, this.props.value) - } - - componentDidUpdate(prevProps) { - if ( - prevProps.value !== this.props.value || - prevProps.language !== this.props.language - ) { - this.validateLanguage(this.props.language, this.props.value) - } - } - - validateLanguage = (language, value) => { - const validate = new Promise((resolve) => { - switch (language) { - case 'json': { - try { - JSON.parse(value) - resolve(false) - } catch (e) { - resolve(e.message) - } - break - } - case 'ini': { - try { - toml.parse(value) - resolve(false) - } catch (e) { - resolve(e.message) - } - break - } - case 'yaml': { - try { - yaml.parse(value) - resolve(false) - } catch (e) { - resolve(e.message) - } - break - } - case 'xml': { - try { - const error = xmlIsInvalid(value) - resolve(error) - } catch (e) { - resolve('Failed to parse XML') - } - break - } - default: { - resolve(false) - break - } - } - }) - - validate.then((error) => { - this.setState({ error }) - }) - } - - render() { - const displayLanguage = - this.props.language === 'ini' ? 'toml' : this.props.language - return this.state.error ? ( - - - - } - > - {`${displayLanguage} validation error, please check your value.
Error: ${this.state.error}`} - - ) : ( - - - - ) - } -} -class ValueEditor extends Component { - state = { - language: 'txt', - } - - componentDidMount() { - if (this.props.language) { - this.setState({ language: this.props.language }) - this.renderValidation(this.props.language) - } - if (!this.props.value) return - try { - const v = JSON.parse(this.props.value) - if (typeof v !== 'object') return - this.setState({ language: 'json' }) - } catch (e) {} - } - - renderValidation = () => ( - - ) - - copyValue = () => { - const res = Clipboard.setString(this.props.value) - toast( - res ? 'Clipboard set' : 'Could not set clipboard :(', - res ? '' : 'danger', - ) - } - - render() { - const { ...rest } = this.props - const showCopy = !this.props.onlyOneLang && !this.props.disabled - return ( -
- {!this.props.onlyOneLang && ( - - { - e.preventDefault() - e.stopPropagation() - this.setState({ language: 'txt' }) - }} - className={cx('txt', { active: this.state.language === 'txt' })} - > - .txt - - { - e.preventDefault() - e.stopPropagation() - this.setState({ language: 'json' }) - }} - className={cx('json', { active: this.state.language === 'json' })} - > - .json {this.state.language === 'json' && this.renderValidation()} - - { - e.preventDefault() - e.stopPropagation() - this.setState({ language: 'xml' }) - }} - className={cx('xml', { active: this.state.language === 'xml' })} - > - .xml {this.state.language === 'xml' && this.renderValidation()} - - { - e.preventDefault() - e.stopPropagation() - - this.setState({ language: 'ini' }) - }} - className={cx('ini', { active: this.state.language === 'ini' })} - > - .toml {this.state.language === 'ini' && this.renderValidation()} - - { - e.preventDefault() - e.stopPropagation() - this.setState({ language: 'yaml' }) - }} - className={cx('yaml', { active: this.state.language === 'yaml' })} - > - .yaml {this.state.language === 'yaml' && this.renderValidation()} - - - )} - - {showCopy && ( - - - - )} - - {E2E ? ( -