feat(compliance-report): project eForm answers into per-template report columns (#1166) - #1174
Merged
Merged
Conversation
…mns (#1166) Add POST api/backend-configuration-pn/compliance-report/eform-columns, which turns a filtered set of compliance rows into per-template answer columns for the Rapport view: grouped by tag, then sub-grouped by the eForm template that was actually answered. The template key is the SDK Case.CheckListId, never AreaRule.EformId -- EformId tracks current configuration while the case records what was answered, and against live data it mismatched or was null for a fifth of rows. Column schema and cells are KEYED, not positional. A field either enters the column list and the type map together or neither, the answer query is driven off the column list, and "no cell" is an absent key rather than an appended empty one. The shipped report code builds headers from a filtered field list and iterates the unfiltered one, so Audio/ShowPdf/Movie shift every later column by one; that bug class cannot be expressed here. The two existing methods carrying it are untouched. Field derivation goes through Advanced_TemplateFieldReadAll, which walks child checklists and FieldGroup children -- several live templates have zero directly attached fields -- and is cached once per checklist per request. When derivation throws, which the SDK still does for a language with translation gaps, the template is flagged SchemaUnavailable rather than rendering as an empty table indistinguishable from "nobody answered". Both FieldValues queries lead with FieldId. FieldValues has no index on CaseId, so the CaseId-first shape every existing consumer uses is a full scan; the FieldId-first shape uses IX_field_values_field_id. Nothing runs per case in a loop. Values are decoded per type rather than rendered raw: select options resolve through FieldOptions and their translations, entity fields resolve entity ids and tolerate the literal string "null", CheckBox accepts both checked/unchecked and dirty true/false, MultiSelect splits pipe-joined keys and ignores legacy comma values, and Timer takes the fourth pipe-separated part as elapsed milliseconds. Anything malformed yields no cell -- including an elapsed value too large for a TimeSpan, which would otherwise have failed the entire report from one junk cell. 34 tests: nested and FieldGroup derivation, the excluded-type set, an Audio+ShowPdf regression that pins the desync bug shut, every non-trivial value decoding including the dirty variants, translation fallback for both field and option labels, image name derivation, CheckListId versus a differing EformId, and the tag/template grouping shapes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LcMeVFxqWqQzvqHjAa3Xkc
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large new reporting projection path with multiple cross-DB queries and nuanced grouping/decoding behavior, so it warrants final human review despite strong integration-test coverage.
Pull request overview
Adds a new backend endpoint for the Compliance “Rapport” view that projects filtered compliance rows into tag → template groups, each with a per-template answer column schema and keyed per-case answer cells (plus image references), enabling the forthcoming UI and exports to render answers without positional column drift.
Changes:
- Introduces
POST api/backend-configuration-pn/compliance-report/eform-columnsreturningList<ComplianceReportTagGroupModel>(tag groups → template groups → columns + keyed cells). - Implements per-template schema derivation (cached per request) and bulk answer/image projection driven by schema field IDs.
- Adds integration-test coverage for grouping, schema derivation, value decoding, translation fallback, and image reference projection.
File summaries
| File | Description |
|---|---|
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Services/BackendConfigurationComplianceReportService/IBackendConfigurationComplianceReportService.cs | Adds the EformColumns service contract returning grouped per-template column/cell models. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Services/BackendConfigurationComplianceReportService/ComplianceReportEformProjector.cs | New per-request projector that derives template schemas and bulk-loads answers/images into keyed cell bags. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Services/BackendConfigurationComplianceReportService/BackendConfigurationComplianceReportService.cs | Implements the EformColumns endpoint logic: candidate set reuse, truncation, enrichment, grouping, and projection orchestration. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Infrastructure/Models/ComplianceReport/ComplianceReportTagGroupModel.cs | New DTO for tag grouping in the Rapport response. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Infrastructure/Models/ComplianceReport/ComplianceReportTemplateGroupModel.cs | New DTO for template subgrouping, columns, cases, and SchemaUnavailable. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Infrastructure/Models/ComplianceReport/ComplianceReportColumnModel.cs | New DTO describing stable column keys, field IDs, labels, and field types. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Infrastructure/Models/ComplianceReport/ComplianceReportCaseModel.cs | New DTO for per-case metadata plus keyed Cells and image references. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Infrastructure/Models/ComplianceReport/ComplianceReportImageModel.cs | New DTO for image references (IDs + derived display name + geo link). |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn/Controllers/ComplianceReportController.cs | Exposes POST eform-columns controller action under the compliance-report route. |
| eFormAPI/Plugins/BackendConfiguration.Pn/BackendConfiguration.Pn.Integration.Test/ComplianceReportEformColumnsTests.cs | Adds DB-backed integration tests for the new endpoint’s schema/answer/image projection and grouping behavior. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1166
to
+1169
| SentrySdk.CaptureException(e); | ||
| logger.LogError(e, "BackendConfigurationComplianceReportService.EformColumns: {Message}", e.Message); | ||
| return new OperationDataResult<List<ComplianceReportTagGroupModel>>(false, | ||
| $"{localizationService.GetString("ErrorWhileGettingCalendarTasks")}: {e.Message}"); |
…dDatas (#1166) All 34 new tests died in [SetUp] with MySqlException : Unknown column 'u.OriginalFileLocation' in 'SELECT' UploadedData.OriginalFileLocation exists on the SDK entity but not in the integration-test bootstrap dump SQL/420_SDK.sql; the column is added by the SDK's own EF migrations, which run only when Core starts. CleanTables read UploadedDatas before that -- RemoveRange(DbSet) enumerates the set and so SELECTs every mapped column -- and the whole fixture failed at setup rather than on an assertion. CleanTables now captures the connection string, awaits GetCore() so the migrations apply, and disposes and recreates MicrotingDbContext before any SDK read: EF caches the model on first query, so a context that ran pre-migration still errors. The pre-warm sits above the entire SDK block, so FieldValues, UploadedDatas and Cases all go through the post-migration context and the existing FK ordering (FieldValues first, since it carries the FK to UploadedData) is untouched. This is the pattern CalendarAttachmentTests and GoogleDriveTests already use for the same reason. Like both of those, the recreated context does not re-apply TestBaseSetup's 300s command timeout and falls back to the ADO default; matching the two green precedents rather than deviating. No implementation file changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LcMeVFxqWqQzvqHjAa3Xkc
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1166. Part of #1160. Depends on #1161 (merged).
Adds
POST api/backend-configuration-pn/compliance-report/eform-columns— a filtered set of compliance rows projected into per-template answer columns, grouped by tag then sub-grouped by eForm template. Backend only; #1167 owns the Rapport UI, #1168 the gallery.The findings this issue exists to respect
The template key is
Case.CheckListId, neverAreaRule.EformId(#1160 finding 1).EformIdtracks current configuration; the case records what was answered. Against live data it mismatched 34 rows and was null for ~16%.Cells are keyed, not positional (finding 3). The code this replaces builds headers from a filtered field list and iterates the unfiltered one, appending an empty cell for unanswered fields — and
Audio/Movie/ShowPdfare excluded from headers but fall through todefault:and always emit. Either path shifts every later column by one, and the bug exists twice in the shipped service. Here a field entersColumnsandFieldTypeByIdtogether or not at all, the answer query is driven offColumns, and "no cell" is an absent key. An excluded field'sFieldValueis never even read from the database. The two existing buggy methods are deliberately untouched.Derivation recurses and is cached (finding 5). Several live templates have zero directly-attached fields — everything hangs off child checklists, and many fields sit under a
FieldGroup.Advanced_TemplateFieldReadAllhandles that; it is called once per checklist per request.Both
FieldValuesqueries lead withFieldId(finding 2). There is no index onCaseId, so theCaseId IN (…)shape every existing consumer uses is a full scan whileFieldId = ? AND CaseId IN (…)usesIX_field_values_field_id. Nothing runs per case in a loop. The image query filters on a picture-field-id set rather than joiningFieldsonFieldTypeId.Not built on
Advanced_FieldValueReadList(N+1 inside a "bulk" method) orField1..Field10(finding 6).Value decoding (finding 4)
Options resolve through
FieldOptions+ translations; entity fields resolve ids and tolerate the literal string"null";CheckBoxacceptschecked/uncheckedand dirtytrue/false;MultiSelectsplits pipe-joined keys and ignores legacy0,1;Timertakes the fourth pipe-separated part. Anything malformed yields no cell.Translation labels use
FirstOrDefault(lang) ?? First(any)throughout — the SDK's other flattener uses a bareFirstAsyncand throws outright for a language with gaps.SchemaUnavailableThe SDK's
TemplateFieldReadAllstill contains a bareFirstAsynconCheckListTranslations, so derivation genuinely throws for a language with gaps. Swallowing that produced a template with zero columns — indistinguishable from "nobody answered". It now setsSchemaUnavailableon the template group and logs at Warning, so #1167 can say so. The SDK fix is a separate release train and is not in this PR.Reviewed twice — 10 defects found, including a hard blocker
Gate 1 (7). The blocker:
Dictionary<int?, …>throwsArgumentNullExceptionon the untagged path — boxing an emptyNullable<int>yields a null reference andDictionarynull-checks its key. The compiler would emitCS8714, but this csproj has nullable analysis off, so nothing warned. The untagged group is the normal path, so the endpoint failed by default and ~30 of the 33 tests would have gone red. Reproduced on net10.0 before fixing. Also: a failed derivation was silent (nowSchemaUnavailable); tag grouping read only the lowest-Id ARP so a filtered row could fall into the untagged bucket;FieldOptionswas filtered onWorkflowState, so a soft-removed option rendered blank even though the answer is historical; a nullFieldTypeproduced a column with no possible cell; three tests passed on a degraded zero-column response; and the 5000-row cap was applied before template-less rows were dropped, making the ceiling unpredictable.Gate 2 (3). A
Timervalue with a 16–19-digit elapsed-ms parses aslong, then overflowsTimeSpan— and the throw escaped to the outer catch, so one junk cell failed the entire report, contradicting the method's own "no cell" contract. Now bound-checked, with a test. A row could still reach the untagged bucket when anAreaRulePlanningTaghas noPlanningTagsrow (no FK across the two databases) — unresolvable tag ids now keep their group rather than being relocated. A danglingUploadedDataIdmaterialisedNULLinto a non-nullableint. Two comments also stated a factually wrong reason (EF Core rewrites!=to preserve C# null semantics, so NULL-state rows are kept, not dropped) — corrected, since someone could have "fixed" the code based on them.Index,OverviewandBuildCandidateSetare provably untouched: the service diff againststablehas zero deleted lines, so all 52 existing tests (11 + 22 + 19) are unaffected and none was edited.Decisions worth confirming in #1167
ShowPictureis excluded from columns. The shipped code renders its raw value only because it falls intodefault:— an accident, not a decision.MergedCheckListIdsis present and single-valued; the merge needs a locale-independent identity and is filed, not built.Sort/PageIndex/PageSizeare ignored (a page boundary through a tag section is meaningless); the shared 5000-row cap applies with a logged warning.ImagesCountincludes images whose stored filename is empty and which therefore cannot be fetched — do not read it as "renderable images".Verification
dotnet buildon both projects → 0 errors. Tests were not run locally; CI runs them.🤖 Generated with Claude Code
https://claude.ai/code/session_01LcMeVFxqWqQzvqHjAa3Xkc