diff --git a/static/app/views/settings/dynamicSampling/organizationSampling.spec.tsx b/static/app/views/settings/dynamicSampling/organizationSampling.spec.tsx new file mode 100644 index 000000000000..82271da41b30 --- /dev/null +++ b/static/app/views/settings/dynamicSampling/organizationSampling.spec.tsx @@ -0,0 +1,157 @@ +import {OrganizationFixture} from 'sentry-fixture/organization'; + +import {render, screen, userEvent, waitFor} from 'sentry-test/reactTestingLibrary'; + +import {OrganizationSampling} from 'sentry/views/settings/dynamicSampling/organizationSampling'; + +jest.mock('@tanstack/react-virtual', () => ({ + useVirtualizer: jest.fn(({count}: {count: number}) => ({ + getVirtualItems: jest.fn(() => + Array.from({length: count}, (_, index) => ({ + key: index, + index, + start: index * 63, + size: 63, + })) + ), + getTotalSize: jest.fn(() => count * 63), + measure: jest.fn(), + })), +})); + +describe('OrganizationSampling', () => { + const organization = OrganizationFixture({ + slug: 'org-slug', + access: ['org:write'], + targetSampleRate: 0.5, + samplingMode: 'organization', + }); + + beforeEach(() => { + MockApiClient.clearMockResponses(); + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/sampling/project-root-counts/', + body: {data: [], end: '', intervals: [], start: ''}, + }); + }); + + it('renders initial state with correct input value and button states', () => { + render(, {organization}); + + expect(screen.getByRole('spinbutton')).toHaveValue(50); + expect(screen.getByRole('button', {name: 'Apply Changes'})).toBeEnabled(); + expect(screen.getByRole('button', {name: 'Reset'})).toBeDisabled(); + }); + + it('resets the input back to the saved value when Reset is clicked', async () => { + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.type(screen.getByRole('spinbutton'), '30'); + + expect(screen.getByRole('button', {name: 'Reset'})).toBeEnabled(); + + await userEvent.click(screen.getByRole('button', {name: 'Reset'})); + + expect(screen.getByRole('spinbutton')).toHaveValue(50); + }); + + it('does not call the API when value is out of range', async () => { + const putMock = MockApiClient.addMockResponse({ + url: '/organizations/org-slug/', + method: 'PUT', + body: OrganizationFixture(), + }); + + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.type(screen.getByRole('spinbutton'), '150'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + // jsdom doesn't support formNoValidate, so browser-native max={100} + // blocks the submit before Zod can run. The error message can't be + // asserted here — it works correctly in real browsers. + expect(putMock).not.toHaveBeenCalled(); + }); + + it('shows a validation error for an empty value on submit', async () => { + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + expect(await screen.findByText('Please enter a valid number')).toBeInTheDocument(); + }); + + it('calls the API with the correct payload on save', async () => { + const putMock = MockApiClient.addMockResponse({ + url: '/organizations/org-slug/', + method: 'PUT', + body: OrganizationFixture({targetSampleRate: 0.3}), + }); + + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.type(screen.getByRole('spinbutton'), '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => { + expect(putMock).toHaveBeenCalledWith( + '/organizations/org-slug/', + expect.objectContaining({data: {targetSampleRate: 0.3}}) + ); + }); + }); + + it('resets form to clean state after a successful save', async () => { + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/', + method: 'PUT', + body: OrganizationFixture({targetSampleRate: 0.3}), + }); + + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.type(screen.getByRole('spinbutton'), '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => + expect(screen.getByRole('button', {name: 'Reset'})).toBeDisabled() + ); + }); + + it('keeps form dirty after an API error', async () => { + MockApiClient.addMockResponse({ + url: '/organizations/org-slug/', + method: 'PUT', + statusCode: 500, + body: {detail: 'Internal Server Error'}, + }); + + render(, {organization}); + + await userEvent.clear(screen.getByRole('spinbutton')); + await userEvent.type(screen.getByRole('spinbutton'), '30'); + await userEvent.click(screen.getByRole('button', {name: 'Apply Changes'})); + + await waitFor(() => + expect(screen.getByRole('button', {name: 'Reset'})).toBeEnabled() + ); + }); + + it('disables Apply Changes and input for users without org:write access', () => { + const orgWithoutAccess = OrganizationFixture({ + access: [], + targetSampleRate: 0.5, + samplingMode: 'organization', + }); + + render(, {organization: orgWithoutAccess}); + + expect(screen.getByRole('button', {name: 'Apply Changes'})).toBeDisabled(); + expect(screen.getByRole('spinbutton')).toBeDisabled(); + }); +}); diff --git a/static/app/views/settings/dynamicSampling/organizationSampling.tsx b/static/app/views/settings/dynamicSampling/organizationSampling.tsx index 5942c2e83c60..5ad9d9c05b61 100644 --- a/static/app/views/settings/dynamicSampling/organizationSampling.tsx +++ b/static/app/views/settings/dynamicSampling/organizationSampling.tsx @@ -1,7 +1,9 @@ import {Fragment, useState} from 'react'; import styled from '@emotion/styled'; +import {z} from 'zod'; import {Button} from '@sentry/scraps/button'; +import {defaultFormOptions, useScrapsForm} from '@sentry/scraps/form'; import {Tooltip} from '@sentry/scraps/tooltip'; import {addErrorMessage, addSuccessMessage} from 'sentry/actionCreators/indicator'; @@ -13,7 +15,6 @@ import {ProjectionPeriodControl} from 'sentry/views/settings/dynamicSampling/pro import {ProjectsPreviewTable} from 'sentry/views/settings/dynamicSampling/projectsPreviewTable'; import {SamplingModeSwitch} from 'sentry/views/settings/dynamicSampling/samplingModeSwitch'; import {useHasDynamicSamplingWriteAccess} from 'sentry/views/settings/dynamicSampling/utils/access'; -import {organizationSamplingForm} from 'sentry/views/settings/dynamicSampling/utils/organizationSamplingForm'; import {parsePercent} from 'sentry/views/settings/dynamicSampling/utils/parsePercent'; import { useProjectSampleCounts, @@ -21,96 +22,130 @@ import { } from 'sentry/views/settings/dynamicSampling/utils/useProjectSampleCounts'; import {useUpdateOrganization} from 'sentry/views/settings/dynamicSampling/utils/useUpdateOrganization'; -const {useFormState, FormProvider} = organizationSamplingForm; const UNSAVED_CHANGES_MESSAGE = t( 'You have unsaved changes, are you sure you want to leave?' ); +export const targetSampleRateSchema = z.object({ + targetSampleRate: z + .string() + .min(1, t('Please enter a valid number')) + .refine(val => !isNaN(Number(val)), {message: t('Please enter a valid number')}) + .refine( + val => { + const n = Number(val); + return n >= 0 && n <= 100; + }, + {message: t('Must be between 0% and 100%')} + ), +}); + export function OrganizationSampling() { const organization = useOrganization(); const hasAccess = useHasDynamicSamplingWriteAccess(); const [period, setPeriod] = useState('24h'); - const formState = useFormState({ - initialValues: { - targetSampleRate: ((organization.targetSampleRate ?? 1) * 100)?.toString(), - }, - }); - - const sampleCountsQuery = useProjectSampleCounts({period}); + const initialTargetSampleRate = ( + (organization.targetSampleRate ?? 1) * 100 + )?.toString(); + const [savedTargetSampleRate, setSavedTargetSampleRate] = useState( + initialTargetSampleRate + ); - const {mutate: updateOrganization, isPending} = useUpdateOrganization(); + const {mutateAsync: updateOrganization, isPending} = useUpdateOrganization(); - const handleSubmit = () => { - updateOrganization( - { - targetSampleRate: parsePercent(formState.fields.targetSampleRate.value), - }, - { - onSuccess: () => { - addSuccessMessage(t('Changes applied.')); - formState.save(); - }, - onError: () => { - addErrorMessage(t('Unable to save changes. Please try again.')); - }, + const form = useScrapsForm({ + ...defaultFormOptions, + defaultValues: { + targetSampleRate: initialTargetSampleRate, + }, + validators: { + onDynamic: targetSampleRateSchema, + }, + onSubmit: async ({value, formApi}) => { + try { + await updateOrganization({ + targetSampleRate: parsePercent(value.targetSampleRate), + }); + addSuccessMessage(t('Changes applied.')); + setSavedTargetSampleRate(value.targetSampleRate); + formApi.reset(value); + } catch { + addErrorMessage(t('Unable to save changes. Please try again.')); } - ); - }; + }, + }); - const handleReset = () => { - formState.reset(); - }; + const sampleCountsQuery = useProjectSampleCounts({period}); return ( - - - locationChange.currentLocation.pathname !== - locationChange.nextLocation.pathname && formState.hasChanged - } - /> - - - - - {sampleCountsQuery.isError ? ( - - ) : ( - - - - - - - } - /> - )} - - {t('Inactive projects are not listed and will be sampled at 100% initially.')} - - + + ({isDirty: s.isDirty, canSubmit: s.canSubmit})}> + {({isDirty, canSubmit}) => ( + + + locationChange.currentLocation.pathname !== + locationChange.nextLocation.pathname && isDirty + } + /> + + + + + {sampleCountsQuery.isError ? ( + + ) : ( + + {field => ( + + + + + {t('Apply Changes')} + + + + } + /> + )} + + )} + + {t( + 'Inactive projects are not listed and will be sampled at 100% initially.' + )} + + + )} + + ); } + const HeadingRow = styled('div')` display: flex; align-items: center; diff --git a/static/app/views/settings/dynamicSampling/percentInput.tsx b/static/app/views/settings/dynamicSampling/percentInput.tsx index 852fc87b41f7..8f08c5f0b06f 100644 --- a/static/app/views/settings/dynamicSampling/percentInput.tsx +++ b/static/app/views/settings/dynamicSampling/percentInput.tsx @@ -10,7 +10,7 @@ export function PercentInput({ref, ...props}: InputProps) { width: 120px; `} > - + % diff --git a/static/app/views/settings/dynamicSampling/projectsPreviewTable.tsx b/static/app/views/settings/dynamicSampling/projectsPreviewTable.tsx index f0743f5d4b7b..4e2f4a004935 100644 --- a/static/app/views/settings/dynamicSampling/projectsPreviewTable.tsx +++ b/static/app/views/settings/dynamicSampling/projectsPreviewTable.tsx @@ -9,7 +9,6 @@ import {ProjectsTable} from 'sentry/views/settings/dynamicSampling/projectsTable import {SamplingBreakdown} from 'sentry/views/settings/dynamicSampling/samplingBreakdown'; import {mapArrayToObject} from 'sentry/views/settings/dynamicSampling/utils'; import {formatPercent} from 'sentry/views/settings/dynamicSampling/utils/formatPercent'; -import {organizationSamplingForm} from 'sentry/views/settings/dynamicSampling/utils/organizationSamplingForm'; import {parsePercent} from 'sentry/views/settings/dynamicSampling/utils/parsePercent'; import {balanceSampleRate} from 'sentry/views/settings/dynamicSampling/utils/rebalancing'; import type { @@ -17,20 +16,29 @@ import type { ProjectSampleCount, } from 'sentry/views/settings/dynamicSampling/utils/useProjectSampleCounts'; -const {useFormField} = organizationSamplingForm; - -interface Props { +interface ProjectsPreviewTableProps { actions: React.ReactNode; isLoading: boolean; + onTargetSampleRateChange: (value: string) => void; period: ProjectionSamplePeriod; sampleCounts: ProjectSampleCount[]; + savedTargetSampleRate: string; + targetSampleRate: string; + targetSampleRateError?: string; } -export function ProjectsPreviewTable({actions, isLoading, period, sampleCounts}: Props) { - const sampleRateField = useFormField('targetSampleRate'); - +export function ProjectsPreviewTable({ + actions, + isLoading, + period, + sampleCounts, + targetSampleRate, + savedTargetSampleRate, + onTargetSampleRateChange, + targetSampleRateError, +}: ProjectsPreviewTableProps) { const debouncedTargetSampleRate = useDebouncedValue( - sampleRateField.value, + targetSampleRate, // For longer lists we debounce the input to avoid too many re-renders sampleCounts.length > 100 ? 200 : 0 ); @@ -55,7 +63,7 @@ export function ProjectsPreviewTable({actions, isLoading, period, sampleCounts}: }, [debouncedTargetSampleRate, balancingItems]); const initialSampleRatesById = useMemo(() => { - const targetRate = parsePercent(sampleRateField.initialValue); + const targetRate = parsePercent(savedTargetSampleRate); const {balancedItems: initialBalancedItems} = balanceSampleRate({ targetSampleRate: targetRate, items: balancingItems, @@ -66,7 +74,7 @@ export function ProjectsPreviewTable({actions, isLoading, period, sampleCounts}: keySelector: item => item.id, valueSelector: item => item.sampleRate, }); - }, [sampleRateField.initialValue, balancingItems]); + }, [savedTargetSampleRate, balancingItems]); const itemsWithFormattedNumbers = useMemo(() => { return balancedItems.map(item => ({ @@ -95,11 +103,11 @@ export function ProjectsPreviewTable({actions, isLoading, period, sampleCounts}: /> !isNaN(Number(val)), {message: t('Please enter a valid number.')}) - .refine( - val => { - const n = Number(val); - return n >= 0 && n <= 100; - }, - {message: t('Must be between 0% and 100%')} - ), -}); - function SamplingModeSwitchModal({ Header, Body, @@ -70,7 +55,7 @@ function SamplingModeSwitchModal({ targetSampleRate: formatPercent(initialTargetRate || 0), }, validators: { - onDynamic: schema, + onDynamic: targetSampleRateSchema, }, onSubmit: ({value}) => { const changes: Parameters[0] = {samplingMode}; diff --git a/static/app/views/settings/dynamicSampling/utils/organizationSamplingForm.tsx b/static/app/views/settings/dynamicSampling/utils/organizationSamplingForm.tsx deleted file mode 100644 index dafc3d7573cd..000000000000 --- a/static/app/views/settings/dynamicSampling/utils/organizationSamplingForm.tsx +++ /dev/null @@ -1,26 +0,0 @@ -import {t} from 'sentry/locale'; -import {createForm} from 'sentry/views/settings/dynamicSampling/utils/formContext'; - -type FormFields = { - targetSampleRate: string; -}; - -export const organizationSamplingForm = createForm({ - validators: { - targetSampleRate: (value: string) => { - if (value === '') { - return t('This field is required.'); - } - - const numericValue = Number(value); - if (isNaN(numericValue)) { - return t('Please enter a valid number.'); - } - - if (numericValue < 0 || numericValue > 100) { - return t('Must be between 0% and 100%'); - } - return undefined; - }, - }, -});