Asset Store/Update/Destroy Actions #15893
Conversation
Updated error handling to ensure all exceptions are properly stored in the errors array and removed unused custom_field_problem variable. Also, uncommented location assignment code in UpdateAssetAction for clearer logic.
Improved the error handling in the BulkAssetsController by re-enabling custom field permission checks. Updated the UpdateAssetAction to better handle encrypted custom fields and streamlined user roles in the BulkEditAssetsTest.
This commit improves the asset update functionality by allowing null values for purchase date, next audit date, and expected check-in date when specified in the request. Additionally, minor corrections were made in the test code to enhance readability and correctness.
Refactor the asset update process to properly associate models and handle location ID changes based on request input. Introduce checks for model_id, company_id, and last_audit_date to ensure correct updates. Add commentary to clarify the handling of location ID logic and temporarily resolve issues with location assignment when requests do not expect JSON responses.
Corrected spelling mistakes in test files and updated code to use the correct array notation in request validation for asset updates. This ensures better readability and consistency in the codebase while preventing potential errors related to incorrect data handling.
Added try-catch blocks to manage ValidationException and generic exceptions during asset saving. This ensures proper logging of errors and reporting of unexpected issues, improving robustness and error handling in the asset import process.
Updated exception handling to capture detailed validation errors in BulkAssetsController. Corrected formatting in notifications view for displaying error messages.
Previously, the error message was set as a string, which led to an inconsistency in how errors were handled. This change updates the error message to be an array, ensuring it aligns with the expected format.
This commit removes various debug dump statements from BulkAssetsController and UpdateAssetAction. These changes clean up the code and improve readability by eliminating unnecessary debugging output. Maintaining clean code ensures better maintainability and performance.
Removed commented-out code related to handling encrypted custom fields in UpdateAssetAction.php. This cleanup helps to maintain a more readable and maintainable codebase.
Refactored error handling by replacing `\Exception` with `Exception` for consistency. Removed redundant comments and unused code to improve readability and maintainability of the `StoreAssetAction` class.
This change introduces the CheckoutNotAllowed exception to the UpdateAssetAction class. It ensures that the action can handle scenarios where checking out an asset is not permitted, improving the robustness of the application.
Removed outdated and redundant comments for improved code clarity and readability. Renamed `bulkUpdate` method to `bulkLocationUpdate` for better context and understanding.
Introduced StoreAssetTest to verify the handling of full asset records and multiple asset entries in the database. Updated StoreAssetAction and AssetsController to support next audit date validation and improved error handling.
Removed the redundant $request parameter from the destroy method in AssetsController. Added feature tests to verify asset deletion functionality with proper permissions and ensure deletion is forbidden without permissions.
Implement tests to ensure users without appropriate permissions are denied access to asset endpoints. Update tests to verify proper soft deletions and asset existence checks in delete scenarios.
Removed incorrect and commented-out authorization check in the destroy method. Ensured proper authorization by explicitly authorizing the asset instance before attempting deletion.
PR Summary
|
|
changed to draft because of all the changes I wasn't aware of in the controllers |
Refactor asset creation and update processes by removing redundant save checks and organizing error handling logic. Enhance success and failure feedback in the asset creation flow to improve clarity and user experience. Add support for detailed multi-success and partial failure messages to alert users of outcomes.
…ction # Conflicts: # app/Http/Controllers/Api/AssetsController.php # app/Http/Controllers/Assets/AssetsController.php # app/Http/Controllers/Assets/BulkAssetsController.php # resources/lang/en-US/admin/hardware/message.php
Simplify the code by cleaning up unused import statements in both `AssetsController.php` and `Api/AssetsController.php`. This reduces potential confusion and helps maintain cleaner code for future development and maintenance.
There was a problem hiding this comment.
😮💨 that was a doozy. Thanks for all of your work on this.
I haven't manually tried the code paths yet but I have (a bunch) of preliminary comments. Most of them are about removing commented code (keeping it cleaner so we don't carry it around forever) but some are legit questions about potential changes.
Thanks again for the effort. I know it's a lot.
| 'category_id' => Category::factory()->assetLaptopCategory()->create(), | ||
| 'purchase_date' => now()->subDecade(), | ||
| //not sure how this ever worked... do these need a category?? | ||
| //'category_id' => Category::factory()->assetLaptopCategory()->create(), |
There was a problem hiding this comment.
I think we can remove this comment. I think turning on validation exceptions is causing this?
| 'category_id' => Category::factory()->assetLaptopCategory()->create(), | ||
| 'purchase_date' => now()->subDecade(), | ||
| //not sure how this ever worked... | ||
| //'category_id' => Category::factory()->assetLaptopCategory()->create(), |
There was a problem hiding this comment.
I think we can remove this comment. I think turning on validation exceptions is causing this?
|
Awesome, thank you so much for all of that @marcusmoore - a lot of that I probably should've found/fixed myself, I think I've just been too close to it for a while. I just went through them and am with you on all of it, I'll implement it all in the morning. |
This refactoring introduces helper methods for handling image uploads, custom fields, and checkouts, improving code organization and readability. Unnecessary imports, comments, and unused variables were removed for cleaner code. Error handling was also improved with additional exception reporting in the asset update flow.
Description
This PR introduces the concept of actions to both Assets controllers to begin to unify the way we do things, as well as replacing a lot of the custom logic that the BulkAssetsController's update method performed.
Now, instead of disparate methods for updating one or many assets, we perform the same logic either way and loop over the action to do it multiple times. The business logic is largely removed from the controllers in these situations, and the logic that is performed inside them consists of setting up the arrays, looping, and catching/returning errors or success.
At first glance the long method signatures can seem unwieldy, but I've come around to them and kind of like how specific they are, by doing this you can use the
StoreAssetActionhow ever you'd like.For example:
The
StoreAssetActiononly really requires three arguments,model_id,status_id, and at least for now an instance ofImageUploadRequest(I'll get into why that is later). Everything else is optional.Using named parameters (PHP 8.1 and up) this makes the action very flexible, and we can use it anywhere we'd like to make a new asset, including on the CLI. PHPStorm also takes advantage of named parameters and method signatures very well, making it hard to get the action wrong in practice.
StoreAssetAction::run(model_id: $request->validated('model_id'), status_id: $request->validated('status_id'), $request, ...)Why are we passing the full request in?
The
handleImages()method on theImageUploadRequestis what does all the image processing, and it uses a lot of request context to do so at the moment. I'd like to see this moved out into some kind of helper method or maybe promoted up to the extended request (as in StoreAssetRequest) - but that seemed like a bit much for this PR.Custom fields logic is also based on the request currently because the code doesn't necessarily know what the possible custom fields in the request are going to be. This is also solvable, but will wait on that for a little bit.
In general, we don't want to pass the request beyond the controller - requests should stay at the Http layer and actions should be able to function purely, without any knowledge of the request, but again because we require it for handling images we're willing to break some rules in the hopes that it will change in the future.
Also, the pattern of try/catching all Actions ensures that eventually we should be able to get to a place where a user should never encounter a 500, the
report()method ensures that we are still alerted by them though.Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Existing tests pass, and some new ones written.
Test Configuration: