Refactor keying and queries for external entities and CSV invite meta#822
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughSwitches external-entity queries and mapping to use metaInformation.externalId and builds composite keys from normalized externalId + entityType; CSV meta lookups now use composite keys of normalized value + field name (and subrole-specific composite keys) for ID resolution. No public APIs changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Utils as utils.fetchAndMapAllExternalEntities
participant API as External Entities API
Caller->>Utils: fetchAndMapAllExternalEntities(entities, tenantCode)
Utils->>Utils: Build query {metaInformation.externalId: {$in: entities}, tenantId: tenantCode}
Utils->>API: GET /entities?query=<query>&projection=...
API-->>Utils: 200 OK [entities]
Utils->>Utils: For each entity: normalize(externalId) + normalize(entityType) => compositeKey
Utils-->>Caller: Map<compositeKey, entity>
sequenceDiagram
autonumber
actor Uploader as CSV Uploader
participant Helper as helpers.extractDataFromCSV
participant Map as externalEntityNameIdMap
Uploader->>Helper: Process CSV row (meta fields)
Helper->>Helper: Normalize values (lowercase, trim, remove spaces)
Helper->>Map: Lookup block: "<value>block"
Helper->>Map: Lookup state: "<value>state"
Helper->>Map: Lookup school: "<value>school"
Helper->>Map: Lookup cluster: "<value>cluster"
Helper->>Map: Lookup district: "<value>district"
alt professional_role present
Helper->>Map: Lookup "<value>professional_role"
end
loop professional_subroles present
Helper->>Map: Lookup "<subrole>professional_subroles"
Helper->>Helper: Collect resolved subrole IDs
end
Helper-->>Uploader: row.meta with resolved IDs or fallbacks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/generics/utils.js (1)
795-800: Normalize input and guard empty queries before calling external serviceSanitize entity names (trim, drop empties) and avoid querying when the array is empty or tenantCode is falsy. This reduces 4xx/empty-result churn and payload size.
Apply:
- let query = { - 'metaInformation.name': { - $in: entities, // Dynamically pass the array here - }, - tenantId: tenantCode, - } + const names = + (Array.isArray(entities) ? entities : []) + .map((v) => (typeof v === 'string' ? v.trim() : v)) + .filter((v) => typeof v === 'string' && v.length > 0) + + if (!tenantCode || names.length === 0) { + return {} + } + + const query = { + 'metaInformation.name': { $in: names }, + tenantId: tenantCode, + }Optional (if lists can be large): chunk names in batches (e.g., 500–1000) and merge results to avoid upstream size/time limits.
src/helpers/userInvite.js (2)
276-316: Centralize composite-key generation and use consistent null semantics
- Repeated string munging is brittle; extract a helper to ensure identical normalization as utils.js.
- Use null (not '') when the field is absent or lookup fails, so downstream “value !== null” checks behave as intended.
- Trim incoming CSV values before normalization.
Apply these line edits (uses a helper shown below):
- block: row?.block - ? externalEntityNameIdMap?.[ - `${row.block?.replaceAll(/\s+/g, '').toLowerCase()}${'block' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || null - : '', + block: row?.block + ? externalEntityNameIdMap?.[compositeKey(row.block, 'block')]?._id ?? null + : null, - state: row?.state - ? externalEntityNameIdMap?.[ - `${row.state?.replaceAll(/\s+/g, '').toLowerCase()}${'state' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || null - : '', + state: row?.state + ? externalEntityNameIdMap?.[compositeKey(row.state, 'state')]?._id ?? null + : null, - school: row?.school - ? externalEntityNameIdMap?.[ - `${row.school?.replaceAll(/\s+/g, '').toLowerCase()}${'school' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || null - : '', + school: row?.school + ? externalEntityNameIdMap?.[compositeKey(row.school, 'school')]?._id ?? null + : null, - cluster: row?.cluster - ? externalEntityNameIdMap?.[ - `${row.cluster?.replaceAll(/\s+/g, '').toLowerCase()}${'cluster' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || null - : '', + cluster: row?.cluster + ? externalEntityNameIdMap?.[compositeKey(row.cluster, 'cluster')]?._id ?? null + : null, - district: row?.district - ? externalEntityNameIdMap?.[ - `${row.district?.replaceAll(/\s+/g, '').toLowerCase()}${'district' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || null - : '', + district: row?.district + ? externalEntityNameIdMap?.[compositeKey(row.district, 'district')]?._id ?? null + : null, - professional_role: row?.professional_role - ? externalEntityNameIdMap?.[ - `${row.professional_role?.replaceAll(/\s+/g, '').toLowerCase()}${'professional_role' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id || '' - : '', + professional_role: row?.professional_role + ? externalEntityNameIdMap?.[compositeKey(row.professional_role, 'professional_role')]?._id ?? null + : null,Add this small helper near the top of extractDataFromCSV (above where row.meta is constructed):
// Local helper to mirror utils.js normalization const compositeKey = (value, field) => `${String(value ?? '').trim().replace(/\s+/g, '').toLowerCase()}${String(field ?? '') .trim() .replace(/\s+/g, '') .toLowerCase()}`Please confirm that the external service’s entityType values exactly match the CSV headers used here (block, state, school, cluster, district, professional_role). If not, we should map header->entityType explicitly.
323-329: Filter out missing IDs (and optionally dedupe) for professional_subrolesAs written, the map can produce undefined entries when a sub-role isn’t found; the trailing “|| []” won’t remove those. Filter falsy and consider de-duping.
Apply:
- .map( - (prof_subRole) => - externalEntityNameIdMap[ - `${prof_subRole - ?.replaceAll(/\s+/g, '') - .toLowerCase()}${'professional_subroles' - .replaceAll(/\s+/g, '') - .toLowerCase()}` - ]?._id - ) || [] + .map((prof_subRole) => + externalEntityNameIdMap[compositeKey(prof_subRole, 'professional_subroles')]?._id + ) + .filter(Boolean)Optional (avoid duplicates):
- professional_subroles: row?.professional_subroles - ? row.professional_subroles - .split(',') - .map((prof_subRole) => externalEntityNameIdMap[compositeKey(prof_subRole, 'professional_subroles')]?._id) - .filter(Boolean) - : [], + professional_subroles: row?.professional_subroles + ? [...new Set( + row.professional_subroles + .split(',') + .map((v) => v.trim()) + .map((prof) => externalEntityNameIdMap[compositeKey(prof, 'professional_subroles')]?._id) + .filter(Boolean) + )] + : [],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/generics/utils.js(3 hunks)src/helpers/userInvite.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/generics/utils.js (4)
src/services/entityType.js (3)
entities(98-104)entities(136-136)entityType(27-27)src/database/queries/entityType.js (1)
entities(141-145)src/services/organization.js (1)
tenantCode(254-254)src/controllers/v1/public.js (1)
tenantCode(7-7)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/generics/utils.js (2)
821-824: Defensive normalization looks good; centralize helper and consider naming
- Nice guard against undefined and consistent normalization.
- Consider extracting normalize into a shared util used by CSV/meta code to avoid drift.
- Rename namePart -> idPart for clarity (it’s externalId, not a name).
825-825: Add a delimiter in the composite key to prevent collisionsConcatenation without a separator can collide (e.g., ab + cd vs abc + d). Use a stable delimiter unlikely to appear post-normalization.
- const key = `${namePart}${typePart}` + const key = `${namePart}::${typePart}`If you adopt this, align any consumers (e.g., CSV invite lookups) to use the same delimiter.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/generics/utils.js(2 hunks)
🔇 Additional comments (1)
src/generics/utils.js (1)
805-808: Guard $in, verify callers supply externalId, and batch large lists
- Found one call site: src/helpers/userInvite.js -> fetchAndMapAllExternalEntities(uniqueEntityValues, service, endPoint, userData.tenant_code). uniqueEntityValues are extracted from CSV (getUniqueEntityValues) — confirm those CSV values are metaInformation.externalId (not human-readable names); if not, convert names to IDs before calling or update the lookup to handle names safely.
- Minimal guard (apply in src/generics/utils.js inside fetchAndMapAllExternalEntities):
- 'metaInformation.externalId': { - $in: entities, // Dynamically pass the array here - }, + 'metaInformation.externalId': { + $in: (entities || []).filter(Boolean), // filter null/empty + },
- If entities can be large (bulk CSV), batch the $in queries (e.g., chunks of ~200) to avoid large-$in performance problems and request size issues.
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor