Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: Asset Count on Users Index Page Counting Soft Deleted Items #15981

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

akemidx
Copy link
Collaborator

@akemidx akemidx commented Dec 17, 2024

This more robustly tallies the assets count on the users index page. Soft deleted assets could still show in the created table under certain circumstances.

old:
Screenshot 2024-12-17 at 5 13 16 PM

new:
Screenshot 2024-12-17 at 5 12 53 PM

Screenshot 2024-12-17 at 5 19 15 PM

Fixes: 27551

Copy link

what-the-diff bot commented Dec 17, 2024

PR Summary

  • Inclusion of Builder Import
    The update brings in a specific tool known as Builder to enhance the code. This tool is part of a package called Illuminate\Database\Eloquent\Builder and is crucial for managing more advanced database operations with ease.

  • Adjustments to withCount Method
    The changes have made the withCount method more flexible and efficient. This function, which is used to count certain items in the application, now uses an array format. Additionally, it incorporates a custom query for excluding assets that are no longer in use or 'trashed', improving the accuracy of our counts.

@snipe
Copy link
Owner

snipe commented Dec 19, 2024

I think we'd probably want to modify the assets method to not use withTrashed() versus this approach, no?

@akemidx
Copy link
Collaborator Author

akemidx commented Dec 20, 2024

We could, but the hesitation with that was what else around the site would that alter?
There are probably a few other places that asset count is used, and we could doink up a count that should include withTrashed()

@akemidx
Copy link
Collaborator Author

akemidx commented Jan 7, 2025

coming back to this.

I personally think we shouldn't modify the method. Doing so would make us have to go to each place we call the method and edit those to use withTrashed() or withoutTrashed()

That is a tone of work for a smaller fix like this, and leaves us the possiblity of missing one or two and adding on more work in the future. I don't disagree with you tho, in that these things should probably be more method based, but cost/benefit analysis in my head says to leave as I have.

@snipe snipe merged commit 587449e into snipe:develop Jan 13, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants