Skip to content
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

fix: New workflow application, node name not internationalized #2066

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

shaohuzhang1
Copy link
Contributor

fix: New workflow application, node name not internationalized

Copy link

f2c-ci-robot bot commented Jan 22, 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 Jan 22, 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

elif len(node_list) > 1:
sorted_node_run_list = sorted(node_list, key=lambda n: n.node.y)
# 获取到可执行的子节点
result_list = [{'node': node, 'future': executor.submit(self.run_chain_manage, node, None)} for node in
result_list = [{'node': node, 'future': executor.submit(self.run_chain_manage, node, None, language)} for
node in
sorted_node_run_list]
for r in result_list:
self.future_list.append(r.get('future'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No significant irregularities found in the provided code.

However, there are a few areas where optimizations or improvements could be made:

  1. String Template Usage: The use of str.format() within exception messages and log outputs can be inefficient when dealing with multiple replacements. Using gettext (or similar methods) would make it easier to manage translations and improve readability.

  2. Locale Activation: Inefficiently activating locale settings within each recursive call of run_chain_manage. Consider using a different approach to manage locales more efficiently or avoid changing them for every node execution unless necessary.

  3. Logging: Although the example doesn't contain explicit logging statements, consider adding some logging at crucial points to monitor the flow of data and identify bottlenecks during runtime.

Here's an updated version incorporating these suggestions slightly cleaned up but no changes were actually made due to the limited scope of the modifications:

# Ensure all imports include full package paths

from django.db import close_old_connections
from django.db.models import QuerySet
from django.utils import translation
from django.utils.translation import get_language
from django.utils.translation import gettext as _
from langchain_core.prompts import PromptTemplate
from rest_framework import status
from rest_framework.exceptions import ErrorDetail, ValidationError

class WorkflowValidator:
    def is_valid_node(self, node: Node):
        edge_list = [edge for edge in self.edges if edge.sourceAnchorId == source_anchor_id]
        if len(edge_list) == 0:
            raise AppApiException(500,
                               _(f'The branch {branch} of the {node.properties.get("stepName")} node needs to be connected'))

class BaseWorkflowHandler:
    @staticmethod
    def activate_locale(current_node, language='zh'):
        """Activates the specified locale for processing."""
        try:
            return translation.activate(language)
        except Exception as e:
            print(f"Error activating locale for {current_node}: {e}")
            logging.error(f"Failed to activate locale for {current_node}", exc_info=True)

    def run_block(self):
        close_old_connections()
        language = "zh"
        self.activate_locale(None, language)
        # Continue running block logic here...

def handle_workflow(request):
    validator = WorkflowValidator(request.data)
    handler = BaseWorkflowHandler()

    # Validate workflow
    valid_status = [
        validator.is_valid_node(node) 
        for node in request.data['nodes']
    ]

    if not all(valid_status):
        raise ValidationError(["Some validation errors occurred."])

    # Proceed with further processing...

In this modified snippet, I've added static method activate_locale that encapsulates the logic for setting the locale. Note that actual implementation will depend on specific requirements and context of your application.

if not os.path.exists(workflow_file_path):
workflow_file_path = os.path.join(PROJECT_DIR, "apps", "application", 'flow',
f'default_workflow_zh.json')
default_workflow_json = get_file_content(workflow_file_path)
default_workflow = json.loads(default_workflow_json)
for node in default_workflow.get('nodes'):
if node.get('id') == 'base-node':
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 has a couple of improvements and corrections:

  1. Translation Context:

    • The gettext_lazy call now uses get_language() instead of hardcoding "zh". This allows for dynamic language translation based on the user's current locale.
    • The file path for reading the JSON default workflow is constructed with a locale-aware filename using to_locale.
  2. Error Handling for Locale Files:

    • Before attempting to open the default workflow file, an error handling mechanism checks if the expected locale-specific file exists. If it does not, it defaults to opening the base "zh" workflow file.
  3. File Path Adjustments:

    • The import statement for users.models.User was moved after other imports, adhering to best practices which usually place commonly used types near the top of their own sections (e.g., models above serializers).
  4. Code Clarity:

    • Added a comment explaining the purpose of checking localization files before using them in case they don't exist.
  5. Import Order:

    • Moved all necessary imports at the beginning of the script for better readability and maintainability.

With these changes, the code should be more robust and adaptable to different locales without errors due to missing files.

@shaohuzhang1 shaohuzhang1 merged commit 3c859a6 into main Jan 22, 2025
4 of 5 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@main@fix_application_i18n branch January 22, 2025 06:54
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.

1 participant