Validation during Import

[gvanto]

A note on the validation during upload, think there might still be a slight bug (I updated to 1.6.4) as I have a rule that my redirect.source should be unique (this validation works as it should with the single record create form in backpack), but I am finding that it does not skip rows that violate this rule, it simply uploads them (replacing old record).

My RedirectCrudController:

<?php

namespace App\Http\Controllers\Admin;

use App\Http\Requests\RedirectRequest;
use App\Models\Redirect;
use App\Traits\CrudAccessUsingPermissions;
use Backpack\CRUD\app\Http\Controllers\CrudController;
use Backpack\CRUD\app\Http\Controllers\Operations\CreateOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\DeleteOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\ListOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\ShowOperation;
use Backpack\CRUD\app\Http\Controllers\Operations\UpdateOperation;
use Backpack\CRUD\app\Library\CrudPanel\CrudPanel;
use Backpack\CRUD\app\Library\CrudPanel\CrudPanelFacade as CRUD;
use RedSquirrelStudio\LaravelBackpackImportOperation\ImportOperation;

/**
 * Class RedirectCrudController
 * @package App\Http\Controllers\Admin
 * @property-read CrudPanel $crud
 */
class RedirectCrudController extends CrudController
{
    use CrudAccessUsingPermissions;

    use ListOperation;
    use CreateOperation;
    use UpdateOperation;
    use DeleteOperation;
    use ShowOperation;
    use ImportOperation;

    /**
     * Configure the CrudPanel object. Apply settings to all operations.
     * 
     * @return void
     */
    public function setup()
    {
        CRUD::setModel(Redirect::class);
        CRUD::setRoute(config('backpack.base.route_prefix') . '/redirects');
        CRUD::setEntityNameStrings('redirect', 'redirects');

        // Assign the redirect to the logged in user
        // https://backpackforlaravel.com/docs/6.x/getting-started-crud-operations#callbacks-1
        Redirect::saving(function (Redirect $redirect) {
            $redirect->user_id = backpack_auth()->user()->id;
        });

        $this->setAccessUsingPermissions();
    }

    /**
     * Define what happens when the List operation is loaded.
     * 
     * @see  https://backpackforlaravel.com/docs/crud-operation-list-entries
     * @return void
     */
    protected function setupListOperation()
    {
        CRUD::column('source');
        CRUD::column('target');
        CRUD::column('user');

        CRUD::column('created_at');
        CRUD::column('updated_at');
    }

    /**
     * Define what happens when the Create operation is loaded.
     * 
     * @see https://backpackforlaravel.com/docs/crud-operation-create
     * @return void
     */
    protected function setupCreateOperation()
    {
        CRUD::setValidation(RedirectRequest::class);

        CRUD::field('source')->type('text');
        CRUD::field('target')->type('text');
    }

    /**
     * Define what happens when the Update operation is loaded.
     * 
     * @see https://backpackforlaravel.com/docs/crud-operation-update
     * @return void
     */
    protected function setupUpdateOperation()
    {
        $this->setupCreateOperation();
    }

    /**
     * CSV import
     * https://github.com/redsquirrelstudio/laravel-backpack-import-operation
     *
     * @return void
     */
    protected function setupImportOperation()
    {
        // https://github.com/redsquirrelstudio/laravel-backpack-import-operation/issues/4
        $this->disableUserMapping();

        CRUD::addColumn([
            'name' => 'source',
            'label' => 'Source',
            'type' => 'text',
            'primary_key' => true
        ]);

        CRUD::addColumn([
            'name' => 'target',
            'label' => 'Target',
            'type' => 'text',
        ]);

        CRUD::setValidation(RedirectRequest::class);
    }
}

My RedirectRequest class:

<?php

namespace App\Http\Requests;

use Closure;
use Illuminate\Foundation\Http\FormRequest;
use Illuminate\Support\Str;

