[feature]: Include cell reference details in error summary#406
[feature]: Include cell reference details in error summary#406deeonwuli wants to merge 2 commits into
Conversation
BundleMonNo change in files bundle size Groups updated (1)
Final result: ✅ View report in BundleMon website ➡️ |
gqcorneby
left a comment
There was a problem hiding this comment.
Thanks @deeonwuli! Nice work 🎉 the tests are thorough. A few in-line and general comments
- Translations - Can we check and remove fuzzies in translation files?
- Cell based data source - As stated in the description, only rows, teis, and events are mapped back to the template. Cell based data sources (usually datasets), are not supported. @MiquelAdell Do we want to include this in this PR or as a separate backlog?
Heads-up: Need to double check but I think we can't just set group = buildRowNumber(cell.ref) in readByCell. Group is the grouping key at ExcelReader.ts:89, so distinct row numbers would re-split the aggregated payload. It probably needs sheet/row/column carried on the data value itself, with the lookup indexing by cell.
file you can use to test:
NHWA Module 1_v1 (1).xlsx
| locationsById.set(id, [...(locationsById.get(id) ?? []), location]); | ||
| }; | ||
|
|
||
| dataPackage.dataEntries.forEach(entry => { |
There was a problem hiding this comment.
This works for row based data sources but follows an imperative pattern. The same output can be built as id → location pairs via flatMap, then a single groupBy. What do you think?
|
|
||
| type Item = { id: string; name: string }; | ||
|
|
||
| function deduplicateErrors(errors: ErrorMessage[]): ErrorMessage[] { |
There was a problem hiding this comment.
Grouping by message alone seems correct for the intended case, maybe we can add it as a comment to add some context?
There's a theoretical hole: two distinct errors sharing an identical generic message where only one resolved a location, the un-located would get dropped as if it were a duplicate. I think it's not a real scenario but opening it up for discussion. Cheap fix would be to update the grouping to .groupBy(e => ${e.message}@@${e.details}`
No, since this is paid by the client and the client only uses tracker programs let's not do that in this one. @deeonwuli you can address the rest of the comments whenever you have a moment. |
📌 References
📝 Implementation
ImportRowLookup, a class that maps every metadata/org unit/TEI/attribute id present in an imported Excel template to its sheet and cell location ({ sheet, row, column }) before the import is sent to DHIS2cell.refin all three Excel reading paths:readByRow(event programs),readTeiEvents(tracker program events), andreadTeiRows(tracker program TEI attributes)ENROLLMENTandTRACKED_ENTITY) are deduplicated by message — the copy with location details is kept, the rest dropped🔥 Notes for the reviewer
📹 Screenshots/Screen capture
📑 Others
#869dg8vmf