-
-
Notifications
You must be signed in to change notification settings - Fork 18
Add new fields and enum for table settings: list_fields, list_per_page, ordering, and ordering_field #1512
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
Conversation
…e, ordering, and ordering_field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request restores four fields (list_fields, list_per_page, ordering, and ordering_field) to the common table settings entity that were previously moved to personal table settings only. This allows these fields to serve as defaults at the connection level, with personal settings able to override them. The builder function has been updated to implement a fallback pattern where personal settings take precedence over common settings.
Changes:
- Added database migration to restore four fields to the
tableSettingstable with appropriate types and constraints - Updated
TableSettingsEntityto include the new fields with proper TypeORM decorators - Modified
buildDAOsTableSettingsDsto implement fallback logic from personal to common settings - Applied consistent tab-based formatting throughout the affected files
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/src/migrations/1768987695763-RestoreFieldsInCommonTableSettings.ts | Adds migration to restore list_fields, list_per_page, ordering, and ordering_field columns to tableSettings table with proper up/down migrations |
| backend/src/entities/table-settings/common-table-settings/table-settings.entity.ts | Adds four new fields to TableSettingsEntity with appropriate TypeORM decorators and imports QueryOrderingEnum |
| shared-code/src/helpers/data-structures-builders/table-settings.ds.builder.ts | Updates type definitions and implements fallback logic for new fields in the builder function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| excluded_fields: commonTableSettings?.excluded_fields, | ||
| list_fields: personalTableSettings?.list_fields || commonTableSettings?.list_fields || [], | ||
| identification_fields: commonTableSettings?.identification_fields, | ||
| list_per_page: personalTableSettings?.list_per_page || commonTableSettings?.list_per_page, |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OR operator will treat 0 as falsy, which could be a valid value for list_per_page (meaning no pagination or 0 items per page). If a user sets personalTableSettings.list_per_page to 0, it will incorrectly fall back to commonTableSettings.list_per_page. Consider using nullish coalescing operator (??) instead of logical OR (||) to properly handle 0 as a valid value.
| list_per_page: personalTableSettings?.list_per_page || commonTableSettings?.list_per_page, | |
| list_per_page: personalTableSettings?.list_per_page ?? commonTableSettings?.list_per_page, |
| list_fields: personalTableSettings?.list_fields || commonTableSettings?.list_fields || [], | ||
| identification_fields: commonTableSettings?.identification_fields, | ||
| list_per_page: personalTableSettings?.list_per_page || commonTableSettings?.list_per_page, | ||
| ordering: personalTableSettings?.ordering || commonTableSettings?.ordering, | ||
| ordering_field: personalTableSettings?.ordering_field || commonTableSettings?.ordering_field, |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OR operator will treat empty strings as falsy values. If ordering_field is set to an empty string in personalTableSettings, it will incorrectly fall back to commonTableSettings.ordering_field instead of using the empty string. Consider using nullish coalescing operator (??) instead of logical OR (||) to properly distinguish between empty strings and null/undefined values.
| list_fields: personalTableSettings?.list_fields || commonTableSettings?.list_fields || [], | |
| identification_fields: commonTableSettings?.identification_fields, | |
| list_per_page: personalTableSettings?.list_per_page || commonTableSettings?.list_per_page, | |
| ordering: personalTableSettings?.ordering || commonTableSettings?.ordering, | |
| ordering_field: personalTableSettings?.ordering_field || commonTableSettings?.ordering_field, | |
| list_fields: personalTableSettings?.list_fields ?? commonTableSettings?.list_fields ?? [], | |
| identification_fields: commonTableSettings?.identification_fields, | |
| list_per_page: personalTableSettings?.list_per_page ?? commonTableSettings?.list_per_page, | |
| ordering: personalTableSettings?.ordering ?? commonTableSettings?.ordering, | |
| ordering_field: personalTableSettings?.ordering_field ?? commonTableSettings?.ordering_field, |
No description provided.