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

Entries count <> Rows if you have deleted rows #226

Closed
4 tasks done
mauthi opened this issue Oct 11, 2020 · 26 comments
Closed
4 tasks done

Entries count <> Rows if you have deleted rows #226

mauthi opened this issue Oct 11, 2020 · 26 comments
Assignees

Comments

@mauthi
Copy link
Contributor

mauthi commented Oct 11, 2020

This is a bug.

Prerequisites

  • Are you running the latest version?
  • Are you reporting to the correct repository?
  • Did you check the documentation?
  • Did you perform a cursory search?

Description

The entries count in table footer is wrong if you have soft deleted rows in your database.
image

Steps to Reproduce

  1. Create a table for a model which uses soft deletes
  2. Delete one entry
  3. Check sum row in footer

Expected behavior

In my screenshot the footer row should be "From 1 to 12 of 12 entries"

Actual behavior

In my screenshot the footer row is "From 1 to 12 of 14 entries"

  • I have 2 soft deleted rows in table
@raftx24 raftx24 self-assigned this Oct 14, 2020
raftx24 added a commit that referenced this issue Oct 14, 2020
aocneanu added a commit that referenced this issue Oct 31, 2020
@aocneanu
Copy link
Member

@mauthi please test

@aocneanu aocneanu assigned mauthi and unassigned raftx24 Oct 31, 2020
@mauthi
Copy link
Contributor Author

mauthi commented Nov 2, 2020

Works for me. Thx for fixing.

@mauthi mauthi closed this as completed Nov 2, 2020
@aocneanu aocneanu reopened this Nov 12, 2020
@aocneanu
Copy link
Member

Unfortunately this has serious performance issues on big tables with complex queries / subqueries.

We will revert @raftx24

aocneanu added a commit that referenced this issue Nov 26, 2020
@aocneanu
Copy link
Member

@mauthi please try the fix/softDeletes branch and report back.

@vmcvlad , @gandesc you can review too.

@mauthi
Copy link
Contributor Author

mauthi commented Nov 26, 2020

Works perfect for my use case. Thx.

@mauthi
Copy link
Contributor Author

mauthi commented Nov 26, 2020

Found an error:
If I use a table with join I get the following error:

"message": "SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'deleted_at' in where clause is ambiguous (SQL: select count(*) as aggregate from suppliersleft jointax_typesonsuppliers.tax_type_id=tax_types.idwheredeleted_at is null)",

My used select:

return Supplier::select(
            \DB::raw('suppliers.id as id, suppliers.name as supplier_name, tax_types.name as tax_type_name')
        )
            ->leftJoin('tax_types', 'suppliers.tax_type_id', '=', 'tax_types.id');

@aocneanu
Copy link
Member

Fixed.

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020

Tested - error is fixed.

@aocneanu
Copy link
Member

But thinking more about it, I guess this approach is not enough. It will solve your case but won't work when you will want your query withTrashed, right?

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020

Right - if I use return CustomerGroup::withTrashed()->withCount('customers'); I get the same amount of rows (# of rows with deleted is null)

@aocneanu
Copy link
Member

Could you paste here your table query captured with telescope?

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020

I got the deleted rows in table but footer is wrong:
image

Query:

select
  `customer_groups`.*,
  (
    select
      count(*)
    from
      `customers`
    where
      `customer_groups`.`id` = `customers`.`customer_group_id`
      and `customers`.`deleted_at` is null
  ) as `customers_count`
from
  `customer_groups`
order by
  `customer_groups`.`id` asc
limit
  20 offset 0

So the total row count is not updated and if I see that correctly it's even not fetched again?

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020

And one additional thing: The delete button is also visible for deleted items and leads to a 404 (but thats not a normal use case to show deleted rows)

@aocneanu
Copy link
Member

ok, this needs more work but I guess we're on the right path

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020 via email

@mauthi
Copy link
Contributor Author

mauthi commented Nov 27, 2020

I found another issue:

    public function query(): Builder
    {
        return Customer::where('post_status','<>','trash')->selectRaw('*');
    }

In the table I have 6 rows, one with post_status = trash
Now I get From 1 to 5 of 6 entries as footer.

@raftx24
Copy link
Member

raftx24 commented Dec 8, 2020

Hi @mauthi
could you please check my commit too?
I checked it and it worked with withTrash,...
thanks

@mauthi
Copy link
Contributor Author

mauthi commented Dec 8, 2020

Hi @raftx24,

I just tried it.
For me it's not working for both - soft deletes and normal where statements (like described in #226 (comment))

With not working I mean the following:

  • Data in Table is correct
  • Pagination and info at the bottom is wrong (shows all entries in both cases)

@raftx24
Copy link
Member

raftx24 commented Dec 8, 2020

@mauthi.
I created 6 companies, and I set the first company name to trash(similar to your situation).
this is my CompanyTable

class CompanyTable implements Table
{
    protected const TemplatePath = __DIR__.'/../Templates/companies.json';

    public function query(): Builder
    {
        return Company::where('name', '<>', 'trash')->selectRaw('*');
    }

    public function templatePath(): string
    {
        return static::TemplatePath;
    }
}

and this is the result.
image

@mauthi
Copy link
Contributor Author

mauthi commented Dec 8, 2020

I tried if again (with your branch /softDeletesWithApplyingScopes):

query():
return CustomerGroup::where('name', 'like', '%an%');

Result:
image

@raftx24
Copy link
Member

raftx24 commented Dec 8, 2020

@mauthi I guess there is another problem with your table. because I used this table(10 companies with (8 + 2 deleted))
image

and I tried to simulate your situation with the following Table class

class CompanyTable implements Table
{
    protected const TemplatePath = __DIR__.'/../Templates/companies.json';

    public function query(): Builder
    {
        return Company::where('name', 'like', '%a%');
    }

    public function templatePath(): string
    {
        return static::TemplatePath;
    }
}

and I got this result.
image


Maybe your cache is enabled and you cannot see the current count.

@mauthi
Copy link
Contributor Author

mauthi commented Dec 8, 2020

Hi again,

Thx for the hint with the cache - I deleted it with php artisan enso:tables:clear and now it looks fine.
But the question is how this cache works and why this is enabled? Do you have a short explaination for me?

@raftx24
Copy link
Member

raftx24 commented Dec 8, 2020

You're welcome.
Yes, from docs

count, is a boolean, default is true. When counting the number of results for the table,
a count query has to be performed and for costly queries there can be quite a performance improvement if the count result is cached.
Note that if caching the count, you should use the TableCache trait on the main/base model, so that the count cache is invalidated if when a model is created or deleted.

@mauthi
Copy link
Contributor Author

mauthi commented Dec 8, 2020 via email

@mauthi
Copy link
Contributor Author

mauthi commented Dec 29, 2020

@aocneanu the fix of @raftx24 works for me (as written above) - is there a reason why his PR is still open or didn't you have time to check? If you need any feedback or help to test this feel free to ask.

@aocneanu
Copy link
Member

aocneanu commented Jan 4, 2021

merged

@aocneanu aocneanu closed this as completed Jan 4, 2021
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

No branches or pull requests

3 participants