Print module refactor #673
Conversation
…rs into print-module-refactor
…rs into print-module-refactor
…rs into print-module-refactor
…rs into print-module-refactor
…rs into print-module-refactor
…rs into print-module-refactor
It has been extremely useful several times that we had commands of successful and failed commands.
Use filter_var instead of regex. Coderabbit suggested the change. I do not agree with the reasoning coderabbit gave, it was functional, this is simply more readable.
Added a missing Ft to Hungarian email and also emailing the recipient.
Send emails to each individual of all of the balance changes. (For better transparency.)
Using the printing filesystem for printing. In case of free pages we are charging for each side we need to put toner on. We try to minimize our paper usage by automatically selecting double-sided printing.
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive overhaul of the print management system, replacing the existing print-related functionality with a more modular and structured approach. The changes span across multiple files, introducing new controllers, models, enums, and routing configurations while removing the previous monolithic Changes
Sequence DiagramsequenceDiagram
participant User
participant PrintJobController
participant PrintAccount
participant Printer
participant PrintJob
User->>PrintJobController: Request to print document
PrintJobController->>PrintAccount: Check balance/free pages
PrintAccount-->>PrintJobController: Confirm availability
PrintJobController->>Printer: Create print job
Printer->>PrintJob: Generate print job record
Printer->>PrintJob: Execute printing process
PrintJob-->>PrintJobController: Return job status
PrintJobController-->>User: Confirm print job
Possibly related issues
Possibly related PRs
Suggested reviewers
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: 5
🧹 Nitpick comments (30)
app/Policies/PrintAccountPolicy.php (1)
43-52: My dear colleague, please consider using a strict comparison operator in your newly introducedtransferBalancemethod. While the current code suffices for many use cases, a strict comparison helps ensure that both the data type and value match, thereby reducing the risk of subtle type-coercion issues.Here is a suggested change:
- return $user->id == $printAccount->user_id; + return $user->id === $printAccount->user_id;resources/views/dormitory/print/free.blade.php (1)
Line range hint
30-45: Nicely done on integrating admin-only columns for created time and user details.
It is quite beneficial for administrators to have an overview of when entries were created and by which user. Should there be scenarios where these fields can be null, consider using a conditional check or fallback text to ensure the table remains readable.database/migrations/2019_10_13_130718_create_roles_table.php (2)
24-24: Defer to a single source of truth for role definitions.My dear colleague, this hardcoded array bypasses the role constants in your model or configuration, potentially leading to discrepancies if the roles defined here ever drift from the canonical list. Kindly consider referencing a single source of truth for role definitions to keep your codebase consistently updated.
25-25: Consider avoiding potential duplicates.When inserting roles, you might adopt a pattern like
firstOrCreateto avoid re-inserting roles that already exist or handle potential duplicates. This ensures your data migrations remain idempotent if run multiple times.resources/views/dormitory/print/print.blade.php (2)
1-9: Splendid documentation of variables!The documentation clearly explains the available variables. However, might I suggest adding type hints in the documentation for enhanced clarity?
/** * Variables: - * $user -> the current user - * $users -> all users, used to transfer money between print accounts - * $printer -> the model corresponding to the printer, which is used to print (currently only one is available at all times) + * $user User -> the current user + * $users Collection<User> -> all users, used to transfer money between print accounts + * $printer Printer -> the model corresponding to the printer, which is used to print (currently only one is available at all times) */
32-35: Consider improving the readability of the free pages conditionWhile the logic is correct, we could make the condition more explicit.
-@if($printAccount->availableFreePages()->sum('amount') > 0) {{-- only show if the user has active free pages --}} +@php + $availableFreePagesCount = $printAccount->availableFreePages()->sum('amount'); +@endphp +@if($availableFreePagesCount > 0)app/Models/PrintAccountHistory.php (1)
50-55: Confirm user and modifier relationshipsThe relationship definitions appear correct. Kindly ensure that
App\Models\Useris properly imported (e.g.,use App\Models\User;) so that the references toUser::classwill be recognised without any ambiguity.app/Console/Commands.php (1)
28-30: Error handling for PDF generation
At present, failures duringpdflatexexecution are not captured. Kindly consider throwing an exception or logging an error if the process is not successful.Possible approach:
$process->run(); + if (!$process->isSuccessful()) { + Log::error("Failed to generate PDF: " . $process->getErrorOutput()); + throw new \RuntimeException("PDF generation failed"); + }app/Models/FreePages.php (1)
117-123: Potential concurrency issue
If multiple processes attemptsubtractPagessimultaneously, race conditions or inconsistent data might arise. For robust concurrency, consider a locking mechanism or wrapping this in a DB transaction.app/Models/Printer.php (2)
57-69: Creating a print job
Nicely structured approach. Do consider additional validation to ensure the file path is accessible before invoking the printing process.
79-112: Printing logic
The approach is correct. However, returning-1to denote a debug scenario may confuse downstream logic. You could throw an exception or return a more descriptive error code.app/Models/PrintJob.php (2)
97-105: Check localisation intranslatedCost
This method returns a string mixing Hungarian and currency text. Do consider localising the phrase"ingyenes oldal"for a fully translated experience.
122-122: Correct the PHPDoc annotation
@param PrintJob $thisis invalid. A docblock should not describe$thisas a parameter. Consider removing or revising that line.- * @param PrintJob $this🧰 Tools
🪛 phpstan (2.0.3)
122-122: PHPDoc tag @param has invalid value (PrintJob $this): Unexpected token "$this", expected variable at offset 108
(phpDoc.parseError)
app/Http/Controllers/Dormitory/Printing/PrintJobController.php (1)
79-94: File handling withinstore
Validation, retrieval, and storing appear consistent. Consider how the system behaves ifPrinter::firstWhere('name', ...)fails to find a matching entity. An explicit error response could help.app/Http/Controllers/Dormitory/Printing/PrintAccountController.php (2)
60-101:transferBalancemethod
Validation for not transferring to oneself and preventing negative or insufficient balance is prudent. Kindly ensure that email notifications accurately reflect the transaction details (notably the negative and positive amounts).
109-146:modifyBalancemethod
This handles admin-only modifications well. Consider auditing modifications for traceability—particularly who authorised each change.app/Enums/PrintJobStatus.php (1)
5-11: Consider either documenting or removing the commented-out statuses, dear colleague.
They appear to be no longer used, and retaining them may cause confusion for future developers.config/print.php (1)
5-6: Kindly consider providing default values for these environment variables, if you please.
This ensures that your application behaves predictably even if the environment variables are not set.database/factories/PrintJobFactory.php (1)
16-16: Well done adding the default printer ID
Setting'printer_id' => 1as a default is perfectly reasonable for testing. However, do keep in mind that if the production environment has multiple printers, you might wish to randomise this ID, or link it to a proper Printer model factory, thereby ensuring broader coverage.resources/views/dormitory/print/send.blade.php (1)
13-13: Adding a hidden input for the current user is a deft touch, allowing the controller to cleanly differentiate between the sender and recipient.resources/views/user/printing.blade.php (1)
21-21: Replacing method calls with property fetch formodifieris a neat improvement.This ensures clarity in the relationship usage, but be cautious if future changes require model method calls for preliminary logic. A property alone may become insufficient in more complex scenarios.
resources/views/dormitory/print/manage/account_history.blade.php (1)
17-17: Fielduser.nameis an elegant change.
Explicitly referencing nested data fosters clarity and aligns with common best practices in Blade files.app/Http/Controllers/Dormitory/Printing/PrinterController.php (1)
44-63: Update method ensures paper availability tracking.
The logic for notifying administrators after 30 minutes is admirably prudent. Please confirm that you have logs or metrics to track these notifications in case of repeated out-of-paper states.app/Utils/PrinterHelper.php (3)
51-52: Align parameter types in the docblock with the method signature.
It appears the docblock states$copiesand$twoSidedare mixed, yet the method acceptsintandbool, respectively. Kindly update the docblock to maintain consistency and clarity.- * @param mixed $copies - * @param mixed $twoSided + * @param int $copies + * @param bool $twoSided
70-70: Revise or remove extraneous@throwstags if not applicable.
You reference exceptions in the docblock (BindingResolutionException,ContainerExceptionInterface, etc.) that might not actually be thrown by this method. If these exceptions cannot arise here any longer, removing or adjusting these annotations would neaten the documentation.🧰 Tools
🪛 phpstan (2.0.3)
70-70: PHPDoc tag @throws with type App\Utils\BindingResolutionException|App\Utils\ContainerExceptionInterface|App\Utils\NotFoundExceptionInterface is not subtype of Throwable
(throws.notThrowable)
83-85: Efficiently retrieve printers for the loop.
Here, you repeatedly invokePrinter::find($printer_id)in a loop. Should you need more data within the loop, consider retrieving records in bulk for improved performance.tests/Feature/PrintControllerTest.php (2)
73-74: Useful TODO noted here.Follow up on the commented-out test to confirm it remains relevant.
108-109: Still commented-out test blocks.Revisit the commented lines to ensure consistency with the new design.
.env.example (2)
74-78: Kindly enhance the documentation for printer configuration variablesI notice the printer configuration lacks proper documentation regarding the expected format and validation requirements. Consider adding comments to specify:
- The expected IP address format (IPv4/IPv6)
- Valid port range
- Whether these values are mandatory for the migration process
#Data used during migration in 2023_12_25_182310_create_printers_table.php to create a new printer -PRINTER_NAME=NemUjBela -PRINTER_IP= -PRINTER_PORT= +# The name of the printer as it appears in the system +PRINTER_NAME=NemUjBela +# IPv4 address of the printer (e.g., 192.168.1.100) +PRINTER_IP= +# Printer port number (typically 9100 for network printers) +PRINTER_PORT=
93-93: Consider improving the debug mode configuration placement and documentationI observe that this security-related configuration is separated from its related command definitions (LPSTAT_COMMAND, CANCEL_COMMAND, etc.). To enhance maintainability:
- Consider moving this configuration closer to the related command definitions
- Add documentation explaining the security implications
LPSTAT_COMMAND=lpstat CANCEL_COMMAND=cancel PDFINFO_COMMAND=pdfinfo PING_COMMAND=ping PDFLATEX_COMMAND=/usr/bin/pdflatex +# Controls whether system commands are actually executed in debug mode +# Set to false in production for security +RUN_COMMANDS_IN_DEBUG_MODE=false -RUN_COMMANDS_IN_DEBUG_MODE=false
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.env.ci(1 hunks).env.example(2 hunks)app/Console/Commands.php(2 hunks)app/Enums/PrintJobStatus.php(1 hunks)app/Enums/PrinterCancelResult.php(1 hunks)app/Http/Controllers/Auth/RegisterController.php(0 hunks)app/Http/Controllers/Dormitory/PrintController.php(0 hunks)app/Http/Controllers/Dormitory/Printing/FreePagesController.php(1 hunks)app/Http/Controllers/Dormitory/Printing/PrintAccountController.php(1 hunks)app/Http/Controllers/Dormitory/Printing/PrintAccountHistoryController.php(1 hunks)app/Http/Controllers/Dormitory/Printing/PrintJobController.php(1 hunks)app/Http/Controllers/Dormitory/Printing/PrinterController.php(1 hunks)app/Http/Controllers/Secretariat/DocumentController.php(1 hunks)app/Mail/ChangedPrintBalance.php(2 hunks)app/Mail/NoPaper.php(1 hunks)app/Models/FreePages.php(2 hunks)app/Models/PrintAccount.php(2 hunks)app/Models/PrintAccountHistory.php(2 hunks)app/Models/PrintJob.php(2 hunks)app/Models/Printer.php(1 hunks)app/Models/Role.php(0 hunks)app/Models/User.php(0 hunks)app/Policies/FreePagesPolicy.php(0 hunks)app/Policies/PrintAccountPolicy.php(1 hunks)app/Policies/PrintJobPolicy.php(0 hunks)app/Utils/Printer.php(0 hunks)app/Utils/PrinterHelper.php(1 hunks)app/Utils/Process.php(1 hunks)config/commands.php(1 hunks)config/print.php(1 hunks)database/factories/PrintJobFactory.php(2 hunks)database/migrations/2019_10_06_224327_create_print_jobs_table.php(2 hunks)database/migrations/2019_10_13_130718_create_roles_table.php(1 hunks)database/migrations/2023_12_25_182310_create_printers_table.php(1 hunks)database/migrations/2023_12_25_184733_update_print_jobs_table.php(1 hunks)database/migrations/2024_12_25_112824_remove_printer_role.php(1 hunks)database/seeders/UsersTableSeeder.php(2 hunks)resources/lang/en/print.php(3 hunks)resources/lang/hu/print.php(3 hunks)resources/views/dormitory/print/app.blade.php(1 hunks)resources/views/dormitory/print/free.blade.php(3 hunks)resources/views/dormitory/print/history.blade.php(3 hunks)resources/views/dormitory/print/manage/account_history.blade.php(2 hunks)resources/views/dormitory/print/manage/app.blade.php(1 hunks)resources/views/dormitory/print/manage/free.blade.php(1 hunks)resources/views/dormitory/print/manage/modify.blade.php(1 hunks)resources/views/dormitory/print/print.blade.php(2 hunks)resources/views/dormitory/print/send.blade.php(1 hunks)resources/views/emails/changed_print_balance.blade.php(1 hunks)resources/views/emails/no_paper.blade.php(1 hunks)resources/views/layouts/navbar.blade.php(2 hunks)resources/views/secretariat/document/import.blade.php(0 hunks)resources/views/secretariat/document/index.blade.php(0 hunks)resources/views/user/printing.blade.php(1 hunks)routes/web.php(2 hunks)tests/Feature/PrintControllerTest.php(5 hunks)tests/Feature/RegistrationTest.php(0 hunks)tests/Unit/PrintTest.php(1 hunks)
💤 Files with no reviewable changes (10)
- resources/views/secretariat/document/index.blade.php
- resources/views/secretariat/document/import.blade.php
- tests/Feature/RegistrationTest.php
- app/Models/User.php
- app/Http/Controllers/Auth/RegisterController.php
- app/Policies/PrintJobPolicy.php
- app/Policies/FreePagesPolicy.php
- app/Models/Role.php
- app/Utils/Printer.php
- app/Http/Controllers/Dormitory/PrintController.php
✅ Files skipped from review due to trivial changes (2)
- resources/views/emails/no_paper.blade.php
- .env.ci
🧰 Additional context used
🪛 phpstan (2.0.3)
app/Http/Controllers/Dormitory/Printing/PrintAccountHistoryController.php
29-29: Relation 'modifier' is not found in App\Models\PrintAccountHistory model.
(larastan.relationExistence)
29-29: Relation 'user' is not found in App\Models\PrintAccountHistory model.
(larastan.relationExistence)
app/Utils/Process.php
55-55: PHPDoc tag @param references unknown parameter: $debugOutput
(parameter.notFound)
app/Console/Commands.php
21-21: Parameter #1 $callback of method App\Utils\Process::run() expects (callable(): mixed)|null, false given.
(argument.type)
app/Models/PrintJob.php
122-122: PHPDoc tag @param has invalid value (PrintJob $this): Unexpected token "$this", expected variable at offset 108
(phpDoc.parseError)
141-141: Access to an undefined property Illuminate\Database\Eloquent\Model::$amount.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
(property.notFound)
app/Http/Controllers/Dormitory/Printing/PrintJobController.php
62-62: Relation 'user' is not found in App\Models\PrintJob model.
(larastan.relationExistence)
118-118: Access to an undefined property Illuminate\Database\Eloquent\Model::$job_id.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
(property.notFound)
178-178: Call to an undefined method Illuminate\Support\Collection<(int|string), mixed>::append().
(method.notFound)
app/Models/FreePages.php
82-82: PHPDoc tag @return with type bool is incompatible with native type Illuminate\Database\Eloquent\Casts\Attribute.
(return.phpDocType)
app/Utils/PrinterHelper.php
55-55: PHPDoc tag @param for parameter $copies with type mixed is not subtype of native type int.
(parameter.phpDocType)
55-55: PHPDoc tag @param for parameter $twoSided with type mixed is not subtype of native type bool.
(parameter.phpDocType)
70-70: PHPDoc tag @throws with type App\Utils\BindingResolutionException|App\Utils\ContainerExceptionInterface|App\Utils\NotFoundExceptionInterface is not subtype of Throwable
(throws.notThrowable)
app/Models/PrintAccount.php
135-135: Argument of an invalid type App\Models\FreePages supplied for foreach, only iterables are supported.
(foreach.nonIterable)
135-135: PHPDoc tag @var above foreach loop does not specify variable name.
(varTag.noVariable)
app/Models/Printer.php
138-138: Result of static method Illuminate\Database\Connection::transaction() (void) is used.
(staticMethod.void)
🪛 GitHub Check: phpunit
app/Http/Controllers/Dormitory/Printing/PrintAccountHistoryController.php
[failure] 29-29:
Relation 'modifier' is not found in App\Models\PrintAccountHistory model.
[failure] 29-29:
Relation 'user' is not found in App\Models\PrintAccountHistory model.
app/Console/Commands.php
[failure] 21-21:
Parameter #1 $callback of method App\Utils\Process::run() expects (callable(): mixed)|null, false given.
app/Http/Controllers/Dormitory/Printing/PrintJobController.php
[failure] 62-62:
Relation 'user' is not found in App\Models\PrintJob model.
[failure] 118-118:
Access to an undefined property Illuminate\Database\Eloquent\Model::$job_id.
[failure] 178-178:
Call to an undefined method Illuminate\Support\Collection<(int|string), mixed>::append().
app/Models/FreePages.php
[failure] 82-82:
PHPDoc tag @return with type bool is incompatible with native type Illuminate\Database\Eloquent\Casts\Attribute.
app/Models/PrintAccount.php
[failure] 135-135:
Argument of an invalid type App\Models\FreePages supplied for foreach, only iterables are supported.
🔇 Additional comments (149)
resources/views/dormitory/print/free.blade.php (2)
57-57: Good consistency in applying the date formatter to the deadline column.
This ensures uniform date displays across the application. Well played.
62-62: Verifying that the ‘modifier.name’ field is always populated.
If there is any possibility that modifier might be null (e.g. no record of who modified it), you may wish to gracefully handle such cases to prevent runtime errors or confusing blank cells.
resources/views/dormitory/print/print.blade.php (2)
17-17: Excellent use of configuration values!
The change to use configuration values for costs and the new print account variable makes the code more maintainable and consistent.
Also applies to: 19-19
45-46: Consider potential race conditions in paper status updates
The paper status check using paper_out_at is straightforward, but do be mindful of potential race conditions when multiple users report paper status changes simultaneously.
Consider implementing:
- Optimistic locking for paper status updates
- A timestamp-based verification before applying changes
- A proper error message when concurrent updates occur
Also applies to: 52-65
app/Mail/NoPaper.php (1)
19-19: Kindly verify references to the removed $recipient property within templates or downstream code.
Well done on removing the redundant $recipient parameter. However, do ensure this change does not break any existing references, for instance in your Blade templates or downstream calls, that might still expect a $recipient.
Here is a script to help detect any stray references to $recipient:
app/Http/Controllers/Dormitory/Printing/PrintAccountHistoryController.php (3)
14-14: Splendid modular design introduction
The controller name clearly conveys its responsibility. Creating specialised controllers for isolated functionalities is an excellent demonstration of single responsibility.
23-25: Ensure consistent authorisation checks
You have invoked $this->authorize('viewAny', PrintJob::class). Kindly verify whether the same permission properly applies to viewing account history, or if a dedicated policy for PrintAccountHistory is warranted.
27-32: Relation mismatch verification
Despite the static analysis warning, the user and modifier relationships do exist in the PrintAccountHistory model. This might be a false positive or a class import/path discrepancy. Kindly confirm that the model has the necessary imports, or review your static analysis tool’s configuration.
🧰 Tools
🪛 phpstan (2.0.3)
29-29: Relation 'modifier' is not found in App\Models\PrintAccountHistory model.
(larastan.relationExistence)
29-29: Relation 'user' is not found in App\Models\PrintAccountHistory model.
(larastan.relationExistence)
🪛 GitHub Check: phpunit
[failure] 29-29:
Relation 'modifier' is not found in App\Models\PrintAccountHistory model.
[failure] 29-29:
Relation 'user' is not found in App\Models\PrintAccountHistory model.
app/Models/PrintAccountHistory.php (1)
8-8: Clarification on observers
Your comment indicates that observers are set up for updating entries rather than database triggers. This approach is indeed more flexible in Laravel. Splendid explanation that helps maintainers understand the design decision.
app/Console/Commands.php (2)
5-5: Imports appear in good order
These additions ensure that the Process utility class and logging capabilities are readily available.
16-17: Validation for IP address is appropriately handled
This check prevents invalid IP addresses from being processed. Excellent precaution.
app/Utils/Process.php (3)
17-32: Conditional command execution is quite clear
The logic to skip execution in debug mode is helpful. However, bear in mind that $process->run() accepts $callback and $env as parameters. Ensure that the correct argument order is passed if you call it inline.
40-47: Conditional output retrieval
Returning debug output as a fallback in debug mode is neatly handled. Good job ensuring the logs remain consistent.
55-63: 🛠️ Refactor suggestion
Remove or correct the $debugOutput param in the docblock
There is no parameter $debugOutput in getExitCode, yet it is mentioned in the docblock. Kindly update the docblock to reflect the actual signature, preventing confusion.
Proposed fix:
- * @param string $debugOutputLikely invalid or redundant comment.
🧰 Tools
🪛 phpstan (2.0.3)
55-55: PHPDoc tag @param references unknown parameter: $debugOutput
(parameter.notFound)
app/Models/FreePages.php (1)
56-58: Casting the deadline attribute
Marking deadline as a date is spot on, ensuring it is always treated as a date instance.
app/Models/Printer.php (5)
21-22: Timestamps disabled
Disabling $timestamps is acceptable if you do not track creation or update times.
30-32: Casting paper_out_at
Casting this field to a datetime aids clarity when comparing or manipulating the date.
38-41: Relationship with PrintJob
A standard hasMany relationship is correctly established. All looks in order.
120-131: Retrieving completed print jobs
Looks fine. Consider verifying that $result truly holds valid job references before returning them.
136-144: Transaction usage
When you return DB::transaction(...), ensure the callback returns a meaningful value, otherwise the outer method might always return null.
🧰 Tools
🪛 phpstan (2.0.3)
138-138: Result of static method Illuminate\Database\Connection::transaction() (void) is used.
(staticMethod.void)
app/Models/PrintAccount.php (7)
62-62: User relationship
Associating the user via belongsTo(User::class) is good. Ensure any user-level constraints (like soft deletes) are handled as needed.
67-67: Has many relationship with FreePages
You are referencing 'user_id' as both local and foreign key. This is consistent with your primary key arrangement.
74-76: Retrieving available free pages
Sorting by deadline is a sensible approach, ensuring earliest-expiring entries are considered first.
79-91: Checking free page sufficiency
The comparison with PrinterHelper::getFreePagesNeeded is consistent. No issues spotted.
93-103: Checking monetary balance
Utilising PrinterHelper::getBalanceNeeded is straightforward. This method is easy to follow and maintain.
105-115: Switching between free pages and balance
The conditional logic for hasEnoughBalanceOrFreePages keeps the code succinct.
124-154: Updating print account history
Your iteration over $availableFreePages is valid so long as it returns an Eloquent collection. The static analysis warning about invalid type can be safely disregarded. However, do consider placing stricter checks to guarantee the collection is not empty or null.
🧰 Tools
🪛 phpstan (2.0.3)
135-135: Argument of an invalid type App\Models\FreePages supplied for foreach, only iterables are supported.
(foreach.nonIterable)
135-135: PHPDoc tag @var above foreach loop does not specify variable name.
(varTag.noVariable)
🪛 GitHub Check: phpunit
[failure] 135-135:
Argument of an invalid type App\Models\FreePages supplied for foreach, only iterables are supported.
app/Models/PrintJob.php (8)
5-7: Enumerations imported successfully
These imports set the foundation for employing typed states and results, which improves clarity and reduces the likelihood of invalid usage.
60-62: State and free page casting
Using the PrintJobStatus enum and a boolean cast is most commendable for type safety and clarity. No issues detected.
67-68: User relationship
This belongsTo relationship indicates that each PrintJob aligns neatly with a single user. Looks well structured.
70-77: Printer relationship
A proper belongsTo relationship for referencing the ‘Printer’ model. This ensures that each PrintJob retains a direct link to its printer.
79-94: Indirect print account relationship
Your printAccount() method makes good use of HasOneThrough. Kindly confirm the expected database structure so that references and keys align accurately.
107-114: Translated state attribute
Employing a translation key __("print." . strtoupper($this->state->value)) is a splendid approach to dynamic localisation.
50-57: Confirm new fillable fields exist in schema
The addition of 'printer_id' and 'used_free_pages' to $fillable is sensible. Kindly ensure that the print_jobs table indeed has these columns and that corresponding migrations exist.
✅ Verification successful
Splendid! The schema changes are properly in place
I can confirm that both printer_id and used_free_pages columns have been properly added to the print_jobs table through migration 2023_12_25_184733_update_print_jobs_table.php. The migration:
- Adds a foreign key
printer_idreferencing theprinterstable - Adds a boolean column
used_free_pageswith a default value of false - Includes proper rollback functionality in the
down()method
The addition of these fields to $fillable is perfectly aligned with the database schema.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Checking migrations for printer_id & used_free_pages columns in print_jobs
rg "Schema::create|Schema::table" -A 15 | gawk '/print_jobs/ {block=1} /}/ {block=0} block{print}'
Length of output: 4478
137-145: Undefined property $pages->amount
Static analysis cannot confirm the presence of an amount attribute for $pages. Do validate that the availableFreePages()->first() indeed returns an object containing amount.
✅ Verification successful
The amount property is indeed present and properly defined
I can confirm that the amount property is properly defined in the printing_free_pages table schema, as evidenced by the migration file. The property is defined as an integer column, and the FreePages model is correctly set up with the corresponding table. The usage in the code is therefore perfectly valid.
Key findings:
- The
amountcolumn is defined in the database schema via migration - The
FreePagesFactoryalso confirms the presence of this property - The relationship chain from
PrintAccount->freePages()->first()will return aFreePagesmodel instance with theamountproperty
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching definitions for 'availableFreePages' to confirm 'amount' existence
rg "function availableFreePages" -A 15
Length of output: 1051
Script:
#!/bin/bash
# Let's check the relationship and schema for freePages
rg "function freePages" -A 5
# Also check for any model definition related to free pages
ast-grep --pattern 'class $_ extends Model'
Length of output: 1251
Script:
#!/bin/bash
# Let's check for the FreePages model and its schema
rg "class FreePages" -A 10
# Also check for any migrations related to free_pages
fd "free_pages" --exec cat {}
Length of output: 5016
🧰 Tools
🪛 phpstan (2.0.3)
141-141: Access to an undefined property Illuminate\Database\Eloquent\Model::$amount.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
(property.notFound)
app/Http/Controllers/Dormitory/Printing/PrintJobController.php (6)
36-37: Updating completed print jobs
Calling PrinterHelper::updateCompletedPrintJobs() here helps keep job statuses synchronised prior to listing. This is a solid practice.
38-49: Pagination approach for user print jobs
The paginatorFrom method is used effectively, returning the correct columns. Kindly ensure those properties exist and are appropriate for users’ needs.
59-60: Repetitive job completion updates
This line is akin to the code in the index() method, ensuring updated job statuses for administrators as well.
61-72: Admin pagination approach
Including ->with('user') is helpful, but kindly double-check that the 'user' relationship is indeed resolvable on PrintJob.
🧰 Tools
🪛 phpstan (2.0.3)
62-62: Relation 'user' is not found in App\Models\PrintJob model.
(larastan.relationExistence)
🪛 GitHub Check: phpunit
[failure] 62-62:
Relation 'user' is not found in App\Models\PrintJob model.
110-113: Cost calculation
Differentiating between free pages and paid printing is cleanly handled. Do confirm that the logic in PrinterHelper::getFreePagesNeeded aligns exactly with your new model changes.
118-118: Potential property mismatch
$printJob->job_id might be unknown if the PrintJob model does not define $job_id as a property or table column. Ensure that the model and migrations match.
🧰 Tools
🪛 phpstan (2.0.3)
118-118: Access to an undefined property Illuminate\Database\Eloquent\Model::$job_id.
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
(property.notFound)
🪛 GitHub Check: phpunit
[failure] 118-118:
Access to an undefined property Illuminate\Database\Eloquent\Model::$job_id.
app/Http/Controllers/Dormitory/Printing/PrintAccountController.php (2)
28-51: Distinction between account modification and transfers
Splitting the logic into two separate flows is commendable. The distinction is clear and ensures simpler code paths.
155-158: No-balance helper
This pleasant redirection pattern is consistent with the rest of the printing domain’s approach. Keep up the consistency.
routes/web.php (2)
18-22: New printing controller imports
Importing your new dedicated controllers is a smart step towards modularising the print module.
130-147: Enhanced route grouping for printing
The new route structure under a print prefix is well-organised and fosters clarity. Adding clear naming (print-job, free-pages) ensures future maintainability.
app/Enums/PrinterCancelResult.php (1)
1-10: Use of enum for cancellation results
A well-defined enum helps standardise possible outcomes and minimises confusion. Looks perfect.
config/commands.php (1)
9-9: Do ensure debug mode is enabled only when strictly necessary, my good friend.
Enabling commands in debug mode might expose internal workings and potentially sensitive information.
resources/views/emails/changed_print_balance.blade.php (1)
4-8: Your conditional structure appears to be in good order, I daresay.
This logic neatly personalises the message and should present minimal issues.
database/factories/PrintJobFactory.php (1)
18-18: Splendid introduction of the enum-based status
Using PrintJobStatus::cases() provides a robust and type-safe method for generating random states. This eliminates the guesswork of possible states, preventing accidental typos or divergences from your enumerations.
resources/views/dormitory/print/manage/modify.blade.php (3)
5-5: Excellent move to a more RESTful approach
Adopting action="{{ route('print-account.update') }}" is more semantically accurate for updating an existing resource. This shift aligns perfectly with the separation of concerns introduced by the new PrintAccountController.
7-7: Tasteful use of PUT
Adding @method('PUT') to the form clarifies that this is an update operation. This is consistent with RESTful best practices — bravo!
8-9: Renaming input fields for clarity
Changing id="user_id_modify" to id="user" and id="balance" to id="amount" helps maintain a consistent naming scheme that feels perfectly legible. Splendid housekeeping indeed!
resources/views/dormitory/print/app.blade.php (2)
12-12: Appropriate route remapping
Switching to route('print.print-job.index') helps decouple the listing functionality from the old monolithic controller. A wise organisational improvement, if I may say so.
15-15: Streamlined route naming
Likewise, referencing route('print.free-pages.index') aligns with the new structure for free pages. This fosters a more modular and comprehensible codebase.
resources/views/dormitory/print/manage/free.blade.php (2)
4-4: Delightfully RESTful submission handling
Using action="{{ route('print.free-pages.store') }}" for creating free pages is perfectly aligned with typical resource-based routing patterns. Very tidy.
6-7: Clean and consistent ID changes
Renaming user_id_free to user_id and free_pages to amount makes the form’s intent clearer. Maintaining meaningful IDs will simplify maintenance and usage in the future.
resources/views/dormitory/print/send.blade.php (4)
7-7: Good day, dear colleague. I see the form action has shifted to print-account.update, adhering more closely to RESTful conventions. This is a commendable approach that enhances clarity and maintainability.
9-9: May I kindly note that introducing an explicit PUT method underlines the resource-updating intention of this form, which is most appropriate for this scenario. Splendid work.
11-11: It is quite prudent to rename the field to other_user. This change fosters clearer semantics regarding the target user.
12-12: Your decision to label the amount field as amount rather than balance nicely captures its quantitative nature. Requiring a minimum value of one is also sensible.
app/Mail/ChangedPrintBalance.php (2)
15-15: Introducing the $print_account_holder property offers improved clarity about ownership of the print account. Very nicely done.
24-27: Updating the constructor to accept $print_account_holder aligns the mail class more closely with actual usage. Ensure that anywhere this class is invoked, the additional parameter is handled properly.
database/migrations/2024_12_25_112824_remove_printer_role.php (3)
13-13: Do mind that DB::table('roles')->where('name', 'printer')->first() could conceivably return null if the printer role is missing. Accessing ->id would then raise an error. A swift safeguard check would be most reassuring.
15-17: Deleting the references in role_users and subsequently removing the printer role is a tidy way to maintain referential integrity. Bravo.
25-34: Your down method elegantly restores the printer role if the migration is reversed. Do ensure that reassigning said role to all verified users is indeed always the intended behaviour.
database/migrations/2019_10_06_224327_create_print_jobs_table.php (1)
22-22: Employing PrintJobStatus::cases() for enumerating possible states is a capital approach, enabling strong consistency with your enum definitions.
resources/views/dormitory/print/manage/app.blade.php (2)
33-33: Splendid adaptation of the route configuration, old chap.
The inclusion of the 'admin' => true parameter ensures proper segregation of admin-level functionalities. Please verify that corresponding server-side checks are present to handle this parameter securely, so that unauthorised users cannot misappropriate the admin interface.
36-36: Route and parameter adjustments appear well-structured.
Kindly confirm, dear colleague, that all references to 'print.print-job.index.admin' are indeed consistent throughout the codebase and tested, particularly regarding the 'admin' => true usage. This will prevent any route mismatch issues and maintain the clarity of admin privileges.
database/migrations/2023_12_25_182310_create_printers_table.php (4)
1-7: Migration file setup is coherent.
Ensuring all necessary imports (Illuminate\Support\Facades\DB etc.) are visible and correct is commendable. You might consider verifying that these imports remain consistent with your application’s environment, so nothing is duplicated or missing.
8-13: Anonymous class usage is thoroughly modern.
It is most encouraging to see a migration defined within an anonymous class. This approach neatly encapsulates the logic and prevents naming collisions.
33-41: Rollback logic is suitably minimalistic.
Dropping the printers table is straightforward and essential for ensuring a clean rollback process. This is perfectly efficient for reversing the migration.
14-31: Database schema creation is correctly structured, old bean.
The table schema is sensibly defined, and the insertion of a default printer is a thoughtful touch that eases setup. However, you may wish to consider how your application might behave if the environment variables are absent, or if the inserted printer record becomes out of date over time.
DB::table('printers')->insert([
- 'name' => env('PRINTER_NAME'),
- 'ip' => env('PRINTER_IP'),
- 'port' => env('PRINTER_PORT'),
+ 'name' => env('PRINTER_NAME', 'default_printer'),
+ 'ip' => env('PRINTER_IP', '192.168.0.100'),
+ 'port' => env('PRINTER_PORT', '9100'),
]);tests/Unit/PrintTest.php (1)
31-36: Extended tests for edge cases are also commented out, dear colleague.
Verifying negative and large amounts is critical for ensuring robust handling of edge cases. Should these tests remain disabled, kindly ensure such validation is tested elsewhere or re-enable them after any pending refactor.
resources/views/user/printing.blade.php (1)
14-19: Property-based check is elegantly succinct.
Accessing $page->available directly simplifies your condition. Kindly confirm that this property is always set or has a default, lest we encounter unexpected errors if $page->available is undefined or null.
database/migrations/2023_12_25_184733_update_print_jobs_table.php (6)
18-18: Mind the foreign key constraints, dear chap.
By referencing Printer::class in this migration, you ensure referential integrity. Just verify that the printers table is indeed created before altering print_jobs.
19-19: Splendid addition of used_free_pages.
This boolean field clearly indicates whether free pages were utilised, which is marvellously descriptive and straightforward.
20-20: Dropping filepath might require caution.
Ensure that no other component relies on the filepath field before removal.
22-24: Updating the printer_id for existing records.
Capital idea to avoid null references. Just confirm that the printers table has a suitable default printer row.
25-27: Converting printer_id to non-nullable.
This course of action fortifies data consistency. Well executed.
37-41: Reverse strategy carefully reverts changes.
Dropping the foreign key and columns in the down() method ensures a clean rollback. Bravo.
resources/views/dormitory/print/manage/account_history.blade.php (2)
9-9: Route update looks spot on.
The revised route presumably aligns with your new controller structure, good show.
42-42: Field modifier.name properly references nested data.
This aligns with your new data model for user relationships. Capital improvement.
app/Http/Controllers/Dormitory/Printing/PrinterController.php (2)
21-28: Index method returning printer data.
Fetching the printer by name from config is a tidy approach; do keep an eye on potential mismatches in environment values.
34-39: Authorisation check for adminIndex.
Requiring 'handleAny' on PrintAccount helps maintain stricter access controls. Splendid approach.
app/Http/Controllers/Dormitory/Printing/FreePagesController.php (4)
19-32: User-specific free pages query is quite sensible.
Ensuring each user can only view their own free pages is a commendable security measure.
38-53: adminIndex for a global view.
Allowing administrators to see all free pages centralises oversight. This architectural choice is perfectly sound.
59-65: Paginator integration is well handled.
Employing a dedicated method to apply sorting and filtering fosters clarity and maintainability. Tip-top.
73-88: store method with validation.
Requiring a valid user, a positive amount, and a future deadline is prudent. Mind that deadline format is consistent with your locale.
app/Utils/PrinterHelper.php (2)
19-21: Consider removing the random integer argument passed to getOutput.
Good day. Might you clarify why the output is retrieved with strval(rand(1, 10))? This appears rather whimsical and may cause unpredictability when collecting the command output. A stable and deterministic argument is generally advisable to maintain consistent results and proper debugging in production.
57-58: The free pages calculation might be incomplete.
You're currently returning $pages * $copies for "free pages needed" without factoring in two-sided printing. You might consider whether two-sided printing reduces the total free pages needed.
resources/lang/en/print.php (4)
10-10: Add clarity to 'The print job was already completed.'
Eloquently worded. The message is coherent and properly communicated to the end user.
18-18: 'Unable to cancel the print job' message usage
This is a beneficial addition, providing explicit feedback to users when cancellations fail.
47-47: others_balance_changed_descr is admirably succinct and descriptive.
No concerns discovered.
66-66: Enhancement to 'your_balance_changed_descr'
Clarity is improved by specifying both the modifier and the new balance.
resources/lang/hu/print.php (4)
9-10: Revised Hungarian phrases are excellent.
The updates convey the completion and ineligibility for cancellation with succinctness.
18-18: Truncating the Hungarian wording
"Nem lehetett megszakítani a nyomtatást" is perfectly comprehensible and precise.
47-47: Foreign balance description
The wording aligns neatly with the new balance modification logic.
66-66: Enhanced user balance description
Retains clarity for the updated balance mechanism.
resources/views/dormitory/print/history.blade.php (6)
10-10: Use of direct state comparison
This condition is more explicit and reveals intent more clearly when toggling the ‘Queued’ condition.
15-16: PUT request to cancel the print job
Moving from POST to PUT is a good RESTful approach, reflecting the update in resource state.
52-53: Table column initialisation
These lines specifying pagination and layout settings appear well-chosen for remote data handling.
60-69: Conditional user column
Displays the user column only for admins—sensible for role-based data visibility.
78-78: Renaming 'cost' to 'translated_cost'
This rename clarifies that a translated or processed field is being displayed rather than the raw cost.
84-89: Renaming 'state' to 'translated_state' and enumerating states
A prudent approach using the newly introduced PrintJobStatus enum for consistency and clarity.
app/Http/Controllers/Secretariat/DocumentController.php (1)
161-161: Ensure correct debug configuration.
You have introduced !config('commands.run_in_debug') alongside config('app.debug'). Please verify that this condition correctly captures the intended logic for controlling the returned file in debug mode.
database/seeders/UsersTableSeeder.php (2)
5-5: Importing the Semester model looks appropriate.
This newly introduced import is beneficial if we intend to seed or manipulate semester data. No issues detected here.
31-32: Nice approach to ensure a current Semester exists.
The invocation of Semester::current() ensures that a valid semester is present before creating test users. Please verify that calling this method out of context cannot introduce side effects, such as inadvertently creating a wrong or duplicate semester record.
tests/Feature/PrintControllerTest.php (39)
22-26: Reduced permissions yield correct 200 status tests.
Testing the /print, /print/free-pages, and /print/print-job endpoints for a user without special permissions is a solid check. No issues detected here.
27-27: Validation of 403 response is valid.
You are correctly asserting forbidden access for /print/free-pages/admin.
29-29: Good to see 403 for unauthorised route.
The endpoint /print/print-job/admin being off-limits to a user without permissions is logically consistent.
31-31: Checking /print/admin for 403 is thorough.
Proper handling of restricted endpoints.
33-33: Guarding /print/print-account-history route.
This ensures accessible routes remain consistent with roles, verifying correct denial of access.
36-37: PUT request returns 302 for balance update attempt.
It would be prudent to confirm that 302 is indeed the expected status (i.e., redirection to a login page or a form validation response). Otherwise, consider using 403 if the user is unauthorised.
42-43: POST returns 302 as well.
Similar to the previous comment, ensure 302 is intended for an unauthorised or invalid request, and not an unexpected redirection.
57-57: Permissioned user confirming 200 response for /print/free-pages.
This test is vital for ensuring the correct assignment of printer privileges.
59-59: Valid access to /print/print-job.
Works as expected for a user with printer permissions.
61-61: 403 for /print/free-pages/admin is correct.
Ensures that a PRINTER role alone is not sufficient for admin-level tasks.
63-63: Valid 403 for /print/print-job/admin.
Restricts administrative routes to higher clearance, consistent with the new permission structure.
65-65: No admin access for /print/admin as well.
Logical limitation kept in place.
67-67: Denying /print/print-account-history is sensible.
Upholds principle of least privilege.
70-70: Ensuring 302 for a non-admin user updating print accounts.
Again, please confirm the correctness of returning 302 for insufficient privileges.
76-76: POST to /print/print-job yielding 302.
Matches the behaviour seen in the user-without-permissions scenario. Confirm if this is intended.
92-92: Confirmation of 200 for /print/free-pages.
Just the right approach for a user with admin-level permissions.
94-94: 200 status for /print/print-job.
No further suggestions, as it aligns with admin privileges.
96-96: Granting admins /print/free-pages/admin.
This test aligns with the principle of admin access.
98-98: Correctly verifying /print/print-job/admin.
Seems consistent with the updated routing structure.
100-100: 200 for /print/admin.
This route is properly accessible to admin-level users only.
102-102: /print/print-account-history open to admin.
Appropriate access control verification.
105-105: Put request to /print-account-update returning 302 for admin.
Kindly confirm if a success or a 200 outcome would be more appropriate after a valid update.
111-111: /print/print-job post returning 302 for admin user.
Might be an intentional redirect or a result of validation. Confirm correctness.
123-124: Creating a receiver user for transfer tests.
Renaming from reciever to receiver improves clarity.
131-131: Asserting zero balance is good.
Clear verification that the newly created user has no starting balance.
134-135: Transferring 10 credits.
Solid check on the immediate effect of transferring partial balances.
138-140: Over-balance transfers remain restricted.
Properly tests that transferring more than the balance is blocked or fails gracefully.
143-144: Zero-amount transfer test.
Ensures no changes if the amount is zero.
147-148: Negative value transfers yield 400.
Appropriate to forbid negative amounts.
151-152: All-balance transfer.
Correctly leaves the sender with zero funds.
155-156: Transfer from an empty account.
Correctly results in no money changing hands.
168-169: Preparing a separate user for balance modification.
Semantic clarity remains consistent with the rest of the test file.
173-173: Starting with zero balance for receiver.
Baseline checks are vital to confirm subsequent changes.
176-176: Modifies balance to 10.
Ensures increments function well.
178-178: Cumulative addition is tested.
Confirms further increments accumulate properly.
210-215: Helper transfer method.
Effectively consolidates repeated code. Verify that the user identity is correct: you pass user()->id as user, which might differ from $sender.
220-226: assertCorrectTransfer reloads both users.
Good approach to ensure up-to-date balances.
229-233: Helper modify method mirrors transfer structure.
Helps reduce duplication. Great approach overall.
237-241: assertCorrectModification re-checks data.
Ensures the final balance matches expectations after each modification.
resources/views/layouts/navbar.blade.php (2)
40-40: Route name updated to print.index.
This new naming convention aligns well with RESTful practices.
162-178: Admin dropdown includes link to print.index.admin.
This ensures that only users with the correct permission can access admin-level printing routes. The collapsed structure is well-structured for manageability.
.env.example (1)
Line range hint 74-93: Verify the impact of removed printer arguments
I notice that PRINTER_ADDITIONAL_ARGS and PRINTER_STAT_ADDITIONAL_ARGS have been removed. While this aligns with the printer module refactoring, we should ensure this doesn't affect existing deployments.
✅ Verification successful
Removal of printer arguments is safe to proceed
After a thorough examination of the codebase, I can confirm that the removal of PRINTER_ADDITIONAL_ARGS and PRINTER_STAT_ADDITIONAL_ARGS is safe. Whilst these variables are referenced in config/print.php, they are not actively used in any of the core printing functionality. The PrinterHelper and Printer model implementations show that printing operations are handled through direct command construction without relying on these additional arguments.
- The printer commands in
app/Models/Printer.phpuse explicit parameter construction for thelpcommand - The status checking in
getCompletedPrintJobs()uses a straightforwardlpstatcommand without additional arguments - No other components in the codebase reference these configuration values
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the removed configuration variables
echo "Checking for references to removed variables..."
rg -l "PRINTER_ADDITIONAL_ARGS|PRINTER_STAT_ADDITIONAL_ARGS"
# Check if these variables are used in any deployment configurations
echo "Checking deployment configurations..."
fd -e php -e yaml -e json -e conf -e ini . config/ | xargs rg "PRINTER_.*ADDITIONAL_ARGS"
Length of output: 527
Script:
#!/bin/bash
# Let's examine the content of config/print.php to understand the usage context
echo "Checking config/print.php content:"
cat config/print.php
# Let's also check if there are any references in the application code
echo -e "\nChecking for any usage of these configuration values in the codebase:"
rg "config\(['\"](print\.stat_additional_args|print\.additional_args)['\"]\)" -A 2
Length of output: 762
Script:
#!/bin/bash
# Let's check for any printer-related classes or services that might use these configurations
echo "Checking for printer-related files:"
fd -t f -e php . app/ | xargs rg -l "printer|print" -i
# Let's also examine any recent changes to printer-related files
echo -e "\nChecking git history for printer-related changes:"
git log --oneline -n 5 -- "**/print*" "**/Print*"
Length of output: 1285
Script:
#!/bin/bash
# Let's examine the PrinterHelper class which is likely to use these configurations
echo "Checking PrinterHelper implementation:"
cat app/Utils/PrinterHelper.php
# Also check the PrinterController for any usage
echo -e "\nChecking PrinterController implementation:"
cat app/Http/Controllers/Dormitory/Printing/PrinterController.php
Length of output: 4840
Script:
#!/bin/bash
# Let's check the Printer model implementation
echo "Checking Printer model implementation:"
cat app/Models/Printer.php
# Also check for any migration files that might reference these configurations
echo -e "\nChecking printer-related migrations:"
fd -e php . database/migrations/ | xargs rg -l "printer" -i
Length of output: 5461
| @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"> | ||
| <form class="form-horizontal" role="form" method="POST" action="{{ route('print.print-job.store') }}" enctype="multipart/form-data"> |
There was a problem hiding this comment.
💡 Codebase verification
Route name in form action needs to be corrected
I notice that the form is using print.print-job.store as the route name, but the actual route defined in routes/web.php is print-job.store. The correct route name should be:
action="{{ route('print.print-job.store') }}"- File:
resources/views/dormitory/print/print.blade.php, Line 26: Update the route name to match the defined route inroutes/web.php
🔗 Analysis chain
Verify the new route implementation
The form action has been updated to use the new print.print-job.store route. Let's ensure this route is properly defined and handled.
Also applies to: 29-30
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new route exists and is handled correctly
# Check for route definition
rg -A 2 "print\.print-job\.store" routes/
# Check for corresponding controller method
ast-grep --pattern 'class PrintJobController {
$$$
store($$$) {
$$$
}
$$$
}'
Length of output: 134
Script:
#!/bin/bash
# Let's try different approaches to verify the route and controller
# Check routes file for print-related routes
rg -l "Route::.*print" routes/
# Search for print-related controllers
fd "PrintController" app/Http/Controllers/
# Search for any print-related route definitions
rg "print.*store" routes/
# Look for route definitions in web.php specifically
cat routes/web.php
# Search for potential controller class names
fd "Controller" app/Http/Controllers/ | grep -i "print"
Length of output: 27549
app/Console/Commands.php
Outdated
| $process = Process::fromShellCommandline(config('commands.ping') . " $router->ip -c 1 | grep 'error\|unreachable'"); | ||
| $process->run($log = false); | ||
| $result = $process->getOutput(debugOutput: rand(1, 10) > 9 ? "error" : ''); |
There was a problem hiding this comment.
Mismatch in parameter type for $process->run()
Kindly adjust your usage of $process->run($log = false);. The method expects the first argument to be a callable or null, rather than a boolean.
Please consider the following adjustment:
- $process->run($log = false);
+ $process->run(callback: null, env: [], log: false);📝 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.
| $process = Process::fromShellCommandline(config('commands.ping') . " $router->ip -c 1 | grep 'error\|unreachable'"); | |
| $process->run($log = false); | |
| $result = $process->getOutput(debugOutput: rand(1, 10) > 9 ? "error" : ''); | |
| $process = Process::fromShellCommandline(config('commands.ping') . " $router->ip -c 1 | grep 'error\|unreachable'"); | |
| $process->run(callback: null, env: [], log: false); | |
| $result = $process->getOutput(debugOutput: rand(1, 10) > 9 ? "error" : ''); |
app/Models/FreePages.php
Outdated
| /** | ||
| * Wether the free pages are still available. | ||
| * @return bool | ||
| */ | ||
| protected function available(): Attribute | ||
| { | ||
| return Attribute::make( | ||
| get: fn () => now()->isBefore($this->deadline) | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
PHPDoc return type vs. actual return type
The available method’s docblock indicates a boolean but actually returns an Attribute. Clarify this by updating the docblock to match the real return type.
- * @return bool
+ * @return \Illuminate\Database\Eloquent\Casts\Attribute📝 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.
| /** | |
| * Wether the free pages are still available. | |
| * @return bool | |
| */ | |
| protected function available(): Attribute | |
| { | |
| return Attribute::make( | |
| get: fn () => now()->isBefore($this->deadline) | |
| ); | |
| /** | |
| * Wether the free pages are still available. | |
| * @return \Illuminate\Database\Eloquent\Casts\Attribute | |
| */ | |
| protected function available(): Attribute | |
| { | |
| return Attribute::make( | |
| get: fn () => now()->isBefore($this->deadline) | |
| ); |
🧰 Tools
🪛 phpstan (2.0.3)
82-82: PHPDoc tag @return with type bool is incompatible with native type Illuminate\Database\Eloquent\Casts\Attribute.
(return.phpDocType)
🪛 GitHub Check: phpunit
[failure] 82-82:
PHPDoc tag @return with type bool is incompatible with native type Illuminate\Database\Eloquent\Casts\Attribute.
| $paginator = TabulatorPaginator::from($printJobs)->sortable($columns)->filterable($columns)->paginate(); | ||
|
|
||
| // Process the data before showing it in a table. | ||
| $paginator->getCollection()->append([ |
There was a problem hiding this comment.
.append() usage on collections
The append() method might not behave as expected on a Laravel collection. Consider using push() or mapping each item to add these attributes.
- $paginator->getCollection()->append([
+ $paginator->getCollection()->each(function($item) {
+ $item->makeVisible(['translated_cost','translated_state']);
+ });Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 phpstan (2.0.3)
178-178: Call to an undefined method Illuminate\Support\Collection<(int|string), mixed>::append().
(method.notFound)
🪛 GitHub Check: phpunit
[failure] 178-178:
Call to an undefined method Illuminate\Support\Collection<(int|string), mixed>::append().
tests/Unit/PrintTest.php
Outdated
| //$this->assertFalse($user->printAccount->hasEnoughMoney(1)); | ||
| //$this->assertTrue($user->printAccount->hasEnoughMoney(0)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Commented-out assertions might reduce test coverage.
By disabling these lines, we are no longer verifying whether a print account can handle balances thoroughly. If the relevant functionality is still critical, consider re-enabling or updating the assertions to reflect any new logic in the hasEnoughMoney method.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
app/Models/Printer.php (3)
17-32: Rather splendid structure, though a bit light on documentation, what?Whilst the model structure is quite proper, I dare say we ought to add some property documentation. Would you be amenable to adding PHP DocBlock comments for the properties?
class Printer extends Model { use HasFactory; public $timestamps = false; + /** + * The attributes that are mass assignable. + * + * @var array<string> + */ protected $fillable = [ 'name', 'ip', 'port', 'paper_out_at', ]; + /** + * The attributes that should be cast. + * + * @var array<string, string> + */ protected $casts = [ 'paper_out_at' => 'datetime', ];
57-69: A spot of bother with the user() helper, I'm afraid.Rather than relying on the global
user()helper, it would be more proper to inject the user as a parameter. This would make the method more testable and explicit about its dependencies.- public function createPrintJob(bool $useFreePages, int $cost, string $filePath, string $originalName, bool $twoSided, int $copyNumber) + public function createPrintJob(User $user, bool $useFreePages, int $cost, string $filePath, string $originalName, bool $twoSided, int $copyNumber) { $jobId = $this->print($twoSided, $copyNumber, $filePath); - return user()->printJobs()->create([ + return $user->printJobs()->create([
147-150: Might I suggest enhancing our exception handling?The
PrinterExceptionclass could be more useful with custom message formatting and error codes.class PrinterException extends \Exception { - // + /** + * @param string $message + * @param int $code + * @param \Throwable|null $previous + */ + public function __construct(string $message = "", int $code = 0, ?\Throwable $previous = null) + { + parent::__construct("Printer error: $message", $code, $previous); + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/Models/Printer.php(1 hunks)
🧰 Additional context used
🪛 phpstan (2.0.3)
app/Models/Printer.php
138-138: Result of static method Illuminate\Database\Connection::transaction() (void) is used.
(staticMethod.void)
🔇 Additional comments (1)
app/Models/Printer.php (1)
34-41: Jolly good implementation of the relationship!
The relationship is properly defined and well-documented. Carry on!
| public function getCompletedPrintJobs() | ||
| { | ||
| try { | ||
| $process = Process::fromShellCommandline(config('commands.lpstat') . " -h $this->ip:$this->port -W completed -o $this->name | awk '{print $1}'"); | ||
| $process->run(); | ||
| $result = explode("\n", $process->getOutput()); | ||
| return $result; | ||
| } catch (\Exception $e) { | ||
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $e->getMessage()); | ||
| throw new PrinterException($e->getMessage(), $e->getCode(), $e->getPrevious()); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Updates the state of the completed printjobs to `PrintJobStatus::SUCCESS`. | ||
| */ | ||
| public function updateCompletedPrintJobs() | ||
| { | ||
| return DB::transaction(function () { | ||
| PrintJob::where('state', PrintJobStatus::QUEUED)->whereIn( | ||
| 'job_id', | ||
| $this->getCompletedPrintJobs() | ||
| )->update(['state' => PrintJobStatus::SUCCESS]); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
A few matters requiring attention in the job handling, if I may.
- The
getCompletedPrintJobsmethod should handle empty output gracefully. - The
updateCompletedPrintJobsmethod should return the number of updated records.
public function getCompletedPrintJobs()
{
try {
$process = Process::fromShellCommandline(config('commands.lpstat') . " -h $this->ip:$this->port -W completed -o $this->name | awk '{print $1}'");
$process->run();
$result = explode("\n", $process->getOutput());
- return $result;
+ return array_filter($result); // Remove empty entries
} catch (\Exception $e) {
Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $e->getMessage());
throw new PrinterException($e->getMessage(), $e->getCode(), $e->getPrevious());
}
}
public function updateCompletedPrintJobs()
{
return DB::transaction(function () {
- PrintJob::where('state', PrintJobStatus::QUEUED)->whereIn(
+ return PrintJob::where('state', PrintJobStatus::QUEUED)->whereIn(
'job_id',
$this->getCompletedPrintJobs()
)->update(['state' => PrintJobStatus::SUCCESS]);
});
}📝 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.
| public function getCompletedPrintJobs() | |
| { | |
| try { | |
| $process = Process::fromShellCommandline(config('commands.lpstat') . " -h $this->ip:$this->port -W completed -o $this->name | awk '{print $1}'"); | |
| $process->run(); | |
| $result = explode("\n", $process->getOutput()); | |
| return $result; | |
| } catch (\Exception $e) { | |
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $e->getMessage()); | |
| throw new PrinterException($e->getMessage(), $e->getCode(), $e->getPrevious()); | |
| } | |
| } | |
| /** | |
| * Updates the state of the completed printjobs to `PrintJobStatus::SUCCESS`. | |
| */ | |
| public function updateCompletedPrintJobs() | |
| { | |
| return DB::transaction(function () { | |
| PrintJob::where('state', PrintJobStatus::QUEUED)->whereIn( | |
| 'job_id', | |
| $this->getCompletedPrintJobs() | |
| )->update(['state' => PrintJobStatus::SUCCESS]); | |
| }); | |
| } | |
| public function getCompletedPrintJobs() | |
| { | |
| try { | |
| $process = Process::fromShellCommandline(config('commands.lpstat') . " -h $this->ip:$this->port -W completed -o $this->name | awk '{print $1}'"); | |
| $process->run(); | |
| $result = explode("\n", $process->getOutput()); | |
| return array_filter($result); // Remove empty entries | |
| } catch (\Exception $e) { | |
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $e->getMessage()); | |
| throw new PrinterException($e->getMessage(), $e->getCode(), $e->getPrevious()); | |
| } | |
| } | |
| /** | |
| * Updates the state of the completed printjobs to `PrintJobStatus::SUCCESS`. | |
| */ | |
| public function updateCompletedPrintJobs() | |
| { | |
| return DB::transaction(function () { | |
| return PrintJob::where('state', PrintJobStatus::QUEUED)->whereIn( | |
| 'job_id', | |
| $this->getCompletedPrintJobs() | |
| )->update(['state' => PrintJobStatus::SUCCESS]); | |
| }); | |
| } |
🧰 Tools
🪛 phpstan (2.0.3)
138-138: Result of static method Illuminate\Database\Connection::transaction() (void) is used.
(staticMethod.void)
| public function print(bool $twoSided, int $copies, string $path) | ||
| { | ||
| if (config('app.debug')) { | ||
| return -1; | ||
| } | ||
| $jobId = null; | ||
| try { | ||
| $process = new Process([ | ||
| 'lp', | ||
| '-h', "$this->ip:$this->port", | ||
| '-d', $this->name, | ||
| ($twoSided ? '-o sides=two-sided-long-edge' : ''), | ||
| '-n', $copies, | ||
| $path | ||
| ]); | ||
| $process->run(); | ||
| if (!$process->isSuccessful()) { | ||
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $process->getErrorOutput()); | ||
| throw new PrinterException($process->getErrorOutput()); | ||
| } | ||
| $result = $process->getOutput(); | ||
| if (!preg_match("/^request id is ([^\s]*) \\([0-9]* file\\(s\\)\\)$/", $result, $matches)) { | ||
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). result:" | ||
| . print_r($result, true)); | ||
| throw new PrinterException($result); | ||
| } | ||
| $jobId = intval($matches[1]); | ||
| } catch (\Exception $e) { | ||
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $e->getMessage()); | ||
| throw new PrinterException($e->getMessage(), $e->getCode(), $e->getPrevious()); | ||
| } | ||
|
|
||
| return $jobId; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
I say, the path validation suggestion requires a slight modification!
Upon thorough examination of the codebase, I observe that the system utilises a dedicated printing queue directory at storage_path('app/printing_queue'), as evidenced in the filesystems configuration. Therefore, we ought to adjust our validation accordingly.
Suggested improvements:
public function print(bool $twoSided, int $copies, string $path)
{
+ // Validate path is within the printing queue directory
+ $realPath = realpath($path);
+ if (!$realPath || !str_starts_with($realPath, storage_path('app/printing_queue'))) {
+ throw new PrinterException('Invalid file path: File must be in the printing queue directory');
+ }
+
if (config('app.debug')) {
+ Log::info("Debug mode: Simulating print job with ID -1");
return -1;
}Regarding the other points:
- The shell command arguments are properly handled by Laravel's Process class, which automatically escapes arguments.
- The debug mode return value of -1 is now documented in the code.
🔗 Analysis chain
I say, we ought to be rather careful with these shell commands!
While the implementation is thorough, there are a few security considerations to address:
- The path parameter should be validated to prevent path traversal attacks.
- The shell command arguments should be properly escaped.
- The debug mode return value of -1 should be documented.
Let's verify the path handling across the codebase:
Suggested improvements:
public function print(bool $twoSided, int $copies, string $path)
{
+ // Validate path is within allowed directory
+ $realPath = realpath($path);
+ if (!$realPath || !str_starts_with($realPath, storage_path('app/prints/'))) {
+ throw new PrinterException('Invalid file path');
+ }
+
if (config('app.debug')) {
+ Log::info("Debug mode: Simulating print job");
return -1;
}Let me search for any path validation mechanisms and print method implementations to properly assess the security concerns.
Additionally, let's check the storage configuration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential path traversal vulnerabilities
rg -g '*.php' "->print\([^)]*\)" -A 5
Length of output: 444
Script:
#!/bin/bash
# Search for print method invocations with proper escaping
rg --type php '\bprint\s*\(' -A 5
# Search for path validation patterns
rg --type php 'realpath|storage_path' -A 2
# Look for print-related classes and their usage
ast-grep --pattern 'class $_ {
$$$
print($$$) {
$$$
}
$$$
}'
Length of output: 6669
Script:
#!/bin/bash
# Check storage configuration
fd -e php config
Length of output: 18
Description
#394 introduced some bugs, this should contain a fix for them