Skip to content

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

Merged
merged 1 commit into from
Mar 28, 2025
Merged

Conversation

shaohuzhang1
Copy link
Contributor

refactor: Optimization of operation menu --bug=1054069 --user=王孝刚 【操作日志】-操作菜单优化建议 https://www.tapd.cn/57709429/s/1677900

--bug=1054069 --user=王孝刚 【操作日志】-操作菜单优化建议 https://www.tapd.cn/57709429/s/1677900
Copy link

f2c-ci-robot bot commented Mar 28, 2025

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.

Copy link

f2c-ci-robot bot commented Mar 28, 2025

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

: '')
}}
</template>
</el-table-column>
<el-table-column
width="120"
prop="user.username"
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 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:

  1. Improve Readability: Consider wrapping long expressions across multiple lines for better readability.
  2. Type Checking: Ensure that row.operation_object?.name is not undefined before accessing it to avoid runtime errors.
  3. Avoid Redundancy: If row.operate and row.operation_object?.name always come together, you can simplify the logic.
  4. 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(
Copy link
Contributor Author

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:

  1. Redundancy: The same @log decorator is applied multiple times with similar parameters on both classes (SpeechToText and TextToSpeech). This redundancy can be removed, which makes the code cleaner.

  2. 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.

  3. 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.

  4. 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, and get_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,
Copy link
Contributor Author

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.

@wxg0103 wxg0103 merged commit 4ffec85 into main Mar 28, 2025
4 of 5 checks passed
@wxg0103 wxg0103 deleted the pr@main@refactor_1054069 branch March 28, 2025 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants