houseassassin/twenty
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix - Record text should always be visible and in first position in views (#14598)

Browse Source 
Fixes https://github.com/twentyhq/twenty/issues/14442

Issues were
1. Table headers always used label metadata identifier (or record text)
as first column, while table body followed viewFields positions
2. Label metadata identifier should always be visible and in first
position for the table views to work as expected, while when updating
the label metadata identifier for an object, no changes were brought to
the viewFields

To fix that
1. In the BE: 
- a new logic is implemented to i) update label identifier's viewFields
position and visibility when an object's label identifier is updated to
a new field; ii) add validation on the update of a viewField's position
and visibility to make sure the label identifier's viewField always has
the lowest position + is visible
- a command was added to check all existing views and viewfields
3. In the FE: at first I tried to replicate the logic of the headers
(based on the label identifier rather than the positions) on the body,
but it was too complex and error-prone as in multiple places we are
based on the positions. It also feels more right to have only one source
of truth which is the viewField position. @lucasbordeau if that does not
suit you, we can throw an error if the field with the lowest position is
not the label metadata identifier, as you said the table view will be
very buggy / wont work if the label identifier is not in the first
position. but now it should never be the case thanks to the validation
implemented in the BE

This should be migrated to viewFieldService V2 when relevant @Weiko
@prastoin
This commit is contained in:
Marie
2025-09-19 12:24:09 +02:00
committed by GitHub
parent 43e0cd5d05
commit 4a6ad48a62
14 changed files with 761 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/twenty-server/src/engine/core-modules/view/services/tests/view-field.service.spec.ts
+224
View File
@@ -1,9 +1,11 @@
import { Test, type TestingModule } from '@nestjs/testing';
import { getRepositoryToken } from '@nestjs/typeorm';
import { UserInputError } from 'apollo-server-core';
import { type Repository } from 'typeorm';
import { ViewFieldEntity } from 'src/engine/core-modules/view/entities/view-field.entity';
import { type ViewEntity } from 'src/engine/core-modules/view/entities/view.entity';
import {
ViewFieldException,
ViewFieldExceptionCode,
@@ -12,10 +14,12 @@ import {
generateViewFieldUserFriendlyExceptionMessage,
} from 'src/engine/core-modules/view/exceptions/view-field.exception';
import { ViewFieldService } from 'src/engine/core-modules/view/services/view-field.service';
import { ViewService } from 'src/engine/core-modules/view/services/view.service';
describe('ViewFieldService', () => {
let viewFieldService: ViewFieldService;
let viewFieldRepository: Repository<ViewFieldEntity>;
let viewService: ViewService;
const mockViewField = {
id: 'view-field-id',
@@ -45,6 +49,12 @@ describe('ViewFieldService', () => {
delete: jest.fn(),
},
},
{
provide: ViewService,
useValue: {
findByIdWithRelatedObjectMetadata: jest.fn(),
},
},
],
}).compile();
@@ -52,6 +62,7 @@ describe('ViewFieldService', () => {
viewFieldRepository = module.get<Repository<ViewFieldEntity>>(
getRepositoryToken(ViewFieldEntity),
);
viewService = module.get<ViewService>(ViewService);
});
it('should be defined', () => {
@@ -203,6 +214,54 @@ describe('ViewFieldService', () => {
),
);
});
it('should throw exception if position is lower than label metadata identifier', async () => {
const labelIdentifierFieldMetadataId =
'label-identifier-field-matadata-id';
const labelIdentifierViewFieldId =
'view-field-for-label-metadata-identifier-id';
const labelIdentifierViewField = {
...mockViewField,
id: labelIdentifierViewFieldId,
fieldMetadataId: labelIdentifierFieldMetadataId,
position: 0,
};
const mockView = {
id: 'view-id',
objectMetadata: {
labelIdentifierFieldMetadataId,
},
viewFields: [
labelIdentifierViewField,
{ ...mockViewField, position: 1 },
],
} as ViewEntity;
jest.spyOn(viewFieldService, 'findById').mockImplementation((id) => {
if (id === mockViewField.id) {
return Promise.resolve(mockViewField);
}
return Promise.resolve(null);
});
jest
.spyOn(viewService, 'findByIdWithRelatedObjectMetadata')
.mockResolvedValue(mockView);
const invalidData = { ...validViewFieldData, position: -1 };
await expect(viewFieldService.create(invalidData)).rejects.toThrow(
new UserInputError(
'Label metadata identifier must keep the minimal position in the view.',
{
userFriendlyMessage:
'Record text must be in first position of the view.',
},
),
);
});
});
describe('update', () => {
@@ -212,10 +271,21 @@ describe('ViewFieldService', () => {
const updateData = { position: 1 };
const updatedViewField = { ...mockViewField, ...updateData };
const mockView = {
id: 'view-id',
objectMetadata: {
labelIdentifierFieldMetadataId: mockViewField.fieldMetadataId,
},
viewFields: [mockViewField],
} as ViewEntity;
jest.spyOn(viewFieldService, 'findById').mockResolvedValue(mockViewField);
jest
.spyOn(viewFieldRepository, 'save')
.mockResolvedValue(updatedViewField);
jest
.spyOn(viewService, 'findByIdWithRelatedObjectMetadata')
.mockResolvedValue(mockView);
const result = await viewFieldService.update(id, workspaceId, updateData);
@@ -246,6 +316,160 @@ describe('ViewFieldService', () => {
),
);
});
it('should throw exception when label metadata identifier is not in first position (label metadata identifier field update case)', async () => {
const workspaceId = 'workspace-id';
const updateData = { position: 2 };
const labelIdentifierFieldMetadataId =
'label-identifier-field-matadata-id';
const labelIdentifierViewFieldId =
'view-field-for-label-metadata-identifier-id';
const labelIdentifierViewField = {
...mockViewField,
id: labelIdentifierViewFieldId,
fieldMetadataId: labelIdentifierFieldMetadataId,
position: 0,
};
const mockView = {
id: 'view-id',
objectMetadata: {
labelIdentifierFieldMetadataId,
},
viewFields: [
labelIdentifierViewField,
{ ...mockViewField, position: 1 },
],
} as ViewEntity;
jest.spyOn(viewFieldService, 'findById').mockImplementation((id) => {
if (id === labelIdentifierViewFieldId) {
return Promise.resolve(labelIdentifierViewField);
}
return Promise.resolve(null);
});
jest
.spyOn(viewService, 'findByIdWithRelatedObjectMetadata')
.mockResolvedValue(mockView);
await expect(
viewFieldService.update(
labelIdentifierViewFieldId,
workspaceId,
updateData,
),
).rejects.toThrow(
new UserInputError(
'Label metadata identifier must keep the minimal position in the view.',
{
userFriendlyMessage:
'Record text must be in first position of the view.',
},
),
);
});
it('should throw exception when label metadata identifier is not in first position (regular field update case)', async () => {
const workspaceId = 'workspace-id';
const updateData = { position: -1 };
const labelIdentifierFieldMetadataId =
'label-identifier-field-matadata-id';
const labelIdentifierViewFieldId =
'view-field-for-label-metadata-identifier-id';
const labelIdentifierViewField = {
...mockViewField,
id: labelIdentifierViewFieldId,
fieldMetadataId: labelIdentifierFieldMetadataId,
position: 0,
};
const mockView = {
id: 'view-id',
objectMetadata: {
labelIdentifierFieldMetadataId,
},
viewFields: [
labelIdentifierViewField,
{ ...mockViewField, position: 1 },
],
} as ViewEntity;
jest.spyOn(viewFieldService, 'findById').mockImplementation((id) => {
if (id === mockViewField.id) {
return Promise.resolve(mockViewField);
}
return Promise.resolve(null);
});
jest
.spyOn(viewService, 'findByIdWithRelatedObjectMetadata')
.mockResolvedValue(mockView);
await expect(
viewFieldService.update(mockViewField.id, workspaceId, updateData),
).rejects.toThrow(
new UserInputError(
'Label metadata identifier must keep the minimal position in the view.',
{
userFriendlyMessage:
'Record text must be in first position of the view.',
},
),
);
});
it('should throw exception when attempting to make label metadata identifier invisible', async () => {
const workspaceId = 'workspace-id';
const updateData = { isVisible: false };
const labelIdentifierFieldMetadataId =
'label-identifier-field-matadata-id';
const labelIdentifierViewFieldId =
'view-field-for-label-metadata-identifier-id';
const labelIdentifierViewField = {
...mockViewField,
id: labelIdentifierViewFieldId,
fieldMetadataId: labelIdentifierFieldMetadataId,
position: 0,
};
const mockView = {
id: 'view-id',
objectMetadata: {
labelIdentifierFieldMetadataId,
},
viewFields: [labelIdentifierViewField, mockViewField],
} as ViewEntity;
jest.spyOn(viewFieldService, 'findById').mockImplementation((id) => {
if (id === labelIdentifierViewFieldId) {
return Promise.resolve(labelIdentifierViewField);
}
return Promise.resolve(null);
});
jest
.spyOn(
viewFieldService['viewService'],
'findByIdWithRelatedObjectMetadata',
)
.mockResolvedValue(mockView);
await expect(
viewFieldService.update(
labelIdentifierViewField.id,
workspaceId,
updateData,
),
).rejects.toThrow(
new UserInputError('Label metadata identifier must stay visible.', {
userFriendlyMessage: 'Record text must stay visible.',
}),
);
});
});
describe('delete', () => {
packages/twenty-server/src/engine/core-modules/view/services/view-field.service.ts
+278 -67
View File
@@ -4,6 +4,7 @@ import { InjectRepository } from '@nestjs/typeorm';
import { isDefined } from 'twenty-shared/utils';
import { IsNull, Repository } from 'typeorm';
import { UserInputError } from 'src/engine/core-modules/graphql/utils/graphql-errors.util';
import { ViewFieldEntity } from 'src/engine/core-modules/view/entities/view-field.entity';
import {
ViewFieldException,
@@ -12,12 +13,14 @@ import {
generateViewFieldExceptionMessage,
generateViewFieldUserFriendlyExceptionMessage,
} from 'src/engine/core-modules/view/exceptions/view-field.exception';
import { ViewService } from 'src/engine/core-modules/view/services/view.service';
@Injectable()
export class ViewFieldService {
constructor(
@InjectRepository(ViewFieldEntity)
private readonly viewFieldRepository: Repository<ViewFieldEntity>,
private readonly viewService: ViewService,
) {}
async findByWorkspaceId(workspaceId: string): Promise<ViewFieldEntity[]> {
@@ -65,35 +68,105 @@ export class ViewFieldService {
async create(
viewFieldData: Partial<ViewFieldEntity>,
): Promise<ViewFieldEntity> {
if (!isDefined(viewFieldData.workspaceId)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.WORKSPACE_ID_REQUIRED,
),
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
if (this.hasRequiredFields(viewFieldData)) {
try {
if (isDefined(viewFieldData.position)) {
const viewFieldDataWithPosition =
viewFieldData as typeof viewFieldData & { position: number };
await this.verifyLabelMetadataIdentifierIsInFirstPositionOrThrow(
viewFieldDataWithPosition,
viewFieldData.workspaceId,
);
}
await this.verifyLabelMetadataIdentifierIsVisibleOrThrow(
viewFieldData,
viewFieldData.workspaceId,
);
const viewFieldDataWithPosition = await this.formatViewFieldData(
viewFieldData,
viewFieldData.workspaceId,
);
const viewField = this.viewFieldRepository.create(
viewFieldDataWithPosition,
);
const savedViewField = await this.viewFieldRepository.save(viewField);
const createdViewField = await this.findById(
savedViewField.id,
viewFieldData.workspaceId,
);
if (!isDefined(createdViewField)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_NOT_FOUND,
),
ViewFieldExceptionCode.VIEW_FIELD_NOT_FOUND,
{
userFriendlyMessage:
generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_NOT_FOUND,
),
},
);
}
return createdViewField;
} catch (error) {
if (
error.message.includes(
'duplicate key value violates unique constraint',
)
) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_ALREADY_EXISTS,
),
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage:
generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_ALREADY_EXISTS,
),
},
);
}
throw error;
}
} else {
if (!isDefined(viewFieldData.workspaceId)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.WORKSPACE_ID_REQUIRED,
),
},
);
}
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.WORKSPACE_ID_REQUIRED,
),
},
);
}
if (!isDefined(viewFieldData.viewId)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_ID_REQUIRED,
),
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
if (!isDefined(viewFieldData.viewId)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_ID_REQUIRED,
),
},
);
}
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_ID_REQUIRED,
),
},
);
}
if (!isDefined(viewFieldData.fieldMetadataId)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.FIELD_METADATA_ID_REQUIRED,
@@ -106,50 +179,6 @@ export class ViewFieldService {
},
);
}
try {
const viewField = this.viewFieldRepository.create(viewFieldData);
const savedViewField = await this.viewFieldRepository.save(viewField);
const createdViewField = await this.findById(
savedViewField.id,
viewFieldData.workspaceId,
);
if (!isDefined(createdViewField)) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_NOT_FOUND,
),
ViewFieldExceptionCode.VIEW_FIELD_NOT_FOUND,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_NOT_FOUND,
),
},
);
}
return createdViewField;
} catch (error) {
if (
error.message.includes('duplicate key value violates unique constraint')
) {
throw new ViewFieldException(
generateViewFieldExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_ALREADY_EXISTS,
),
ViewFieldExceptionCode.INVALID_VIEW_FIELD_DATA,
{
userFriendlyMessage: generateViewFieldUserFriendlyExceptionMessage(
ViewFieldExceptionMessageKey.VIEW_FIELD_ALREADY_EXISTS,
),
},
);
}
throw error;
}
}
async update(
@@ -168,6 +197,26 @@ export class ViewFieldService {
ViewFieldExceptionCode.VIEW_FIELD_NOT_FOUND,
);
}
const viewId = existingViewField.viewId;
if (this.updatesPosition(updateData)) {
await this.verifyLabelMetadataIdentifierIsInFirstPositionOrThrow(
{
...updateData,
viewId,
id,
fieldMetadataId: existingViewField.fieldMetadataId,
},
workspaceId,
);
}
if (this.disablesVisibility(updateData)) {
await this.verifyLabelMetadataIdentifierIsVisibleOrThrow(
{ ...updateData, viewId, id },
workspaceId,
);
}
const updatedViewField = await this.viewFieldRepository.save({
id,
@@ -212,4 +261,166 @@ export class ViewFieldService {
return viewField;
}
private updatesPosition(
data: Partial<ViewFieldEntity>,
): data is Partial<ViewFieldEntity> & { position: number } {
return isDefined(data.position);
}
private disablesVisibility(
data: Partial<ViewFieldEntity>,
): data is Partial<ViewFieldEntity> & { isVisible: boolean } {
return data.isVisible === false;
}
private async verifyLabelMetadataIdentifierIsVisibleOrThrow(
newOrUpdatedViewField: Partial<ViewFieldEntity> & {
viewId: string;
},
workspaceId: string,
) {
const view = await this.viewService.findByIdWithRelatedObjectMetadata(
newOrUpdatedViewField.viewId,
workspaceId,
);
if (!isDefined(view)) {
throw new Error(`View not found: ${newOrUpdatedViewField.viewId}`);
}
const labelMetadataIdentifierFieldMetadataId =
view.objectMetadata.labelIdentifierFieldMetadataId;
const labelMetadataIdentifierViewField = view.viewFields.find(
(viewField) =>
viewField.fieldMetadataId === labelMetadataIdentifierFieldMetadataId,
);
if (
!isDefined(labelMetadataIdentifierViewField) ||
labelMetadataIdentifierViewField.id !== newOrUpdatedViewField.id
) {
return;
}
if (newOrUpdatedViewField.isVisible === false) {
throw new UserInputError('Label metadata identifier must stay visible.', {
userFriendlyMessage: 'Record text must stay visible.',
});
}
}
private async verifyLabelMetadataIdentifierIsInFirstPositionOrThrow(
newOrUpdatedViewField: Partial<ViewFieldEntity> & { viewId: string } & {
position: number;
},
workspaceId: string,
) {
const view = await this.viewService.findByIdWithRelatedObjectMetadata(
newOrUpdatedViewField.viewId,
workspaceId,
);
if (!isDefined(view)) {
throw new Error(`View not found: ${newOrUpdatedViewField.viewId}`);
}
const viewFieldsWithoutUpdatedViewField = view.viewFields.filter(
(viewField) => viewField.id !== newOrUpdatedViewField?.id,
);
if (viewFieldsWithoutUpdatedViewField.length === 0) {
return;
}
const labelMetadataIdentifierFieldMetadataId =
view.objectMetadata.labelIdentifierFieldMetadataId;
if (
labelMetadataIdentifierFieldMetadataId ===
newOrUpdatedViewField.fieldMetadataId
) {
const minPositionInViewWithoutUpdatedViewField =
viewFieldsWithoutUpdatedViewField.reduce(
(minViewField, viewField) =>
viewField.position < minViewField.position
? viewField
: minViewField,
viewFieldsWithoutUpdatedViewField[0],
).position;
if (
newOrUpdatedViewField.position >=
minPositionInViewWithoutUpdatedViewField
) {
throw new UserInputError(
'Label metadata identifier must keep the minimal position in the view.',
{
userFriendlyMessage:
'Record text must be in first position of the view.',
},
);
}
} else {
const labelMetadataIdentifierViewFieldPosition = view.viewFields.find(
(viewField) =>
viewField.fieldMetadataId === labelMetadataIdentifierFieldMetadataId,
)?.position;
if (
isDefined(labelMetadataIdentifierViewFieldPosition) &&
newOrUpdatedViewField.position <=
labelMetadataIdentifierViewFieldPosition
) {
throw new UserInputError(
'Label metadata identifier must keep the minimal position in the view.',
{
userFriendlyMessage:
'Record text must be in first position of the view.',
},
);
}
}
}
private async formatViewFieldData(
viewFieldData: Partial<ViewFieldEntity> & { viewId: string },
workspaceId: string,
): Promise<Partial<ViewFieldEntity>> {
if (!isDefined(viewFieldData.position)) {
const view = await this.viewService.findByIdWithRelatedObjectMetadata(
viewFieldData.viewId,
workspaceId,
);
if (!isDefined(view)) {
throw new Error(`View not found: ${viewFieldData.viewId}`);
}
const highestPositionInView = view.viewFields.reduce(
(maxViewField, viewField) =>
viewField.position > maxViewField.position ? viewField : maxViewField,
view.viewFields[0],
);
return { ...viewFieldData, position: highestPositionInView.position + 1 };
} else {
return viewFieldData;
}
}
private hasRequiredFields(
data: Partial<ViewFieldEntity>,
): data is Partial<ViewFieldEntity> & {
viewId: string;
fieldMetadataId: string;
workspaceId: string;
} {
return (
isDefined(data.viewId) &&
isDefined(data.fieldMetadataId) &&
isDefined(data.workspaceId)
);
}
}
packages/twenty-server/src/engine/core-modules/view/services/view.service.ts
+16
View File
@@ -84,6 +84,22 @@ export class ViewService {
return view || null;
}
async findByIdWithRelatedObjectMetadata(
id: string,
workspaceId: string,
): Promise<ViewEntity | null> {
const view = await this.viewRepository.findOne({
where: {
id,
workspaceId,
deletedAt: IsNull(),
},
relations: ['workspace', 'objectMetadata', 'viewFields'],
});
return view || null;
}
async create(viewData: Partial<ViewEntity>): Promise<ViewEntity> {
if (!isDefined(viewData.workspaceId)) {
throw new ViewException(