Editor 1.6 not handling errors on failed edit.

IsaacBen

I had the 1.5.6 trial version and things went fine, now I replaced it with the licensed 1.6 version and it broke the edit error handling.
In my backend I check for uniqueness of the value. If it's not unique I return an error, but now when I do that I get an error in the console.

Cannot read property 'length' of undefined

It doesn't call the error handler that I have

        editor.on('submitError', function (e, xhr, err, thrown, data) {
            var errorMessage = JSON.parse(xhr.responseText);
            console.log(errorMessage);
            //Get the first key of the object.
            var firstKey = Object.keys(errorMessage)[0];
            toastr.error(errorMessage[firstKey][0]);
        });

How come it worked fine in the 1.5.6 trial version?

IsaacBen

For some reason the

if ( json.error || json.fieldErrors.length )

Is not returning true.
See in the image the error line.

Edit: the code in the image is inside,

Editor.prototype._submitSuccess

which doesn't make sense, it should be in submitError instead. I'm returning from the server 422.

IsaacBen

I did a bit more testing and it seems to be an issue with version 1.6. For now I rolled back to 1.5.6. Hope this will get fixed soon.

IsaacBen

From the release notes of 1.6 I suspect that this is the issue

"Improved REST support. HTTP status codes other than 2XX codes can now be processed as successful responses if they contain valid JSON. This is useful when integrating with a REST service were status codes such as 400 are used it indicate validation errors."

So it returns 422 code as success, but since my error object is not in the right format it breaks the process. It's not good, this should be returned as error since the action failed. So I need a way to revert this to return an error instead, because I prefer not to customize the way I return errors in the backend.

IsaacBen

So far I modified this part inside dataTable.editor.js.

        complete: [ function ( xhr, text ) {
            // Use `complete` rather than `success` so that all status codes are
            // caught and can return valid JSON (useful when working with REST
            // services).
            var json = xhr.status === 204 ?
                {} :
                xhr.responseJSON;

            if ( $.isPlainObject( json ) && (xhr.status !== 422) ) {
                success( json );
            }
            else {
                error( xhr, text, thrown );
            }
        } ]

I added this - && (xhr.status !== 422) so in case of 422 it won't return success.
However, it still treats it as a server error so it adds "A system error has occurred". To remove it I added this part.

            i18n: {
                error: {
                    system: ""//Don't show anything
                }
            },

Sorry for all the self comments, but I had to "solve" this issue.

I really hope this change would be reverted as it confuses everything. An error is an error and it should be treated that way.

allan

Thanks for your thoughts on this!

I really hope this change would be reverted as it confuses everything. An error is an error and it should be treated that way.

The problem is that there is a difference between a "known error" (i.e. one whereby validation fails and you correctly respond to that - that I consider to be a successful request since it was properly and correctly handled), and an "unknown error" (typically resulting in invalid JSON returned with an error message from an exception).

Could you show me the JSON that you are returning when this issue happens?

Thanks,
Allan

IsaacBen

Yes, but when you pass it to the success function you're now forcing me to start customizing the way my backend framework returns errors.
Plus, I have now one success function that needs to handle all success and all error messages. This is a bit too much I think.
My backend returns the error in this format

 {data.5.name: Array[1]}

You're pretty much forcing me to use an older version, which is fine I guess as it works too, but that "small" change you did will break all error validations now for everyone who will update to it. It doesn't worth it and it doesn't help at all.

Sorry if I sound aggressive, I'm not at all, just trying to explain that this change is just not right.

IsaacBen

At least don't check the JSON format if it's an error code. Don't treat it like a data object, because it's clearly not. That way it will go through success function, but it won't break everything, because of format requirements.

allan

Thanks - I'm sorry if I sound stubborn! I'm not - I'm just trying to fully understand this, so if I do change it again, then I'm not going to muck everything up. I'm very much open to the suggestion that it should be changed, but I'm not fully understanding it at the moment I'm afraid.

At least don't check the JSON format if it's an error code.

If validation fails it is quite valid to return a 400 Bad request with a JSON payload detailing the error per the client / server documentation.

That's why I've got everything that is valid JSON going into the "success" handler (because it has been correctly dealt with), while anything that is not JSON is an unexpected value and therefore in error.

I think the problem here is the overloading of the term "error". An error can mean either a server-side processing failure, or a controlled fail such as validation. submitError occurs for the former.

My backend returns the error in this format [...]

You must be using something to convert it to the format Editor expects? What are you using for that?

Allan

IsaacBen

Let me explain the problem from my view.
There are two case scenarios aside of server errors.

1. Success - In that case I'm crating/updating/deleting data and I return the data in a format that DataTables requires. Like explained here - https://editor.datatables.net/manual/server
2. Error - in this case a 422 validation error. You're right, this is still a successful request, but for me and the user this is an error message that I need to present to him. In this case my server just returns an error in a JSON format that I have no control on.

The change that you introduced has two impacts:

1. It sends 400 errors to the success function. If it was that alone then that might be fine.
2. It checks for "json.data.length", which doesn't exists, as so far I know that I'm handling the errors. Now I need to make sure that all errors are returned as a data object, which in my case means that I have to manually change things in my backend.

All I'm asking is that you either change it to the way it was, or don't look for a data object. If you see that the response is 400 then return it right away so I can handle it. That way I don't have to work hard on my backend to change things.

IsaacBen

Now, you can always say "I don't care, that's a problem in you backend", but the point here is that DataTables shouldn't care in what format my error objects are(aside of being valid JSON). Let me show it to the user the way I want to.

IsaacBen

Okay, I tried now to play by the DataTables rules and returned my error object as expected, but now DataTables tried to actually insert it into a row and because of missing parameters I get an error.

This is how I return it

        $validator = Validator::make($request->all(), [
            'data.*.name' => 'unique:priorities',
        ]);

        if ($validator->fails()) {
            return response()->json(['data' => [$priorityName." Priority already exists, please use a different name."]], 422);
        }

Returned object is

{"data":["Fast Priority already exists, please use a different name."]}

I get a warning alert

DataTables warning: table id=DataTables_Table_0 - Requested unknown parameter 'tickets_count' for row 2, column 1. For more information about this error, please see http://datatables.net/tn/4

Not sure what I'm missing here.

allan

or don't look for a data object.

Very happy to put that change in and it will be in 1.6.1 - now committed. I think that is a correct change and should be considered a bug that I wasn't checking for that already.

Immediately after the block:

    if ( !json.fieldErrors ) {
        json.fieldErrors = [];
    }

I've added:

    if ( !json.data ) {
        json.data = [];
    }

That will allow any data checks which happen later on (used int he data source "plug-ins" as well as the line it is erroring on at the moment) to just do nothing.

If you are okay to put that into your local copy I'd appreciate it if you could let me know if that does the business for you.

Regards,
Allan

IsaacBen

Perfect, that sends it to the success function and now I can set rules to handle it by myself.
Thank you very much.

Maybe consider checking status code too, because if someone will return an error as a data object it might still break it.

IsaacBen

Okay, I spotted now a problem. If I return an error then I can't edit anymore until I refresh the page.
Your solution removes the console error, but something is still wrong there.

allan

Could you show me your full Editor initialisation please? I'll replicate a test case here. I think what might be required is to use postSubmit to add an error property to the JSON object.

You are presumably using something to manipulate the data into a format Editor understands at the moment. What is that code?

DataTables warning: table id=DataTables_Table_0 - Requested unknown parameter 'tickets_count'

What is happening because the data you are returning is an array, not an object, so there is no tickets_count property (which DataTables has been configured to look for with columns.data).

Sorry this is frustrating!

Allan

IsaacBen

