Skip to content

feat: Folder move order by name #4313

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.

Sign up for GitHub

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

Merged
zhanweizhang7 merged 1 commit into from
Nov 4, 2025
Merged

feat: Folder move order by name #4313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion apps/folders/serializers/folder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ def edit(self, instance):
field in instance and instance.get(field) is not None)}

QuerySet(Folder).filter(id=current_id).update(**edit_dict)
current_node.refresh_from_db()

if parent_id is not None and current_id != current_node.workspace_id and current_node.parent_id != parent_id:

Expand All @@ -204,7 +205,9 @@ def edit(self, instance):
workspace_id=current_node.workspace_id).exists():
raise serializers.ValidationError(_('Folder name already exists'))

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

Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Nov 4, 2025

Choose a reason for hiding this comment

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

Here are some suggestions to optimize your code and address potential issues:

Optimizations

  1. Batch Updates: Instead of updating each field individually, batch update the Folder object with all changes at once using bulk_update. This can improve performance by reducing SQL queries.

  2. Avoid Duplicate Updates: Check if an update is necessary before making it. For example, only refresh from DB after the move operation.

  3. Error Handling Consistency: Ensure consistent error messages across different parts of the function.

  4. Resource Management: If moving folders, consider opening resources (like database connections) for a shorter duration.

Code Changes

def edit(self, instance):
    if current_node.parent_id == parent.id:
        return
    
    edit_fields = {
        field: value for field, value in self.cleaned_data.items() 
        if hasattr(instance, field) and instance.get(field) is not None}
    
    # Batch update the Folder object
    Folder.objects.bulk_update([current_node], fields=edit_fields)

    # Refresh from DB after bulk update
    current_node.refresh_from_db()

    if parent_id is not None and current_id != current_node.workspace_id and current_node.parent_id != parent_id:
        
        if (
            Folder.objects.filter(
                workspace_id=current_node.workspace_id,
                path__startswith=f"{parent.full_path}/",
                id__ne=current_id
            ).exists()
        ):
            raise serializers.ValidationError(_('Folder name already exists'))

        # Move parent node without refreshing
        try:
            parent.load_content(current_id)
        except PermissionDenied:
            raise AppApiException(403, _('No permission for the target folder'))

Further Suggestion

  • Validation Order: Ensure the validation order does not prevent successful moves. Consider checking permissions before trying to access certain resources like content.

This revision reduces the number of individual updates made to the Folder object and ensures that the resource is opened for as short a time as possible during critical operations.

Expand Down
1 change: 0 additions & 1 deletion ui/src/components/folder-tree/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,6 @@ const handleDrop = (draggingNode: any, dropNode: any, dropType: string, ev: Drag
emit('refreshTree')
})
.catch(() => {
MsgError(t('components.folder.requiredMessage'))
emit('refreshTree')
})
}
Copy link
Copy Markdown
Contributor Author

@shaohuzhang1 shaohuzhang1 Nov 4, 2025

Choose a reason for hiding this comment

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

There are no serious issues with the provided code. However, there is a minor suggestion to improve readability:

const handleDrop = (draggingNode: any, dropNode: any, dropType: string, ev: DragEvent) => {
  try {
    emit('refreshTree');
  } catch (error) {
    MsgError(t('components.folder.requiredMessage'));
    emit('refreshTree');
  }
};

In this version of the function:

  1. The if statement has been replaced with a try-catch block for error handling, which enhances the structure and clarity of the code.
  2. The variable name for ev (DraggingData) has been changed to DragEvent, which aligns with common practice when dealing with drag and drop events in JavaScript.

This change makes it easier to understand that Ev stands for "Drag Event" and can help other developers reading the code quickly grasp its purpose.

Expand Down
Loading