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

refactor(API): Refactor workspace-membership into a separate module #421

Merged

Conversation

unamdev0
Copy link
Contributor

@unamdev0 unamdev0 commented Sep 10, 2024

User description

Description

Created seperate modulr for workspace membership

Fixes #319

Documentation Update

  • Need to update postman and add new module workspace-membership,also need to remove some endpoints from workspace module

PR Type

enhancement, tests


Description

  • Refactored the workspace module to separate membership-related logic into a new workspace-membership module.
  • Implemented a new WorkspaceMembershipService for managing workspace memberships, including transferring ownership, inviting users, and updating roles.
  • Created a WorkspaceMembershipController to handle HTTP requests related to workspace memberships.
  • Added comprehensive end-to-end and unit tests for the new workspace membership functionalities.
  • Updated existing tests to integrate with the new workspace membership module.

Changes walkthrough 📝

Relevant files
Enhancement
6 files
workspace.service.ts
Refactor workspace service to separate membership logic   

apps/api/src/workspace/service/workspace.service.ts

  • Removed workspace membership-related methods.
  • Simplified workspace service by focusing on workspace creation and
    updates.
  • Refactored to use the new workspace-membership module.
  • +82/-1057
    workspace-membership.service.ts
    Implement workspace membership management service               

    apps/api/src/workspace-membership/service/workspace-membership.service.ts

  • Implemented service for managing workspace memberships.
  • Added methods for transferring ownership and managing invitations.
  • Included role updates and membership checks.
  • +1011/-0
    workspace-membership.controller.ts
    Create controller for workspace membership operations       

    apps/api/src/workspace-membership/controller/workspace-membership.controller.ts

  • Created controller for workspace membership operations.
  • Added endpoints for managing membership and invitations.
  • Integrated with workspace membership service.
  • +160/-0 
    create.workspace-membership.ts
    Define DTO for workspace member creation                                 

    apps/api/src/workspace-membership/dto/create.workspace/create.workspace-membership.ts

  • Defined DTO for creating workspace members.
  • Included validation for email and role slugs.
  • +12/-0   
    workspace-membership.module.ts
    Create module for workspace membership management               

    apps/api/src/workspace-membership/workspace-membership.module.ts

  • Created module for workspace membership.
  • Registered service and controller for membership management.
  • +9/-0     
    create.workspace.ts
    Simplify workspace DTO by removing membership details       

    apps/api/src/workspace/dto/create.workspace/create.workspace.ts

  • Removed WorkspaceMemberDTO interface.
  • Focused DTO on workspace creation details.
  • +1/-6     
    Tests
    5 files
    workspace-membership.e2e.spec.ts
    Add end-to-end tests for workspace membership                       

    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts

  • Added end-to-end tests for workspace membership functionalities.
  • Tested ownership transfer, user invitations, and membership updates.
  • Verified event creation for membership actions.
  • +1047/-0
    project.e2e.spec.ts
    Integrate workspace membership service in project tests   

    apps/api/src/project/project.e2e.spec.ts

  • Integrated workspace membership service into project tests.
  • Updated tests to use new membership module.
  • +8/-2     
    workspace-membership.controller.spec.ts
    Add unit tests for workspace membership controller             

    apps/api/src/workspace-membership/controller/workspace-membership.controller.spec.ts

  • Added unit tests for workspace membership controller.
  • Verified controller initialization and basic functionality.
  • +38/-0   
    workspace-membership.service.spec.ts
    Add unit tests for workspace membership service                   

    apps/api/src/workspace-membership/service/workspace-membership.service.spec.ts

  • Added unit tests for workspace membership service.
  • Ensured service methods are defined and functional.
  • +34/-0   
    create.workspace-membership.spec.ts
    Add test for CreateWorkspaceMember DTO                                     

    apps/api/src/workspace-membership/dto/create.workspace/create.workspace-membership.spec.ts

  • Added test for CreateWorkspaceMember DTO.
  • Verified DTO definition and validation.
  • +7/-0     

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

    @rajdip-b rajdip-b force-pushed the feature/refactor-workspace-module branch from bec47c3 to b077ab1 Compare September 10, 2024 16:01
    @rajdip-b
    Copy link
    Member

    Do fill up the description about with the pointers whenever you can

    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Code Removal
    Large portions of code related to workspace membership functionality have been removed from this file. Ensure all removed functionality is properly implemented in the new workspace-membership module.

    New Service Implementation
    This new service file contains a lot of complex logic related to workspace membership. Carefully review all methods for correctness, especially around invitation handling, role updates, and ownership transfers.

    Test Coverage
    This new end-to-end test file is quite extensive. Ensure it covers all critical paths and edge cases for the new workspace membership functionality.

    Copy link
    Contributor

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

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a transaction for workspace deletion to ensure atomicity

    Consider using a transaction for the delete operation to ensure atomicity. This can
    prevent partial deletions if an error occurs during the process.

    apps/api/src/workspace/service/workspace.service.ts [157-162]

     // Delete the workspace
    -await this.prisma.workspace.delete({
    -  where: {
    -    id: workspace.id
    -  }
    -})
    +await this.prisma.$transaction(async (prisma) => {
    +  // Delete related entities first (if any)
    +  await prisma.workspaceMember.deleteMany({ where: { workspaceId: workspace.id } });
    +  // Delete the workspace
    +  await prisma.workspace.delete({
    +    where: {
    +      id: workspace.id
    +    }
    +  });
    +});
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Using a transaction for deletion ensures atomicity and prevents partial deletions, which is crucial for maintaining data consistency. This is a significant improvement, hence a high score.

    9
    Use a more descriptive variable name for workspace roles to improve readability

    In the 'beforeEach' function, consider using a more descriptive variable name for
    the workspace roles, such as memberWorkspaceRole instead of memberRole, to improve
    code readability.

    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [150-157]

    -memberRole = await prisma.workspaceRole.create({
    +memberWorkspaceRole = await prisma.workspaceRole.create({
       data: {
         name: 'Member',
         slug: 'member',
         workspaceId: workspace1.id,
         authorities: [Authority.READ_WORKSPACE]
       }
     })
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using a more descriptive variable name can improve code readability, but the current name is already fairly descriptive, so the improvement is minor.

    5
    Enhancement
    Add an assertion to verify the removal of the old role after updating a member's role

    In the 'should be able to update the role of a member' test, consider adding an
    assertion to check that the old role (adminRole) is no longer associated with the
    user after the update.

    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [595-599]

     expect(membership.roles).toEqual([
       {
         roleId: memberRole.id
       }
     ])
    +expect(membership.roles).not.toContainEqual({
    +  roleId: adminRole.id
    +})
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding an assertion to verify the removal of the old role after updating a member's role is a valuable enhancement, as it ensures that the role update process is functioning correctly and completely.

    8
    Provide a more informative error message for BadRequestException

    Consider using a more specific error message in the BadRequestException. Instead of
    using an empty string, provide a clear explanation of why the request is invalid.

    apps/api/src/workspace/service/workspace.service.ts [152-153]

     throw new BadRequestException(
    +  'Invalid request: Unable to perform the requested action on the workspace.'
     )
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion improves the clarity of error messages, which enhances user experience and debugging. However, it does not address a critical issue, so it receives a moderate score.

    7
    Add assertions to check the structure of returned member objects in the test

    In the 'should be able to get all the members of the workspace' test, consider
    adding an assertion to check the structure of the returned member object, ensuring
    it contains expected properties like id, email, or name.

    apps/api/src/workspace-membership/workspace-membership.e2e.spec.ts [1014-1016]

     expect(response.statusCode).toBe(200)
     expect(response.json().items).toBeInstanceOf(Array)
     expect(response.json().items).toHaveLength(1)
    +expect(response.json().items[0]).toHaveProperty('id')
    +expect(response.json().items[0]).toHaveProperty('email')
    +expect(response.json().items[0]).toHaveProperty('name')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding assertions to check the structure of the returned member object enhances the test by ensuring that the response contains the expected properties, improving test robustness and reliability.

    7
    Add a method to resend invitation emails for pending invitations

    Consider adding a method to resend invitation emails for pending invitations, which
    could be useful if the original invitation email was not received or expired.

    apps/api/src/workspace-membership/service/workspace-membership.service.ts [1011]

     }
     
    +async resendInvitation(
    +  user: User,
    +  workspaceSlug: Workspace['slug'],
    +  inviteeEmail: User['email']
    +): Promise<void> {
    +  const workspace = await this.authorityCheckerService.checkAuthorityOverWorkspace({
    +    userId: user.id,
    +    entity: { slug: workspaceSlug },
    +    authorities: [Authority.ADD_USER],
    +    prisma: this.prisma
    +  })
    +
    +  const invitee = await this.prisma.user.findUnique({
    +    where: { email: inviteeEmail }
    +  })
    +
    +  if (!invitee) {
    +    throw new NotFoundException(`User with email ${inviteeEmail} not found`)
    +  }
    +
    +  const membership = await this.prisma.workspaceMember.findUnique({
    +    where: {
    +      workspaceId_userId: {
    +        workspaceId: workspace.id,
    +        userId: invitee.id
    +      }
    +    }
    +  })
    +
    +  if (!membership || membership.invitationAccepted) {
    +    throw new BadRequestException(`No pending invitation found for ${inviteeEmail}`)
    +  }
    +
    +  await this.mailService.workspaceInvitationMailForUsers(
    +    inviteeEmail,
    +    workspace.name,
    +    `${process.env.WORKSPACE_FRONTEND_URL}/workspace/${workspace.id}/join`,
    +    user.name,
    +    true
    +  )
    +
    +  this.log.debug(`Resent invitation to ${inviteeEmail} for workspace ${workspace.name}`)
    +}
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Adding a method to resend invitations is a useful enhancement for user experience, but it is not critical to the current functionality and could be implemented later as an additional feature.

    6
    Possible issue
    Add a check for workspace existence before updating

    Consider adding a check for the existence of the workspace before attempting to
    update it. This can prevent potential errors if the workspace doesn't exist.

    apps/api/src/workspace/service/workspace.service.ts [94-110]

    +const existingWorkspace = await this.prisma.workspace.findUnique({
    +  where: { id: workspace.id }
    +});
    +if (!existingWorkspace) {
    +  throw new NotFoundException(`Workspace with ID ${workspace.id} not found`);
    +}
     const updatedWorkspace = await this.prisma.workspace.update({
       where: {
         id: workspace.id
       },
       data: {
         name: dto.name,
         slug: dto.name
           ? await generateEntitySlug(dto.name, 'WORKSPACE', this.prisma)
           : undefined,
         description: dto.description,
         lastUpdatedBy: {
           connect: {
             id: user.id
           }
         }
       }
     })
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a check for workspace existence before updating is a good practice to prevent errors and ensure data integrity. This suggestion addresses a potential issue, making it valuable.

    8
    Security
    Add input validation for the update workspace method

    Consider adding input validation for the dto parameter in the updateWorkspace
    method. This can help prevent potential issues with invalid or unexpected input.

    apps/api/src/workspace/service/workspace.service.ts [71-75]

     async updateWorkspace(
       user: User,
       workspaceSlug: Workspace['slug'],
       dto: UpdateWorkspace
     ) {
    +  if (!dto || (typeof dto.name !== 'undefined' && dto.name.trim() === '')) {
    +    throw new BadRequestException('Invalid workspace update data');
    +  }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Input validation is essential for preventing invalid data from causing errors or security issues. This suggestion enhances the robustness of the code, making it an important improvement.

    8
    Implement rate limiting for the invitation system

    Consider implementing rate limiting for the inviteUsersToWorkspace method to prevent
    potential abuse of the invitation system.

    apps/api/src/workspace-membership/service/workspace-membership.service.ts [186-190]

    +@RateLimit({ points: 10, duration: 60 }) // Example rate limit: 10 invitations per minute
     async inviteUsersToWorkspace(
       user: User,
       workspaceSlug: Workspace['slug'],
       members: CreateWorkspaceMember[]
     ): Promise<void> {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Implementing rate limiting can prevent abuse and enhance security, but the suggestion lacks details on how to integrate it with the existing system, which limits its immediate applicability.

    7
    Input validation
    Add input validation for pagination and sorting parameters

    Consider adding input validation for the page, limit, sort, order, and search
    parameters in the getAllMembersOfWorkspace method to ensure they are within
    acceptable ranges or formats.

    apps/api/src/workspace-membership/service/workspace-membership.service.ts [436-444]

     async getAllMembersOfWorkspace(
       user: User,
       workspaceSlug: Workspace['slug'],
       page: number,
       limit: number,
       sort: string,
       order: string,
       search: string
     ) {
    +  if (page < 0) throw new BadRequestException('Page must be non-negative')
    +  if (limit < 1 || limit > 100) throw new BadRequestException('Limit must be between 1 and 100')
    +  if (!['asc', 'desc'].includes(order.toLowerCase())) throw new BadRequestException('Order must be "asc" or "desc"')
    +  // Add more validations as needed
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding input validation for pagination and sorting parameters is a good practice to prevent invalid inputs and potential errors, enhancing the robustness of the method.

    8
    Error handling
    Improve error handling by using a more specific error type

    Consider using a more specific error type instead of InternalServerErrorException in
    the transferOwnership method. This could provide more meaningful error information
    to the client.

    apps/api/src/workspace-membership/service/workspace-membership.service.ts [149-153]

     } catch (e) {
       this.log.error('Error in transaction', e)
    -  throw new InternalServerErrorException('Error in transaction')
    +  throw new Error('Failed to transfer ownership: ' + e.message)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: The suggestion to use a more specific error type could improve error clarity, but replacing InternalServerErrorException with a generic Error does not provide a more specific error type. The suggestion does not enhance the specificity of the error handling.

    4

    @rajdip-b rajdip-b changed the title refactor(API): Refactored workspace module; created new workspace-membership module refactor(API): Refactor workspace-membership into a separate module Sep 11, 2024
    @rajdip-b rajdip-b merged commit 574170f into keyshade-xyz:develop Sep 11, 2024
    5 of 6 checks passed
    @unamdev0 unamdev0 deleted the feature/refactor-workspace-module branch September 11, 2024 15:07
    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

    🎉 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
    2 participants