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.

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

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

Merged
merged 2 commits into from
Sep 14, 2024

Conversation

rajdip-b
Copy link
Member

@rajdip-b rajdip-b commented Sep 14, 2024

User description

Description

Fixes #355


PR Type

Enhancement, Tests


Description

  • Implemented a new WorkspaceRoleController to manage workspace roles, including methods for creating, updating, deleting, and fetching roles.
  • Defined TypeScript types for workspace role operations, ensuring type safety and clarity in API interactions.
  • Added comprehensive tests for the WorkspaceRoleController, covering all CRUD operations and role existence checks.
  • Updated Jest configuration to focus on workspace role tests, ensuring targeted and efficient test execution.

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

    Copy link
    Contributor

    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.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Sep 14, 2024

    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:

    -  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]

     async checkWorkspaceRoleExists(
    -  request: CheckWOrkspaceRoleExistsRequest,
    +  request: CheckWorkspaceRoleExistsRequest,
       headers?: Record<string, string>
     ): Promise<ClientResponse<CheckWorkspaceRoleExistsResponse>> {
     
    • 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]

    -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:

    -      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]

    +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]

    -return await parseResponse<CreateWorkspaceRoleResponse>(response)
    +try {
    +  return await parseResponse<CreateWorkspaceRoleResponse>(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 rajdip-b merged commit b03ce8e into develop Sep 14, 2024
    4 checks passed
    @rajdip-b rajdip-b deleted the feat/api-client-workspace-role-controller branch September 14, 2024 11:23
    rajdip-b pushed a commit that referenced this pull request Sep 16, 2024
    ## [2.5.0](v2.4.0...v2.5.0) (2024-09-16)
    
    ### 🚀 Features
    
    * **api-client:** Added workspace controller ([#427](#427)) ([2f4edec](2f4edec))
    * **api-client:** Added workspace role controller ([#430](#430)) ([b03ce8e](b03ce8e))
    * **api-client:** Synced with latest API ([27f4309](27f4309))
    * **api:** Add slug in entities ([#415](#415)) ([89e2fcc](89e2fcc))
    * **api:** Included default workspace details in getSelf function ([#414](#414)) ([e67bbd6](e67bbd6))
    * **platform:** Add loading skeleton in the [secure]s page ([#423](#423)) ([a97681e](a97681e))
    * **schema:** Added a schema package ([01ea232](01ea232))
    * **web:** Update about and careers page ([e167f53](e167f53))
    
    ### 🐛 Bug Fixes
    
    * **api:** Error messages fixed in api-key service ([#418](#418)) ([edfbce0](edfbce0))
    
    ### 📚 Documentation
    
    * Fixed minor typo in postman workspace link ([#411](#411)) ([ed23116](ed23116))
    * Updated Postman links ([444bfb1](444bfb1))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Suppressed version check test in [secure] ([4688e8c](4688e8c))
    * **api:** Update slug generation method ([#420](#420)) ([1f864df](1f864df))
    
    ### 🔨 Code Refactoring
    
    * **API:** Refactor workspace-membership into a separate module ([#421](#421)) ([574170f](574170f))
    * **platform:** added optional chaining due to strict null check ([#413](#413)) ([907e369](907e369))
    @rajdip-b
    Copy link
    Member Author

    🎉 This PR is included in version 2.5.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
    Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
    ## [2.5.0](keyshade-xyz/keyshade@v2.4.0...v2.5.0) (2024-09-16)
    
    ### 🚀 Features
    
    * **api-client:** Added workspace controller ([keyshade-xyz#427](keyshade-xyz#427)) ([2f4edec](keyshade-xyz@2f4edec))
    * **api-client:** Added workspace role controller ([keyshade-xyz#430](keyshade-xyz#430)) ([b03ce8e](keyshade-xyz@b03ce8e))
    * **api-client:** Synced with latest API ([27f4309](keyshade-xyz@27f4309))
    * **api:** Add slug in entities ([keyshade-xyz#415](keyshade-xyz#415)) ([89e2fcc](keyshade-xyz@89e2fcc))
    * **api:** Included default workspace details in getSelf function ([keyshade-xyz#414](keyshade-xyz#414)) ([e67bbd6](keyshade-xyz@e67bbd6))
    * **platform:** Add loading skeleton in the [secure]s page ([keyshade-xyz#423](keyshade-xyz#423)) ([a97681e](keyshade-xyz@a97681e))
    * **schema:** Added a schema package ([01ea232](keyshade-xyz@01ea232))
    * **web:** Update about and careers page ([e167f53](keyshade-xyz@e167f53))
    
    ### 🐛 Bug Fixes
    
    * **api:** Error messages fixed in api-key service ([keyshade-xyz#418](keyshade-xyz#418)) ([edfbce0](keyshade-xyz@edfbce0))
    
    ### 📚 Documentation
    
    * Fixed minor typo in postman workspace link ([keyshade-xyz#411](keyshade-xyz#411)) ([ed23116](keyshade-xyz@ed23116))
    * Updated Postman links ([444bfb1](keyshade-xyz@444bfb1))
    
    ### 🔧 Miscellaneous Chores
    
    * **api:** Suppressed version check test in [secure] ([4688e8c](keyshade-xyz@4688e8c))
    * **api:** Update slug generation method ([keyshade-xyz#420](keyshade-xyz#420)) ([1f864df](keyshade-xyz@1f864df))
    
    ### 🔨 Code Refactoring
    
    * **API:** Refactor workspace-membership into a separate module ([keyshade-xyz#421](keyshade-xyz#421)) ([574170f](keyshade-xyz@574170f))
    * **platform:** added optional chaining due to strict null check ([keyshade-xyz#413](keyshade-xyz#413)) ([907e369](keyshade-xyz@907e369))
    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.

    API-CLIENT: Create controller for Workspace Role module
    1 participant