class RedirectRequest extends FormRequest
{
    /**
     * Determine if the user is authorized to make this request.
     *
     * @return bool
     */
    public function authorize()
    {
        // only allow updates if the user is logged in
        return backpack_auth()->check();
    }

    /**
     * Get the validation rules that apply to the request.
     *
     * @return array
     */
    public function rules()
    {
        return [
            'source' =>  [
                'required',
                'unique:redirects',
                function (string $attribute, mixed $value, Closure $fail) {
                    if (!Str::startsWith($value, '/')) {
                        $fail("The {$attribute} must start with '/'.");
                    }
                },
            ],
            'target' => [
                'required',
                function (string $attribute, mixed $value, Closure $fail) {
                    if (!Str::startsWith($value, '/') && !Str::startsWith($value, 'http')) {
                        $fail("The {$attribute} must start with '/' or 'http'.");
                    }
                },
            ],
        ];
    }

    /**
     * Get the validation attributes that apply to the request.
     *
     * @return array
     */
    public function attributes()
    {
        return [
            //
        ];
    }

    /**
     * Get the validation messages that apply to the request.
     *
     * @return array
     */
    public function messages()
    {
        return [
            //
        ];
    }
}

Before import:

Import CSV: source,target /foo1,/bar-dupe-test-4 /foo2,/bar2 /foo3,/bar3

After import (notice /foo1 has been updated instead of skipped)

[redsquirrelstudio]

Hi @gvanto

I think this poses a slight issue. Because you have specified 'source' as the primary key, it is used to determine whether to create or update a row.

The way I would go about this, is to set another column as the primary key, and then you can make source unique.

For example, maybe an ID field?

Hope this makes sense, Many thanks

[redsquirrelstudio]

Hi again @gvanto

While developing events, I can see that there was a bug with validation, it may solve your problem.

Please try installing v1.6.5 and see if the behaviour has changed for you.

[gvanto]

Hi @redsquirrelstudio I've updated to 1.6.5 but getting this on import:

Looks like ImportRowProcessed and ImportRowSkipped below should be ImportRowProcessedEvent and ImportRowSkippedEvent ?

[redsquirrelstudio]

Hi @gvanto

My bad, shouldn't update whilst tired, please run composer require redsquirrelstudio/laravel-backpack-import-operation

It should redownload a fixed version of 1.6.5

Many thank for your patience

[gvanto]

No worries @redsquirrelstudio and thanks for the quick fix.

I am however still getting same behaviour, the row is getting replaced ie validation not quite working.

Before:

CSV file:

source,target /foo1,/bar1-csv /foo2,/bar2-csv /foo3,/bar3-csv

After: (foo1 replaced with CSV file's version):

[redsquirrelstudio]

Hi @gvanto

I think this poses a slight issue. Because you have specified 'source' as the primary key, it is used to determine whether to create or update a row.

The way I would go about this, is to set another column as the primary key, and then you can make source unique.

For example, maybe an ID field?

Hope this makes sense, Many thanks

Hi @gvanto did you see the above comment?

[gvanto]

Hey @redsquirrelstudio,

Saw your comment ...

Couple of points

• It shouldn't matter what the primary key is (I shouldn't need to create an 'id' field just for the purpose of validation, right?)
• Strangely enough, for my use case it actually doesn't matter that my records get overridden 🤣 Which actually makes me think what would be awesome for this package is the following: The ability to decide what happens in case of validation fail!

In most cases ideally records should be skipped and and the user notified of those records, not with a flash message - disappears too quickly - but rather a big red box above the list view showing failed records (I guess this is do-able using your ImportEvents ... which I haven't got around to doing because well, nothing is failing 🤣 )

And in the case of records getting overwritten (ie I selected that option during upload or in code), they can be shown as a notification at the end, as with failed records ...

Again, thanks for an awesome package 👊

[redsquirrelstudio]

Hi @gvanto

Thank you for following up, rest assured I want to get to a solution for this issue :)

