[Bug] Crud List returns wrong count when there is a groupBy clause in the query

[gstrat88]

Bug report

Crud List returns wrong count when there is a groupBy clause in the query

What I did

I created my crud controller model etc, I added a filter which adds a group by clause.

What I expected to happen

I was expecting to have a correct count

What happened

It returns wrong count because of this laravel/framework#22883

What I've already tried to fix it

I overrided this class "crud/src/app/Library/CrudPanel/Traits/Query.php" using composer with a customized one I using this function for counting

/**
     * Count the number of results.
     *
     * @return int
     */
    public function count()
    {
        return $this->query->toBase()->getCountForPagination();
    }

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

PHP VERSION:

PHP 7.2.24-0ubuntu0.18.04.3 (cli) (built: Feb 11 2020 15:55:52) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
with Zend OPcache v7.2.24-0ubuntu0.18.04.3, Copyright (c) 1999-2018, by Zend Technologies

LARAVEL VERSION:

v6.18.16@73f18a6bc58fb91aa83925161db25aa3674b73e9

BACKPACK VERSION:

4.0.61@1977e0cc52fa7cf9547eaeadf03f5cd88402b574

[welcome]

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps a lot in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication channels:

• Bug Reports, Feature Requests - Github Issues (here);
• Quick help (How do I do X) - Gitter Chatroom;
• Long questions (I have done X and Y and it won't do Z wtf) - Stackoverflow, using the backpack-for-laravel tag;
• Showing off something you've made, asking for opinion on Backpack/Laravel matters - Reddit;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome awesome community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

[pxpm]

Hello @gstrat88

I was able to reproduce this in 4.1 so I think this is something that persists for long time.

I read the description of the issue in laravel repository you provided and could understand the problem.

To reproduce your issue I added to my setup() function in crud controler $this->crud->groupBy('category_id'), i am expecting that shows only the entries that has category_id value, one per category, and the pagination to be accordingly.

With your solution I got the correct results.

Before applying the correction:

Showing 1 to 10 of 73 entries (filtered from 216 total entries).
I only have 14 categories. After page 2 I got 6 more pages of empty results: No matching entries found

Can you test this @tabacitu too ? I tried in monster crud, $this->crud->groupBy('select') (select is category relation).

I expect a Crud with 14 entries (the number of my categories if they are all selected), and one representing the ones that don't have category, so 15 total entries in crud.

The solution provided give me the expected results from my understanding.

Could you see something breaking with this change ?

Best.

[tabacitu]

Yep, same here! Thank you @gstrat88 for raising the issue - and for the neat solution! I've talked to @pxpm and he's kind enough to put together a PR for ListOperation to do toBase()->getCountForPagination(); instead of count() - so basically:

    public function search()
    {
        $this->crud->hasAccessOrFail('list');

        $this->crud->applyUnappliedFilters();

        $totalRows = $this->crud->model->count();
-        $filteredRows = $this->crud->query->count();
+        $filteredRows = $this->crud->query->toBase()->getCountForPagination();
        $startIndex = request()->input('start') ?: 0;

Which will fix a big part of the problem. There's one more debatable issue here - what happens to the TOTAL number. Should it be the total number of items in the DB table (before they're grouped) or the total number of items (after they're grouped). Not sure about this one... What do you think @gstrat88 ?

But the fix above should solve the bulk of the problem. You can do it right now in a search() method on the CrudControllers until the PR is put together and merged.

Cheers!

[gstrat88]

Total rows executes the query without clauses(group by), and I believe this is the correct approach. If someone wants a "grouped model" he can use a custom scope to execute the group there. At least this is my opinion :);

[tabacitu]

Yeah I guess that makes sense from one point of view. But from another point of view - if you use GroupBy you might want to abstract away the database structure - the end-use (the admin) might not know or care that the items get grouped. But that's a different issue than this one, I guess.

I've just merged @pxpm 's #3014 which fixes this, using your solution, thanks a lot @gstrat88 . It'll be available later today using composer update.

One thing I've noticed though - we're pushing the fix for the 4.1 version, but from your initial post I see you're still using 4.0. I recommend you upgrade to 4.1 - this one is the only one that is getting this kind of bug fixes, 4.0 is only receiving security fixes at this point. For most projects it shouldn't take more than 30 minutes by following the upgrade guide. Don't get fooled by the numbers, 4.1 is a MAJOR version, with huge benefits. Like this fix 😆

Cheers! Thanks a lot for the issue & solution @gstrat88 . Keep 'em coming.