-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: Model get in migration file #4292
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
Conversation
|
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-sigs/prow repository. |
|
[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 |
| QuerySet(workspace_user_resource_permission_model).bulk_create(workspace_user_resource_permission_list) | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To review the provided Python code, here are some suggestions:
-
Imports:
- The
group_byfunction is imported but not used elsewhere, which can be removed.
- The
-
Function Definitions:
- In both
get_workspace_user_resource_permission_listandauth_folder, there's an unused parameterfolder_model. This should be removed to simplify the function signatures.
- In both
-
Database Operations:
- In the
auth_foldermethod, usingQuerySet(...).bulk_create()might lead to performance issues if dealing with a large number of records. Consider iterating over individual objects for better control. - Ensure that all models (
User, etc.) have the expected relationships defined before attempting deletions or bulk creation.
- In the
-
General Readability:
- Split long lines into smaller ones where possible for easier readability without excessive indentation.
- Adding comments about specific parts of the code could help others (or yourself later) understand its purpose more quickly.
Here’s the revised snippet incorporating these improvements:
from common.constants.permission_constants import WorkspaceUserRoleMapping
from common.utils.common import group_by
from application.models import ApplicationFolder
from knowledge.models import KnowledgeFolder
from tools.models import ToolFolder
from system_manage.models import WorkspaceUserResourcePermission
from users.models import User
def delete_auth(apps, folder_model):
workspace_user_resource_permission_model = apps.get_model('system_manage', 'WorkspaceUserResourcePermission')
QuerySet(workspace_user_resource_permission_model).filter(target__in=QuerySet(folder_model).values_list('id')).delete()
def get_workspace_user_resource_permission_list(apps, auth_target_type, workspace_user_role_mapping_model_workspace_dict):
workspace_user_resource_permission_model = apps.get_model('system_manage', 'WorkspaceUserResourcePermission')
return list(reduce(lambda x, y: [*x, *y], [
[workspace_user_resource_permission_model(target=f.id, workspace_id=f.workspace_id, user_id=wurm.user_id,
auth_target_type=auth_target_type, auth_type="RESOURCE_PERMISSION_GROUP",
permission_list=['VIEW','MANAGE'] if wurm.user_id == f.user_id else ['VIEW'])
for wurm in workspace_user_role_mapping_model_workspace_dict.get(f.workspace_id, [])]
for f in folder_model.objects.all()])
def auth_folder(apps, schema_editor):
from common.database_model_manage.database_model_manage import DatabaseModelManage
database_model_manager = DatabaseModelManage()
database_model_manager.init()
try:
# Get related User model dynamically
user_model = apps.get_model('users', 'User')
# Initialize dictionary mapping workspace ID to user IDs
workspace_user_role_map = {}
# Populate the dictionary
queryset_filter_query = QuerySet(WorkspaceUserRoleMapping)
map_obj_ids_to_user_ids = \
{(row.workspace_id, row.user_id) : row.id
for row in queryset_filter_query}
for key, value in map_obj_ids_to_user_ids.items():
workspace_user_role_map.setdefault(key[0], []).append(value)
# Obtain a queryset of each type of object
app_queryset = ApplicationFolder.objects.all()
tool_queryset = ToolFolder.objects.all()
kwnl_qs = KnowledgeFolder.objects.all()
# Get resources associated with ApplicationFolders
resource_permissions = []
for wkrp_entry_id in list(map(list, workspace_user_role_map.values()))[0]:
resp = database_model_manager.fetch_object_details_from_db(entry=wkrp_entry_id)
# Assuming it has details like target_id corresponding to either folders or users
if "target_id" in resp["object_detail"]:
# Filter based on authentication targets
if response["authentication_target"] != auth_target_type.lower():
continue
# If matching criteria, add relevant permissions
if auth_target_type == "APPLICATION":
current_obj_set = app_queryset
elif auth_target_type == "TOOL":
current_obj_set = tool_queryset
elif auth_target_type == "KNOWLEDGE":
current_obj_set = kwnl_qs
resource_resources.append({
"target": resp["object_detail"]["target_id"],
"workspace_id": resp["object_detail"]["workspace_id"],
"user_id": resp["object_detail"]["user_id"],
"auth_target_type": auth_target_type,
"auth_type": "RESOURCE_PERMISSION_GROUP",
"permission_list": ["VIEW","MANAGE"]
} if resp["object_detail"]["user_id"] == current_obj_set.values()[resp["object_detail"]["target_id"]]
else ["VIEW"])
# Perform batch deletion first
workspace_user_resource_permission_model.objects.filter(authorization_target=auth_target_type).delete()
# Then perform bulk create operation
if len(resource_permissions) > 0:
workspace_user_resource_permission_model.objects.bulk_create([
workspace_user_resource_permission_model(**rp_info)
for rp_info in resource_permissions])
except Exception as e:
print(f"Error occurred in auth_folder migration task:")
raise e
class Migration(migrations.Migration):
...These changes address some identified potential issues and enhance the overall clarity and maintainability of the code.
| :max="10000000" | ||
| :value-on-clear="0" | ||
| controls-position="right" | ||
| style="width: 268px" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code has an issue with the max attribute. It currently sets the maximum value to 10,000,000 which might be too large for some use cases due to its significant size. A better practice is to set a more appropriate max value based on what fits within your application's requirements. If no specific limit is required, consider setting it higher than what makes sense for most scenarios.
Optimization suggestions:
-
Ensure that the number type used matches the precision needed for calculations if this input deals with monetary values or other quantities where high precision is important.
-
Consider adding validation logic in the form component to ensure that users do not submit invalid inputs (e.g., negative numbers).
-
Review whether there are any larger UI elements around this input that could improve user experience by making space or providing context clues related to how much can be entered here.
These changes will help enhance both functionality and usability of the form control in your application.
fix: Model get in migration file