diff --git a/functions/levante-admin/README_DISAPPEARING_ASSIGNMENTS.md b/functions/levante-admin/README_DISAPPEARING_ASSIGNMENTS.md new file mode 100644 index 0000000..0e9a16e --- /dev/null +++ b/functions/levante-admin/README_DISAPPEARING_ASSIGNMENTS.md @@ -0,0 +1,67 @@ +# Fix disappearing assignments + +## Issue summary + +Some assignment creation flows appeared to succeed in the UI, but one or more user assignment documents were missing under: + +- `users/{uid}/assignments/{administrationId}` + +This was reported as "disappearing assignments". + +## What we found + +Two different behaviors were mixed together: + +1. Expected non-assignment due to conditions + - Some administrations intentionally assign only users that match an `assigned` condition (for example `userType == student`). + - In those cases, parent/teacher users are correctly not assigned. + +2. Real backend risk in async fan-out + - Assignment fan-out runs asynchronously in `updateAssignmentsForOrgChunk`. + - If the payload includes undefined values or malformed org arrays, Firestore writes can fail and prevent some assignment docs from being written. + +## Why this can look like a UI success + +Administration creation and assignment fan-out are separate phases: + +1. administration doc is created/updated +2. fan-out tasks are enqueued and processed later + +If phase 1 succeeds and phase 2 partially fails, users can see successful creation while assignment docs are incomplete. + +## Changes in this fix + +### 1) Harden upsert payloads (`src/upsertAdministration.ts`) + +- Normalize org ID arrays (`districts`, `schools`, `classes`, `groups`) to non-empty string IDs only. +- Strip undefined recursively from assessments and legal fields. +- Normalize assessment `params` to an object. +- Validate required assessment fields (`taskId`, `variantId`, `variantName`). +- Reject invalid dates explicitly. +- Ensure update payload preserves a valid `createdBy` value. + +These changes reduce malformed data entering downstream fan-out. + +### 2) Harden assignment writes (`src/assignments/assignment-utils.ts`) + +- Use cleaned assessments when building assignment docs. +- Sanitize assignment documents before `transaction.set(...)` in both add and update flows. + +This prevents transaction failures caused by nested undefined fields. + +### 3) Improve recursive sanitizer (`src/utils/utils.ts`) + +- `removeUndefinedFields()` now recursively cleans nested arrays, not only top-level array items. + +This closes a gap where undefined values inside array objects could survive and break Firestore writes. + +## Why this fixes disappearing assignments + +The core failure mode was bad shape/undefined data reaching Firestore write paths in async fan-out. +By normalizing and validating input early and sanitizing assignment payloads immediately before write, task execution is significantly less likely to fail partway through chunk processing. + +## Operational recommendation + +- Monitor `updateAssignmentsForOrgChunk` error rates and alert on repeated payload-shape failures. +- Keep an audit test that compares expected eligible users against actual assignment docs. + diff --git a/functions/levante-admin/src/assignments/assignment-utils.ts b/functions/levante-admin/src/assignments/assignment-utils.ts index 91a510a..1523e0f 100644 --- a/functions/levante-admin/src/assignments/assignment-utils.ts +++ b/functions/levante-admin/src/assignments/assignment-utils.ts @@ -244,7 +244,7 @@ const prepareNewAssignment = async ( sequential: administrationData.sequential ?? false, assigningOrgs: usersAssigningOrgs, readOrgs: userReadOrgs, - assessments, + assessments: cleanedAssessments, progress, userData: userDataCopy, testData: administrationData.testData ?? false, @@ -287,10 +287,13 @@ export const addAssignmentToUsers = async ( return _map(assignments, ([assignmentRef, assignmentData]) => { if (assignmentRef && assignmentData) { + const cleanedAssignmentData = removeUndefinedFields(assignmentData); logger.debug(`Adding new assignment at ${assignmentRef.path}`, { - assignmentSummary: summarizeAssignmentForLog(assignmentData), + assignmentSummary: summarizeAssignmentForLog(cleanedAssignmentData), + }); + return transaction.set(assignmentRef, cleanedAssignmentData, { + merge: true, }); - return transaction.set(assignmentRef, assignmentData, { merge: true }); } else { return transaction; } @@ -668,10 +671,13 @@ export const updateAssignmentForUsers = async ( return _map(assignments, ([assignmentRef, assignmentData]) => { if (assignmentRef && assignmentData) { + const cleanedAssignmentData = removeUndefinedFields(assignmentData); logger.info(`Updating or creating assignment ${assignmentRef.path}`, { - assignmentSummary: summarizeAssignmentForLog(assignmentData), + assignmentSummary: summarizeAssignmentForLog(cleanedAssignmentData), + }); + return transaction.set(assignmentRef, cleanedAssignmentData, { + merge: true, }); - return transaction.set(assignmentRef, assignmentData, { merge: true }); } else if (assignmentRef) { return transaction.delete(assignmentRef); } else { diff --git a/functions/levante-admin/src/upsertAdministration.ts b/functions/levante-admin/src/upsertAdministration.ts index a2b0e04..257395e 100644 --- a/functions/levante-admin/src/upsertAdministration.ts +++ b/functions/levante-admin/src/upsertAdministration.ts @@ -8,6 +8,7 @@ import { HttpsError } from "firebase-functions/v2/https"; import { logger } from "firebase-functions/v2"; import type { IAssessment, IOrgsList } from "./interfaces.js"; // Assuming necessary types/helpers are in common import type { Class, Group, School } from "../firestore-schema.js"; +import { removeUndefinedFields } from "./utils/utils.js"; interface UpsertAdministrationData { name: string; @@ -51,6 +52,39 @@ interface IAdministrationDoc { creatorName: string; } +const normalizeIdList = (ids: unknown): string[] => { + if (!Array.isArray(ids)) return []; + return [...new Set(ids)] + .filter((id): id is string => typeof id === "string") + .map((id) => id.trim()) + .filter((id) => id.length > 0); +}; + +const normalizeOrgs = (orgs?: IOrgsList): Required => { + return { + districts: normalizeIdList(orgs?.districts), + schools: normalizeIdList(orgs?.schools), + classes: normalizeIdList(orgs?.classes), + groups: normalizeIdList(orgs?.groups), + }; +}; + +const normalizeAssessments = (assessments: IAssessment[]): IAssessment[] => { + return assessments.map((assessment) => { + const cleanedAssessment = removeUndefinedFields(assessment) as IAssessment; + const params = + cleanedAssessment.params && + typeof cleanedAssessment.params === "object" && + !Array.isArray(cleanedAssessment.params) + ? cleanedAssessment.params + : {}; + return { + ...cleanedAssessment, + params, + }; + }); +}; + export const upsertAdministrationHandler = async ( callerAdminUid: string, data: UpsertAdministrationData @@ -67,12 +101,7 @@ export const upsertAdministrationHandler = async ( dateOpen, dateClose, sequential = true, - orgs = { - districts: [], - schools: [], - classes: [], - groups: [], - }, + orgs, tags = [], administrationId, isTestData = false, @@ -80,10 +109,19 @@ export const upsertAdministrationHandler = async ( creatorName, } = data as UpsertAdministrationData; + const normalizedOrgs = normalizeOrgs(orgs); + const rawAssessments = Array.isArray(assessments) ? assessments : []; + const cleanedAssessments = normalizeAssessments( + removeUndefinedFields(rawAssessments) as IAssessment[] + ); + const cleanedLegal = removeUndefinedFields(legal ?? {}); + const cleanedTags = normalizeIdList(tags); + if ( !name || - !assessments || - !Array.isArray(assessments) || + !cleanedAssessments || + !Array.isArray(cleanedAssessments) || + cleanedAssessments.length === 0 || !dateOpen || !dateClose ) { @@ -93,11 +131,33 @@ export const upsertAdministrationHandler = async ( ); } + const hasInvalidAssessment = cleanedAssessments.some( + (assessment) => + typeof assessment.taskId !== "string" || + !assessment.taskId.trim() || + typeof assessment.variantId !== "string" || + !assessment.variantId.trim() || + typeof assessment.variantName !== "string" || + !assessment.variantName.trim() + ); + if (hasInvalidAssessment) { + throw new HttpsError( + "invalid-argument", + "Assessments must include taskId, variantId, and variantName." + ); + } + let dateOpenedTs: Timestamp; let dateClosedTs: Timestamp; try { const dateOpenObj = new Date(dateOpen); const dateCloseObj = new Date(dateClose); + if ( + Number.isNaN(dateOpenObj.getTime()) || + Number.isNaN(dateCloseObj.getTime()) + ) { + throw new Error("Invalid date"); + } dateOpenedTs = Timestamp.fromDate(dateOpenObj); dateClosedTs = Timestamp.fromDate(dateCloseObj); @@ -134,43 +194,44 @@ export const upsertAdministrationHandler = async ( `Administration with ID ${administrationId} not found for update.` ); } + const existingData = existingDoc.data() as Partial; // Prepare data for update (merge: true will handle partial updates) - const updateData: Partial = { + const updateData: Partial = removeUndefinedFields({ // Use Partial for updates name, publicName: publicName ?? name, normalizedName, - // createdBy should not be updated - groups: orgs.groups ?? [], - classes: orgs.classes ?? [], - schools: orgs.schools ?? [], - districts: orgs.districts ?? [], + createdBy: existingData.createdBy ?? callerAdminUid, + groups: normalizedOrgs.groups, + classes: normalizedOrgs.classes, + schools: normalizedOrgs.schools, + districts: normalizedOrgs.districts, // dateCreated should not be updated dateOpened: dateOpenedTs, dateClosed: dateClosedTs, - assessments: assessments, + assessments: cleanedAssessments, sequential: sequential, - tags: tags, - legal: legal, + tags: cleanedTags, + legal: cleanedLegal, testData: isTestData ?? false, // Explicitly construct org lists for update readOrgs: { // Re-enabled - districts: orgs.districts ?? [], - schools: orgs.schools ?? [], - classes: orgs.classes ?? [], - groups: orgs.groups ?? [], + districts: normalizedOrgs.districts, + schools: normalizedOrgs.schools, + classes: normalizedOrgs.classes, + groups: normalizedOrgs.groups, }, minimalOrgs: { // Re-enabled - districts: orgs.districts ?? [], - schools: orgs.schools ?? [], - classes: orgs.classes ?? [], - groups: orgs.groups ?? [], + districts: normalizedOrgs.districts, + schools: normalizedOrgs.schools, + classes: normalizedOrgs.classes, + groups: normalizedOrgs.groups, }, updatedAt: FieldValue.serverTimestamp() as Timestamp, - }; + }); // --- Write 1 (Update Path) --- Update administration doc using transaction.update() transaction.update(administrationDocRef, updateData); // Switched from set with merge to update @@ -186,30 +247,30 @@ export const upsertAdministrationHandler = async ( const siteId = data.siteId; // Prepare Administration Data for creation - const administrationData: IAdministrationDoc = { + const administrationData: IAdministrationDoc = removeUndefinedFields({ name, publicName: publicName ?? name, normalizedName, createdBy: callerAdminUid, - creatorName: creatorName, - groups: orgs.groups ?? [], - classes: orgs.classes ?? [], - schools: orgs.schools ?? [], - districts: orgs.districts ?? [], + creatorName: creatorName ?? "", + groups: normalizedOrgs.groups, + classes: normalizedOrgs.classes, + schools: normalizedOrgs.schools, + districts: normalizedOrgs.districts, dateCreated: FieldValue.serverTimestamp() as Timestamp, dateOpened: dateOpenedTs, dateClosed: dateClosedTs, - assessments: assessments, + assessments: cleanedAssessments, sequential: sequential, - tags: tags, - legal: legal, + tags: cleanedTags, + legal: cleanedLegal, testData: isTestData ?? false, - readOrgs: orgs, - minimalOrgs: orgs, + readOrgs: normalizedOrgs, + minimalOrgs: normalizedOrgs, siteId, createdAt: FieldValue.serverTimestamp() as Timestamp, updatedAt: FieldValue.serverTimestamp() as Timestamp, - }; + }); // --- Write 1 (Create Path) --- Create administration doc transaction.set(administrationDocRef, administrationData); // Use set without merge for creation diff --git a/functions/levante-admin/src/utils/utils.ts b/functions/levante-admin/src/utils/utils.ts index 4f34529..d2ad7b6 100644 --- a/functions/levante-admin/src/utils/utils.ts +++ b/functions/levante-admin/src/utils/utils.ts @@ -196,7 +196,9 @@ export const doesDocExist = async (docRef, transaction) => { */ export const removeUndefinedFields = (obj: any): any => { if (Array.isArray(obj)) { - return _without(obj, undefined); + return obj + .map((value) => removeUndefinedFields(value)) + .filter((value) => value !== undefined); } else if (obj && typeof obj === "object") { return Object.entries(obj).reduce((acc, [key, value]) => { const cleanedValue = removeUndefinedFields(value);