feat: Modify the authorization information of resources for users

[shaohuzhang1]

feat: Modify the authorization information of resources for users

[f2c-ci-robot]

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.

Details

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.

[f2c-ci-robot]

[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.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

The provided code snippet is a series of serializer definitions and API methods related to managing user resource permissions within a Django application. While the structure looks comprehensive, there are a few areas that could be improved for better clarity and functionality:

1. Translation Usage: The _ function is used as a translation context in the ResultSerializer, but it seems unnecessary since these fields (user id, name, etc.) do not actually require translation. Consider removing the translations.

2. Duplicate Parameter Definitions: There might be duplicate parameters across different serializers or classes if they all use similar paths or query strings.

3. Simplified Query Parameters: For pagination requests, you can simplify the syntax by using generic parameter names like page_number instead of current_page. This enhances readability and consistency with other systems and libraries.

4. Field Descriptions Alignment: Ensure consistent descriptions for each field to maintain clear understanding across serializers.

5. Error Handling: Adding error handling logic would improve the robustness and user experience of the APIs.

6. Code Structure Review: It would be beneficial to review the overall structural organization of the module to ensure logical flow and modularity.

Here's an updated version addressing some of these points:

from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import OpenApiParameter
from rest_framework import serializers

from common.mixins.api_mixin import APIMixin
from common.result import ResultSerializer, ResultPageSerializer

class APIUserResourcePermissionResponse(ResultSerializer):
    pass

class EditUserResourcePermissionAPI(APIMixin):
    @staticmethod
    def get_request():
        return UpdateUserResourcePermissionRequest()

# New Response Serializer with Simple Field Labels
class ResourceUserPermissionSimpleResponse(serializers.Serializer):
    id = serializers.CharField(label='id')
    nick_name = serializers.CharField(allow_null=True, allow_blank=True)
    username = serializers.CharField(allow_null=True, allow_blank=True)
    permission = serializers.CharField()  # Simplified to string

class APIResourceUserPermissionResponse(ResultSerializer):
    def get_data(self):
        return ResourceUserPermissionSimpleResponse(many=True)

class ResourceUserPermissionAPI(APIMixin):
    @staticmethod
    def get_parameters():
        return [
            OpenApiParameter(
                name="workspace_id",
                description="Workspace ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="target",
                description="Target resource ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="resource_type",
                description="Resource type",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="username",
                description="Username (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
            OpenApiParameter(
                name="nickname",
                description="Nickname (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
            OpenApiParameter(
                name="permission",
                description="Permission value (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
        ]

    @staticmethod
    def get_response():
        return APIResourceUserPermissionResponse


class ResourceUserPermissionPageAPI(APIMixin):
    @staticmethod
    def get_parameters():
        return [
            OpenApiParameter(
                name="workspace_id",
                description="Workspace ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="target",
                description="Target resource ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="resource_type",
                description="Resource type",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="page_number",
                description="Page number",
                type=OpenApiTypes.INT,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="page_size",
                description="Page size",
                type=OpenApiTypes.INT,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="username",
                description="Username (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
            OpenApiParameter(
                name="nickname",
                description="Nickname (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
            OpenApiParameter(
                name="permission",
                description="Permission value (optional)",
                type=OpenApiTypes.STRING,
                location='query',
                required=False,
            ),
        ]

    @staticmethod
    def get_response():
        return ResourceUserPermissionPageResponse()


class ResourceUserPermissionEditAPI(APIMixin):
    @staticmethod
    def get_parameters():
        return [
            OpenApiParameter(
                name="workspace_id",
                description="Workspace ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="target",
                description="Target resource ID",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
            OpenApiParameter(
                name="resource_type",
                description="Resource type",
                type=OpenApiTypes.STRING,
                location='path',
                required=True,
            ),
        ]
    
    @staticmethod
    def get_request():
        return [ResourceUserPermissionEditRequest(), ]  # Use list for multiple instances
        
    @staticmethod
    def get_response():
        return APIResourceUserPermissionResponse()

These changes aim to enhance readability and maintainability while ensuring that the API remains functional.

[shaohuzhang1]

{
  "error": "The patch contains multiple issues:"
}

[shaohuzhang1]

The provided code has several issues that need to be addressed:

Issues Detected

1. Duplicate Imports in Serializer Classes:

   • ResourceUserPermissionSerializer appears twice in different imports under the same module. This is unnecessary and should be unified.

2. Incorrect Usage of Class Methods:

   • The resource_user_permission_view.py, edit_resource_user_permission_api.py, page_resource_user_permission_api.py, and put_resource_user_permission_api.py files contain methods like get_parameters(), get_request(), and get_response(). These methods are intended for API endpoints but not defined within their respective classes. They should inherit from a parent class or define them inside each file where they're used.

3. Missing Documentation Comments:

   • Most method definitions lack comprehensive comments explaining what each part does. Adding detailed documentation will help others understand the functionality better and facilitate maintenance.

4. Variable Naming Conventions Not Followed:

   • Variable names result, request, workspace_id, etc., could use more descriptive names to improve readability and maintainability, especially if they aren't universally understood or have specific meanings.

5. Inconsistent Use of Typing:

   • There's some inconsistency with typing hints throughout the codebase (e.g., Request vs. other types), which can make it harder for static type checkers to enforce proper usage patterns.

Optimization Suggestions

1. Reduce Code Duplication:

   • Remove the duplicate import statements related to ResourceUserPermissionSerializer.

2. Define Method Definitions Clearly:

   • Organize the methods in appropriate parent classes or ensure all essential logic is encapsulated within individual files where these utilities are needed.

3. Enforce Better Coding Practices:

   • Adopt a consistent style guide for coding practices across the project to enhance readability and maintainability.

Here’s an improved version incorporating these suggestions:

# Import necessary modules
from rest_framework.views.generic import APIView
from rest_framework.permissions import TokenAuth
from utils.extend_schema_decorator import extend_schema  # Assuming this function exists somewhere
from common.log.log import log
from common.result import DefaultResultSerializer
from system_manage.api.user_resource_permission import EditUserResourcePermissionAPI
from system_manage.serializers.user_resource_permission import UserResourcePermissionSerializer


@extend_schema(
    responses={},
    tags=[_("Resources authorization")]  # type: ignore
)
class ResourceUserPermissionAPIView(APIView):    
    authentication_classes = [TokenAuth]

    def GET(request, workspace_id, target, resource):
        return result.success(UserResourcePermissionSerializer(data={
            'workspace_id': workspace_id,
            "target": target,
            'auth_target_type': resource,
        }).list({
            'username': request.query_params.get("username"),
            'nick_name': request.query_params.get("nick_name"),
            'permission': request.query_params.get("permission")
        }))

    def PUT(request, workspace_id, target, resource):
        edit_data = request.data.copy()
        user_info = {
            'username': request.query_params.get("uName"),
            'nick_name': request.query_params.get("nick_name")
        }
        edit_data.update(user_info)
        edited_resources = [
            {"user_id": r["user_id"], "perm_value": r["perm_value"]}
            for r in edit_data['resources']
            if isinstance(r.get('status').lower(), str) and r.get('role')
        ]
        
        permission_obj = self.edit_user_permissions(workspace_id, edited_resources)

        return result.success(permission_obj)

    @staticmethod
    def edit_user_permissions(id, resources_to_edit):
        """Helper method for editing permissions based on input id & new permission list."""
        pass

    def POST(request, workspace_id, target, resource):
        edit_data = request.data.copy()
        user_info = {
            'username': request.query_params.get("uName"),
            'nick_name': request.query_params.get("nick_name")
        }
        edit_data.update(user_info)

        try:
            ret_result = edit_data.validate_all(required=False)
            if not res_result.status_code == 200:
                raise Exception(res_result.body["message"])
            
            edit_resources = []
            for item in edit_data.resources:
                temp_dict = {"user_id": item["userId"], "perm_type":item["roleId"],"operateType":"EDIT"}
                edit_resources.append(temp_dict)
            
            obj = edit_resources.serialize_many(edit_resources)   
            # Assuming there's a service call elsewhere in your app to handle the update
            
            return True
        
        except Exception as e:
            print(f'Error during adding resource permission {str(e)}')  

This revised code attempts to address the identified issues while improving overall structure and clarity. Please note that I assumed the existence of functions like validate_all() and .serialize_many() which were mentioned but not provided inline here, so those lines would need to be properly implemented according to your application's needs. Additionally, placeholders like "uName" should be replaced with actual query parameter keys relevant to your application context.