Laravel-Backpack / community-forum

A workspace to discuss improvement and feature ideas, before they're actually implemented.
28 stars 0 forks source link

`select2_from_ajax` Pagination meta data does not match `ResourceCollection` #61

Open juventus18 opened 4 years ago

juventus18 commented 4 years ago

Bug report

I haven't upgraded to BP 4.x yet so I'm not sure if it is still an issue in the latest version. In 3.6, the select_2_from_ajax field requires certain pagination properties to be present in order for the pagination features to function properly. The pagination object returned from Laravel's ResourceCollection (this is the preferred way to return REST API data per Laravel's docs) is incompatible with select2's expected pagination object. I propose we modify the select2_from_ajax settings to match Laravel's default resource pagination schema.

What I did

Create REST API using Resources and ResourceCollections

class CourseController extends Controller
{
    public function index(Request $request)
    {
        $search_term = $request->input('q');

        $options = Course::query();

        if ($search_term) {
            $results = $options->join('prefixes', 'prefixes.id', '=', 'courses.prefix_id')
                ->where(DB::raw("CONCAT(prefixes.name, ' ', courses.catalog_number)"), 'LIKE', '%' . $search_term . '%')
                ->paginate();
        } else {
            $results = $options->paginate();
        }

        return new CourseCollection($results);
    }
}

Create a Course Resource and ResourceCollection

class Course extends JsonResource
{
    /**
     * Transform the resource into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        return [
            'id'               => $this->id,
            'prefix'           => \App\Http\Resources\Prefix::make($this->prefix),
            'identifiable_name' => $this->identifiable_name,
            'catalog_number'   => $this->catalog_number,
            'title'            => $this->title,
            'description'      => $this->description,
            'is_transferable'  => $this->is_transferable,
            'credits_min'      => $this->credits_min,
            'credits_max'      => $this->credits_max,
        ];
    }
}
class CourseCollection extends ResourceCollection
{
    /**
     * Transform the resource collection into an array.
     *
     * @param  \Illuminate\Http\Request  $request
     * @return array
     */
    public function toArray($request)
    {
        return [
            'data' => $this->collection,
        ];
    }
}

What I expected to happen

Laravel's default pagination object should be compatible with select2

What happened

Inconsistencies between what Laravel outputs and what select2 expects. Here's the Laravel output using Resources and ResourceCollections:

{ 
"data":[ ... ],
"links":{ 
    "first":"http:\/\/127.0.0.1:8000\/api\/course?page=1",
    "last":"http:\/\/127.0.0.1:8000\/api\/course?page=757",
    "prev":null,
    "next":"http:\/\/127.0.0.1:8000\/api\/course?page=2"
},
"meta":{ 
    "current_page":1,
    "from":1,
    "last_page":757,
    "path":"http:\/\/127.0.0.1:8000\/api\/course",
    "per_page":15,
    "to":15,
    "total":11355
}
}

