Skip to content

feat: UI permission instruction #3010

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

Merged
merged 1 commit into from
Apr 28, 2025
Merged

feat: UI permission instruction #3010

merged 1 commit into from
Apr 28, 2025

Conversation

shaohuzhang1
Copy link
Contributor

feat: UI permission instruction

Copy link

f2c-ci-robot bot commented Apr 28, 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 Apr 28, 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

},
})
},
}
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 functions as intended to conditionally display elements based on user permissions using Vue directives with TypeScript types. However, there are a few minor improvements and optimizations that could be made:

  1. Type Safety: Ensure that binding.value is always an object before accessing its properties. This helps prevent runtime errors due to undefined values.

  2. Error Handling: Add some basic error handling to ensure that permission logic runs smoothly even if input is unexpected.

  3. Dynamic Permission Check: If you have dynamic or complex permission checks, consider implementing more sophisticated validation within the display function.

  4. Optimization: Since this directive only needs to run during creation and updates, you can use asynchronous methods like async created() without unnecessary complexity.

Here's the revised code:

import type { App } from 'vue'
import { hasPermission } from '@/utils/permission'

const display = async (el: HTMLElement, binding: DirectiveBinding) => {
  // Validate inputs
  if (!isObject(binding.value)) {
    console.error('Directive "hasPermission" requires an object argument.');
    return;
  }

  const has = await new Promise((resolve) => {
    resolve(hasPermission(
      binding.value.permission || binding.value,
      binding.value.compare || 'OR',
    ));
  });

  if (!has) {
    el.style.display = 'none';
  } else {
    delete el.style.display;
  }
}

export default {
  install: (app: App) => {
    app.directive('hasPermission', {
      async created(el: HTMLElement, binding: DirectiveBinding) {
        display(el, binding);
      },
      async updated(el: HTMLElement, binding: DirectiveBinding) {
        display(el, binding);
      },
    });
  },
};

// Helper function to check if a value is an object
function isObject(obj: unknown): obj is Record<string, any> {
  return typeof obj === 'object' && obj !== null;
}

These changes enhance maintainability and robustness of the code.

>
default工作空间的知识库xxx权限
</div>
</template>
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 code appears to be correct for the most part, but here are some minor improvements and suggestions:

  1. Ensure that v-hasPermission is properly imported and used. If it's a custom directive or function, make sure you've defined it before using it.

  2. Remove the newline after <script> tags: While this doesn't affect functionality, adhering to consistency can prevent confusion for others reading the code.

  3. Add a line break between the opening tag of the template and its content to improve readability. This isn't strictly necessary in this case but is good practice for larger templates.

  4. Consider adding more context comments explaining each section if needed, especially for newlines or significant changes like removing the previous template.

*/
getWorkspaceResourcePermission(workspace_id: string, resource: string, resource_id: string) {
return `${this.permission}:/WORKSPACE/${workspace_id}/${resource}/${resource_id}`
}
}
/**
* 复杂权限对象
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 obvious syntax errors or logical issues with the provided code snippet. However, here are some potential suggestions for further improvement:

  1. Type Annotations: Add type annotations to both class fields and methods to improve readability and maintainability.

  2. Error Handling: Consider adding error handling around the API calls if applicable, such as catching exceptions when retrieving permissions.

  3. Code Refactoring: Depending on the specific requirements, you might want to refactor how the permission strings are generated. For example, using template literals could make it more readable and easier to extend in the future.

Here's an updated version of the code including some minor adjustments:

import { WorkspaceId } from './workspace-id'; // Assuming WorkspaceId is defined somewhere

export interface Permission {
  permission: string;
}

export class Permission implements Permission {
  private readonly permission: string;

  constructor(permission: string) {
    this.permission = permission;
  }

  /**
   * 获取工作空间权限
   * @param workspace_id 工作空间ID
   * @returns 工作空间权限字符串
   */
  public getWorkspacePermission(workspace_id: WorkshopId): string {
    return `${this.permission}:/WORKSPACE/${workspace_id}`;
  }

  /**
   * 获取工作空间资源权限
   * @param workspace_id 工作空间ID
   * @param resource 源资源类型(如 'user', 'project' 等)
   * @param resourceId 资源ID
   * @returns 工作空间资源权限字符串
   */
  public getResourceWorkspacePermission(
    workspace_id: WorkshopId,
    resource: string,
    resourceId: number | string
  ): string {
    const resourcePartSuffix = typeof resourceId === 'number'
      ? `/r${resourceId.toString()}` : `/${resource_id}`;

    return `${this.permission}:/WORKSPACE/${workspace_id}/${resource}${resourcePartSuffix}`;
  }
}

In this revised version, I've introduced a new WorkshopId type (assuming it exists somewhere) and added parameter declarations with types. Additionally, I optimized the function names slightly and used typeof to append additional parts to the URL based on whether resourceId is a number or not. Note that these changes should be tested with actual usage patterns to ensure they meet all requirements.

@shaohuzhang1 shaohuzhang1 merged commit 99fd328 into v2 Apr 28, 2025
3 of 4 checks passed
@shaohuzhang1 shaohuzhang1 deleted the pr@v2@feat_ui_permission branch April 28, 2025 02:20
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.

1 participant