-
Notifications
You must be signed in to change notification settings - Fork 760
WEB-368 The UI must not offer to edit or delete the relationship column of a (custom) Data Table #2740
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
…lationship column of a (custom) Data Table"
WalkthroughThe EditDataTableComponent is enhanced with a new method to map application table names to relationship column names. Constructor and initialization logic are updated to conditionally treat relationship columns as system columns and to only remove the 'id' column if it's actually the first column. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts (1)
179-195: Move the table mapping to a class-level constant.The
tableToColumnMapobject is recreated on every method call. Moving it to a class-level constant would improve performance and make it easier to maintain.Apply this diff to refactor:
+ /** Mapping of application table names to relationship column names */ + private static readonly TABLE_TO_COLUMN_MAP: { [key: string]: string } = { + m_client: 'client_id', + m_group: 'group_id', + m_center: 'center_id', + m_office: 'office_id', + m_loan: 'loan_id', + m_savings_account: 'savings_account_id', + m_savings_account_transaction: 'savings_transaction_id', + m_product_loan: 'product_loan_id', + m_savings_product: 'savings_product_id', + m_share_product: 'share_product_id' + }; + /** * Gets the relationship column name. * @param {string} appTableName Application table name. * @returns {string} Relationship column name. */ getRelationshipColumnName(appTableName: string): string { - // Map application table names to their relationship column names - const tableToColumnMap: { [key: string]: string } = { - m_client: 'client_id', - m_group: 'group_id', - m_center: 'center_id', - m_office: 'office_id', - m_loan: 'loan_id', - m_savings_account: 'savings_account_id', - m_savings_account_transaction: 'savings_transaction_id', - m_product_loan: 'product_loan_id', - m_savings_product: 'savings_product_id', - m_share_product: 'share_product_id' - }; - - return tableToColumnMap[appTableName] || ''; + return EditDataTableComponent.TABLE_TO_COLUMN_MAP[appTableName] || ''; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (4)
src/app/system/manage-data-tables/edit-data-table/edit-data-table.component.ts (4)
158-159: LGTM!The relationship column name computation is correctly positioned and clearly implemented.
161-168: LGTM!The system column marking logic correctly identifies and protects both standard system columns (id, created_at, updated_at) and the entity-specific relationship column.
222-226: LGTM!The conditional check properly guards against removing the wrong column. The improvement from unconditional removal to checking for the 'id' column name is a solid defensive coding practice.
179-195: All application table names are properly mapped.Verification confirms that all 10 tables from
appTableData(m_client, m_group, m_center, m_office, m_loan, m_savings_account, m_product_loan, m_savings_account_transaction, m_savings_product, m_share_product) are included in thetableToColumnMap. The mapping is complete and covers all application tables.
|
@gkbishnoi07 plz take a look |
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.
LGTM
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.
thanks for the PR
Description
This change prevents users from editing or deleting system-generated columns when editing a custom Data Table. When a Data Table is created and linked to an entity (e.g., m_office, m_client, m_loan), Fineract automatically generates several columns including a foreign key relationship column (office_id, client_id, loan_id, etc.), along with created_at and updated_at timestamps. Previously, the UI displayed edit and delete buttons for all columns including these system-generated ones, which would result in backend errors if users attempted to modify them.
The fix identifies and marks these system-generated columns (id, created_at, updated_at, and the relationship column) with a system: true flag in the constructor. The existing template conditional (*ngIf="!column.system") then hides the edit and delete buttons for these protected columns, making them read-only like they should be. Only user-defined custom columns now display the edit and delete buttons.
A helper method getRelationshipColumnName() was added to map entity types to their corresponding relationship column names, and the initData() method was updated to properly handle the id column removal while preserving the relationship column as a visible but locked field.
Related issues and discussion
#WEB-368
Screenshots, if any
BEFORE
AFTER
WHEN DIFFERENT ENTITY TYPE (loan product)
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit