feat: NoValue is bot properly created the backend #9110
Conversation
magrinj
force-pushed
the
feat/view-group-no-value
branch
from
73623fe to
8ca14e7
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds proper handling of 'No Value' view groups for nullable fields, including a backfill command to fix existing data and frontend support for displaying these groups.
- Added
ViewGroupNoValueBackfillCommandin/packages/twenty-server/src/database/commands/upgrade-version/0-40/0-40-view-group-no-value-backfill.command.tsto fix existing view groups - Added
syncNoValueViewGroupmethod inFieldMetadataRelatedRecordsServiceto manage empty value groups based on field nullability - Modified frontend handling in
useHandleRecordGroupFieldto create 'No Value' groups for nullable fields - Updated
mapViewGroupsToRecordGroupDefinitionsto properly display 'No Value' columns in the UI - Added transaction support in
FieldMetadataRelatedRecordsServicefor data consistency
7 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| !existingGroupKeys.has(`${fieldMetadataItem.id}:`) && | ||
| fieldMetadataItem.isNullable === true |
There was a problem hiding this comment.
logic: The check ${fieldMetadataItem.id}: looks for an exact match with a colon at the end, but line 54 creates keys with ${group.fieldMetadataId}:${group.fieldValue}. This means the check will never match existing empty value groups.
| id: v4(), | ||
| fieldValue: '', | ||
| isVisible: true, | ||
| position: fieldMetadataItem.options.length, |
There was a problem hiding this comment.
style: Using options.length as position could cause conflicts if options were deleted and re-added, since existing groups may already use these positions. Consider using max position + 1 instead.
| if (!selectedOption && selectFieldMetadataItem.isNullable === false) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
logic: This check could prevent valid 'No Value' groups from being created if fieldValue is empty string or null
| // We're assuming for now that all viewGroups have the same fieldMetadataId | ||
| const viewGroup = view.viewGroups?.[0]; | ||
| const fieldMetadataId = viewGroup?.fieldMetadataId; |
There was a problem hiding this comment.
logic: Dangerous assumption that all viewGroups share same fieldMetadataId. Should verify this assumption holds or handle cases where it doesn't.
| await this.fieldMetadataRelatedRecordsService.syncNoValueViewGroup( | ||
| fieldMetadata, | ||
| view, | ||
| viewGroupRepository, | ||
| ); |
There was a problem hiding this comment.
logic: Missing transaction manager parameter. Should pass transaction to ensure atomic operations.
| const views = await viewRepository.find({ | ||
| relations: ['viewGroups'], | ||
| }); |
There was a problem hiding this comment.
style: Consider batching view fetches for better performance with large datasets.
...rver/src/database/commands/upgrade-version/0-40/0-40-view-group-no-value-backfill.command.ts
Outdated
Show resolved
Hide resolved
| transactionManager?: EntityManager, | ||
| ): Promise<void> { | ||
| const noValueGroup = view.viewGroups.find( | ||
| (group) => group.fieldValue === '', |
There was a problem hiding this comment.
out of curiosity, why did we decide that no value would be an empty string rather than a null value?
There was a problem hiding this comment.
That's something we can think of, as this field was not nullable, I go that way on the first implementation few PR's ago when we want to make it movable.
cc @charlesBochet, what do you think ?
There was a problem hiding this comment.
by project convention we don't want to have two states representing a null value.
For a string: '' and null would both represent a null string. ==> We have chosen to only rely on ''
For a JSON: {}, '', [] and null would all represent the null case. ==> We have chosen to only rely on null
For an array: [] and null ==> we use []
| @@ -150,7 +178,7 @@ export class FieldMetadataRelatedRecordsService { | |||
| 'view', | |||
| ); | |||
|
|
|||
| return await viewRepository.find({ | |||
| return viewRepository.find({ | |||
There was a problem hiding this comment.
why do we not need await anymore here ?
There was a problem hiding this comment.
Because we're directly returning a promise so await is not needed as it doesn't change anything
There was a problem hiding this comment.
@ijreilly @magrinj we had issues in the past where people were updating the code from
return viewRepository.find({
...to
const views = viewRepository.find({
... // doing some new stuff between fetching the views and returning it
return views;forgetting to put back the await and breaking the code so I usually put it anyway. Not a strong requirement though, we don't have consensus yet but just a FYI.
The only advantage I see with removing the await is that it delegates the decision to await to the caller but in practice we don't benefit from this that often
…-40/0-40-view-group-no-value-backfill.command.ts Co-authored-by: Marie <[email protected]>
No Valueview groups wasn't properly created when we select a group by field metadata, this PR fix the issue.Also a script is added to backfill the current view groups.