diff --git a/app/Helpers/Helper.php b/app/Helpers/Helper.php index b8ed973d60..ce919d7116 100644 --- a/app/Helpers/Helper.php +++ b/app/Helpers/Helper.php @@ -3,7 +3,6 @@ namespace App\Helpers; use App\Models\Accessory; -use App\Models\Asset; use App\Models\AssetModel; use App\Models\Component; use App\Models\Consumable; @@ -846,113 +845,140 @@ class Helper */ public static function checkLowInventory() { - $alert_threshold = Setting::getSettings()->alert_threshold; - $consumables = Consumable::withCount('consumableAssignments as consumables_users_count')->whereNotNull('min_amt')->get(); - $accessories = Accessory::withCount('checkouts as checkouts_count')->whereNotNull('min_amt')->get(); - $components = Component::withCount('assets as sum_unconstrained_assets')->whereNotNull('min_amt')->get(); - $asset_models = AssetModel::where('min_amt', '>', 0)->withCount(['availableAssets', 'assets'])->get(); - $licenses = License::withCount('availCount as licenses_available')->where('min_amt', '>', 0)->get(); + $alert_threshold = (int) Setting::getSettings()->alert_threshold; + + // Push the "below threshold" filter into SQL via havingRaw on the + // withCount alias, so only rows that will actually alert get + // hydrated. Previous shape loaded every row with min_amt set and + // filtered in PHP — on a 1000-item deployment with 5 low-inventory + // items that meant 200× more rows than needed. Also select only + // the columns the foreach uses (id / name / qty / min_amt), + // avoiding hydration of long text columns like License::serial. + // select() must come BEFORE withCount(): withCount uses addSelect + // under the hood, so a select() after would wipe the count alias. + // GROUP BY primary key satisfies SQLite's strict "HAVING requires + // an aggregated query" check — MariaDB allows bare HAVING but the + // test suite runs SQLite. Grouping by a unique key is a no-op for + // row cardinality (functional dependency), so nothing else shifts. + $consumables = Consumable::select('id', 'name', 'qty', 'min_amt') + ->withCount('consumableAssignments as consumables_users_count') + ->whereNotNull('min_amt') + ->groupBy('consumables.id') + ->havingRaw('(qty - consumables_users_count) < (min_amt + ?)', [$alert_threshold]) + ->get(); + + $accessories = Accessory::select('id', 'name', 'qty', 'min_amt') + ->withCount('checkouts as checkouts_count') + ->whereNotNull('min_amt') + ->groupBy('accessories.id') + ->havingRaw('(qty - checkouts_count) < (min_amt + ?)', [$alert_threshold]) + ->get(); + + $components = Component::select('id', 'name', 'qty', 'min_amt') + ->withCount('assets as sum_unconstrained_assets') + ->whereNotNull('min_amt') + ->groupBy('components.id') + ->havingRaw('(qty - sum_unconstrained_assets) < (min_amt + ?)', [$alert_threshold]) + ->get(); + + $asset_models = AssetModel::select('id', 'name', 'min_amt') + ->where('min_amt', '>', 0) + ->withCount(['availableAssets', 'assets']) + ->groupBy('models.id') + ->havingRaw('available_assets_count < (min_amt + ?)', [$alert_threshold]) + ->get(); + + // Use the licenses_available withCount alias directly in the + // foreach below rather than $license->remaincount(). remaincount() + // fires two extra queries per row (licenseSeatsCount via + // getLicenseSeatsCountAttribute() and assigned_seats_count via + // getAssignedSeatsCountAttribute()), plus a third + // unReassignableCount() query for non-reassignable licenses — + // classic N+1 on a licenses-with-min_amt list. + $licenses = License::select('id', 'name', 'min_amt') + ->withCount('availCount as licenses_available') + ->where('min_amt', '>', 0) + ->groupBy('licenses.id') + ->havingRaw('licenses_available < (min_amt + ?)', [$alert_threshold]) + ->get(); $items_array = []; $all_count = 0; foreach ($consumables as $consumable) { - $avail = $consumable->numRemaining(); - if ($avail < ($consumable->min_amt) + $alert_threshold) { - if ($consumable->qty > 0) { - $percent = number_format((($avail / $consumable->qty) * 100), 0); - } else { - $percent = 100; - } + $avail = $consumable->qty - $consumable->consumables_users_count; + $percent = $consumable->qty > 0 + ? number_format((($avail / $consumable->qty) * 100), 0) + : 100; - $items_array[$all_count]['id'] = $consumable->id; - $items_array[$all_count]['name'] = $consumable->name; - $items_array[$all_count]['type'] = 'consumables'; - $items_array[$all_count]['percent'] = $percent; - $items_array[$all_count]['remaining'] = $avail; - $items_array[$all_count]['min_amt'] = $consumable->min_amt; - $all_count++; - } + $items_array[$all_count]['id'] = $consumable->id; + $items_array[$all_count]['name'] = $consumable->name; + $items_array[$all_count]['type'] = 'consumables'; + $items_array[$all_count]['percent'] = $percent; + $items_array[$all_count]['remaining'] = $avail; + $items_array[$all_count]['min_amt'] = $consumable->min_amt; + $all_count++; } foreach ($accessories as $accessory) { $avail = $accessory->qty - $accessory->checkouts_count; - if ($avail < ($accessory->min_amt) + $alert_threshold) { - if ($accessory->qty > 0) { - $percent = number_format((($avail / $accessory->qty) * 100), 0); - } else { - $percent = 100; - } + $percent = $accessory->qty > 0 + ? number_format((($avail / $accessory->qty) * 100), 0) + : 100; - $items_array[$all_count]['id'] = $accessory->id; - $items_array[$all_count]['name'] = $accessory->name; - $items_array[$all_count]['type'] = 'accessories'; - $items_array[$all_count]['percent'] = $percent; - $items_array[$all_count]['remaining'] = $avail; - $items_array[$all_count]['min_amt'] = $accessory->min_amt; - $all_count++; - } + $items_array[$all_count]['id'] = $accessory->id; + $items_array[$all_count]['name'] = $accessory->name; + $items_array[$all_count]['type'] = 'accessories'; + $items_array[$all_count]['percent'] = $percent; + $items_array[$all_count]['remaining'] = $avail; + $items_array[$all_count]['min_amt'] = $accessory->min_amt; + $all_count++; } foreach ($components as $component) { - $avail = $component->numRemaining(); - if ($avail < ($component->min_amt) + $alert_threshold) { - if ($component->qty > 0) { - $percent = number_format((($avail / $component->qty) * 100), 0); - } else { - $percent = 100; - } + $avail = $component->qty - $component->sum_unconstrained_assets; + $percent = $component->qty > 0 + ? number_format((($avail / $component->qty) * 100), 0) + : 100; - $items_array[$all_count]['id'] = $component->id; - $items_array[$all_count]['name'] = $component->name; - $items_array[$all_count]['type'] = 'components'; - $items_array[$all_count]['percent'] = $percent; - $items_array[$all_count]['remaining'] = $avail; - $items_array[$all_count]['min_amt'] = $component->min_amt; - $all_count++; - } + $items_array[$all_count]['id'] = $component->id; + $items_array[$all_count]['name'] = $component->name; + $items_array[$all_count]['type'] = 'components'; + $items_array[$all_count]['percent'] = $percent; + $items_array[$all_count]['remaining'] = $avail; + $items_array[$all_count]['min_amt'] = $component->min_amt; + $all_count++; } foreach ($asset_models as $asset_model) { + $total_owned = $asset_model->assets_count; + $avail = $asset_model->available_assets_count; + $percent = $avail > 0 + ? number_format((($avail / $total_owned) * 100), 0) + : 100; - $asset = new Asset; - $total_owned = $asset_model->assets_count; // requires the withCount() clause in the initial query! - $avail = $asset_model->available_assets_count; // requires the withCount() clause in the initial query! - - if ($avail < ($asset_model->min_amt) + $alert_threshold) { - if ($avail > 0) { - $percent = number_format((($avail / $total_owned) * 100), 0); - } else { - $percent = 100; - } - $items_array[$all_count]['id'] = $asset_model->id; - $items_array[$all_count]['name'] = $asset_model->name; - $items_array[$all_count]['type'] = 'models'; - $items_array[$all_count]['percent'] = $percent; - $items_array[$all_count]['remaining'] = $avail; - $items_array[$all_count]['min_amt'] = $asset_model->min_amt; - $all_count++; - } + $items_array[$all_count]['id'] = $asset_model->id; + $items_array[$all_count]['name'] = $asset_model->name; + $items_array[$all_count]['type'] = 'models'; + $items_array[$all_count]['percent'] = $percent; + $items_array[$all_count]['remaining'] = $avail; + $items_array[$all_count]['min_amt'] = $asset_model->min_amt; + $all_count++; } foreach ($licenses as $license) { - $avail = $license->remaincount(); - if ($avail < ($license->min_amt) + $alert_threshold) { - if ($avail > 0) { - $percent = number_format((($avail / $license->min_amt) * 100), 0); - } else { - $percent = 100; - } - - $items_array[$all_count]['id'] = $license->id; - $items_array[$all_count]['name'] = $license->name; - $items_array[$all_count]['type'] = 'licenses'; - $items_array[$all_count]['percent'] = $percent; - $items_array[$all_count]['remaining'] = $avail; - $items_array[$all_count]['min_amt'] = $license->min_amt; - $all_count++; - } + $avail = $license->licenses_available; + $percent = $avail > 0 + ? number_format((($avail / $license->min_amt) * 100), 0) + : 100; + $items_array[$all_count]['id'] = $license->id; + $items_array[$all_count]['name'] = $license->name; + $items_array[$all_count]['type'] = 'licenses'; + $items_array[$all_count]['percent'] = $percent; + $items_array[$all_count]['remaining'] = $avail; + $items_array[$all_count]['min_amt'] = $license->min_amt; + $all_count++; } return $items_array; diff --git a/app/Livewire/AlertMenu.php b/app/Livewire/AlertMenu.php new file mode 100644 index 0000000000..64dfb290ab --- /dev/null +++ b/app/Livewire/AlertMenu.php @@ -0,0 +1,48 @@ +show_alerts_in_menu == '1') ` + * so nothing about this component runs when the operator turned the + * bell off. + */ +#[Lazy] +class AlertMenu extends Component +{ + public function placeholder(): string + { + // Reserve the same visual footprint as the loaded bell so the + // top-nav doesn't shift when the real component swaps in. + return <<<'HTML' +
  • + +
    • + HTML; + } + + public function render(): View + { + return view('livewire.alert-menu', [ + 'alert_items' => Helper::checkLowInventory(), + 'deprecations' => Helper::deprecationCheck(), + ]); + } +} diff --git a/resources/views/blade/alert-menu.blade.php b/resources/views/blade/alert-menu.blade.php deleted file mode 100644 index 4bcb65e87e..0000000000 --- a/resources/views/blade/alert-menu.blade.php +++ /dev/null @@ -1,90 +0,0 @@ -@props([ - 'snipeSettings' => \App\Models\Setting::getSettings(), -]) - -@if ($snipeSettings->show_alerts_in_menu=='1') - - - -
  • - - -
    • - -@if (!$slot->isEmpty()) - {{ $slot }} -@endif -@endif \ No newline at end of file diff --git a/resources/views/layouts/default.blade.php b/resources/views/layouts/default.blade.php index cd73c9691a..074c57d569 100644 --- a/resources/views/layouts/default.blade.php +++ b/resources/views/layouts/default.blade.php @@ -1493,7 +1493,9 @@ @endcan @can('admin') - + @if ($snipeSettings->show_alerts_in_menu == '1') + + @endif @endcan diff --git a/resources/views/livewire/alert-menu.blade.php b/resources/views/livewire/alert-menu.blade.php new file mode 100644 index 0000000000..e55af51d67 --- /dev/null +++ b/resources/views/livewire/alert-menu.blade.php @@ -0,0 +1,67 @@ +{{-- Top-nav alert bell. Hydrated lazily via in + layouts/default.blade.php. The setting-gate lives on the parent + tag — this view assumes the operator opted in. --}} +
  • + +
      + + @if ((count($alert_items) + count($deprecations)) > 0) + + @can('superadmin') + @if ($deprecations) + @foreach ($deprecations as $deprecation) + @if ($deprecation['check']) +
    • {!! $deprecation['message'] !!}
      • + @endif + @endforeach + @endif + @endcan + + @if ($alert_items) +
    • + {{ trans_choice('general.quantity_minimum', count($alert_items)) }} +
      • +
    • + +
      • + @endif + @else +
    • + {{ trans_choice('general.quantity_minimum', 0) }} +
      • + @endif +
    +
    • diff --git a/tests/Feature/Console/SendInventoryAlertsTest.php b/tests/Feature/Console/SendInventoryAlertsTest.php new file mode 100644 index 0000000000..40dc08e3e2 --- /dev/null +++ b/tests/Feature/Console/SendInventoryAlertsTest.php @@ -0,0 +1,82 @@ +getKey() on the notifiable during send-time tracking. + // Output-level assertions still catch what matters: the command + // completes without exception, took the correct branch, and reported + // the right thing to cron. + public function test_reports_low_inventory_count_when_items_are_low(): void + { + $this->settings->set([ + 'alerts_enabled' => 1, + 'alert_email' => 'ops@example.test', + 'alert_threshold' => 0, + ]); + + Consumable::factory()->create(['qty' => 0, 'min_amt' => 1]); + + $this->artisan('snipeit:inventory-alerts') + ->expectsOutputToContain('below minimum inventory') + ->assertExitCode(0); + } + + public function test_reports_nothing_to_send_when_no_low_inventory(): void + { + $this->settings->set([ + 'alerts_enabled' => 1, + 'alert_email' => 'ops@example.test', + 'alert_threshold' => 0, + ]); + + Consumable::factory()->create(['qty' => 10, 'min_amt' => 1]); + + $this->artisan('snipeit:inventory-alerts') + ->expectsOutputToContain('No low inventory items found') + ->assertExitCode(0); + } + + public function test_no_op_when_alerts_disabled_in_settings(): void + { + $this->settings->set([ + 'alerts_enabled' => 0, + 'alert_email' => 'ops@example.test', + ]); + + Consumable::factory()->create(['qty' => 0, 'min_amt' => 1]); + + $this->artisan('snipeit:inventory-alerts') + ->expectsOutputToContain('Alerts are disabled') + ->assertExitCode(0); + } + + public function test_no_op_when_alert_email_is_blank(): void + { + $this->settings->set([ + 'alerts_enabled' => 1, + 'alert_email' => '', + ]); + + Consumable::factory()->create(['qty' => 0, 'min_amt' => 1]); + + $this->artisan('snipeit:inventory-alerts') + ->expectsOutputToContain('No alert email configured') + ->assertExitCode(0); + } +} diff --git a/tests/Feature/Helpers/CheckLowInventoryTest.php b/tests/Feature/Helpers/CheckLowInventoryTest.php index 0362767796..87f26293d3 100644 --- a/tests/Feature/Helpers/CheckLowInventoryTest.php +++ b/tests/Feature/Helpers/CheckLowInventoryTest.php @@ -9,6 +9,8 @@ use App\Models\AssetModel; use App\Models\Component; use App\Models\Consumable; use App\Models\License; +use App\Models\User; +use Illuminate\Support\Facades\DB; use Tests\TestCase; class CheckLowInventoryTest extends TestCase @@ -209,6 +211,112 @@ class CheckLowInventoryTest extends TestCase ); } + /** + * Checkouts reduce effective availability: an accessory with qty=5 + * and min_amt=3 is above the floor at rest, but with 3 units checked + * out only 2 remain — below the floor — so the alert must fire. + * Guards against the havingRaw filter silently ignoring the + * checkouts_count subquery. + */ + public function test_accessory_with_checkouts_below_min_amt_is_flagged() + { + $this->settings->set(['alert_threshold' => 0]); + + $accessory = Accessory::factory()->create(['qty' => 5, 'min_amt' => 3]); + $user = User::factory()->create(); + + $accessory->checkouts()->createMany([ + ['assigned_to' => $user->id, 'assigned_type' => User::class, 'created_by' => 1], + ['assigned_to' => $user->id, 'assigned_type' => User::class, 'created_by' => 1], + ['assigned_to' => $user->id, 'assigned_type' => User::class, 'created_by' => 1], + ]); + + $this->assertContains( + $accessory->id, + $this->idsForType(Helper::checkLowInventory(), 'accessories'), + ); + } + + public function test_accessory_with_checkouts_still_above_min_amt_is_not_flagged() + { + $this->settings->set(['alert_threshold' => 0]); + + // 5 total, 1 checked out → 4 remaining, min is 3 → above the floor. + $accessory = Accessory::factory()->create(['qty' => 5, 'min_amt' => 3]); + $user = User::factory()->create(); + $accessory->checkouts()->create([ + 'assigned_to' => $user->id, + 'assigned_type' => User::class, + 'created_by' => 1, + ]); + + $this->assertNotContains( + $accessory->id, + $this->idsForType(Helper::checkLowInventory(), 'accessories'), + ); + } + + /** + * License seats are created by the License::created observer, and + * assigning a seat should drop the "available" count. Locks in the + * behaviour of the licenses_available withCount alias — the previous + * code path went through $license->remaincount() which fires + * additional N+1 queries and could drift from the alias. + */ + public function test_license_with_assigned_seats_below_min_amt_is_flagged() + { + $this->settings->set(['alert_threshold' => 0]); + + // 5 seats, min 3 → healthy at rest. Assign 3 → 2 available → below min. + $license = License::factory()->create(['seats' => 5, 'min_amt' => 3]); + $seats = $license->licenseseats()->orderBy('id')->take(3)->get(); + foreach ($seats as $seat) { + $seat->update(['assigned_to' => User::factory()->create()->id]); + } + + $this->assertContains( + $license->id, + $this->idsForType(Helper::checkLowInventory(), 'licenses'), + ); + } + + /** + * Regression pin for the query-count refactor: the whole checkLowInventory + * call should fire a bounded, small number of queries regardless of how + * many low-inventory rows exist across each category. If someone + * accidentally re-introduces a $model->numRemaining() / remaincount() + * call in the foreach loops this will spike well past the ceiling. + */ + public function test_check_low_inventory_query_count_stays_bounded() + { + $this->settings->set(['alert_threshold' => 0]); + + // Populate each category with several low-inventory rows so the + // foreach paths that used to trigger extra per-row queries actually + // execute. + Consumable::factory()->count(3)->create(['qty' => 0, 'min_amt' => 1]); + Accessory::factory()->count(3)->create(['qty' => 0, 'min_amt' => 1]); + Component::factory()->count(3)->create(['qty' => 1, 'min_amt' => 5]); + AssetModel::factory()->count(3)->create(['min_amt' => 5]); + License::factory()->count(3)->create(['seats' => 1, 'min_amt' => 5]); + + DB::flushQueryLog(); + DB::enableQueryLog(); + + Helper::checkLowInventory(); + + // Ceiling covers: 1 settings + 5 category queries + 1 status_labels + // lookup for the RTD() scope in the AssetModel branch = 7. Held to + // 10 to leave a little headroom for a future settings-related + // memoization change without loosening the N+1 guard. + $count = count(DB::getQueryLog()); + $this->assertLessThanOrEqual( + 10, + $count, + "checkLowInventory fired {$count} queries — expected <=10. Something is doing per-row DB access again.", + ); + } + private function idsForType(array $items, string $type): array { return collect($items) diff --git a/tests/Feature/Livewire/AlertMenuTest.php b/tests/Feature/Livewire/AlertMenuTest.php new file mode 100644 index 0000000000..72ac1522dd --- /dev/null +++ b/tests/Feature/Livewire/AlertMenuTest.php @@ -0,0 +1,26 @@ +superuser()->create()) + ->test(AlertMenu::class) + ->assertStatus(200); + } + + public function test_placeholder_reserves_bell_footprint(): void + { + $placeholder = (new AlertMenu)->placeholder(); + + $this->assertStringContainsString('dropdown', $placeholder); + $this->assertStringContainsString('fa-bell', $placeholder); + } +}