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(api,api-client): Add environmentSlug in multiple places across the secret module #509

Merged
merged 5 commits into from
Oct 25, 2024

Conversation

anudeeps352
Copy link
Contributor

@anudeeps352 anudeeps352 commented Oct 23, 2024

User description

Description

Add environmentSlug in multiple places across the secret module

Fixes #486

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

  • Enhanced the secret module to include environmentSlug alongside environmentId in various places.
  • Updated the secret service to select and return both id and slug for environments.
  • Modified API client types to reflect the inclusion of environment details in secret versions.
  • Added and updated tests in both the API and API client to verify the presence of environment.id and environment.slug.

Changes walkthrough 📝

Relevant files
Tests
secret.e2e.spec.ts
Add tests for environment slug and id in secret versions 

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

  • Added tests to verify environment.id and environment.slug in secret
    versions.
  • Included environment in the Prisma query for secret versions.
  • +10/-0   
    secret.spec.ts
    Add tests for environment slug and id in API client           

    packages/api-client/tests/secret.spec.ts

  • Added tests to verify environment.id and environment.slug in API
    client.
  • Initialized environment variable for test setup.
  • +5/-1     
    Enhancement
    secret.service.ts
    Include environment slug and id in secret service               

    apps/api/src/secret/service/secret.service.ts

  • Updated secret service to include environment.id and environment.slug.
  • Modified Prisma queries to select environment details.
  • +39/-3   
    secret.types.d.ts
    Update secret types to include environment slug                   

    packages/api-client/src/types/secret.types.d.ts

  • Updated Secret and UpdateSecretResponse interfaces to include
    environment object.
  • Added slug to the environment object in types.
  • +9/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    486 - Fully compliant

    Fully compliant requirements:

    • Add slug along with environmentId across multiple places in secret module
    • Return environment data in the format: { id: '', slug: '' }
    • Update specific files in the project
    • Update tests in secret.e2e.spec.ts
    • Update types in api-client package under secret.ts
    • Update tests in api-client package under secret.spec.ts
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Performance Issue
    The changes in the getSecretsByProject method now include additional data for versions. This might impact performance for large datasets. Consider pagination or limiting the number of versions returned if not already implemented.

    Code Duplication
    The environment selection logic is repeated in multiple places. Consider extracting this into a reusable function to improve maintainability.

    Type Consistency
    Ensure that the new environment object type is consistent across all interfaces where it's used. Verify if any other types need to be updated for consistency.

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Oct 23, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Reduce code duplication by extracting repeated selection objects into constants

    Consider extracting the repeated environment selection object into a constant to
    improve code maintainability and reduce duplication.

    apps/api/src/secret/service/secret.service.ts [259-265]

    -environment: {
    +const environmentSelection = {
       select: {
         id: true,
         slug: true
       }
    -},
    +};
     
    +// Then use it like this:
    +environment: environmentSelection,
    +
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Extracting the repeated environment selection object into a constant improves maintainability and reduces duplication, making the code cleaner and easier to update.

    8
    Enhancement
    Improve test readability and reduce duplication by using a custom matcher for environment properties

    Consider using a custom matcher or helper function to check for the presence and
    correctness of environment properties in secret versions. This can make the tests
    more readable and reduce duplication.

    apps/api/src/secret/secret.e2e.spec.ts [212-213]

    -expect(body.versions[0].environment.id).toBe(environment1.id)
    -expect(body.versions[0].environment.slug).toBe(environment1.slug)
    +expect(body.versions[0].environment).toMatchObject({
    +  id: environment1.id,
    +  slug: environment1.slug
    +})
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion enhances test readability and reduces duplication by using a custom matcher, which is beneficial for maintainability and clarity in the test code.

    7
    Best practice
    ✅ Improve code clarity by using more descriptive variable names
    Suggestion Impact:The suggestion to use a more descriptive variable name was implemented by removing the separate 'environmentSlug' variable and directly using 'environment.slug' throughout the code.

    code diff:

    -  let environmentSlug: string | null
       let secretSlug: string | null
    -  let environment
    +  let environment: Environment
     
       beforeAll(async () => {
         //Create the user's workspace
    @@ -60,7 +60,6 @@
         ).json()) as any
     
         environment = createEnvironmentResponse
    -    environmentSlug = environment.slug
       })
     
       afterAll(async () => {
    @@ -78,7 +77,7 @@
             note: 'Secret 1 note',
             entries: [
               {
    -            environmentSlug,
    +            environmentSlug: environment.slug,
                 value: 'Secret 1 value'
               }
             ],
    @@ -105,7 +104,7 @@
             note: 'Secret 2 note',
             entries: [
               {
    -            environmentSlug,
    +            environmentSlug: environment.slug,
                 value: 'Secret 1 value'
               }
             ],
    @@ -117,8 +116,7 @@
         expect(secret.data.name).toBe('Secret 2')
         expect(secret.data.slug).toBeDefined()
         expect(secret.data.versions.length).toBe(1)
    -    expect(secret.data.versions[0].environment.id).toBe(environment.id)
    -    expect(secret.data.versions[0].environment.slug).toBe(environmentSlug)
    +    expect(secret.data.versions[0].environment.slug).toBe(environment.slug)
         expect(secret.error).toBe(null)
     
         // Delete the secret
    @@ -155,7 +153,7 @@
             entries: [
               {
                 value: 'Updated Secret 1 value',
    -            environmentSlug
    +            environmentSlug: environment.slug
               }
             ],
             secretSlug
    @@ -173,7 +171,7 @@
             entries: [
               {
                 value: 'Secret 1 value',
    -            environmentSlug
    +            environmentSlug: environment.slug
               }
             ],
             secretSlug
    @@ -186,7 +184,7 @@
             entries: [
               {
                 value: 'Updated Secret 1 value',
    -            environmentSlug
    +            environmentSlug: environment.slug
               }
             ],
             secretSlug
    @@ -195,7 +193,7 @@
         )
     
         const rollbackSecret = await secretController.rollbackSecret(
    -      { secretSlug, environmentSlug, version: 1 },
    +      { secretSlug, environmentSlug: environment.slug, version: 1 },
           { 'x-e2e-user-email': email }
         )
     
    @@ -215,7 +213,7 @@
       it('should get all secrets of an environment', async () => {
         const secrets: any = await secretController.getAllSecretsOfEnvironment(
           {
    -        environmentSlug,
    +        environmentSlug: environment.slug,
             projectSlug
           },
           { 'x-e2e-user-email': email }
    @@ -248,7 +246,7 @@
     
       it('should be able to fetch revisions of a secret', async () => {
         const revisions = await secretController.getRevisionsOfSecret(
    -      { secretSlug, environmentSlug },
    +      { secretSlug, environmentSlug: environment.slug },

    Consider using a more descriptive variable name for the environment object to
    improve code readability and maintainability.

    packages/api-client/tests/secret.spec.ts [62-63]

    -environment = createEnvironmentResponse
    -environmentSlug = environment.slug
    +createdEnvironment = createEnvironmentResponse
    +environmentSlug = createdEnvironment.slug
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Using a more descriptive variable name enhances code readability and maintainability, although the impact is relatively minor compared to functional changes.

    6

    💡 Need additional feedback ? start a PR chat

    @anudeeps352 anudeeps352 changed the title fix(api,api-client):Add environmentSlug in multiple places across the secret module #486 fix(api,api-client): Add environmentSlug in multiple places across the secret module #486 Oct 23, 2024
    apps/api/src/secret/service/secret.service.ts Show resolved Hide resolved
    packages/api-client/tests/secret.spec.ts Outdated Show resolved Hide resolved
    packages/api-client/tests/secret.spec.ts Outdated Show resolved Hide resolved
    @rajdip-b rajdip-b changed the title fix(api,api-client): Add environmentSlug in multiple places across the secret module #486 fix(api,api-client): Add environmentSlug in multiple places across the secret module Oct 24, 2024
    Copy link

    codecov bot commented Oct 25, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 87.64%. Comparing base (ce50743) to head (b37531d).
    Report is 206 commits behind head on develop.

    Additional details and impacted files
    @@             Coverage Diff             @@
    ##           develop     #509      +/-   ##
    ===========================================
    - Coverage    91.71%   87.64%   -4.08%     
    ===========================================
      Files          111      105       -6     
      Lines         2510     2743     +233     
      Branches       469      415      -54     
    ===========================================
    + Hits          2302     2404     +102     
    - Misses         208      339     +131     
    Flag Coverage Δ
    api-e2e-tests 87.64% <ø> (-4.08%) ⬇️

    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 ee58f07 into keyshade-xyz:develop Oct 25, 2024
    7 checks passed
    @rajdip-b rajdip-b added the hacktoberfest-accepted Your PR has this = Congrats! label Oct 25, 2024
    rajdip-b pushed a commit that referenced this pull request Nov 5, 2024
    ## [2.7.0](v2.6.0...v2.7.0) (2024-11-05)
    
    ### 🚀 Features
    
    * **cli:** Add functionality to operate on Variables ([#514](#514)) ([32d93e6](32d93e6))
    * **platform:** Create ui link for resend otp ([#489](#489)) ([46eb5c5](46eb5c5))
    
    ### 🐛 Bug Fixes
    
    * **api,api-client:** Add environmentSlug in multiple places across the [secure] module ([#509](#509)) ([ee58f07](ee58f07))
    * **cli:** Removed unnecessary console log in [secure]s ([#515](#515)) ([9403cc4](9403cc4))
    
    ### 🔧 Miscellaneous Chores
    
    * Fixed lint issues ([835397a](835397a))
    * Minor housekeeping ([922bf31](922bf31))
    * Update eslint ([c583718](c583718))
    * Update eslint ([7c0c596](7c0c596))
    * Update pnpx commands to pnpm dlx ([#511](#511)) ([534a231](534a231))
    @rajdip-b
    Copy link
    Member

    rajdip-b commented Nov 5, 2024

    🎉 This PR is included in version 2.7.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, API-CLIENT: Add environmentSlug in multiple places across the secret module
    2 participants