snipe / snipe-it

A free open source IT asset/license management system
https://snipeitapp.com
GNU Affero General Public License v3.0
9.91k stars 3.01k forks source link

Fixes #14701 - wrong total asset count #14702

Closed Toreg87 closed 1 month ago

Toreg87 commented 1 month ago

Description

The total asset count in the sidenav shows the ready to deploy count instead of the total count. Fix this by adjusting the query to all assets. Also respect the setting for archived assets. Add a default value for total assets, since we are now using the settings-variable, which is not available during the setup process.

While at it, move the block for total assets before the ready to deploy assets to match the ordering of the sidenav.

Fixes #14701

Type of change

Please delete options that are not relevant.

Checklist:

what-the-diff[bot] commented 1 month ago

PR Summary

snipe commented 1 month ago

Thanks!

jerm commented 1 month ago

@Toreg87 Thanks again for submitting this!

In the interest of being helpful, I wanted to call your attention to a follow-up PR to this one: https://github.com/snipe/snipe-it/pull/14711

tl;dr: while your fix worked, your use of all() in $total_assets = Asset::all()->count(); had the unfortunate effect of slowing down execution and using a lot of memory. The reason is because all() tells the ORM to fetch everything from the database, and then operate on it. So instead of asking the database to do the count(efficient), php was being asked to do the count, after getting ALL of the data from the database (inefficient).

Leaving out all() makes the query select count(*) from assets, vs select * from assets.

In general, use of all() is an anti-pattern and should only be used if absolutely necessary.

Hope this is helpful đŸ™‚

Thanks again for your contribution!