search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
208 stars 105 forks source link

feat(api-client): Added workspace role controller #430

Closed rajdip-b closed 2 months ago

rajdip-b commented 2 months ago

User description

Description

Fixes #355

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
workspace-role.ts
Implement Workspace Role Controller with CRUD operations 
packages/api-client/src/controllers/workspace-role.ts
  • Added WorkspaceRoleController class for managing workspace roles.
  • Implemented methods for creating, updating, deleting, and fetching
    workspace roles.
  • Included method to check if a workspace role exists.
    		•  +102/-0 
    workspace-role.types.d.ts
    Define Types for Workspace Role Operations                             
    packages/api-client/src/types/workspace-role.types.d.ts
  • Defined types for workspace role requests and responses.
  • Included interfaces for creating, updating, and deleting workspace
    roles.
  • Added interfaces for checking existence and fetching workspace roles.
    		•  +68/-0   
    Tests
    workspace-role.spec.ts
    Add Tests for Workspace Role Controller                                   
    packages/api-client/tests/workspace-role.spec.ts
  • Added tests for WorkspaceRoleController methods.
  • Tested CRUD operations and existence check for workspace roles.
  • Ensured proper setup and teardown of test data.
    		•  +180/-0 
    Configuration changes
    jest.config.ts
    Update Jest Configuration for Workspace Role Tests             
    packages/api-client/jest.config.ts - Updated test match pattern to focus on workspace role tests.
    		+1/-1     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The controller methods don't include explicit error handling. Consider adding try-catch blocks or error handling middleware to manage potential API errors gracefully. Code Duplication
    The API endpoint construction is repeated in each method. Consider extracting this logic into a separate method to improve maintainability. Test Data Management
    The test suite creates and deletes test data in beforeAll/afterAll hooks. Consider using a separate test database or mocking the API calls to avoid potential side effects.
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    ✅ Correct the typo in the type name for consistency ___
    Suggestion Impact:The typo in the type name CheckWOrkspaceRoleExistsRequest was corrected to CheckWorkspaceRoleExistsRequest. code diff: ```diff - CheckWOrkspaceRoleExistsRequest + CheckWorkspaceRoleExistsRequest } from '@api-client/types/workspace-role.types' export default class WorkspaceRoleController { @@ -62,7 +63,7 @@ } async checkWorkspaceRoleExists( - request: CheckWOrkspaceRoleExistsRequest, + request: CheckWorkspaceRoleExistsRequest, ```
    ___ **The checkWorkspaceRoleExists method has a typo in the type name
    CheckWOrkspaceRoleExistsRequest. This should be corrected to maintain consistency
    and avoid potential issues.** [packages/api-client/src/controllers/workspace-role.ts [64-67]](https://github.com/keyshade-xyz/keyshade/pull/430/files#diff-7bed7d7dd377ccffbc718e4a4195a7f10edbb5bcab01927160935892168408bfR64-R67) ```diff async checkWorkspaceRoleExists( - request: CheckWOrkspaceRoleExistsRequest, + request: CheckWorkspaceRoleExistsRequest, headers?: Record ): Promise> { ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Correcting the typo in the type name is crucial for consistency and avoiding potential bugs, making this a high-priority fix.
    		9
    Best practice
    ✅ Use a more robust method for constructing URLs with query parameters ___ **The getWorkspaceRolesOfWorkspace method constructs the URL manually, which can be
    error-prone. Consider using a URL builder library or a helper function to construct
    the URL with query parameters more safely and efficiently.** [packages/api-client/src/controllers/workspace-role.ts [92-98]](https://github.com/keyshade-xyz/keyshade/pull/430/files#diff-7bed7d7dd377ccffbc718e4a4195a7f10edbb5bcab01927160935892168408bfR92-R98) ```diff -let url = `/api/workspace-role/${request.workspaceSlug}/all?` -request.page && (url += `page=${request.page}&`) -request.limit && (url += `limit=${request.limit}&`) -request.sort && (url += `sort=${request.sort}&`) -request.order && (url += `order=${request.order}&`) -request.search && (url += `search=${request.search}&`) +const queryParams = new URLSearchParams({ + ...(request.page && { page: request.page.toString() }), + ...(request.limit && { limit: request.limit.toString() }), + ...(request.sort && { sort: request.sort }), + ...(request.order && { order: request.order }), + ...(request.search && { search: request.search }), +}); +const url = `/api/workspace-role/${request.workspaceSlug}/all?${queryParams}`; const response = await this.apiClient.get(url, headers) ``` `[Suggestion has been applied]`
    Suggestion importance[1-10]: 8 Why: Using a URL builder library or helper function reduces the risk of errors in URL construction, improving code reliability and readability. This is a significant improvement in best practices.
    		8
    ✅ Improve null value handling in test suite ___
    Suggestion Impact:The non-null assertion was removed from the workspaceSlug assignment, which aligns with the suggestion to handle potential null values more robustly. code diff: ```diff - workspaceSlug: workspaceSlug!, + workspaceSlug, ```
    ___ **The test suite is using non-null assertions (!) frequently. Consider using a more
    robust approach to handle potential null values, such as conditional checks or
    throwing meaningful errors if the values are unexpectedly null.** [packages/api-client/tests/workspace-role.spec.ts [38-45]](https://github.com/keyshade-xyz/keyshade/pull/430/files#diff-50ef242eeac8e109aa4db36b3bb908a4d5c5c12ae9e1c95585ae5f37efa4d06dR38-R45) ```diff +if (!workspaceSlug) { + throw new Error('Workspace slug is null or undefined'); +} const createWorkspaceRoleRequest: CreateWorkspaceRoleRequest = { - workspaceSlug: workspaceSlug!, + workspaceSlug: workspaceSlug, name: 'Developer', description: 'Role for developers', colorCode: '#FF0000', authorities: ['READ_WORKSPACE', 'READ_PROJECT'], projectSlugs: [] } ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: This suggestion enhances the robustness of the test suite by handling potential null values more explicitly, which is a good practice but not critical for the test's functionality.
    		6
    Error handling
    Implement more specific error handling for API responses ___ **Consider using a more specific error handling mechanism instead of relying on the
    generic parseResponse function. This could involve creating custom error classes for
    different types of API errors and handling them explicitly in the controller
    methods.** [packages/api-client/src/controllers/workspace-role.ts [36]](https://github.com/keyshade-xyz/keyshade/pull/430/files#diff-7bed7d7dd377ccffbc718e4a4195a7f10edbb5bcab01927160935892168408bfR36-R36) ```diff -return await parseResponse(response) +try { + return await parseResponse(response); +} catch (error) { + if (error instanceof ApiError) { + // Handle specific API errors + throw new WorkspaceRoleError(error.message); + } + throw error; +} ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: This suggestion improves error handling by introducing specific error classes, which enhances code robustness and maintainability. However, it is not critical for functionality, hence a moderate score.
    		7
    rajdip-b commented 2 months ago

    :tada: This PR is included in version 2.5.0 :tada:

    The release is available on GitHub release

    Your semantic-release bot :package::rocket: