Allow specifying the QR-Code size in invoices#1489
Allow specifying the QR-Code size in invoices#1489nielsdrost7 merged 7 commits intoInvoicePlane:prep/v172from
Conversation
Comment out the schedule section in the workflow.
Removed duplicate permissions section in workflow.
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment Tip Migrating from UI to YAML configuration.Use the |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
application/helpers/invoice_helper.php (2)
126-128: Consider adding parameter type hints for clarity.The coding guidelines recommend using type hints for all parameters where possible. Adding type hints would make the API contract clearer.
♻️ Suggested improvement
/** * Returns a QR code for invoice payments. * - * `@param` number invoice-id - * `@param` number width + * `@param` int $invoice_id + * `@param` int $width Width in pixels (0 or negative to omit) */ -function invoice_qrcode($invoice_id, $width = 64): string +function invoice_qrcode(int $invoice_id, int $width = 64): stringAs per coding guidelines: "Use type hints for all parameters and return types where possible".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application/helpers/invoice_helper.php` around lines 126 - 128, The invoice_qrcode function lacks parameter type hints; update its signature (function invoice_qrcode) to add appropriate types for $invoice_id and $width (e.g., int $invoice_id, int $width = 64) while keeping the existing string return type, and then run a quick search for usages/call sites to adjust any places passing non-integer values or cast them as needed so the new type hints do not break callers.
128-128: Consider increasing the default or passing a custom width in PDF templates.The PDF template at
application/views/invoice_templates/pdf/InvoicePlane.php:352currently uses the default 64px width without customization. At 300 DPI, this translates to ~5.4mm, which may be marginal for reliable QR code scanning in print contexts. Either increase the default width or pass a larger custom value specifically for PDF templates.Additionally, document the recommended size ranges for different output contexts (web vs. PDF).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application/helpers/invoice_helper.php` at line 128, The invoice_qrcode function currently defaults to a small 64px width; update it to either a larger safer default (e.g. 128px) or keep 64px but ensure the PDF template calls invoice_qrcode($invoice_id, $width) with an increased width (suggest 128–300px depending on DPI), and add a short docblock above invoice_qrcode describing recommended sizes for web (64–128px) vs PDF/print (128–300px) so template authors know what to use; make the change by editing the invoice_qrcode signature and its docblock and by updating the PDF template (InvoicePlane.php) to pass a larger width if you opt to keep the smaller default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@application/helpers/invoice_helper.php`:
- Around line 126-128: The invoice_qrcode function lacks parameter type hints;
update its signature (function invoice_qrcode) to add appropriate types for
$invoice_id and $width (e.g., int $invoice_id, int $width = 64) while keeping
the existing string return type, and then run a quick search for usages/call
sites to adjust any places passing non-integer values or cast them as needed so
the new type hints do not break callers.
- Line 128: The invoice_qrcode function currently defaults to a small 64px
width; update it to either a larger safer default (e.g. 128px) or keep 64px but
ensure the PDF template calls invoice_qrcode($invoice_id, $width) with an
increased width (suggest 128–300px depending on DPI), and add a short docblock
above invoice_qrcode describing recommended sizes for web (64–128px) vs
PDF/print (128–300px) so template authors know what to use; make the change by
editing the invoice_qrcode signature and its docblock and by updating the PDF
template (InvoicePlane.php) to pass a larger width if you opt to keep the
smaller default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 680582af-46ac-4720-b973-f9cb8cf1c0fd
📒 Files selected for processing (1)
application/helpers/invoice_helper.php
|
Re: nitpicks I wanted to do the first, but that would be a breaking change. The second one ist really applicable. See Screenshots for Details. |
|
@mpldr thanks for the PR, man! Merged |
Pull Request Checklist
Please check the following steps before submitting your PR. If any items are incomplete, consider marking it as
[WIP](Work in Progress).Checklist
Description
Having the QR-Code be 10x10 cm in size is quite a bit too large for most invoices. This PR changes the size to be configurable, with the default at a much more reasonable 64 px.
Related Issue(s)
Fixes #1376 (again)
Motivation and Context
Had to edit the helper function in my deployment and thought to upstream the change.
Issue Type (Check one or more)
Screenshots (If Applicable)
My banking app had no issues reading the smaller QR code, please feel free to test with your own :P
Thank you for your contribution to InvoicePlane! We appreciate your time and effort.
Summary by CodeRabbit