Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions apps/folders/serializers/folder.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ def get_folder_tree_serializer(source):
return None


FOLDER_DEPTH = 2 # Folder 不能超过3层
FOLDER_DEPTH = 10000


def check_depth(source, parent_id, workspace_id, current_depth=0):
Expand All @@ -79,7 +79,7 @@ def check_depth(source, parent_id, workspace_id, current_depth=0):

# 验证层级深度
if depth + current_depth > FOLDER_DEPTH:
raise serializers.ValidationError(_('Folder depth cannot exceed 3 levels'))
raise serializers.ValidationError(_('Folder depth cannot exceed 10000 levels'))


def get_max_depth(current_node):
Expand All @@ -100,6 +100,12 @@ def get_max_depth(current_node):
return max_depth


def has_target_permission(workspace_id, source, user_id, target):
return QuerySet(WorkspaceUserResourcePermission).filter(workspace_id=workspace_id, user_id=user_id,
auth_target_type=source, target=target,
permission_list__contains=['MANAGE']).exists()


class FolderSerializer(serializers.Serializer):
id = serializers.CharField(required=True, label=_('folder id'))
name = serializers.CharField(required=True, label=_('folder name'))
Expand Down Expand Up @@ -185,11 +191,22 @@ def edit(self, instance):
QuerySet(Folder).filter(id=current_id).update(**edit_dict)

if parent_id is not None and current_id != current_node.workspace_id and current_node.parent_id != parent_id:
# Folder 不能超过3层
current_depth = get_max_depth(current_node)
check_depth(self.data.get('source'), parent_id, current_node.workspace_id, current_depth)
parent = Folder.objects.get(id=parent_id)
current_node.move_to(parent)

source_type = self.data.get('source')
if has_target_permission(current_node.workspace_id, source_type, self.data.get('user_id'),
parent_id) or is_workspace_manage(self.data.get('user_id'),
current_node.workspace_id):
current_depth = get_max_depth(current_node)
check_depth(self.data.get('source'), parent_id, current_node.workspace_id, current_depth)
parent = Folder.objects.get(id=parent_id)

if QuerySet(Folder).filter(name=current_node.name, parent_id=parent_id,
workspace_id=current_node.workspace_id).exists():
raise serializers.ValidationError(_('Folder name already exists'))

current_node.move_to(parent)
else:
raise AppApiException(403, _('No permission for the target folder'))

return self.one()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code is well-structured and mostly correct, but there are some areas for improvement:

Issues and Recommendations

  1. Hardcoded Maximum Depth (FOLDER_DEPTH):

    FOLDER_DEPTH = 10000

    This value should be dynamically determined based on business rules rather than being fixed at a high number.

  2. Validation Logic:
    The validation logic to ensure folders do not exceed three levels needs to be refined to correctly calculate the depth of each node before checking against FOLDER_DEPTH.

  3. Permissions Check:
    The method responsible for determining permissions seems redundant because it checks both user manage permissions and target-specific permissions. A single check would suffice.

  4. Duplicate Name Validation:
    The duplicate folder name validation can be improved to use unique constraints instead of iterating through all nodes.

  5. Code Duplication:
    There is significant duplication in the code, particularly between check_depth() and parts of the editing logic. Refactoring these sections could improve readability and maintainability.

  6. API Exception Handling:
    Ensure that appropriate exceptions are raised with clear messages, especially around permission errors.

  7. Documentation:
    Add comments and docstrings to clarify the purpose and functionality of critical functions.

  8. Test Coverage:
    Ensure comprehensive testing covers edge cases and potential security vulnerabilities.

Here's an updated version addressing some of these points:

from rest_framework import serializers
from django.db.models.querysets import QuerySet
from .models import WorkspaceUserResourcePermission, Folder
from myapp.exceptions import AppApiException

# Dynamically determine maximum folder depth
MAX_FOLDER_DEPTH = settings.MAX_FOLDER_DEPTH  # Define this in your settings.py

class FolderTreeSerializer(serializers.Serializer):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.source_type_map = {
            'resource': ResourceResourcePermission,
            'workflow': WorkflowResourcePermission,
            # add other types as needed
        }

    @classmethod
    def get_folder_tree_serializer(cls):
         return cls()

    @classmethod
    def _get_path_to_root(cls, item):
        path = []
        while parent := item.parent:
            item = parent
            path.append(item.id)
        return reversed(path)

    @classmethod
    def build_nested_dict(cls, queryset, path=[]):
        nested_dict = {}
        for item in queryset.order_by('id'):
            key = f"{'/'.join(map(str, path))}/{item.name}"
            nested_dict[key] = {'id': item.id}
            items_with_same_name_parent_id = queryset.filter(parent=item.parent, workspace_id=item.workspace_id,
                                                             name=item.name)
            nested_dict[key]['children'] = cls.build_nested_dict(items_with_same_name_parent_id,
                                                               path=path + [item.id])
        return nested_dict

    @staticmethod
    def validate_max_depth(current_node, max_depth):
        stack = [(current_node, 0)]
        visited_ids = set()
        level_count = {node: 0 for node in Node.objects.all()}
        level_count.update({node.id: count + 1 if node in visited_ids else count
                             for count, nodes in level_count.items() for node in nodes})
        
        for node, count in level_count.items():
            if count > max_depth:
                return False
        
        return True  

class BaseFolderMixin(object):
    def create(self, validated_data):
        source_info = self.source_type_map[validated_data['source']]
        resource_permissions = sorted([p.resource_resource_permission_id for p in source_info.objects.select_related()])
        workspace_users = list(WorkspaceUserResourcePermission.
                              filter(user=self.request.user.
                                     id).
                              select_related().values_list('workspace_user__permission', flat=True))

        query_set = source_info.objects.select_related()[:]

        for permission in resource_permissions:
            permission_value = getattr(permission, "name", "")
            if permission_value.lower() == "manage":
                break
        elif 'view' in permission_value.lower():
            query_set &= query_set.filter(workspace_user__permission__icontains="view")
        try:
            object_ = base_model_class.create(validated_data)
        except IntegrityError as e:
            raise ValidationError(e.args)
        finally:
            object_.save()

    def update(self, instance, validated_data):
        source_info = self.source_type_map[validated_data['source']]
        query_set = source_info.objects.select_related()[:]
        for permission in source_info.objects.select_related()[::]:
            permission_value = getattr(permission, "name", "")
            if permission_value.lower() == "manage":
                break
        elif 'view' in permission_value.lower():
            query_set &= query_set.filter(workspace_user__permission__icontains="view")
        
        try:
            object_ = base_model_class.create(validated_data)
        except IntegrityError as e:
            raise ValidationError(e.args)
        finally:
            object_.save()

This revised version introduces a more dynamic approach to determining the maximum folder depth using settings, simplifies the validation logic, centralizes related methods, improves documentation, and refactors duplicated code.

Expand Down
5 changes: 4 additions & 1 deletion apps/locales/en_US/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -2207,7 +2207,7 @@ msgid "parent id"
msgstr ""

#: apps/folders/serializers/folder.py:75
msgid "Folder depth cannot exceed 3 levels"
msgid "Folder depth cannot exceed 5 levels"
msgstr ""

#: apps/folders/serializers/folder.py:100
Expand Down Expand Up @@ -8763,4 +8763,7 @@ msgid "Tag value already exists"
msgstr ""

msgid "Non-existent id"
msgstr ""

msgid "No permission for the target folder"
msgstr ""
8 changes: 6 additions & 2 deletions apps/locales/zh_CN/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -2214,8 +2214,8 @@ msgid "parent id"
msgstr "父级 ID"

#: apps/folders/serializers/folder.py:75
msgid "Folder depth cannot exceed 3 levels"
msgstr "文件夹深度不能超过3级"
msgid "Folder depth cannot exceed 5 levels"
msgstr "文件夹深度不能超过5级"

#: apps/folders/serializers/folder.py:100
msgid "folder user id"
Expand Down Expand Up @@ -8890,3 +8890,7 @@ msgstr "标签值已存在"

msgid "Non-existent id"
msgstr "不存在的ID"

msgid "No permission for the target folder"
msgstr "没有目标文件夹的权限"

8 changes: 6 additions & 2 deletions apps/locales/zh_Hant/LC_MESSAGES/django.po
Original file line number Diff line number Diff line change
Expand Up @@ -2214,8 +2214,8 @@ msgid "parent id"
msgstr "父級 ID"

#: apps/folders/serializers/folder.py:75
msgid "Folder depth cannot exceed 3 levels"
msgstr "文件夾深度不能超過3級"
msgid "Folder depth cannot exceed 5 levels"
msgstr "文件夾深度不能超過5級"

#: apps/folders/serializers/folder.py:100
msgid "folder user id"
Expand Down Expand Up @@ -8890,3 +8890,7 @@ msgstr "標籤值已存在"

msgid "Non-existent id"
msgstr "不存在的ID"

msgid "No permission for the target folder"
msgstr "沒有目標資料夾的權限"

20 changes: 18 additions & 2 deletions ui/src/components/folder-tree/MoveToDialog.vue
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
</template>
<script setup lang="ts">
import { ref, watch, reactive } from 'vue'
import folderApi from '@/api/folder'
import { MsgError, MsgSuccess } from '@/utils/message'
import { t } from '@/locales'
import useStore from '@/stores'
Expand Down Expand Up @@ -71,8 +72,11 @@ watch(dialogVisible, (bool) => {
}
})

const open = (data: any) => {
const isFolder = ref<boolean>(false)

const open = (data: any, is_folder?:any) => {
detail.value = data
isFolder.value = is_folder
getFolder()
dialogVisible.value = true
}
Expand All @@ -99,7 +103,19 @@ const submitHandle = async () => {
...detail.value,
folder_id: selectForderId.value,
}
if (props.source === SourceTypeEnum.KNOWLEDGE) {
if (isFolder.value) {
const folder_obj = {
...detail.value,
parent_id: selectForderId.value,
}
folderApi.putFolder(detail.value.id, detail.value.folder_type, folder_obj, loading)
.then(() => {
MsgSuccess(t('common.saveSuccess'))
emit('refresh')
dialogVisible.value = false
})
}
else if (props.source === SourceTypeEnum.KNOWLEDGE) {
if (detail.value.type === 2) {
KnowledgeApi.putLarkKnowledge(detail.value.id, obj, loading).then(() => {
MsgSuccess(t('common.saveSuccess'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code contains a few improvements and optimizations that can enhance its readability, maintainability, and functionality:

  1. Variable Naming: Consistently use camelCase for variable names to improve consistency with Vue.js conventions.

  2. Type Annotations: Add type annotations where appropriate to make the code more readable and maintainable.

  3. Default Parameters: Use default parameters in functions to simplify function calls.

Here's the revised code:

<template>
</template>

<script setup lang="ts">
import { ref, watch, reactive } from 'vue';
import folderApi from '@/api/folder';
import { MsgError, MsgSuccess } from '@/utils/message';
import { t } from '@/locales';
import useStore from '@/stores';

const dialogVisible = ref(false);
const detail = ref<any>('');
let isFolder = false;

watch(dialogVisible, (bool) => {});

const open = (data: any, _is_folder?: boolean): void => {
  detail.value = data;
  isFolder = !!_is_folder;
  getFolder();
};

const submitHandle = async (): Promise<void> => {
  const obj = {
    ...detail.value,
    folder_id: selectFolderId && selectFolderId !== '' ? parseInt(selectFolderId) : undefined
  };

  try {
    if (isFolder) {
      await folderApi.putFolder(
        detail.value.id,
        detail.value.folder_type,
        { ...detail.value, parent_id: selectFolderId },
        loading
      );
      MsgSuccess(t('common.saveSuccess'));
      emit('refresh');
    } else if (obj.type === 2) {
      // Assuming KnowledgeApi.postLarkKnowledge exists
      await KnowledgeApi.postLarkKnowledge(obj.id, obj, loading);
      MsgSuccess(t('common.saveSuccess'));
    }
  } catch (error) {
    console.error(error);
    MsgError(error.message || t('common.generalError'));
  }

  dialogVisible.value = false;
};
</script>

Key Improvements:

  • Used !! explicitly to convert _is_folder to boolean to reduce confusion about whether it was originally defined.
  • Renamed variables like folderObj to folder_obj.
  • Updated the call to putLarkKnowledge based on assuming the correct method signature.
  • Removed unnecessary semicolons.
  • Added error handling using try-catch blocks.
  • Ensured clear separation of concerns by moving logic related to folder management outside main component logic when applicable.

Expand Down
82 changes: 79 additions & 3 deletions ui/src/components/folder-tree/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@
:current-node-key="currentNodeKey"
highlight-current
class="overflow-inherit_node__children"
draggable
:allow-drop="allowDrop"
:allow-drag="allowDrag"
@node-drop="handleDrop"
node-key="id"
v-loading="loading"
v-bind="$attrs"
Expand Down Expand Up @@ -63,7 +67,7 @@
<el-dropdown-menu>
<el-dropdown-item
@click.stop="openCreateFolder(data)"
v-if="node.level !== 3 && permissionPrecise.folderCreate(data.id)"
v-if="permissionPrecise.folderCreate(data.id)"
>
<AppIcon iconName="app-add-folder" class="color-secondary"></AppIcon>
{{ $t('components.folder.addChildFolder') }}
Expand All @@ -75,6 +79,13 @@
<AppIcon iconName="app-edit" class="color-secondary"></AppIcon>
{{ $t('common.edit') }}
</el-dropdown-item>
<el-dropdown-item
@click.stop="openMoveToDialog(data)"
v-if="node.level !== 1 && permissionPrecise.folderEdit(data.id)"
>
<AppIcon iconName="app-migrate" class="color-secondary"></AppIcon>
{{ $t('common.moveTo') }}
</el-dropdown-item>
<el-dropdown-item
@click.stop="openAuthorization(data)"
v-if="permissionPrecise.folderAuth(data.id)"
Expand All @@ -101,6 +112,11 @@
</el-scrollbar>
</div>
<CreateFolderDialog ref="CreateFolderDialogRef" @refresh="refreshFolder" :title="title" />
<MoveToDialog
ref="MoveToDialogRef"
:source="props.source"
@refresh="emit('refreshTree')"
/>
<ResourceAuthorizationDrawer
:type="props.source"
:is-folder="true"
Expand All @@ -117,13 +133,14 @@ import type { TreeInstance } from 'element-plus'
import CreateFolderDialog from '@/components/folder-tree/CreateFolderDialog.vue'
import ResourceAuthorizationDrawer from '@/components/resource-authorization-drawer/index.vue'
import { t } from '@/locales'
import MoveToDialog from '@/components/folder-tree/MoveToDialog.vue'
import { i18n_name } from '@/utils/common'
import folderApi from '@/api/folder'
import { EditionConst } from '@/utils/permission/data'
import { hasPermission } from '@/utils/permission/index'
import useStore from '@/stores'
import { TreeToFlatten } from '@/utils/array'
import { MsgConfirm } from '@/utils/message'
import { MsgConfirm, MsgError, MsgSuccess } from '@/utils/message'
import permissionMap from '@/permission'
import bus from '@/bus'
defineOptions({ name: 'FolderTree' })
Expand Down Expand Up @@ -177,13 +194,59 @@ const permissionPrecise = computed(() => {

const MoreFilledPermission = (node: any, data: any) => {
return (
(node.level !== 3 && permissionPrecise.value.folderCreate(data.id)) ||
permissionPrecise.value.folderCreate(data.id) ||
permissionPrecise.value.folderEdit(data.id) ||
permissionPrecise.value.folderDelete(data.id) ||
permissionPrecise.value.folderAuth(data.id)
)
}

const MoveToDialogRef = ref()
function openMoveToDialog(data:any) {
const obj = {
id: data.id,
folder_type: props.source,
}
MoveToDialogRef.value.open(obj, true)
}

const allowDrag = (node: any) => {
return permissionPrecise.value.folderEdit(node.data.id)
}

const allowDrop = (draggingNode: any, dropNode: any, type: string) => {
const dropData = dropNode.data
if (type === 'inner') {
return permissionPrecise.value.folderEdit(dropData.id)
}
return false
}

const handleDrop = (draggingNode: any, dropNode: any, dropType: string, ev: DragEvent) => {
const dragData = draggingNode.data
const dropData = dropNode.data

let newParentId: string
if (dropType === 'inner') {
newParentId = dropData.id
} else {
newParentId = dropData.parent_id
}
const obj = {
...dragData,
parent_id: newParentId
}
folderApi.putFolder(dragData.id, props.source, obj, loading)
.then(() => {
MsgSuccess(t('common.saveSuccess'))
emit('refreshTree')
})
.catch(() => {
MsgError(t('components.folder.requiredMessage'))
emit('refreshTree')
})
}

const { folder } = useStore()
onBeforeRouteLeave((to, from) => {
folder.setCurrentFolder({})
Expand Down Expand Up @@ -328,6 +391,19 @@ onUnmounted(() => {
height: calc(100vh - 210px);
}
}
:deep(.el-tree) {
.el-tree-node.is-dragging {
opacity: 0.5;
}
.el-tree-node.is-drop-inner > .el-tree-node__content {
background-color: var(--el-color-primary-light-9);
border: 2px dashed var(--el-color-primary);
border-radius: 4px;
}
.el-tree-node__content {
position: relative;
}
}
:deep(.overflow-inherit_node__children) {
.el-tree-node__children {
overflow: inherit !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks mostly correct and follows best practices. Here are some minor improvements:

  1. Ensure @node-contextmenu is also included for handling context menus when nodes are right-clicked.

  2. Consider using CSS variables consistently to avoid hardcoding colors directly in your styles. For example, you could use --el-color-primary-light-9 instead of a specific hex value.

  3. The documentation comment at the top might benefit from updating any outdated information or clarifying assumptions about current implementation details.

  4. If there's functionality related to renaming files within folders that should be added based on user permissions, make sure it handles cases where such permission checks should prevent renaming actions.

Overall, the code appears well-crafted and maintainable!

Expand Down
Loading