Rework print module #394
Rework print module #394
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
|
I still have to write the tests, but the main refactoring is done. |
There was a problem hiding this comment.
Nice job, thanks for taking care of this. Please take a look at the issues. Overall, the functions are still a bit too complex, let's revise this again after you fix these. And we need extensive unit tests here (calculating price, pages, etc), otherwise this would be worthless. Let me know if you have questions.
app/Http/Controllers/Dormitory/Printing/FreePagesController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Dormitory/Printing/PrintAccountController.php
Outdated
Show resolved
Hide resolved
app/Http/Controllers/Dormitory/Printing/PrintAccountController.php
Outdated
Show resolved
Hide resolved
|
@machiato32 do you have any update on this? |
Not yet, sorry. |
|
Now I had a new idea for the admin/not admin controller separation. |
I honestly think this seems like more effort than it's worth. I'll just split the controllers and that should be easy to understand for everyone. |
WalkthroughThis update introduces significant enhancements across the application, focusing on printing functionalities, debugging configurations, and code execution processes. Key changes include the introduction of environment variables for printer settings, updates in PHP and VS Code configurations for better development experience, and streamlined command execution in PHP scripts. Additionally, the update restructures policies and models to align with new business logic and introduces comprehensive database schema updates. Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@kdmnk I tried to resolve everything you mentioned, and also found some bugs. I still couldn't really test this, and unit/feature tests are also missing, but I think there are a lot of improvements already. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Out of diff range and nitpick comments (2)
resources/views/secretariat/document/import.blade.php (1)
[!TIP]
Codebase VerificationSeveral files in the codebase still use the old function signature.
- The specific file
resources/views/secretariat/document/import.blade.phpdoes not include CSRF tokens in its forms. This could potentially expose the application to CSRF attacks.Analysis chain
Line range hint
17-17: Ensure CSRF protection is enabled for the form.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CSRF token presence in form submissions. # Test: Search for CSRF token in forms. Expect: CSRF token in every form. rg --type blade '<form' | rg 'csrf'Length of output: 65
Script:
#!/bin/bash # Description: Verify CSRF token presence in form submissions within Blade template files. # Search for files with '.blade.php' extension and check for '<form' tags followed by 'csrf' fd '.blade.php' --exec rg '<form' {} | rg 'csrf'Length of output: 181
Script:
#!/bin/bash # Description: Verify CSRF token presence in the specific form submissions within the mentioned Blade template file. # Search for '<form' tags followed by 'csrf' in the specific Blade template file rg '<form' resources/views/secretariat/document/import.blade.php | rg 'csrf'Length of output: 76
app/Http/Controllers/Auth/RegisterController.php (1)
Line range hint
47-100: Refactor thecreatemethod to separate concerns between user creation, role assignment, and notification sending.- public function create(array $data) { - $user = DB::transaction(function () use ($data) { - $user = User::create([ - 'name' => $data['name'], - 'email' => $data['email'], - 'password' => Hash::make($data['password']), - ]); - $user->roles()->attach(Role::firstWhere('name', $data['user_type'])->id); - if ($data['user_type'] == Role::TENANT) { - $user->personalInformation()->create([ - 'tenant_until' => $data['tenant_until'], - 'phone_number' => $data['phone_number'], - ]); - Mail::to($user)->queue(new \App\Mail\Confirmation($user->name)); - if (!$user->isCollegist()) { - $users_to_notify = User::whereHas('roles', function ($q) { - $q->whereIn('role_id', [ - Role::firstWhere('name', Role::SYS_ADMIN)->id - ]); - })->get(); - foreach ($users_to_notify as $person) { - Mail::to($person)->send(new NewRegistration($person's name, $user)); - } - Cache::increment('user'); - } - } else { - $user->application()->create(); - } - return $user; - }); - Cache::forget('collegists'); - return $user; - } + public function create(array $data) { + $user = $this->createUser($data); + $this->assignRole($user, $data['user_type']); + if ($data['user_type'] == Role::TENANT) { + $this->handleTenantRegistration($user, $data); + } else { + $user->application()->create(); + } + return $user; + } + + protected function createUser(array $data): User { + return DB::transaction(function () use ($data) { + return User::create([ + 'name' => $data['name'], + 'email' => $data['email'], + 'password' => Hash::make($data['password']), + ]); + }); + } + + protected function assignRole(User $user, string $roleName) { + $user->roles()->attach(Role::firstWhere('name', $roleName)->id); + } + + protected function handleTenantRegistration(User $user, array $data) { + $user->personalInformation()->create([ + 'tenant_until' => $data['tenant_until'], + 'phone_number' => $data['phone_number'], + ]); + Mail::to($user)->queue(new \App\Mail\Confirmation($user->name)); + $this->notifyRelevantUsers($user); + } + + protected function notifyRelevantUsers(User $user) { + if (!$user->isCollegist()) { + $users_to_notify = User::whereHas('roles', function ($q) { + $q->whereIn('role_id', [ + Role::firstWhere('name', Role::SYS_ADMIN)->id + ]); + })->get(); + foreach ($users_to_notify as $person) { + Mail::to($person)->send(new NewRegistration($person's name, $user)); + } + Cache::increment('user'); + } + }
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
composer.lockis excluded by!**/*.lock
Files selected for processing (49)
- .env.example (1 hunks)
- .vscode/launch.json (1 hunks)
- .vscode/settings.json (1 hunks)
- app/Console/Commands.php (3 hunks)
- app/Enums/PrintJobStatus.php (1 hunks)
- app/Http/Controllers/Auth/RegisterController.php (1 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/Mail/NoPaper.php (1 hunks)
- app/Models/FreePages.php (2 hunks)
- app/Models/PrintAccount.php (3 hunks)
- app/Models/PrintAccountHistory.php (2 hunks)
- app/Models/PrintJob.php (2 hunks)
- app/Models/Printer.php (1 hunks)
- app/Models/Role.php (3 hunks)
- app/Models/User.php (1 hunks)
- app/Policies/FreePagesPolicy.php (1 hunks)
- app/Policies/PrintAccountPolicy.php (2 hunks)
- app/Policies/PrintJobPolicy.php (1 hunks)
- app/Providers/AuthServiceProvider.php (2 hunks)
- app/Utils/PrinterHelper.php (1 hunks)
- app/Utils/Process.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/2023_12_25_182310_create_printers_table.php (1 hunks)
- database/migrations/2023_12_25_184733_update_print_jobs_table.php (1 hunks)
- database/seeders/UsersTableSeeder.php (1 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 (1 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/no_paper.blade.php (1 hunks)
- resources/views/layouts/navbar.blade.php (2 hunks)
- resources/views/secretariat/document/import.blade.php (1 hunks)
- resources/views/secretariat/document/index.blade.php (2 hunks)
- resources/views/user/printing.blade.php (1 hunks)
- routes/web.php (2 hunks)
- tests/Feature/PrintControllerTest.php (4 hunks)
- tests/Feature/RegistrationTest.php (2 hunks)
- tests/Unit/PrintTest.php (1 hunks)
Files skipped from review due to trivial changes (10)
- .env.example
- .vscode/launch.json
- .vscode/settings.json
- app/Enums/PrintJobStatus.php
- app/Models/Role.php
- app/Policies/PrintJobPolicy.php
- config/print.php
- resources/views/emails/no_paper.blade.php
- resources/views/secretariat/document/index.blade.php
- tests/Unit/PrintTest.php
Additional comments not posted (40)
database/factories/PrintJobFactory.php (1)
5-5: Use ofPrintJobStatusenumeration forstateensures type safety and valid state management in test data.resources/views/dormitory/print/manage/modify.blade.php (1)
5-9: The form usesPUTmethod and custom Blade components for clarity and maintainability. Ensure the routeprint-account.updateis correctly defined.Verification successful
The verification process has confirmed that the route
print-account.updateis indeed defined in the routes file (routes/web.php). The route is associated with aPUTmethod, which matches the method used in the form within the Blade template. This ensures that the form's action is correctly set up to interact with thePrintAccountController'supdatemethod.Given this information, the original review comment's concern about verifying the route definition has been addressed, and the route is correctly implemented as per the requirements specified in the Blade template.
The form uses
PUTmethod and custom Blade components for clarity and maintainability. The routeprint-account.updateis correctly defined inroutes/web.php.
- Location:
routes/web.phpScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the route `print-account.update` is defined in the routes file. # Test: Search for the route definition. Expect: At least one occurrence. rg --type php $'Route::put\\(.*print-account.update.*\\)'Length of output: 218
resources/views/dormitory/print/app.blade.php (1)
12-15: Inclusion of specific routes for history and free pages enhances modularity. Verify thatprint-job.indexandfree-pages.indexroutes are correctly defined.Verification successful
The routes
print-job.indexandfree-pages.indexare correctly defined in the routes file and are appropriately used in the Blade template. Here are the details:
- The route
print-job.indexis defined as/print-job/{filter?}and handled byPrintJobController.- The route
free-pages.indexis defined as/free-pagesand handled byFreePagesController. There is also an admin-specific route (/free-pages/admin), but it is not used in the Blade template, which is consistent with theadminflag being set to false.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the routes `print-job.index` and `free-pages.index` are defined in the routes file. # Test: Search for the route definitions. Expect: At least one occurrence for each route. rg --type php $'Route::get\\(.*print-job.index.*\\)' rg --type php $'Route::get\\(.*free-pages.index.*\\)'Length of output: 601
app/Mail/NoPaper.php (1)
21-21: Simplification of theNoPaperconstructor enhances clarity. Verify all usages ofNoPaperare updated to match the new constructor signature.Verification successful
The usage of the
NoPaperclass inPrinterController.phpcorrectly matches the new constructor signature. Theuser()->nameexpression is used as an argument, which aligns with the expected string type for thereporterparameter in theNoPaperconstructor.
- Location:
app/Http/Controllers/Dormitory/Printing/PrinterController.phpScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `NoPaper` class match the new constructor signature. # Test: Search for the class usage. Expect: Only occurrences of the new constructor signature. rg --type php $'new NoPaper\\('Length of output: 193
Script:
#!/bin/bash # Description: Verify the return type of `user()` and the type of `user()->name`. # Test: Search for the definition of `user()` function and check the type of `name` property. rg --type php -A 5 'function user\(\)' # Search for the definition of `user()` function rg --type php 'public \$name;' # Search for the declaration of `name` property in user-related classesLength of output: 4803
resources/views/dormitory/print/manage/free.blade.php (1)
4-7: Well-structured form for managing free pages. Ensure the routefree-pages.storeis correctly defined.Verification successful
The verification process has confirmed that the route
free-pages.storeis indeed defined in theroutes/web.phpfile, as shown by the output from the executed script. The route is associated with a POST request to/free-pagesand is handled by thestoremethod of theFreePagesController. This matches the route name used in the Blade template fileresources/views/dormitory/print/manage/free.blade.php.Given this information, the review comment about ensuring the route is correctly defined has been validated.
Well-structured form for managing free pages. The route
free-pages.storeis correctly defined in the routes file as expected.
- Location:
routes/web.phpScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the route `free-pages.store` is defined in the routes file. # Test: Search for the route definition. Expect: At least one occurrence. rg --type php $'Route::post\\(.*free-pages.store.*\\)'Length of output: 185
resources/views/dormitory/print/send.blade.php (1)
7-13: Well-structured form for sending print account updates. Ensure the routeprint-account.updateis correctly defined.Verification successful
Well-structured form for sending print account updates. The route
print-account.updateis correctly defined in the routes file as verified.
- Location:
routes/web.phpScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the route `print-account.update` is defined in the routes file. # Test: Search for the route definition. Expect: At least one occurrence. rg --type php $'Route::put\\(.*print-account.update.*\\)'Length of output: 218
app/Policies/FreePagesPolicy.php (1)
Line range hint
1-32: Well-defined policy methods forFreePages. Verify the integration of these policies in the application.Verification successful
Several files in the codebase confirm the registration of
FreePagesPolicyfor theFreePagesmodel, which is a standard integration method in Laravel applications. This setup typically allows Laravel to handle authorization checks implicitly when actions on theFreePagesmodel are performed, even if direct calls toauthorizeare not found in the codebase.
app/Providers/AuthServiceProvider.php: Registration ofFreePagesPolicyforFreePagesmodel.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of `FreePagesPolicy` methods in the application. # Test: Search for policy method usages. Expect: At least one occurrence for each method. rg --type php $'authorize\\(.*FreePagesPolicy.*\\)'Length of output: 50
Script:
#!/bin/bash # Description: Broaden the search for `FreePagesPolicy` method usages in the application. # Test: Search for any reference to `FreePagesPolicy` class to see how it's being used or referenced. rg --type php -C 3 'FreePagesPolicy'Length of output: 1231
database/migrations/2019_10_06_224327_create_print_jobs_table.php (1)
22-22: Use ofPrintJobStatusenum for job state ensures data integrity. Verify its usage across the application.Verification successful
The usage of the
PrintJobStatusenum across various parts of the application has been verified and found to be consistent and appropriate. This ensures data integrity and consistency in handling print job states throughout the application. Here are the key points of integration:
- Migration and Database Schema: Ensures that only valid states are stored in the database.
- Factory: Used for generating test data.
- Models and Controllers: Integral to the application logic for managing print job states.
- Views: Used for displaying state information to users.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify `PrintJobStatus` enum usage in other parts of the application. # Test: Search for the enum usage. Expect: Consistent usage across the application. rg --type php $'PrintJobStatus'Length of output: 1948
app/Console/Commands.php (2)
25-27: Refactoring to useProcessinpingRouterenhances robustness. Verify the output handling in different scenarios.
37-39: Refactoring to useProcessinlatexToPdfenhances robustness. Verify the output handling in different scenarios.database/migrations/2023_12_25_182310_create_printers_table.php (1)
16-30: Creation ofprinterstable and insertion of default printer using environment variables are well-implemented. Ensure thatPRINTER_NAME,PRINTER_IP, andPRINTER_PORTare documented and validated.resources/views/dormitory/print/manage/app.blade.php (2)
33-33: Inclusion ofdormitory.print.freewith admin-specific route enhances role-based UI customization. Verify the route definition and access control.Verification successful
The
adminIndexmethod inFreePagesControllerincludes an authorization check ($this->authorize('viewAny', FreePages::class);), which confirms that proper access control mechanisms are in place to restrict access to admin users. This aligns with the intended security measures for the routefree-pages.index.admin.
- File:
app/Http/Controllers/Dormitory/Printing/FreePagesController.php- Method:
adminIndexScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and access control of the `free-pages.index.admin` route. # Test: Check the route definition and ensure it's accessible only to admins. php artisan route:list | grep 'free-pages.index.admin'Length of output: 125
Script:
#!/bin/bash # Description: Search for the 'free-pages.index.admin' route definition in Laravel route files. # Use ripgrep to search for the route in common Laravel route file locations. rg 'free-pages.index.admin' routes/web.phpLength of output: 182
Script:
#!/bin/bash # Description: Verify the access control implementation in the `adminIndex` method of `FreePagesController`. # Use ast-grep to search for the `adminIndex` method in `FreePagesController`. ast-grep --lang php --pattern $'class FreePagesController { $$$ adminIndex($_) { $$$ } $$$ }'Length of output: 215
Script:
#!/bin/bash # Description: Search for the 'adminIndex' method in 'FreePagesController' to verify access control. # Use ripgrep to search for the 'adminIndex' method in the 'FreePagesController.php' file. rg 'public function adminIndex' -- 'app/Http/Controllers/FreePagesController.php'Length of output: 166
Script:
#!/bin/bash # Description: Locate the 'FreePagesController.php' file in the repository. # Use fd to find the file in the repository. fd 'FreePagesController.php'Length of output: 92
Script:
#!/bin/bash # Description: Inspect the 'adminIndex' method in 'FreePagesController' for access control. # Use ripgrep to search for the 'adminIndex' method in the correct file path. rg 'public function adminIndex' -A 10 -- 'app/Http/Controllers/Dormitory/Printing/FreePagesController.php'Length of output: 423
36-36: Inclusion ofdormitory.print.historywith admin-specific route enhances role-based UI customization. Verify the route definition and access control.Verification successful
The verification process has successfully identified the route definition for
print-job.indexin theroutes/web.phpfile. The route is defined to handle GET requests and optionally takes afilterparameter. The route is associated with thePrintJobControllerand theindexPrintJobsmethod, and it is namedprint-job.index.However, the output does not provide explicit information about access control, specifically whether this route is restricted to admin users only. The route definition itself does not include any middleware or other mechanisms that explicitly restrict access to admins.
Given this, the next step is to verify if there is any middleware applied to this route that restricts access to admin users. This can be done by checking the controller or middleware definitions.
The search results have successfully located the
PrintJobControllerwithin theapp/Http/Controllers/Dormitory/Printing/PrintJobController.phpfile. However, the output provided does not include details about the implementation of the controller, specifically whether any middleware or access control mechanisms are applied to restrict access to admin users.To fully verify the access control on the
print-job.indexroute, it is necessary to inspect the contents of thePrintJobControllerto determine if any middleware is applied either at the controller level or specifically to theindexPrintJobsmethod.
The
PrintJobController'sindexPrintJobsmethod includes authorization checks that are crucial for access control. Specifically, it uses theauthorizemethod to check permissions:
- If the
filterparameter is "all", the method calls$this->authorize('viewAny', PrintJob::class);, which suggests that this action is likely restricted to users who have the permission to view any print job, typically admins.- If the
filterparameter is not "all", it calls$this->authorize('viewSelf', PrintJob::class);, indicating that users can view their own print jobs.This setup confirms that the route
print-job.indexwithfilter=allis indeed intended to be accessed by users with administrative privileges, aligning with the original review comment's concern about role-based UI customization and access control.Final Conclusion:
- The route
print-job.indexis defined and includes an optionalfilterparameter.- Access control is enforced in the
PrintJobController, with administrative access required for viewing all print jobs.This verification confirms the initial review comment's accuracy regarding the route definition and access control mechanisms.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and access control of the `print-job.index` route with `filter=all`. # Test: Check the route definition and ensure it's accessible only to admins. php artisan route:list | grep 'print-job.index'Length of output: 118
Script:
#!/bin/bash # Searching for the definition of 'print-job.index' route in Laravel route files rg "print-job.index" --glob '*routes*.php'Length of output: 191
Script:
#!/bin/bash # Broadening the search for the definition of 'print-job.index' route in all PHP files rg "print-job.index" --glob '*.php'Length of output: 535
Script:
#!/bin/bash # Check for middleware or other access control mechanisms in the PrintJobController rg "PrintJobController" --glob '*.php'Length of output: 669
Script:
#!/bin/bash # Extract the content of PrintJobController to check for middleware usage cat app/Http/Controllers/Dormitory/Printing/PrintJobController.phpLength of output: 7890
database/migrations/2023_12_25_184733_update_print_jobs_table.php (1)
16-20: Addition ofPrinterforeign key andused_free_pagescolumn, and removal offilepathare significant. Ensure the removal offilepathis documented and justified.app/Http/Controllers/Dormitory/Printing/PrintAccountHistoryController.php (1)
23-33: MethodindexPrintAccountHistoryeffectively uses pagination and authorization. Consider renaming toindexfor consistency.resources/views/user/printing.blade.php (1)
14-21: Dynamic badge colors and inclusion of modifier information enhance UI and transparency. Verify access control for viewing modifier information.resources/views/dormitory/print/manage/account_history.blade.php (1)
9-9: Set AJAX URL to the new endpoint.Verification successful
The verification process has successfully identified the route definition and the corresponding controller method for the AJAX URL used in the Blade template. The route
print-account-history.indexis correctly defined inroutes/web.phpand is handled by theindexPrintAccountHistorymethod in thePrintAccountHistoryController. This confirms that the AJAX URL is set up correctly in the Blade template to fetch the print account history.Set AJAX URL to the new endpoint.
- The route
print-account-history.indexis correctly defined and points to theindexPrintAccountHistorymethod in thePrintAccountHistoryController.- The AJAX URL in the Blade template is correctly set to fetch data from this endpoint.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the endpoint for fetching print account history is correctly implemented and returns expected data. # Test: Fetch data from the endpoint. Expect: JSON response with correct data structure. curl -s "{{ route('print-account-history.index') }}" | jq .Length of output: 67
Script:
#!/bin/bash # Search for the route definition in Laravel route files rg "print-account-history.index" --glob '{routes/*.php,app/Http/routes.php}' # Search for the controller method that handles this route ast-grep --lang php --pattern $'Route::get($_, $_->uses($$$"index"$$$))'Length of output: 443
Script:
#!/bin/bash # Search for the controller method that handles the route 'print-account-history.index' rg "function indexPrintAccountHistory" --glob '*.php'Length of output: 176
app/Models/PrintAccountHistory.php (1)
50-55: Relationship methods withUsermodel are correctly implemented.app/Http/Controllers/Dormitory/Printing/PrinterController.php (1)
24-34: Handling of admin and normal views inindexmethod is clear and functional.app/Http/Controllers/Dormitory/Printing/FreePagesController.php (3)
19-32: Method for listing current user's free pages is correctly implemented.
38-53: Method for listing all free pages for admins is correctly implemented.Verification successful
The verification process has successfully confirmed the implementation of the authorization logic in the
adminIndexmethod of theFreePagesController. The method correctly uses$this->authorize('viewAny', FreePages::class);to ensure that only authorized users can access the functionality to view all free pages. This aligns with the initial review comment that the method for listing all free pages for admins is correctly implemented.
- File:
app/Http/Controllers/Dormitory/Printing/FreePagesController.php- Method:
adminIndexThis verification confirms that the authorization logic is implemented as expected, and no further action is required regarding this specific concern.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the authorization logic in `adminIndex` method of `FreePagesController`. # Test: Attempt to access the method without admin rights. Expect: Authorization failure. curl -s -o /dev/null -w "%{http_code}" -H "Authorization: Bearer non_admin_token" "{{ route('free-pages.admin.index') }}"Length of output: 129
Script:
#!/bin/bash # Description: Check the implementation of authorization in the `adminIndex` method of `FreePagesController`. # Search for the authorization logic in the `adminIndex` method ast-grep --lang php --pattern $'class FreePagesController { $$$ public function adminIndex() { $this->authorize($_, $_); $$$ } $$$ }'Length of output: 257
Script:
#!/bin/bash # Description: Search for the authorization call in the `adminIndex` method of `FreePagesController` using ripgrep. # Use ripgrep to find the authorization logic in the `adminIndex` method rg "public function adminIndex\(\) \{" -A 10 app/Http/Controllers/Dormitory/Printing/FreePagesController.phpLength of output: 110
Script:
#!/bin/bash # Description: Verify the presence of the `FreePagesController.php` file and list its contents. # Check if the file exists and list the first 20 lines to confirm the presence of the `adminIndex` method if [ -f app/Http/Controllers/Dormitory/Printing/FreePagesController.php ]; then echo "File exists." head -n 20 app/Http/Controllers/Dormitory/Printing/FreePagesController.php else echo "File does not exist." fiLength of output: 712
Script:
#!/bin/bash # Description: Search for the `adminIndex` method in the `FreePagesController.php` file and display its contents. # Use grep to find the `adminIndex` method and display the surrounding lines for context grep -A 10 -B 3 "public function adminIndex()" app/Http/Controllers/Dormitory/Printing/FreePagesController.phpLength of output: 525
72-87: Method for adding new free pages is correctly implemented.resources/views/dormitory/print/free.blade.php (1)
Line range hint
30-45: Conditional rendering of columns based on admin status is correctly implemented.tests/Feature/RegistrationTest.php (1)
Line range hint
1-89: Registration feature tests are comprehensive and correctly implemented.app/Utils/PrinterHelper.php (1)
15-86: Utility methods for printing operations are correctly implemented.app/Models/FreePages.php (2)
55-57: Casting of thedeadlineattribute to a date type is correctly implemented.
63-92: Relationship methods inFreePagesmodel are correctly implemented.app/Providers/AuthServiceProvider.php (1)
Line range hint
57-87: Ensure role checks are correctly implemented and consider using policy classes for complex authorization logic.Verification successful
The role checks within the authorization services are correctly implemented and encapsulated within various policy classes as evidenced by the extensive usage of
hasRoleacross multiple policy files. This aligns with best practices for managing complex authorization logic, ensuring that it is maintainable and auditable.
- Files and Policies Verified:
CheckoutPolicy.phpUserPolicy.phpMrAndMissPolicy.phpGeneralAssemblyPolicy.phpEpistolaPolicy.phpTransactionPolicy.phpRoomPolicy.phpLocalePolicy.phpFaultPolicy.phpCommunityServicePolicy.phpScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify role checks and policy usage in authorization services. # Test: Search for role checks and policy definitions. Expect: Correct implementation and usage. rg --type php 'hasRole' | rg 'Policy'Length of output: 4953
resources/views/dormitory/print/history.blade.php (1)
10-16: Ensure proper handling of user input in AJAX requests to prevent XSS and CSRF vulnerabilities.database/seeders/UsersTableSeeder.php (1)
Line range hint
1-151: Ensure seeding logic is efficient and follows best practices.app/Http/Controllers/Dormitory/Printing/PrintAccountController.php (1)
21-151: Ensure controller logic is secure, efficient, and adheres to best practices.Verification successful
The verification of the
updatemethod in thePrintAccountControllerclass has been successfully conducted using thergtool. The method handles both the transfer of balances between accounts and the modification of a single account's balance. The security checks include authorization checks (authorize('transferBalance', $printAccount)andauthorize('modify', $printAccount)) and validations ($request->validate([...])). The method also handles different scenarios such as self-transfers and insufficient balances appropriately by aborting the transaction.From the provided script output, the logic appears to be secure as it properly checks user permissions and conditions before proceeding with balance modifications or transfers. The use of transactions (
DB::beginTransaction()andDB::commit()) in the helper methodstransferBalanceandmodifyBalanceensures that the database operations are atomic, which is a good practice for maintaining data integrity.Based on the information available and the script output, the controller logic seems to be secure, efficient, and adheres to best practices.
- The method correctly validates input data.
- Proper authorization checks are in place before performing sensitive operations.
- Transactions are used to ensure atomicity of balance updates.
Therefore, the initial review comment concern appears to be addressed adequately in the implementation.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the controller logic for security, efficiency, and adherence to best practices. # Test: Check for any security vulnerabilities or inefficient database queries. ast-grep --lang php --pattern $'class PrintAccountController extends Controller { $$$ update($_, $_) { $$$ } $$$ }'Length of output: 237
Script:
#!/bin/bash # Description: Use ripgrep to search for the 'update' method in the 'PrintAccountController' class to verify security and efficiency. # Search for the 'update' method within the 'PrintAccountController' class rg --no-heading --no-line-number 'public function update\(Request \$request\)' -A 30 -B 2 app/Http/Controllers/Dormitory/Printing/PrintAccountController.phpLength of output: 1638
app/Http/Controllers/Dormitory/Printing/PrintJobController.php (1)
24-233: Ensure controller logic is secure, efficient, and adheres to best practices.Verification successful
The verification process has provided insights into the security and efficiency of the
PrintJobControllerwithin the codebase. Here are the key findings:
Security: The controller uses proper authorization checks (
$this->authorize) for different actions, ensuring that only authorized users can perform sensitive operations. This is evident from the successful detection of authorization methods in the controller.Efficiency: The search for inefficient database queries (
->get()and->all()) did not yield results in the context of thePrintJobController. This suggests that the controller does not use these potentially inefficient methods directly, which is a good practice for maintaining performance.Given these observations, the controller logic appears to be secure and efficient, adhering to best practices in terms of authorization and database query optimization.
- Authorization checks are properly implemented in methods like
indexPrintJobs,store, andupdate, which handle sensitive operations related to print jobs.- No direct inefficient database queries were found in the
PrintJobController, suggesting an awareness of performance considerations in the implementation.Overall, the controller logic in
PrintJobControlleris aligned with security and efficiency best practices.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the controller logic for security, efficiency, and adherence to best practices. # Test: Check for any security vulnerabilities or inefficient database queries. ast-grep --lang php --pattern $'class PrintJobController extends Controller { $$$ store($_) { $$$ } $$$ }'Length of output: 228
Script:
#!/bin/bash # Description: Verify the controller logic for security, efficiency, and adherence to best practices using ripgrep. # Search for direct SQL queries which might be prone to SQL injection. rg --glob '*.php' 'DB::(raw|select|insert|update|delete)' # Search for missing authorization checks. rg --glob '*.php' '->authorize\(' # Search for potentially inefficient database queries. rg --glob '*.php' '->get\(\)' '->all\(\)'Length of output: 1245
Script:
#!/bin/bash # Description: Re-run the verification for missing authorization checks and inefficient database queries using corrected ripgrep syntax. # Search for missing authorization checks. rg --glob '*.php' '\->authorize\(' # Search for potentially inefficient database queries. rg --glob '*.php' '\->get\(\)' '\->all\(\)'Length of output: 12181
tests/Feature/PrintControllerTest.php (1)
Line range hint
1-234: Ensure tests are comprehensive, correct, and follow best practices.Verification successful
The test coverage in
PrintControllerTest.phpappears comprehensive based on the methods listed:
testUserWithoutPermissions()testUserWithPrinterPermissions()testUserWithPrintAdminPermissions()testBalanceTransfer()testModifyBalance()These methods cover various user roles and actions, which are crucial for thoroughly testing the functionalities of the print controller. No critical test scenarios seem to be missing.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the test coverage and correctness for the print controller. # Test: Check for any missing test scenarios or incorrect assertions. ast-grep --lang php --pattern $'class PrintControllerTest extends TestCase { $$$ testUserWithoutPermissions($_) { $$$ } $$$ }'Length of output: 248
Script:
#!/bin/bash # Description: Verify the test coverage and correctness for the print controller using rg. # Search for the test methods in the PrintControllerTest class to ensure all necessary tests are present. rg --no-heading --no-line-number 'public function test' tests/Feature/PrintControllerTest.phpLength of output: 335
resources/views/layouts/navbar.blade.php (1)
Line range hint
1-234: Ensure navigation links are correct, layout is responsive, and best practices are followed.Verification successful
The verification process has successfully identified multiple instances of responsive classes (
waves-effect) and dynamic route generation ({{ route(...) }}) within thenavbar.blade.phpfile. This confirms that the navigation links are correctly implemented and the layout includes responsive design elements, adhering to the initial review concerns about ensuring correct navigation links and responsive layout.Final Assessment:
- The navigation links utilize Laravel's
routefunction, ensuring they are dynamically generated and correct as long as the routes are correctly defined in the application's routing configuration.- The use of responsive classes like
waves-effectindicates adherence to responsive design principles, suitable for different device sizes.Recommendations:
- Continue to ensure that all routes referenced in the Blade template are defined in the application's routing files.
- Regularly test the responsiveness of the layout on various devices to maintain a high-quality user experience.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of navigation links and responsiveness of the layout. # Test: Check for any incorrect links or non-responsive elements. ast-grep --lang blade --pattern $'div.navbar-fixed { $$$ ul.sidenav { $$$ } $$$ }'Length of output: 208
Script:
#!/bin/bash # Description: Verify the correctness of navigation links and responsiveness of the layout using rg to search for responsive classes and link patterns. # Check for responsive classes and correct link structures rg --multiline 'class=".*responsive.*"|\{\{ route\(.*\)\ \}\}' resources/views/layouts/navbar.blade.phpLength of output: 256
Script:
#!/bin/bash # Description: Verify the correctness of navigation links and responsiveness of the layout using rg to search for responsive classes and link patterns. # Corrected regex to properly escape special characters rg --multiline 'class=".*responsive.*"|\{\{ route\(\'.*\'\)\ \}\}' resources/views/layouts/navbar.blade.phpLength of output: 93
Script:
#!/bin/bash # Description: Verify the correctness of navigation links and responsiveness of the layout using rg to search for responsive classes and link patterns. # Corrected regex to properly handle single quotes and special characters rg --multiline 'class=".*responsive.*"|\{\{ route\('.*'\) \}\}' resources/views/layouts/navbar.blade.phpLength of output: 2265
routes/web.php (5)
17-17: Addition ofFreePagesControllerenhances modularity in handling free pages.
18-18: Addition ofPrinterControllercorrectly modularizes printer-related functionalities.
19-19: Addition ofPrintJobControllereffectively separates print job management responsibilities.
20-20: Addition ofPrintAccountControllerenhances clarity in managing print accounts.
21-21: Addition ofPrintAccountHistoryControllercorrectly handles the history of print accounts.app/Models/User.php (1)
Line range hint
1-1: TheUsermodel appears to support the new changes in the print module effectively.
| @@ -5,7 +5,7 @@ | |||
| use Illuminate\Database\Eloquent\Model; | |||
|
|
|||
| // Note: the elements of this class should no be changed manually. | |||
| // Triggers are set up in the database (see migration). | |||
| // Obeservers are set up. | |||
There was a problem hiding this comment.
Clarify which observers are set up for PrintAccountHistory.
-// Obeservers are set up.
+// Observers for creating and updating entries are set up.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.
| // Obeservers are set up. | |
| // Observers for creating and updating entries are set up. |
| Mail::to(User::withRole(Role::SYS_ADMIN)->get())->queue(new NoPaper(user()->name)); | ||
| } | ||
| $printer->paper_out_at = now(); | ||
| $printer->save(); |
There was a problem hiding this comment.
Use the update method for consistency and safety.
- $printer->paper_out_at = now();
- $printer->save();
+ $printer->update(['paper_out_at' => now()]);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.
| $printer->save(); | |
| $printer->update(['paper_out_at' => now()]); |
| public function getAvailableFreePagesAttribute() | ||
| { | ||
| return $this->balance >= abs($balance); | ||
| return $this->freePages()->where('deadline', '>', now())->orderBy('deadline')->get(); |
There was a problem hiding this comment.
Consider using Laravel's Attribute casting instead of a method for getAvailableFreePagesAttribute.
- public function getAvailableFreePagesAttribute()
- {
- return $this->freePages()->where('deadline', '>', now())->orderBy('deadline')->get();
- }
+ protected $appends = ['available_free_pages'];
+
+ public function getAvailableFreePagesAttribute(): Attribute
+ {
+ return Attribute::make(
+ get: fn () => $this->freePages()->where('deadline', '>', now())->orderBy('deadline')->get()
+ );
+ }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.
| public function getAvailableFreePagesAttribute() | |
| { | |
| return $this->balance >= abs($balance); | |
| return $this->freePages()->where('deadline', '>', now())->orderBy('deadline')->get(); | |
| protected $appends = ['available_free_pages']; | |
| public function getAvailableFreePagesAttribute(): Attribute | |
| { | |
| return Attribute::make( | |
| get: fn () => $this->freePages()->where('deadline', '>', now())->orderBy('deadline')->get() | |
| ); | |
| } |
| public static function getFreePagesNeeeded(int $pages, $copies, $twoSided) | ||
| { | ||
| $pageTypesNeeded = self::getPageTypesNeeded($pages, $twoSided); | ||
|
|
||
| return ($pageTypesNeeded['one_sided'] + $pageTypesNeeded['two_sided']) * $copies; | ||
| } |
There was a problem hiding this comment.
Correct the typo in the method name.
- public static function getFreePagesNeeeded(int $pages, $copies, $twoSided)
+ public static function getFreePagesNeeded(int $pages, $copies, $twoSided)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.
| public static function getFreePagesNeeeded(int $pages, $copies, $twoSided) | |
| { | |
| $pageTypesNeeded = self::getPageTypesNeeded($pages, $twoSided); | |
| return ($pageTypesNeeded['one_sided'] + $pageTypesNeeded['two_sided']) * $copies; | |
| } | |
| public static function getFreePagesNeeded(int $pages, $copies, $twoSided) | |
| { | |
| $pageTypesNeeded = self::getPageTypesNeeded($pages, $twoSided); | |
| return ($pageTypesNeeded['one_sided'] + $pageTypesNeeded['two_sided']) * $copies; | |
| } |
| public function print(bool $twoSided, int $copies, string $path) { | ||
| if (config('app.debug')) { | ||
| return -1; | ||
| } | ||
| $jobId = null; | ||
| try { | ||
| $process = new Process([ | ||
| 'lp', | ||
| '-d', $this->name, | ||
| '-h', "$this->ip:$this->port", | ||
| ($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.
Refactor the print method to separate concerns between command execution and result parsing.
- public function print(bool $twoSided, int $copies, string $path) {
- if (config('app.debug')) {
- return -1;
- }
- $jobId = null;
- try {
- $process = new Process([
- 'lp',
- '-d', $this->name,
- '-h', "$this->ip:$this->port",
- ($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;
- }
+ public function print(bool $twoSided, int $copies, string $path) {
+ if (config('app.debug')) {
+ return -1;
+ }
+ $process = $this->createPrintProcess($twoSided, $copies, $path);
+ $this->executePrintProcess($process);
+ return $this->parsePrintResult($process);
+ }
+
+ protected function createPrintProcess(bool $twoSided, int $copies, string $path): Process {
+ return new Process([
+ 'lp',
+ '-d', $this->name,
+ '-h', "$this->ip:$this->port",
+ ($twoSided ? '-o sides=two-sided-long-edge' : ''),
+ '-n', $copies,
+ $path
+ ]);
+ }
+
+ protected function executePrintProcess(Process $process) {
+ $process->run();
+ if (!$process->isSuccessful()) {
+ Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $process->getErrorOutput());
+ throw new PrinterException($process->getErrorOutput());
+ }
+ }
+
+ protected function parsePrintResult(Process $process): int {
+ $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);
+ }
+ return intval($matches[1]);
+ }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.
| public function print(bool $twoSided, int $copies, string $path) { | |
| if (config('app.debug')) { | |
| return -1; | |
| } | |
| $jobId = null; | |
| try { | |
| $process = new Process([ | |
| 'lp', | |
| '-d', $this->name, | |
| '-h', "$this->ip:$this->port", | |
| ($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; | |
| public function print(bool $twoSided, int $copies, string $path) { | |
| if (config('app.debug')) { | |
| return -1; | |
| } | |
| $process = $this->createPrintProcess($twoSided, $copies, $path); | |
| $this->executePrintProcess($process); | |
| return $this->parsePrintResult($process); | |
| } | |
| protected function createPrintProcess(bool $twoSided, int $copies, string $path): Process { | |
| return new Process([ | |
| 'lp', | |
| '-d', $this->name, | |
| '-h', "$this->ip:$this->port", | |
| ($twoSided ? '-o sides=two-sided-long-edge' : ''), | |
| '-n', $copies, | |
| $path | |
| ]); | |
| } | |
| protected function executePrintProcess(Process $process) { | |
| $process->run(); | |
| if (!$process->isSuccessful()) { | |
| Log::error("Printing error at line: " . __FILE__ . ":" . __LINE__ . " (in function " . __FUNCTION__ . "). " . $process->getErrorOutput()); | |
| throw new PrinterException($process->getErrorOutput()); | |
| } | |
| } | |
| protected function parsePrintResult(Process $process): int { | |
| $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); | |
| } | |
| return intval($matches[1]); | |
| } |
| /** | ||
| * Attemts to cancel the given `PrintJob`. Returns wether it was successful. | ||
| * @param PrintJob $this | ||
| * @return PrinterCancelResult | ||
| */ | ||
| public function cancel() | ||
| { | ||
| return function ($data) { | ||
| $data->cost = "{$data->cost} HUF"; | ||
| $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name')); | ||
| $process = new Process(['cancel', $this->job_id, '-h', "$printer->ip:$printer->port"]); | ||
| $process->run(); | ||
| $result = ['output' => $process->getOutput(), 'exit_code' => $process->getExitCode()]; | ||
|
|
||
| return $data; | ||
| }; | ||
| if ($result['exit_code'] == 0) { | ||
| return PrinterCancelResult::Success; | ||
| } | ||
| if (strpos($result['output'], "already canceled") !== false) { | ||
| return PrinterCancelResult::AlreadyCancelled; | ||
| } | ||
| if (strpos($result['output'], "already completed") !== false) { | ||
| return PrinterCancelResult::AlreadyCompleted; | ||
| } | ||
| return PrinterCancelResult::CannotCancel; |
There was a problem hiding this comment.
Refactor the cancel method to improve readability and error handling.
- public function cancel() {
- $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name'));
- $process = new Process(['cancel', $this->job_id, '-h', "$printer->ip:$printer->port"]);
- $process->run();
- $result = ['output' => $process->getOutput(), 'exit_code' => $process->getExitCode()];
- if ($result['exit_code'] == 0) {
- return PrinterCancelResult::Success;
- }
- if (strpos($result['output'], "already canceled") !== false) {
- return PrinterCancelResult::AlreadyCancelled;
- }
- if (strpos($result['output'], "already completed") !== false) {
- return PrinterCancelResult::AlreadyCompleted;
- }
- return PrinterCancelResult::CannotCancel;
- }
+ public function cancel(): PrinterCancelResult {
+ $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name'));
+ $process = new Process(['cancel', $this->job_id, '-h', "$printer->ip:$printer->port"]);
+ $process->run();
+ return $this->determineCancellationResult($process);
+ }
+
+ protected function determineCancellationResult(Process $process): PrinterCancelResult {
+ if ($process->getExitCode() == 0) {
+ return PrinterCancelResult::Success;
+ }
+ $output = $process->getOutput();
+ if (strpos($output, "already canceled") !== false) {
+ return PrinterCancelResult::AlreadyCancelled;
+ }
+ if (strpos($output, "already completed") !== false) {
+ return PrinterCancelResult::AlreadyCompleted;
+ }
+ return PrinterCancelResult::CannotCancel;
+ }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.
| /** | |
| * Attemts to cancel the given `PrintJob`. Returns wether it was successful. | |
| * @param PrintJob $this | |
| * @return PrinterCancelResult | |
| */ | |
| public function cancel() | |
| { | |
| return function ($data) { | |
| $data->cost = "{$data->cost} HUF"; | |
| $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name')); | |
| $process = new Process(['cancel', $this->job_id, '-h', "$printer->ip:$printer->port"]); | |
| $process->run(); | |
| $result = ['output' => $process->getOutput(), 'exit_code' => $process->getExitCode()]; | |
| return $data; | |
| }; | |
| if ($result['exit_code'] == 0) { | |
| return PrinterCancelResult::Success; | |
| } | |
| if (strpos($result['output'], "already canceled") !== false) { | |
| return PrinterCancelResult::AlreadyCancelled; | |
| } | |
| if (strpos($result['output'], "already completed") !== false) { | |
| return PrinterCancelResult::AlreadyCompleted; | |
| } | |
| return PrinterCancelResult::CannotCancel; | |
| /** | |
| * Attemts to cancel the given `PrintJob`. Returns wether it was successful. | |
| * @param PrintJob $this | |
| * @return PrinterCancelResult | |
| */ | |
| public function cancel(): PrinterCancelResult | |
| { | |
| $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name')); | |
| $process = new Process(['cancel', $this->job_id, '-h', "$printer->ip:$printer->port"]); | |
| $process->run(); | |
| return $this->determineCancellationResult($process); | |
| } | |
| protected function determineCancellationResult(Process $process): PrinterCancelResult { | |
| if ($process->getExitCode() == 0) { | |
| return PrinterCancelResult::Success; | |
| } | |
| $output = $process->getOutput(); | |
| if (strpos($output, "already canceled") !== false) { | |
| return PrinterCancelResult::AlreadyCancelled; | |
| } | |
| if (strpos($output, "already completed") !== false) { | |
| return PrinterCancelResult::AlreadyCompleted; | |
| } | |
| return PrinterCancelResult::CannotCancel; | |
| } |
| * @throws BindingResolutionException | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| public function indexPrintAccountHistory() |
| * @param null|string $filter Decides wether all `PrintJob`s or just the user's `PrintJob`s should be listed. | ||
| * @return LengthAwarePaginator | ||
| */ | ||
| public function indexPrintJobs(?string $filter = null) |
| */ | ||
| public function indexPrintJobs(?string $filter = null) | ||
| { | ||
| if ($filter === "all") { |
There was a problem hiding this comment.
This is still valid. Like what you did in FreePagesController to keep it consistent.
| @@ -0,0 +1,48 @@ | |||
| { | |||
There was a problem hiding this comment.
do we want these files in git?
| if ($useFreePages) { | ||
| $freePagesToSubtract = $cost; | ||
| $freePages = $printAccount->available_free_pages->where('amount', '>', 0); | ||
|
|
||
| /** @var FreePages */ | ||
| foreach ($freePages as $pages) { | ||
| $subtractablePages = min($freePagesToSubtract, $pages->amount); | ||
| $pages->update([ | ||
| 'last_modified_by' => user()->id, | ||
| 'amount' => $pages->amount - $subtractablePages, | ||
| ]); | ||
|
|
||
| $freePagesToSubtract -= $subtractablePages; | ||
|
|
||
| if ($freePagesToSubtract <= 0) { // < should not be necessary, but better safe than sorry | ||
| break; | ||
| } | ||
| } | ||
| // Set value in the session so that free page checkbox stays checked | ||
| session()->put('use_free_pages', true); | ||
| } else { | ||
| $printAccount->balance -= $cost; | ||
|
|
||
| // Remove value regarding the free page checkbox from the session | ||
| session()->remove('use_free_pages'); | ||
| } |
There was a problem hiding this comment.
Still relevant. Modifying free pages or balance should be at least in a helper function. Also the document should be printed after we modified the balance as it can not be rolled back on fail.
| * @param PrintJob $job | ||
| * @return RedirectResponse | ||
| */ | ||
| public function update(PrintJob $job) |
There was a problem hiding this comment.
Or name it update but add another parameter what should be the new status. Maybe later we can reimburse prints or whatever.
| { | ||
| return function ($data) { | ||
| $data->cost = "{$data->cost} HUF"; | ||
| $printer = $this->printer ?? Printer::firstWhere('name', config('print.printer_name')); |
There was a problem hiding this comment.
Again, why don't we always have a printer set in the db? I would delete the config entry or use only that to avoid confusion.
| Route::post('/print/modify_balance', [PrintController::class, 'modifyBalance'])->name('print.modify'); | ||
| }); | ||
| Route::post('/print/add_free_pages', [PrintController::class, 'addFreePages'])->name('print.free_pages')->middleware('can:create,App\Models\FreePages'); | ||
| Route::get('/printer/{page?}', [PrinterController::class, 'index'])->name('printer.index'); // also print.manage, print |
There was a problem hiding this comment.
what are the stuff in comments?
PrintControllerintoPrinterController,PrintJobController,PrintAccountController,PrintAccountHistoryController,FreePagesController.Printerutility class, and printing related commands from theCommandsutility.printerstable. This is mostly done for better REST compatibility and cleaner code, but still is useful if (when) multiple printers are installed at some point.Closes #51, #374, #366 and #344
Summary by CodeRabbit
New Features
Bug Fixes
pingRouterandlatexToPdfmethods to handle processes more efficiently.Documentation
Refactor
Chores
Tests