Skip to content

Commit f403b76

Browse files
HugoGresseclaude
HugoGresse
and
claude
committed
Address Copilot review on JSON import
- eventDataNormalization: skip non-object session/speaker entries so a malformed paste yields a validation result instead of crashing; fix parseEventJson error wording to match the actual (root-is-object) check. - importEventData: validate doc ids up front (reject empty or "/"- containing ids with a clear error), notify + rethrow on failure so the user gets feedback and the wizard does not redirect on a half import. - importEventData: use a type-only firebase import so the compat bundle is not pulled into the runtime admin chunk. - JsonImportApi: reword the comment so it no longer claims "read-only" (isReadOnly() returns false: the import yields an editable event). - Add project.importFail i18n (en/fr) and a unit test for invalid ids. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent cd8e8ba commit f403b76

7 files changed

Lines changed: 120 additions & 50 deletions

File tree

‎src/admin/project/core/actions/importEventData.spec.ts‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ const makeCollection = (collectionName: string) => ({
1616
doc: (docId: string) => ({ __collection: collectionName, __docId: docId }),
1717
})
1818

19+
vi.mock('../../../notification/notifcationActions', () => ({
20+
addNotification: (payload: unknown) => ({ type: 'NOTIFY', payload }),
21+
}))
22+
1923
vi.mock('../../../../firebase', () => ({
2024
serverTimestamp: () => 'SERVER_TS',
2125
fireStoreMainInstance: {
@@ -51,13 +55,12 @@ vi.mock('../../../../firebase', () => ({
5155

5256
import { importEventData } from './importEventData'
5357

54-
const run = (projectId: string, data: unknown) =>
55-
// The thunk ignores dispatch/getState; call with no-op deps.
58+
const run = (projectId: string, data: unknown, dispatch: unknown = () => {}) =>
5659
(
5760
importEventData(projectId, data as never) as (
5861
d: unknown
5962
) => Promise<unknown>
60-
)(() => {})
63+
)(dispatch)
6164

6265
describe('importEventData', () => {
6366
beforeEach(() => {
@@ -121,4 +124,24 @@ describe('importEventData', () => {
121124
expect(recordedOps).toHaveLength(0)
122125
expect(committedBatches).toBe(0)
123126
})
127+
128+
it('rejects ids containing a slash and notifies, without writing', async () => {
129+
const dispatch = vi.fn()
130+
await expect(
131+
run(
132+
'proj1',
133+
{
134+
sessions: { 'a/b': { id: 'a/b', title: 'Bad' } },
135+
speakers: {},
136+
},
137+
dispatch
138+
)
139+
).rejects.toThrow(/Invalid talk id/)
140+
141+
// No partial write happened and the user was notified.
142+
expect(recordedOps).toHaveLength(0)
143+
expect(dispatch).toHaveBeenCalledWith(
144+
expect.objectContaining({ type: 'NOTIFY' })
145+
)
146+
})
124147
})

‎src/admin/project/core/actions/importEventData.ts‎

Lines changed: 70 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
1-
import firebase from 'firebase/compat/app'
1+
import type firebase from 'firebase/compat/app'
22
import { fireStoreMainInstance, serverTimestamp } from '../../../../firebase'
33
import {
44
EventData,
55
normalizeEventData,
66
} from '../../../../core/setupType/eventDataNormalization'
7+
// @ts-expect-error - JS module without types
8+
import { addNotification } from '../../../notification/notifcationActions'
79

810
// Firestore caps a write batch at 500 operations; stay under it.
911
const BATCH_LIMIT = 450
@@ -18,6 +20,23 @@ interface WriteOp {
1820
data: Record<string, unknown>
1921
}
2022

23+
// A Firestore document id cannot be empty nor contain a slash (it would be
24+
// read as a sub-path). Validate up front so a bad id from the imported JSON
25+
// produces a clear error rather than throwing synchronously mid-import.
26+
const isValidDocId = (id: string): boolean =>
27+
typeof id === 'string' && id.length > 0 && !id.includes('/')
28+
29+
const assertValidIds = (ids: string[], kind: string): void => {
30+
const invalid = ids.filter((id) => !isValidDocId(id))
31+
if (invalid.length > 0) {
32+
throw new Error(
33+
`Invalid ${kind} id(s) (empty or containing "/"): ${invalid
34+
.slice(0, 5)
35+
.join(', ')}`
36+
)
37+
}
38+
}
39+
2140
const commitInBatches = async (ops: WriteOp[]): Promise<void> => {
2241
for (let i = 0; i < ops.length; i += BATCH_LIMIT) {
2342
const batch = fireStoreMainInstance.batch()
@@ -34,39 +53,60 @@ const commitInBatches = async (ops: WriteOp[]): Promise<void> => {
3453
// as a regular openfeedbackv1 event, so everything is editable afterwards.
3554
export const importEventData =
3655
(projectId: string, rawData: EventData) =>
37-
async (): Promise<ImportSummary> => {
38-
const data = normalizeEventData(rawData)
39-
const projectRef = fireStoreMainInstance
40-
.collection('projects')
41-
.doc(projectId)
56+
// eslint-disable-next-line @typescript-eslint/no-explicit-any
57+
async (dispatch: any): Promise<ImportSummary> => {
58+
try {
59+
const data = normalizeEventData(rawData)
60+
const projectRef = fireStoreMainInstance
61+
.collection('projects')
62+
.doc(projectId)
63+
64+
const speakers = Object.values(data.speakers || {})
65+
const sessions = Object.values(data.sessions || {})
4266

43-
const speakers = Object.values(data.speakers || {})
44-
const sessions = Object.values(data.sessions || {})
67+
assertValidIds(
68+
speakers.map((s) => s.id),
69+
'speaker'
70+
)
71+
assertValidIds(
72+
sessions.map((s) => s.id),
73+
'talk'
74+
)
4575

46-
const speakerOps: WriteOp[] = speakers.map((speaker) => ({
47-
ref: projectRef.collection('speakers').doc(speaker.id),
48-
data: {
49-
...speaker,
50-
createdAt: serverTimestamp(),
51-
updatedAt: serverTimestamp(),
52-
},
53-
}))
76+
const speakerOps: WriteOp[] = speakers.map((speaker) => ({
77+
ref: projectRef.collection('speakers').doc(speaker.id),
78+
data: {
79+
...speaker,
80+
createdAt: serverTimestamp(),
81+
updatedAt: serverTimestamp(),
82+
},
83+
}))
5484

55-
const talkOps: WriteOp[] = sessions.map((session) => ({
56-
ref: projectRef.collection('talks').doc(session.id),
57-
data: {
58-
...session,
59-
createdAt: serverTimestamp(),
60-
updatedAt: serverTimestamp(),
61-
},
62-
}))
85+
const talkOps: WriteOp[] = sessions.map((session) => ({
86+
ref: projectRef.collection('talks').doc(session.id),
87+
data: {
88+
...session,
89+
createdAt: serverTimestamp(),
90+
updatedAt: serverTimestamp(),
91+
},
92+
}))
6393

64-
// Speakers first so talk.speakers references resolve immediately.
65-
await commitInBatches(speakerOps)
66-
await commitInBatches(talkOps)
94+
// Speakers first so talk.speakers references resolve immediately.
95+
await commitInBatches(speakerOps)
96+
await commitInBatches(talkOps)
6797

68-
return {
69-
talkCount: talkOps.length,
70-
speakerCount: speakerOps.length,
98+
return {
99+
talkCount: talkOps.length,
100+
speakerCount: speakerOps.length,
101+
}
102+
} catch (err) {
103+
dispatch(
104+
addNotification({
105+
type: 'error',
106+
i18nkey: 'project.importFail',
107+
message: (err as Error).toString(),
108+
})
109+
)
110+
throw err
71111
}
72112
}

‎src/admin/translations/languages/en.admin.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@
4848
"saveSuccess": "Event saved",
4949
"saveFail": "Failed to save the event",
5050
"newSuccess": "New event created! Redirecting you now...",
51-
"newFail": "Fail to create a new event, "
51+
"newFail": "Fail to create a new event, ",
52+
"importFail": "The event was created but importing the JSON failed, "
5253
},
5354
"dashboard": {
5455
"comments": "Comments",

‎src/admin/translations/languages/fr.admin.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,8 @@
4848
"saveSuccess": "Évènement sauvegardé",
4949
"saveFail": "Erreur lors de la sauvegarde de l'évènement",
5050
"newSuccess": "Nouvel évènement créé! Redirection en cours...",
51-
"newFail": "Erreur lors de la création de l'évènement, "
51+
"newFail": "Erreur lors de la création de l'évènement, ",
52+
"importFail": "L'évènement a été créé mais l'import du JSON a échoué, "
5253
},
5354
"dashboard": {
5455
"comments": "Commentaires",

‎src/core/setupType/eventDataNormalization.spec.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,11 @@ import { normalizeEventData, parseEventJson } from './eventDataNormalization'
33

44
describe('normalizeEventData', () => {
55
it('coerces numeric session and speaker ids to strings', () => {
6+
// Numeric ids are common in real exports; cast to feed bad input.
67
const result = normalizeEventData({
7-
// @ts-expect-error - numeric ids are common in real data
88
sessions: { 2: { id: 2, title: 'Talk', speakers: ['s1'] } },
9-
// @ts-expect-error - numeric ids are common in real data
109
speakers: { s1: { id: 's1', name: 'Jane' } },
11-
})
10+
} as never)
1211

1312
expect(result.sessions?.['2'].id).toBe('2')
1413
expect(typeof result.sessions?.['2'].id).toBe('string')
@@ -20,9 +19,7 @@ describe('normalizeEventData', () => {
2019
sessions: {
2120
t1: {
2221
id: 't1',
23-
// @ts-expect-error - mixed array on purpose
2422
speakers: ['s1', 42, null, 's2'],
25-
// @ts-expect-error - mixed array on purpose
2623
tags: ['a', {}, 'b'],
2724
},
2825
},

‎src/core/setupType/eventDataNormalization.ts‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@ export interface EventData {
2424

2525
const isString = (value: unknown): value is string => typeof value === 'string'
2626

27+
const isObject = (value: unknown): value is Record<string, unknown> =>
28+
typeof value === 'object' && value !== null && !Array.isArray(value)
29+
2730
const filterStrings = (value: unknown): unknown =>
2831
Array.isArray(value) ? value.filter(isString) : value
2932

@@ -32,6 +35,11 @@ export const normalizeEventData = (
3235
): EventData => {
3336
const sessions: Record<string, SessionData> = {}
3437
Object.values(data?.sessions || {}).forEach((session) => {
38+
// Skip non-object entries (e.g. a stray string/number/null) so a
39+
// malformed paste surfaces as a validation result, not a crash.
40+
if (!isObject(session)) {
41+
return
42+
}
3543
const id = '' + session.id
3644
sessions[id] = {
3745
...session,
@@ -43,6 +51,9 @@ export const normalizeEventData = (
4351

4452
const speakers: Record<string, SpeakerData> = {}
4553
Object.values(data?.speakers || {}).forEach((speaker) => {
54+
if (!isObject(speaker)) {
55+
return
56+
}
4657
const id = '' + speaker.id
4758
speakers[id] = {
4859
...speaker,
@@ -75,14 +86,10 @@ export const parseEventJson = (text: string): ParseResult => {
7586
} catch (e) {
7687
return { data: null, error: (e as Error).message }
7788
}
78-
if (
79-
typeof parsed !== 'object' ||
80-
parsed === null ||
81-
Array.isArray(parsed)
82-
) {
89+
if (!isObject(parsed)) {
8390
return {
8491
data: null,
85-
error: 'The JSON must be an object with "sessions" and "speakers" keys.',
92+
error: 'The JSON must be an object (with "sessions" and "speakers" keys).',
8693
}
8794
}
8895
return { data: normalizeEventData(parsed as EventData), error: null }

‎src/core/setupType/jsonimport/JsonImportApi.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
import { EventData } from '../eventDataNormalization'
22

3-
// Read-only API over an in-memory, already-normalized JSON event blob.
4-
// Exposes the same getTalks()/getSpeakers() surface the setup validation
5-
// (src/core/setupType/validation.js) expects, so an uploaded/pasted JSON
6-
// file can be validated exactly like a fetched JSON URL.
3+
// Thin API over an in-memory, already-normalized JSON event blob. Exposes the
4+
// getTalks()/getSpeakers() surface the setup validation
5+
// (src/core/setupType/validation.js) expects, so an uploaded/pasted JSON file
6+
// can be validated exactly like a fetched JSON URL. isReadOnly() returns false
7+
// because the import produces a normal, editable openfeedbackv1 event.
78
class JsonImportApi {
89
data: EventData
910

0 commit comments

Comments
 (0)