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: Remove project IDs from workspace role export data and update tests #448

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

Meeran-Tofiq
Copy link
Contributor

@Meeran-Tofiq Meeran-Tofiq commented Sep 18, 2024

User description

Description

Give a summary of the change that you have made

Changed the exportData function in the workspace controller to no longer return project IDs. I updated the e2e test to now check for the important fields that are supposed to be returned by exportData.

Fixes #425

Dependencies

Mention any dependencies/packages used
None

Future Improvements

Mention any improvements to be done in future related to any file/feature
The test currently doesn't test if there is extra fields that is not needed/not supposed to be there, only that the important fields are there. If necessary, I will change the test to be more precise.

Mentions

Mention and tag the people

Screenshots of relevant screens

Add screenshots of relevant screens
Screenshot from 2024-09-18 19-29-22
Screenshot from 2024-09-18 19-30-07

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

enhancement, tests


Description

  • Removed project IDs from the exportData function in the workspace service to streamline the data returned.
  • Updated end-to-end tests to verify that workspaceRoles contain the expected fields such as name, description, hasAdminAuthority, authorities, and colorCode.

Changes walkthrough 📝

Relevant files
Enhancement
workspace.service.ts
Remove project ID selection from exportData function         

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

  • Removed the selection of project IDs from the exportData function.
  • Kept other fields like name, description, and authorities.
  • +1/-6     
    Tests
    workspace.e2e.spec.ts
    Update e2e test to verify workspaceRole fields                     

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

  • Updated test to check for specific fields in workspaceRoles.
  • Ensured workspaceRoles contains expected properties.
  • +7/-0     

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

    Signed-off-by: Meeran Tofiq <101665499+Meeran-Tofiq@users.noreply.github.com>
    Mdified an existing test to now check for the fiel
    -ds of the workspaceRole, when using the exportDat
    -a api endpoint on a workspace.
    
    Signed-off-by: Meeran Tofiq <101665499+Meeran-Tofiq@users.noreply.github.com>
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Data Consistency
    Removal of project IDs from workspace role export data might affect data consistency or cause issues in other parts of the application that may depend on this information.

    Test Precision
    The updated test checks for the presence of expected fields but doesn't verify the absence of unnecessary fields or the correctness of the data.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a type assertion or interface for the workspace role structure in the test

    Consider using a type assertion or interface to define the structure of
    exampleWorkspaceRole. This will provide better type safety and make the expected
    structure more explicit.

    apps/api/src/workspace/workspace.e2e.spec.ts [533-538]

    -const exampleWorkspaceRole = body.workspaceRoles[0]
    +interface WorkspaceRole {
    +  name: string;
    +  description: string;
    +  hasAdminAuthority: boolean;
    +  authorities: string[];
    +  colorCode: string;
    +}
    +const exampleWorkspaceRole = body.workspaceRoles[0] as WorkspaceRole;
     expect(exampleWorkspaceRole).toHaveProperty('name')
     expect(exampleWorkspaceRole).toHaveProperty('description')
     expect(exampleWorkspaceRole).toHaveProperty('hasAdminAuthority')
     expect(exampleWorkspaceRole).toHaveProperty('authorities')
     expect(exampleWorkspaceRole).toHaveProperty('colorCode')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a type assertion or interface enhances type safety and clarity, making the expected structure explicit, which is a best practice in TypeScript.

    8
    Maintainability
    Add a comment explaining the removal of project IDs from the select statement

    Consider adding a comment explaining why projects were removed from the select
    statement. This will help future developers understand the reasoning behind this
    change.

    apps/api/src/workspace/service/workspace.service.ts [275-281]

     select: {
       name: true,
       description: true,
       colorCode: true,
       hasAdminAuthority: true,
       authorities: true
     }
    +// Project IDs are no longer included in workspace role export data
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a comment to explain the removal of project IDs would improve code maintainability and help future developers understand the change rationale, though it is not critical for functionality.

    7
    Enhancement
    Assert the types of workspace role properties instead of just checking for their presence

    Instead of checking for the presence of properties, consider asserting the types of
    these properties. This will provide stronger guarantees about the structure of the
    exampleWorkspaceRole object.

    apps/api/src/workspace/workspace.e2e.spec.ts [534-538]

    -expect(exampleWorkspaceRole).toHaveProperty('name')
    -expect(exampleWorkspaceRole).toHaveProperty('description')
    -expect(exampleWorkspaceRole).toHaveProperty('hasAdminAuthority')
    -expect(exampleWorkspaceRole).toHaveProperty('authorities')
    -expect(exampleWorkspaceRole).toHaveProperty('colorCode')
    +expect(typeof exampleWorkspaceRole.name).toBe('string')
    +expect(typeof exampleWorkspaceRole.description).toBe('string')
    +expect(typeof exampleWorkspaceRole.hasAdminAuthority).toBe('boolean')
    +expect(Array.isArray(exampleWorkspaceRole.authorities)).toBe(true)
    +expect(typeof exampleWorkspaceRole.colorCode).toBe('string')
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Asserting the types of properties provides stronger guarantees about the object's structure, improving test robustness, though it is a minor enhancement.

    7

    💡 Need additional feedback ? start a PR chat

    @rajdip-b rajdip-b merged commit 8fdb328 into keyshade-xyz:develop Sep 18, 2024
    14 checks passed
    Kiranchaudhary537 pushed a commit to Kiranchaudhary537/keyshade that referenced this pull request Oct 13, 2024
    …ests (keyshade-xyz#448)
    
    Signed-off-by: Meeran Tofiq <101665499+Meeran-Tofiq@users.noreply.github.com>
    rajdip-b pushed a commit that referenced this pull request Oct 24, 2024
    ## [2.6.0](v2.5.0...v2.6.0) (2024-10-24)
    
    ### 🚀 Features
    
    * **api:**  Add icon and remove description field from workspace ([#435](#435)) ([a99c0db](a99c0db))
    * **api-client:** Added workspace-membership and related tests ([#452](#452)) ([6a1c091](6a1c091))
    * **api-client:** Create controller for User module ([#484](#484)) ([f9d8e83](f9d8e83))
    * **api:** Add prod env schema in env file ([#436](#436)) ([21c3004](21c3004))
    * **api:** Add resend otp implementation ([#445](#445)) ([4dc6aa1](4dc6aa1))
    * **api:** Fetch total count of environments, [secure]s and variables in project ([#434](#434)) ([0c9e50a](0c9e50a))
    * **api:** Replace `projectId` with `name` and `slug` in workspace-role response.  ([#432](#432)) ([af06071](af06071))
    * **cli:** Add functionality to operate on Secrets ([#504](#504)) ([1b4bf2f](1b4bf2f))
    * **cli:** Add project command ([#451](#451)) ([70448e1](70448e1))
    * **cli:** Add workspace operations ([#441](#441)) ([ed38d22](ed38d22))
    * **cli:** implement commands to get, list, update, and delete, workspace roles ([#469](#469)) ([957ea8d](957ea8d))
    * **cli:** Implemented pagination support ([#453](#453)) ([feb1806](feb1806))
    * **cli:** Secret scan ([#438](#438)) ([85cb8ab](85cb8ab))
    * **cli:** Update environment command outputs ([f4af874](f4af874))
    * **platform:** Clearing email field after waitlisting the user email ([#481](#481)) ([256d659](256d659))
    * Remove project IDs from workspace role export data and update tests ([#448](#448)) ([8fdb328](8fdb328))
    * **web:** Configured extra check for waitlisted users already in the list and created toast message for them ([#492](#492)) ([2ddd0ef](2ddd0ef))
    * **web:** show the toast only when email add successfully ([#490](#490)) ([783c411](783c411))
    
    ### 🐛 Bug Fixes
    
    * **api,api-client:** Add environmentSlug in multiple places across the variable module ([#468](#468)) ([d970aff](d970aff))
    * **api:** Replace the id with slug in the global-search service ([#455](#455)) ([74804b1](74804b1))
    * **platform:** Fixed duplicate Google Logo UI fix  ([#450](#450)) ([fb0d6f7](fb0d6f7))
    * resolve footer website name cut-off or overlap issue ([#444](#444)) ([fe03ba2](fe03ba2))
    * **web:** Horizontal Scrolling issue on the website ([#440](#440)) ([655177b](655177b))
    
    ### 📚 Documentation
    
    * Add documentation for environment in CLI ([#462](#462)) ([dad7394](dad7394))
    * Add documentation for project in CLI ([#466](#466)) ([341fb32](341fb32))
    * Add documentation for scan in CLI ([#461](#461)) ([72281e6](72281e6))
    * Add documentation for workspace command ([#464](#464)) ([4aad8a2](4aad8a2))
    * Add instructions for resetting the local Prisma database ([#495](#495)) ([#501](#501)) ([b07ea17](b07ea17))
    * Added docker support documentation ([#465](#465)) ([bc04be4](bc04be4))
    * Added documentation for running the platform ([#473](#473)) ([8b8386b](8b8386b))
    * Added missing mappings to pages ([5de9fd8](5de9fd8))
    * Fix Documentation Hyperlink and update expired Discord invite link ([#496](#496)) ([5a10e39](5a10e39))
    * Updated CLI docs ([#460](#460)) ([c7e0f13](c7e0f13))
    
    ### 🔧 Miscellaneous Chores
    
    * Add more logging to Sentry init ([#470](#470)) ([de4925d](de4925d))
    * **api:** Optimise API docker image size ([#360](#360)) ([ea40dc1](ea40dc1))
    * **api:** Updated lockfile ([a968e78](a968e78))
    * **CI:** Add [secure] scan validation ([f441262](f441262))
    * **cli:** Update controller invocation in environment commands ([#477](#477)) ([596bd1a](596bd1a))
    * Minor changes to variables ([fe01ca6](fe01ca6))
    * **[secure]-scan:** Failing lint issues ([#507](#507)) ([48f45df](48f45df))
    * **[secure]-scan:** Formatted files ([5884833](5884833))
    * Update .env.example ([70ad4f7](70ad4f7))
    * Updated scripts ([9eb76a7](9eb76a7))
    * **web:** email validation ([#487](#487)) ([e8e737a](e8e737a))
    @rajdip-b
    Copy link
    Member

    🎉 This PR is included in version 2.6.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: Remove projectIds from Export Workspace Data response.
    2 participants