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-2098] Update qr code styling #291

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

enjinabner
Copy link
Contributor

@enjinabner enjinabner commented Nov 26, 2024

PR Type

enhancement


Description

  • Added configuration options for QR code image URL and size in enjin-platform.php.
  • Enhanced QR code generation in QrController by merging an image into the QR code.
  • Set error correction level to 'Q' and customized the eye color of the QR code.

Changes walkthrough 📝

Relevant files
Configuration changes
enjin-platform.php
Add QR code image URL and size configuration                         

config/enjin-platform.php

  • Added configuration for QR code image URL.
  • Introduced image_size configuration for QR code.
  • +2/-0     
    Enhancement
    QrController.php
    Enhance QR code generation with image and styling               

    src/Http/Controllers/QrController.php

  • Enhanced QR code generation with image merging.
  • Added error correction level 'Q' to QR code.
  • Customized eye color for QR code.
  • +8/-1     

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

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Configuration Defaults
    The default image path uses a relative directory which might not resolve correctly in all deployment environments. Consider using a full path or a more robust method to define the default image path.

    Hardcoded Values
    The eye color values for the QR code are hardcoded, which might not be flexible for different branding requirements. Consider making these values configurable.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Security
    Ensure data integrity by validating the $data variable before generating the QR code

    Validate the $data variable before generating the QR code to ensure it contains
    valid, non-malicious content.

    src/Http/Controllers/QrController.php [30-36]

    +// Validate $data before use
    +if (!isValidData($data)) {
    +    throw new InvalidArgumentException('Invalid data provided');
    +}
     $qrCode = QrCode::format($format)->size($size)
         ->errorCorrection('Q')
         ->merge(config('enjin-platform.qr.image'), config('enjin-platform.qr.image_size'), true)
         ->eyeColor(0, 121, 102, 221, 121, 102, 221)
         ->eyeColor(1, 121, 102, 221, 121, 102, 221)
         ->eyeColor(2, 121, 102, 221, 121, 102, 221)
         ->generate($data);
    Suggestion importance[1-10]: 8

    Why: Adding validation for the $data variable before generating the QR code is crucial for security and data integrity, preventing potential misuse or errors during QR code generation.

    8
    Possible issue
    Verify the directory path resolution in the configuration to prevent potential path errors

    Ensure that the DIR usage in QR_CODE_IMAGE_URL is correctly resolving to the
    intended directory path.

    config/enjin-platform.php [233]

    -'image' => env('QR_CODE_IMAGE_URL', __DIR__ . '/../resources/images/qr-code-enjin-logo.jpg'),
    +'image' => env('QR_CODE_IMAGE_URL', realpath(__DIR__ . '/../resources/images/qr-code-enjin-logo.jpg')),
    Suggestion importance[1-10]: 4

    Why: Using realpath to resolve the directory path in the configuration file is a good practice to ensure the path is correct, although it's not a critical issue. This suggestion enhances reliability in path resolution.

    4

    @enjinabner enjinabner marked this pull request as ready for review November 28, 2024 01:48
    @@ -230,6 +230,8 @@
    'adapter' => PlatformQrAdapter::class,
    'size' => env('QR_CODE_SIZE', 512),
    'format' => env('QR_CODE_FORMAT', 'png'),
    'image' => env('QR_CODE_IMAGE_URL', __DIR__ . '/../resources/images/enjin-logo.jpg'),
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Please could we make this optional, so self-hosted users can choose not to have an image?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We need to add this in our .env yeah?

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Yes for the image and size, however would we need to tweak the QRCode format method in the GraphQlController class to skip the merge if one isn't set?

    $qrCode = QrCode::format($format)
    ->size($size)
    ->errorCorrection('Q')
    ->merge(config('enjin-platform.qr.image'), config('enjin-platform.qr.image_size'), true)
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Might need to bypass this if a QR Image hasn't been set in the .env, or at least test it works without an image :)

    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.

    3 participants