Here you go

        var editor = new $.fn.dataTable.Editor( {
            ajax: {
                edit: {
                    type: 'PUT',
                    url:  '/priorities/_id_',
                    dataType: 'JSON',
                    beforeSend: function (xhr) {
                        var token = $('meta[name="csrf_token"]').attr('content');
                        if (token) {
                            return xhr.setRequestHeader('X-CSRF-TOKEN', token);
                        }
                    },
                },
                remove: {
                    type: 'DELETE',
                    url:  '/priorities/_id_',
                    dataType: 'JSON',
                    beforeSend: function (xhr) {
                        var token = $('meta[name="csrf_token"]').attr('content');
                        if (token) {
                            return xhr.setRequestHeader('X-CSRF-TOKEN', token);
                        }
                    },
                }
            },
            i18n: {
                remove: {
                    confirm: "Are you sure you want to delete this priority?"
                },
                error: {
                    system: ""
                }
            },
            table: '.table.priority-table',
            idSrc:  'id',
            fields: [
                { label: 'Priority', name: 'name' },
                { label: 'Count', name: 'tickets_count' },
            ]
        });

        editor.on('submitSuccess', function (e, json, data) {
            console.log(json);
            if(json.formError) {
                toastr.error(json.formError[0]);
            }
            if(json.deleteSucess) {
                toastr.success(json.deleteSucess);
            } else {
                toastr.success("Priority has been updated successfully!");
            }
        });

        editor
        .on( 'onOpen', function () {
            // Listen for a tab key event
            $(document).on( 'keydown.editor', function ( e ) {
                if ( e.keyCode === 9 ) {
                    e.preventDefault();

                    // Find the cell that is currently being edited
                    var cell = $('div.DTE').parent();

                    if ( e.shiftKey && cell.prev().length && cell.prev().index() !== 0 ) {
                        // One cell to the left (skipping the first column)
                        cell.prev().click();
                    }
                    else if ( e.shiftKey ) {
                        // Up to the previous row
                        cell.parent().prev().children().last(0).click();
                    }
                    else if ( cell.next().length ) {
                        // One cell to the right
                        cell.next().click();
                    }
                    else {
                        // Down to the next row
                        cell.parent().next().children().eq(1).click();
                    }
                }
            } );
        } )
        .on( 'onClose', function () {
            $(document).off( 'keydown.editor' );
        } );

        var editIcon = function ( data, type, row ) {
            if ( type === 'display' ) {
                return data + ' <i class="fa fa-pencil"/>';
            }
            return data;
        };

        $('.table.priority-table').on( 'click', 'tbody td:not(:nth-child(2)) i', function (e) {
            e.stopImmediatePropagation();
            editor.inline( $(this).parent() );
        } );


        var table = $('.table.priority-table').DataTable({
            select: {
                style: 'single'
            },
            lengthChange: true,
            //sDom: "",
            ajax: '/priorities',
            columns: [
                { "data": "name", render: editIcon},
                { "data": "tickets_count" },
            ],
            dom: '<<t>B>',
            buttons: [
                { extend: "remove", editor: editor }
            ],
            initComplete: function(settings, json) {
                 table.buttons().container().appendTo( '.colsm6:eq(0)', table.table().container() );
            }
        });

IsaacBen

The alert of the missing parameter happened before I added the fix. However it will happen to everyone who returns an error as a data object since you're currently not checking for the status code, which I think is the most important part. The way data is returned should never break behavior.

The only problem now is to allow to edit again after an error.

TBH, just revert it back, it doesn't worth your frustration
I'm pretty sure no one asked for that change as well, since this is how it works by default in jQuery AJAX.

allan

I'm pretty sure no one asked for that change as well

Actually, this change was based on a number of requests whereby people wanted to return an error status code, with valid JSON from their REST API. The idea was that valid JSON should be consider successful in terms of the request being processed (i.e. including validation failing) regardless of error code.

I've just sent you a PM with a new build of Editor that I believe should address the issue you were seeing when no data property was returned. To test I used the following in my PHP:

if ( isset( $_POST['action'] ) && $_POST['action'] === 'edit' ) {
    http_response_code( 422 );
    echo '{ "data.5.name": [ "Info" ] }';
    exit;
}

i.e. on edit just respond with the JSON you mentioned above. The table remains editable with the latest build and the rows are unaffected.

Regards,
Allan