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): Included default workspace details in getSelf function #414

Merged

Conversation

unamdev0
Copy link
Contributor

@unamdev0 unamdev0 commented Sep 7, 2024

User description

Description

Added default workspace details in getSelf function for a user
Fixes #365

Further change

  • Need to update Postman example for this API and include defaultWorkspace details

Sample response after updating should be like this

    "id": "cm0s8xc1y000013zv8gextk2n",
    "email": "regularuser@gmail.com",
    "name": null,
    "profilePictureUrl": null,
    "isActive": true,
    "isOnboardingFinished": false,
    "isAdmin": false,
    "authProvider": "EMAIL_OTP",
    "defaultWorkspace": {
        "id": "713d0227-eb11-4228-961a-ff7d5d707471",
        "name": "My Workspace",
        "description": null,
        "isFreeTier": true,
        "createdAt": "2024-09-07T14:34:15.256Z",
        "updatedAt": "2024-09-07T14:34:15.256Z",
        "lastUpdatedById": null
    }
}

PR Type

enhancement, tests


Description

  • Enhanced the getSelf function to include default workspace details for non-admin users.
  • Updated end-to-end tests to verify the inclusion of defaultWorkspace in the user response.
  • Added assertions to ensure the correct properties of the default workspace are returned.

Changes walkthrough 📝

Relevant files
Enhancement
user.service.ts
Include default workspace details in getSelf function       

apps/api/src/user/service/user.service.ts

  • Added logic to fetch default workspace details for non-admin users.
  • Modified getSelf function to include defaultWorkspace in the response.

  • +20/-1   
    Tests
    user.e2e.spec.ts
    Update tests for getSelf function with workspace details 

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

  • Updated test to check for defaultWorkspace in user response.
  • Added assertions for default workspace properties for regular users.
  • +30/-2   

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Performance Concern
    The getSelf function performs a database query for non-admin users, which may impact performance for frequent API calls.

    Copy link
    Contributor

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

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for cases where the default workspace is not found for a non-admin user

    Consider adding error handling for the case when the default workspace is not found
    for a non-admin user. This could help prevent unexpected behavior if the database
    query doesn't return a result.

    apps/api/src/user/service/user.service.ts [34-49]

     if (!user.isAdmin) {
       defaultWorkspace = await this.prisma.workspace.findFirst({
         where: {
           ownerId: user.id,
           isDefault: true
         },
         select: {
           id: true,
           name: true,
           description: true,
           isFreeTier: true,
           createdAt: true,
           updatedAt: true,
           lastUpdatedById: true
         }
       })
    +  if (!defaultWorkspace) {
    +    this.log.warn(`Default workspace not found for user ${user.id}`);
    +  }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for when the default workspace is not found is important for preventing unexpected behavior and improving the robustness of the application.

    8
    Enhancement
    ✅ Improve type safety by using a more specific type for the defaultWorkspace variable
    Suggestion Impact:The type of defaultWorkspace was changed from null to Workspace | null, improving type safety.

    code diff:

    -    let defaultWorkspace = null
    -    if (!user.isAdmin) {
    -      defaultWorkspace = await this.prisma.workspace.findFirst({
    +    const defaultWorkspace: Workspace | null =
    +      await this.prisma.workspace.findFirst({

    Consider using a more specific type for the defaultWorkspace variable instead of
    null. You could use Workspace | null or create a custom type that matches the
    selected fields.

    apps/api/src/user/service/user.service.ts [33-49]

    -let defaultWorkspace = null
    +interface DefaultWorkspace {
    +  id: string;
    +  name: string;
    +  description: string;
    +  isFreeTier: boolean;
    +  createdAt: Date;
    +  updatedAt: Date;
    +  lastUpdatedById: string;
    +}
    +
    +let defaultWorkspace: DefaultWorkspace | null = null;
     if (!user.isAdmin) {
       defaultWorkspace = await this.prisma.workspace.findFirst({
         where: {
           ownerId: user.id,
           isDefault: true
         },
         select: {
           id: true,
           name: true,
           description: true,
           isFreeTier: true,
           createdAt: true,
           updatedAt: true,
           lastUpdatedById: true
         }
    -  })
    +  });
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using a more specific type for defaultWorkspace improves type safety and code readability, but it is not a critical issue.

    7
    Improve test specificity by using more detailed assertions for the defaultWorkspace object

    Consider using more specific assertions for the defaultWorkspace object properties
    instead of expect.any(Object). This would provide a more robust test that ensures
    the correct structure of the returned data.

    apps/api/src/user/user.e2e.spec.ts [106-110]

     expect(result.statusCode).toEqual(200)
     expect(JSON.parse(result.body)).toEqual({
       ...regularUser,
    -  defaultWorkspace: expect.any(Object)
    +  defaultWorkspace: expect.objectContaining({
    +    id: expect.any(String),
    +    name: expect.any(String),
    +    description: expect.any(String),
    +    isFreeTier: expect.any(Boolean),
    +    createdAt: expect.any(String),
    +    updatedAt: expect.any(String),
    +    lastUpdatedById: expect.any(String)
    +  })
     })
     
    Suggestion importance[1-10]: 6

    Why: Using more specific assertions enhances test robustness and ensures the correct structure of the returned data, but it is not a critical improvement.

    6
    Best practice
    ✅ Simplify test assertions by using toMatchObject for comparing defaultWorkspace properties
    Suggestion Impact:The suggestion to use toMatchObject was implemented, simplifying the test assertions for defaultWorkspace properties.

    code diff:

    -    expect(result.json().defaultWorkspace.id).toEqual(workspace.id)
    -    expect(result.json().defaultWorkspace.name).toEqual(workspace.name)
    -    expect(result.json().defaultWorkspace.description).toEqual(
    -      workspace.description
    -    )
    -    expect(result.json().defaultWorkspace.isFreeTier).toEqual(
    -      workspace.isFreeTier
    -    )
    -    expect(result.json().defaultWorkspace.createdAt).toEqual(
    -      workspace.createdAt.toISOString()
    -    )
    -    expect(result.json().defaultWorkspace.updatedAt).toEqual(
    -      workspace.updatedAt.toISOString()
    -    )
    -    expect(result.json().defaultWorkspace.lastUpdatedById).toEqual(
    -      workspace.lastUpdatedById
    -    )
    +    expect(result.json().defaultWorkspace).toMatchObject({
    +      id: workspace.id,
    +      name: workspace.name,
    +      description: workspace.description,
    +      isFreeTier: workspace.isFreeTier,
    +      createdAt: workspace.createdAt.toISOString(),
    +      updatedAt: workspace.updatedAt.toISOString(),
    +      lastUpdatedById: workspace.lastUpdatedById
    +    })

    Consider using toMatchObject instead of multiple individual assertions for the
    defaultWorkspace properties. This would make the test more concise and easier to
    maintain.

    apps/api/src/user/user.e2e.spec.ts [112-128]

    -expect(result.json().defaultWorkspace.id).toEqual(workspace.id)
    -expect(result.json().defaultWorkspace.name).toEqual(workspace.name)
    -expect(result.json().defaultWorkspace.description).toEqual(
    -  workspace.description
    -)
    -expect(result.json().defaultWorkspace.isFreeTier).toEqual(
    -  workspace.isFreeTier
    -)
    -expect(result.json().defaultWorkspace.createdAt).toEqual(
    -  workspace.createdAt.toISOString()
    -)
    -expect(result.json().defaultWorkspace.updatedAt).toEqual(
    -  workspace.updatedAt.toISOString()
    -)
    -expect(result.json().defaultWorkspace.lastUpdatedById).toEqual(
    -  workspace.lastUpdatedById
    -)
    +expect(result.json().defaultWorkspace).toMatchObject({
    +  id: workspace.id,
    +  name: workspace.name,
    +  description: workspace.description,
    +  isFreeTier: workspace.isFreeTier,
    +  createdAt: workspace.createdAt.toISOString(),
    +  updatedAt: workspace.updatedAt.toISOString(),
    +  lastUpdatedById: workspace.lastUpdatedById
    +})
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using toMatchObject simplifies the test code, making it more concise and maintainable, which is a good practice but not essential.

    7

    apps/api/src/user/service/user.service.ts Outdated Show resolved Hide resolved
    apps/api/src/user/user.e2e.spec.ts Show resolved Hide resolved
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Sep 7, 2024

    Good work on the PR! Don't worry about the api, we would put that in.

    @unamdev0
    Copy link
    Contributor Author

    unamdev0 commented Sep 8, 2024

    @rajdip-b after running API tests, eslint-custom-config/package-lock.json and tsconfig/package-lock.json is created, last time had to remove them manually, do we need to add these to .gitignore?

    @rajdip-b
    Copy link
    Member

    rajdip-b commented Sep 8, 2024

    This is a strange thing that it's getting created over and over again. I feel yes, you can add them to .gitignore since we are not using them.

    @unamdev0
    Copy link
    Contributor Author

    unamdev0 commented Sep 8, 2024

    Creating a seperate PR for it

    Copy link

    codecov bot commented Sep 8, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 87.04%. Comparing base (ce50743) to head (a1dd53f).
    Report is 139 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #414      +/-   ##
    ===========================================
    - Coverage    91.71%   87.04%   -4.67%     
    ===========================================
      Files          111      112       +1     
      Lines         2510     2501       -9     
      Branches       469      369     -100     
    ===========================================
    - Hits          2302     2177     -125     
    - Misses         208      324     +116     
    Flag Coverage Δ
    api-e2e-tests 87.04% <100.00%> (-4.67%) ⬇️

    Flags with carried forward coverage won't be shown. Click here to find out more.

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    Copy link
    Member

    @rajdip-b rajdip-b left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    LGTM

    @rajdip-b rajdip-b merged commit e67bbd6 into keyshade-xyz:develop Sep 9, 2024
    4 checks passed
    @unamdev0 unamdev0 deleted the fix/get-user-api-add-default-workspace branch September 9, 2024 06:15
    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 📦🚀

    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: Include default workspace of user in getSelf function
    2 participants