Conversation
WebShells
added
enhancement
CodeIgniter4
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a customer-facing secondary sales display with cross-tab synchronization, integrates popup state and dual tender inputs into the register, extends Sales controller and routes, updates config/model/language, and adds a CI migration step. ChangesCustomer Display Feature & Register Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
88b0120 to
7769a65
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/Sales.php`:
- Around line 81-155: getSecondDisplay() currently always renders the second
display view even when the feature toggle is off; add a gating check at the
start of getSecondDisplay to read the config flag (second_display_enabled) and
short-circuit when disabled (e.g., return a 404/forbidden or empty
ResponseInterface) so the endpoint is effectively disabled. Locate the
getSecondDisplay method and insert the check before any session/sale_lib logic
(use $this->config['second_display_enabled'] or equivalent) and ensure the
method returns an appropriate ResponseInterface when the feature is off.
- Around line 125-126: The change-due logic double-counts overpayment because
amount_due is already negative when overpaid; update the assignment for
$data['payment_change_due'] to use the negative amount_due when amount_due < 0
and only compute payments_total - amount_due when amount_due >= 0. In other
words, keep $data['amount_change'] = $data['amount_due'] * -1, and change the
$data['payment_change_due'] calculation to: if ((float)$data['amount_due'] < 0)
set it to -(float)$data['amount_due'] else set it to
max(((float)$data['payments_total']) - ((float)$data['amount_due']), 0), so you
reference the $data['amount_due'] and $data['payments_total'] fields used in the
diff.
In `@app/Views/sales/register.php`:
- Around line 609-610: Currently window.notifySecondDisplay() is invoked before
the form/post is submitted which can cause the popup to reload stale session
state; update each place in register.php (calls to window.notifySecondDisplay()
around lines referenced) to call notifySecondDisplay only after the sale
mutation completes — i.e., move the call into the AJAX/fetch/XHR success
callback (or in the promise .then() after the POST resolves) or trigger it from
the next page load (e.g., on pageshow or after window.location.href navigation
completes) so the popup reloads after the server has the updated state.
- Around line 578-605: The opener's sessionStorage flag is never cleared when
the popup closes, causing refreshSecondDisplay() to later call window.open('',
'second_display') which can create a new blank window; fix by (1) in
openSecondDisplay() set the popup to notify the opener on unload (e.g., have the
opened window run a beforeunload/unload handler that clears
sessionStorage.getItem('secondDisplayOpen') and sets window.secondDisplayWindow
= null on the opener), (2) update refreshSecondDisplay() to prefer the stored
window.secondDisplayWindow reference and only attempt window.open('',
'second_display') as a fallback, and (3) ensure notifySecondDisplay() calls
refreshSecondDisplay() unchanged; reference functions: openSecondDisplay,
refreshSecondDisplay, notifySecondDisplay.
In `@app/Views/sales/second_display.php`:
- Around line 267-412: Replace all hardcoded, customer-facing strings in this
view with lang() lookups so they are localized: change titles like the panel
headings "Items" and "Summary", the summary section headers "Customer" and
"Change", labels such as "Customer Name", "Gift Card Balance", "Loyalty Reward
Points", "Rate", "Payments Total", "Amount Due", "Change Due" (and the
currency-specific "Total" / "Total <?= esc($secondary_currency_label) ?>") to
use existing language keys via lang('...') similar to other sales UI uses of
lang(); update the strings rendered in the panel-heading elements, the <th>
texts in the summary tables, and any inline headings inside the
second-display-main-row/second-display-summary-column so they call lang() with
appropriate keys (reuse existing Sales.* keys where available or add new keys)
instead of hardcoded English.
🪄 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: d859900c-55ce-4a0e-a68a-7ba1147a8f66
📒 Files selected for processing (8)
app/Config/Routes.phpapp/Controllers/Config.phpapp/Controllers/Sales.phpapp/Language/en/Config.phpapp/Language/en/Sales.phpapp/Views/configs/general_config.phpapp/Views/sales/register.phpapp/Views/sales/second_display.php
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/Views/sales/register.php (2)
609-660:⚠️ Potential issue | 🟠 Major
notifySecondDisplay()called before AJAX mutations complete — popup refreshed with stale state.This was flagged in the previous review and remains unresolved. The three AJAX handlers for
changeItemNumber(line 630),changeItemName(line 645), andchangeItemDescription(line 660) all callwindow.notifySecondDisplay()before the corresponding$.ajax()request is even dispatched. The popup will reload against the old server state.Move the call into the AJAX success callback:
🤖 Proposed fix (changeItemNumber; same pattern for the other two handlers)
$("input[name='item_number']").change(function() { var item_id = $(this).parents('tr').find("input[name='item_id']").val(); var item_number = $(this).val(); - window.notifySecondDisplay(); $.ajax({ url: "<?= site_url('sales/changeItemNumber') ?>", method: 'post', data: { 'item_id': item_id, 'item_number': item_number, }, - dataType: 'json' + dataType: 'json', + success: function() { window.notifySecondDisplay(); } }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/register.php` around lines 609 - 660, The handlers for input[name='item_number'], input[name='name'], and input[name='item_description'] call window.notifySecondDisplay() before the AJAX call completes, causing the second-display popup to refresh with stale state; move the window.notifySecondDisplay() invocation into the $.ajax success callback for the AJAX requests to changeItemNumber, changeItemName, and changeItemDescription (i.e., remove the pre-request notifySecondDisplay() and call it inside success: function(response){ window.notifySecondDisplay(); } for each handler so the popup reloads after the server update completes).
578-605:⚠️ Potential issue | 🟠 Major
sessionStorageflag not cleared on popup close +window.open('', 'second_display')spawns a blank window.The two related issues flagged in the previous review remain unresolved:
sessionStorage.setItem('secondDisplayOpen', '1')(line 581) is never cleared, so after the popup closes the flag persists indefinitely in the opener tab.- At line 595,
window.open('', 'second_display')will create a new blank window when the named popup no longer exists. The!secondDisplayWindow.closedguard then passes (the fresh blank window is not closed), sosecondDisplayWindow.location.reload()reloadsabout:blankinstead of the second display URL.Additionally, because
window.secondDisplayWindowis re-initialized tonullon every page navigation (line 578), the firstnotifySecondDisplay()call after any register redirect will always hit thewindow.open('', …)branch and exhibit the blank-window bug.The fix is to rely on the stored
window.secondDisplayWindowreference rather than re-acquiring it throughwindow.open:🤖 Proposed fix
window.openSecondDisplay = function(url) { - sessionStorage.setItem('secondDisplayOpen', '1'); window.secondDisplayWindow = window.open(url, 'second_display', 'width=1280,height=720,resizable=yes,scrollbars=yes'); if (window.secondDisplayWindow && !window.secondDisplayWindow.closed) { + window.secondDisplayWindow.addEventListener('beforeunload', function() { + window.secondDisplayWindow = null; + }); window.secondDisplayWindow.focus(); } return false; }; window.refreshSecondDisplay = function() { - if (sessionStorage.getItem('secondDisplayOpen') !== '1') { - return; - } - - const secondDisplayWindow = window.open('', 'second_display'); - if (secondDisplayWindow && !secondDisplayWindow.closed) { - secondDisplayWindow.location.reload(); - secondDisplayWindow.focus(); - window.secondDisplayWindow = secondDisplayWindow; + if (!window.secondDisplayWindow || window.secondDisplayWindow.closed) { + window.secondDisplayWindow = null; + return; } + window.secondDisplayWindow.location.reload(); + window.secondDisplayWindow.focus(); };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/register.php` around lines 578 - 605, Remove the unconditional re-initialization of window.secondDisplayWindow and stop using window.open('', 'second_display') to re-acquire the popup; instead: 1) delete the line that sets window.secondDisplayWindow = null on load, 2) in openSecondDisplay(url) store the returned window in window.secondDisplayWindow and attach a polling/interval or onbeforeunload handler on that popup to clear sessionStorage.removeItem('secondDisplayOpen') and set window.secondDisplayWindow = null when the popup closes/unloads, and 3) rewrite refreshSecondDisplay() to first check window.secondDisplayWindow && !window.secondDisplayWindow.closed and only then reload/focus it, otherwise clear the 'secondDisplayOpen' flag and do nothing (no window.open call). Ensure notifySecondDisplay() continues to call refreshSecondDisplay().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/sales/register.php`:
- Line 100: The conditional in register.php uses
($config['second_display_enabled'] ?? true) == 1 which makes the Second Display
opt-in default to enabled when the config key is missing; change the default to
disabled by replacing the null-coalescing default true with false/0 (e.g.,
($config['second_display_enabled'] ?? 0) == 1) or use a strict check like
!empty($config['second_display_enabled']) or ($config['second_display_enabled']
?? false) === true so the button only appears when
$config['second_display_enabled'] is explicitly set/enabled.
---
Duplicate comments:
In `@app/Views/sales/register.php`:
- Around line 609-660: The handlers for input[name='item_number'],
input[name='name'], and input[name='item_description'] call
window.notifySecondDisplay() before the AJAX call completes, causing the
second-display popup to refresh with stale state; move the
window.notifySecondDisplay() invocation into the $.ajax success callback for the
AJAX requests to changeItemNumber, changeItemName, and changeItemDescription
(i.e., remove the pre-request notifySecondDisplay() and call it inside success:
function(response){ window.notifySecondDisplay(); } for each handler so the
popup reloads after the server update completes).
- Around line 578-605: Remove the unconditional re-initialization of
window.secondDisplayWindow and stop using window.open('', 'second_display') to
re-acquire the popup; instead: 1) delete the line that sets
window.secondDisplayWindow = null on load, 2) in openSecondDisplay(url) store
the returned window in window.secondDisplayWindow and attach a polling/interval
or onbeforeunload handler on that popup to clear
sessionStorage.removeItem('secondDisplayOpen') and set
window.secondDisplayWindow = null when the popup closes/unloads, and 3) rewrite
refreshSecondDisplay() to first check window.secondDisplayWindow &&
!window.secondDisplayWindow.closed and only then reload/focus it, otherwise
clear the 'secondDisplayOpen' flag and do nothing (no window.open call). Ensure
notifySecondDisplay() continues to call refreshSecondDisplay().
🪄 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: ef29849b-5885-499a-9089-8a10bed1a1ec
📒 Files selected for processing (8)
app/Config/Routes.phpapp/Controllers/Config.phpapp/Controllers/Sales.phpapp/Language/en/Config.phpapp/Language/en/Sales.phpapp/Views/configs/general_config.phpapp/Views/sales/register.phpapp/Views/sales/second_display.php
✅ Files skipped from review due to trivial changes (2)
- app/Language/en/Sales.php
- app/Language/en/Config.php
🚧 Files skipped from review as they are similar to previous changes (5)
- app/Views/configs/general_config.php
- app/Controllers/Config.php
- app/Config/Routes.php
- app/Controllers/Sales.php
- app/Views/sales/second_display.php
7769a65 to
bdf55e7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
48faeb5 to
7769a65
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/configs/general_config.php`:
- Line 32: The anchor for the theme preview has a malformed rel attribute;
update the <a> tag that builds the BootsWatch URL (uses $config['theme']) to use
a valid quoted rel value such as rel="noopener noreferrer" (or at minimum
rel="noopener") so the attribute is properly formed and safe when opening.
- Around line 498-509: The IIFE uses deprecated arguments.callee which breaks
strict mode; replace it with a named function: define function
enable_disable_gcaptcha_enable() { var gcaptcha_enable =
$("#gcaptcha_enable").is(":checked"); if (gcaptcha_enable) {
$("#gcaptcha_site_key, `#gcaptcha_secret_key`").prop("disabled",
!gcaptcha_enable).addClass("required"); $("#config_gcaptcha_site_key,
`#config_gcaptcha_secret_key`").addClass("required"); } else {
$("#gcaptcha_site_key, `#gcaptcha_secret_key`").prop("disabled",
gcaptcha_enable).removeClass("required"); $("#config_gcaptcha_site_key,
`#config_gcaptcha_secret_key`").removeClass("required"); } } then assign var
enable_disable_gcaptcha_enable = enable_disable_gcaptcha_enable (or just use the
function name), remove the return arguments.callee, and update the other two
usages (the event handler bound to `#gcaptcha_enable` and the explicit call later)
to call the named function enable_disable_gcaptcha_enable().
In `@app/Views/sales/customer_display.php`:
- Around line 26-33: The table header outputs unescaped dynamic values—wrap
numeric width attributes ($cartItemWidth, $cartPriceWidth, $cartOriginalWidth,
$cartQuantityWidth, $cartDiscountWidth, $cartTotalWidth) with an integer cast
(e.g., (int)) to prevent injection and escape textual content
($customerDisplayCurrencyLabel, $originalCurrencyLabel) with the esc() helper;
also apply the same fixes where dynamic values are used elsewhere in this view
(including the referenced lines ~39 and ~69-73 and any colspan attributes) so
all attribute and content contexts are safely sanitized.
🪄 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: e1749344-1def-4d2a-85b7-6b3d4b9e9ac7
📒 Files selected for processing (9)
app/Config/Routes.phpapp/Controllers/Config.phpapp/Controllers/Sales.phpapp/Language/en/Config.phpapp/Language/en/Sales.phpapp/Views/configs/general_config.phpapp/Views/partial/customer_display_header.phpapp/Views/sales/customer_display.phpapp/Views/sales/register.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Views/sales/register.php
- app/Language/en/Sales.php
- app/Language/en/Config.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Views/configs/general_config.php (1)
132-172:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnresolved Git merge conflict — page will be broken as-is.
Lines 133, 136, and 161 contain raw conflict markers (
<<<<<<< HEAD,=======,>>>>>>> 3ef01a6ee). A PHP/HTML page with these literal strings in it will either:
- Trigger a PHP parse error if the surrounding PHP tags make the markers syntactically invalid, or
- Silently emit the conflict text into the HTML, corrupting the page layout.
Additionally, the
<div class="col-xs-3">opened on line 135 (HEAD side) is never closed on the HEAD path because the=======marker cuts it off, leaving unclosed markup regardless of which resolution is chosen.The OTHER side (lines 137–160) duplicates the
theme+login_formblocks that are already rendered earlier (lines 19–54), so the resolution should simply keep the HEAD side (receiving_cost_price_methodlabel +col-xs-3wrapper) and discard the OTHER side.🔧 Suggested resolution
-<<<<<<< HEAD <?= form_label(lang('Config.receiving_cost_price_method'), 'receiving_cost_price_method', ['class' => 'control-label col-xs-2']) ?> <div class="col-xs-3"> -======= - <?= form_label(lang('Config.theme'), 'theme', ['class' => 'control-label col-xs-2']) ?> - ... (entire duplicated theme + login_form block — remove this side) ->>>>>>> 3ef01a6ee (Customer Display fixes) <?= form_dropdown( 'receiving_cost_price_method', [ 'average' => lang('Config.receiving_cost_price_method_average'), 'new' => lang('Config.receiving_cost_price_method_new'), ], (($config['receiving_cost_price_method'] ?? (($config['receiving_calculate_average_price'] ?? 1) ? 'average' : 'new'))), ['id' => 'receiving_cost_price_method', 'class' => 'form-control'] ) ?> </div> </div>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/configs/general_config.php` around lines 132 - 172, Remove the raw Git conflict markers (<<<<<<<, =======, >>>>>>>) and the duplicated OTHER-side block for 'theme'/'login_form'; keep the HEAD-side form-group that renders the receiving_cost_price_method (labels/id 'receiving_cost_price_method' and the surrounding <div class="col-xs-3">), ensure that the opened <div class="col-xs-3"> is properly closed and the markup hierarchy matches surrounding form-groups, and delete the entire duplicated theme/login_form block (the code between the ======= and >>>>>>> markers) so the page renders only the intended receiving_cost_price_method dropdown.
🧹 Nitpick comments (1)
app/Views/configs/general_config.php (1)
445-457: ⚡ Quick winUse
?? 1instead of?? truefor consistency and clarity.
($config['customer_display_enabled'] ?? true) == 1relies on PHP loose comparison (true == 1→true), which works but is misleading — a reader unfamiliar with PHP's type juggling rules could interpret?? trueas a boolean guard rather than a numeric default. Every other checkbox in this file (e.g.,multi_pack_enabled,include_hsn,category_dropdown) uses== 1directly against the raw config value. Using?? 1aligns with that convention and makes the intent (default-enabled) explicit.♻️ Proposed fix
- 'checked' => ($config['customer_display_enabled'] ?? true) == 1 + 'checked' => ($config['customer_display_enabled'] ?? 1) == 1- 'checked' => ($config['show_grid_enabled'] ?? true) == 1 + 'checked' => ($config['show_grid_enabled'] ?? 1) == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/configs/general_config.php` around lines 445 - 457, The ternary default for the checkboxes uses boolean true which is misleading when compared with == 1; update the default fallbacks for the 'customer_display_enabled' and nearby 'show_grid_enabled' checkbox evaluations to use null coalescing to 1 (i.e., ($config['customer_display_enabled'] ?? 1) == 1 and ($config['show_grid_enabled'] ?? 1) == 1) so they match the convention used by other checkboxes and clearly express a default-enabled intent in the form_checkbox rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/Views/configs/general_config.php`:
- Around line 132-172: Remove the raw Git conflict markers (<<<<<<<, =======,
>>>>>>>) and the duplicated OTHER-side block for 'theme'/'login_form'; keep the
HEAD-side form-group that renders the receiving_cost_price_method (labels/id
'receiving_cost_price_method' and the surrounding <div class="col-xs-3">),
ensure that the opened <div class="col-xs-3"> is properly closed and the markup
hierarchy matches surrounding form-groups, and delete the entire duplicated
theme/login_form block (the code between the ======= and >>>>>>> markers) so the
page renders only the intended receiving_cost_price_method dropdown.
---
Nitpick comments:
In `@app/Views/configs/general_config.php`:
- Around line 445-457: The ternary default for the checkboxes uses boolean true
which is misleading when compared with == 1; update the default fallbacks for
the 'customer_display_enabled' and nearby 'show_grid_enabled' checkbox
evaluations to use null coalescing to 1 (i.e.,
($config['customer_display_enabled'] ?? 1) == 1 and
($config['show_grid_enabled'] ?? 1) == 1) so they match the convention used by
other checkboxes and clearly express a default-enabled intent in the
form_checkbox rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ed03b48-37ab-442f-8487-ef88428bac83
📒 Files selected for processing (2)
app/Views/configs/general_config.phpapp/Views/sales/customer_display.php
✅ Files skipped from review due to trivial changes (1)
- app/Views/sales/customer_display.php
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
app/Views/configs/general_config.php (1)
486-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace deprecated
arguments.calleecallback patternLines 486-497 still rely on
arguments.callee. This is deprecated and can throw in strict mode; define a named function and bind/call it directly.Suggested fix
- var enable_disable_gcaptcha_enable = (function() { + function enableDisableGcaptchaEnable() { var gcaptcha_enable = $("#gcaptcha_enable").is(":checked"); if (gcaptcha_enable) { $("#gcaptcha_site_key, `#gcaptcha_secret_key`").prop("disabled", !gcaptcha_enable).addClass("required"); $("#config_gcaptcha_site_key, `#config_gcaptcha_secret_key`").addClass("required"); } else { $("#gcaptcha_site_key, `#gcaptcha_secret_key`").prop("disabled", gcaptcha_enable).removeClass("required"); $("#config_gcaptcha_site_key, `#config_gcaptcha_secret_key`").removeClass("required"); } - - return arguments.callee; - })(); + } + enableDisableGcaptchaEnable(); - $("#gcaptcha_enable").change(enable_disable_gcaptcha_enable); + $("#gcaptcha_enable").change(enableDisableGcaptchaEnable); @@ - enable_disable_gcaptcha_enable(); + enableDisableGcaptchaEnable();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/configs/general_config.php` around lines 486 - 499, The IIFE uses the deprecated arguments.callee; refactor it into a named function (e.g., function enable_disable_gcaptcha_enable()) that contains the gcaptcha_enable logic and returns nothing, then immediately invoke that function once and attach it to the change handler via $("#gcaptcha_enable").change(enable_disable_gcaptcha_enable); update references inside the function (gcaptcha_enable, $("#gcaptcha_site_key"...) etc.) unchanged so behavior remains the same but without using arguments.callee.app/Views/sales/customer_display.php (3)
73-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDead ternary:
$cartHasCustomerDisplay ? 2 : 2always evaluates to2.Both branches return the same value; simplify to a static attribute.
♻️ Proposed fix
-<td colspan="<?= $cartHasCustomerDisplay ? 2 : 2 ?>" class="serial-cell"> +<td colspan="2" class="serial-cell">🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/customer_display.php` at line 73, The colspan attribute uses a dead ternary ($cartHasCustomerDisplay ? 2 : 2) in the customer_display view; replace that expression with the static value 2 so the <td class="serial-cell"> uses colspan="2" directly (remove the ternary and $cartHasCustomerDisplay reference).
39-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
$cartColspanis still unescaped — the previous fix was not applied.Line 39 still outputs
$cartColspandirectly with no(int)cast, even though the prior review comment was marked as addressed (commit a24e776). Every othercolspanin this file was fixed; this one was missed.🛡️ Proposed fix
-<td colspan="<?= $cartColspan ?>"> +<td colspan="<?= (int) $cartColspan ?>">As per coding guidelines: "Sanitize user input; escape output using
esc()helper".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/customer_display.php` at line 39, The colspan attribute still outputs $cartColspan unescaped; update the echo of $cartColspan in the <td colspan="<?= $cartColspan ?>"> usage to sanitize it by casting to an integer or passing through the esc() helper (e.g., (int)$cartColspan or esc($cartColspan, 'attr')) so the colspan value is safe before output; locate the occurrence referencing $cartColspan in the customer_display.php view and replace the direct variable output with the sanitized form.
104-104:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
number_format($rate)silently truncates decimal places.
number_formatwith one argument defaults to 0 decimal places, rounding the value (e.g.,1234.56becomes"1,235"). For a customer-facing exchange rate, this can display a misleading value. Specify an explicit precision — evennumber_format($rate, 2)is safer than relying on the implicit default.🛠️ Proposed fix
-<td><?= number_format($rate) ?></td> +<td><?= number_format((float)$rate, 2) ?></td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/customer_display.php` at line 104, The view uses number_format($rate) which defaults to 0 decimals and can misrepresent exchange rates; update the call in customer_display.php (the table cell rendering the rate) to specify an explicit precision like number_format($rate, 2) (or a suitable decimal count for your currency) so decimals are preserved and displayed correctly.
🧹 Nitpick comments (1)
app/Views/sales/customer_display.php (1)
121-121: ⚡ Quick win
$giftcardRemainderis not formatted as currency.Every other monetary value in this view uses
to_currency(), but the gift card balance is emitted as a raw escaped string. This will display as a bare decimal (e.g.,"10.5") rather than a formatted currency value (e.g.,"$10.50").♻️ Proposed fix
-<td class="giftcard-value"><?= esc((string)($giftcardRemainder ?? '0')) ?></td> +<td class="giftcard-value"><?= to_currency((float)($giftcardRemainder ?? 0)) ?></td>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/customer_display.php` at line 121, The gift card remainder is output as a raw escaped string; change the cell to format it as currency using the existing to_currency helper by passing a numeric default (e.g., $giftcardRemainder ?? 0) instead of the raw string—replace the esc((string)($giftcardRemainder ?? '0')) usage with a to_currency call (ensuring the value is numeric/cast as needed) so the gift card balance matches other monetary fields in the view.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/configs/general_config.php`:
- Around line 284-286: The external link opening in a new tab (the anchor tag
wrapping the span with class "glyphicon glyphicon-info-sign" that points to
"https://www.google.com/recaptcha/admin" and uses) must include
rel="noopener noreferrer" to prevent reverse-tabnabbing; update that <a> element
to add rel="noopener noreferrer" alongside the existing target attribute.
In `@app/Views/sales/customer_display.php`:
- Around line 170-209: The current logic using plain localStorage keys
customerDisplayOpen and customerDisplayDirtyAt causes cross-tab/register
interference; change the code in this view (functions scheduleRefresh,
checkForRefresh, storage and beforeunload listeners) to use namespaced keys that
include a per-display identifier (e.g., displayId) instead of the plain keys,
obtaining that displayId from the URL or a dedicated localStorage entry;
coordinate with the writer side so it writes customerDisplayId and uses the same
suffix when setting its dirty timestamp (e.g.,
customerDisplayDirtyAt_<displayId>) and open flag
(customerDisplayOpen_<displayId>), then update all references in this file
(getItem, setItem, removeItem, and the storage event check for event.key) to use
the suffixed key so each register/display pair is isolated.
- Line 21: The view app/Views/sales/customer_display.php contains hardcoded
headings and summary table labels ("Items", "Summary", "Total", "Rate",
"Customer Name", "Gift Card Balance", "Loyalty Reward Points", "Payments Total",
"Amount Due", "Change Due", and the currency-suffixed variants)—replace each
literal string with the matching language call like lang('Sales.Items'),
lang('Sales.Summary'), lang('Sales.Total'), etc.; for currency-specific labels
use the language key (e.g., lang('Sales.AmountDue')) combined with the currency
variable or use a language string with a placeholder if available; ensure the
keys you reference match the new entries in app/Language/en/Sales.php and update
all occurrences in the view (panel-heading and the summary table cell labels) to
maintain i18n consistency.
---
Duplicate comments:
In `@app/Views/configs/general_config.php`:
- Around line 486-499: The IIFE uses the deprecated arguments.callee; refactor
it into a named function (e.g., function enable_disable_gcaptcha_enable()) that
contains the gcaptcha_enable logic and returns nothing, then immediately invoke
that function once and attach it to the change handler via
$("#gcaptcha_enable").change(enable_disable_gcaptcha_enable); update references
inside the function (gcaptcha_enable, $("#gcaptcha_site_key"...) etc.) unchanged
so behavior remains the same but without using arguments.callee.
In `@app/Views/sales/customer_display.php`:
- Line 73: The colspan attribute uses a dead ternary ($cartHasCustomerDisplay ?
2 : 2) in the customer_display view; replace that expression with the static
value 2 so the <td class="serial-cell"> uses colspan="2" directly (remove the
ternary and $cartHasCustomerDisplay reference).
- Line 39: The colspan attribute still outputs $cartColspan unescaped; update
the echo of $cartColspan in the <td colspan="<?= $cartColspan ?>"> usage to
sanitize it by casting to an integer or passing through the esc() helper (e.g.,
(int)$cartColspan or esc($cartColspan, 'attr')) so the colspan value is safe
before output; locate the occurrence referencing $cartColspan in the
customer_display.php view and replace the direct variable output with the
sanitized form.
- Line 104: The view uses number_format($rate) which defaults to 0 decimals and
can misrepresent exchange rates; update the call in customer_display.php (the
table cell rendering the rate) to specify an explicit precision like
number_format($rate, 2) (or a suitable decimal count for your currency) so
decimals are preserved and displayed correctly.
---
Nitpick comments:
In `@app/Views/sales/customer_display.php`:
- Line 121: The gift card remainder is output as a raw escaped string; change
the cell to format it as currency using the existing to_currency helper by
passing a numeric default (e.g., $giftcardRemainder ?? 0) instead of the raw
string—replace the esc((string)($giftcardRemainder ?? '0')) usage with a
to_currency call (ensuring the value is numeric/cast as needed) so the gift card
balance matches other monetary fields in the view.
🪄 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: 97a3cd87-ee4f-4a4b-9846-457b1085fc70
📒 Files selected for processing (2)
app/Views/configs/general_config.phpapp/Views/sales/customer_display.php
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/Views/sales/customer_display.php (1)
137-137: 💤 Low value
$payments_totalusessnake_case— violates PSR-12 camelCase convention.All sibling variables in the same block (
$amount_dueline 141,$paymentChangeDueline 151) show mixed conventions.$payments_totalshould be$paymentsTotal(and$amount_due→$amountDue) for consistency.♻️ Proposed fix
-<td><?= to_currency($payments_total) ?></td> +<td><?= to_currency($paymentsTotal) ?></td>-<th><?= lang('Sales.amount_due') ?></th> -<td><?= to_currency($amount_due) ?></td> +<th><?= lang('Sales.amount_due') ?></th> +<td><?= to_currency($amountDue) ?></td>Coordinate the rename with the controller's view-model assignments.
As per coding guidelines: "PSR-12 naming conventions:
camelCasefor variables and functions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Views/sales/customer_display.php` at line 137, The view uses snake_case variables ($payments_total, $amount_due) which violates PSR-12 and is inconsistent with nearby camelCase ($paymentChangeDue); rename these view variables to camelCase (e.g., $paymentsTotal and $amountDue) and update the controller or view-model that assigns them so the view receives the new names (search for assignments/compact keys or data array entries that set payments_total and amount_due and change them to paymentsTotal and amountDue, and update all occurrences in customer_display.php to match).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/sales/customer_display.php`:
- Around line 170-171: The current customerDisplayId assignment uses
sessionStorage.getItem('customerDisplayId') and
localStorage.getItem('customerDisplayId') fallbacks which re-introduce
cross-register interference; change the initialization of customerDisplayId so
it only uses the URL param (new
URLSearchParams(window.location.search).get('displayId')) and a default empty
string (''), removing both sessionStorage and localStorage fallbacks; update any
related logic that builds customerDisplayStorageSuffix (the
customerDisplayStorageSuffix variable) to rely on that URL-only
customerDisplayId and keep the existing suffix behavior when customerDisplayId
is non-empty.
- Around line 28-30: Replace the hardcoded "Price (" concatenation with a
localized string: add a language key Sales.price_with_currency with value "Price
(%s)" and render the headers using that key, e.g. use
sprintf(lang('Sales.price_with_currency'), esc($customerDisplayCurrencyLabel))
for the $cartPriceWidth header and sprintf(lang('Sales.price_with_currency'),
esc($originalCurrencyLabel)) for the $cartOriginalWidth header (referencing
$cartPriceWidth, $cartOriginalWidth, $customerDisplayCurrencyLabel, and
$originalCurrencyLabel to locate the lines).
---
Nitpick comments:
In `@app/Views/sales/customer_display.php`:
- Line 137: The view uses snake_case variables ($payments_total, $amount_due)
which violates PSR-12 and is inconsistent with nearby camelCase
($paymentChangeDue); rename these view variables to camelCase (e.g.,
$paymentsTotal and $amountDue) and update the controller or view-model that
assigns them so the view receives the new names (search for assignments/compact
keys or data array entries that set payments_total and amount_due and change
them to paymentsTotal and amountDue, and update all occurrences in
customer_display.php to match).
🪄 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: 7d0e975d-c6b2-4f99-89fe-11f5b9c76c55
📒 Files selected for processing (4)
app/Language/en/Sales.phpapp/Views/configs/general_config.phpapp/Views/sales/customer_display.phpapp/Views/sales/register.php
🚧 Files skipped from review as they are similar to previous changes (3)
- app/Language/en/Sales.php
- app/Views/configs/general_config.php
- app/Views/sales/register.php
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/Controllers/Config.php (1)
505-509:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate and normalize
secondary_currency_codebefore savingLine 507 still persists raw input. Please enforce a strict currency-code format (e.g., 3 letters) and normalize casing before
batch_save.🔧 Minimal fix
+ $secondaryCurrencyCode = strtoupper(trim((string) $this->request->getPost('secondary_currency_code'))); + if (!preg_match('/^[A-Z]{3}$/', $secondaryCurrencyCode)) { + $secondaryCurrencyCode = ''; + } + $batch_save_data = [ 'currency_symbol' => htmlspecialchars($currency_symbol ?? ''), 'currency_code' => $this->request->getPost('currency_code'), 'secondary_currency_enabled' => $this->request->getPost('secondary_currency_enabled') != null, 'secondary_currency_symbol' => htmlspecialchars($this->request->getPost('secondary_currency_symbol') ?? ''), - 'secondary_currency_code' => $this->request->getPost('secondary_currency_code'), + 'secondary_currency_code' => $secondaryCurrencyCode, 'secondary_currency_rate' => $this->request->getPost('secondary_currency_rate', FILTER_SANITIZE_NUMBER_FLOAT, FILTER_FLAG_ALLOW_FRACTION),As per coding guidelines,
app/**/*.php: “Sanitize user input; escape output usingesc()helper” and{app/Controllers/**/*.php,app/Models/**/*.php}: “Validate and sanitize all user input”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` around lines 505 - 509, The secondary_currency_code value is taken raw; before calling batch_save, validate and normalize it by retrieving via $this->request->getPost('secondary_currency_code'), trimming and uppercasing it (e.g., $code = strtoupper(trim(...))), ensure it matches a 3-letter alphabetic ISO format (preg_match('/^[A-Z]{3}$/', $code')), and only set 'secondary_currency_code' in the payload to the normalized code if it passes validation (otherwise set to empty string or null); update the array key handling where 'secondary_currency_code' is currently assigned and ensure any downstream use (e.g., in batch_save) uses the validated/normalized value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@app/Controllers/Config.php`:
- Around line 505-509: The secondary_currency_code value is taken raw; before
calling batch_save, validate and normalize it by retrieving via
$this->request->getPost('secondary_currency_code'), trimming and uppercasing it
(e.g., $code = strtoupper(trim(...))), ensure it matches a 3-letter alphabetic
ISO format (preg_match('/^[A-Z]{3}$/', $code')), and only set
'secondary_currency_code' in the payload to the normalized code if it passes
validation (otherwise set to empty string or null); update the array key
handling where 'secondary_currency_code' is currently assigned and ensure any
downstream use (e.g., in batch_save) uses the validated/normalized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ffce4db-c11a-4f2a-b4c3-88de92e800f3
📒 Files selected for processing (3)
app/Controllers/Config.phpapp/Controllers/Sales.phpapp/Views/sales/register.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/sales/register.php
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/Config.php (1)
373-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
receiving_cost_price_methodagainst allowed values before persisting.Line 373 accepts any posted string, but downstream logic treats this like an enum (
average/new). Persisting unexpected values can silently desync settings behavior.Proposed fix
- $receiving_cost_price_method = $this->request->getPost('receiving_cost_price_method'); + $receiving_cost_price_method = (string) $this->request->getPost('receiving_cost_price_method'); + if (!in_array($receiving_cost_price_method, ['average', 'new'], true)) { + $receiving_cost_price_method = ''; + } if ($receiving_cost_price_method === null || $receiving_cost_price_method === '') { $receiving_cost_price_method = $this->request->getPost('receiving_calculate_average_price') != null ? 'average' : 'new'; }As per coding guidelines,
{app/Controllers/**/*.php,app/Models/**/*.php}: “Validate and sanitize all user input”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` around lines 373 - 388, The posted receiving_cost_price_method is not validated; ensure the value of $receiving_cost_price_method (used in batchSaveData keys 'receiving_cost_price_method' and 'receiving_calculate_average_price') is explicitly checked against allowed values 'average' and 'new' before persisting: read the posted value, if it is not exactly 'average' or 'new' fall back to the existing legacy logic (use receiving_calculate_average_price to decide) or default to 'new', and set receiving_calculate_average_price based on the validated final choice; update the validation near where $receiving_cost_price_method is assigned in the Config controller so only the sanctioned values are stored.
♻️ Duplicate comments (3)
app/Controllers/Config.php (3)
395-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
implode()input to avoid PHP 8+TypeError.Line 395 can receive
nullwhen no types are posted;implode(',', null)is unsafe in PHP 8+.Proposed fix
- 'image_allowed_types' => implode(',', $this->request->getPost('image_allowed_types')), + 'image_allowed_types' => implode(',', (array) ($this->request->getPost('image_allowed_types') ?? [])),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` at line 395, The call that builds 'image_allowed_types' uses implode(',', $this->request->getPost('image_allowed_types')) which can receive null and throw a TypeError on PHP 8+; change it to guard/normalize the input from $this->request->getPost('image_allowed_types') (e.g., cast to array or coalesce to an empty array) before calling implode so implode always receives an array (update the assignment where 'image_allowed_types' is set in Config.php to use the normalized value).
485-490:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate and normalize
currency_codebefore save.Line 489 still persists raw POST input. Apply the same strict 3-letter normalization used for other currency-code fields.
Proposed fix
$exploded = explode(":", $this->request->getPost('language')); $currency_symbol = $this->request->getPost('currency_symbol'); + $currencyCode = strtoupper(trim((string) $this->request->getPost('currency_code'))); + if (!preg_match('/^[A-Z]{3}$/', $currencyCode)) { + $currencyCode = ''; + } $batch_save_data = [ 'currency_symbol' => htmlspecialchars($currency_symbol ?? ''), - 'currency_code' => $this->request->getPost('currency_code'), + 'currency_code' => $currencyCode,As per coding guidelines,
app/**/*.php: “Sanitize user input; escape output using esc() helper” and{app/Controllers/**/*.php,app/Models/**/*.php}: “Validate and sanitize all user input”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` around lines 485 - 490, The currency_code value is being saved directly from POST into batch_save_data; normalize and validate it to a strict 3-letter uppercase code before saving: read the raw value from $this->request->getPost('currency_code'), trim it, strtoupper it, take the first three characters, validate with a regex /^[A-Z]{3}$/ and if it fails set to '' (or a safe default), then assign the sanitized value (escaped via htmlspecialchars or esc()) into the 'currency_code' key in $batch_save_data so it matches the same normalization applied to other currency-code fields.
1032-1036:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the just-posted
invoice_enablevalue when switching register mode.Lines 1033-1036 branch on
$this->config['invoice_enable'](constructor snapshot), so mode can be computed from stale pre-save state right after a successful save.Proposed fix
+ $invoiceEnabled = $this->request->getPost('invoice_enable') != null; $batch_save_data = [ - 'invoice_enable' => $this->request->getPost('invoice_enable') != null, + 'invoice_enable' => $invoiceEnabled, ... ]; ... if ($success) { - if ($this->config['invoice_enable']) { + if ($invoiceEnabled) { $this->sale_lib->set_mode($this->config['default_register_mode']); } else { $this->sale_lib->set_mode('sale'); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` around lines 1032 - 1036, After a successful save, the current code uses the constructor-snapshot $this->config['invoice_enable'] to decide the register mode, which can be stale; update the success branch so it reads the freshly posted invoice_enable value (e.g. from the request/post payload used to save config) instead of $this->config['invoice_enable'] and then call $this->sale_lib->set_mode(...) with that computed mode; adjust references around the success block where sale_lib->set_mode is invoked so the decision uses the posted invoice_enable value rather than the old $this->config snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Views/sales/customer_display.php`:
- Line 103: The current cell uses the null-coalescing fallback (<?=
esc($customerName ?? lang('Sales.walk_in_customer')) ?>) which fails when
$customerName is an empty string; replace that expression with a trimmed-empty
check so that if trim($customerName) === '' you use
lang('Sales.walk_in_customer') otherwise use $customerName, and pass the
resulting value through esc() before rendering (refer to $customerName,
lang('Sales.walk_in_customer') and esc() in the customer-name-value table cell).
- Around line 144-149: The code builds storage keys using customerDisplayId
which, when missing, yields empty suffix and causes cross-register collisions;
update the logic around customerDisplayId, customerDisplayStorageSuffix and
customerDisplayStorageKeys so that if customerDisplayId === '' you skip any
localStorage/sessionStorage sync (or alternatively generate and use a per-window
unique ID) — i.e., add a guard early that prevents calling the
storage-sync/remove code paths when customerDisplayId is empty and apply the
same guard to the other related blocks (the sections around the existing uses at
lines 151-153 and 188-190) so no shared keys are read/written without a valid
displayId.
---
Outside diff comments:
In `@app/Controllers/Config.php`:
- Around line 373-388: The posted receiving_cost_price_method is not validated;
ensure the value of $receiving_cost_price_method (used in batchSaveData keys
'receiving_cost_price_method' and 'receiving_calculate_average_price') is
explicitly checked against allowed values 'average' and 'new' before persisting:
read the posted value, if it is not exactly 'average' or 'new' fall back to the
existing legacy logic (use receiving_calculate_average_price to decide) or
default to 'new', and set receiving_calculate_average_price based on the
validated final choice; update the validation near where
$receiving_cost_price_method is assigned in the Config controller so only the
sanctioned values are stored.
---
Duplicate comments:
In `@app/Controllers/Config.php`:
- Line 395: The call that builds 'image_allowed_types' uses implode(',',
$this->request->getPost('image_allowed_types')) which can receive null and throw
a TypeError on PHP 8+; change it to guard/normalize the input from
$this->request->getPost('image_allowed_types') (e.g., cast to array or coalesce
to an empty array) before calling implode so implode always receives an array
(update the assignment where 'image_allowed_types' is set in Config.php to use
the normalized value).
- Around line 485-490: The currency_code value is being saved directly from POST
into batch_save_data; normalize and validate it to a strict 3-letter uppercase
code before saving: read the raw value from
$this->request->getPost('currency_code'), trim it, strtoupper it, take the first
three characters, validate with a regex /^[A-Z]{3}$/ and if it fails set to ''
(or a safe default), then assign the sanitized value (escaped via
htmlspecialchars or esc()) into the 'currency_code' key in $batch_save_data so
it matches the same normalization applied to other currency-code fields.
- Around line 1032-1036: After a successful save, the current code uses the
constructor-snapshot $this->config['invoice_enable'] to decide the register
mode, which can be stale; update the success branch so it reads the freshly
posted invoice_enable value (e.g. from the request/post payload used to save
config) instead of $this->config['invoice_enable'] and then call
$this->sale_lib->set_mode(...) with that computed mode; adjust references around
the success block where sale_lib->set_mode is invoked so the decision uses the
posted invoice_enable value rather than the old $this->config snapshot.
🪄 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: 6e6e7fb4-d1e1-4755-8a0e-435599d506dc
📒 Files selected for processing (4)
app/Controllers/Config.phpapp/Controllers/Sales.phpapp/Language/en/Sales.phpapp/Views/sales/customer_display.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Language/en/Sales.php
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Controllers/Config.php (1)
373-388:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
receiving_cost_price_methodagainst an allowlist before saving.Line 373-388 currently accepts any posted string and persists it. Restrict to expected values (
average,new) before storing.Suggested patch
- $receiving_cost_price_method = $this->request->getPost('receiving_cost_price_method'); + $receiving_cost_price_method = (string) $this->request->getPost('receiving_cost_price_method'); + if (!in_array($receiving_cost_price_method, ['average', 'new'], true)) { + $receiving_cost_price_method = ''; + } if ($receiving_cost_price_method === null || $receiving_cost_price_method === '') { $receiving_cost_price_method = $this->request->getPost('receiving_calculate_average_price') != null ? 'average' : 'new'; }As per coding guidelines,
{app/Controllers/**/*.php,app/Models/**/*.php}: “Validate and sanitize all user input”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Controllers/Config.php` around lines 373 - 388, The controller currently takes the posted receiving_cost_price_method value and persists it directly; validate it against an allowlist before saving by checking that $receiving_cost_price_method is one of the allowed strings ('average' or 'new') and if not, fall back to the existing default logic (use receiving_calculate_average_price to choose 'average' else 'new'); apply this validation before assigning into batchSaveData['receiving_cost_price_method'] and ensure batchSaveData['receiving_calculate_average_price'] remains consistent (set it to true only when the validated method === 'average'), referencing the receiving_cost_price_method variable, the request->getPost('receiving_calculate_average_price') check, and the batchSaveData array entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Controllers/Config.php`:
- Around line 485-495: The code assumes $this->request->getPost('language')
contains "code:name" and indexes $exploded[0] and [1] directly; validate and
sanitize the input before indexing by checking the POST value is a non-empty
string and contains a ':' (or use explode with a limit and count check), trim
parts, and fallback to safe defaults if malformed; update the block using the
existing $exploded variable (and the array keys 'language_code' and 'language')
to only assign $exploded[0] and $exploded[1] after verifying count($exploded) >=
2, and apply htmlspecialchars/trim to each part to avoid undefined offsets and
invalid config saves.
---
Outside diff comments:
In `@app/Controllers/Config.php`:
- Around line 373-388: The controller currently takes the posted
receiving_cost_price_method value and persists it directly; validate it against
an allowlist before saving by checking that $receiving_cost_price_method is one
of the allowed strings ('average' or 'new') and if not, fall back to the
existing default logic (use receiving_calculate_average_price to choose
'average' else 'new'); apply this validation before assigning into
batchSaveData['receiving_cost_price_method'] and ensure
batchSaveData['receiving_calculate_average_price'] remains consistent (set it to
true only when the validated method === 'average'), referencing the
receiving_cost_price_method variable, the
request->getPost('receiving_calculate_average_price') check, and the
batchSaveData array entries.
🪄 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: 4d9cc3f3-dc60-4756-96a1-8b268335ab96
📒 Files selected for processing (6)
app/Config/Routes.phpapp/Controllers/Config.phpapp/Controllers/Sales.phpapp/Language/en/Sales.phpapp/Views/sales/customer_display.phpapp/Views/sales/register.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Views/sales/register.php
|
@objecttothis last review let's merge and check since this had alot of changes mixed between shortcut and secondary currency. Let's merge so i can jump and finish the other opened one and i'll stop until merged not to face any conflicts before proceeding with accounting or sales grid section. |
@WebShells I suspect @jekkos will want us to wait to merge on this until after the minor release has been done. @jekkos what do you think? If you have a list of the remaining issues to sort out before we do a minor release, that might allow the other devs to help get it done so we can release. |
objecttothis
left a comment
objecttothis
left a comment
There was a problem hiding this comment.
@WebShells I'm not sure what happened to this PR but Github thinks the whole file has changed, so it's difficult to see what changes you made. Some of my comments might be on code you didn't touch. It's taking me too long to go through this PR because of it thinking the whole file is a difference.
| ] | ||
| ] | ||
| ]; | ||
| $receiving_cost_price_method = $this->request->getPost('receiving_cost_price_method'); |
There was a problem hiding this comment.
All new functions, variables and classes must use PSR compliant naming. Sorry to keep harping on this but we need to unify the code.
|
See what I mean? Every file in the PR looks like this: |
yes that's why i need to finish it and move rather than reworking or getting back to it later, it took ma alot to clear the mess |
|
@objecttothis Am going to do final kdiff to double check the differences between the changed files and current master, fix the last comments, proceed with merging. Let's confirm with @jekkos which release he wants to publish before this one. |
objecttothis
changed the title
@jekkos says he is creating the minor release tomorrow. I want to review your code before we merge. I think it will be OK, but since github is showing me diffs on just about the whole file, it's difficult to do this way. Tomorrow after the merge I'm going to merge Plugins #4407, but that shouldn't cause any merge conflicts because Plugins really only adds Events::trigger() calls into a few Controllers and not much else. |
|
@objecttothis maybe hold back a little merging the plugins as there could be new user bug reports we need to take care of |
|
So I would wait for a little in case something needs to be fixed we can do it with a hotfix. We never really had issues wirh this branching model but if we merge pkugins, then we need to ship plugins along with the hotfix in case that is required |
Understood. I wonder if we should consider a more tiered release system. That would resolve the traffic jam a little. |
|
shall i proceed with this ? or it'll need modification after merging other work ? |
@jekkos needs to create a release after we sort out 1 or 2 minor regressions. Probably Friday. Then he wants us to wait on merging big changes like this a week to give users a chance to report any bugs that can be resolved with a hotfix. If we merge right away after the minor release then it means having to ship a big change with the hotfix. This is why I suggested a tiered beta release workflow as a way to be able to merge big changes but still give buffer for hotfixes after minor releases. |
|
@WebShells I had claude take a look and the reason the diff is so messy is because you have mass line ending changes. If you change your branch back to CRLF line endings and push that, it should make the diff readable. |
|
The other issue is that you've got two changesets in this PR. The Secondary Display is one and the other is receiving_cost_price code changes. Can we separate those out to a different branch and make that a separate PR? With the exception of PSR refactoring and minor code cleanup, we should aim to have one feature set per PR. Unrelated changes bundled into this PR Receiving cost price method feature (new/average toggle for incoming stock cost) |
@objecttothis the receiving cost change is a very minor change just to make it easier for the user to select average or new cost. |
|
OK, let's start with getting the line endings changed back so that it's easier to make sense of the diff. You may be right that receiving cost change is minor but it's also a new feature and with it carries testing requirements to try to prevent unintentional regressions. I'll take a look after the line ending issue gets sorted. |
objecttothis
removed
the
CodeIgniter4
This branch has not been deployed
3 participants
Adds a CI4-aligned secondary display window for the sales, including the controller route, config toggle, language keys, register integration, and the popup view. The feature is intended for a second monitor or customer-facing display and keeps the secondary screen separate from the main sales register.
What was added:
. Added a new Sales::getSecondDisplay() endpoint to render the secondary display as its own popup page.
. Added a Second Display toggle in General configuration so the feature can be enabled or disabled from the UI.
. Added the missing route for /sales/secondDisplay.
. Added the required English language strings for the sales and config screens.
. Added a Second Display button in the sales register action bar when the feature is enabled.
. Added popup-window handling so the second display opens in a separate window instead of reusing the current tab.
. Added refresh hooks so the second display can be updated when the sales cart, customer, or payment state changes.
. Added a dedicated app/Views/sales/second_display.php template for the second monitor UI.
Behavior:
. When enabled, the register shows a Second Display button.
. Clicking it opens a separate popup window for the customer-facing display.
. The popup is rendered from its own view and is not tied to the main register page layout.
. The second display can reflect register changes through the refresh hooks added in the sales flow.
. When the feature is disabled, the register button is hidden and the secondary display is not exposed in the UI.
#2411 #1461 #422
Updated Version of PR #2482
Summary by CodeRabbit
New Features
Bug Fixes
Chores