diff --git a/frontend/common/services/useCohort.ts b/frontend/common/services/useCohort.ts new file mode 100644 index 000000000000..022a0268ee7b --- /dev/null +++ b/frontend/common/services/useCohort.ts @@ -0,0 +1,82 @@ +import { Res } from 'common/types/responses' +import { Req } from 'common/types/requests' +import { service } from 'common/service' +import toFormData from 'common/utils/toFormData' + +export const cohortService = service + .enhanceEndpoints({ addTagTypes: ['Cohort', 'Segment'] }) + .injectEndpoints({ + endpoints: (builder) => ({ + createCohort: builder.mutation({ + invalidatesTags: (q, e, arg) => [ + { id: 'LIST', type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + query: (query) => ({ + body: { + description: query.description, + metadata: query.metadata, + name: query.name, + }, + method: 'POST', + url: `environments/${query.environmentApiKey}/cohorts/`, + }), + }), + deleteCohort: builder.mutation({ + invalidatesTags: (q, e, arg) => [ + { id: 'LIST', type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + query: (query) => ({ + method: 'DELETE', + url: `environments/${query.environmentApiKey}/cohorts/${query.cohortId}/`, + }), + }), + syncCohortCsv: builder.mutation< + Res['cohortCsvSync'], + Req['syncCohortCsv'] + >({ + invalidatesTags: (q, e, arg) => [ + { id: arg.cohortId, type: 'Cohort' }, + { id: `LIST${arg.projectId}`, type: 'Segment' }, + ], + queryFn: async (query, baseQueryApi, extraOptions, baseQuery) => { + // projectId only feeds tag invalidation; keep it out of the form data. + const { cohortId, environmentApiKey, projectId: _, ...rest } = query + const formData = toFormData({ ...rest }) + const { data, error } = await baseQuery({ + body: formData, + method: 'POST', + url: `environments/${environmentApiKey}/cohorts/${cohortId}/sync-csv/`, + }) + return { data, error } as { + data: Res['cohortCsvSync'] + error: never + } + }, + }), + // END OF ENDPOINTS + }), + }) + +export async function deleteCohort( + store: any, + data: Req['deleteCohort'], + options?: Parameters[1], +) { + return store.dispatch( + cohortService.endpoints.deleteCohort.initiate(data, options), + ) +} + +export const { + useCreateCohortMutation, + useDeleteCohortMutation, + useSyncCohortCsvMutation, + // END OF EXPORTS +} = cohortService + +/* Usage examples: +const [createCohort, { isLoading, data, isSuccess }] = useCreateCohortMutation() +const [syncCohortCsv, { isLoading }] = useSyncCohortCsvMutation() +*/ diff --git a/frontend/common/types/requests.ts b/frontend/common/types/requests.ts index 8f4ae03ca9e3..a603c549352f 100644 --- a/frontend/common/types/requests.ts +++ b/frontend/common/types/requests.ts @@ -5,6 +5,7 @@ import { FeatureStateValue, ImportStrategy, Approval, + Metadata, MultivariateOption, SAMLConfiguration, Segment, @@ -173,6 +174,26 @@ export type Req = { projectId: number segment: Omit } + createCohort: { + environmentApiKey: string + projectId: number + name: string + description?: string + metadata?: Metadata[] + } + deleteCohort: { + environmentApiKey: string + cohortId: number + projectId: number + } + syncCohortCsv: { + environmentApiKey: string + cohortId: number + projectId: number + file: File + identifier_column?: number + has_header?: boolean + } cloneSegment: { projectId: number segmentId: number diff --git a/frontend/common/types/responses.ts b/frontend/common/types/responses.ts index 5eec1303ede8..799b3da75e66 100644 --- a/frontend/common/types/responses.ts +++ b/frontend/common/types/responses.ts @@ -174,6 +174,16 @@ export type SegmentMembersResponse = PagedResponse & { // Pass as `cursor` to fetch the next page; null when there are no more rows. next_cursor: string | null } +export type SegmentCohort = { + id: number + environment: number + environment_api_key: string + environment_name: string + source_type: 'csv' + version: number + deletion_requested_at: string | null +} + export type Segment = { id: number rules: SegmentRule[] @@ -184,6 +194,7 @@ export type Segment = { feature?: number metadata: Metadata[] | [] membership_counts?: SegmentMembership[] + cohort?: SegmentCohort | null } export type ProjectChangeRequest = Omit< ChangeRequest, @@ -995,6 +1006,29 @@ export type Metadata = { field_value: string } +export type Cohort = { + id: number + uuid: string + name: string + description: string | null + segment: number + source_type: 'csv' + version: number + created_at: string +} + +export type CohortCsvSyncResult = { + version: number + added: number + removed: number + unchanged: number + ignored: { + empty: number + duplicates: number + too_long: number + } +} + export type MetadataFieldModelField = { id: number content_type: number @@ -1324,6 +1358,8 @@ export type TrustRelationship = { export type Res = { segments: PagedResponse segment: Segment + cohort: Cohort + cohortCsvSync: CohortCsvSyncResult segmentMembers: SegmentMembersResponse auditLogs: PagedResponse organisationLicence: {} diff --git a/frontend/common/utils/__tests__/csv.test.ts b/frontend/common/utils/__tests__/csv.test.ts index 1d6e0664b7c1..0f49a1439d06 100644 --- a/frontend/common/utils/__tests__/csv.test.ts +++ b/frontend/common/utils/__tests__/csv.test.ts @@ -1,4 +1,10 @@ -import { extractIdentifiers, parseCsvText, toParsedCsv } from 'common/utils/csv' +import { + extractIdentifiers, + MAX_IDENTIFIER_BYTES, + parseCsvText, + toCsvColumn, + toParsedCsv, +} from 'common/utils/csv' describe('parseCsvText', () => { const cases: [string, string, string[][]][] = [ @@ -68,6 +74,7 @@ describe('extractIdentifiers', () => { duplicateCount: 2, emptyCount: 2, identifiers: ['a', 'b'], + tooLongCount: 0, }) }) @@ -76,6 +83,40 @@ describe('extractIdentifiers', () => { duplicateCount: 0, emptyCount: 1, identifiers: ['y'], + tooLongCount: 0, }) }) + + test('identifiers over the UTF-8 byte limit are dropped', () => { + // 'é' is 2 UTF-8 bytes, so 513 of them exceed 1024 bytes in 513 chars. + const rows = [ + ['a'.repeat(MAX_IDENTIFIER_BYTES)], + ['a'.repeat(MAX_IDENTIFIER_BYTES + 1)], + ['é'.repeat(513)], + ] + expect(extractIdentifiers(rows, 0)).toEqual({ + duplicateCount: 0, + emptyCount: 0, + identifiers: ['a'.repeat(MAX_IDENTIFIER_BYTES)], + tooLongCount: 2, + }) + }) +}) + +describe('toCsvColumn', () => { + test.each([ + ['plain values', ['a', 'b'], 'a\nb'], + ['comma quoted', ['Doe, Jane', 'b'], '"Doe, Jane"\nb'], + ['quote escaped', ['say "hi"'], '"say ""hi"""'], + ['newline quoted', ['line1\nline2'], '"line1\nline2"'], + ])('%s', (_, values, expected) => { + expect(toCsvColumn(values)).toEqual(expected) + }) + + test('round-trips through parseCsvText', () => { + const values = ['plain', 'Doe, Jane', 'say "hi"', 'multi\nline'] + expect(parseCsvText(toCsvColumn(values)).map((row) => row[0])).toEqual( + values, + ) + }) }) diff --git a/frontend/common/utils/csv.ts b/frontend/common/utils/csv.ts index e99824e4c448..4abc3f50eb26 100644 --- a/frontend/common/utils/csv.ts +++ b/frontend/common/utils/csv.ts @@ -7,8 +7,13 @@ export type ExtractedIdentifiers = { duplicateCount: number emptyCount: number identifiers: string[] + tooLongCount: number } +// Mirrors the API's COHORT_IDENTIFIER_MAX_BYTES (Edge identifiers are +// DynamoDB sort keys, capped at 1024 bytes). +export const MAX_IDENTIFIER_BYTES = 1024 + export function parseCsvText(text: string): string[][] { const rows: string[][] = [] let row: string[] = [] @@ -58,7 +63,10 @@ export function toParsedCsv( if (!rawRows.length) { return { columns: [], rows: [] } } - const columnCount = Math.max(...rawRows.map((cells) => cells.length)) + const columnCount = rawRows.reduce( + (max, cells) => Math.max(max, cells.length), + 0, + ) if (hasHeaders) { const [header, ...rows] = rawRows return { @@ -81,12 +89,16 @@ export function extractIdentifiers( ): ExtractedIdentifiers { const seen = new Set() const identifiers: string[] = [] + const encoder = new TextEncoder() let emptyCount = 0 let duplicateCount = 0 + let tooLongCount = 0 for (const cells of rows) { const value = (cells[columnIndex] ?? '').trim() if (!value) { emptyCount++ + } else if (encoder.encode(value).length > MAX_IDENTIFIER_BYTES) { + tooLongCount++ } else if (seen.has(value)) { duplicateCount++ } else { @@ -94,5 +106,13 @@ export function extractIdentifiers( identifiers.push(value) } } - return { duplicateCount, emptyCount, identifiers } + return { duplicateCount, emptyCount, identifiers, tooLongCount } +} + +export function toCsvColumn(values: string[]): string { + return values + .map((value) => + /[",\r\n]/.test(value) ? `"${value.replace(/"/g, '""')}"` : value, + ) + .join('\n') } diff --git a/frontend/web/components/CsvUpload/CsvUpload.tsx b/frontend/web/components/CsvUpload/CsvUpload.tsx index f1e93ebbb93d..9fa6668ab757 100644 --- a/frontend/web/components/CsvUpload/CsvUpload.tsx +++ b/frontend/web/components/CsvUpload/CsvUpload.tsx @@ -9,6 +9,7 @@ import './CsvUpload.scss' export type CsvUploadType = { value: File | null + maxSizeBytes?: number rowCount?: number onChange: (file: File, text: string) => void } @@ -20,7 +21,12 @@ const formatFileSize = (bytes: number) => { return `${(bytes / 1024).toFixed(1)} KB` } -const CsvUpload: FC = ({ onChange, rowCount, value }) => { +const CsvUpload: FC = ({ + maxSizeBytes, + onChange, + rowCount, + value, +}) => { const [error, setError] = useState('') const onDrop = useCallback( @@ -46,12 +52,17 @@ const CsvUpload: FC = ({ onChange, rowCount, value }) => { accept: { 'text/csv': ['.csv'], }, + maxSize: maxSizeBytes, multiple: false, noClick: true, noKeyboard: true, onDrop, - onDropRejected: () => { - setError('Please select a CSV file') + onDropRejected: (rejections) => { + setError( + rejections[0]?.errors?.[0]?.code === 'file-too-large' && maxSizeBytes + ? `Please select a file smaller than ${formatFileSize(maxSizeBytes)}` + : 'Please select a CSV file', + ) }, }) @@ -91,7 +102,11 @@ const CsvUpload: FC = ({ onChange, rowCount, value }) => { )} - {!!error && } + {!!error && ( +
+ +
+ )} ) } diff --git a/frontend/web/components/modals/ConfirmRemoveSegment.tsx b/frontend/web/components/modals/ConfirmRemoveSegment.tsx index d1de098dff78..377be3f045a4 100644 --- a/frontend/web/components/modals/ConfirmRemoveSegment.tsx +++ b/frontend/web/components/modals/ConfirmRemoveSegment.tsx @@ -6,6 +6,7 @@ import Utils from 'common/utils/utils' import Button from 'components/base/forms/Button' import ModalHR from './ModalHR' import { deleteSegment } from 'common/services/useSegment' +import { deleteCohort } from 'common/services/useCohort' import { getStore } from 'common/store' type ConfirmRemoveSegmentType = { @@ -19,7 +20,15 @@ export const handleRemoveSegment = ( ) => { const removeSegmentCallback = async () => { try { - const res = await deleteSegment(getStore(), { id: segment.id, projectId }) + // Cohort-managed segments must be deleted via their cohort; the segment + // endpoint rejects them. + const res = segment.cohort + ? await deleteCohort(getStore(), { + cohortId: segment.cohort.id, + environmentApiKey: segment.cohort.environment_api_key, + projectId: Number(projectId), + }) + : await deleteSegment(getStore(), { id: segment.id, projectId }) if (res.error) throw new Error(res.error) toast(
diff --git a/frontend/web/components/modals/CreateSegment.tsx b/frontend/web/components/modals/CreateSegment.tsx index dceff143c620..eb23e23f32c2 100644 --- a/frontend/web/components/modals/CreateSegment.tsx +++ b/frontend/web/components/modals/CreateSegment.tsx @@ -143,6 +143,12 @@ const CreateSegment: FC = ({ projectId, }) const [segment, setSegment] = useState(_segment || defaultSegment) + // A cohort owns its segment's rules, and the API rejects updating one. + const isCohortManaged = !!_segment?.cohort + const isReadOnly = readOnly || isCohortManaged + const readOnlyMessage = isCohortManaged + ? 'This segment is managed by a cohort. Its rules follow the cohort membership and cannot be edited.' + : undefined const [description, setDescription] = useState(segment.description) const [name, setName] = useState(segment.name) const [rules, setRules] = useState(segment.rules) @@ -254,6 +260,7 @@ const CreateSegment: FC = ({ const save = async (e: FormEvent) => { try { Utils.preventDefault(e) + if (isReadOnly) return setValueChanged(false) setMetadataValueChanged(false) const segmentData: Omit = { @@ -462,7 +469,7 @@ const CreateSegment: FC = ({ /> = ({ )} - {!readOnly && ( + {!isReadOnly && (
addRule(topLevelRuleType === 'ANY' ? 'ALL' : 'ANY') @@ -504,7 +511,7 @@ const CreateSegment: FC = ({
)} - {topLevelRuleType !== 'ANY' && ( + {!isReadOnly && topLevelRuleType !== 'ANY' && (
addRule('NONE')} className='text-center'>
@@ -273,7 +375,7 @@ const CreateSegmentFromCsv: FC = ({ projectId }) => { {form} - + = ({ projectId }) => { projectId={Number(projectId)} entityContentType={segmentContentType.id} entity={segmentContentType.model} + onChange={(m) => setMetadata(m as Metadata[])} /> } /> diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts new file mode 100644 index 000000000000..c69812825bf9 --- /dev/null +++ b/frontend/web/components/modals/CreateSegmentFromCsv/__tests__/submitCohortCsv.test.ts @@ -0,0 +1,76 @@ +import { submitCohortCsv } from 'components/modals/CreateSegmentFromCsv/submitCohortCsv' + +describe('submitCohortCsv', () => { + test('first submit creates the cohort, records it, then syncs', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 7 }) + const syncCsv = jest.fn().mockResolvedValue({ added: 3 }) + const onCohortCreated = jest.fn() + + const result = await submitCohortCsv({ + createCohort, + createdCohort: null, + formKey: 'a', + onCohortCreated, + syncCsv, + }) + + expect(createCohort).toHaveBeenCalledTimes(1) + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'a', id: 7 }) + expect(syncCsv).toHaveBeenCalledWith(7) + expect(result).toEqual({ added: 3 }) + }) + + test('retry with an unchanged form reuses the cohort and only syncs', async () => { + const createCohort = jest.fn() + const syncCsv = jest.fn().mockResolvedValue({ added: 3 }) + const onCohortCreated = jest.fn() + + await submitCohortCsv({ + createCohort, + createdCohort: { formKey: 'a', id: 7 }, + formKey: 'a', + onCohortCreated, + syncCsv, + }) + + expect(createCohort).not.toHaveBeenCalled() + expect(onCohortCreated).not.toHaveBeenCalled() + expect(syncCsv).toHaveBeenCalledWith(7) + }) + + test('retry after editing the form creates a new cohort instead of reusing it', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 9 }) + const syncCsv = jest.fn().mockResolvedValue({ added: 1 }) + const onCohortCreated = jest.fn() + + await submitCohortCsv({ + createCohort, + createdCohort: { formKey: 'a', id: 7 }, + formKey: 'b', + onCohortCreated, + syncCsv, + }) + + expect(createCohort).toHaveBeenCalledTimes(1) + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'b', id: 9 }) + expect(syncCsv).toHaveBeenCalledWith(9) + }) + + test('a failed sync still records the created cohort for retry', async () => { + const createCohort = jest.fn().mockResolvedValue({ id: 7 }) + const syncCsv = jest.fn().mockRejectedValue(new Error('sync failed')) + const onCohortCreated = jest.fn() + + await expect( + submitCohortCsv({ + createCohort, + createdCohort: null, + formKey: 'a', + onCohortCreated, + syncCsv, + }), + ).rejects.toThrow('sync failed') + + expect(onCohortCreated).toHaveBeenCalledWith({ formKey: 'a', id: 7 }) + }) +}) diff --git a/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts new file mode 100644 index 000000000000..3fab248848f7 --- /dev/null +++ b/frontend/web/components/modals/CreateSegmentFromCsv/submitCohortCsv.ts @@ -0,0 +1,35 @@ +// A created cohort belongs to the form state that created it. +export type CreatedCohort = { + id: number + formKey: string +} + +export type SubmitCohortCsvArgs = { + // The cohort created by an earlier attempt, kept so a retry only syncs. + createdCohort: CreatedCohort | null + // Fingerprint of the inputs that feed cohort creation. + formKey: string + createCohort: () => Promise<{ id: number }> + syncCsv: (cohortId: number) => Promise + onCohortCreated: (cohort: CreatedCohort) => void +} + +// Two-step save: create the cohort, then sync the CSV. A cohort left behind by +// a failed sync is reused only while the inputs that created it are unchanged, +// so an edited retry never syncs against the previous form's cohort. +// onCohortCreated fires before the sync so a failed sync can still retry. +export async function submitCohortCsv({ + createCohort, + createdCohort, + formKey, + onCohortCreated, + syncCsv, +}: SubmitCohortCsvArgs): Promise { + let cohortId = + createdCohort?.formKey === formKey ? createdCohort.id : undefined + if (cohortId === undefined) { + cohortId = (await createCohort()).id + onCohortCreated({ formKey, id: cohortId }) + } + return syncCsv(cohortId) +} diff --git a/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx b/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx index 0ea6c89708b1..4e576b9ab4cf 100644 --- a/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx +++ b/frontend/web/components/modals/CreateSegmentRulesTabForm.tsx @@ -36,6 +36,8 @@ interface CreateSegmentRulesTabFormProps { setDescription: (description: string) => void identity?: boolean readOnly?: boolean + // Explains why editing is unavailable; defaults to the permission hint. + readOnlyMessage?: string showDescriptions: boolean setShowDescriptions: (show: boolean) => void allWarnings: string[] @@ -64,6 +66,7 @@ const CreateSegmentRulesTabForm: React.FC = ({ onCancel, onCreateChangeRequest, readOnly, + readOnlyMessage, rulesEl, save, segment, @@ -257,7 +260,8 @@ const CreateSegmentRulesTabForm: React.FC = ({ } place='left' > - {Constants.projectPermissions(ProjectPermission.ADMIN)} + {readOnlyMessage || + Constants.projectPermissions(ProjectPermission.ADMIN)}
) : ( diff --git a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx index 758d27e6f662..4f4d32b8f8c3 100644 --- a/frontend/web/components/segments/SegmentRow/SegmentRow.tsx +++ b/frontend/web/components/segments/SegmentRow/SegmentRow.tsx @@ -1,4 +1,5 @@ import { FC } from 'react' +import classNames from 'classnames' import { useHistory } from 'react-router-dom' import { useHasPermission } from 'common/providers/Permission' @@ -6,6 +7,7 @@ import { useHasPermission } from 'common/providers/Permission' import { Segment } from 'common/types/responses' import SegmentAction from './components/SegmentAction' import { SegmentMembershipTotalBadge } from 'components/segments/SegmentMembershipBadge' +import Chip from 'components/base/Chip' import ConfirmCloneSegment from 'components/modals/ConfirmCloneSegment' import { useCloneSegmentMutation } from 'common/services/useSegment' import { handleRemoveSegment } from 'components/modals/ConfirmRemoveSegment' @@ -19,7 +21,9 @@ interface SegmentRowProps { const SegmentRow: FC = ({ index, projectId, segment }) => { const history = useHistory() - const { description, feature, id, name } = segment + const { cohort, description, feature, id, name } = segment + + const isPendingDeletion = !!cohort?.deletion_requested_at const { permission: manageSegmentsPermission } = useHasPermission({ id: projectId, @@ -65,11 +69,18 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { } return ( - + history.push( `${document.location.pathname.replace(/\/$/, '')}/${id}`, @@ -82,6 +93,21 @@ const SegmentRow: FC = ({ index, projectId, segment }) => { {feature && (
Feature-Specific
)} + {!!cohort && ( + + {cohort.source_type.toUpperCase()} + + )} + {!!cohort && ( + + {cohort.environment_name} + + )} + {isPendingDeletion && ( + + Deleting + + )} @@ -91,13 +117,16 @@ const SegmentRow: FC = ({ index, projectId, segment }) => {
- + {!isPendingDeletion && ( + + )}
) diff --git a/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx b/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx index 15e60d87da5d..d5c894b30672 100644 --- a/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx +++ b/frontend/web/components/segments/SegmentRow/components/SegmentAction.tsx @@ -52,6 +52,7 @@ const SegmentAction: FC = ({ icon={} label='Clone Segment' handleActionClick={() => { + setIsOpen(false) onClone() }} entity='segment' @@ -64,6 +65,7 @@ const SegmentAction: FC = ({ icon={} label='Remove Segment' handleActionClick={() => { + setIsOpen(false) onRemove() }} entity='segment'