yajra / laravel-datatables

jQuery DataTables API for Laravel
https://yajrabox.com/docs/laravel-datatables
MIT License
4.76k stars 859 forks source link

Logical bug with totalRecords and filteredRecords #333

Closed intech closed 8 years ago

intech commented 8 years ago
$this->totalRecords = $this->count();

https://github.com/yajra/laravel-datatables/blob/master/src/Engines/BaseEngine.php#L587

$this->filteredRecords = $this->isFilterApplied ? $this->count() : $this->totalRecords;

https://github.com/yajra/laravel-datatables/blob/master/src/Engines/BaseEngine.php#L642

totalRecords and filteredRecords are always equal in all conditions! They use one function $this->count() which returns the filtered results...

yajra commented 8 years ago

Not sure if there is a bug here since filtered and total values works fine for me. Can you please submit a PR to fix this?

intech commented 8 years ago

Greetings! https://datatables.net/faqs/index#Server-side-processing

https://datatables.net/manual/server-side#Returned-data

Open this is example: https://datatables.net/examples/server_side/row_details.html

Query:

...
order[0][column]:1
order[0][dir]:asc
start:0
length:10
search[value]:te
search[regex]:false
...

Results:

{
data: [{DT_RowId: "row_3", first_name: "Ashton", last_name: "Cox", position: "Junior Technical Author",…},…],
draw: 7,
recordsFiltered: 14,
recordsTotal: 57
}

Showing 1 to 10 of 14 entries (filtered from 57 total entries)

This is 14 - count filtered results with searching (te) and 57 is total without filtering.

I propose to introduce a separate function totalCount() to be in any of the engines comply with the request without filtering conditions.

yajra commented 8 years ago

Based on this demo, it shows the same expected result when searching "ne".

Showing 1 to 10 of 125 entries (filtered from 500 total entries)

screen shot 2016-01-09 at 3 35 41 pm
intech commented 8 years ago

What engine do you use? Link to the local name :)

yajra commented 8 years ago

Oops sorry! ^_^.. Here's the live demo using eloquent.

intech commented 8 years ago

https://github.com/yajra/laravel-datatables/blob/6.0/src/Engines/QueryBuilderEngine.php#L75 This engine is written with the terms of its own logic calculation. I'm in your engine under ElasticSearch made a similar but this is not correct. It is better to divide it into two different functions.

The new engine is necessary inherit from BaseEngine where counting is wrong.

yajra commented 8 years ago

Can you please submit a PR to implement your proposal? Quite busy at work projects. Thanks a lot!

intech commented 8 years ago

Oh sure! I had already done in its working draft.

intech commented 8 years ago

All the same, I agree that this is not a bug. This architectural solution for a particular problem. We need to make it more versatile for the other engines. Thank you for your activity, together, we understand the issue :)

intech commented 8 years ago

Why this happened:

In the example of the query builder, we call the count() several times. Before and after the application of filters, thereby counting from the beginning of the totalRecords and filteredRecords after filtered.

In my case, no query builder. To take the totalRecords I need to make a very different request, nothing like getting the data. I have come filteredRecords from the database in response to the data itself.

yajra commented 8 years ago

Makes more sense now. Thanks for the PR. Merge and released on v6.1.3. :100:

BTW, any chance you may want your engine to be included in the package? It would be a great help to the community to have an ElasticSearchEngine for DataTables. ^_^

intech commented 8 years ago

Thanks! Yes, I was planning to do it later when I have code refactoring. :)