-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Nc feat/links from existing cols #8367
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe update brings significant enhancements across components and services, focusing on advanced options, custom links, error handling, and relational settings management. These changes aim to boost flexibility, usability, and robustness in handling complex data relationships and user interactions. Changes
These changes collectively elevate the system's capability to handle intricate data structures and user interactions, showcasing ongoing architectural and functional enhancements in the application. Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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: 13
Out of diff range and nitpick comments (10)
tests/playwright/pages/Dashboard/Details/FieldsPage.ts (1)
189-189
: Clarify the logic for selecting relation types in thecreateOrUpdate
method.Consider adding a comment or refactoring this line to make the logic more understandable. For example, using a mapping object for relation types could improve readability.
packages/nocodb-sdk/src/lib/sqlUi/SqliteUi.ts (2)
Line range hint
559-700
: Ensure consistent data type handling ingetDataTypeForUiType
.The method
getDataTypeForUiType
maps UI types to database data types. However, there are inconsistencies in how data types are assigned. For example, bothPhoneNumber
andvarchar
, but they also include avalidate
property which is not used elsewhere. This could lead to confusion and potential bugs if the validation logic is not implemented consistently across the application. Consider standardizing how data types and validations are handled to ensure consistency and prevent errors.
Line range hint
700-931
: Review data type mappings ingetDataTypeListForUiType
.The method
getDataTypeListForUiType
provides a list of possible SQL data types based on the UI type. This method is crucial for ensuring that the data types used in the database are compatible with the UI elements. However, the method includes types likeyear
andgeometry
which are not standard SQLite data types. This could lead to errors when these types are used in a SQLite context. It's important to ensure that only compatible data types are listed for each UI type to avoid runtime errors and data integrity issues.packages/nocodb/src/services/columns.service.ts (1)
2113-2113
: Ensure consistent handling of thecustom
parameter in relation deletion.The
custom
parameter is passed to thedeleteHmOrBtRelation
method but not used within the method. If this parameter is intended to affect the behavior of the method, please ensure it is utilized appropriately. Otherwise, consider removing it from the parameter list to clean up the code.packages/nocodb/src/db/BaseModelSqlv2.ts (6)
Line range hint
1-1
: Consider breaking downBaseModelSqlv2
into smaller, more focused classes.Breaking down this large class into smaller classes each handling a specific part of the functionality (e.g.,
DataFormatter
,ErrorHandler
,QueryBuilder
) could improve modularity and maintainability.
Line range hint
1-1
: Add comprehensive documentation to the class and its methods.Proper documentation will make the codebase more accessible to new developers and aid in maintenance.
Line range hint
1-1
: Standardize error handling across methods.Implementing a consistent error handling strategy, possibly using a centralized error handling mechanism, could improve the robustness and consistency of the application.
Line range hint
1-1
: Optimize thegetColumnName
method to reduce complexity.Refactoring this method to simplify the conditional logic and possibly split it into smaller sub-methods could make it more readable and maintainable.
Line range hint
1-1
: Review and optimize the query building inreadByPk
.Optimizing the query building process in
readByPk
could improve performance, especially for large datasets.
Line range hint
1-1
: Optimize thelist
method for better performance with filters.Optimizing how filters are applied in the
list
method could enhance performance, particularly when dealing with large amounts of data.
// check column association with any custom links or LTAR | ||
if (!isVirtualCol(column)) { | ||
const columns = await table.getColumns(ncMeta); | ||
|
||
let link = columns.find((c) => { | ||
return ( | ||
isLinksOrLTAR(c.uidt) && | ||
((c.colOptions as LinkToAnotherRecordColumn)?.fk_child_column_id === | ||
param.columnId || | ||
(c.colOptions as LinkToAnotherRecordColumn)?.fk_parent_column_id === | ||
param.columnId || | ||
(c.colOptions as LinkToAnotherRecordColumn) | ||
?.fk_mm_child_column_id === param.columnId || | ||
(c.colOptions as LinkToAnotherRecordColumn) | ||
?.fk_mm_parent_column_id === param.columnId) | ||
); | ||
}); | ||
if (!link) { | ||
link = await ncMeta.metaGet2( | ||
null, | ||
null, | ||
MetaTable.COL_RELATIONS, | ||
{}, | ||
null, | ||
{ | ||
_or: [ | ||
{ fk_child_column_id: { eq: param.columnId } }, | ||
{ fk_parent_column_id: { eq: param.columnId } }, | ||
{ fk_mm_child_column_id: { eq: param.columnId } }, | ||
{ fk_mm_parent_column_id: { eq: param.columnId } }, | ||
], | ||
}, | ||
); | ||
} | ||
|
||
// if relation found then throw error | ||
if (link) { | ||
const linkCol = await Column.get({ colId: link.fk_column_id }, ncMeta); | ||
const table = await linkCol.getModel(ncMeta); | ||
NcError.columnAssociatedWithLink(column.id, { | ||
customMessage: `Column is associated with Link column '${ | ||
linkCol.title || linkCol.column_name | ||
}' (${ | ||
table.title || table.table_name | ||
}). Please delete the link column first.`, | ||
}); | ||
} |
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.
Check for potential race conditions in column deletion logic.
The method columnDelete
checks for column associations with custom links or LTAR before proceeding with the deletion. However, there's a potential race condition where the column could be modified between the check and the actual deletion operation. Consider implementing a transaction or locking mechanism to ensure atomicity and consistency.
if (!custom) { | ||
await this.deleteHmOrBtRelation( | ||
{ | ||
relationColOpt: null, | ||
parentColumn: parentColumn, | ||
childTable: mmTable, | ||
sqlMgr, | ||
parentTable: parentTable, | ||
childColumn: mmParentCol, | ||
source, | ||
ncMeta, | ||
virtual: !!relationColOpt.virtual, | ||
}, | ||
true, | ||
); | ||
|
||
await this.deleteHmOrBtRelation( | ||
{ | ||
relationColOpt: null, | ||
parentColumn: childColumn, | ||
childTable: mmTable, | ||
sqlMgr, | ||
parentTable: childTable, | ||
childColumn: mmChildCol, | ||
source, | ||
ncMeta, | ||
virtual: !!relationColOpt.virtual, | ||
}, | ||
true, | ||
); | ||
await this.deleteHmOrBtRelation( | ||
{ | ||
relationColOpt: null, | ||
parentColumn: childColumn, | ||
childTable: mmTable, | ||
sqlMgr, | ||
parentTable: childTable, | ||
childColumn: mmChildCol, | ||
source, | ||
ncMeta, | ||
virtual: !!relationColOpt.virtual, | ||
}, | ||
true, | ||
); | ||
} |
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.
Refactor deletion logic to avoid deep nesting and improve readability.
The deletion logic within the columnDelete
method for handling many-to-many relationships is deeply nested and hard to follow. Consider refactoring this into smaller, more manageable functions or methods to improve readability and maintainability.
- if (!custom) {
- await this.deleteHmOrBtRelation(
- {
- relationColOpt: null,
- parentColumn: parentColumn,
- childTable: mmTable,
- sqlMgr,
- parentTable: parentTable,
- childColumn: mmParentCol,
- source,
- ncMeta,
- virtual: !!relationColOpt.virtual,
- },
- true,
- );
- await this.deleteHmOrBtRelation(
- {
- relationColOpt: null,
- parentColumn: childColumn,
- childTable: mmTable,
- sqlMgr,
- parentTable: childTable,
- childColumn: mmChildCol,
- source,
- ncMeta,
- virtual: !!relationColOpt.virtual,
- },
- true,
- );
+ // Refactor into a new method
+ await handleManyToManyDeletion(relationColOpt, mmTable, parentColumn, childColumn, sqlMgr, ncMeta);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if (!custom) { | |
await this.deleteHmOrBtRelation( | |
{ | |
relationColOpt: null, | |
parentColumn: parentColumn, | |
childTable: mmTable, | |
sqlMgr, | |
parentTable: parentTable, | |
childColumn: mmParentCol, | |
source, | |
ncMeta, | |
virtual: !!relationColOpt.virtual, | |
}, | |
true, | |
); | |
await this.deleteHmOrBtRelation( | |
{ | |
relationColOpt: null, | |
parentColumn: childColumn, | |
childTable: mmTable, | |
sqlMgr, | |
parentTable: childTable, | |
childColumn: mmChildCol, | |
source, | |
ncMeta, | |
virtual: !!relationColOpt.virtual, | |
}, | |
true, | |
); | |
await this.deleteHmOrBtRelation( | |
{ | |
relationColOpt: null, | |
parentColumn: childColumn, | |
childTable: mmTable, | |
sqlMgr, | |
parentTable: childTable, | |
childColumn: mmChildCol, | |
source, | |
ncMeta, | |
virtual: !!relationColOpt.virtual, | |
}, | |
true, | |
); | |
} | |
if (!custom) { | |
// Refactor into a new method | |
await handleManyToManyDeletion(relationColOpt, mmTable, parentColumn, childColumn, sqlMgr, ncMeta); | |
} |
}); | ||
const parentColumn = await Column.get({ | ||
colId: ltarCustomPRops.column_id, | ||
}); | ||
|
||
if ( | ||
(param.column as LinkToAnotherColumnReqType).type === 'hm' || | ||
(param.column as LinkToAnotherColumnReqType).type === 'bt' | ||
) { | ||
await createHmAndBtColumn( | ||
child, | ||
parent, | ||
childColumn, | ||
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | ||
(param.column as LinkToAnotherColumnReqType).title, | ||
null, | ||
(param.column as LinkToAnotherColumnReqType).virtual, | ||
null, | ||
param.column['meta'], | ||
true, | ||
param.colExtra, | ||
parentColumn, | ||
true, | ||
); | ||
} else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | ||
await createOOColumn( | ||
child, | ||
parent, | ||
childColumn, | ||
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | ||
(param.column as LinkToAnotherColumnReqType).title, | ||
null, | ||
(param.column as LinkToAnotherColumnReqType).virtual, | ||
null, | ||
param.column['meta'], | ||
param.colExtra, | ||
parentColumn, | ||
true, | ||
); | ||
} else if ((param.column as LinkToAnotherColumnReqType).type === 'mm') { | ||
await Column.insert({ | ||
title: getUniqueColumnAliasName( | ||
await child.getColumns(), | ||
pluralize(parent.title), | ||
), | ||
uidt: param.column.uidt, | ||
type: 'mm', | ||
|
||
// ref_db_alias | ||
fk_model_id: child.id, | ||
// db_type: | ||
|
||
fk_child_column_id: childColumn.id, | ||
fk_parent_column_id: parentColumn.id, | ||
|
||
fk_mm_model_id: ltarCustomPRops.junc_model_id, | ||
fk_mm_child_column_id: ltarCustomPRops.junc_ref_column_id, | ||
fk_mm_parent_column_id: ltarCustomPRops.junc_column_id, | ||
fk_related_model_id: parent.id, | ||
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | ||
meta: { | ||
plural: pluralize(parent.title), | ||
singular: singularize(parent.title), | ||
custom: true, | ||
}, | ||
// if self referencing treat it as system field to hide from ui | ||
system: parent.id === child.id, | ||
}); | ||
await Column.insert({ | ||
title: getUniqueColumnAliasName( | ||
await parent.getColumns(), | ||
param.column.title ?? pluralize(child.title), | ||
), | ||
|
||
uidt: param.column.uidt, | ||
type: 'mm', | ||
|
||
fk_model_id: parent.id, | ||
|
||
fk_mm_model_id: ltarCustomPRops.junc_model_id, | ||
fk_mm_child_column_id: ltarCustomPRops.junc_column_id, | ||
fk_mm_parent_column_id: ltarCustomPRops.junc_ref_column_id, | ||
|
||
fk_child_column_id: parentColumn.id, | ||
fk_parent_column_id: childColumn.id, | ||
|
||
fk_related_model_id: child.id, | ||
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | ||
meta: { | ||
plural: param.column['meta']?.plural || pluralize(child.title), | ||
singular: | ||
param.column['meta']?.singular || singularize(child.title), | ||
custom: true, | ||
}, | ||
|
||
// column_order and view_id if provided | ||
...param.colExtra, | ||
}); | ||
} | ||
|
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.
Validate custom link creation parameters early in the method.
The method createLTARColumn
performs parameter validation after several database operations have already been initiated. This could lead to unnecessary database calls if the parameters are invalid. Consider moving the validation to the beginning of the method to fail fast and avoid unnecessary operations.
+ validateParams(['custom'], param.column as any);
+ validateParams(
+ ['column_id', 'ref_model_id', 'ref_column_id'],
+ (param.column as any).custom,
+ );
- const child = await Model.get(ltarCustomPRops.ref_model_id);
- const parent = await Model.get(param.tableId);
- const childColumn = await Column.get({
- colId: ltarCustomPRops.ref_column_id,
- });
- const parentColumn = await Column.get({
- colId: ltarCustomPRops.column_id,
- });
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
if ((param.column as any).is_custom_link) { | |
validateParams(['custom'], param.column as any); | |
validateParams( | |
['column_id', 'ref_model_id', 'ref_column_id'], | |
(param.column as any).custom, | |
); | |
const ltarCustomPRops: { | |
column_id: string; | |
ref_model_id: string; | |
ref_column_id: string; | |
junc_model_id: string; | |
junc_column_id: string; | |
junc_ref_column_id: string; | |
} = (param.column as any).custom; | |
const child = await Model.get(ltarCustomPRops.ref_model_id); | |
const parent = await Model.get(param.tableId); | |
const childColumn = await Column.get({ | |
colId: ltarCustomPRops.ref_column_id, | |
}); | |
const parentColumn = await Column.get({ | |
colId: ltarCustomPRops.column_id, | |
}); | |
if ( | |
(param.column as LinkToAnotherColumnReqType).type === 'hm' || | |
(param.column as LinkToAnotherColumnReqType).type === 'bt' | |
) { | |
await createHmAndBtColumn( | |
child, | |
parent, | |
childColumn, | |
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | |
(param.column as LinkToAnotherColumnReqType).title, | |
null, | |
(param.column as LinkToAnotherColumnReqType).virtual, | |
null, | |
param.column['meta'], | |
true, | |
param.colExtra, | |
parentColumn, | |
true, | |
); | |
} else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | |
await createOOColumn( | |
child, | |
parent, | |
childColumn, | |
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | |
(param.column as LinkToAnotherColumnReqType).title, | |
null, | |
(param.column as LinkToAnotherColumnReqType).virtual, | |
null, | |
param.column['meta'], | |
param.colExtra, | |
parentColumn, | |
true, | |
); | |
} else if ((param.column as LinkToAnotherColumnReqType).type === 'mm') { | |
await Column.insert({ | |
title: getUniqueColumnAliasName( | |
await child.getColumns(), | |
pluralize(parent.title), | |
), | |
uidt: param.column.uidt, | |
type: 'mm', | |
// ref_db_alias | |
fk_model_id: child.id, | |
// db_type: | |
fk_child_column_id: childColumn.id, | |
fk_parent_column_id: parentColumn.id, | |
fk_mm_model_id: ltarCustomPRops.junc_model_id, | |
fk_mm_child_column_id: ltarCustomPRops.junc_ref_column_id, | |
fk_mm_parent_column_id: ltarCustomPRops.junc_column_id, | |
fk_related_model_id: parent.id, | |
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | |
meta: { | |
plural: pluralize(parent.title), | |
singular: singularize(parent.title), | |
custom: true, | |
}, | |
// if self referencing treat it as system field to hide from ui | |
system: parent.id === child.id, | |
}); | |
await Column.insert({ | |
title: getUniqueColumnAliasName( | |
await parent.getColumns(), | |
param.column.title ?? pluralize(child.title), | |
), | |
uidt: param.column.uidt, | |
type: 'mm', | |
fk_model_id: parent.id, | |
fk_mm_model_id: ltarCustomPRops.junc_model_id, | |
fk_mm_child_column_id: ltarCustomPRops.junc_column_id, | |
fk_mm_parent_column_id: ltarCustomPRops.junc_ref_column_id, | |
fk_child_column_id: parentColumn.id, | |
fk_parent_column_id: childColumn.id, | |
fk_related_model_id: child.id, | |
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | |
meta: { | |
plural: param.column['meta']?.plural || pluralize(child.title), | |
singular: | |
param.column['meta']?.singular || singularize(child.title), | |
custom: true, | |
}, | |
// column_order and view_id if provided | |
...param.colExtra, | |
}); | |
} | |
if ((param.column as any).is_custom_link) { | |
validateParams(['custom'], param.column as any); | |
validateParams( | |
['column_id', 'ref_model_id', 'ref_column_id'], | |
(param.column as any).custom, | |
); | |
const ltarCustomPRops: { | |
column_id: string; | |
ref_model_id: string; | |
ref_column_id: string; | |
junc_model_id: string; | |
junc_column_id: string; | |
junc_ref_column_id: string; | |
} = (param.column as any).custom; | |
if ( | |
(param.column as LinkToAnotherColumnReqType).type === 'hm' || | |
(param.column as LinkToAnotherColumnReqType).type === 'bt' | |
) { | |
await createHmAndBtColumn( | |
child, | |
parent, | |
childColumn, | |
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | |
(param.column as LinkToAnotherColumnReqType).title, | |
null, | |
(param.column as LinkToAnotherColumnReqType).virtual, | |
null, | |
param.column['meta'], | |
true, | |
param.colExtra, | |
parentColumn, | |
true, | |
); | |
} else if ((param.column as LinkToAnotherColumnReqType).type === 'oo') { | |
await createOOColumn( | |
child, | |
parent, | |
childColumn, | |
(param.column as LinkToAnotherColumnReqType).type as RelationTypes, | |
(param.column as LinkToAnotherColumnReqType).title, | |
null, | |
(param.column as LinkToAnotherColumnReqType).virtual, | |
null, | |
param.column['meta'], | |
param.colExtra, | |
parentColumn, | |
true, | |
); | |
} else if ((param.column as LinkToAnotherColumnReqType).type === 'mm') { | |
await Column.insert({ | |
title: getUniqueColumnAliasName( | |
await child.getColumns(), | |
pluralize(parent.title), | |
), | |
uidt: param.column.uidt, | |
type: 'mm', | |
// ref_db_alias | |
fk_model_id: child.id, | |
// db_type: | |
fk_child_column_id: childColumn.id, | |
fk_parent_column_id: parentColumn.id, | |
fk_mm_model_id: ltarCustomPRops.junc_model_id, | |
fk_mm_child_column_id: ltarCustomPRops.junc_ref_column_id, | |
fk_mm_parent_column_id: ltarCustomPRops.junc_column_id, | |
fk_related_model_id: parent.id, | |
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | |
meta: { | |
plural: pluralize(parent.title), | |
singular: singularize(parent.title), | |
custom: true, | |
}, | |
// if self referencing treat it as system field to hide from ui | |
system: parent.id === child.id, | |
}); | |
await Column.insert({ | |
title: getUniqueColumnAliasName( | |
await parent.getColumns(), | |
param.column.title ?? pluralize(child.title), | |
), | |
uidt: param.column.uidt, | |
type: 'mm', | |
fk_model_id: parent.id, | |
fk_mm_model_id: ltarCustomPRops.junc_model_id, | |
fk_mm_child_column_id: ltarCustomPRops.junc_column_id, | |
fk_mm_parent_column_id: ltarCustomPRops.junc_ref_column_id, | |
fk_child_column_id: parentColumn.id, | |
fk_parent_column_id: childColumn.id, | |
fk_related_model_id: child.id, | |
virtual: (param.column as LinkToAnotherColumnReqType).virtual, | |
meta: { | |
plural: param.column['meta']?.plural || pluralize(child.title), | |
singular: | |
param.column['meta']?.singular || singularize(child.title), | |
custom: true, | |
}, | |
// column_order and view_id if provided | |
...param.colExtra, | |
}); | |
} |
static isEqual(dataType1: string, dataType2: string) { | ||
if (dataType1 === dataType2) return true; | ||
|
||
const abstractType1 = this.getAbstractType({ dt: dataType1 }); | ||
const abstractType2 = this.getAbstractType({ dt: dataType2 }); | ||
|
||
if ( | ||
abstractType1 && | ||
abstractType1 === abstractType2 && | ||
['integer', 'float'].includes(abstractType1) | ||
) | ||
return true; | ||
|
||
return false; | ||
} |
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.
Optimize and clarify the isEqual
method.
The isEqual
method is designed to compare two data types and determine if they are equivalent. While the method correctly handles basic comparisons and abstract type checks, the implementation could be optimized by directly returning the result of the condition in line 942 instead of using an if statement. Additionally, the method could benefit from more detailed comments explaining the rationale behind the abstract type comparisons, especially for types grouped under 'integer' and 'float'. This would improve the readability and maintainability of the code.
- if (
- abstractType1 &&
- abstractType1 === abstractType2 &&
- ['integer', 'float'].includes(abstractType1)
- )
- return true;
- return false;
+ return abstractType1 && abstractType1 === abstractType2 && ['integer', 'float'].includes(abstractType1);
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
static isEqual(dataType1: string, dataType2: string) { | |
if (dataType1 === dataType2) return true; | |
const abstractType1 = this.getAbstractType({ dt: dataType1 }); | |
const abstractType2 = this.getAbstractType({ dt: dataType2 }); | |
if ( | |
abstractType1 && | |
abstractType1 === abstractType2 && | |
['integer', 'float'].includes(abstractType1) | |
) | |
return true; | |
return false; | |
} | |
static isEqual(dataType1: string, dataType2: string) { | |
if (dataType1 === dataType2) return true; | |
const abstractType1 = this.getAbstractType({ dt: dataType1 }); | |
const abstractType2 = this.getAbstractType({ dt: dataType2 }); | |
return abstractType1 && abstractType1 === abstractType2 && ['integer', 'float'].includes(abstractType1); | |
} |
static isEqual(dataType1: string, dataType2: string) { | ||
if (dataType1 === dataType2) return true; | ||
|
||
const abstractType1 = this.getAbstractType({ dt: dataType1 }); | ||
const abstractType2 = this.getAbstractType({ dt: dataType2 }); | ||
|
||
if ( | ||
abstractType1 && | ||
abstractType1 === abstractType2 && | ||
['integer', 'float'].includes(abstractType1) | ||
) | ||
return true; | ||
|
||
return false; | ||
} |
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.
Tip
Codebase Verification
The verification process has confirmed that there are no unit tests for the isEqual
method in PgUi.ts
. It is crucial to add appropriate unit tests to ensure the method behaves as expected across all scenarios, especially given its role in comparing data types.
- Please add unit tests for the
isEqual
method to validate its functionality.
Analysis chain
The new isEqual
method introduces a flexible way to compare data types by abstracting them to a higher level. Ensure this method is covered by unit tests to validate its behavior across all expected data type comparisons.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that unit tests cover the new isEqual method in PgUi.ts
# Find test files related to PgUi.ts
fd 'PgUi.test.ts' --exec cat {} | grep 'isEqual'
Length of output: 48
Script:
#!/bin/bash
# Verify the existence of PgUi.test.ts and check its contents for references to 'isEqual'
# Check if PgUi.test.ts exists
echo "Checking for the existence of PgUi.test.ts:"
fd 'PgUi.test.ts'
# If exists, display the contents to verify if it contains tests for 'isEqual'
echo "Contents of PgUi.test.ts (if exists):"
fd 'PgUi.test.ts' --exec cat {}
Length of output: 233
const resetSelectedColumns = (isJunction: boolean = false, resetOnChangeDataType: boolean = false) => { | ||
if (isJunction) { | ||
if (vModel.value.custom.junc_column_id) { | ||
if (resetOnChangeDataType) { | ||
if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_column_id)?.dt) { | ||
vModel.value.custom.junc_column_id = null | ||
} | ||
} else { | ||
vModel.value.custom.junc_column_id = null | ||
} | ||
} | ||
|
||
if (vModel.value.custom.junc_ref_column_id) { | ||
if (resetOnChangeDataType) { | ||
if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_ref_column_id)?.dt) { | ||
vModel.value.custom.junc_ref_column_id = null | ||
} | ||
} else { | ||
vModel.value.custom.junc_ref_column_id = null | ||
} | ||
} | ||
} else { | ||
if (vModel.value.custom.ref_column_id) { | ||
if (resetOnChangeDataType) { | ||
if (sourceColumn.value?.dt !== refTableColumns.value.find((c) => c.id === vModel.value.custom.ref_column_id)?.dt) { | ||
vModel.value.custom.ref_column_id = null | ||
} | ||
} else { | ||
vModel.value.custom.ref_column_id = null | ||
} | ||
} | ||
} |
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.
Refactor resetSelectedColumns
to reduce redundancy and improve readability.
- if (isJunction) {
- if (vModel.value.custom.junc_column_id) {
- if (resetOnChangeDataType) {
- if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_column_id)?.dt) {
- vModel.value.custom.junc_column_id = null;
- }
- } else {
- vModel.value.custom.junc_column_id = null;
- }
- }
- if (vModel.value.custom.junc_ref_column_id) {
- if (resetOnChangeDataType) {
- if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_ref_column_id)?.dt) {
- vModel.value.custom.junc_ref_column_id = null;
- }
- } else {
- vModel.value.custom.junc_ref_column_id = null;
- }
- }
- } else {
- if (vModel.value.custom.ref_column_id) {
- if (resetOnChangeDataType) {
- if (sourceColumn.value?.dt !== refTableColumns.value.find((c) => c.id === vModel.value.custom.ref_column_id)?.dt) {
- vModel.value.custom.ref_column_id = null;
- }
- } else {
- vModel.value.custom.ref_column_id = null;
- }
- }
- }
+ const resetColumn = (columnKey, columns) => {
+ if (!resetOnChangeDataType || sourceColumn.value?.dt !== columns.value.find((c) => c.id === vModel.value[columnKey])?.dt) {
+ vModel.value[columnKey] = null;
+ }
+ };
+ if (isJunction) {
+ resetColumn('custom.junc_column_id', juncTableColumns);
+ resetColumn('custom.junc_ref_column_id', juncTableColumns);
+ } else {
+ resetColumn('custom.ref_column_id', refTableColumns);
+ }
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const resetSelectedColumns = (isJunction: boolean = false, resetOnChangeDataType: boolean = false) => { | |
if (isJunction) { | |
if (vModel.value.custom.junc_column_id) { | |
if (resetOnChangeDataType) { | |
if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_column_id)?.dt) { | |
vModel.value.custom.junc_column_id = null | |
} | |
} else { | |
vModel.value.custom.junc_column_id = null | |
} | |
} | |
if (vModel.value.custom.junc_ref_column_id) { | |
if (resetOnChangeDataType) { | |
if (sourceColumn.value?.dt !== juncTableColumns.value.find((c) => c.id === vModel.value.custom.junc_ref_column_id)?.dt) { | |
vModel.value.custom.junc_ref_column_id = null | |
} | |
} else { | |
vModel.value.custom.junc_ref_column_id = null | |
} | |
} | |
} else { | |
if (vModel.value.custom.ref_column_id) { | |
if (resetOnChangeDataType) { | |
if (sourceColumn.value?.dt !== refTableColumns.value.find((c) => c.id === vModel.value.custom.ref_column_id)?.dt) { | |
vModel.value.custom.ref_column_id = null | |
} | |
} else { | |
vModel.value.custom.ref_column_id = null | |
} | |
} | |
} | |
const resetSelectedColumns = (isJunction: boolean = false, resetOnChangeDataType: boolean = false) => { | |
const resetColumn = (columnKey, columns) => { | |
if (!resetOnChangeDataType || sourceColumn.value?.dt !== columns.value.find((c) => c.id === vModel.value[columnKey])?.dt) { | |
vModel.value[columnKey] = null; | |
} | |
}; | |
if (isJunction) { | |
resetColumn('custom.junc_column_id', juncTableColumns); | |
resetColumn('custom.junc_ref_column_id', juncTableColumns); | |
} else { | |
resetColumn('custom.ref_column_id', refTableColumns); | |
} | |
} |
const isAdvancedOptionsShownEasterEgg = ref(false) | ||
|
||
const cusValidators = { | ||
'custom.column_id': [{ required: true, message: t('general.required') }], | ||
'custom.ref_model_id': [{ required: true, message: t('general.required') }], | ||
'custom.ref_column_id': [{ required: true, message: t('general.required') }], | ||
} | ||
|
||
const cusJuncTableValidations = { | ||
'custom.junc_model_id': [{ required: true, message: t('general.required') }], | ||
'custom.junc_column_id': [{ required: true, message: t('general.required') }], | ||
'custom.junc_ref_column_id': [{ required: true, message: t('general.required') }], | ||
} | ||
|
||
const onCustomSwitchToggle = () => { | ||
if (vModel.value?.is_custom_link) | ||
setAdditionalValidations({ | ||
childId: [], | ||
...cusValidators, | ||
...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}), | ||
}) | ||
else | ||
setAdditionalValidations({ | ||
childId: [{ required: true, message: t('general.required') }], | ||
}) | ||
} | ||
|
||
const handleShowAdvanceOptions = () => { | ||
isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value | ||
|
||
if (!isAdvancedOptionsShownEasterEgg.value) { | ||
vModel.value.is_custom_link = false | ||
} | ||
} |
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.
Consider simplifying the toggle logic for advanced options.
- const onCustomSwitchToggle = () => {
- if (vModel.value?.is_custom_link)
- setAdditionalValidations({
- childId: [],
- ...cusValidators,
- ...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}),
- })
- else
- setAdditionalValidations({
- childId: [{ required: true, message: t('general.required') }],
- })
- }
- const handleShowAdvanceOptions = () => {
- isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value
- if (!isAdvancedOptionsShownEasterEgg.value) {
- vModel.value.is_custom_link = false
- }
- }
+ const toggleAdvancedOptions = () => {
+ isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value;
+ vModel.value.is_custom_link = isAdvancedOptionsShownEasterEgg.value;
+ setAdditionalValidations(isAdvancedOptionsShownEasterEgg.value ? {
+ childId: [],
+ ...cusValidators,
+ ...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}),
+ } : {
+ childId: [{ required: true, message: t('general.required') }],
+ });
+ }
This refactoring combines the logic of onCustomSwitchToggle
and handleShowAdvanceOptions
into a single method, reducing redundancy and improving readability.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const isAdvancedOptionsShownEasterEgg = ref(false) | |
const cusValidators = { | |
'custom.column_id': [{ required: true, message: t('general.required') }], | |
'custom.ref_model_id': [{ required: true, message: t('general.required') }], | |
'custom.ref_column_id': [{ required: true, message: t('general.required') }], | |
} | |
const cusJuncTableValidations = { | |
'custom.junc_model_id': [{ required: true, message: t('general.required') }], | |
'custom.junc_column_id': [{ required: true, message: t('general.required') }], | |
'custom.junc_ref_column_id': [{ required: true, message: t('general.required') }], | |
} | |
const onCustomSwitchToggle = () => { | |
if (vModel.value?.is_custom_link) | |
setAdditionalValidations({ | |
childId: [], | |
...cusValidators, | |
...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}), | |
}) | |
else | |
setAdditionalValidations({ | |
childId: [{ required: true, message: t('general.required') }], | |
}) | |
} | |
const handleShowAdvanceOptions = () => { | |
isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value | |
if (!isAdvancedOptionsShownEasterEgg.value) { | |
vModel.value.is_custom_link = false | |
} | |
} | |
const isAdvancedOptionsShownEasterEgg = ref(false) | |
const cusValidators = { | |
'custom.column_id': [{ required: true, message: t('general.required') }], | |
'custom.ref_model_id': [{ required: true, message: t('general.required') }], | |
'custom.ref_column_id': [{ required: true, message: t('general.required') }], | |
} | |
const cusJuncTableValidations = { | |
'custom.junc_model_id': [{ required: true, message: t('general.required') }], | |
'custom.junc_column_id': [{ required: true, message: t('general.required') }], | |
'custom.junc_ref_column_id': [{ required: true, message: t('general.required') }], | |
} | |
const toggleAdvancedOptions = () => { | |
isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value; | |
vModel.value.is_custom_link = isAdvancedOptionsShownEasterEgg.value; | |
setAdditionalValidations(isAdvancedOptionsShownEasterEgg.value ? { | |
childId: [], | |
...cusValidators, | |
...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}), | |
} : { | |
childId: [{ required: true, message: t('general.required') }], | |
}); | |
} |
@@ -299,7 +299,7 @@ const isDeleteAllowed = computed(() => { | |||
return column?.value && !column.value.system | |||
}) | |||
const isDuplicateAllowed = computed(() => { | |||
return column?.value && !column.value.system | |||
return column?.value && !column.value.system && !column.value.meta?.custom |
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.
Tip
Codebase Verification
The verification process has successfully identified the implementation of the isDuplicateAllowed
computed property in the Menu.vue
and FieldMenu.vue
components within the nc-gui
package. The condition for isDuplicateAllowed
is consistent across these components, as shown in the script output:
- In
Menu.vue
, the condition is:return column?.value && !column.value.system && !column.value.meta?.custom
- In
FieldMenu.vue
, the condition is slightly different:return column?.value && !column.value.system
This indicates that while the condition is implemented in both components, there is a variation in the FieldMenu.vue
where the check for column.value.meta?.custom
is not included.
Conclusion:
- The new condition for
isDuplicateAllowed
is implemented in the identified Vue components. - There is a discrepancy in the implementation between
Menu.vue
andFieldMenu.vue
which might be intentional or an oversight.
Action Points:
- Confirm if the difference in conditions between
Menu.vue
andFieldMenu.vue
is intentional. - If the discrepancy is not intended, align the condition in
FieldMenu.vue
with that inMenu.vue
.
Analysis chain
Ensure the new condition for isDuplicateAllowed
is correctly implemented.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new condition for `isDuplicateAllowed` is correctly implemented across the system.
# Find files that use the `isDuplicateAllowed` computed property.
rg --files-with-matches --type vue $'isDuplicateAllowed' | while read file; do
# Test: Search for the usage of the `isDuplicateAllowed` to validate the new condition. Expect: Matches.
ast-grep --lang vue --pattern $'isDuplicateAllowed' "$file"
done
Length of output: 95
Script:
#!/bin/bash
# Description: Verify that the new condition for `isDuplicateAllowed` is correctly implemented across the system.
# Find Vue files and check for the usage of `isDuplicateAllowed`.
rg --files-with-matches --glob '*.vue' 'isDuplicateAllowed' | while read file; do
# Test: Search for the usage of the `isDuplicateAllowed` to validate the new condition. Expect: Matches.
ast-grep --lang vue --pattern $'isDuplicateAllowed' "$file"
done
Length of output: 522
Script:
#!/bin/bash
# Description: Verify that the new condition for `isDuplicateAllowed` is correctly implemented across the system.
# Find Vue files and check for the usage and context of `isDuplicateAllowed`.
rg --glob '*.vue' -C 3 'isDuplicateAllowed'
Length of output: 2826
// todo: enable later | ||
// const modelId = (tables.find((t) => t.id === tableIdOrTitle) || tables.find((t) => t.title === tableIdOrTitle))?.id | ||
// | ||
// if (!modelId) { | ||
// console.warn(`Table '${tableIdOrTitle}' is not found in the table list`) | ||
// return null | ||
// } | ||
|
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.
Consider removing or implementing the commented-out code to avoid confusion and maintain code cleanliness.
let dashboard: DashboardPage; | ||
let context: any; | ||
|
||
// todo: Break the test into smaller tests |
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.
TODO: Break the test into smaller tests.
Would you like assistance in breaking down this test into smaller, more manageable units? This can improve test maintainability and readability.
Uffizzi Ephemeral Environment
|
59c1b90
to
06c16b2
Compare
06c16b2
to
e13413a
Compare
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: 1
Out of diff range and nitpick comments (1)
packages/nc-gui/components/general/DeleteModal.vue (1)
63-69
: Add a descriptive comment for the warning slot.It would be beneficial to add a comment explaining the purpose of the warning slot, which helps maintainers understand its use without digging through the entire codebase.
const onCustomSwitchToggle = () => { | ||
if (vModel.value?.is_custom_link) | ||
setAdditionalValidations({ | ||
childId: [], | ||
...cusValidators, | ||
...(vModel.value.type === RelationTypes.MANY_TO_MANY ? cusJuncTableValidations : {}), | ||
}) | ||
else | ||
setAdditionalValidations({ | ||
childId: [{ required: true, message: t('general.required') }], | ||
}) | ||
} | ||
|
||
const handleShowAdvanceOptions = () => { | ||
isAdvancedOptionsShownEasterEgg.value = !isAdvancedOptionsShownEasterEgg.value | ||
|
||
if (!isAdvancedOptionsShownEasterEgg.value) { | ||
vModel.value.is_custom_link = false | ||
} | ||
} |
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.
Tip
Codebase Verification
The refactoring of the toggle logic in LinkedToAnotherRecordOptions.vue
does not appear to have been implemented as expected. The old method names onCustomSwitchToggle
and handleShowAdvanceOptions
are still present in the file. This suggests that the suggested changes were not made or were not made correctly.
- Location:
packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
Analysis chain
Ensure the refactored toggle logic is implemented as suggested.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the refactored toggle logic is implemented in the `LinkedToAnotherRecordOptions.vue` file.
# Test: Search for the old method names to find omissions. Expect: No matches.
rg --files-with-matches --type vue $'onCustomSwitchToggle|handleShowAdvanceOptions' packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
Length of output: 190
Script:
#!/bin/bash
# Description: Verify that the refactored toggle logic is implemented in the `LinkedToAnotherRecordOptions.vue` file.
# Test: Search for the old method names to find omissions. Expect: No matches.
rg --files-with-matches 'onCustomSwitchToggle|handleShowAdvanceOptions' packages/nc-gui/components/smartsheet/column/LinkedToAnotherRecordOptions.vue
Length of output: 229
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: 3
const childBaseModel = await Model.getBaseModelSQL({ | ||
model: childTable, | ||
dbDriver: this.dbDriver, | ||
}); | ||
|
||
const parentBaseModel = await Model.getBaseModelSQL({ | ||
model: childTable, | ||
dbDriver: this.dbDriver, | ||
}); |
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.
Ensure correct model assignment in parentBaseModel
initialization.
- const parentBaseModel = await Model.getBaseModelSQL({
- model: childTable,
- dbDriver: this.dbDriver,
- });
+ const parentBaseModel = await Model.getBaseModelSQL({
+ model: parentTable,
+ dbDriver: this.dbDriver,
+ });
The parentBaseModel
should be initialized with parentTable
instead of childTable
. This appears to be a copy-paste error that could lead to incorrect behavior in methods that rely on parentBaseModel
.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const childBaseModel = await Model.getBaseModelSQL({ | |
model: childTable, | |
dbDriver: this.dbDriver, | |
}); | |
const parentBaseModel = await Model.getBaseModelSQL({ | |
model: childTable, | |
dbDriver: this.dbDriver, | |
}); | |
const childBaseModel = await Model.getBaseModelSQL({ | |
model: childTable, | |
dbDriver: this.dbDriver, | |
}); | |
const parentBaseModel = await Model.getBaseModelSQL({ | |
model: parentTable, | |
dbDriver: this.dbDriver, | |
}); |
const assocBaseModel = await Model.getBaseModelSQL({ | ||
model: vTable, | ||
dbDriver: this.dbDriver, | ||
}); |
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.
Verify the correct model assignment in assocBaseModel
initialization.
- const assocBaseModel = await Model.getBaseModelSQL({
- model: vTable,
- dbDriver: this.dbDriver,
- });
+ const assocBaseModel = await Model.getBaseModelSQL({
+ model: childTable,
+ dbDriver: this.dbDriver,
+ });
The assocBaseModel
should be initialized with childTable
instead of vTable
. This seems to be an error in assigning the correct model, which could affect the functionality of the removeLinks
method where this model is used.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const assocBaseModel = await Model.getBaseModelSQL({ | |
model: vTable, | |
dbDriver: this.dbDriver, | |
}); | |
const assocBaseModel = await Model.getBaseModelSQL({ | |
model: childTable, | |
dbDriver: this.dbDriver, | |
}); |
const parentBaseModel = await Model.getBaseModelSQL({ | ||
model: childTable, | ||
dbDriver: this.dbDriver, | ||
}); |
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.
Check the model assignment for parentBaseModel
.
- const parentBaseModel = await Model.getBaseModelSQL({
- model: childTable,
- dbDriver: this.dbDriver,
- });
+ const parentBaseModel = await Model.getBaseModelSQL({
+ model: parentTable,
+ dbDriver: this.dbDriver,
+ });
The parentBaseModel
is incorrectly initialized with childTable
. It should be initialized with parentTable
to ensure that the operations performed using parentBaseModel
are correct.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
const parentBaseModel = await Model.getBaseModelSQL({ | |
model: childTable, | |
dbDriver: this.dbDriver, | |
}); | |
const parentBaseModel = await Model.getBaseModelSQL({ | |
model: parentTable, | |
dbDriver: this.dbDriver, | |
}); |
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of