Here's what is expected by Backpack's configuration of select2 (this is Laravel's default pagination object from an Eloquent query)

{ 
"current_page":1,
"data":[ ... ],
"first_page_url":"http:\/\/127.0.0.1:8000\/api\/course?page=1",
"from":1,
"last_page":757,
"last_page_url":"http:\/\/127.0.0.1:8000\/api\/course?page=757",
"next_page_url":"http:\/\/127.0.0.1:8000\/api\/course?page=2",
"path":"http:\/\/127.0.0.1:8000\/api\/course",
"per_page":15,
"prev_page_url":null,
"to":15,
"total":11355
}

What I've already tried to fix it

I have not attempted to fix this, but the proposed fix is modifying backpack's configuration of the select2 plugin. Below is the relevant part of the current configuration:

processResults: function (data, params) {
    params.page = params.page || 1;

    var result = {
        results: $.map(data.data, function (item) {
            textField = "identifiable_name";
            return {
                text: item[textField],
                id: item["id"]
            }
        }),
       pagination: {
             more: data.current_page < data.last_page
       }
    };

    return result;
},

Backpack, Laravel, PHP, DB version

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

λ php artisan backpack:base:version
### PHP VERSION:
PHP 7.2.11 (cli) (built: Oct 10 2018 02:04:07) ( ZTS MSVC15 (Visual C++ 2017) x64 )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.2.0, Copyright (c) 1998-2018 Zend Technologies
    with Xdebug v2.7.2, Copyright (c) 2002-2019, by Derick Rethans

### BACKPACK PACKAGES VERSION:
backpack/backupmanager                1.4.10             Admin interface for managing backups in Backpack, on Laravel 5.2+
backpack/base                         1.1.13             Laravel Backpack's base package, which offers admin authentication and a blank admin panel using AdminLTE
backpack/crud                         3.6.33             Quickly build an admin interface for your Eloquent models, using Laravel 5. Build a CMS in a matter of minutes.
backpack/generators                   1.2.7              Generate files for laravel projects
backpack/permissionmanager            4.0.6              Users and permissions management interface for Laravel 5 using Backpack CRUD.
eduardoarandah/backpacklogviewer      1.0.6              Integrate ArcaneDev/LogViewer in your Laravel-Backpack project
laravel/framework                     v6.6.1             The Laravel Framework.

### MYSQL VERSION:
mysql  Ver 15.1 Distrib 10.1.40-MariaDB, for Win32 (AMD64)
pxpm commented 4 years ago

Hello @juventus18

Indeed compatibility with Laravel default resources could/should be made. But I don't forecast for it to be done in 3.6. Better chances in v4 ++ versions.

If you happen to make it, you might want to ping @tabacitu if he's on into merging this into 3.6. I think it at all depends on the effort needed to make it happen. v4 is priority atm.

Going to close this, but feel free to keep the discussion.

Best, Pedro

tabacitu commented 4 years ago

Thanks for bringing this up @juventus18 !

Indeed, as @pxpm said, we're no longer pushing updates to 3.6 - only to 4.x. So if we're to make this change, it should be done in Backpack v4.

I do agree with you that this should be tested and fixed in Backpack v4.0. If pagination inside the select2_from_ajax field doesn't work as expected, because select2 expects one thing and Laravel's pagination gives something else, then indeed, we can fix it in the blade file's javascript. And I think we should do that ASAP. Even if things change drastically when we introduce "relationship" and other complicated features in v4.1.

@pxpm could you please double-check if pagination works as expected in v4.0? And if not, work on a PR to solve this? Perhaps @juventus18 could help out, or double-check the solution, since he's the one that discovered this.

Cheers!

PS. We're actually working on better ways to add select2_from_ajax fields in Backpack v4. @pxpm is spearheading the efforts, but it looks like the BIG changes will only be merged in Backpack v4.1 (in 1-2 months). This also involves a Fetch operation that you can add to your CrudControllers; so for example if you have a ProductCrudController, and then in another entry would like to have a select2_from_ajax to pick Products, you won't have to create a route, controller, etc for it; just add the Fetch operation on the ProductCrudController.

juventus18 commented 4 years ago

No problem that it won't be merged into 3.6 (I will get around to upgrading... eventually). I also wanted to push this issue upstream to Laravel people since really, its THEIR collections pagination that returns something different from THEIR resources pagination - Backpack is just currently configured to use L6 collections pagination instead of L6 resources pagination for the select2 control (but I believe the resources pagination should take priority since that is the preferred way to send JSON resources i.e. select2_from_ajax, so it is still kind of a BP problem). Having said that, you may wish to wait to see what they do, if anything. I've submitted the issue to Laravel/Framework#31235

juventus18 commented 4 years ago

I took a look at what you have for your Fetch so far... in that implementation, you will probably want to stick with the current behavior/configuration for select2_from_ajax as that is using Eloquent pagination instead of Resource pagination... lets just hope it is resolved upstream and takes away the headache for everyone ;)

juventus18 commented 4 years ago

My previous issue was nixed as 'not a bug', so I posted on Laravel/Ideas#2037 if you want to track this further