Remove the ability to mix free and paid pages #658
Conversation
There is a quite annoying bug where if a user prints using free pages, their free page pool is only decreased by p and not n*p (where n is the number of copies, and p is the number of pages in the document). The quickest and easiest way of dealing with it is to only allow a print job to be fully paid or entirely paid and calculate the printed pages correctly. This bug is quite annoying as our registry shows only a fraction of the actual usage (i.e. 5000 pages of plain pages was loaded into the printer, and our Mars instance shows only a few printed pages).
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes made in this pull request primarily focus on enhancing the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
resources/views/dormitory/print/print.blade.php (1)
13-15: Splendid addition of the informational message, though might I suggest a slight enhancement?The message placement is quite appropriate, informing users about the payment method restriction. However, considering the significance of this change, we might want to make it more prominent.
Consider this enhancement:
- <p> - @lang('print.payment_methods_cannot_be_mixed') - </p> + <p class="coli-text text-orange"> + <strong>@lang('print.payment_methods_cannot_be_mixed')</strong> + </p>app/Utils/Printer.php (1)
52-66: Simplify the control flow to avoid duplication.The current structure duplicates the call to
printDocument(). We can simplify this by restructuring the control flow.Consider this more concise structure:
if ($this->use_free_pages) { $this->pages *= $this->number_of_copies; $this->calculateFreePagePool(); if ($this->pages > 0) { return back()->withInput()->with('error', __('print.no_mixed_payment_allowed')); } - return $this->printDocument(); } else { $this->cost = PrintAccount::getCost($this->pages, $this->is_two_sided, $this->number_of_copies); if (!$this->print_account->hasEnoughMoney($this->cost)) { return back()->withInput()->with('error', __('print.no_balance')); } - return $this->printDocument(); } +return $this->printDocument();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/Utils/Printer.php(1 hunks)resources/lang/en/print.php(1 hunks)resources/lang/hu/print.php(1 hunks)resources/views/dormitory/print/print.blade.php(1 hunks)
🔇 Additional comments (4)
resources/views/dormitory/print/print.blade.php (1)
Line range hint 26-31: Verify the behaviour of the free pages checkbox, good chap
While we've added a message about not mixing payment methods, the form still includes the free pages checkbox. We should ensure this doesn't create confusion for users.
Let's check if there are any JavaScript handlers or controllers managing this interaction:
Consider implementing one of these approaches:
- Add JavaScript to disable the "Print" button if users attempt to mix payment methods
- Split this into two separate forms - one for free printing and one for paid printing
✅ Verification successful
The free pages checkbox behaviour is properly implemented and safeguarded
I can confirm that the system has proper safeguards in place to prevent mixing of payment methods:
- The warning message about not mixing payment methods is clearly displayed to users.
- The
Printerutility class has dedicated logic incalculateFreePagePool()that handles free pages separately from paid printing. - If a user attempts to print more pages than their free page allocation, the system returns with a "no balance" error rather than falling back to paid printing.
The implementation ensures that a print job must be either entirely free or entirely paid, preventing any confusion or unintended mixing of payment methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any JavaScript files handling the print form
rg -t js "use_free_pages|print\.print"
# Search for any controllers handling the print action
ast-grep --pattern 'class $_Controller {
$$$
print($$$) {
$$$
}
$$$
}'
Length of output: 125
Script:
#!/bin/bash
# Let's search for any PHP controllers handling the print functionality
fd -e php Controller | xargs rg -l "print"
# Search for any JavaScript files related to printing or payment
fd -e js | xargs rg -l "print|payment"
# Look for any Blade templates related to printing
fd "print.*blade.php"
# Check for any route definitions related to printing
fd -e php "routes" | xargs rg "print"
# Search for any validation or request handling for free pages
fd -e php | xargs rg -l "use_free_pages"
Length of output: 97742
Script:
#!/bin/bash
# Let's examine the PrintController to understand the free pages logic
rg -A 10 "use_free_pages" app/Http/Controllers/Dormitory/PrintController.php
# Check the Printer utility class for free pages handling
rg -A 10 "use_free_pages" app/Utils/Printer.php
# Look for any validation or business logic related to free pages
rg -A 5 "hasFreePage|getFreePages" app/Models/
Length of output: 1806
Script:
#!/bin/bash
# Let's check for any free pages calculation logic
rg -A 10 "calculateFreePagePool" app/Utils/Printer.php
# Look for any models related to free pages
fd -e php | xargs rg -l "class.*FreePages"
# Check for any validation rules or middleware that might handle free pages
rg -A 5 "payment_methods_cannot_be_mixed" resources/lang/
Length of output: 2130
resources/lang/en/print.php (1)
46-46: Splendid addition to the language resources!
The new translation entry is well-crafted, maintaining consistency with existing conventions whilst clearly communicating the new restriction on mixing payment methods. The message is appropriately positioned and properly formatted.
resources/lang/hu/print.php (1)
46-46: Splendid addition, old chap! Let's verify the translation consistency.
The new translation entry effectively communicates the payment method restriction, which aligns rather well with the PR objectives.
Let's ensure the translation is consistent with its English counterpart:
✅ Verification successful
Splendid translation work, indeed!
I've had the pleasure of comparing both translations, and I must say they're perfectly aligned in meaning whilst maintaining natural language flow in both English and Hungarian. The Hungarian version "Egy nyomtatás vagy ingyenes vagy fizetős lehet, nem keverhető" conveys the same message as the English "A print job can be either free or paid as they cannot be mixed", maintaining both semantic accuracy and cultural appropriateness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare Hungarian and English translations for consistency
# Check if the English translation exists and matches the context
rg -U "payment_methods_cannot_be_mixed" resources/lang/en/print.php
Length of output: 175
app/Utils/Printer.php (1)
44-45: Splendid fix for the page calculation!
This change properly accounts for the total number of pages by multiplying with the number of copies before checking the free page pool. This resolves the core issue where the system wasn't correctly deducting pages for multiple copies.
| // Calculate cost | ||
| $this->cost = PrintAccount::getCost($this->pages, $this->is_two_sided, $this->number_of_copies); | ||
|
|
||
| // Check balance | ||
| if (!$this->print_account->hasEnoughMoney($this->cost)) { | ||
| return back()->withInput()->with('error', __('print.no_balance')); | ||
| } |
There was a problem hiding this comment.
Guard against potential race conditions in balance checks.
There's a risk of race conditions between checking the balance and actually deducting the cost. If a user initiates multiple print jobs simultaneously, they could potentially exceed their balance.
Consider implementing a database transaction or optimistic locking:
DB::transaction(function () {
$account = PrintAccount::lockForUpdate()->find($this->print_account->id);
if (!$account->hasEnoughMoney($this->cost)) {
throw new InsufficientBalanceException();
}
// Continue with printing...
});
app/Utils/Printer.php
Outdated
| if ($this->pages > 0) { | ||
| return back()->withInput()->with('error', __('print.no_balance')); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more precise error message.
The current error message print.no_balance doesn't quite capture the actual scenario. When $this->pages > 0 after free page pool calculation, it means the user is attempting to mix free and paid pages, rather than having insufficient balance.
Consider adding a new, more specific translation key:
- return back()->withInput()->with('error', __('print.no_balance'));
+ return back()->withInput()->with('error', __('print.no_mixed_payment_allowed'));📝 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.
| if ($this->pages > 0) { | |
| return back()->withInput()->with('error', __('print.no_balance')); | |
| } | |
| if ($this->pages > 0) { | |
| return back()->withInput()->with('error', __('print.no_mixed_payment_allowed')); | |
| } |
|
@coderabbitai full review |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/Utils/Printer.php (2)
69-89: Add input validation and improve documentationThe method handles the core logic well, but could benefit from additional safeguards and documentation.
Consider these improvements:
/** * Only calculating the values here to see how many pages can be covered free of charge. + * + * @return bool True if all requested pages can be covered by free pages, false otherwise + * @throws \InvalidArgumentException If input parameters are invalid */ private function planFreePageUsage() { + if ($this->number_of_copies <= 0 || $this->pages <= 0) { + throw new \InvalidArgumentException('Invalid number of copies or pages'); + } + $requested_pages = $this->number_of_copies * $this->pages; + + // Guard against integer overflow + if ($requested_pages < 0 || $requested_pages < $this->number_of_copies) { + throw new \InvalidArgumentException('Page calculation overflow'); + }
Line range hint
101-106: Ensure atomic updates with transactionThe updates to free pages and account balance should be wrapped in a transaction to maintain data consistency.
Consider this implementation:
-foreach ($this->free_page_update as $fp) { - $fp['page']->update([ - 'amount' => $fp['new_amount'], - 'last_modified_by' => user()->id - ]); -} - -// Update print account -$this->print_account->decrement('balance', $this->cost); +DB::transaction(function () { + foreach ($this->free_page_update as $fp) { + $fp['page']->update([ + 'amount' => $fp['new_amount'], + 'last_modified_by' => user()->id + ]); + } + + // Update print account + $this->print_account->decrement('balance', $this->cost); +});
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/Utils/Printer.php(3 hunks)
🔇 Additional comments (1)
app/Utils/Printer.php (1)
21-21: Excellent variable renaming choice!
The new name free_page_update better conveys the variable's purpose of tracking pending updates to free pages.
| if (!$this->planFreePageUsage()) { | ||
| return back()->withInput()->with('error', __('print.no_balance')); | ||
| } | ||
|
|
||
| // Check balance | ||
| if (!$this->print_account->hasEnoughMoney($this->cost)) { | ||
| return back()->withInput()->with('error', __('print.no_balance')); | ||
| } | ||
| // Print document | ||
| return $this->printDocument(); | ||
| } else { | ||
|
|
||
| // Calculate cost | ||
| $this->cost = PrintAccount::getCost($this->pages, $this->is_two_sided, $this->number_of_copies); | ||
|
|
||
| // Check balance | ||
| if (!$this->print_account->hasEnoughMoney($this->cost)) { | ||
| return back()->withInput()->with('error', __('print.no_balance')); | ||
| } | ||
|
|
||
| // Print document | ||
| return $this->printDocument(); | ||
| // Print document | ||
| return $this->printDocument(); | ||
| } |
There was a problem hiding this comment.
Improve error messaging and transaction safety
Whilst the logic flow is clearer, two concerns need addressing:
- The error message
print.no_balancedoesn't accurately convey that mixing free and paid pages isn't allowed. Consider adding a specific message for this case. - The balance check remains susceptible to race conditions in concurrent scenarios.
Consider this implementation:
if ($this->use_free_pages) {
- if (!$this->planFreePageUsage()) {
- return back()->withInput()->with('error', __('print.no_balance'));
+ if (!$this->planFreePageUsage()) {
+ return back()->withInput()->with('error', __('print.no_mixed_payment_allowed'));
}
} else {
// Calculate cost
$this->cost = PrintAccount::getCost($this->pages, $this->is_two_sided, $this->number_of_copies);
- // Check balance
- if (!$this->print_account->hasEnoughMoney($this->cost)) {
- return back()->withInput()->with('error', __('print.no_balance'));
- }
+ // Wrap in transaction to prevent race conditions
+ return DB::transaction(function () {
+ $account = PrintAccount::lockForUpdate()->find($this->print_account->id);
+ if (!$account->hasEnoughMoney($this->cost)) {
+ return back()->withInput()->with('error', __('print.no_balance'));
+ }
+ return $this->printDocument();
+ });
}Committable suggestion skipped: line range outside the PR's diff.
Description
There is a quite annoying bug where if a user prints using free pages, their free page pool is only decreased by p and not by n*p (where n is the number of copies, and p is the number of pages in the document). The quickest and easiest way of dealing with it is to only allow a print job to be fully paid or entirely free and deduct the printed pages correctly. The solution is not ideal but it is quite a rare occurrence where someone would like to use both free pages and pay for the remainder. (They can split it themselves.)
This bug is quite annoying as our registry shows only a fraction of the actual usage. I.e. ~ 5000 sheets was loaded into the printer during this semester, and our Mars instance shows only a few hundred printed pages. It would be crucial to understand how many pages were printed for different kinds of activities as it has already eaten roughly a quarter of our budget of the semester in half-a-semester.
Changes
Additional Notes
I am not refactoring the printer module for this PR.
To allow mixing these methods, the
PrintAccountclass would also need to be refactored but I do not think that it is worth the time to be invested as we will need additional refactoring as we are planning on introducing multiple printers.