Feature: positioning elements (manually)#1400
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTwo new boolean settings enable configurable PDF layout inversion for invoices and quotes: one reverses customer/company positioning, the other repositions the logo. Settings are exposed in the general settings panel and conditionally applied in PDF templates through CSS ID variants, with supporting stylesheet rules added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable options to control the positioning of the logo and the relative positions of customer and company blocks in PDF invoice/quote templates.
Changes:
- Adds new SCSS rules and IDs to support normal and inverted layouts for logo, client, and company sections in PDF templates.
- Updates invoice and quote PDF templates to conditionally append
-inverttologo,client, andcompanyIDs based on new settings. - Extends the general settings UI and English language file with two new options: reversing customer/company order and left-aligning the logo on PDFs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| assets/core/scss/_templates.scss | Adds layout rules for #logo-invert, #company-invert, #client, and #client-invert to support inverted header layouts. |
| application/views/quote_templates/pdf/InvoicePlane.php | Makes logo and client/company container IDs conditional on new invert-related settings for quotes. |
| application/views/invoice_templates/pdf/InvoicePlane.php | Makes logo and client/company container IDs conditional on new invert-related settings for invoices. |
| application/modules/settings/views/partial_settings_general.php | Adds two dropdowns under general settings to toggle logo position and customer/company inversion for PDFs. |
| application/language/english/ip_lang.php | Introduces language strings for the new PDF layout settings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
assets/core/scss/_templates.scss (1)
56-96: Remove stray backticks from selectors at lines 80, 86, 92.Backticks around selectors produce invalid CSS that won't match HTML elements with
id="company-invert",id="client", andid="client-invert", causing layout regressions.🛠️ Proposed fix
-`#company-invert` { +#company-invert { float: left; text-align: left; width: 40%; } -`#client` { +#client { float: left; width: 55%; margin-right: 5%; } -`#client-invert` { +#client-invert { float: right; width: 55%; margin-left: 5%; }
Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/8c81a4f8-e3a1-4c1d-86c4-585cf1c118fd Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…afety, dead code, DOM sanitizer Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/bf2724e4-678b-4ed5-9c24-3a3758d28493 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…er entry point Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/bf2724e4-678b-4ed5-9c24-3a3758d28493 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/bf2724e4-678b-4ed5-9c24-3a3758d28493 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…ed as HTML' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ate CHANGELOG and UPGRADE guide Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/d48e22c6-b03a-42e2-86e9-941490936d14 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…ENTS.md Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/f9be7229-f85e-423e-9683-9ea095e6bd74 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
…ADME Agent-Logs-Url: https://github.com/InvoicePlane/InvoicePlane/sessions/ef8eee14-a7f4-4753-be32-4afd75e6a563 Co-authored-by: nielsdrost7 <47660417+nielsdrost7@users.noreply.github.com>
nielsdrost7
force-pushed
the
feat/positioning-elements
branch
from
d7c2296 to
ce6ba16
Compare
nielsdrost7
force-pushed
the
feat/positioning-elements
branch
from
1939e2c to
dba0eed
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| AND ' . $this->db->escape($to_date) . ' >= inv.invoice_date_created | ||
| AND ' . (int) $minQuantity . ' <= | ||
| AND ' . (int) $this->db->escape($minQuantity) . ' <= | ||
| ( |
There was a problem hiding this comment.
$this->db->escape($minQuantity) returns a quoted SQL literal (e.g. '5'). Casting that escaped string to int results in 0, which will break the min/max filtering logic (the generated SQL becomes AND 0 <= (...)). Cast/validate $minQuantity before escaping (or drop the (int) and rely on proper numeric validation / query bindings).
| This document provides a comprehensive analysis of the security vulnerabilities addressed and DRY (Don't Repeat Yourself) principles applied in Pull Request #1441. | ||
|
|
||
| **PR Title:** Refactor input sanitization to follow DRY principles and fix log injection vulnerabilities | ||
| **Related Issue:** Feedback on PR #1439 | ||
| **Files Modified:** 5 files, +72 insertions, -23 deletions | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
This file appears to be a security/DRY analysis for a different PR (#1441) with a different title and set of changes than this PR (“Feature: positioning elements (manually)”). Keeping it in this PR is likely to confuse reviewers and future readers; consider removing it from this PR or renaming/updating it so it accurately documents the changes here.
| if (CI::$APP->config->item('controller_suffix') == null) { | ||
| $class = str_replace('', '', get_class($this)); | ||
| $class = str_replace('', '', get_class($this)); | ||
| } else { | ||
| $class = str_replace(CI::$APP->config->item('controller_suffix'), '', get_class($this)); | ||
| $class = str_replace(CI::$APP->config->item('controller_suffix'), '', get_class($this)); | ||
| } |
There was a problem hiding this comment.
Indentation in this block no longer matches the rest of the file (the $class = ... lines are not indented inside the if/else). This makes vendor code harder to read and can cause style/lint churn; please restore consistent indentation (or avoid modifying third_party files unless necessary).
| <div id="logo<?php if (get_setting('default_reverse_logo') === '1') { | ||
| echo '-invert'; | ||
| } ?>"> | ||
| <?php echo invoice_logo_pdf(); ?> | ||
| </div> | ||
|
|
||
| <div id="client"> | ||
| <div id="client<?php if (get_setting('default_reverse_customer_company') === '1') { | ||
| echo '-invert'; | ||
| } ?>"> |
There was a problem hiding this comment.
These templates introduce new behavior based on settings keys default_reverse_logo and default_reverse_customer_company, but these keys are not referenced anywhere else in the codebase (no settings UI/defaults/seed data found). As-is, the feature won’t be discoverable/configurable without manual DB edits; consider adding the corresponding settings definitions and defaults in the Settings/Setup flow.
| <div id="logo<?php if (get_setting('default_reverse_logo') === '1') { | ||
| echo '-invert'; | ||
| } ?>"> | ||
| <?php echo invoice_logo_pdf(); ?> | ||
| </div> | ||
|
|
||
| <div id="client"> | ||
| <div id="client<?php if (get_setting('default_reverse_customer_company') === '1') { | ||
| echo '-invert'; | ||
| } ?>"> |
There was a problem hiding this comment.
These templates introduce new behavior based on settings keys default_reverse_logo and default_reverse_customer_company, but these keys are not referenced anywhere else in the codebase (no settings UI/defaults/seed data found). As-is, the feature won’t be discoverable/configurable without manual DB edits; consider adding the corresponding settings definitions and defaults in the Settings/Setup flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/quickstart.yml:
- Around line 59-68: The workflow uses Laravel-specific steps ("Prepare Laravel
environment", "Run database migrations", "Run smoke tests") and commands (php
artisan, .env.testing.example) which don't exist in this CodeIgniter 3 repo;
replace those four steps with CodeIgniter-compatible smoke checks: run PHP lint
(php -l) across PHP files for syntax validation, run composer install --dry-run
to validate dependencies, start the PHP built-in server and request
index.php/setup (or curl the URL) to ensure bootstrap works, and run PHPUnit
against tests/ only if tests exist (otherwise omit the PHPUnit step). Update the
job step names and commands accordingly so the workflow replaces the "Prepare
Laravel environment", "Run database migrations", "Run smoke tests" steps with
the php -l, composer install --dry-run, built-in-server+index.php/setup check,
and conditional PHPUnit step.
In `@application/modules/reports/models/Mdl_reports.php`:
- Line 564: The condition is zeroing out $minQuantity by applying (int) to the
escaped string; locate the clause that builds the SQL fragment using
$this->db->escape($minQuantity) (the same area where $maxQuantity is used) and
replace the escaped-and-cast expression with the integer value directly (use the
already-integer $minQuantity or explicit (int)$minQuantity) so it matches the
neighboring $maxQuantity handling and avoids calling $this->db->escape on an
int.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53cb7247-e894-4590-9070-1d65ac70ca4d
📒 Files selected for processing (7)
.github/workflows/quickstart.yml.junie/PR-1441-security-dry-analysis.mdapplication/modules/reports/models/Mdl_reports.phpapplication/third_party/MX/Controller.phpapplication/views/invoice_templates/pdf/InvoicePlane.phpapplication/views/quote_templates/pdf/InvoicePlane.phpassets/core/scss/_templates.scss
✅ Files skipped from review due to trivial changes (3)
- application/third_party/MX/Controller.php
- application/views/quote_templates/pdf/InvoicePlane.php
- .junie/PR-1441-security-dry-analysis.md
🚧 Files skipped from review as they are similar to previous changes (2)
- application/views/invoice_templates/pdf/InvoicePlane.php
- assets/core/scss/_templates.scss
| AND ' . $this->db->escape($from_date) . ' <= inv.invoice_date_created | ||
| AND ' . $this->db->escape($to_date) . ' >= inv.invoice_date_created | ||
| AND ' . (int) $minQuantity . ' <= | ||
| AND ' . (int) $this->db->escape($minQuantity) . ' <= |
There was a problem hiding this comment.
Bug: (int) cast on escaped string zeroes out $minQuantity.
$this->db->escape($minQuantity) returns a quoted string literal (e.g. '5'). Applying (int) to that string yields 0 (PHP stops at the leading '), so this branch always emits AND 0 <= (...), effectively disabling the minimum-quantity filter whenever taxChecked and maxQuantity are set.
Since $minQuantity is already cast to int at line 214, escape+cast is both redundant and wrong here. Use the integer directly (matching the neighboring $maxQuantity usage on line 574):
🐛 Proposed fix
- AND ' . (int) $this->db->escape($minQuantity) . ' <=
+ AND ' . $this->db->escape($minQuantity) . ' <=📝 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.
| AND ' . (int) $this->db->escape($minQuantity) . ' <= | |
| AND ' . $this->db->escape($minQuantity) . ' <= |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@application/modules/reports/models/Mdl_reports.php` at line 564, The
condition is zeroing out $minQuantity by applying (int) to the escaped string;
locate the clause that builds the SQL fragment using
$this->db->escape($minQuantity) (the same area where $maxQuantity is used) and
replace the escaped-and-cast expression with the integer value directly (use the
already-integer $minQuantity or explicit (int)$minQuantity) so it matches the
neighboring $maxQuantity handling and avoids calling $this->db->escape on an
int.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation