RowReorder/Editor still updated table even if server responded with error.

mavris

While reordering if the server responded with an error the table was still updated.
@allan, to fix this I changed the datatables.rowReorder.js file.
Can you check my solution, and maybe add it to the official file?
You should first change the error checking for the response so that it will be the same with the rest of the datatable editor.

I moved the update to a new function and then changed the function _mouseUp in line 452. The 2 functions are below

    /**
     * Mouse up event handler - release the event handlers and perform the
     * table updates
     *
     * @param  {object} e Mouse event
     * @private
     */
    _mouseUp: function ( e )
    {
        var that = this;
        var dt = this.s.dt;
        var i, ien;
        var dataSrc = this.c.dataSrc;
 
        this.dom.clone.remove();
        this.dom.clone = null;
 
        this.dom.target.removeClass( 'dt-rowReorder-moving' );
        //this.dom.target = null;
 
        $(document).off( '.rowReorder' );
        $(document.body).removeClass( 'dt-rowReorder-noOverflow' );
 
        clearInterval( this.s.scrollInterval );
        this.s.scrollInterval = null;
 
        // Calculate the difference
        var startNodes = this.s.start.nodes;
        var endNodes = $.unique( dt.rows( { page: 'current' } ).nodes().toArray() );
        var idDiff = {};
        var fullDiff = [];
        var diffNodes = [];
        var getDataFn = this.s.getDataFn;
        var setDataFn = this.s.setDataFn;
 
        for ( i=0, ien=startNodes.length ; i<ien ; i++ ) {
            if ( startNodes[i] !== endNodes[i] ) {
                var id = dt.row( endNodes[i] ).id();
                var endRowData = dt.row( endNodes[i] ).data();
                var startRowData = dt.row( startNodes[i] ).data();
 
                if ( id ) {
                    idDiff[ id ] = getDataFn( startRowData );
                }
 
                fullDiff.push( {
                    node: endNodes[i],
                    oldData: getDataFn( endRowData ),
                    newData: getDataFn( startRowData ),
                    newPosition: i,
                    oldPosition: $.inArray( endNodes[i], startNodes )
                } );
 
                diffNodes.push( endNodes[i] );
            }
        }
 
        // Create event args
        var eventArgs = [ fullDiff, {
            dataSrc:    dataSrc,
            nodes:      diffNodes,
            values:     idDiff,
            triggerRow: dt.row( this.dom.target )
        } ];
 
        // Emit event
        this._emitEvent( 'row-reorder', eventArgs );
 
        // Editor interface
        if (this.c.editor) {
            // Disable user interaction while Editor is submitting
            this.c.enabled = false;
 
            this.c.editor
                .edit(
                    diffNodes,
                    false,
                    $.extend({submit: 'changed'}, this.c.formOptions)
                )
                .multiSet(dataSrc, idDiff)
                .one('submitComplete', function (event, response) {
                    that.c.enabled = true;
                    if (response && !($.isEmptyObject(response.error) && $.isEmptyObject(response.fieldErrors))) {
                        if (that.c.onError && typeof  that.c.onError === 'function') that.c.onError();
                        dt.draw(false);
                    } else {
                        // Do update if required and if successful update
                        that._updateIfRequired.call(that, fullDiff, dt, dataSrc, eventArgs);
                    }
                })
                .submit();
        } else {
            // Do update if required and if not using editor
            this._updateIfRequired.call(this, fullDiff, dt, dataSrc, eventArgs);
        }
    },
 
    _updateIfRequired: function (fullDiff, dt, dataSrc, eventArgs) {
        // Do update if required
        if (this.c.update) {
            for (var i = 0, ien = fullDiff.length; i < ien; i++) {
                var row = dt.row(fullDiff[i].node);
                var rowData = row.data();
 
                this.s.setDataFn(rowData, fullDiff[i].newData);
 
                // Invalidate the cell that has the same data source as the dataSrc
                (function (i) {
                    dt.columns().every(function () {
                        if (this.dataSrc() === dataSrc) {
                            dt.cell(fullDiff[i].node, this.index()).invalidate('data');
                        }
                    });
                })(i);
            }
 
            // Trigger row reordered event
            this._emitEvent('row-reordered', eventArgs);
 
            dt.draw(false);
        }
    },

The user can also add a function as a RowReorder option named onError, that will be called whenever an error occurs (i.e. to notify the user). You can init it like this for example

                rowReorder: {
                    dataSrc: 'ordering',
                    selector: '.reorder',
                    editor: editor,
                    onError: function () {
                        "use strict";
                        App.alert({
                            type: 'danger',
                            message: 'Error while changing the row ordering.',
                            close: true,
                            reset: false,
                            focus: true,
                            closeInSeconds: 10,
                            icon: 'warning'
                        });
                    }

What do you think? Is there something wrong with my solution (besides changing the library files )?

mavris

There is an error in the code. that makes the ordering display wrongly in the table (if the request is successful) but I am too tired to fix it now. I will try again tommorrow.

@allan, is there no better way to do this without tampering datatables.rowReorder.js? I will try again tommorrow eith a clear head to see if I find another method or fix this one. Can you tell me if I am in the right track?

Regardless of my above code, why is the extension not caring if the request succeded? Shouldn't the display not change if ajax returned error for the editor? FYI only my edtor uses ajax (the table data are loaded from the dom) if that matters. I consider that a bug (but obviously failed to fix it yet )

allan

That's a very good point - thanks! I'll certainly look at getting RowReorder updated for this. I agree that this is a bug, and only the returned data should be used rather than the local values.

Regards,
Allan

mavris

Thank you allan.
For now I have just added a php validation to throw an error if the ordering you requested is invalid (creating duplicate order values for example).
I will come back to this later, but if you do fix it can you send me the updated files?

mavris

Until @allan has the time to fix the autoupdate bug in the RowReorder extension, I found a workaround that seems to work for me so far with the original RowReorder extension code.
At init of Rowreorder I set update to false and then during rowreorder event I check the server response and if something went wrong, I notify the user and redraw the table.

                rowReorder: {
                    dataSrc: 'ordering',
                    selector: '.reorder.sorting_1',
                    editor: editor,
                    update: false
                },

        table.on('row-reorder', function (e, fullDiff, edit) {
            editor.one('submitComplete', function (event, response) {
                // Simple error checking
                if (response && ((response.error && response.error.length > 0) || (response.fieldErrors && response.fieldErrors.length > 0))) {
                    //editor.disable(['ordering', 'icon','name','description']);
                    App.alert({
                        type: 'danger',
                        message: 'Error while changing the row ordering.',
                        close: true,
                        reset: false,
                        focus: true,
                        closeInSeconds: 10,
                        icon: 'warning'
                    });
                    table.draw(false);
                }
            }, this)
        });

So far it works properly.
To be honest I had already tried something like this yesterday, but the data returned did not contain valid DT_RowId values so it wasn't working correctly.

Also @allan as a note (not sure if it can be considered bug or not) If you have sorted the table by a column other than the one used for ordering (with class reorder) then when you reorder an item the resulting ordering values are unpredictable (undestandable as I would not know how to change the order of the items myself in that case).
I thnink you should not allow any reordering donw unless the ordering column is also used for sorting (basically in my above example the selector should be set as '.reorder.sorting_1') And the css of the RowReorder should become like below instead

table.dataTable td.reorder {
    cursor: default;
}
table.dataTable td.reorder.sorting_1 {
    cursor: move;
}

Like that you can only sort items if the table is sorted by the reorder column. The fix is easy but I think it should be included in the RowReorder as well.
What do you think?

allan

I thnink you should not allow any reordering donw unless the ordering column is also used for sorting

I mostly agree. The reason I haven't put this in and have it always on is that I can imagine some limited situations where it might be useful to allow reordering outside of the sorted order. Perhaps there should be an option to enable and disable this behaviour.

Allan