Conversation
WalkthroughThe QrCodeable trait’s generateQrCode now composes a new PNG by layering a resized external logo, the QR image, and caption text, then saves the composite. A new static method handleCampRedirect($id) determines redirect target based on camp end date (+3 days) and performs a redirect; imports for EckEntity were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Svc as Caller Service
participant Q as QrCodeable::generateQrCode
participant GD as GD Library
participant IO as saveQrCode
Svc->>Q: generateQrCode(data, saveOptions)
Q->>GD: Decode QR PNG to image
Q->>GD: Load external logo (URL)
Q->>GD: Resize logo (scale ≈ 0.4 of QR)
Q->>GD: Create canvas, compose logo+QR
Q->>GD: Draw caption text
Q->>IO: saveQrCode(finalImage, saveOptions)
IO-->>Q: Success/Exception
Q-->>Svc: TRUE on success / FALSE on error
note over Q,GD: Errors logged via try/catch
sequenceDiagram
autonumber
participant UI as Request / Router
participant Q as QrCodeable::handleCampRedirect(id)
participant E as EckEntity
participant SYS as CRM_Utils_System
UI->>Q: handleCampRedirect(id)
Q->>E: Fetch Collection_Camp by id
E-->>Q: Camp + Intent_Details (End_Date)
alt End_Date exists and today > End_Date+3
Q->>SYS: redirect("https://goonj.org/")
else
Q->>SYS: redirect("{baseURL}/actions/collection-camp/{id}")
end
note over Q: Logs errors if exceptions occur
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php (4)
49-53: Magic number + emoji comment; make logo scale configurable and clamp.Hard-coded 0.4 hurts reuse; clamp to sane bounds.
- $scaleFactor = 0.4; // 👈 change this (0.5 = 50% width, 0.3 = 30% width, etc.) + $scaleFactor = (float)($saveOptions['logoScale'] ?? 0.4); + $scaleFactor = max(0.1, min($scaleFactor, 0.8));
69-75: Canvas alpha not preserved.If logo has transparency, enable alpha for canvas.
- $canvas = imagecreatetruecolor($qrWidth, $canvasHeight); + $canvas = imagecreatetruecolor($qrWidth, $canvasHeight); + imagealphablending($canvas, false); + imagesavealpha($canvas, true);
89-97: Compression level not specified; minor size win possible.Set PNG compression (0–9). Default is 6; consider 9.
- imagepng($canvas); + imagepng($canvas, null, 9);
116-150: Remove debug prints; use Civi logging only on errors.error_log() lines are noisy and some are unreachable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: belwalshubham
PR: ColoredCow/goonj#284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
📚 Learning: 2024-10-11T09:53:14.458Z
Learnt from: belwalshubham
PR: ColoredCow/goonj#284
File: wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php:520-520
Timestamp: 2024-10-11T09:53:14.458Z
Learning: In `wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php`, it's acceptable that the `reGenerateCollectionCampQr` method calls `generateQrCode` with only the `$collectionCampId` parameter, as QR codes are generated when the Dropping Center is authorized. We'll implement changes if any issues arise in the future.
Applied to files:
wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php
🔇 Additional comments (4)
wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php (4)
76-81: Unconditional logo copy can warn when logo failed to load.Guard copy when $logoImage/$resizedLogo exist; otherwise set $newLogoHeight=0 and skip.
Please ensure the code branches correctly when logo fetch fails after applying the earlier guard. I can supply a follow-up diff once you confirm desired fallback (QR-only vs. hard-fail).
22-29: QR config choices: low ECC and fixed version may harm scan robustness.Consider ECC_M or ECC_Q and let version auto-size unless you have strict size constraints.
Would you like me to propose data-driven defaults (auto version, ECC_M, scale derived from target pixel size)?
31-38: QR binary is mishandled (base64 decode likely corrupts image).QRCode::render(OUTPUT_IMAGE_PNG) already returns PNG bytes. Stripping a data URI and base64_decoding will break binary and can make imagecreatefromstring() fail.
Apply:
- // Generate QR as PNG binary - $qrcode = (new QRCode($options))->render($data); - $qrcode = str_replace('data:image/png;base64,', '', $qrcode); - $qrcode = base64_decode($qrcode); + // Generate QR as PNG binary + $qrcodeBinary = (new QRCode($options))->render($data);And below:
- $qrImage = imagecreatefromstring($qrcode); + $qrImage = imagecreatefromstring($qrcodeBinary); + if ($qrImage === false) { + throw new \RuntimeException('Failed to create QR GD image'); + }⛔ Skipped due to learnings
Learnt from: belwalshubham PR: ColoredCow/goonj#352 File: wp-content/civi-extensions/goonjcustom/Civi/QrCodeService.php:51-53 Timestamp: 2024-10-09T15:24:42.859Z Learning: In `QrCodeService::generateQrCode`, when using `QRCode::OUTPUT_IMAGE_PNG`, the `render` method returns a data URI with base64-encoded image data. Therefore, it's necessary to remove the `data:image/png;base64,` prefix and base64-decode the data to obtain the raw binary image.
98-103: AllgenerateQrCodecallers includecustomGroupNameandcustomFieldNamein their$saveOptions; no action required.
| // --- Step 2: Load logo image (from URL) --- | ||
| $logoUrl = "https://goonj.org/wp-content/uploads/2020/06/Goonj-logo-10June20.png"; | ||
| $logoData = file_get_contents($logoUrl); | ||
| $logoImage = imagecreatefromstring($logoData); | ||
|
|
||
| $logoWidth = imagesx($logoImage); | ||
| $logoHeight = imagesy($logoImage); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remote logo fetch without timeout/validation; hard external dependency.
Network I/O on hot path + no timeout/error handling. Prefer a bundled/local asset or make URL configurable; at minimum add a short timeout and validate GD resource creation.
- $logoUrl = "https://goonj.org/wp-content/uploads/2020/06/Goonj-logo-10June20.png";
- $logoData = file_get_contents($logoUrl);
- $logoImage = imagecreatefromstring($logoData);
+ $logoUrl = $saveOptions['logoUrl'] ?? "https://goonj.org/wp-content/uploads/2020/06/Goonj-logo-10June20.png";
+ $ctx = stream_context_create([
+ 'http' => ['timeout' => 4],
+ 'ssl' => ['verify_peer' => true, 'verify_peer_name' => true],
+ ]);
+ $logoData = @file_get_contents($logoUrl, false, $ctx);
+ if ($logoData === false) {
+ \CRM_Core_Error::debug_log_message('QR: logo fetch failed, proceeding without logo.');
+ }
+ $logoImage = $logoData ? @imagecreatefromstring($logoData) : false;
+ if ($logoImage === false) {
+ // proceed without logo
+ $logoWidth = $logoHeight = 0;
+ }Note: If you keep logo optional, guard all subsequent logo math/copies accordingly.
Committable suggestion skipped: line range outside the PR's diff.
| // --- Step 3: Create new canvas (logo + QR + text space) --- | ||
| $text = "Hello here is the qr code"; | ||
| $fontHeight = 30; // reserve space for text | ||
|
|
||
| $canvasHeight = $newLogoHeight + $qrHeight + $fontHeight + 20; | ||
| $canvas = imagecreatetruecolor($qrWidth, $canvasHeight); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Placeholder caption, no i18n, incorrect text metrics.
Use configurable caption, compute font metrics with imagefont* to center, and only reserve space when text is non-empty.
- $text = "Hello here is the qr code";
- $fontHeight = 30; // reserve space for text
+ $text = (string)($saveOptions['caption'] ?? '');
+ $font = 5; // GD built-in font index
+ $fontHeight = $text !== '' ? imagefontheight($font) : 0;
+ $textWidth = $text !== '' ? imagefontwidth($font) * strlen($text) : 0;
@@
- $canvasHeight = $newLogoHeight + $qrHeight + $fontHeight + 20;
+ $extra = $text !== '' ? ($fontHeight + 20) : 0;
+ $canvasHeight = $newLogoHeight + $qrHeight + $extra;
@@
- $black = imagecolorallocate($canvas, 0, 0, 0);
- $x = 10;
- $y = $newLogoHeight + $qrHeight + 20;
- imagestring($canvas, 5, $x, $y, $text, $black);
+ if ($text !== '') {
+ $black = imagecolorallocate($canvas, 0, 0, 0);
+ $x = max(0, (int)(($qrWidth - $textWidth) / 2));
+ $y = $newLogoHeight + $qrHeight + 10;
+ imagestring($canvas, $font, $x, $y, $text, $black);
+ }Also consider translating caption via ts()/E::ts().
Also applies to: 82-87
| $baseFileName = "qr_code_{$entityId}.png"; | ||
|
|
||
| $saveOptions['baseFileName'] = $baseFileName; | ||
| $saveOptions['entityId'] = $entityId; | ||
|
|
||
| self::saveQrCode($qrcode, $saveOptions); | ||
| } | ||
| catch (\Exception $e) { | ||
| self::saveQrCode($finalImage, $saveOptions); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ignoring save result; generateQrCode returns TRUE even if save fails.
Propagate failure from saveQrCode.
- self::saveQrCode($finalImage, $saveOptions);
+ $attachment = self::saveQrCode($finalImage, $saveOptions);
+ if ($attachment === FALSE) {
+ throw new \RuntimeException('Failed to save QR attachment');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $baseFileName = "qr_code_{$entityId}.png"; | |
| $saveOptions['baseFileName'] = $baseFileName; | |
| $saveOptions['entityId'] = $entityId; | |
| self::saveQrCode($qrcode, $saveOptions); | |
| } | |
| catch (\Exception $e) { | |
| self::saveQrCode($finalImage, $saveOptions); | |
| $baseFileName = "qr_code_{$entityId}.png"; | |
| $saveOptions['baseFileName'] = $baseFileName; | |
| $saveOptions['entityId'] = $entityId; | |
| $attachment = self::saveQrCode($finalImage, $saveOptions); | |
| if ($attachment === FALSE) { | |
| throw new \RuntimeException('Failed to save QR attachment'); | |
| } |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php around
lines 98 to 103, the code calls self::saveQrCode($finalImage, $saveOptions) but
ignores its return value so generateQrCode can return true even when saving
fails; capture the boolean result of saveQrCode, and if it returns false
propagate that failure by returning false (or throwing the existing error path)
from generateQrCode so callers receive the real outcome.
| } catch (\Exception $e) { | ||
| \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage()); | ||
| return FALSE; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Catch broader throwables; log with context.
Catching only \Exception misses \Error. Use \Throwable and include entityId.
- } catch (\Exception $e) {
- \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage());
+ } catch (\Throwable $e) {
+ \CRM_Core_Error::debug_log_message('Error generating QR code for entityId '
+ . ($saveOptions['entityId'] ?? 'unknown') . ': ' . $e->getMessage());
return FALSE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (\Exception $e) { | |
| \CRM_Core_Error::debug_log_message('Error generating QR code: ' . $e->getMessage()); | |
| return FALSE; | |
| } | |
| } catch (\Throwable $e) { | |
| \CRM_Core_Error::debug_log_message('Error generating QR code for entityId ' | |
| . ($saveOptions['entityId'] ?? 'unknown') . ': ' . $e->getMessage()); | |
| return FALSE; | |
| } |
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php around
lines 104 to 107, the catch currently only handles \Exception and logs the error
message without context; change the catch to handle \Throwable instead of
\Exception and update the debug log to include the relevant entityId (or other
identifying variable in scope) along with the throwable message (and optionally
trace) so the log reads something like "Error generating QR code for
entityId=<id>: <message>" to ensure Errors are captured and the log contains
entity context.
| public static function handleCampRedirect($id) { | ||
| $camp = EckEntity::get('Collection_Camp', TRUE) | ||
| ->addSelect('Collection_Camp_Intent_Details.End_Date') | ||
| ->addWhere('id', '=', $id) | ||
| ->execute() | ||
| ->first(); | ||
|
|
||
| error_log("data: " . print_r($camp, TRUE)); | ||
|
|
||
| $endRaw = $camp['Collection_Camp_Intent_Details.End_Date'] ?? NULL; | ||
| error_log("endRaw: " . print_r($endRaw, TRUE)); | ||
|
|
||
| if ($endRaw) { | ||
| $endDate = new \DateTime($endRaw); | ||
| $endDate->modify('+3 days'); | ||
| $today = new \DateTime(); | ||
|
|
||
| if ($today > $endDate) { | ||
| error_log("working"); | ||
|
|
||
| // Redirect to goonj.org after end+3 days. | ||
| \CRM_Utils_System::redirect('https://goonj.org/'); | ||
| error_log("is redirect??: "); | ||
|
|
||
| return; | ||
|
|
||
| error_log("Checking ??: "); | ||
|
|
||
| } | ||
| } | ||
|
|
||
| // Otherwise normal camp page. | ||
| $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL; | ||
| \CRM_Utils_System::redirect("{$baseUrl}actions/collection-camp/{$id}"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redirect logic does not belong in this trait; remove debug noise and fix date/time handling.
SRP violation (QR trait doing HTTP redirects), noisy error_log(), timezone-sensitive comparison, and missing null checks on $camp. Suggest moving to a controller/service; meanwhile, fix logic.
- public static function handleCampRedirect($id) {
- $camp = EckEntity::get('Collection_Camp', TRUE)
- ->addSelect('Collection_Camp_Intent_Details.End_Date')
- ->addWhere('id', '=', $id)
- ->execute()
- ->first();
-
- error_log("data: " . print_r($camp, TRUE));
-
- $endRaw = $camp['Collection_Camp_Intent_Details.End_Date'] ?? NULL;
- error_log("endRaw: " . print_r($endRaw, TRUE));
-
- if ($endRaw) {
- $endDate = new \DateTime($endRaw);
- $endDate->modify('+3 days');
- $today = new \DateTime();
-
- if ($today > $endDate) {
- error_log("working");
-
- // Redirect to goonj.org after end+3 days.
- \CRM_Utils_System::redirect('https://goonj.org/');
- error_log("is redirect??: ");
-
- return;
-
- error_log("Checking ??: ");
-
- }
- }
-
- // Otherwise normal camp page.
- $baseUrl = \CRM_Core_Config::singleton()->userFrameworkBaseURL;
- \CRM_Utils_System::redirect("{$baseUrl}actions/collection-camp/{$id}");
- }
+ public static function handleCampRedirect($id) {
+ $id = (int)$id;
+ $camp = EckEntity::get('Collection_Camp', true)
+ ->addSelect('Collection_Camp_Intent_Details.End_Date')
+ ->addWhere('id', '=', $id)
+ ->setLimit(1)
+ ->execute()
+ ->first();
+
+ $endRaw = is_array($camp) ? ($camp['Collection_Camp_Intent_Details.End_Date'] ?? null) : null;
+ $tz = new \DateTimeZone('Asia/Kolkata'); // align with org TZ if different
+
+ if ($endRaw) {
+ $endDate = new \DateTime($endRaw, $tz);
+ $threshold = (clone $endDate)->modify('+3 days')->setTime(23, 59, 59);
+ $today = new \DateTime('now', $tz);
+
+ if ($today > $threshold) {
+ \CRM_Utils_System::redirect('https://goonj.org/');
+ return;
+ }
+ }
+
+ $baseUrl = rtrim(\CRM_Core_Config::singleton()->userFrameworkBaseURL, '/');
+ \CRM_Utils_System::redirect("{$baseUrl}/actions/collection-camp/{$id}");
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/Traits/QrCodeable.php around
lines 116 to 150, the trait currently performs HTTP redirects and leaves noisy
error_log() calls and fragile date handling; change it to be SRP-compliant by
removing all redirects and debug logging, add a null-check for $camp before
accessing the end date, parse the end date into a DateTimeImmutable with an
explicit timezone (e.g. from config or UTC), add 3 days with add(new
DateInterval('P3D')) and compare using DateTimeImmutable instances, and return a
simple status (e.g. boolean or enum: 'redirect_to_goonj' / 'show_camp') so a
controller/service can perform the actual redirect; keep the method lightweight
and side-effect free and document the return values.
1 participant
New QR code with more details
Summary by CodeRabbit