4f8aaeaab0
Follow-up to the discussion on #23485 and closes https://github.com/twentyhq/twenty/issues/23484. `FlatNavigationMenuItemValidatorService` receives `UniversalFlatEntityValidationArgs<'navigationMenuItem'>`, so the entity it validates is a `UniversalFlatNavigationMenuItem`: `viewId`, `pageLayoutId` and `targetObjectMetadataId` do not exist at that scope. The validator read the right universal keys but passed them through a bag of booleans named after the ids (`hasViewId`, `hasPageLayoutId`, ...) and then reported the id names in its errors. Nothing tied a message to the property it checked, so fixing one message string leaves the other five wrong. ## Changes - Replace the private `validateNavigationMenuItemType` boolean bag with `validateNavigationMenuItemTypeRequiredProperties({ flatNavigationMenuItem })` under `flat-navigation-menu-item/validators/utils/`, in line with `validateAgentRequiredProperties` and `validateNavigationMenuItemPageLayoutReferenceCrossEntity`. It takes the universal entity, so a message can only name a property that exists at that scope. - The util is an explicit `switch` on `NavigationMenuItemType` closed by `assertUnreachable`, so adding a type fails to compile until its contract is declared. - Each case validates its own properties instead of checking presence generically: - `FOLDER`: non blank `name` - `OBJECT`, `VIEW`, `PAGE_LAYOUT`: `targetObjectMetadataUniversalIdentifier` / `viewUniversalIdentifier` / `pageLayoutUniversalIdentifier` must be valid uuids - `RECORD`: `targetRecordId` and `targetObjectMetadataUniversalIdentifier`, both uuids, reported separately - `LINK`: `link` must pass `isValidUrl` - Both call sites spread the result; the update path passes the merged `{ ...from, ...update }` entity, which removes the redundant `name` re-merge. `targetRecordId` stays an id: it points at workspace record data rather than metadata, so it has no universal counterpart. ## Behaviour - Errors name the universal property (`viewUniversalIdentifier`) instead of the id (`viewId`). - Blank strings are now uniformly treated as missing; creation previously accepted `link: " "`. - `RECORD` reports each missing property separately instead of one merged error. - Values that are present but malformed are now rejected: non uuid identifiers and links that are not urls. Standard application identifiers are all v4 uuids and the create/update inputs already carry `@IsUUID`, so this only tightens the app manifest path. ## Verification - Unit tests for the util cover each type valid and invalid, blank names, non url links and non uuid identifiers (23 tests pass alongside the sibling suite) - `nx typecheck twenty-server` clean - oxlint (type-aware) and oxfmt clean on the changed files <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/23566?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
167 lines
5.4 KiB
TypeScript
167 lines
5.4 KiB
TypeScript
import { expectOneNotInternalServerErrorSnapshot } from 'test/integration/graphql/utils/expect-one-not-internal-server-error-snapshot.util';
|
|
import { buildBaseManifest } from 'test/integration/metadata/suites/application/utils/build-base-manifest.util';
|
|
import { cleanupApplicationAndAppRegistration } from 'test/integration/metadata/suites/application/utils/cleanup-application-and-app-registration.util';
|
|
import { setupApplicationForSync } from 'test/integration/metadata/suites/application/utils/setup-application-for-sync.util';
|
|
import { syncApplication } from 'test/integration/metadata/suites/application/utils/sync-application.util';
|
|
import { type NavigationMenuItemManifest } from 'twenty-shared/application';
|
|
import {
|
|
eachTestingContextFilter,
|
|
type EachTestingContext,
|
|
} from 'twenty-shared/testing';
|
|
import { NavigationMenuItemType } from 'twenty-shared/types';
|
|
|
|
const TEST_APP_ID = 'c1b2c3d4-0001-4000-a000-000000000001';
|
|
const TEST_ROLE_ID = 'c1b2c3d4-0002-4000-a000-000000000002';
|
|
const TEST_NAVIGATION_MENU_ITEM_ID = 'c1b2c3d4-0003-4000-a000-000000000003';
|
|
|
|
type TestContext = {
|
|
navigationMenuItem: NavigationMenuItemManifest;
|
|
};
|
|
|
|
const failingNavigationMenuItemSyncTestCases: EachTestingContext<TestContext>[] =
|
|
[
|
|
{
|
|
title: 'when syncing a FOLDER item without name',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.FOLDER,
|
|
position: 0,
|
|
icon: 'IconFolder',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title: 'when syncing a FOLDER item with a blank name',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.FOLDER,
|
|
position: 0,
|
|
name: ' ',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title: 'when syncing a LINK item without link',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.LINK,
|
|
position: 0,
|
|
name: 'Link without url',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title: 'when syncing a LINK item with a link that is not a valid url',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.LINK,
|
|
position: 0,
|
|
name: 'Link with invalid url',
|
|
link: 'not a link',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title:
|
|
'when syncing an OBJECT item without targetObjectUniversalIdentifier',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.OBJECT,
|
|
position: 0,
|
|
name: 'Object without target',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title:
|
|
'when syncing an OBJECT item with a targetObjectUniversalIdentifier that is not a uuid',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.OBJECT,
|
|
position: 0,
|
|
name: 'Object with malformed target',
|
|
targetObjectUniversalIdentifier: 'not-a-uuid',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title:
|
|
'when syncing a VIEW item with a viewUniversalIdentifier that is not a uuid',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.VIEW,
|
|
position: 0,
|
|
name: 'View with malformed identifier',
|
|
viewUniversalIdentifier: 'not-a-uuid',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title:
|
|
'when syncing a PAGE_LAYOUT item with a pageLayoutUniversalIdentifier that is not a uuid',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: NavigationMenuItemType.PAGE_LAYOUT,
|
|
position: 0,
|
|
name: 'Page layout with malformed identifier',
|
|
pageLayoutUniversalIdentifier: 'not-a-uuid',
|
|
},
|
|
},
|
|
},
|
|
{
|
|
title: 'when syncing an item with an unknown type',
|
|
context: {
|
|
navigationMenuItem: {
|
|
universalIdentifier: TEST_NAVIGATION_MENU_ITEM_ID,
|
|
type: 'UNKNOWN_NAVIGATION_MENU_ITEM_TYPE' as NavigationMenuItemType,
|
|
position: 0,
|
|
name: 'Item with unknown type',
|
|
},
|
|
},
|
|
},
|
|
];
|
|
|
|
describe('Sync application should fail on invalid navigation menu items', () => {
|
|
beforeAll(async () => {
|
|
await setupApplicationForSync({
|
|
applicationUniversalIdentifier: TEST_APP_ID,
|
|
name: 'Test Invalid Navigation Menu Item App',
|
|
description: 'App for testing navigation menu item manifest validation',
|
|
sourcePath: 'test-invalid-navigation-menu-item',
|
|
});
|
|
}, 60000);
|
|
|
|
afterAll(async () => {
|
|
await cleanupApplicationAndAppRegistration({
|
|
applicationUniversalIdentifier: TEST_APP_ID,
|
|
});
|
|
});
|
|
|
|
it.each(eachTestingContextFilter(failingNavigationMenuItemSyncTestCases))(
|
|
'$title',
|
|
async ({ context }) => {
|
|
const { errors } = await syncApplication({
|
|
manifest: buildBaseManifest({
|
|
appId: TEST_APP_ID,
|
|
roleId: TEST_ROLE_ID,
|
|
overrides: {
|
|
navigationMenuItems: [context.navigationMenuItem],
|
|
},
|
|
}),
|
|
expectToFail: true,
|
|
});
|
|
|
|
expectOneNotInternalServerErrorSnapshot({ errors });
|
|
},
|
|
60000,
|
|
);
|
|
});
|