Skip to content

Conversation

@OzanKurt
Copy link
Contributor

The only reason we're using setTotalRecords is to prevent the count query.

Even though we're seting the totalRecords we also need to set performedTotalRecordsCount so that;

IF -> there are filters applied to the query (any kind of search)
THEN -> don't perform total count AGAIN

There was an update about this topic recently: 9f7169a

Sadly it didn't cover all the cases.

Here is the related code in QueryDataTable#L275

// If no modification between the original query and the filtered one has been made
// the filteredRecords equals the totalRecords
if ($this->query == $initialQuery && $this->performedTotalRecordsCount) {
    $this->filteredRecords ??= $this->totalRecords;
} else {
    $this->filteredCount();
}

@OzanKurt
Copy link
Contributor Author

Can we please merge this asap? @yajra

@yajra
Copy link
Owner

yajra commented Aug 21, 2024

The changes look good but ci/cd checks are failing. Can you please fix it? Thanks!

@OzanKurt
Copy link
Contributor Author

As far as I see it's only 1 test failing am I right?

image

I will update the test so that it matches the expected behaviour.

@yajra
Copy link
Owner

yajra commented Aug 21, 2024

@JurianArie
Copy link
Contributor

This breaks my use case wherein we only want to make a query for the filtered count but never for the total count.

@OzanKurt
Copy link
Contributor Author

OzanKurt commented Aug 22, 2024

This breaks my use case wherein we only want to make a query for the filtered count but never for the total count.

Are you sure? This is exactly what I am intending as well.

if ($this->query == $initialQuery ...) part check if there is a modification to the "filtered" query. Which should result in false && true which will call $this->filteredCount();

@sonarqubecloud
Copy link

@OzanKurt
Copy link
Contributor Author

Finally fixed all the checks 😄

@JurianArie
Copy link
Contributor

I tested the changes locally, and I can confirm that it breaks my use case.

if ($this->query == $initialQuery ...) part check if there is a modification to the "filtered" query. Which should result in false && true which will call $this->filteredCount();

This assumes that filters are applied. If no filters are applied, it will result in true && true.

@OzanKurt
Copy link
Contributor Author

OzanKurt commented Aug 22, 2024

I am confused;

If there are no filters filteredCount must be equal to totalCount anyway.

Can you please send your DataTable so that I can test as well?

@JurianArie
Copy link
Contributor

The test cases before reflected the correct behavior. Here, the users table contains 20 records, and we manually set the total records to zero. The filtered records should still be 20.

$router->get('/zero-total-records', fn (DataTables $dataTable) => $dataTable->query(DB::table('users'))
->setTotalRecords(0)
->toJson());

public function it_can_set_zero_total_records()
{
$crawler = $this->call('GET', '/zero-total-records');
$crawler->assertJson([
'draw' => 0,
'recordsTotal' => 0,
'recordsFiltered' => 20,
]);
}

@OzanKurt
Copy link
Contributor Author

Exactly what I mean.

Having 0 total records and displaying 20 of them doesn't make any sense.

@JurianArie
Copy link
Contributor

You should think of setTotalRecords(0) as being the same as calling the now deprecated skipTotalRecords. I want to skip the total records count, not the filtered records count.

@OzanKurt
Copy link
Contributor Author

Unrelated update, we need to move performedTotalRecordsCount to count().

image

@OzanKurt
Copy link
Contributor Author

Case 1: (Your case)

// Total 2500: / Filtered: 100
$dataTable->setTotalRows(0)->make();

This should result in:

    No Filter -> "Displaying 100 of 0 rows"
    Filtered  -> "Displaying 100 of 0 rows"

Case 2: (My case)

// Total 2500: / Filtered: 100
$dataTable->setTotalRows(2500)->make();

This should result in:

    No Filter -> "Displaying 2500 of 2500 rows"
    Filtered  -> "Displaying 100 of 2500 rows"

Am I correct about this?

@JurianArie
Copy link
Contributor

1. Fresh setup of using the quick start guide

Guide: https://yajrabox.com/docs/laravel-datatables/master/quick-starter
image

2. Update UsersDataTable::dataTable with setTotalRecords(0)

image

3. Apply your change

image

As you can see when I apply your change the pagination is broken.

@OzanKurt
Copy link
Contributor Author

OzanKurt commented Aug 22, 2024

I still can't understand the logic.

My point is:

There is no reason to query the filtered count, IF the query is not being filtered.

Your version:

image
image

In both instances you will send a count query.

My version:

image
image


image


Example:

$dataTable->setTotalRecords(1000);

No filters:
image

HAS filters:
image

@yajra
Copy link
Owner

yajra commented Aug 26, 2024

Fixed via #3170

@yajra yajra closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants