Fix/group column handling for multilang and link columns#46
Fix/group column handling for multilang and link columns#46TimoGebendorfer wants to merge 6 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes an issue where multilanguage and link columns nested inside group columns were not properly handled by the tablesToLanguages and referencer functions. The fix adds special handling logic to process group columns by iterating through their sub-columns and applying appropriate transformations for link columns (extracting IDs) and multilanguage columns (applying language fallback logic).
Changes:
- Added group column handling to
tablesToLanguagesfunction to properly extract link IDs and resolve multilanguage values within groups - Added group column handling to
referencerfunction to resolve link references within group columns - Added comprehensive test coverage for numeric groups, multilanguage groups, and groups containing link columns
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| CHANGELOG.md | Documents the bug fixes for group column handling in version 8.9.0 |
| src/tablesToLanguages.js | Implements group column processing with link ID extraction and multilanguage fallback support |
| src/tablesToLanguages.spec.js | Adds three new test cases covering numeric groups, multilanguage groups, and link columns within groups |
| src/referencer/referencer.js | Implements processGroupColumn helper function to resolve link references within group columns |
| src/referencer/referencer.spec.js | Adds two test cases for simple group columns and link columns inside group columns |
| lib/tablesToLanguages.js | Transpiled version of the source file with identical logic |
| lib/referencer/referencer.js | Transpiled version of the source file with identical logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
TimoGebendorfer
requested review from
McHunkyTrunk,
Zwergal,
smnhgn and
zingmane
McHunkyTrunk
left a comment
McHunkyTrunk
left a comment
There was a problem hiding this comment.
Funktional scheint alls in Ordnung zu sein. Test-Fixtures sind noch nicht ganz integer.
| "de-DE": "Beschreibung", | ||
| "en-GB": "Description" | ||
| }, | ||
| "groups": [ |
There was a problem hiding this comment.
[MAJOR] Da stimmt doch was an der Fixture nicht, oder? Spalte 1 in der Gruppe ist ein Shorttext und Spalte 2 ist die Group-Column selbst. Es sollten also eigentlich 2 separate numeric Spalten sein, die durch diese Group-Column zusammengefasst werden. Entsprechend anders sind dann auch die Row-Daten.
| "languageType": "language" | ||
| }, | ||
| { | ||
| "id": 2, |
There was a problem hiding this comment.
[MAJOR] Gleiches hier: Es soll eine separate Spalte mit der anderen ID sein.
| "id": 2, | ||
| "ordering": 20, | ||
| "name": "detailNumber", | ||
| "kind": "number", |
There was a problem hiding this comment.
| "kind": "number", | |
| "kind": "numeric", |
| "description": "Description" | ||
| }, | ||
| { | ||
| "id": 2, |
There was a problem hiding this comment.
[MAJOR] Hatten wir schon: Es muss eine weitere numeric Column sein.
| "description": "Beschreibung" | ||
| }, | ||
| { | ||
| "id": 2, |
There was a problem hiding this comment.
[MAJOR] Existiert im Fixture nicht.
| denormalized[table.id][rowId][column.name] = _.map(cellValue, idInOtherTableValue => { | ||
| const idInOtherTable = idInOtherTableValue.id || idInOtherTableValue; | ||
| const res = orEmpty(denormalized, column.toTable, idInOtherTable); | ||
| res.linkRowId = idInOtherTable; | ||
| return res; |
There was a problem hiding this comment.
[SUGGESTION] Sollen wir es als processLinkColumn() extrahieren und auch unten innerhalb von Group-Column anwenden? Sieht gleich aus.
| const subColumn = column.groups[subIndex]; | ||
|
|
||
| if (subColumn && subColumn.kind === "link") { | ||
| return _.map(subValue, cell => cell.id); |
There was a problem hiding this comment.
| return _.map(subValue, cell => cell.id); | |
| return _.map(subValue, "id"); |
2 participants
No description provided.