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(platform): Check if Env. Name is left empty #646

Merged
merged 4 commits into from
Jan 20, 2025

Conversation

mrswastik-robot
Copy link
Contributor

@mrswastik-robot mrswastik-robot commented Jan 20, 2025

User description

Description

I've made two changes:

  • In the frontend (createProjectDialogue/index.tsx), I modified the createNewProject function to filter out any environments with empty names before sending the request.
  • In the API, I changed the default environment name from "Default" to "default"

Fixes #628

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

Bug fix


Description

  • Updated API to rename default environment from "Default" to "default".

  • Modified frontend to filter out environments with empty names.

  • Ensured proper handling of environment data during project creation.


Changes walkthrough 📝

Relevant files
Bug fix
project.service.ts
Rename default environment to "default" in API                     

apps/api/src/project/service/project.service.ts

  • Changed default environment name from "Default" to "default".
  • Updated slug generation to use "default" instead of "Default".
  • +2/-2     
    index.tsx
    Filter out empty environment names in frontend                     

    apps/platform/src/components/dashboard/project/createProjectDialogue/index.tsx

  • Added logic to filter out environments with empty names.
  • Updated project creation logic to handle filtered environment data.
  • +10/-2   

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    628 - PR Code Verified

    Compliant requirements:

    • Change default environment name from "Default" to "default" in API
    • In platform, ensure empty environment names are not passed in the array

    Requires further human verification:

    • Verify that empty environment names are properly filtered out in the UI when creating a project
    • Verify that default environment is created correctly with name "default"
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Edge Case

    The environment name filter only checks for empty strings after trim(). Consider handling null/undefined environment names as well.

    newProjectData.environments?.filter(
      (env) => env.name.trim() !== ''
    ) || []

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Validate minimum environment requirements

    Add validation to ensure at least one environment with a non-empty name exists
    before allowing project creation. Currently, filtering out empty environments could
    lead to a project with no environments.

    apps/platform/src/components/dashboard/project/createProjectDialogue/index.tsx [61-68]

    +const filteredEnvironments = newProjectData.environments?.filter(
    +  (env) => env.name.trim() !== ''
    +) || [];
    +if (filteredEnvironments.length === 0) {
    +  throw new Error('At least one environment with a name is required');
    +}
     const projectData = {
       ...newProjectData,
       workspaceSlug: selectedWorkspace.slug,
    -  environments:
    -    newProjectData.environments?.filter(
    -      (env) => env.name.trim() !== ''
    -    ) || []
    +  environments: filteredEnvironments
     }
    Suggestion importance[1-10]: 9

    Why: The suggestion addresses a critical validation gap that could lead to invalid project states. Ensuring at least one valid environment exists is essential for proper project functionality, as the codebase assumes environments will be present.

    9

    @rajdip-b rajdip-b merged commit 5f3fac8 into keyshade-xyz:develop Jan 20, 2025
    5 checks passed
    @mrswastik-robot mrswastik-robot deleted the env-variable branch January 20, 2025 17:56
    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.

    PLATFORM: Creating a project with no environment specified creates environments with empty ("") name
    2 participants