From 76191a09ed3424241e79fc68a1bc41350fb763b1 Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Wed, 12 Jul 2023 16:39:45 +0100 Subject: [PATCH 1/2] Improvements to asset_tag auto-incrementing, with auto-fixups for gaps --- app/Http/Controllers/Api/AssetsController.php | 3 +- app/Models/Asset.php | 13 +- app/Observers/AssetObserver.php | 27 +++- tests/Support/Settings.php | 11 ++ tests/Unit/AssetTest.php | 128 +++++++++++++++--- 5 files changed, 152 insertions(+), 30 deletions(-) diff --git a/app/Http/Controllers/Api/AssetsController.php b/app/Http/Controllers/Api/AssetsController.php index 222f528e4a..e8f37d8574 100644 --- a/app/Http/Controllers/Api/AssetsController.php +++ b/app/Http/Controllers/Api/AssetsController.php @@ -545,7 +545,8 @@ class AssetsController extends Controller $asset->model_id = $request->get('model_id'); $asset->order_number = $request->get('order_number'); $asset->notes = $request->get('notes'); - $asset->asset_tag = $request->get('asset_tag', Asset::autoincrement_asset()); + $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! $asset->user_id = Auth::id(); $asset->archived = '0'; $asset->physical = '1'; diff --git a/app/Models/Asset.php b/app/Models/Asset.php index e3d17d5e35..861269d5f2 100644 --- a/app/Models/Asset.php +++ b/app/Models/Asset.php @@ -785,24 +785,17 @@ class Asset extends Depreciable * @since [v4.0] * @return string | false */ - public static function autoincrement_asset() + public static function autoincrement_asset(int $additional_increment = 0) { $settings = \App\Models\Setting::getSettings(); if ($settings->auto_increment_assets == '1') { - $temp_asset_tag = \DB::table('assets') - ->where('physical', '=', '1') - ->max('asset_tag'); - - $asset_tag_digits = preg_replace('/\D/', '', $temp_asset_tag); - $asset_tag = preg_replace('/^0*/', '', $asset_tag_digits); - if ($settings->zerofill_count > 0) { - return $settings->auto_increment_prefix.self::zerofill($settings->next_auto_tag_base, $settings->zerofill_count); + return $settings->auto_increment_prefix.self::zerofill($settings->next_auto_tag_base + $additional_increment, $settings->zerofill_count); } - return $settings->auto_increment_prefix.$settings->next_auto_tag_base; + return $settings->auto_increment_prefix.($settings->next_auto_tag_base + $additional_increment); } else { return false; } diff --git a/app/Observers/AssetObserver.php b/app/Observers/AssetObserver.php index a399e1d2a6..84595f04b3 100644 --- a/app/Observers/AssetObserver.php +++ b/app/Observers/AssetObserver.php @@ -71,12 +71,33 @@ class AssetObserver public function created(Asset $asset) { if ($settings = Setting::getSettings()) { - $settings->increment('next_auto_tag_base'); - $settings->save(); + $tag = $asset->asset_tag; + $prefix = $settings->auto_increment_prefix; + $number = substr($tag, strlen($prefix)); + // IF - auto_increment_assets is on, AND (the prefix matches the start of the tag OR there is no prefix) + // AND the rest of the string after the prefix is all digits, THEN... + if ($settings->auto_increment_assets && (strpos($tag, $prefix) === 0 || $prefix=='') && preg_match('/\d+/',$number) === 1) { + // new way of auto-trueing-up auto_increment ID's + $next_asset_tag = intval($number, 10) + 1; + // we had to use 'intval' because the $number could be '01234' and + // might get interpreted in Octal instead of decimal + + // only modify the 'next' one if it's *bigger* than the stored base + // + if($next_asset_tag > $settings->next_auto_tag_base) { + $settings->next_auto_tag_base = $next_asset_tag; + $settings->save(); + } + + } else { + // legacy method + $settings->increment('next_auto_tag_base'); + $settings->save(); + } } $logAction = new Actionlog(); - $logAction->item_type = Asset::class; + $logAction->item_type = Asset::class; // can we instead say $logAction->item = $asset ? $logAction->item_id = $asset->id; $logAction->created_at = date('Y-m-d H:i:s'); $logAction->user_id = Auth::id(); diff --git a/tests/Support/Settings.php b/tests/Support/Settings.php index fa8886b751..17f8af23d2 100644 --- a/tests/Support/Settings.php +++ b/tests/Support/Settings.php @@ -56,6 +56,17 @@ class Settings ]); } + public function enableAutoIncrement(): Settings + { + return $this->update([ + 'auto_increment_assets' => 1, + 'auto_increment_prefix' => 'ABCD', + 'next_auto_tag_base' => '123', + 'zerofill_count' => 5 + ]); + + } + /** * @param array $attributes Attributes to modify in the application's settings. */ diff --git a/tests/Unit/AssetTest.php b/tests/Unit/AssetTest.php index 432165dc07..63d335a7b4 100644 --- a/tests/Unit/AssetTest.php +++ b/tests/Unit/AssetTest.php @@ -12,24 +12,120 @@ class AssetTest extends TestCase { use InteractsWithSettings; - // public function testAutoIncrementMixed() - // { - // $expected = '123411'; - // $next = Asset::nextAutoIncrement( - // collect([ - // ['asset_tag' => '0012345'], - // ['asset_tag' => 'WTF00134'], - // ['asset_tag' => 'WTF-745'], - // ['asset_tag' => '0012346'], - // ['asset_tag' => '00123410'], - // ['asset_tag' => 'U8T7597h77'], - // ]) - // ); + public function testAutoIncrementCollision() + { + $this->settings->enableAutoIncrement(); - // \Log::debug('Next: '.$next); - // $this->assertEquals($expected, $next); - // } + // we have to do this by hand to 'simulate' two web pages being open at the same time + $a = Asset::factory()->make(['asset_tag' => Asset::autoincrement_asset() ]); + $b = Asset::factory()->make(['asset_tag' => Asset::autoincrement_asset() ]); + $this->assertTrue($a->save()); + $this->assertFalse($b->save()); + } + + public function testAutoIncrementDouble() + { + // make one asset with the autoincrement *ONE* higher than the next auto-increment + // make sure you can then still make another + $this->settings->enableAutoIncrement(); + + $gap_number = Asset::autoincrement_asset(1); + $final_number = Asset::autoincrement_asset(2); + $a = Asset::factory()->make(['asset_tag' => $gap_number]); //make an asset with an ID that is one *over* the next increment + $b = Asset::factory()->make(['asset_tag' => Asset::autoincrement_asset()]); //but also make one with one that is *at* the next increment + $this->assertTrue($a->save()); + $this->assertTrue($b->save()); + + //and ensure a final asset ends up at *two* over what would've been the next increment at the start + $c = Asset::factory()->make(['asset_tag' => Asset::autoincrement_asset()]); + $this->assertTrue($c->save()); + $this->assertEquals($c->asset_tag, $final_number); + } + + public function testAutoIncrementGapAndBackfill() + { + // make one asset 3 higher than the next auto-increment + // manually make one that's 1 lower than that + // make sure the next one is one higher than the 3 higher one. + $this->settings->enableAutoIncrement(); + + $big_gap = Asset::autoincrement_asset(3); + $final_result = Asset::autoincrement_asset(4); + $backfill_one = Asset::autoincrement_asset(0); + $backfill_two = Asset::autoincrement_asset(1); + $backfill_three = Asset::autoincrement_asset(2); + $a = Asset::factory()->create(['asset_tag' => $big_gap]); + $this->assertModelExists($a); + + $b = Asset::factory()->create(['asset_tag' => $backfill_one]); + $this->assertModelExists($b); + + $c = Asset::factory()->create(['asset_tag' => $backfill_two]); + $this->assertModelExists($c); + + $d = Asset::factory()->create(['asset_tag' => $backfill_three]); + $this->assertModelExists($d); + + $final = Asset::factory()->create(['asset_tag' => Asset::autoincrement_asset()]); + $this->assertModelExists($final); + $this->assertEquals($final->asset_tag, $final_result); + } + + public function testPrefixlessAutoincrementBackfill() + { + // TODO: COPYPASTA FROM above, is there a way to still run this test but not have it be so duplicative? + $this->settings->enableAutoIncrement()->set(['auto_increment_prefix' => '']); + + $big_gap = Asset::autoincrement_asset(3); + $final_result = Asset::autoincrement_asset(4); + $backfill_one = Asset::autoincrement_asset(0); + $backfill_two = Asset::autoincrement_asset(1); + $backfill_three = Asset::autoincrement_asset(2); + $a = Asset::factory()->create(['asset_tag' => $big_gap]); + $this->assertModelExists($a); + + $b = Asset::factory()->create(['asset_tag' => $backfill_one]); + $this->assertModelExists($b); + + $c = Asset::factory()->create(['asset_tag' => $backfill_two]); + $this->assertModelExists($c); + + $d = Asset::factory()->create(['asset_tag' => $backfill_three]); + $this->assertModelExists($d); + + $final = Asset::factory()->create(['asset_tag' => Asset::autoincrement_asset()]); + $this->assertModelExists($final); + $this->assertEquals($final->asset_tag, $final_result); + } + + public function testUnzerofilledPrefixlessAutoincrementBackfill() + { + // TODO: COPYPASTA FROM above (AGAIN), is there a way to still run this test but not have it be so duplicative? + $this->settings->enableAutoIncrement()->set(['auto_increment_prefix' => '','zerofill_count' => 0]); + + $big_gap = Asset::autoincrement_asset(3); + $final_result = Asset::autoincrement_asset(4); + $backfill_one = Asset::autoincrement_asset(0); + $backfill_two = Asset::autoincrement_asset(1); + $backfill_three = Asset::autoincrement_asset(2); + $a = Asset::factory()->create(['asset_tag' => $big_gap]); + $this->assertModelExists($a); + + $b = Asset::factory()->create(['asset_tag' => $backfill_one]); + $this->assertModelExists($b); + + $c = Asset::factory()->create(['asset_tag' => $backfill_two]); + $this->assertModelExists($c); + + $d = Asset::factory()->create(['asset_tag' => $backfill_three]); + $this->assertModelExists($d); + + $final = Asset::factory()->create(['asset_tag' => Asset::autoincrement_asset()]); + $this->assertModelExists($final); + $this->assertEquals($final->asset_tag, $final_result); + } + public function testWarrantyExpiresAttribute() { From e648da9dc5825748d233790d84552b74af94f29a Mon Sep 17 00:00:00 2001 From: Brady Wetherington Date: Wed, 12 Jul 2023 16:51:23 +0100 Subject: [PATCH 2/2] Also do a sanity-check that the normal asset autoincrementing works --- tests/Unit/AssetTest.php | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/tests/Unit/AssetTest.php b/tests/Unit/AssetTest.php index 63d335a7b4..7670d10753 100644 --- a/tests/Unit/AssetTest.php +++ b/tests/Unit/AssetTest.php @@ -12,6 +12,17 @@ class AssetTest extends TestCase { use InteractsWithSettings; + public function testAutoIncrement() + { + $this->settings->enableAutoIncrement(); + + $a = Asset::factory()->create(['asset_tag' => Asset::autoincrement_asset() ]); + $b = Asset::factory()->create(['asset_tag' => Asset::autoincrement_asset() ]); + + $this->assertModelExists($a); + $this->assertModelExists($b); + + } public function testAutoIncrementCollision() { $this->settings->enableAutoIncrement(); @@ -125,7 +136,7 @@ class AssetTest extends TestCase $this->assertModelExists($final); $this->assertEquals($final->asset_tag, $final_result); } - + public function testWarrantyExpiresAttribute() {