To address your points:

• The primary key is important as it is used to see whether a row should be updated or created. Therefore it is not validated for uniqueness because otherwise we wouldn't be able to update a row.

I suppose a work around for this would be adding a "don't update" option which would only create records.

• I agree this would be a good feature, my thoughts are either: 1) The user should be able to provide a class that has a 'handle' function that stops the import and does some sort of action. 2) A "stop import on fail" option should be added so that the import just stops, the user can then use the import row skipped event to determine what happens (maybe rename this to import row failed).

Let me know your thoughts about the above,

Thank you for helping to improve the package and it's use cases :D

[gvanto]

Hey @redsquirrelstudio,

"The primary key is important as it is used to see whether a row should be updated or created. Therefore it is not validated for uniqueness because otherwise we wouldn't be able to update a row."

I see the issue now ...

"I suppose a work around for this would be adding a "don't update" option which would only create records." This would be nice yep!

Yes absolutely agree with points 1 and 2.

Things can get complicated if allowing the user (note: not the developer), to select what happens on import fail (this would require selecting "skip/not" on every rule!) I think it kind of defeats the purpose of having validation at all perhaps - a better approach might be to let the developer create a '{modelName}ImportRequest' class and define import-specific rules there (?). IMHO giving users too many options/choices just overcomplicates things and inevitably they will screw things up 🤣 This also forces them to ensure their import data is clean to begin with ...

What would be great (especially if you have some examples in your README), is how to implement the "showing which rows failed to import after the import" part ... I am not entirely sure how to do this (show a message box with feedback info back on the list view, after returning from the import - guess it involves adding stuff to flash and then showing them if they exist ...), as I'm still quite a backpack noob (but loving this package and ones like yours, making building great UIs a breeze!)

Hope you have a great day Lewis!

[gvanto]

Hi @redsquirrelstudio

I've also just tested import validation for 'target' field of my Redirect class, seems its also not working as expected.

The following CSV file uploaded ALL rows (redirects table empty before upload). The first row should have been skipped, since 'bar1-csv' does not meet validation criteria.

source,target /foo1,bar1-csv /foo2,/bar2-csv /foo3,/bar3-csv

protected function setupImportOperation()
    {
        // https://github.com/redsquirrelstudio/laravel-backpack-import-operation/issues/4
        $this->disableUserMapping();

        CRUD::setValidation(CreateRedirectRequest::class);

        CRUD::addColumn([
            'name' => 'source',
            'label' => 'Source',
            'type' => 'text',
            'primary_key' => true
        ]);

        CRUD::addColumn([
            'name' => 'target',
            'label' => 'Target',
            'type' => 'text',
        ]);
    }

CreateRedirectRequest rules:

public function rules()
    {
        return [
            'source' =>  [
                'required',
                'unique:redirects',
                function (string $attribute, mixed $value, Closure $fail) {
                    if (!Str::startsWith($value, '/')) {
                        $fail("The {$attribute} must start with '/'.");
                    }
                },
            ],
            'target' => [
                'required',
                function (string $attribute, mixed $value, Closure $fail) {
                    if (!Str::startsWith($value, '/') && !Str::startsWith($value, 'https://')) {
                        $fail("The {$attribute} must start with '/' or 'https://'.");
                    }
                },
            ],
        ];
    }

[gvanto]

And with empty value for 'target', it still uploads the record:

source,target /foo1,bar1-csv /foo2,/bar2-csv /foo3,/bar3-csv /foo4,

using package 1.6.5 btw

[redsquirrelstudio]

And with empty value for 'target', it still uploads the record:

source,target /foo1,bar1-csv /foo2,/bar2-csv /foo3,/bar3-csv /foo4,

using package 1.6.5 btw

Hi @gvanto, could you try v1.6.7 please

With many thanks and warm regards,

Lewis

[gvanto]

hi @redsquirrelstudio

Updated to 1.6.7 and both rows still upload: