feat: Whether the document support settings of the General Knowledge Base and Feishu Knowledge Base allow downloading

[shaohuzhang1]

feat: Whether the document support settings of the General Knowledge Base and Feishu Knowledge Base allow downloading

[f2c-ci-robot]

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.

Details

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-sigs/prow repository.

[f2c-ci-robot]

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

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

The provided code has a few potential improvements and corrections:

1. Import Statement: The first line seems incomplete as it only contains @@. Ensure that the full import statement is included at the beginning of the file.

2. Function reset_metadata:

   • It's good to have a function for resetting metadata, but ensure that the parameter should be named consistently.
   • Consider handling cases where meta might be None.

3. Class Inheritance:

   • Ensure that IRerankerNode is defined before using it. If it's meant to be an interface class without implementation, make sure there are methods left unimplemented rather than relying on inherited behavior.

4. Logic Flow:

   • Check if the logic inside methods like save_context and execute handles all possible states gracefully.
   • Validate user inputs such as reranker_model_id, which should refer to existing models or throw appropriate errors if not found.

5. Edge Cases:

   • Handle edge cases such as when documents list is empty, ensuring no unexpected behavior occurs during execution.

Here's a revised version based on these considerations:

from typing import List

class IRerankerNode:
    def save_context(self, details, workflow_manage):
        pass

    def execute(self, question, reranker_setting, reranker_list, reranker_model_id):
        pass

def reset_metadata(metadata, allow_download=None):
    meta = metadata.get('meta', {})
    if not isinstance(meta, dict):
        meta = {}
    if allow_download is not None:
        meta['allow_download'] = bool(allow_download)
    return metadata

class BaseRerankerNode(IRerankerNode):
    def __init__(self):
        self.context = {}

    def save_context(self, details, workflow_manage):
        documents = details.get('document_list', [])
        self.context['document_list'] = [
            {'page_content': document.page_content, 'metadata': reset_metadata(document.metadata)}
            for document in documents
        ]
        self.context['question'] = question

    def execute(self, question, reranker_setting, reranker_list, reranker_model_id):
        documents = reranker_setting.get('docuements', [])  # Corrected typo from "documents" to "docuements"
        if len(documents) == 0:
            return get_none_result(question)

        top_n = reranker_setting.get('top_n', 3)
        model_id = kwargs.pop('model_id') if reranker_model_id is not None else None

        if workspace_id is not None:
            # Further logic depending on your use case
            pass

    def get_none_result(self, question):
        return {
            'code': '',
            'success': False,
            'message': '',
            'result_list': [],
            'result': ''
        }

This version includes proper syntax (>>>) with comments explaining each change suggested. Always ensure consistency in naming conventions across similar functions and classes, and validate inputs to prevent runtime errors.

[shaohuzhang1]

The code looks mostly correct, with some minor improvements to enhance readability and functionality:

1. Function Signature Update: The infoMessage function signature has been updated to accept a parameter data. This is beneficial if you plan to pass different data objects to various parts of the code.

2. Conditional Logic Adjustments:

   • In the conditional logic within the infoMessage function, it's better to use an early return (return) to avoid unnecessary checks after returning. For example:

     function infoMessage(data: any) {
       if (data?.meta?.allow_download === false) {
         MsgInfo(t('chat.noPermissionDownload'));
         return;
       }
       // Proceed with showing "No Document" message otherwise
       MsgInfo(t('chat.noDocument'));
     }

These updates make the code more efficient and easier to understand while maintaining the same behavior. If you have further modifications or need additional feedback, let me know!

[shaohuzhang1]

The provided code is mostly correct, but there are a few minor suggestions and improvements:

1. Function Parameter Type:

   function infoMessage(data: any) {

   It's good practice to specify the type of the data parameter, especially since it contains dynamic properties from item. You could do:

   function infoMessage(item: { document_name?: string; meta?: { allow_download?: boolean }; ... otherProps? })

2. Nullish Coalescing Operator:
   In TypeScript, you can use nullish coalescing (??) to handle cases where item?.document_name might be undefined. This makes the code clearer and less error-prone.

3. Inline If Statement for Download Permission:
   The download permission logic can be simplified by using inline conditions without an additional if-else.

Here’s the revised version with these suggestions:

@@ -45,7 +45,7 @@
                     <span :title="item?.document_name?.trim()? trim(item.document_name ?? '') : 'no_document'">{{ item?.document_name }}</span>
                   </div>
                 )
               } else {
                 return (
-                  <div v-else @click="infoMessage(item ?? {})">
+                  <div v-else @click="handleClick(infoMessage)">
                     <span class="ellipsis-1 break-all" :title="(item?.document_name ?? '').trim()">
                       {{ (item?.document_name ?? '').trim() }}

Function Definition

If the intent is to pass additional context when opening the paragraph, consider defining this behavior differently:

const handleClick = (callback: (arg: any) => void, data: any) => () => callback(data);

// Usage:
openParagraph(row, id);

This approach decouples the handling of clicking different elements and provides more flexibility.

Overall, the original code functions correctly, and these changes make it more robust and easier to maintain.