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

Fix seeds #7543

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Fix seeds #7543

merged 3 commits into from
Oct 9, 2024

Conversation

ijreilly
Copy link
Collaborator

@ijreilly ijreilly commented Oct 9, 2024

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 pull request addresses seed data and field validation issues in the Twenty application. Here's a concise summary of the key changes:

  • Updated seed data for company and people custom fields in fieldsMetadata.ts
    • Added default value for 'tagline' field in company custom fields
    • Changed 'isNullable' property of 'whatsapp' field in people custom fields to true
  • Improved non-nullable field validation in field-metadata.service.ts
    • Imported isDefined function for more accurate checks
    • Updated validateFieldMetadataInput method to use isDefined for default value validation
  • These changes enhance data consistency and prevent potential validation errors for falsy values

2 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -15,6 +15,7 @@ export const getDevSeedCompanyCustomFields = (
icon: 'IconAdCircle',
isActive: true,
isNullable: false,
defaultValue: "''",
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: The default value for 'tagline' is set to an empty string. Ensure this is the intended behavior and doesn't conflict with the 'isNullable: false' setting.

Copy link
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -98,7 +99,7 @@ export const getDevSeedPeopleCustomFields = (
description: "Contact's Whatsapp Number",
icon: 'IconBrandWhatsapp',
isActive: true,
isNullable: false,
isNullable: true,
Copy link
Member

@Weiko Weiko Oct 9, 2024

Choose a reason for hiding this comment

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

could remain false since you set a default value? up to you!

@charlesBochet charlesBochet merged commit b80fa9c into main Oct 9, 2024
7 of 8 checks passed
@charlesBochet charlesBochet deleted the fix-seeds branch October 9, 2024 15:03
@ijreilly ijreilly mentioned this pull request Oct 10, 2024
ijreilly added a commit that referenced this pull request Oct 10, 2024
In [this](#7522) and
[this](#7543) PR we introduced
the impossibility to save a field that would be non nullable but without
a default value.
The check is actually called on the input while the defaultValue is
added by the service on a "built" fieldMetadata to create or save. So
far all fields created from the app it currently fails as both
isNullable and defaultValue are undefined so falsy at that stage.
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
- Fixing seeds after introducing the requirement for non-nullable fields
to have a default value (twentyhq#7522).
- Empty string needs to be considered a valid default value
harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
In [this](twentyhq#7522) and
[this](twentyhq#7543) PR we introduced
the impossibility to save a field that would be non nullable but without
a default value.
The check is actually called on the input while the defaultValue is
added by the service on a "built" fieldMetadata to create or save. So
far all fields created from the app it currently fails as both
isNullable and defaultValue are undefined so falsy at that stage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants