-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Knowledge Base Workflow Execution Record #4435
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 |
| } | ||
| defineExpose({ open, close }) | ||
| </script> | ||
| <style lang="scss" scoped></style> |
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 looks mostly correct, but there are a few suggestions and improvements:
-
Use of
watchEffect: Instead of usingcomputed, consider usingwatchEffectto simplify managing dependencies whenpaginationConfig.current_pagechanges.import { watchEffect } from 'vue'; // ... const paginationConfig = reactive({ current_page: 1, page_size: 30, total: 0, }); watchEffect(async () => { await loadSharedApi({ type: 'knowledge', systemType: apiType.value }) .pageWorkflowAction(active_knowledge_id.value, paginationConfig, query); });
This way, you can ensure that
datais updated whenever any part ofpaginationConfigchanges. -
Error Handling: Add error handling to handle cases where the API call fails:
try { const response = await loadSharedApi({ type: 'knowledge', systemType: apiType.value }) .pageWorkflowAction(active_knowledge_id.value, paginationConfig, query); paginationConfig.total = response.data?.total; data.value = response.data.records; } catch (error) { console.error('Failed to fetch workflow actions:', error); }
-
Optional Parameters for API Call: Consider adding optional parameters to the API call function so that only necessary data is passed.
async function fetchWorkflowActions( active_knowledge_id? = '', pagination_config? = paginationConfig, query? = query ) { return await loadSharedApi({ type: 'knowledge', systemType: apiType.value }) .pageWorkflowAction(active_knowledge_id, pagination_config, query); } // Use the function instead of passing all options explicitly watchEffect(async () => { const result = await fetchWorkflowActions(); paginationConfig.total = result.data?.total || 0; data.value = result.data?.records || []; });
-
Event Emitter for Parent Component: If this component needs to emit events to its parent, consider creating an event emitter using Vue's
$emit.<template> <div> <!-- Drawer content --> {{ /* Existing template */ }} <!-- Optional event slot for custom handlers --> <slot name="event-emitter"></slot> </div> </template> <script setup lang="ts"> import { onMounted, inject } from 'vue'; // Assuming $parent.$emit exists const parentEmitter = inject('$parent'); onMounted(() => { // Emit event to parent if (parentEmitter) { parentEmitter('workflow-details-updated'); } }); </script>
-
Scoping Issues with
$router: Ensure that$routeris properly scoped within the component. -
Comments Enhancements: Consider adding more detailed comments explaining logic flow or important sections.
By applying these improvements, you will make the code more robust, efficient, and easier to maintain.
| 'meta': knowledge_action.meta} | ||
|
|
||
|
|
||
| class KnowledgeWorkflowSerializer(serializers.Serializer): |
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 looks generally correct and well-structured. However, there are a few suggestions for improvement:
-
Import Statement: Ensure that all necessary imports are present without any duplicates. In this case,
common.db.searchis already imported at the beginning. -
Variable Naming: Use meaningful variable names to improve readability. For example,
query_setinstead ofa. -
Documentation Comments: Add comments to explain important parts of the code, especially logic blocks or function purposes.
-
Type Hinting: Consider using type hints to improve code clarity and maintainability. Although not strictly required for Python 3.6+, it's recommended for better IDE support and static analysis tools.
Here’s an improved version of the code with these considerations:
from __future__ import annotations
import uuid
from django.db.models import QuerySet
from rest_framework import serializers
from typing import List
from application.flow.knowledge_workflow_manage import KnowledgeWorkflowManage
from application.flow.step_node import get_node
from application.serializers.application import get_mcp_tools
from common.db.search import page_search
from common.exception.app_exception import AppApiException
from common.utils.rsa_util import rsa_long_decrypt
from common.utils.tool_code import ToolExecutor
from .models import KnowledgeAction
from .enums import State
class KnowledgeWorkflowActionRequestSerializer(serializers.Serializer):
knowledge_base = serializers.DictField(required=True, label=_("knowledge base data"))
class KnowledgeWorkflowActionListQuerySerializer(serializers.Serializer):
user_name = serializers.CharField(required=False, label=_("Name"), allow_blank=True, allow_null=True)
class KnowledgeWorkflowActionSerializer(serializers.Serializer):
workspace_id = serializers.CharField(required=True, label=_("workspace id"))
knowledge_id = serializers.UUIDField(required=True, label=_("knowledge id"))
def validate_knowledge_id(self, value):
# Validate the correctness of the knowledge ID
if not QuerySet(KnowledgeAction).exists(id=value):
raise serializers.ValidationError(_("Invalid knowledge id"))
return value
@staticmethod
def _get_user_filter(user=None, **kwargs) -> dict:
filter_kwargs = {}
if user:
filter_kwargs['meta'] = {"$elemMatch": {"user_id": str(user.id), "user_name": kwargs.get('user_name')}}
return filter_kwargs
def get_query_set(self, instance: Dict[str, Any]):
query_set = (
QuerySet(KnowledgeAction)
.filter(knowledge_id=self.validate_knowledge_id(self.data["knowledge_id"]))
.order_by("-create_time")
.values(*{"id", "knowledge_id", "state", "details", "created_at"})
)
if isinstance(instance, dict) and (user_name := instance.setdefault("user_name", None)) is not None:
if user:
return query_set.filter(**self._get_user_filter(user, user_name=user_name))
else:
return query_set.annotate(user_exists=Exists(User.objects.filter(pk=F("meta.user_id")))).filter(user_exists=True)
return query_set
def list(self, instance: dict, is_valid=True) -> List[dict]:
if is_valid:
self.is_valid(raise_exception=True)
serializer_data = KnowledgeWorkflowActionListQuerySerializer(data=instance).data
user_name = serializer_data.get("user_name") if user_name :=(serializer_data.pop("user_name", None)) else None
query_set = self.get_query_set(is_valid).Key Points:
- Added validation to ensure the validity of the
knowledge_id. - Refactored the
_get_user_filtermethod to handle optional filtering based onuseranduser_name. - Used dictionary comprehensions for cleaner syntax.
- Removed unnecessary parameters from the constructor methods (
with_valid=True).
These changes enhance both the functionality and readability of the code while maintaining its integrity.
| data={'workspace_id': workspace_id, 'knowledge_id': knowledge_id}).upload_document(request.data, | ||
| request.user, 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.
- In
KnowledgeWorkflowUploadDocumentView, changeTruetorequest.userwhen calling.upload_document(). - Remove the redundant parameter
selffrom the decorators (@extend_schema()and@has_permissions()). - In
KnowledgeWorkflowActionView(post method), removeTrue. It is not needed since it's already default. - In
KnowledgeWorkflowActionView\Page:- Add the necessary imports for
PageAPI. - Change
data={...}'todata=request.datato pass the user correctly.
- Add the necessary imports for
Suggested changes:
class KnowledgeWorkflowUploadDocumentView(APIView):
...
def post(self, request: Request, workspace_id: str, knowledge_id: str) -> Response:
return result.success(KnowledgeWorkflowActionSerializer(
data={'workspace_id': workspace_id, 'knowledge_id': knowledge_id}).upload_document(request.data,
request.user))
class KnowledgeWorkflowActionView(APIView):
...
def post(self, request: Request, workspace_id: str, knowledge_id: str) -> Response:
return result.success(KnowledgeWorkflowActionSerializer(data=request.data).action(request.data, request.user))The remaining changes need to be adapted based on the actual implementation of get_parameters(), get_request(), and so forth in your view.
feat: Knowledge Base Workflow Execution Record