Make dataset problem listings use category labels rather than ids - #281
Make dataset problem listings use category labels rather than ids#281jtuomist wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #281 +/- ##
=======================================
Coverage 52.69% 52.70%
=======================================
Files 375 375
Lines 52563 52602 +39
Branches 8029 8037 +8
=======================================
+ Hits 27698 27722 +24
- Misses 22724 22734 +10
- Partials 2141 2146 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e8735cb9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def from_violation(cls, violation: RuleViolation, labels: CoordinateLabels | None = None) -> Self: | ||
| labels = labels if labels is not None else {} |
There was a problem hiding this comment.
When clients request the new fields through InstanceEditorFields.problems or any data-point mutation returning DatasetEditorMutation._current_violations, those unchanged call sites still invoke from_violation(violation) without a lookup. This optional default therefore silently returns the dimension and category identifiers as labels even when localized labels are resolvable, making the same GraphQL type behave differently depending on which field or mutation produced it; build and pass the lookup in those paths as well, or make omission impossible.
AGENTS.md reference: AGENTS.md:L221-L221
Useful? React with 👍 / 👎.
| scope.dimension_id: scope | ||
| for scope in DimensionScope.objects | ||
| .filter( | ||
| scope_content_type=dataset.scope_content_type, | ||
| scope_id=dataset.scope_id, |
There was a problem hiding this comment.
For an instance-wide violation list spanning multiple datasets, this queryset is executed once per dataset by the enclosing loop, its category prefetch adds another query per dataset, and the later DatasetSchemaDimension lookup adds a third. Thus the advertised fixed query cost is actually at least 3N + 1 for N affected datasets, which can make the editor's problem-list resolver issue dozens of queries; load scopes, categories, and schema dimensions in bulk for all collected datasets.
Useful? React with 👍 / 👎.
1 participant
Description
I made a functionality that uses actual category labels in the relevant language rather than category ids. This update also has changes in kausal-paths-ui.
Backend (3 files, uncommitted)
DatasetDimensionCoordinate gains dimensionLabel and categoryLabel. Labels resolve at the GraphQL boundary, not at evaluation time — deliberately: a materialization is shared across users and languages, and RuleViolation.key baseline-diffs on the identifier coordinates at edit time, so baking labels into the stored payload would be wrong twice over.
build_coordinate_labels(violations) does one query pass over the datasets involved, so a violation list costs a fixed number of queries rather than one per coordinate. It reproduces the evaluator's own column rule — DatasetSchemaDimension.column_name when set, the scoped dimension identifier otherwise — so the lookup key matches what the evaluator recorded. Falls back to the identifier when unresolvable. Wired into all three construction sites: the per-dataset resolver, the instance-wide one, and the publish-mutation payload.
UI (5 files, on feat/dataset-validation)
Renders coordinates.map(c => c.categoryLabel).join(' + ') instead of the group id, falling back to the group id. Types regenerated. I also reworded both strings, since "Kategoriegruppe" no longer describes what the placeholder holds:
▎ Für Value fehlt ein erforderlicher Wert für „Industrie + Strom“ in den Jahren 2020, 2021, 2022, 2023. Eine ausdrücklich eingetragene Null gilt als Wert.
Verified against the real data — all 8 violations resolve: "Private Haushalte + Strom", "Industrie + Erdgas", "Private Haushalte + Fernwärme", etc. dimensionLabel is exposed too (giving "Sektoren: Industrie + Energieträger: Strom") but unused for now — available if you want it in a tooltip or a grid.
Two things you should know
English will show German labels, and it's a data problem, not a code one. MODELTRANS_FALLBACK/LANGUAGE_CODE make en the default language, so modeltrans reads the base column for English. These categories store German in the base column (label='Industrie') with i18n={'label_en': 'Industry'} duplicating English into the JSON — so English resolves to the base column and gets German, while German resolves by fallback and gets it right by accident. I verified the mechanism itself works: an in-memory category with label_de set resolves correctly under de and fi. So this affects anything rendering these labels, not just validation messages, and it's pre-existing. Fixing it means either flipping the storage for de-default instances or setting label_de explicitly.
The UI branch won't lint clean until the backend is deployed. eslint's GraphQL plugin resolves the schema via graphql.config.ts, which falls back to https://api.paths.kausal.dev when no local schema.graphql exists — and production doesn't have the new fields yet, so it reports Cannot query field "dimensionLabel". With a local schema export in place it's 0 errors, 9 pre-existing deprecation warnings. If you want it quiet locally: python manage.py export_schema paths.schema > ~/devel/kausal-paths-ui/schema.graphql (note that filename isn't gitignored, so I removed mine after use).
Backend: 1774 tests pass, ruff clean, Run Mypy...Passed. UI: tsc reports no errors in the files I touched (86 total, all pre-existing elsewhere), and the generated diff is purely the two added fields. One thing to flag — I used npm run graphql-codegen in what is a pnpm project, which rewrote packageManager in package.json; I reverted that, but pnpm isn't on PATH here, so you may want to re-run codegen with pnpm yourself if that matters for the lockfile.
✅ Pre-Merge Checklist
Type of Change
Testing
Manual testing instructions
If feature requires manual testing by reviewer, you can provide instructions here.Internationalization & Accessibility
Dependencies
Screenshots/Videos (if applicable)
Add screenshots or videos demonstrating the changes if applicable.
Additional Notes
Any additional information that reviewers should know about this PR.