tonytomov / jqGrid

jQuery grid plugin
www.trirand.com
2.84k stars 1.2k forks source link

After upgrade from 5.8.2 to current master ajax json call fail after using `datatype:local` #1028

Closed fredsal closed 1 year ago

fredsal commented 1 year ago

I make a debug and if i comment next line everything works again

if(ts.p.datatype==='local' && !grp._locgr) {
-   ts.p.datatype = 'jsonstring';
+   // ts.p.datatype = 'jsonstring';
    ts.p.datastr = ts.p.data;
}

https://github.com/tonytomov/jqGrid/blob/a64f9320c8bad23b19b97df2928bb1ec245f6af0/js/grid.base.js#L3815-L3818

Well, i have a grid with local datatype, but, depending on changes on a parent table, i change the datatype to json and reload data(only when required), but with that line the grid never reload the rows, but the rows do come in an ajax call, I don't know if it's a bug or if I'm doing something wrong, but what's clear to me is that with version 5.8.2 everything worked

This is the code for changing datatype and reload grid data

// i found this code on stackoverflow, and it was working without problems till now
let grid=$('#jqGrid');
grid.setGridParam({datatype:'json',postData:$.extend(true,OriginalData,MyDataObject)}); // MyDataObject is custom data
grid.trigger('reloadGrid',[page:1]);

I could make a demo if it is necessary

PD: Sorry for mi english, i'm learning, please be patient

tonytomov commented 1 year ago

Hello,

We always need a demo in order to look at this. Thank you

fredsal commented 1 year ago

@tonytomov thanks for answering, After digging I found out that 5.8.2 is not the problem, the problem is in master, And the problem only shows with grouping:true propertie, without that everything work as expected

Related commit: https://github.com/tonytomov/jqGrid/commit/8d2f3caa2fbe1c8084abd67d475c886eed700aab Demos: Both demos have the same code, just change jqgrid.base js from 5.8.2 to master Working Demo on v5.8.2: https://jsfiddle.net/6Lb9xy30/ Failing Demo on Master: https://jsfiddle.net/v2rfLwbh/ (here not showing rows)

Best regards, Fred

tonytomov commented 1 year ago

Hello,

Thank you very much for the test case.

The problem you point is more conceptual, rather than a bug. In your first working example (offical 5.8.2) if you replace datatype local with jsonstring and set datastr the problem is the same. Like this

  $("#jqGrid").jqGrid({
    url: '/echo/json/',
    datatype: "jsonstring",
    datastr :"",
    ...

Switching from one datatype to another when grouping require to reset a certain parameter in order this to work. I have created a method called groupingResetCalcs which should be called before you trigger the grid with the new datatype.

let grid=$('#jqGrid');
grid.jqGrid('setGridParam' , {datatype:'json',postData:{...})
.jqGrid('groupingResetCalcs')
.trigger('reloadGrid',{page:1});

We will describe this into the docs when the new release appear.

The commit is in GitHub, so you can test it.

Best Regards

fredsal commented 1 year ago

Test groupingResetCalcs: https://jsfiddle.net/jom906n1/ (it works)

It would have been great if it was something more internal since it looks like a BC to me, all previous versions worked, but at least I can fix it with groupingResetCalcs

Thank you very much, this is a great plugin

systemsolutionweb commented 1 year ago

@tonytomov hi, if we already set datatype to local, why change it to jsonstring internally? So, if we check if datatype is local on runtime, it would be false because it changes to jsonstring?

What about this, for avoid breaking changes

setGridParam : function (newParams, overwrite){
  return this.each(function(){
    if( $.jgrid.isNull(overwrite) ) {
        overwrite = false;
    }
    if (this.grid && typeof newParams === 'object') {
                // start added fix
                if (this.p.grouping && newParams.datatype && ['local', 'jsonstring'].includes(this.p.datatype)) {
                      $(this).jqGrid('groupingResetCalcs');
                }
                // end added fix
        if(overwrite === true) {
            var params = $.extend({}, this.p, newParams);
            this.p = params;
        } else {
            $.extend(true,this.p,newParams);
        }
    }
  });
},

https://github.com/tonytomov/jqGrid/blob/a95cc5bd59f07f5045068b67e095814f0f2b7387/js/grid.base.js#L5888-L5902

fredsal commented 1 year ago

@systemsolutionweb i did test your solution, it works without changing all mi code Demo: https://jsfiddle.net/gra2Lwpe/1/

tonytomov commented 1 year ago

Reason for this change was that with local datatype there was not possible to loop through the entire data and perform calculation when grouping is on, while with jsonstring this is possible.

More over after set jsonstring and performing calculation the datatype becomes local See here

and here to

The fix will be not accepted.