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

refactor: Search ignores capitalization #2279

Merged
merged 1 commit into from
Feb 14, 2025
Merged

Conversation

shaohuzhang1
Copy link
Contributor

refactor: Search ignores capitalization --story=1017866 --user=王孝刚 【Bug转需求】【搜索】-函数库、模型、团队成员界面搜索不支持忽略大小写 #2126 https://www.tapd.cn/57709429/s/1654282

--story=1017866 --user=王孝刚 【Bug转需求】【搜索】-函数库、模型、团队成员界面搜索不支持忽略大小写 #2126 https://www.tapd.cn/57709429/s/1654282
Copy link

f2c-ci-robot bot commented Feb 14, 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 Feb 14, 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

@wxg0103 wxg0103 merged commit 9c67f6b into main Feb 14, 2025
4 checks passed
@wxg0103 wxg0103 deleted the pr@main@refactor_1017866 branch February 14, 2025 02:56
const filterData = computed(() => props.data.filter((v: any) => v.name.includes(filterText.value)))
const filterData = computed(() =>
props.data.filter((v: any) => v.name.toLowerCase().includes(filterText.value.toLowerCase()))
)

const allIndeterminate: any = ref({
[TeamEnum.MANAGE]: computed(() => {
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 looks mostly correct and follows best practices. However, there are a couple of suggestions to optimize or improve it:

  1. Case Insensitive Search: The current filter function uses includes, which is case-sensitive. If you want the search to be case-insensitive, use toLowerCase() on both filterText and each element's name. This ensures that searches are not affected by capitalization.

  2. Use of Computed Values: The allIndeterminate object keys should be properly formatted (i.e., starting with an uppercase letter). In JavaScript/TypeScript, variable names must start with a letter or underscore. It's also generally good practice to make allChecked into a reactive object instead of just using a plain reference.

Here's the revised code with these considerations addressed:

<template>
  <!-- Existing template -->
</template>

<script setup lang="ts">
import { ref, computed } from 'vue';
import { TeamEnum } from './teamConstants'; // Assuming this file exists

const props = defineProps({
  data: Array<any>,
  manage: Boolean,
  selectAllAction: Function,
});

const allChecked: Record<string, boolean> = reactronRef({ [TeamEnum.MANAGE]: true }, false);

const filterText = ref('');

// Case insensitive search
const filterData = computed(() =>
  props.data.filter(item => item.name.toLowerCase().includes(filterText.value.toLowerCase()))
);

const allIndeterminate: Record<string, boolean> = {
  [TeamEnum.MANAGE]: computed(() => Object.values(allChecked).some(val => val)),
};

// Emit action when selecting/deselecting rows
function handleSelect(row: any, checked: boolean) {
  if (!row.manage) return;
  selectedValues[row.id] = checked;
  allChecked[TeamEnum.MANAGE] = Object.values(selectedValues).every(value => value);
}

onMounted(async () => {
  // Load initial state
});
</script>

<style scoped>
/* Existing styles */
</style>

Explanation of Changes:

  • Added reactronRef usage for allChecked to create a reactive object.
  • Updated the logic inside the handleSelect function to correctly update nested properties in selectedValues.
  • Made allIndeterminate a more typical way to track indeterminate states in Vue.
  • Corrected the formatting of property keys in allIndeterminate.

These changes ensure the code remains efficient, maintainable, and adheres to standard JavaScript/TypeScript conventions.

@@ -105,7 +105,7 @@ def get_query_set(self):
query_set = QuerySet(FunctionLib).filter(
(Q(user_id=self.data.get('user_id')) | Q(permission_type='PUBLIC')))
if self.data.get('name') is not None:
query_set = query_set.filter(name__contains=self.data.get('name'))
query_set = query_set.filter(name__icontains=self.data.get('name'))
if self.data.get('desc') is not None:
query_set = query_set.filter(desc__contains=self.data.get('desc'))
if self.data.get('is_active') is not None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no significant issues with this code snippet. The name__icontains filter should be used instead of name__contains to ensure that the search matches names which contain the provided substring, regardless of case sensitivity. Here's the corrected line:

query_set = query_set.filter(name__icontains=self.data.get('name'))

Additionally, it would be beneficial to add error handling around the data extraction from self.data. For example, you can check if the key exists before accessing its value:

if 'user_id' in self.data:
    user_id = self.data['user_id']
else:
    raise ValueError("User ID is required.")

if 'permission_type' in self.data:
    permission_type = self.data['permission_type']
else:
    permission_type = 'PUBLIC'

These changes will help prevent potential errors when attempting to access non-existent keys in self.data.

@@ -106,7 +106,7 @@ def list(self, with_valid):
model_query_set = QuerySet(Model).filter((Q(user_id=user_id) | Q(permission_type='PUBLIC')))
query_params = {}
if name is not None:
query_params['name__contains'] = name
query_params['name__icontains'] = name
if self.data.get('model_type') is not None:
query_params['model_type'] = self.data.get('model_type')
if self.data.get('model_name') is not None:
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 Python function has the following improvements:

  1. Use self.data.get('model_type') instead of directly accessing it without a check. This prevents errors if 'model_type' is missing and avoids using an undefined variable.

  2. Update the field lookup to use _icontains which means partial case-insensitive matching in Django's ORM, while __contains would perform exact string matching.

  3. Remove unnecessary brackets around conditions (Q(user_id=user_id) | Q(permission_type='PUBLIC')), since this can be simplified as shown above.

This changes will make the query more robust by handling optional parameters safely and improving performance when dealing with potentially large dataset queries.

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