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

Allow standard field default value and settings editing (#7104) #8559

Merged
merged 5 commits into from
Nov 26, 2024

Conversation

ad-elias
Copy link
Contributor

No description provided.

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 enables editing of default values and settings for standard fields, removing previous restrictions that only allowed modifications to custom fields.

  • Modified buildUpdatableStandardFieldInput in field-metadata.service.ts to include defaultValue and settings for standard fields
  • Removed isCreatingField prop from SettingsDataModelFieldSettingsFormCard to control field settings via isCustom property
  • Removed edit mode restriction in SettingsDataModelFieldBooleanForm to allow default value editing for existing boolean fields
  • Added support for MULTI_SELECT fields alongside SELECT fields in field metadata service
  • Removed canPersistFieldMetadataItemUpdate function that previously restricted updates to custom fields only

5 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 194 to 201
actionButton={
shouldDisplaySaveAndCancel && (
<SaveAndCancelButtons
isSaveDisabled={!canSave}
isCancelDisabled={isSubmitting}
onCancel={() => navigate(`/settings/objects/${objectSlug}`)}
onSave={formConfig.handleSubmit(handleSave)}
/>
)
<SaveAndCancelButtons
isSaveDisabled={!canSave}
isCancelDisabled={isSubmitting}
onCancel={() => navigate(`/settings/objects/${objectSlug}`)}
onSave={formConfig.handleSubmit(handleSave)}
/>
}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Removing the restriction on standard field editing could have unintended consequences. Consider adding validation to prevent critical system fields from being modified.

@@ -44,9 +42,6 @@ export const SettingsDataModelFieldBooleanForm = ({
value={value}
onChange={onChange}
selectClassName={className}
// TODO: temporary fix - disabling edition because after editing the defaultValue,
// newly created records are not taking into account the updated defaultValue properly.
Copy link
Contributor Author

@ad-elias ad-elias Nov 18, 2024

Choose a reason for hiding this comment

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

Could not replicate this issue so removed the comment

@ad-elias ad-elias changed the title Allow standard field default value and settings editing Allow standard field default value and settings editing (#7104) Nov 18, 2024
@ad-elias
Copy link
Contributor Author

Actually not ready to be merged yet, npx nx run twenty-server:command workspace:sync-metadata -f overrides edited default value. settings is not getting overridden.

@ijreilly
Copy link
Collaborator

@ad-elias can you ping me when this is ready for review again? thanks :)

@ad-elias
Copy link
Contributor Author

@ijreilly Ready for review!

Copy link
Collaborator

@ijreilly ijreilly left a comment

Choose a reason for hiding this comment

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

great work! thanks a lot!
(added a little commit to include Number fields that had been forgotten i think).

@ijreilly ijreilly merged commit 2e75fae into twentyhq:main Nov 26, 2024
17 checks passed
Copy link

Thanks @ad-elias for your contribution!
This marks your 11th PR on the repo. You're top 3% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

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.

3 participants