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.

Sign up for GitHub

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
+11 −4
Merged

refactor: Search ignores capitalization #2279

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/function_lib/serializers/function_lib_serializer.py
View file
Original file line number Diff line number Diff line change
@@ -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.

2 changes: 1 addition & 1 deletion apps/setting/serializers/provider_serializers.py
View file
Original file line number Diff line number Diff line change
@@ -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.

11 changes: 9 additions & 2 deletions ui/src/views/team/component/PermissionSetting.vue
View file
Original file line number Diff line number Diff line change
@@ -57,7 +57,12 @@
/>
</template>
</el-table-column>
<el-table-column :label="$t('views.team.setting.check')" align="center" width="100" fixed="right">
<el-table-column
:label="$t('views.team.setting.check')"
align="center"
width="100"
fixed="right"
>
<template #header>
<el-checkbox
:disabled="props.manage"
@@ -135,7 +140,9 @@ const allChecked: any = ref({

const filterText = ref('')

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.