Better auto increment asset-tags [FD-32892] #13301

uberbrady · 2023-07-12T15:56:15Z

Before, it was possible to 'mess up' the auto-incrementing asset tags by bumping the suggested asset tag up by one - then, forever after, each 'suggested' new asset tag would conflict with one already in the system, until you finally go into the asset tag settings and manually bump up the "Next auto-increment" setting by one.

Now, we try to be intelligent and set the next auto-increment value to the currently-just-created asset's asset_tag, plus one, IF AND ONLY IF the currently created asset has a higher value than the next.

Additionally, I've added tests to confirm that various failure scenarios in asset-tag generation no longer happen.

I unfortunately fixed the problem before I added the tests, but I just back-ported the tests only to the existing code-base, and I ran them, and got this lovely output (proving that the tests do break on our current code, but are fixed on my new code. Which is want you want :) )

FAIL Tests\Unit\AssetTest
✓ auto increment
✓ auto increment collision
⨯ auto increment double
⨯ auto increment gap and backfill
⨯ prefixless autoincrement backfill
⨯ unzerofilled prefixless autoincrement backfill
✓ warranty expires attribute

what-the-diff · 2023-07-12T15:56:42Z

PR Summary

Improved Code Clarity in AssetsController
In the AssetsController.php file, a conditional comment was added. This provides better understanding of why certain code is necessary and beneficial, thereby making it easier to interpret and maintain.
Enhanced Autoincrement Function in Asset Method
The autoincrement_asset method in the Asset.php file now accepts additional increments, allowing for greater flexibility in asset growth management.
Updated Asset Creation Process
Within the AssetObserver.php file, the created method has been adjusted to automatically increment asset tags dependent on specific conditions. This helps streamline the asset creation process and maintain a strict and organised asset hierarchy.
Added Option for Autoincrement during Testing
A new method, enableAutoIncrement, has been introduced to the Settings.php file. It serves to enable testing of the auto-increment mechanism of assets, ensuring its correct functionality under test scenarios.
Expanded Test Coverage with Asset Scenarios
Multiple new test scenarios have been introduced to the AssetTest.php file. These tests ensure the accurate functionality of the auto-increment feature under diverse conditions, confirming robustness and reliability of the feature.

snipe · 2023-07-12T16:17:16Z

This looks fantastic, @uberbrady - I need to test a bit more, but so far it looks excellent.

marcusmoore

Just a couple small comments but 🎉 tests! 😄

marcusmoore · 2023-07-12T20:32:30Z

tests/Unit/AssetTest.php

+
+    public function testPrefixlessAutoincrementBackfill()
+    {
+        // TODO: COPYPASTA FROM above, is there a way to still run this test but not have it be so duplicative?


One option for reducing duplication is PHPUnit's Data Providers since only the first line of these tests are different.

Here is an example in our codebase and a really good resource on getting started with Data Providers and a gotcha to look out for (see the Caveat section).

In this case though...I would just keep the duplication.

Yes, agreed. I don't like that data provider thing at all, it gives me all of the Oogs. Every single one of them. I think just accepting the repetition is the move.

marcusmoore · 2023-07-12T20:42:04Z

app/Observers/AssetObserver.php

        }

        $logAction = new Actionlog();
-        $logAction->item_type = Asset::class;
+        $logAction->item_type = Asset::class; // can we instead say $logAction->item = $asset ?


I just tried it and it works 👍🏾 up to you if you want to include it in this chunk of work or not.

I think it ought to go in a different PR entirely, tbh. It's just a comment that I wanted recorded for posterity, so that we hopefully fix it at some point later.

I agree 😄

marcusmoore · 2023-07-12T20:46:03Z

tests/Unit/AssetTest.php

+
+    public function testUnzerofilledPrefixlessAutoincrementBackfill()
+    {
+        // TODO: COPYPASTA FROM above (AGAIN), is there a way to still run this test but not have it be so duplicative?


I think we can remove the TODOs in this file so they don't end up confusing anyone in the future 👍🏾

If we decide that this is the right way (and it's seeming like we are deciding that), then I will leave the comment in just to keep people aware that changes to one spot might need to be copied to the others - but I would pull the TODO tag so we don't think that we need to actually do anything about it.

marcusmoore · 2023-07-12T20:49:25Z

app/Http/Controllers/Api/AssetsController.php

+        $asset->asset_tag               = $request->get('asset_tag', Asset::autoincrement_asset()); //yup, problem :/
+        // NO IT IS NOT!!! This is never firing; we SHOW the asset_tag you're going to get, so it *will* be filled in!


I got confused by these comments. If I'm reading this right we can remove both comments right?

I was hoping to record, for posterity, how, exactly this system is actually working, rather than "how we think it might work" so that the next time someone has to look at this problem, they don't go through the same mental gymnastics realizing how it really works versus how we think it might want to work.

Very happy to workshop a better way of conveying this to future-us (who are going to look at this again in, like, a few months or years or something...)

Ah gotcha. I was thinking these were debug comments for yourself as you worked through this PR. Keeping them there works for me 👍🏾

uberbrady added 2 commits July 12, 2023 16:39

Improvements to asset_tag auto-incrementing, with auto-fixups for gaps

76191a0

Also do a sanity-check that the normal asset autoincrementing works

e648da9

uberbrady requested a review from snipe as a code owner July 12, 2023 15:56

probot-autolabeler bot added api backend tests labels Jul 12, 2023

marcusmoore approved these changes Jul 12, 2023

View reviewed changes

snipe merged commit 981c3a4 into snipe:develop Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better auto increment asset-tags [FD-32892] #13301

Better auto increment asset-tags [FD-32892] #13301

uberbrady commented Jul 12, 2023

what-the-diff bot commented Jul 12, 2023

snipe commented Jul 12, 2023

marcusmoore left a comment

marcusmoore Jul 12, 2023

uberbrady Jul 12, 2023

marcusmoore Jul 12, 2023

uberbrady Jul 12, 2023

marcusmoore Jul 12, 2023

marcusmoore Jul 12, 2023

uberbrady Jul 12, 2023

marcusmoore Jul 12, 2023

uberbrady Jul 12, 2023

marcusmoore Jul 12, 2023

		$asset->asset_tag = $request->get('asset_tag', Asset::autoincrement_asset()); //yup, problem :/
		// NO IT IS NOT!!! This is never firing; we SHOW the asset_tag you're going to get, so it will be filled in!

Better auto increment asset-tags [FD-32892] #13301

Better auto increment asset-tags [FD-32892] #13301

Conversation

uberbrady commented Jul 12, 2023

what-the-diff bot commented Jul 12, 2023

PR Summary

snipe commented Jul 12, 2023

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

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

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