Tests For API Asset Store + Refactor For Auto Increment & Form Requests #13964

spencerrlongg · 2023-11-28T21:28:43Z

Description

This is fairly large but I think will end up being a good simplification of some things.

I've refactored the store() method in the controller to just ->fill($request->validated()) with the already validated attributes from the form request, just makes things a little more concise and reduce some duplication (will be really nice when this gets applied to the GUI controller as well - hopefully next week 🤞).

The only real effect this has is now basically everything needs to be included on the rules array in the model. I didn't change to using Asset::create() so we still have access to everything we're used to, and I even left a couple of things in the model that make more sense there - like $asset->user_id = Auth::id();

The way I think about it, anything a user is setting should be included in the validation rules, and anything we're setting explicitly should be set in the controller still.

I also got rid of setting archived, depreciate, and physical in the controller. archive and depreciate are both defaulted at the db level to what we were setting them to, so we shouldn't notice any difference there. physical is not defaulted at the db level, but it is nullable, and I couldn't find any evidence of it being used anywhere. In fact, I could only find archived being used in a single scope, but decided to leave it be for now as it should continue to work the same.

last_audit_date was included in the documentation, but wasn't in the controller, since that's not in the validation rules right now it will continue to not be set if included in the request.

A good amount of tests have been added, and I'm happy to go add any more that we feel we're missing now that I've got a handle on how to do that.

The AutoIncrement function seems to work just fine at the prepareForValidation() method (and there's a test written for this as well).

Thanks to @marcusmoore for help with tests (and for having already written a couple 🙂)

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?

Tested manually and also wrote some tests.

Test Configuration:

PHP version: 8.1
MySQL version 8.1

what-the-diff · 2023-11-28T21:29:18Z

PR Summary

Modifications in AssetsController.php
The code for storing assets has been updated. We have included additional information about the assets and implemented a new method to validate these inputs.
Changes in StoreAssetRequest.php
We've adjusted how we validate asset data before storing it. We included a process to provide default values for the asset's tag and the company it belongs to, if these aren't provided.
Updates in Asset Model (Asset.php)
We've updated the rules for validating the company attribute and the asset attributes (things that define the asset) when saving assets.
Additions to AssetStoreTest.php Test Case
New tests have been added to make sure all information about an asset is correctly stored and asset tags are increasing as expected. We also instituted checks for unique serial numbers, and how asset records behave when they are removed.
Expansion of CustomTestMacros.php
A new testing feature has been added to check the status message during automated testing.
Augmented Support/Settings.php Class
New options to adjust settings related to asset tags and unique serial numbers have been added - they can now be turned on or off for more flexibility.

marcusmoore

Ah, we broke some other tests because the Asset's factory doesn't adhere to the rules that were added. I made a comment about how to update the factory for order_number to get us back to green.

Might be helpful but my process for finding the issues was to drop a dd($asset) right after Asset::factory()->create() in a test that was failing, seeing what errors were in the validationErrors property, then updating the factory, and repeating.

app/Models/Asset.php

marcusmoore

This tests are making me 🥲 😄

Solid work 🎉

marcusmoore · 2023-11-30T23:11:12Z

tests/Feature/Api/Assets/AssetStoreTest.php

+        $this->assertEquals($userAssigned->id, $asset->assigned_to);
+        $this->assertTrue($asset->company->is($company));
+        // I don't see this on the GUI side either, but it's in the docs so I'm guessing that's a mistake? It wasn't in the controller.
+        // $this->assertEquals('2023-09-03', $asset->last_audit_date);


@snipe can verify but yeah I think it can be removed...

tests/Feature/Api/Assets/AssetStoreTest.php

marcusmoore · 2023-11-30T23:20:40Z

tests/Feature/Api/Assets/AssetStoreTest.php

+        $this->assertTrue($apiAsset->adminuser->is($user));
+        $this->assertTrue($apiAsset->checkedOutToAsset());
+        // I think this makes sense, but open to a sanity check
+        $this->assertTrue($asset->assignedAssets()->find($response['payload']['id'])->is($apiAsset));


I also think this makes sense... 😅

uberbrady

I feel like this is a masterclass on how to write PR's. It has everything I like:

TONS of new tests
a full explanation of why the choices that were made were made in the PR description
comments in the tricky bits - e.g. "This looks weird, why are we doing this?"
additional sprinkles of static typing when it's appropriate
Lovely cleanups of some verbose code
I'm going to want to take this for a spin, but this looks absolutely exquisite and I can't wait to get it live. Nice work! As soon as I've run through some smoke-testing I'll approve it and try to get it merged.

snipe · 2023-12-13T10:10:57Z

app/Http/Controllers/Assets/AssetsController.php

@@ -131,9 +131,6 @@ public function store(ImageUploadRequest $request)
            $asset->order_number            = $request->input('order_number');
            $asset->notes                   = $request->input('notes');
            $asset->user_id                 = Auth::id();
-            $asset->archived                = '0';
-            $asset->physical                = '1';
-            $asset->depreciate              = '0';


It's possible we put these defaults in here so that MySQL doesn't throw up on them if they're null and the db field is not nullable.

archive and depreciate are both defaulted at the db level to what we were setting them to, so we shouldn't notice any difference there. physical is not defaulted at the db level, but it is nullable, and I couldn't find any evidence of it being used anywhere.

I think we're good and I never got it to error.

We've been bitten by that before though. If you pass it and it's null, without mutators (like we did for qty), it can cause MySQL to crash out.

Seems fine, writing a real test for it now.

Tests added.

uberbrady · 2023-12-14T13:38:34Z

Just pulled it down, and the first thing I did is run all of the tests - I did see two that didn't pass from our lovely new test suite, so I'm going to see if there's something simple that we can fix there, or if we have something deeper I need to look at. I'll report back. If I get stuck, or it seems too heavy for me to handle, I'll just set the PR to "request changes."

uberbrady · 2023-12-14T13:51:23Z

Ah! Okay, it seems like the test passes in MySQL, but fails in SQLite. While we don't really like supporting alternate databases, we do kinda-sorta try and do it, when we can. (If it fails in SQLite, it might also fail on PostgreSQL, which we do have some users on. So I think if we can fix this to pass there, we should be in good shape. I think it may also be possible that this failure could be related to [SC-24174] - which I may also have some thoughts on. If we can fix this as it is, without fixing [SC-24174], then that would be great! I would rather not conflate the two together :/

shortcut-integration · 2023-12-14T13:51:27Z

This pull request has been linked to Shortcut Story #24174: Auto-increment can exceed BIGINT mysql limitations and crash out.

uberbrady

LOVE the new tests btw! As I mentioned in my comment these all PASS in MySQL - but I get two failures in SQLite. Seems to possibly be related to asset tags? Anyways, more details in the comments.

If we can get this to pass in both database, I'm ready to approve it!

spencerrlongg · 2023-12-17T19:13:59Z

I'll work on that this week at some point. Thanks!

snipe · 2023-12-19T14:07:29Z

app/Http/Requests/StoreAssetRequest.php

@@ -20,7 +20,20 @@ public function authorize(): bool

    public function prepareForValidation(): void
    {
-        //
+        // this needed to be added in because the observer fails if `assigned_to` is null :shrug:


Can't remember, and that's a dumb comment on my part, I'll look back into it this week and try to remember exactly what the deal was.

@snipe i removed that bit and everything seems to be working fine and checkout tests are still passing. not totally sure what it was about, it may have been failing before i finished getting the rules array set up and i put it in as a stop gap.

spencerrlongg · 2024-01-03T00:28:49Z

@uberbrady tests fixed for SQLite (problem was 'next_auto_tag_base' in settings for ->disableAutoIncrement was being set as an empty string, which isn't castable as an int so it was failing, just changed it to be 0)

some refactor + tests

75ac7f8

probot-autolabeler bot added api backend tests labels Nov 28, 2023

spencerrlongg added 2 commits November 28, 2023 17:16

get rid of deleted_at as it doesn't do anything

5b77e66

cleanup validation rules a little

4aae82f

marcusmoore reviewed Nov 29, 2023

View reviewed changes

app/Models/Asset.php Outdated Show resolved Hide resolved

spencerrlongg added 6 commits November 28, 2023 21:19

standardize some of the validation rules, fix int/string issue

cba5f86

test checkout to user on store

15d11f7

checkout on store tests almost done

c886fb5

a couple notes

b17078f

asset assigned to asset works now

65ca24f

a couple quick things

9d8433b

unfurl-links bot mentioned this pull request Nov 29, 2023

Manage EOL date by checkbox of eol_explicit #13846

Closed

10 tasks

spencerrlongg added 12 commits November 29, 2023 14:57

fixed eol, add calculated eol/eol explicit tests

c3492f1

note

d794608

get rid of firsts and find by responses id

151ceb5

added some more check out assertions

135b7bd

return types

df954a3

cleanup rules formatting

7e780c1

organization

133c469

removed references to archived

88b6a54

get rid of physical

68d8538

gets rid of depreciate

6e5f296

cleaned up

ca8765e

more cleanup

2d57b93

spencerrlongg marked this pull request as ready for review November 30, 2023 23:09

spencerrlongg requested a review from snipe as a code owner November 30, 2023 23:09

spencerrlongg requested a review from marcusmoore November 30, 2023 23:09

spencerrlongg requested a review from uberbrady November 30, 2023 23:09

marcusmoore approved these changes Nov 30, 2023

View reviewed changes

cleanup test

cef1c5d

uberbrady reviewed Dec 13, 2023

View reviewed changes

snipe reviewed Dec 13, 2023

View reviewed changes

Add new asset tests for archive, depreciate, and physical fields

e028145

uberbrady requested changes Dec 14, 2023

View reviewed changes

snipe reviewed Dec 19, 2023

View reviewed changes

spencerrlongg added 4 commits January 2, 2024 17:28

add marcus' migration changes to branch

7c30261

Merge branch 'develop' into refactor/store_asset_form_request

8546407

fix auto-increment settings for test settings

9e84521

rm code, was probably fixed through rules at some point

901b1b8

spencerrlongg requested review from uberbrady and snipe January 3, 2024 00:31

snipe merged commit cce9540 into snipe:develop Jan 3, 2024
7 checks passed

Sxderp mentioned this pull request Apr 26, 2024

API Asset create performing invalid redirect when parameters do not validate #14653

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests For API Asset Store + Refactor For Auto Increment & Form Requests #13964

Tests For API Asset Store + Refactor For Auto Increment & Form Requests #13964

spencerrlongg commented Nov 28, 2023 •

edited

Loading

what-the-diff bot commented Nov 28, 2023

marcusmoore left a comment

marcusmoore left a comment

marcusmoore Nov 30, 2023

marcusmoore Nov 30, 2023

uberbrady left a comment

snipe Dec 13, 2023

spencerrlongg Dec 13, 2023 •

edited

Loading

snipe Dec 13, 2023

spencerrlongg Dec 13, 2023

spencerrlongg Dec 13, 2023

uberbrady commented Dec 14, 2023

uberbrady commented Dec 14, 2023

shortcut-integration bot commented Dec 14, 2023

uberbrady left a comment

spencerrlongg commented Dec 17, 2023

snipe Dec 19, 2023

spencerrlongg Dec 19, 2023

spencerrlongg Jan 3, 2024

spencerrlongg commented Jan 3, 2024 •

edited

Loading

Tests For API Asset Store + Refactor For Auto Increment & Form Requests #13964

Tests For API Asset Store + Refactor For Auto Increment & Form Requests #13964

Conversation

spencerrlongg commented Nov 28, 2023 • edited Loading

Description

Type of change

How Has This Been Tested?

what-the-diff bot commented Nov 28, 2023

PR Summary

marcusmoore left a comment

Choose a reason for hiding this comment

marcusmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberbrady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerrlongg Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uberbrady commented Dec 14, 2023

uberbrady commented Dec 14, 2023

shortcut-integration bot commented Dec 14, 2023

uberbrady left a comment

Choose a reason for hiding this comment

spencerrlongg commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerrlongg commented Jan 3, 2024 • edited Loading

spencerrlongg commented Nov 28, 2023 •

edited

Loading

spencerrlongg Dec 13, 2023 •

edited

Loading

spencerrlongg commented Jan 3, 2024 •

edited

Loading