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
2 changes: 1 addition & 1 deletion apps/knowledge/views/knowledge_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class Page(APIView):
def get(self, request: Request, workspace_id: str, knowledge_id: str, current_page: int, page_size: int):
return result.success(
KnowledgeWorkflowActionSerializer(data={'workspace_id': workspace_id, 'knowledge_id': knowledge_id})
.page(current_page, page_size, request.data))
.page(current_page, page_size, request.query_params))

class Operate(APIView):
authentication_classes = [TokenAuth]
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 some potential issues and can be optimized:

  1. Potential Error with query_params: The method .page() is supposed to accept pagination parameters like current_page and page_size. If these should come from request.data, it suggests that there might be an issue with how the data is being accessed or passed.

  2. Security Consideration: The use of TokenAuth for authentication in the Operate view does not provide any additional protection beyond what is necessary for API calls that don't involve sensitive operations. Ensure that other security measures (like Django's UserAuthentication) are in place if required for more secure access controls.

  3. Code Documentation: While not strictly a coding mistake, adding proper documentation comments to your methods would help clarify their purpose and usage.

Here’s a revised version of the code with some improvements:

from django.views import View
from rest_framework.request import Request
import serialization_module  # Assuming KnowledgeWorkflowActionSerializer lives here

class Page(View):
    def get(self, request: Request, workspace_id: str, knowledge_id: str, current_page: int, page_size: int):
        paginator = pagination.PageNumberPagination(page_size=page_size)
        serializer = serializers.KnowledgeWorkflowActionSerializer(data={
            'workspace_id': workspace_id,
            'knowledge_id': knowledge_id
        })
        
        queryset = serializer.get_queryset()
        paginated_queryset = paginator.paginate_queryset(queryset)

        return paginator.get_paginated_response(paginated_queryset)

class Operate(View):
    authentication_classes = [TokenAuthentication]  # Add Django authentication if needed

In this updated version:

  • I've assumed pagination.ModuleName.pagination.PageNumberPagination and serializers.module_name.KnowledgeWorkflowActionSerializer are correctly defined in your project.
  • The query set retrieval and pagination steps are now clearer to follow.
  • Basic authentication classes have been added for demonstration purposes; further customization may be needed based on project requirements.

Always ensure that any custom logic related to get_queryset() within the serializer is properly implemented and handles permissions and security appropriately.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
</el-select>
<el-select
v-if="filter_type === 'state'"
v-model="query.status"
@change="getList"
v-model="query.state"
@change="getList(true)"
style="width: 220px"
clearable
>
Expand All @@ -31,7 +31,7 @@
<el-input
v-else
v-model="query.user_name"
@change="getList"
@change="getList(true)"
:placeholder="$t('common.search')"
prefix-icon="Search"
style="width: 220px"
Expand Down Expand Up @@ -133,7 +133,7 @@ const paginationConfig = reactive({
})
const query = ref<any>({
user_name: '',
status: '',
state: '',
})
const loading = ref(false)
const filter_type = ref<string>('user_name')
Expand Down Expand Up @@ -165,38 +165,22 @@ const changePage = () => {
getList()
}

const getList = () => {
const getList = (clear?: boolean) => {
if (clear) {
paginationConfig.current_page = 1
}
return loadSharedApi({ type: 'knowledge', systemType: apiType.value })
.getWorkflowActionPage(active_knowledge_id.value, paginationConfig, query.value, loading)
.then((ok: any) => {
paginationConfig.total = ok.data?.total
data.value = data.value.concat(ok.data.records)
if (clear) {
data.value = ok.data.records
} else {
data.value = data.value.concat(ok.data.records)
}
})
}

const setRowClass = ({ row }: any) => {
return currentId.value === row?.id ? 'highlight' : ''
}

/**
* 下一页
*/
const nextRecord = () => {
const index = tableIndexMap.value[currentId.value] + 1
if (index >= data.value.length) {
if (index >= paginationConfig.total - 1) {
return
}
paginationConfig.current_page = paginationConfig.current_page + 1
getList().then(() => {
currentId.value = data.value[index].id
currentContent.value = data.value[index]
})
} else {
currentId.value = data.value[index].id
currentContent.value = data.value[index]
}
}
const pre_disable = computed(() => {
const index = tableIndexMap.value[currentId.value] - 1
return index < 0
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:

  1. Duplicate Code: The getList function is called with different parameters (query.user_name) in both places where it's triggered by changes to filters (status vs. user_name). It should consistently use one of these based on the filter_type.

  2. Logic Inconsistency: When clearing the page, the condition for resetting the page number is only checked once before calling getList. This makes it inconsistent with how other logic works.

  3. Variable Confusion: There seems to be confusion between the currentId, tableIndexMap, data.value, currentContent, and related variables. These need to be clearly defined and used consistently throughout the component.

Here are some optimizations and corrections:

a. Consistently Use Either status or user_name:

const getList = (clear?: boolean) => {
  if (clear) {
    paginationConfig.current_page = 1;
  }  
  const params = { ...query.value };
  if (!Object.keys(params).length) params[user_filter_key.value] = ''; // Assuming this key exists

  return loadSharedApi({ type: 'knowledge', systemType: apiType.value })
    .getWorkflowActionPage(active_knowledge_id.value, paginationConfig, params, loading)
    .then((ok: any) => {
      paginationConfig.total = ok.data?.total;
      if (clear) {
        data.value = ok.data.records;  
      } else {
        data.value = data.value.concat(ok.data.records);
      }
    });
};

b. Ensure Consistent Use of Variable Names:
Correcting naming inconsistencies helps clarify intentions and avoids mistakes:

type RowState = 'highlight';
let selectedRowHighlightID: string | null = null;

// Check functions can now refer accurately

setRowClass(row): string {
  if (selectedRowHighlightID === row.id) {   
    return 'highlight';
  }

  // Default behavior for un-highlighted rows
}

onNextClick(): void {
  let currentIndex = tableData.findIndex(r => r.id === currRowID);
  if (currentIndex < 0 || currentIndex >= tableData.length - 1) return;

  selectedRowHighlightID = tableData[currentIndex + 1].id;
} 

onPrevClick(): void {
  let currentIndex = tableData.findIndex(r => r.id === currRowID);
  if (currentIndex <= 0 ) return;

  selectedRowHighlightID = tableData[currentIndex - 1].id;
}

This approach ensures that there isn't unnecessary duplication or misinterpretation of context.

Expand Down
Loading