Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactored and improved seeds #8695

Merged
merged 31 commits into from
Dec 24, 2024
Merged

Refactored and improved seeds #8695

merged 31 commits into from
Dec 24, 2024

Conversation

lucasbordeau
Copy link
Contributor

@lucasbordeau lucasbordeau commented Nov 22, 2024

  • Added a new Seeder service to help with custom object seeds
  • Added RichTextFieldInput to edit a rich text field directly on the table, but deactivated it for now.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR adds comprehensive seeding functionality and rich text field support across the application, including new demo data and field types for testing.

  • Added RichTextFieldInput with BlockNote editor integration for inline rich text editing in tables
  • Introduced new SeederService with support for custom objects, composite fields, and improved data seeding capabilities
  • Added 10 new demo field types to Company seeds (UUID, RichText, Array, Rating, etc.) with corresponding metadata and views
  • Added new seed data files for pets and survey results with 100 records each for testing
  • Fixed color naming consistency from 'grey' to 'gray' across multiple theme and UI files

51 file(s) reviewed, 36 comment(s)
Edit PR Review Bot Settings | Greptile

fieldName,
);

const fieldValueParsed = parseJson<PartialBlock[]>(fieldValue);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: parseJson could return undefined/null - need to handle this case explicitly to avoid runtime errors when using fieldValueParsed

Comment on lines 334 to 335
@WorkspaceIsNullable()
demoRichText: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: demoRichText should be typed as FieldRichTextValue or string[] instead of string to match the RichText field type

@@ -44,8 +44,7 @@
"nx",
"run",
"twenty-server:command",
"my-command",
"--my-parameter value",
"workspace:seed:demo",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert

Copy link
Member

@Weiko Weiko Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 let's revert this

@@ -8,7 +8,7 @@ export const DEMO_SEED_WORKSPACE_MEMBER_IDS = {
TIM: '20202020-1553-45c6-a028-5a9064cce07e',
};

export const workspaceMemberPrefillData = async (
export const seedWorkspaceMemberWithDemoData = async (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we move it out from prefill data, aren't we breaking workspace creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check

Copy link
Member

@Weiko Weiko Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this breaking workspace creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ok

@FelixMalfait
Copy link
Member

Small comment from @Bonapara: the default workspace of Tim Apple should be Apple and not ACME

@Bonapara
Copy link
Member

Maybe we could use a fake logo to make it more fun:

image

@lucasbordeau lucasbordeau enabled auto-merge (squash) December 24, 2024 11:42
@@ -44,8 +44,7 @@
"nx",
"run",
"twenty-server:command",
"my-command",
"--my-parameter value",
"workspace:seed:demo",
Copy link
Member

@Weiko Weiko Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 let's revert this

throw new Error('No fields found for seeding, check metadata');
}

filteredFields.unshift({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "name" should already be there by the createOneObject?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes indeed, this bit of logic is not enough self-explanatory, it's here because the name field is inserted by default by the creation mechanism, so we have to add it to the array of fields here because we cannot control this mechanism, which could be problematic because it's like a silent behavior that is not obvious while working on this part of the code.

});

const fieldMetadataMap = filteredFields
.map((field) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: You should be able to use flatMap to avoid map().flat()

Suggested change
.map((field) => {
filteredFields
.flatMap((field) => {
if (!isCompositeFieldMetadataType(field.type)) {
return field.name;
}
const compositeFieldTypeDefinition = compositeTypeDefinitions.get(
field.type,
);
if (!isDefined(compositeFieldTypeDefinition)) {
throw new Error(
`Composite field type definition not found for ${field.type}`,
);
}
return (
compositeFieldTypeDefinition.properties?.map(
(property) => `${field.name}${capitalize(property.name)}`,
) ?? []
);
})
.filter(isDefined);

const flattenedSeed = {};

for (const field of filteredFields) {
if (isCompositeFieldMetadataType(field.type)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Could be refactored, looks like we are doing the same checks here and above

private readonly workspaceDataSourceService: WorkspaceDataSourceService,
) {}

public async seedCustomObjects(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This method seems to have a lot of responsibilities, could be refactored in smaller functions maybe?

@@ -8,7 +8,7 @@ export const DEMO_SEED_WORKSPACE_MEMBER_IDS = {
TIM: '20202020-1553-45c6-a028-5a9064cce07e',
};

export const workspaceMemberPrefillData = async (
export const seedWorkspaceMemberWithDemoData = async (
Copy link
Member

@Weiko Weiko Dec 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this breaking workspace creation?

@lucasbordeau lucasbordeau merged commit e971760 into main Dec 24, 2024
22 checks passed
@lucasbordeau lucasbordeau deleted the feat/new-seeds branch December 24, 2024 13:44
@lucasbordeau lucasbordeau mentioned this pull request Dec 24, 2024
Weiko pushed a commit that referenced this pull request Dec 24, 2024
Follow-up : Fixed relations eager load on refactored hooks from PR
#8695
samyakpiya pushed a commit to samyakpiya/twenty that referenced this pull request Dec 28, 2024
- Added a new Seeder service to help with custom object seeds
- Added RichTextFieldInput to edit a rich text field directly on the
table, but deactivated it for now.
samyakpiya pushed a commit to samyakpiya/twenty that referenced this pull request Dec 28, 2024
Follow-up : Fixed relations eager load on refactored hooks from PR
twentyhq#8695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants