feat(API): Included default workspace details in getSelf function

unamdev0 commented 3 weeks ago

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

codiumai-pr-agent-free[bot] commented 3 weeks ago

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.

codiumai-pr-agent-free[bot] commented 3 weeks ago

PR Code Suggestions ✨

Category	Suggestion	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]](https://github.com/keyshade-xyz/keyshade/pull/414/files#diff-6ca55e419496e622d41d22930ddd6d7e7d9f907148c36afcae40ef6320dab922R34-R49) ```diff 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: ```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]](https://github.com/keyshade-xyz/keyshade/pull/414/files#diff-6ca55e419496e622d41d22930ddd6d7e7d9f907148c36afcae40ef6320dab922R33-R49) ```diff -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
Enhancement	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]](https://github.com/keyshade-xyz/keyshade/pull/414/files#diff-e033f59a1c8df8834aa27fc532b74514ecc01b9a1c0eef7c56bdff989f127094R106-R110) ```diff 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: ```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]](https://github.com/keyshade-xyz/keyshade/pull/414/files#diff-e033f59a1c8df8834aa27fc532b74514ecc01b9a1c0eef7c56bdff989f127094R112-R128) ```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 +}) ``` - [ ] 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

rajdip-b commented 3 weeks ago

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

unamdev0 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

Creating a seperate PR for it

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/keyshade-xyz/keyshade/pull/414/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz) | Coverage Δ | | |---|---|---| | [api-e2e-tests](https://app.codecov.io/gh/keyshade-xyz/keyshade/pull/414/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz) | `87.04% <100.00%> (-4.67%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=keyshade-xyz#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rajdip-b commented 2 weeks 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:

keyshade-xyz / keyshade