diff --git a/cli/src/pr-review.js b/cli/src/pr-review.js index 9bf4ce787..78a80ee09 100755 --- a/cli/src/pr-review.js +++ b/cli/src/pr-review.js @@ -39,6 +39,14 @@ function isGeneratedDependencyManifest(path) { return /(^|\/)(package-lock|npm-shrinkwrap)\.json$/.test(path); } +function isLocaleJsonFile(path) { + // Locale JSON dictionaries are DATA (parallel key -> translation maps), not + // application logic: translated strings legitimately repeat across locale + // dictionaries (e.g. es_AR and es_ES sharing the same neutral phrasing) — + // DRY/extract-shared-logic rules apply to logic, not to data. + return /\/locales\/[^/]+\.json$/.test(path); +} + function normalizeDuplicateLine(line) { const trimmed = line.trim(); if (!trimmed) return ''; @@ -171,6 +179,7 @@ export function detectDuplicatedBlocks(diffText, { minLines = DUPLICATE_MIN_LINE // rules do not apply, same as the artifacts/ skip above. if (run.path.startsWith('cli/cache/')) continue; if (isGeneratedDependencyManifest(run.path)) continue; + if (isLocaleJsonFile(run.path)) continue; const normalizedLines = run.lines .map((entry) => ({ ...entry, normalized: normalizeDuplicateLine(entry.text) })) .filter((entry) => entry.normalized); @@ -313,7 +322,7 @@ function isExemptFromSizeGate(path) { return true; } // Locale JSON files grow predictably as the app is translated — skip them. - if (/\/locales\/[^/]+\.json$/.test(path)) { + if (isLocaleJsonFile(path)) { return true; } // Generated contract snapshots are intentionally verbose and are checked diff --git a/cli/test/pr-review.test.js b/cli/test/pr-review.test.js index f39be5bf8..85f7ccbec 100644 --- a/cli/test/pr-review.test.js +++ b/cli/test/pr-review.test.js @@ -140,6 +140,53 @@ describe('detectDuplicatedBlocks', () => { ['tools/app-shell/src/foo.js', 'tools/app-shell/src/bar.js'], ); }); + + it('ignores duplicated blocks in locale JSON dictionaries', () => { + const block = [ + '+ "csvImport": "Importar",', + '+ "csvImportEditCorrespondence": "Editar correspondencia",', + '+ "csvImportReviewQueue": "Cola de revisión",', + '+ "csvImportApplyToAll": "Aplicar a todos",', + '+ "csvImportRevalidate": "Revalidar",', + '+ "csvImportDiscardRow": "Descartar fila"', + ].join('\n'); + + const diff = [ + makeDiff('tools/app-shell/src/locales/es_AR.json', block), + makeDiff('tools/app-shell/src/locales/es_ES.json', block), + ].join('\n'); + + assert.deepEqual(detectDuplicatedBlocks(diff), []); + }); + + it('still flags the same duplicated block when it lands in real source (locale skip is path-scoped, not a blanket disable)', () => { + const block = [ + '+ "csvImport": "Importar",', + '+ "csvImportEditCorrespondence": "Editar correspondencia",', + '+ "csvImportReviewQueue": "Cola de revisión",', + '+ "csvImportApplyToAll": "Aplicar a todos",', + '+ "csvImportRevalidate": "Revalidar",', + '+ "csvImportDiscardRow": "Descartar fila"', + ].join('\n'); + + // The locale JSON copy is silently dropped; the two real source copies must still + // collide. This proves the skip only removes locales/ runs from the comparison, + // rather than disabling duplication detection for the whole diff. + const diff = [ + makeDiff('tools/app-shell/src/locales/es_AR.json', block), + makeDiff('tools/app-shell/src/foo.js', block), + makeDiff('tools/app-shell/src/bar.js', block), + ].join('\n'); + + const findings = detectDuplicatedBlocks(diff); + assert.equal(findings.length, 1); + assert.equal(findings[0].code, 'DUPLICATED_BLOCK'); + assert.equal(findings[0].severity, 'blocker'); + assert.deepEqual( + findings[0].locations.map((location) => location.path), + ['tools/app-shell/src/foo.js', 'tools/app-shell/src/bar.js'], + ); + }); }); describe('analyzeChangedFiles', () => { diff --git a/packages/app-shell-core/package.json b/packages/app-shell-core/package.json index b8d9eb808..2d327918d 100644 --- a/packages/app-shell-core/package.json +++ b/packages/app-shell-core/package.json @@ -39,7 +39,7 @@ "!src/**/__tests__/**" ], "scripts": { - "test": "node --test test/*.test.js src/auth/__tests__/*.test.js src/i18n/__tests__/*.test.js src/runtime/__tests__/*.test.js src/theme/__tests__/*.test.js src/components/ui/__tests__/*.test.js src/lib/__tests__/*.test.js src/lib/validation/__tests__/*.test.js src/lib/import/__tests__/*.test.js", + "test": "node --test test/*.test.js src/auth/__tests__/*.test.js src/i18n/__tests__/*.test.js src/runtime/__tests__/*.test.js src/theme/__tests__/*.test.js src/components/ui/__tests__/*.test.js src/lib/__tests__/*.test.js src/lib/validation/__tests__/*.test.js src/lib/import/__tests__/*.test.js src/lib/csv/__tests__/*.test.js", "test:vitest": "vitest run", "test:consumer": "node scripts/package-consumer-smoke.mjs" }, diff --git a/packages/app-shell-core/src/components/import/ImportDialog.jsx b/packages/app-shell-core/src/components/import/ImportDialog.jsx index d6bd60ec9..353b5c268 100644 --- a/packages/app-shell-core/src/components/import/ImportDialog.jsx +++ b/packages/app-shell-core/src/components/import/ImportDialog.jsx @@ -18,7 +18,38 @@ import { buildOperations } from '../../lib/import/buildOperations.js'; import { runImport, sendRow, SEND_STATUS } from '../../lib/import/importEngine.js'; import { buildTemplateCsv } from '../../lib/import/buildTemplateCsv.js'; -const DEFAULT_LABELS = { title: 'Import', revalidating: 'Revalidating rows…', downloadTemplate: 'Download CSV template' }; +// Root-level labels for ImportDialog's own chrome. `importButton` is a function of the +// valid-row count, mirroring the (n) => string labels the confirm step already uses, so the +// whole flow's button text is translatable rather than the hardcoded `Import ${n}` it was. +const DEFAULT_LABELS = { title: 'Import', revalidating: 'Revalidating rows…', downloadTemplate: 'Download CSV template', importButton: (n) => `Import ${n}` }; + +/** + * Shape of the optional `labels` prop — a NESTED object: root-level keys for this dialog's + * own chrome, plus one sub-slice per child component, each matching that child's own + * DEFAULT_LABELS. Every level is optional; any omitted key/slice falls back to the child's + * hardcoded English DEFAULT_LABELS. The functional app (etendo_schema_forge's ListView) + * builds this object from its useUI() dictionary and MUST match this shape exactly. + * + * { + * title, revalidating, downloadTemplate, importButton: (n) => string, // this dialog + * dropzone: { dropHere, dropHint }, // ImportDropzone + * progress: { title, subtitle }, // ImportProgressStep + * mapping: { notImported, mappedSummary, editMatch, editTitle, save, cancel }, // ImportColumnMapping + * confirm: { title, willImport: (n) => string, willSkip: (n) => string, cancel, confirm }, // ImportConfirmStep + * fileError: { title, cancel, retry }, // ImportFileErrorDialog + * reviewQueue: { filterAll, filterOk, filterError, skip, skipped, unskip, downloadErrors, + * status, statusOk, statusError, fieldErrorsTooltip, bulkApplyTitle, + * bulkApplyDescription, bulkApplyOnlyThis, bulkApplyAll, retry }, // ImportReviewQueue + * // NB: `retry` feeds ImportReviewQueue's separate `retryLabel` prop, not its DEFAULT_LABELS + * systemError: { title, subtitle, copy, copied, copyFailed, close, showReport, hideReport, + * rowData, requestSent, serverResponse }, // ImportSystemErrorDialog + * } + * + * Templated strings (mappedSummary's {mapped}/{total}, fieldErrorsTooltip's {fields}, + * bulkApplyDescription's {count}/{raw}/{value}) keep their {placeholders} — each child fills + * them at render time. `translate` is a separate, plain (key, params) => string function + * (e.g. useUI's `ui`) injected into the send pipeline so backend errors get localized too. + */ const STEP = { DROPZONE: 'dropzone', MAPPING: 'mapping', CONFIRM: 'confirm', SENDING: 'sending', FILE_ERROR: 'fileError', RESULT: 'result' }; @@ -40,7 +71,7 @@ function renameRowKeys(row, mapping) { return renamed; } -export function ImportDialog({ open, onOpenChange, config, token, postBatch, simSearchFn, onImported, labels }) { +export function ImportDialog({ open, onOpenChange, config, token, postBatch, simSearchFn, onImported, labels, translate }) { const text = { ...DEFAULT_LABELS, ...labels }; const [step, setStep] = useState(STEP.DROPZONE); const [fileErrorMessage, setFileErrorMessage] = useState(null); @@ -98,13 +129,17 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim // which made `simSearch`'s own guard clause short-circuit to "no match" for every row // instead of actually querying — the descriptor then threw "country could not be // resolved" for every address-bearing row (confirmed via a real browser run). + // `translate` is threaded into the descriptor config too — a composite descriptor (e.g. + // Contacts) throws its own row-level errors (an unresolved country FK) and needs the app's + // translate fn to localize them, exactly like the send pipeline does for backend errors. const operationsConfig = useMemo(() => ({ spec: config.spec, entity: config.entity, descriptorName: config.descriptor, targets: config.fields.map((f) => f.target), token, - }), [config.spec, config.entity, config.descriptor, config.fields, token]); + translate, + }), [config.spec, config.entity, config.descriptor, config.fields, token, translate]); // The real config shape (decisions.json → window.import, verified against // artifacts/contacts/decisions.json) is `dedupe: { scope, key: string[] }`, not a flat @@ -240,6 +275,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim ({ results } = await runImport(toSend.map((e) => e.row), { buildRowOperations: (row) => buildOperations(row, operationsConfig), postBatch, + translate, concurrency: config.concurrency, maxRows: config.maxRows, onProgress: (completed, total) => setProgress(Math.round((completed / total) * 100)), @@ -251,7 +287,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim } const okCount = results.filter((r) => r.status === 'ok').length; // A DUPLICATE result (the row's record already exists server-side, a unique-constraint - // rejection — see importEngine.js's isDuplicateKeyError) is not an actionable failure: + // rejection — see importEngine.js's classifyImportError) is not an actionable failure: // retrying would only repeat the identical rejection, and there's nothing for the user // to fix. Reported as a skipped entry (same treatment the pre-send in-file dedupe // already uses — greyed out, no retry/skip buttons, per ImportReviewQueue) rather than @@ -289,12 +325,12 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim onImported({ okCount, failedCount: trueFailures.length }); if (okCount > 0) toast.success(`${okCount} records imported successfully`); if (duplicateResults.length > 0) toast.info(`${duplicateResults.length} row(s) skipped — already exist`); - }, [entries, operationsConfig, config.concurrency, config.maxRows, postBatch, onImported]); + }, [entries, operationsConfig, config.concurrency, config.maxRows, postBatch, onImported, translate]); const handleRetryEntryPostSend = useCallback(async (index) => { const entry = entries[index]; const operations = await buildOperations(entry.row, operationsConfig); - const result = await sendRow(operations, { postBatch }); + const result = await sendRow(operations, { postBatch, translate }); setEntries((prev) => { const next = [...prev]; if (result.status === 'ok') { @@ -311,7 +347,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim if (result.status !== 'ok' && result.status !== SEND_STATUS.DUPLICATE) { setSystemError({ message: result.error?.message || 'Unknown error', raw: result.error?.raw, row: entry.row, operations }); } - }, [entries, operationsConfig, postBatch]); + }, [entries, operationsConfig, postBatch, translate]); const handleRetryFile = useCallback(() => { setFileErrorMessage(null); @@ -328,7 +364,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim {step === STEP.DROPZONE && (
- +
@@ -399,10 +438,11 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim skipCount={skipCount} onCancel={() => setStep(STEP.MAPPING)} onConfirm={handleSend} + labels={labels?.confirm} data-testid="ImportConfirmStep__38a6c3" /> )} - {step === STEP.SENDING && } + {step === STEP.SENDING && } {step === STEP.RESULT && (
@@ -418,7 +458,8 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim onUnskipEntry={handleUnskipEntry} onApplyFkValue={handleApplyFkValue} onDownloadErrors={() => downloadCsv(buildErrorsCsv(entries, headers, mapping), 'import-errors.csv')} - retryLabel="Retry" + retryLabel={labels?.reviewQueue?.retry ?? 'Retry'} + labels={labels?.reviewQueue} simSearchFn={simSearchFn} token={token} data-testid="ImportReviewQueue__38a6c3" /> @@ -430,7 +471,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim disabled={validCount === 0} data-testid="ImportDialog__importButton" > - {`Import ${validCount}`} + {text.importButton(validCount)}
@@ -444,6 +485,7 @@ export function ImportDialog({ open, onOpenChange, config, token, postBatch, sim operations={systemError?.operations} raw={systemError?.raw} onClose={() => setSystemError(null)} + labels={labels?.systemError} data-testid="ImportSystemErrorDialog__38a6c3" /> ); diff --git a/packages/app-shell-core/src/components/import/ImportReviewQueue.jsx b/packages/app-shell-core/src/components/import/ImportReviewQueue.jsx index 5111ac67a..c09c0f4a8 100644 --- a/packages/app-shell-core/src/components/import/ImportReviewQueue.jsx +++ b/packages/app-shell-core/src/components/import/ImportReviewQueue.jsx @@ -7,6 +7,7 @@ import { Popover, PopoverTrigger, PopoverContent } from '../ui/popover.jsx'; import { Command, CommandInput, CommandList, CommandEmpty, CommandGroup, CommandItem } from '../ui/command.jsx'; import { Dialog, DialogContent, DialogHeader, DialogFooter, DialogTitle, DialogDescription } from '../ui/dialog.jsx'; import { ScrollPane } from '../ui/scroll-pane.jsx'; +import { csvField } from '../../lib/csv/csvSerializer.js'; const DEFAULT_LABELS = { filterAll: 'All', @@ -79,11 +80,6 @@ const STATUS_TAG_COLORS = { neutral: 'bg-[#F5F7F9] text-[#3F3F50]', }; -function csvEscape(value) { - const s = String(value ?? ''); - return /[",\n]/.test(s) ? `"${s.replace(/"/g, '""')}"` : s; -} - /** Line number + status pill, shared by every row in the frozen leading column. */ function StatusLineTag({ index, tag, children }) { return ( @@ -266,12 +262,12 @@ function FkMismatchCell({ index, field, value, error, onSelect, simSearchFn, tok */ export function buildErrorsCsv(entries, headers, mapping) { const mappedHeaders = headers.filter((h) => mapping[h]); - const lines = [[...mappedHeaders, 'Error'].map(csvEscape).join(',')]; + const lines = [[...mappedHeaders, 'Error'].map(csvField).join(',')]; for (const entry of entries) { if (entry.errors.length === 0) continue; const values = mappedHeaders.map((h) => entry.row[mapping[h]]); const errorText = entry.errors.map((e) => (e.target ? `${e.target}: ${e.message}` : e.message)).join(' | '); - lines.push([...values, errorText].map(csvEscape).join(',')); + lines.push([...values, errorText].map(csvField).join(',')); } return lines.join('\n'); } diff --git a/packages/app-shell-core/src/components/import/__tests__/ImportDialog.labels.test.jsx b/packages/app-shell-core/src/components/import/__tests__/ImportDialog.labels.test.jsx new file mode 100644 index 000000000..8ecda1a28 --- /dev/null +++ b/packages/app-shell-core/src/components/import/__tests__/ImportDialog.labels.test.jsx @@ -0,0 +1,209 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, cleanup } from '@testing-library/react'; + +// ETP-4669: ImportDialog is the single point that fans the nested `labels` prop out to every +// child of the import flow. These tests mock each child so we can assert the EXACT sub-slice +// each one receives (by object identity), not merely that something rendered — a mis-wiring +// (e.g. forwarding `labels.confirm` where `labels.mapping` belongs) would slip past a +// render-only assertion but is caught here. The root-level chrome (title, downloadTemplate, +// importButton) is asserted through the real DOM, since ImportDialog renders it itself. + +const captured = vi.hoisted(() => ({})); + +vi.mock('../ImportDropzone.jsx', () => ({ + ImportDropzone: (props) => { + captured.dropzone = props; + return ( +
+
+ ); + }, +})); + +vi.mock('../ImportColumnMapping.jsx', () => ({ + ImportColumnMapping: (props) => { + captured.mapping = props; + return
; + }, +})); + +// ImportDialog imports both ImportReviewQueue and buildErrorsCsv from this module — the mock +// must preserve both named exports or the import itself breaks. +vi.mock('../ImportReviewQueue.jsx', () => ({ + buildErrorsCsv: () => '', + ImportReviewQueue: (props) => { + captured.reviewQueue = props; + return
; + }, +})); + +vi.mock('../ImportConfirmStep.jsx', () => ({ + ImportConfirmStep: (props) => { + captured.confirm = props; + return