Show error when assigned_to not null but assigned_type is null

[snipe]

This is a slight re-think of #14458 by @spencerrlongg. I'd be curious for feedback on it.

Need to test:

• API calls still work as expected
• No adverse affects on Importer

[what-the-diff]

PR Summary

• Enhancements made to Asset Handling
  In the AssetsController.php file, we've incorporated a new module dedicated to managing images. This also includes updated custom fields and improved management of user, asset, and location assignments.

• Improved Validation for Asset Checkout
  The AssetCheckoutRequest.php file now features a new method that prepares for validation. This includes updated validation rules for fields like assigned_user, assigned_asset, and assigned_location. The checkout_to_type rule has also been updated.

• Introduction of deleted() Method in Location and User Factories
  In both LocationFactory.php and UserFactory.php files, we've added a new method named deleted(). This produces a state where the deletion time and date are set at random.

• Addition of Comprehensive Test Cases for Asset Update
  In our AssetUpdateTest.php file, we've designed new test cases that include a variety of scenarios. These tests validate behaviors like checking out assets to users, attempting checkouts to deleted users (which should fail), and similar tests for locations and other assets.

[uberbrady]

I like this approach - it looks like what we're doing here is figuring out if the user is passing a user, location, or asset that they're changing the assignment to, and then we're dynamically setting the assigned_type accordingly. Much easier. So long as it doesn't break current implementations that are already using assigned_type.

There's a few bits in the replace method that I don't fully understand, but that could be just me not understanding that part so well. If they're already working the way they're supposed to, I would love a few more comments explaining what we're doing there, and why.

[spencerrlongg]

I don't think this really has much to do with what I did in #14458 - I think they could be complimentary, though.

In #14458 I'm adding Form Request validation to the update() method to remove the possibility of data getting through to controllers that hasn't been validated yet, much like I did with the store() method a while back which I think has proven to be helpful in reducing 500s for things like arrays being submitted where we don't accept them.

#14458 also adds route model binding to the API middleware so that we can start reducing a lot of the repetitive logic around finding assets or returning 404 etc. (longer term goal to add it to the gui's middlewaret too)

I did at some point experiment with putting the logic for determining assigned type in prepareForValidation() but I ran into an issue with it, just can't remember what exactly and you could have very possibly solved it here.

[marcusmoore]

From a quick scan I agree with @spencerrlongg that this might be complimentary to #14458 for the reasons he listed. We might want to make some changes to that PR but overall form request validation and route model binding will help clear up a lot of code from our controllers.

[marcusmoore]

I haven't given this a full look yet but this is really standing out to me. I think calling a request in this way and not in the expected "parameter in a controller method" is, for me, jarring and not in the spirit how they should be used 😅

[snipe]

I opted to do it this way to consider PATCH and POST, so we skip the whole validation altogether if the request doesn't have the field or the field is empty.

Open to hearing alternate suggestions tho.

[uberbrady]

No, I completely misunderstood - leaving it here in case someone was scratching their head looking at it.

Wait @marcusmoore I think I understand what you're saying - you're saying

Instead of saying:

$request->filled('assigned_user')

We should be saying:

$this->assigned_user

To fit with the rest of the whole "FormRequest ethos"? If that's all that's no big deal and, yes, you're right.

[snipe]

We invoke FormRequests independently using the app facade elsewhere in the code. (See the image/file upload requests sprinkled throughout), so it's not like a one-time thing. 90% of the time, the update is just an update. Sometimes it's not and it's also a checkout.

[snipe]

@uberbrady I think he's talking about the method signature itself and the manual invocation of the form request (so we can use more than one form request within a controller) - but I could be wrong :)

[marcusmoore]

I opted to do it this way to consider PATCH and POST, so we skip the whole validation altogether if the request doesn't have the field or the field is empty.

Can we introduce that logic to the form request itself? On a similar note, I'm not all that adverse to the duplication that comes with having dedicated "Store" and "Update" form requests because of how often we end up with different rules for each operation but that's a bigger topic for another time.

We invoke FormRequests independently using the app facade elsewhere in the code. (See the image/file upload requests sprinkled throughout), so it's not like a one-time thing. 90% of the time, the update is just an update. Sometimes it's not and it's also a checkout.

But it still goes against the traditional way form requests are invoked and intended to be used. I'm all for finding unique ways to play with the tools we're provided but I'm hesitant/side-eyeing this one...

[spencerrlongg]

I went ahead and added your tests and rules to #14458 to see what it would look like - you can view the specific changes here: 8ce577d

Happy to talk about this some more.

(It says the tests are failing but that's due to something in the base branch)

[snipe]

Where did we net out on this? Do we want @spencerrlongg's and mine? Just his, just mine?

[marcusmoore]

@snipe I'm still very hesitant with the inlined app('App\Http\Requests\AssetCheckoutRequest'). You mentioned we invoke ImageUploadRequest::class inline elsewhere in the application but from what I can tell that is to call the handleImages method on that class which feels different that what was done in the PR (invoking validation in this PR vs calling a method on the class).

I really think we should strive for validation to sit at the very top of the controller methods. We have a lot of other places where validation is in the middle of controller methods that I'd like to see moved up and I want to avoid adding to that with this specific line.

[marcusmoore]

@snipe I think since @spencerrlongg updated his PR to cover the tests included in this PR we should go with that one. In either case, his PR covers some additional use-cases so we'd want to get that in as well I think.