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

[PLA-2094] Changes start script to add notification email #55

Merged
merged 2 commits into from
Jan 8, 2025

Conversation

leonardocustodio
Copy link
Contributor

@leonardocustodio leonardocustodio commented Jan 7, 2025

PR Type

Enhancement, Configuration changes


Description

  • Added functionality to prompt users for notification email.

  • Updated .env file to include UPGRADE_NOTIFICATION_EMAIL variable.

  • Enhanced PHP dependency requirements in composer.json.

  • Improved user input validation for email addresses.


Changes walkthrough 📝

Relevant files
Enhancement
start.bat
Add email input handling in batch script                                 

start.bat

  • Added functions to handle notification email input.
  • Validated email input and updated .env file accordingly.
  • Integrated notification email check into the startup process.
  • +36/-0   
    start.sh
    Add email input handling in shell script                                 

    start.sh

  • Added shell functions for notification email input.
  • Validated email input and updated .env file accordingly.
  • Integrated notification email check into the startup process.
  • +38/-0   
    Configuration changes
    .env
    Add email placeholder to `.env` file                                         

    configs/core/.env

    • Added UPGRADE_NOTIFICATION_EMAIL variable placeholder.
    +1/-0     
    composer.json
    Update PHP and Laravel dependencies                                           

    configs/core/composer.json

  • Updated PHP version requirements to include ^8.4.
  • Adjusted laravel/reverb dependency to stable ^1.0.
  • +2/-2     

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

    Copy link

    github-actions bot commented Jan 7, 2025

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Input Validation

    The email validation logic in the read_email function may not handle all edge cases for invalid email formats. Ensure that the validation is robust and accounts for a wide range of invalid inputs.

    echo Please input the email address: (e.g. user@example.com)
    set /p NOTIFICATION_EMAIL=
    :: Check if the email address is empty
    if "%NOTIFICATION_EMAIL%"=="" (
        goto :EOF
    )
    :: Check if the email address is valid
    if not "%NOTIFICATION_EMAIL%"=="%NOTIFICATION_EMAIL: =%" (
        echo The email address you provided is not valid. Please try again.
        call :read_email
    Input Validation

    The email validation regex in the read_email function may not cover all edge cases for invalid email formats. Consider testing with various invalid email inputs to confirm robustness.

    echo "Please input the email address: (e.g. user@example.com)"
    read NOTIFICATION_EMAIL
    
    # Check if the email address is empty
    if [ -z "$NOTIFICATION_EMAIL" ]; then
        return
    fi
    
    # Check if the email address is valid
    if ! [[ $NOTIFICATION_EMAIL =~ ^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$ ]]; then
        echo "The email address you provided is not valid. Please try again."
        read_email

    Copy link

    github-actions bot commented Jan 7, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Ensure the .env file is updated correctly even if the target variable does not exist

    Ensure that the sed command for updating the .env file handles cases where the
    UPGRADE_NOTIFICATION_EMAIL variable does not already exist in the file.

    start.sh [79]

    -sed -i "s/^UPGRADE_NOTIFICATION_EMAIL=/UPGRADE_NOTIFICATION_EMAIL=\"$NOTIFICATION_EMAIL\"/g" configs/core/.env
    +if grep -q "^UPGRADE_NOTIFICATION_EMAIL=" configs/core/.env; then
    +    sed -i "s/^UPGRADE_NOTIFICATION_EMAIL=/UPGRADE_NOTIFICATION_EMAIL=\"$NOTIFICATION_EMAIL\"/g" configs/core/.env
    +else
    +    echo "UPGRADE_NOTIFICATION_EMAIL=\"$NOTIFICATION_EMAIL\"" >> configs/core/.env
    +fi
    Suggestion importance[1-10]: 10

    Why: The suggestion ensures that the UPGRADE_NOTIFICATION_EMAIL variable is added to the .env file if it does not already exist. This is a critical improvement as it prevents potential errors or inconsistencies when updating the file, ensuring the script functions correctly in all scenarios.

    10
    Prevent infinite recursion in email validation by limiting retry attempts

    Add a mechanism to prevent infinite recursion in the read_email function if the user
    repeatedly inputs invalid email addresses.

    start.sh [58-60]

    -if ! [[ $NOTIFICATION_EMAIL =~ ^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$ ]]; then
    +local retry_count=0
    +while ! [[ $NOTIFICATION_EMAIL =~ ^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$ ]]; do
         echo "The email address you provided is not valid. Please try again."
    -    read_email
    -fi
    +    ((retry_count++))
    +    if [ $retry_count -ge 3 ]; then
    +        echo "Too many invalid attempts. Exiting."
    +        return
    +    fi
    +    read NOTIFICATION_EMAIL
    +done
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical issue by preventing infinite recursion in the read_email function. By introducing a retry limit, it ensures that the script does not hang indefinitely if the user repeatedly inputs invalid email addresses, significantly improving the script's reliability.

    9
    Possible issue
    Improve email validation logic to ensure robust handling of invalid email formats

    Ensure that the email validation logic correctly handles invalid email formats, as
    the current check using string replacement may not robustly validate all invalid
    cases.

    start.bat [34-37]

    -if not "%NOTIFICATION_EMAIL%"=="%NOTIFICATION_EMAIL: =%" (
    -    echo The email address you provided is not valid. Please try again.
    -    call :read_email
    -)
    +powershell -Command "if (-not ([regex]::IsMatch('%NOTIFICATION_EMAIL%', '^[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}$'))) { echo The email address you provided is not valid. Please try again. & exit /b 1 }"
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the robustness of email validation by replacing the simplistic string replacement logic with a regex-based validation using PowerShell. This ensures that invalid email formats are handled more accurately, addressing a potential issue in the current implementation.

    8

    @leonardocustodio leonardocustodio merged commit e51427b into master Jan 8, 2025
    1 check passed
    @leonardocustodio leonardocustodio deleted the PLA-2094 branch January 8, 2025 03:36
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants