search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

feat(api): Add pagination metadata to Workspace Role module #388

Closed muntaxir4 closed 2 months ago

muntaxir4 commented 2 months ago

User description

Description

Fixes #340

Future Improvements

N/A

Screenshots of relevant screens

Improved tests: image

Developer's checklist

If changes are made in the code:

Documentation Update

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
workspace-role.service.ts
Add pagination metadata to Workspace Role service               
apps/api/src/workspace-role/service/workspace-role.service.ts
  • Added pagination metadata to getWorkspaceRolesOfWorkspace method.
  • Included calculation of total count and metadata.
  • Adjusted return type to include items and metadata.
    		•  +29/-5   
    Tests
    workspace-role.e2e.spec.ts
    Update e2e tests for pagination metadata                                 
    apps/api/src/workspace-role/workspace-role.e2e.spec.ts
  • Updated e2e tests to check for pagination metadata.
  • Added assertions for metadata fields in the response.
    		•  +32/-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 2 months ago

    PR Reviewer Guide 🔍

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

    Type Conversion
    Consider verifying the type of `page` and `limit` parameters before using them in calculations and database queries to ensure they are valid integers. This can prevent potential runtime errors or unexpected behavior. Pagination Logic
    Ensure that the pagination logic correctly handles cases where `page` or `limit` might be zero or negative, as this could lead to incorrect data retrieval or errors.
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a transaction to ensure consistency between fetching items and counting them ___ **Consider using a transaction for the operations that fetch workspace roles and count
    them. This ensures that both operations see the same state of the database, which is
    crucial for consistency in pagination.** [apps/api/src/workspace-role/service/workspace-role.service.ts [315-336]](https://github.com/keyshade-xyz/keyshade/pull/388/files#diff-fa0a0e5d18d758148d81c6236ae88c9e4e117214826e1a03cc9cc0441d111320R315-R336) ```diff -const items = await this.prisma.workspaceRole.findMany({ - where: { - workspaceId, - name: { - contains: search +const [items, totalCount] = await this.prisma.$transaction([ + this.prisma.workspaceRole.findMany({ + where: { + workspaceId, + name: { + contains: search + } + }, + skip: page * limit, + take: Number(limit), + orderBy: { + [sort]: order } - }, - skip: page * limit, - take: Number(limit), - orderBy: { - [sort]: order - } -}); -const totalCount = await this.prisma.workspaceRole.count({ - where: { - workspaceId, - name: { - contains: search + }), + this.prisma.workspaceRole.count({ + where: { + workspaceId, + name: { + contains: search + } } - } -}); + }) +]); ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 9 Why: Using a transaction for fetching and counting ensures that both operations see the same state of the database, which is crucial for consistency in pagination. This is a significant improvement in terms of data integrity.
    		9
    Possible issue
    Add validation for page and limit to ensure they are non-negative ___ **Validate the page and limit parameters to ensure they are non-negative integers
    before using them in database queries to prevent potential runtime errors or
    unexpected behavior.** [apps/api/src/workspace-role/service/workspace-role.service.ts [322-323]](https://github.com/keyshade-xyz/keyshade/pull/388/files#diff-fa0a0e5d18d758148d81c6236ae88c9e4e117214826e1a03cc9cc0441d111320R322-R323) ```diff -skip: page * limit, -take: Number(limit), +skip: Math.max(0, page) * Math.max(1, limit), +take: Math.max(1, Number(limit)), ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: Validating `page` and `limit` parameters to ensure they are non-negative prevents potential runtime errors or unexpected behavior, which is important for robustness.
    		8
    Maintainability
    Refactor pagination metadata generation into a separate method ___ **Refactor the pagination metadata generation to a separate method to improve code
    reusability and maintainability.** [apps/api/src/workspace-role/service/workspace-role.service.ts [339-348]](https://github.com/keyshade-xyz/keyshade/pull/388/files#diff-fa0a0e5d18d758148d81c6236ae88c9e4e117214826e1a03cc9cc0441d111320R339-R348) ```diff -const metadata = paginate( - totalCount, - `/workspace-role/${workspaceId}/all`, - { - page: Number(page), - limit: Number(limit), - sort, - order, - search - } -); +const metadata = this.generatePaginationMetadata(totalCount, workspaceId, { page, limit, sort, order, search }); +private generatePaginationMetadata(totalCount: number, workspaceId: string, queryOptions: { page: number; limit: number; sort: string; order: string; search: string }): PaginatedMetadata { + return paginate( + totalCount, + `/workspace-role/${workspaceId}/all`, + { + page: Number(queryOptions.page), + limit: Number(queryOptions.limit), + sort: queryOptions.sort, + order: queryOptions.order, + search: queryOptions.search + } + ); +} + ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: Refactoring the pagination metadata generation into a separate method improves code reusability and maintainability. This is a good practice but not critical for functionality.
    		7
    Enhancement
    Use destructuring to enhance code clarity when extracting user ID ___ **Use destructuring for the user object to directly extract id when passing it to
    checkAuthorityOverWorkspace to enhance code clarity and reduce redundancy.** [apps/api/src/workspace-role/service/workspace-role.service.ts [309]](https://github.com/keyshade-xyz/keyshade/pull/388/files#diff-fa0a0e5d18d758148d81c6236ae88c9e4e117214826e1a03cc9cc0441d111320R309-R309) ```diff -userId: user.id, +const { id: userId } = user; +userId, ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 6 Why: Using destructuring for the `user` object to directly extract `id` enhances code clarity and reduces redundancy. This is a minor enhancement for readability.
    		6