-
Notifications
You must be signed in to change notification settings - Fork 2.1k
refactor: Optimization of operation menu #2717
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
--bug=1054069 --user=王孝刚 【操作日志】-操作菜单优化建议 https://www.tapd.cn/57709429/s/1677900
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. Instructions 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/test-infra 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
: '') | ||
}} | ||
</template> | ||
</el-table-column> | ||
<el-table-column | ||
width="120" | ||
prop="user.username" |
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 modified line seems to correct a minor issue in how the operation details are displayed. The template expression now includes the operation object's name if it exists, separated by "【」" for clarity.
Optimization Suggestions:
- Improve Readability: Consider wrapping long expressions across multiple lines for better readability.
- Type Checking: Ensure that
row.operation_object?.name
is not undefined before accessing it to avoid runtime errors. - Avoid Redundancy: If
row.operate
androw.operation_object?.name
always come together, you can simplify the logic. - Internationalization (i18n): Ensure consistent use of
$t
for internationalized labels.
Here’s an improved version:
<template>
<!-- ... -->
<el-table-column
prop="operate"
:label="$t('views.operateLog.table.operate.detail')"
>
<template #default="{ row }">
{{ row.operate }}
{{
row.operation_object && row.operation_object.name
? ` 【${row.operation_object.name}】`
: ''
}}
</template>
</el-table-column>
<!-- ... -->
</template>
<script setup lang="ts">
// No additional changes required unless further modifications are needed beyond this point
</script>
This should handle the operation object display correctly while making the code more readable and maintaining good practices.
@@ -668,8 +666,6 @@ class TextToSpeech(APIView): | |||
dynamic_tag=keywords.get( | |||
'application_id'))], | |||
compare=CompareConstants.AND)) | |||
@log(menu='Application', operate="text to speech", | |||
get_operation_object=lambda r, k: get_application_operation_object(k.get('application_id'))) | |||
def post(self, request: Request, application_id: str): | |||
byte_data = ApplicationSerializer.Operate( | |||
data={'application_id': application_id, 'user_id': request.user.id}).text_to_speech( |
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.
Here are some concerns and suggestions for review:
-
Redundancy: The same
@log
decorator is applied multiple times with similar parameters on both classes (SpeechToText
andTextToSpeech
). This redundancy can be removed, which makes the code cleaner. -
Functionality Duplication: Both functions in the post methods seem to have very similar logic. Extracting this common functionality into a separate method would make things more maintainable and reduce duplication.
-
Code Style: Ensure consistent spacing, alignment, and use of comments where necessary. Proper organization of the class hierarchy and function definitions will also improve readability.
-
Security: If there's sensitive information about the application ID being logged, consider implementing additional security measures such as anonymization or whitelisting specific IDs.
Here’s an optimized version of the code based on these suggestions:
from django.utils.decorators import method_decorator
from logging_decorators import log
class SpeechToText(APIView):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.dynamic_tag_key = "application_id"
@method_decorator(log(menu='Application', operate="speech to text"))
def post(self, request: Request, application_id: str):
return apply_operation(request.user.id)
def text_to_speech(application_id, user_id):
# Placeholder for actual implementation
pass
def apply_operation(user_id):
keywords = {}
dynamic_tag = keywords.pop(self.dynamic_tag_key) if self.dynamic_tag_key in keywords else None
operation_object = get_application_operation_object(dynamic_tag=dynamic_tag)
return result.success(ApplicationSerializer.Operate(data={'application_id': application_id, 'user_id': user_id}))
# Replace get_application_operation_object with appropriate logic
def get_application_operation_object(dynamic_tag=None):
# Placeholder for actual implementation
pass
Key Changes:
- Removed redundant
@log
decorators. - Introduced a helper method
apply_operation
that encapsulates duplicate logic. - Created two separate functions:
text_to_speech
for handling text-to-speech operations, andget_application_operation_object
. - Used
@method_decorator
to apply the logging middleware globally.
These changes should lead to cleaner, more organized, and potentially less error-prone code.
@@ -60,6 +60,8 @@ class Export(APIView): | |||
[lambda r, keywords: Permission(group=Group.APPLICATION, operate=Operate.USE, | |||
dynamic_tag=keywords.get('application_id'))]) | |||
) | |||
@log(menu='Conversation Log', operate="Export conversation", | |||
get_operation_object=lambda r, k: get_application_operation_object(k.get('application_id'))) | |||
def post(self, request: Request, application_id: str): | |||
return ChatSerializers.Query( | |||
data={**query_params_to_single_dict(request.query_params), 'application_id': application_id, |
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 looks mostly well-structured, but there's one change needed to fix a syntax error:
@@ -62,7 +64 @@ class Export(APIView):
To correct the syntax error and ensure proper function definitions within your APIView
, update line 62 from -62,7
to +62
. This should resolve the issue with the indentation. Here is the corrected line:
@post(...)
def post(self, request, application_id): # Note the removal of commas here due to incorrect indenting
... rest of the code ...
Additionally, consider checking if get_log_menu_name
is defined properly as it appears in "Export conversation"
within the annotation.
If you need further optimization suggestions or additional improvements, please let me know! Otherwise, this should address any immediate issues found in your code.
refactor: Optimization of operation menu --bug=1054069 --user=王孝刚 【操作日志】-操作菜单优化建议 https://www.tapd.cn/57709429/s/1677900