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

Translations are not loaded with Datatables in server-side mode #171

Closed
weidmaster opened this issue Aug 28, 2020 · 10 comments
Closed

Translations are not loaded with Datatables in server-side mode #171

weidmaster opened this issue Aug 28, 2020 · 10 comments
Assignees

Comments

@weidmaster
Copy link

Picking up from #113 the problem is not with the select()at all, it is with the datatable when using server-side mode.

When using server-side, we don't get the results from Eloquent, we let the datatable package make the queries and assemble the final result to send to the client-side, and that is why the translations are not loaded with the request.

The following code will be the example:

 $datas = Generalsetting::orderBy('id');
        //--- Integrating This Collection Into Datatables
        return Datatables::of($datas)
            ->filterColumn('title', function ($query, $keyword) {
                $query->whereTranslationLike('title', "%{$keyword}%", 'pt-br');
            })
            ->toJson(); //--- Returning Json Data To Client Side

The filterColumn is needed because without it when performing a search in the datatable, it would try to search the vanilla field of title. So the problem is just that we need to copy such filter logic in every table ^^'

One approach I can see is for Translatable package to notice that the model (Generalsetting in the example) does use the trait, or have the $translatedAttributes array, and modify the query to join the translated table so the field would be available in the query.

Other approach would be for withTranslation() to make the query without using get() as well, thus making the query available.

Another way that I can see this working is for the method listsTranslations() to accept an array of fields and return the query as well, so instead of doing like: Generalsetting::listsTranslations('title')->get() it would be just `Generalsetting::listsTranslations('title')' and we could chain the order and other eloquent calls, without getting the results right away, so the effect would be to only return the fields we care about in the particular table.

I guess the bottom line is in the fact that every method needs to call get() and can't be used in chain with other Eloquent methods.

@Gummibeer
Copy link
Member

Hey,

thanks for clarifying the real issue.
Because there's a working and clean solution with this package I would recommend to add a wrapper in your application.
For example an explicit GeneralsettingDatatable class with a make() method that accepts what gets passed to of(), applies all possible filterColumn() methods and returns the instance.
You could also use a single Datatables class with methods per model generalSettings() or similar.

I don't see a good solution that could be added to this package. If there's more you could imagine in a translatable-datatable bridge I would recommend a dedicated package. For sure I will list it in the docs - I'm thinking/planning something similar for Nova.
My major reason for this is the separation of scopes - I will do my best to make this package somehow compatible with other packages. But I won't add dedicated bridges/adjustments only for a foreign package. This would be an endless story.

The dedicated class would bee something similar like the repository pattern. In theory you could even add a datatable() method to a custom query builder class that will do the required things. and you could do Generalsetting::orderBy('id')->datatable()->toJson().

@Gummibeer Gummibeer self-assigned this Aug 28, 2020
@weidmaster
Copy link
Author

I understand that the package needs to be agnostic as possible. Regarding your statement:

I will do my best to make this package somehow compatible with other packages

I think then it only needs to provide a new method that can be chainable, instead of relying to using get().

As for the wrapper idea, I don't know how to do it, but I understand the concept of having a base class that will handle the interaction. I just don't understand how I will be able to wrap the package in my code.

Thanks as always.

@Gummibeer
Copy link
Member

Hey,
I'm sorry for the delay - I'm in holiday since last week.

The wrapper, as query builder, could be something like this - it's not tested and more pseudo as I'm writing it on my phone. 😅

class PostQueryBuilder extends Eloquent\QueryBuilder
{
  public function datatable()
  {
    return Datatable::of($this)->filter column('title', ...);
  }
}

After registering it on your model via getQueryBuilder() (I hope that was the name) you can do something like this:

Post::wherePublished(true)->datatable()->toJson();
Post::datatable();

This way you can build your query as normal, instead of get() or nothing use datatable() which also adds the translatable column filters or even some other custom filters or whatever you want to customize on the datatable instance in every case.
At least in my head that's the cleanest solution I could imagine.
Custom query builders are super powerful and also super clean - in comparison to simple model scopes.

@weidmaster
Copy link
Author

Oh I hope you are enjoying your holidays! And thanks for the info, it looks very interesting indeed. I can see a lot of potential. I just never heard about custom query builder. I will need to do some research, but if it works like you explained, that would help a lot.

@Gummibeer
Copy link
Member

<?php

namespace App\Builder\Eloquent;

use App\Enums\FileStatusEnum;
use App\Enums\FileTypeEnum;
use Illuminate\Database\Eloquent\Builder;
use Illuminate\Support\Carbon;

class FileQueryBuilder extends Builder
{
    public function wherePublished(): self
    {
        return $this->where('status', FileStatusEnum::PUBLISHED());
    }

    public function whereImages(): self
    {
        return $this->where('type', FileTypeEnum::IMAGE());
    }

    public function wherePublishedImages(): self
    {
        return $this->wherePublished()->whereImages();
    }

    public function whereStale(): self
    {
        return $this->whereIn('status', [
            FileStatusEnum::DENIED(),
            FileStatusEnum::CLOUD_UPLOAD(),
            FileStatusEnum::CREATED(),
        ])->where('created_at', '<', Carbon::now()->subDay());
    }
}

That's one of our builders. We use them as soon as some scopes come together on one model to keep the models clean.

@weidmaster
Copy link
Author

This looks awesome! And I see another cool stuff, the enum type there. That is something I have to catch up on as well.

@Gummibeer
Copy link
Member

If you do use directly the v2 dev - it will be released soon but we need some real world feedback. 😉
spatie/laravel-enum#44

@weidmaster
Copy link
Author

Oh so you contribute to the spatie codebase. That is pretty awesome. I will take a look. I saw it is for L8 only though, so it may take a while.

@Gummibeer
Copy link
Member

Yeah, several packages - most common the activitylog which I maintain.

Yeah, the Castable::castUsing() signature changed in L8 so i had to drop L7.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2020

This issue is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale label Oct 2, 2020
@github-actions github-actions bot closed this as completed Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants