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

fix(cli): Workspace membership API client payload fixed (#614) #648

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

csehatt741
Copy link
Contributor

@csehatt741 csehatt741 commented Jan 20, 2025

User description

Description

Fixed the WorkspaceMembershipController API client to include only the members in the payload of the inviteUsers method.

Fixes #614

Screenshots of relevant screens

image

image

image

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

Bug fix


Description

  • Fixed the payload for the inviteUsers method in WorkspaceMembershipController.

  • Ensured only members are included in the API request payload.

  • Addressed the issue causing API call failures during user invitations.


Changes walkthrough 📝

Relevant files
Bug fix
workspace-membership.ts
Corrected payload structure for `inviteUsers` method         

packages/api-client/src/controllers/workspace-membership.ts

  • Updated the inviteUsers method to send only members in the payload.
  • Fixed API call issues related to workspace user invitations.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    614 - PR Code Verified

    Compliant requirements:

    • Fix the API call failure when using keyshade workspace invite <workspace-slug> -e <user-email> -r <role> command

    Requires further human verification:

    • Verify that the CLI command works end-to-end with the new API payload structure
    • Test with different roles and multiple email addresses
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Payload Validation

    Verify that the request.members object contains all required fields for the API endpoint and matches the expected schema

    request.members,

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Structure API request body properly

    Ensure the request body is properly structured by spreading the members array into
    an object, as sending raw array data might cause issues with some API frameworks.

    packages/api-client/src/controllers/workspace-membership.ts [51-55]

     const response = await this.apiClient.post(
       `/api/workspace-membership/${request.workspaceSlug}/invite-users`,
    -  request.members,
    +  { members: request.members },
       headers
     )
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential API compatibility issue by properly structuring the request body as an object with a 'members' key, which is a more robust approach than sending a raw array and could prevent request parsing errors.

    8

    @rajdip-b
    Copy link
    Member

    I tried a lot of ways, but none of them succeeded.

    It turns out that our api-client package has the error, and hence it's not able to make the request. If you run pnpm test:api-client, you would encounter this failure:
    image

    I think this bug has been there for a really long time, just that we didn't notice this before.

    P.S. could you please delete the invitations on your local database and try it again using the CLI ONLY? Just reiterating this because the API is fine, its the package that's at fault in here

    @csehatt741
    Copy link
    Contributor Author

    Definitely, the issue is with the WorkspaceMembershipController API client in the api-client package. This PR fixes that issue.

    I also created the invitation via the CLI.

    Don't forget to rebuild the api-client package before testing this fix!

    @csehatt741
    Copy link
    Contributor Author

    As for running pnpm test:api-client, how to execute api-client tests properly?

    It requires the api, so I set up dependencies by executing docker compose up -d and then started the api by executing pnpm dev:api. When I start the tests, it immediately invokes docker compose down, which breaks the database connection. Though, it then invokes docker compose -f ../../docker-compose-test.yml up -d, but the API never gets back working again.

    Could you give me guidance, please?

    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.

    CLI: Can't invite users
    2 participants