-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Folder authorization frontend #4186
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
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| function clickMemberHandle(item: any) { | ||
| currentUser.value = item.id | ||
| currentType.value = item.type |
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 provided code has a few areas that could be improved:
-
Duplicated Code: The
toTreefunction is duplicated inside thecomputed('treeData'). It would be more efficient to define this function outside of the component. -
Logical Errors in Tree Construction: There's an error where you're creating a new node map but not properly using it during the tree construction process. This can lead to incorrect results when constructing the tree structure.
-
Pagination Configuration: In the original
getPermissionListmethod, the pagination configuration wasn’t being passed correctly. This should be addressed to ensure proper pagination handling if needed in the future.
Here is the corrected version of the code with some suggested improvements:
// Define the toTree function outside the component
function toTree(nodeList: any[], parentIdKey = 'parentId'): { id: number; children?: Array<{ id: number }> }[] {
// Convert the nodeList into an object for quick lookups
const nodeMap = Object.fromEntries(nodeList.map((item: any) => [item.id, item]));
// Build the tree structure
nodeList.forEach((element) => {
if (!element.children) {
element.children = [];
}
// Find the parent element based on parentIdKey
if (element[parentIdKey]) {
const pParent = nodeMap[element[parentIdKey]];
if (pParent) {
pParent.children.push(element);
}
}
});
// Filter out root nodes (nodes without a parentId)
return nodeList.filter((item) => !item[parentIdKey]);
}
// Computed property for tree data
const treeData = computed(() => {
const resourceType = route.meta?.resource || 'APPLICATION';
if (resourceType === 'MODEL') {
return permissionData;
}
return toTree(permissionData.value, 'folder_id');
});With these changes:
- The
toTreefunction is defined once and reused across different parts. - Logical errors related to building the tree are fixed by ensuring all necessary nodes have their corresponding parents added to the correct level.
- Proper use of pagination parameters in
getPermissionListremains omitted since no such parameters were shown in the snippet. Adjustments here depend on your application’s requirements.
Additionally, consider adding type annotations to improve code readability and maintainability further.
| function refreshFolder() { | ||
| emit('refreshTree') | ||
| } |
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 provided code is clean and well-organized. Here are some minor improvements and suggestions:
-
Type Annotations: Since
currentNodeis used within template expressions, it should be declared with a type annotation (ref<(...args: any[]) => void>). -
Arrow Function Shorthand: The arrow function for
openAuthorizationcan be simplified ifthis.is not needed.
After these changes, the code looks like this:
<div id="file-list-container">
<el-scrollbar style="height: calc(100vh - 35px);">
<!-- Existing JSX content -->
</el-scrollbar>
</div>
<CreateFolderDialog ref="CreateFolderDialogRef" @refresh="refreshFolder" :title="title" />
<ResourceAuthorizationDrawer
:type="props.source"
:is-folder="true"
:is-root-folder="!currentNode?.parent_id"
ref="ResourceAuthorizationDrawerRef"
/>Additional Suggestion:
Consider adding more descriptive variable names to help clarify their purpose. For example:
currentFolder: Instead of usingdata, which could be ambiguous due to its use in other contexts.resourceAuthdrawerInstance: Describes the instance being managed by the drawer component.
These small adjustments make the code easier to read and maintain.
| }) || [] | ||
| paginationConfig.total = res.data.total || 0 | ||
| }) | ||
| } |
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.
Your code has several improvements and optimizations:
-
Template Refactoring: The
permissionOptionstemplate loop is replaced with calling a new methodgetFolderPermissionOptions(). This modularization makes the component easier to maintain and reduces repetition. -
Optimization of Permission Retrieval:
- A map (
permissionOptionMap) is introduced to store options based on whether it's a root or regular folder. getFolderPermissionOptions()determines which set of options should be used based on the provided prop values (isRootFolderorisFolder).
- A map (
-
Handling Special Case:
- If
props.isRootFolderand an item’s permission is'NOT_AUTH', it returns'VIEW'.
- If
These changes improve readability and efficiency by encapsulating logic related to handling different types of folders into separate methods, reducing redundant repeated code, and providing better control over when certain conditions apply.
feat: Folder authorization frontend