feat: allow multiple values on relation columns - #2979
Conversation
… Updated related components and services to handle multiple relation values, including UI adjustments for selection and display. Added tests to verify correct formatting of multiple relation values in analytics. Signed-off-by: Talha Bin Yousaf <talhayousaf4420@gmail.com>
|
@he-is-talha please clean you second commit message before merging. |
Keep only the first related id per row when switching to single-select, fix multi filters / API scalar shape, and soften CSV import parsing. Signed-off-by: Talha Bin Yousaf <talhayousaf4420@gmail.com>
9b6cdef to
888a5f9
Compare
|
@samin-z Cleaned the second commit message and force-pushed. |
|
few points:
for point 4 lets fix it for now for this case though. |
Reload truncated values after switching multi→single, warn about data loss, reject empty mandatory relations in UI/API, and block enabling mandatory when some rows still have no related value. Signed-off-by: Talha Bin Yousaf <talhayousaf4420@gmail.com>
|
@samin-z Ready for the review again |
|
Maybe @Koc would be a good reviewer |
blizzz
left a comment
There was a problem hiding this comment.
Thanks for the contriubtion!
If it is correct, that the PR is also AI assisted, please add the "Assisted-by: $AGENT_NAME:$MODEL_VERSION" trailer to each commit where it applies to.
| <div style="width: 100%"> | ||
| <div class="row space-T"> | ||
| <div class="fix-col-4 title"> | ||
| <div class="form-col-4 title"> |
There was a problem hiding this comment.
The rename does not rename the styling. Intentional? Why is the rename necessary in first place?
There was a problem hiding this comment.
Reverted — the rename wasn’t needed.
| } elseif (str_contains($value, ',')) { | ||
| $value = array_map(trim(...), explode(',', $value)); |
There was a problem hiding this comment.
i.e. labels would not be able to contains , I am not aware that such a restriction is in place anyhwere?
There was a problem hiding this comment.
Good catch. Names with commas are kept as one label now; we only split when the full name doesn’t match.
| } | ||
|
|
||
| if (is_string($value)) { | ||
| $decoded = json_decode($value, true); |
There was a problem hiding this comment.
when this is not a json string, integer values would turn into a string, and then may not survive the check in resolveRelationId()
There was a problem hiding this comment.
Fixed — number IDs are handled correctly now.
| try { | ||
| $column = $this->columnMapper->find($columnId); | ||
| if ($column->getType() === Column::TYPE_RELATION | ||
| && !(bool)($column->getCustomSettingsArray()[Column::RELATION_ALLOW_MULTIPLE] ?? false) |
There was a problem hiding this comment.
this bleeds customSettings into Row2Mapper which it should not know about.
There was a problem hiding this comment.
Agreed. Moved that logic out of Row2Mapper.
| } catch (DoesNotExistException) { | ||
| // Column may have been deleted; keep aggregated value as-is | ||
| } |
There was a problem hiding this comment.
this column was already fetched in line 776, hence the check is not necessary know (but does not hurt). What would be better is to use the object that was already fetched.
There was a problem hiding this comment.
Done — we reuse the column we already loaded.
| $this->normalizeToIds($value, $column, throwOnInvalid: true); | ||
| return true; | ||
| } catch (BadRequestError) { | ||
| return $this->normalizeToIds($value, $column, throwOnInvalid: false) !== []; |
There was a problem hiding this comment.
this probably does not need to be called a second time and can be solved differently.
There was a problem hiding this comment.
Simplified — only one pass now.
| if (is_array($value)) { | ||
| return count($value) > 0; | ||
| } | ||
| return $value !== null && $value !== ''; |
There was a problem hiding this comment.
this check was already run in line 503
There was a problem hiding this comment.
Removed the extra check.
| if (this.isMandatory && newValue.length === 0) { | ||
| showError(t('tables', 'This column is mandatory and cannot be empty.')) | ||
| this.cancelEdit() | ||
| this.localLoading = false |
There was a problem hiding this comment.
Removed the redundant line.
| const cellLabel = this.getLabel(cell.value)?.toLowerCase() | ||
| const rawFilter = filter.magicValuesEnriched ? filter.magicValuesEnriched : filter.value | ||
| const filterValue = String(rawFilter ?? '').toLowerCase() | ||
| const ids = this.normalizeIds(cell?.value ?? cell) |
There was a problem hiding this comment.
| const ids = this.normalizeIds(cell?.value ?? cell) | |
| const ids = this.normalizeIds(cell?.value) |
should be just this?
There was a problem hiding this comment.
Applied, thanks.
| $qb->expr()->eq($alias . '.column_id', $qb->createNamedParameter($sortData['columnId'])) | ||
| ) | ||
| ); | ||
| $qb->addOrderBy($qb->func()->max($alias . '.value'), $sortData['mode']); |
There was a problem hiding this comment.
fyi, sorting would be by label id not display value.
There was a problem hiding this comment.
Thanks for the note — left as-is for now.
Move single-value unwrapping into the relation cell mapper, harden label/id parsing, dedupe exclusion filters, and apply the other requested cleanups from code review. Signed-off-by: Talha Bin Yousaf <talhayousaf4420@gmail.com> Assisted-by: Cursor:Composer
|
@blizzz Thanks for the review! Addressed the inline comments. Also added the |



Summary
customSettings.allowMultiple)Closes #2952
🖼️ Screenshots
Notes
int[](legacy scalars are still accepted and normalized)🤖 AI (if applicable)
The content of this PR was partly using AI