Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions apps/knowledge/views/knowledge_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ class KnowledgeWorkflowActionView(APIView):
tags=[_('Knowledge Base')] # type: ignore
)
@has_permissions(
PermissionConstants.KNOWLEDGE_READ.get_workspace_knowledge_permission(),
PermissionConstants.KNOWLEDGE_READ.get_workspace_permission_workspace_manage_role(),
PermissionConstants.KNOWLEDGE_DOCUMENT_CREATE.get_workspace_knowledge_permission(),
PermissionConstants.KNOWLEDGE_DOCUMENT_CREATE.get_workspace_permission_workspace_manage_role(),
RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
ViewPermission(
[RoleConstants.USER.get_workspace_role()],
Expand Down Expand Up @@ -148,8 +148,8 @@ class Publish(APIView):
responses=DefaultResultSerializer,
tags=[_('Knowledge')] # type: ignore
)
@has_permissions(PermissionConstants.KNOWLEDGE_EDIT.get_workspace_knowledge_permission(),
PermissionConstants.KNOWLEDGE_EDIT.get_workspace_permission_workspace_manage_role(),
@has_permissions(PermissionConstants.KNOWLEDGE_WORKFLOW_EDIT.get_workspace_knowledge_permission(),
PermissionConstants.KNOWLEDGE_WORKFLOW_EDIT.get_workspace_permission_workspace_manage_role(),
ViewPermission([RoleConstants.USER.get_workspace_role()],
[PermissionConstants.KNOWLEDGE.get_workspace_knowledge_permission()],
CompareConstants.AND),
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 few issues and can be optimized:

Issues:

  1. Repeated Methods:

    • The Publish method is copied and pasted within the same class with minor modifications. This redundancy can be removed to improve readability and maintainability.
  2. Permissions:

    • There are multiple instances of similar permission checks across different methods. Consolidate these into a single set of permissions wherever possible to avoid duplication.
  3. Comments vs. Docstrings:

    • Some comments use underscores instead, which are not correct for docstring formatting (e.g., _('Knowledge Base')) should be replaced with regular double quotes ("").
  4. Function Annotations:

    • Ensure that all function annotations match their types and signatures properly.

Optimizations:

  1. Consolidated Permissions:

    from openai.permissions import PermissionConstants, RoleConstants, ViewPermission
    
    class KnowledgeWorkflowActionView(APIView):
        @has_permissions(
            PermissionConstants.KNOWLEDGE_DOCUMENT_CREATE.get_workspace_knowledge_permission(),
            PermissionConstants.KNOWLEDGE_DOCUMENT_CREATE.get_workspace_permission_workspace_manage_role(),
            RoleConstants.WORKSPACE_MANAGE.get_workspace_role(),
            ViewPermission([RoleConstants.USER.get_workspace_role()],
                             [PermissionConstants.KNOWLEDGE_PERMISSION.get_workspace_knowledge_permission()],
                             CompareConstants.AND)
        )
        def get(self, request):
            ...  # Existing implementation...
    
        def post(self, request):
            ...  # Existing implementation...
    
        def put(self, request):
            ...  # Existing implementation...
    
        def patch(self, request):
            ...  # Existing implementation...
  2. Removed Redundant Method Copies:

    def publish(request):
        """Post an action to update workflow status."""
        return JsonResponse({"status": "success"})

By consolidating permissions and removing redundant method copies, you make the code cleaner, more maintainable, and less prone to errors.

Expand Down
1 change: 1 addition & 0 deletions ui/src/locales/lang/en-US/views/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default {
'The Community Edition supports up to 5 APP. If you need more APP, please upgrade to the Professional Edition.',
saveErrorMessage: 'Saving failed, please check your input or try again later',
loadingErrorMessage: 'Failed to load configuration, please check your input or try again later',
noDocPermission: 'No permission to create documents',
},

form: {
Expand Down
1 change: 1 addition & 0 deletions ui/src/locales/lang/zh-CN/views/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export default {
professionalMessage: '社区版最多支持 5 个应用,如需拥有更多应用,请升级为专业版。',
saveErrorMessage: '保存失败,请检查输入或稍后再试',
loadingErrorMessage: '加载配置失败,请检查输入或稍后再试',
noDocPermission: '无文档创建权限',
},
form: {
appName: {
Expand Down
1 change: 1 addition & 0 deletions ui/src/locales/lang/zh-Hant/views/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export default {
professionalMessage: '社群版最多支援 5 個應用,如需擁有更多應用,請升級為專業版。',
saveErrorMessage: '儲存失敗,請檢查輸入或稍後再試',
loadingErrorMessage: '載入配置失敗,請檢查輸入或稍後再試',
noDocPermission: '無文檔創建權限',
},
form: {
appName: {
Expand Down
6 changes: 6 additions & 0 deletions ui/src/permission/knowledge/system-manage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,12 @@ const systemManage = {
PermissionConst.RESOURCE_KNOWLEDGE_WORKFLOW_READ
],'OR'
),
workflow_edit: () =>
hasPermission([
RoleConst.ADMIN,
PermissionConst.RESOURCE_KNOWLEDGE_WORKFLOW_EDIT
],'OR'
),
chat_user_edit: () =>false,


Expand Down
3 changes: 3 additions & 0 deletions ui/src/permission/knowledge/system-share.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ const share = {
hasPermission([RoleConst.ADMIN, PermissionConst.SHARED_KNOWLEDGE_TAG_DELETE], 'OR'),
debug: () =>
hasPermission([RoleConst.ADMIN, PermissionConst.SHARED_KNOWLEDGE_WORKFLOW_READ], 'OR'),
workflow_edit: () =>
hasPermission([RoleConst.ADMIN, PermissionConst.SHARED_KNOWLEDGE_WORKFLOW_EDIT], 'OR'),

chat_user_edit: () => false,

auth: () => false,
Expand Down
1 change: 1 addition & 0 deletions ui/src/permission/knowledge/workspace-share.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ const workspaceShare = {
folderDelete: () => false,
hit_test: () => false,
debug: () => true,
workflow_edit: () => true,
}

export default workspaceShare
15 changes: 15 additions & 0 deletions ui/src/permission/knowledge/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,21 @@ const workspace = {
],
'OR',
),
workflow_edit: (source_id: string) =>
hasPermission(
[
new ComplexPermission(
[RoleConst.USER],
[PermissionConst.KNOWLEDGE.getKnowledgeWorkspaceResourcePermission(source_id)],
[],
'AND',
),
RoleConst.WORKSPACE_MANAGE.getWorkspaceRole,
PermissionConst.KNOWLEDGE_WORKFLOW_EDIT.getKnowledgeWorkspaceResourcePermission(source_id),
PermissionConst.KNOWLEDGE_WORKFLOW_EDIT.getWorkspacePermissionWorkspaceManageRole,
],
'OR',
),
hit_test: () => false,
}

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 code provided is mostly clean and well-structured. However, there are a few suggestions to improve clarity and maintainability:

  1. Comments: Add comments explaining the purpose of each permission chain and their relationship. This can help other developers (or yourself at a later time) understand what each function is achieving.

  2. Whitespace Consistency: Ensure consistent use of whitespace around operators and between statements for better readability.

Here's an updated version with some comments added:

const workspace = {
  // Function to check if user has permission to edit the given knowledge workspace
  workflow_edit: (source_id: string) => {
    return hasPermission(
      [
        new ComplexPermission(
          [RoleConst.USER], // Check if the role is a user
          [PermissionConst.KNOWLEDGE.getKnowledgeWorkspaceResourcePermission(source_id)], 
          [], // No additional conditions required
          'AND', // All specified permissions must be met
        ),
        RoleConst.WORKSPACE_MANAGE.getWorkspaceRole, // Check if the role allows workspace management
        PermissionConst.KNOWLEDGE_WORKFLOW_EDIT.getKnowledgeWorkspaceResourcePermission(source_id), 
        PermissionConst.KNOWLEDGE_WORKFLOW_EDIT.getWorkspacePermissionWorkspaceManageRole, 
      ], // OR condition implies any one of these conditions must be true
      'OR'
    );
  },
  
  // Placeholder for HIT test functionality
  hit_test: () => false
};

// Example usage:
console.log(workspace.workflow_edit('example_source_id')); // Use this to determine if the user can edit a specific knowledge space

Summary of Changes:

  • Added comments to explain the logic and intent behind each section.
  • Ensured whitespace consistency within functions for easier reading and maintenance.

These changes should enhance both the quality and maintainability of the codebase.

Expand Down
15 changes: 14 additions & 1 deletion ui/src/views/knowledge-workflow/component/DebugDrawer.vue
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ import applicationApi from '@/api/application/application'
import KnowledgeBase from '@/views/knowledge-workflow/component/action/KnowledgeBase.vue'
import { WorkflowType } from '@/enums/application'
import { loadSharedApi } from '@/utils/dynamics-api/shared-api.ts'
import permissionMap from '@/permission'
import { MsgError } from '@/utils/message'
import { t } from '@/locales'

import { useRoute, useRouter } from 'vue-router'
provide('upload', (file: any, loading?: Ref<boolean>) => {
return applicationApi.postUploadFile(file, id, 'KNOWLEDGE', loading)
Expand Down Expand Up @@ -128,8 +132,14 @@ const up = () => {
active.value = 'data_source'
})
}

const permissionPrecise = computed(() => {
return permissionMap['knowledge'][apiType.value]
})

const upload = () => {
ActionRef.value.validate().then(() => {
if (permissionPrecise.value.doc_create(id)) {
ActionRef.value.validate().then(() => {
form_data.value[active.value] = ActionRef.value.get_data()
loadSharedApi({ type: 'knowledge', systemType: apiType.value })
.workflowAction(id, form_data.value, loading)
Expand All @@ -138,6 +148,9 @@ const upload = () => {
active.value = 'result'
})
})
} else {
MsgError(t('views.application.tip.noDocPermission'))
}
}
const continueImporting = () => {
action_id.value = undefined
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 points to consider and potential improvements:

  1. Imports: The MsgError and t methods are imported but not used within the function scope.

  2. Empty Function Block: If the else block is reached due to missing permissions, an unnecessary empty block can be simplified or removed if no further logic needs to be executed at that point.

  3. Code Consistency: Ensure consistency in variable naming, spacing, and indentation for readability and adherence to coding standards.

  4. Avoid Global Scope Variables: Minimize direct global access to variables like permissionMap. Consider encapsulating these values in a class or context where they are needed.

  5. Nullish Coalescing Operator: Use the nullish coalescing operator (??) to safely handle cases where id, form_data.value[active.value], etc., might be undefined.

  6. Dynamic API Calls: Consider using dependency injection mechanisms to manage dynamic API calls more cleanly, especially with loadSharedApi.

Here's a revised version of the function incorporating these suggestions:

const upload = () => {
  const permissionPrecise = computed(() => {
    return permissionMap['knowledge'][apiType.value];
  });

  // Check if document creation privilege exists before validating form
  if (permissionPrecise && permissionPrecise.doc_create(id)) {
    ActionRef.value.validate().then(() => {
      form_data.value[active.value] = ActionRef.value.get_data();
      
      loadSharedApi({ type: 'knowledge', systemType: apiType.value })
        .workflowAction(id, form_data.value, loading)
        .finally(() => {
          active.value = 'result';
        });
    });
  } else {
    MsgError(t('views.application.tip.noDocPermission'));
  }
};

By making these adjustments, you improve code safety, maintainability, and readability.

Expand Down
7 changes: 4 additions & 3 deletions ui/src/views/knowledge-workflow/index.vue
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@
<AppIcon iconName="app-debug-outlined" class="mr-4"></AppIcon>
{{ $t('common.debug') }}
</el-button>
<el-button @click="saveknowledge(true)">
<el-button v-if="permissionPrecise.workflow_edit(id)"
@click="saveknowledge(true)">
<AppIcon iconName="app-save-outlined" class="mr-4"></AppIcon>
{{ $t('common.save') }}
</el-button>
<el-button type="primary" @click="publish">
<el-button type="primary" v-if="permissionPrecise.workflow_edit(id)" @click="publish">
{{ $t('common.publish') }}
</el-button>

Expand All @@ -57,7 +58,7 @@
<AppIcon iconName="app-history-outlined" class="color-secondary"></AppIcon>
{{ $t('views.workflow.setting.releaseHistory') }}
</el-dropdown-item>
<el-dropdown-item>
<el-dropdown-item v-if="permissionPrecise.workflow_edit(id)">
<AppIcon iconName="app-save-outlined" class="color-secondary"></AppIcon>
{{ $t('views.workflow.setting.autoSave') }}
<div class="ml-4">
Expand Down
Loading