Add Form Request and Tests for Update Asset API Method

[spencerrlongg]

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

[shortcut-integration]

This pull request has been linked to Shortcut Story #24884: ErrorException: Object of class Illuminate\Database\Eloquent\Collection could not be converted to int....

[what-the-diff]

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.

[marcusmoore]

A couple things to address but it looks good 😄

[marcusmoore]

Changes look good to me 😄

[spencerrlongg]

I can not figure out how to get rid of this, sorry 🙄

[spencerrlongg]

Conflicts are resolved here @snipe

[spencerrlongg]

Ack, need to get marcus' changed test in from the other day

[spencerrlongg]

Cool, tests passing and last_audit_date fixed up

[spencerrlongg]

working on fixing latest conflict and getting this all correct again, will ping again when it's actually good to go

[spencerrlongg]

alright, this is good to go again

[marcusmoore]

Looks good to me 😄

[snipe]

Why are we extending ImageUploadRequest here?

[spencerrlongg]

Because we still use some methods from ImageUploadRequest in the controller, this makes it so they're still available via the request - we do the same thing in StoreAssetRequest

[uberbrady]

This looks great! I love the route-model binding thing, and I like the FormRequest changes, and I love the new tests. Unfortunately, due to a bug, we needed to merge the other PR first to fix the couple of customers who were having some problems. But I think if you can fix the conflicts here - and make sure that both your tests and the new tests I wrote all pass - I think we'll be in some pretty good shape. I had some more specific comments on some of the rule duplication which made me a little bit concerned, which you can view in-line.

Another thing you might want to consider (but is not necessary) would be to squash a lot of these changes into a new set of changes - once you've fixed the conflicts, of course. But that's your call; either way is fine.

I think if we can figure out a better way to handle the rule duplication problem and address some of the minor nits I raised, I'd be ready for us to take this.

It's some excellent work, and I'm really excited to see where this stuff is going! Thank you!

[uberbrady]

I get these three rules - where we are removing the 'required' attribute. But a lot of the rest of the rules look exactly the same as the rules on the model. Can we do a kind-of 3-way array_merge() here, where we grab the Asset's $rules (or maybe rules()?), override these couple of ones where we need to pull the required attribute, then grab the parent::rules() and merge those in too? I feel like that would cut down on the duplication. Or, another way you could go - grab the Asset::rules, and grep-out the 'required' attributes, and merge in the parent::rules()?

I just hate repeating rules in multiple places and fear that if we change some constraint somewhere - let's say, we decide to make 'name' have a max of 1023 characters for example - then having to go and update that in 4 or more places. That feels like a foot-gun waiting to happen.

[spencerrlongg]

Alright, I've put a couple of different options for resolving this in the rules() method - $rules & $rules2

One quick thing, the conversion of the rule strings to arrays in the model is not necessary if we go with option 1 ($rules) - converting them to arrays just makes it a little easier to manipulate.