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

Enhance postgre sql setup script and documentation for various distros 7636 #7637

Conversation

dostavic
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 pull request enhances the PostgreSQL setup script, improves the SettingsObjectNewFieldConfigure component, and introduces a new file for mapping field types to icons.

  • Enhanced provision-postgres-linux.sh to support multiple Linux distributions (Debian/Ubuntu and Arch Linux)
  • Added packages/twenty-front/src/pages/settings/data-model/constants/FieldTypeIcons.ts for mapping field types to default icons
  • Updated SettingsObjectNewFieldConfigure.tsx to use default icons and improve form configuration
  • Improved error handling and user prompts in the PostgreSQL setup script
  • Added a new useEffect hook in SettingsObjectNewFieldConfigure.tsx to update icons when field type changes

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

Comment on lines 71 to 73
useEffect(() => {
formConfig.setValue('icon', defaultIconsByFieldType[fieldType] || 'IconUsers');
}, [fieldType, formConfig]);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a dependency array to this useEffect to prevent unnecessary re-renders

Comment on lines 208 to 212
fieldMetadataItem={{
icon: formConfig.watch('icon'),
label: formConfig.watch('label') || 'New Field',
type: fieldType as FieldMetadataType,
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Ensure that fieldType is always a valid FieldMetadataType to avoid potential runtime errors

Comment on lines 23 to 26
[FieldMetadataType.Numeric]: 'IconUsers',
[FieldMetadataType.Position]: 'IconUsers',
[FieldMetadataType.RichText]: 'IconUsers',
[FieldMetadataType.TsVector]: 'IconUsers'
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using more specific icons for Numeric, Position, RichText, and TsVector field types instead of the generic IconUsers

Comment on lines +101 to +103
if ! yay -S --noconfirm pg_graphql; then
handle_error "Failed to install pg_graphql package from AUR."
fi
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 script assumes 'yay' is installed for Arch Linux, but doesn't check for its presence or provide an alternative. This could cause issues for users without yay.

Comment on lines +106 to +110
if sudo -u postgres sh -c 'test "$(ls -A /var/lib/postgres/data 2>/dev/null)"'; then
echo "PostgreSQL data directory already contains data. Skipping initdb."
else
sudo -iu postgres initdb --locale en_US.UTF-8 -D /var/lib/postgres/data || handle_error "Failed to initialize PostgreSQL database."
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

style: The indentation for the 'else' block is incorrect, which could lead to readability issues.

@@ -0,0 +1,27 @@
import { FieldMetadataType } from '~/generated-metadata/graphql';
Copy link
Member

Choose a reason for hiding this comment

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

@dostavic what is this change about?

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Thank you @dostavic

I'm not sure about the FieldTypeIcon.ts file change? Could you remove this change?

Regarding the provision-postres-linux.sh, looks great! I don't have linux setup on my side so I will trust you on this one :)

@dostavic
Copy link
Contributor Author

Thank you, @charlesBochet!

Regarding the file change, that was my mistake 😊. I've removed it now

@charlesBochet
Copy link
Member

Thank you for improving the experience on Linux. If you see anything else, do not hesitate!

@charlesBochet charlesBochet merged commit a4e52c5 into twentyhq:main Oct 13, 2024
3 checks passed
@charlesBochet
Copy link
Member

/award 300

Copy link

oss-gg bot commented Oct 13, 2024

Awarding dostavic: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/dostavic

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.

2 participants