Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the ability to mix free and paid pages #658

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Remove the ability to mix free and paid pages #658

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
645a6ae
Remove the ability to mix free and paid pages
horcsinbalint Nov 15, 2024
ec468f2
Move multiple copy logic into `planFreePageUsage`
horcsinbalint Nov 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 31 additions & 28 deletions app/Utils/Printer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class Printer
private $number_of_copies;
private $use_free_pages;
private $print_account;
private $free_page_pool = [];
private $free_page_update = [];
private $cost = 0;

public function __construct($filename, $path, $use_free_pages = false, $is_two_sided = true, $number_of_copies = 1)
Expand All @@ -41,49 +41,52 @@ public function print()

// If using free pages, check the amount that can be used
if ($this->use_free_pages) {
$this->calculateFreePagePool();
}

// Calculate cost
$this->cost = PrintAccount::getCost($this->pages, $this->is_two_sided, $this->number_of_copies);
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'));
}
Comment on lines +53 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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...
});


// Print document
return $this->printDocument();
// Print document
return $this->printDocument();
}
Comment on lines +45 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error messaging and transaction safety

Whilst the logic flow is clearer, two concerns need addressing:

  1. The error message print.no_balance doesn't accurately convey that mixing free and paid pages isn't allowed. Consider adding a specific message for this case.
  2. 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.

}

/**
* Only calculating the values here to see how many pages can be covered free of charge.
*/
private function calculateFreePagePool()
private function planFreePageUsage()
{
$this->free_page_pool = [];
$available_pages = 0;
$requested_pages = $this->number_of_copies * $this->pages;
$this->free_page_update = [];
$deducted_pages = 0;
$all_pages = user()->freePages
->where('deadline', '>', Carbon::now())
->sortBy('deadline');

foreach ($all_pages as $key => $free_page) {
if ($available_pages + $free_page->amount >= $this->pages) {
$this->free_page_pool[] = [
'page' => $free_page,
'new_amount' => $free_page->amount - ($this->pages - $available_pages)
];
$available_pages = $this->pages;
break;
}
$this->free_page_pool[] = [
$deduce_from_current = min($requested_pages - $deducted_pages, $free_page->amount);
$this->free_page_update[] = [
'page' => $free_page,
'new_amount' => 0
'new_amount' => $free_page->amount - $deduce_from_current
];
$available_pages += $free_page->amount;
$deducted_pages += $deduce_from_current;
if($deducted_pages == $requested_pages) {
break;
}
}

$this->pages -= $available_pages;
return $deducted_pages == $requested_pages;
}

private function printDocument()
Expand All @@ -95,7 +98,7 @@ private function printDocument()

// Update print account history
$this->print_account->update(['last_modified_by' => user()->id]);
foreach ($this->free_page_pool as $fp) {
foreach ($this->free_page_update as $fp) {
$fp['page']->update([
'amount' => $fp['new_amount'],
'last_modified_by' => user()->id
Expand Down
1 change: 1 addition & 0 deletions resources/lang/en/print.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
'number_of_copies' => 'Number of copies',
'number_of_printed_documents' => 'Number of printed documents',
'options' => 'Printing options',
'payment_methods_cannot_be_mixed' => 'A print job can be either free or paid as they cannot be mixed.',
'pdf_description' => 'Only .pdf files can be printed.',
'pdf_maxsize' => 'The maximum file size is :maxsize MB.',
'print' => 'Print',
Expand Down
1 change: 1 addition & 0 deletions resources/lang/hu/print.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
'number_of_copies' => 'Példányszám',
'number_of_printed_documents' => 'Nyomtatott dokumentumok száma',
'options' => 'Beállítások',
'payment_methods_cannot_be_mixed' => 'Egy nyomtatás vagy ingyenes vagy fizetős lehet, nem keverhető.',
'pdf_description' => 'Csak .pdf fájl nyomtatható.',
'pdf_maxsize' => 'A fájl mérete legfeljebb :maxsize MB lehet.',
'print' => 'Nyomtatás',
Expand Down
3 changes: 3 additions & 0 deletions resources/views/dormitory/print/print.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
@lang('print.available_money'): <b class="coli-text text-orange"> {{ user()->printAccount->balance }}</b> HUF.
@lang('print.upload_money')
</p>
<p>
@lang('print.payment_methods_cannot_be_mixed')
</p>
</blockquote>
<form class="form-horizontal" role="form" method="POST" action="{{ route('print.print') }}"
enctype="multipart/form-data">
Expand Down