Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add Form Request and Tests for Update Asset API Method #14458

Open
wants to merge 43 commits into
base: develop
Choose a base branch
from

Conversation

spencerrlongg
Copy link
Collaborator

@spencerrlongg spencerrlongg commented Mar 20, 2024

Description

This does a couple things:

Adds a new Form Request for Asset Updates to continue our rollout of that feature.

Unfortunately, the only way it makes sense to do this on an update request was to duplicate the validation set. There were a couple reasons for this:

  • Required rules don't work well on duplicate, I did experiment with just resupplying them in the prepareForValidation() method, but it got a little confusing because of:
  • The unique_undeleted rule does not play nice in a form request because of the trait on the model that makes it work.

I still believe that the form request itself is still worth it, it makes the application safer by making sure that our data is validated before we operate on it. And after all, an update request is still it's own request with it's own logic, so it stands to reason rules could be slightly different.


Adds the SubstituteBindings middleware to API routes to allow us to use Route Model Binding.

https://laravel.com/docs/8.x/routing#route-model-binding

Really just a starting step to allowing us to get rid of a lot of lines like

if ($asset = Asset:find($asset_id) {
    ...
} // return some model-not-found message

SubstituteBindings is default middleware in Laravel now, so I'm not sure why it wasn't there and I couldn't find any PRs or Issues about it being removed on purpose, I'm assuming it was a symptom of an upgrade and it didn't get added because we already had custom stuff going on in our Kernel - but that's a guess, please let me know if there's a reason it isn't there, and I can look into other options.

The model-not-found exception is already inline with what we had written manually, so the error bubbles up the same if a model isn't found.

This only effects areas in which we're using type-hinting with a method & model (like public function update(Asset $asset)), so we shouldn't see any adverse effects from this from what I can tell.

Fixes # [sc-24884]

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Tests written
Test Configuration:

  • PHP version: 8.1
  • MySQL version 8.2

Copy link

Copy link

what-the-diff bot commented Mar 20, 2024

PR Summary

  • Modification of Asset Controller
    The Asset Controller is now using a new system, called "UpdateAssetRequest," to update various asset attributes. The old system, called "ImageUploadRequest," is no longer in use.

  • Kernel Modification
    In the file 'Kernel', we included a new middleware (sort of like a compliance officer), called 'SubstituteBindings.' This change is specifically for our API operations.

  • New File for Asset Updating
    We created a new file 'UpdateAssetRequest' to supervise the authorization and validation, making sure everything is according to the rules when updating an asset.

  • Asset Model Modification
    Our Asset model got an upgrade. It can now update the dynamic validation rules depending on the selected model of the asset.

  • ValidationServiceProvider Modification
    In our 'ValidationServiceProvider', we added a custom validation rule. This rule checks if a unique serial number exists, according to the settings.

  • AssetFactory Upgrade
    Our AssetFactory can now create an asset without the need for a purchase or end-of-life date. This gives us more versatility when creating assets.

  • API Route Update
    To make things more organized in our 'api.php' routes file, we've separated the 'update' route from the group of resource routes.

  • AssetStoreTest Enhancement
    We improved our 'AssetStoreTest' to test if the end-of-life date is calculated accurately when a purchase date is set.

  • New Asset Update Test
    We have a new file 'AssetUpdateTest.php' which is designed to test the process of updating assets.

  • Thorough Testing
    This new test file covers a wide range of scenarios when updating asset attributes. It helps ensure the changes we make to asset attributes get the expected results.

  • Future Improvements Indicated
    Our testing methods come with TODOs and comments, shedding light on possible areas of improvement or issues to address in the future.

@spencerrlongg spencerrlongg marked this pull request as ready for review March 20, 2024 21:01
@spencerrlongg spencerrlongg removed the request for review from snipe March 20, 2024 21:01
@spencerrlongg spencerrlongg changed the base branch from develop to snipeit_v7_laravel10 May 23, 2024 18:55
@spencerrlongg
Copy link
Collaborator Author

@uberbrady / @snipe conflicts are resolved and this is retargeted at v7.

@spencerrlongg spencerrlongg added this to the Snipe-IT v7 milestone May 23, 2024
@marcusmoore marcusmoore self-requested a review June 6, 2024 00:28
@spencerrlongg
Copy link
Collaborator Author

@marcusmoore any idea what's going on with this SQLite test?

@snipe
Copy link
Owner

snipe commented Jul 2, 2024

107) Tests\Feature\Api\Users\UpdateUserApiTest::testUsersGroupsAreNotClearedIfNoGroupPassedBySuperUser
LogicException: Invalid key supplied

WAT?

@snipe
Copy link
Owner

snipe commented Jul 2, 2024

@spencerrlongg long shot, but if you can retarget (again) to develop, since v7 is live, maybe that will help?

@marcusmoore
Copy link
Collaborator

Yeah, I'm pretty sure I had to mess with the SQLite tests after v7 got merged. Catching the branch up should help.

@spencerrlongg
Copy link
Collaborator Author

alright, this should be good to go now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants