[FIXED][3.1] paramsAsHash not working correctly in TreeLoader

[Gjslick]

Ext version tested:


Ext 3.1.0



Adapter used:


ext



css used:


only default ext-all.css





Browser versions tested against:


IE8
FF3.5 (firebug 1.4.5 installed)



Operating System:


WinXP Pro



Description:


Using a directFn with 'paramsAsHash: true' on an Ext.tree.TreeLoader doesn't exactly work as expected. If baseParams are defined when a node is clicked, the data sent to the server from the Direct function looks like this:

data: [ "id-of-clicked-node", { param1: 'value1', param2: 'value2' } ]



I believe that the resulting data should really look like this:

data: [ { param1: 'value1', param2: 'value2', node: "id-of-clicked-node" } ]

Which would maintain consistency with using 'paramsAsHash: true' on Ext.data.DirectProxy's as well.



Test Case:



var myTree = new Ext.tree.TreePanel( {
title: "Tree",
renderTo: Ext.getBody(),
height: 400,
width: 400,

loader: new Ext.tree.TreeLoader( {
directFn: Ext.ss.Test.getTreeNodes,
paramsAsHash: true,
baseParams: {
param1: "hello",
param2: "there"
}
} ),

root: new Ext.tree.AsyncTreeNode( {
text: "Root"
} )
} );
Check the data posted to the server by the Direct function when the root node is clicked.


Debugging already done:


The problem lies in how the params are put together in the TreeLoader


// Ext.tree.TreeLoader

getParams: function(node){
var buf = [], bp = this.baseParams;
if(this.directFn){
buf.push(node.id);
if(bp){
if(this.paramOrder){
for(var i = 0, len = this.paramOrder.length; i < len; i++){
buf.push(bp[this.paramOrder[i]]);
}
}else if(this.paramsAsHash){
buf.push(bp);
}
}
return buf;
}else{
var o = Ext.apply({}, bp);
o[this.nodeParameter] = node.id;
return o;
}
},

We end up with a two element array for 'buf', the first element being the string id of the TreeNode, the second being the baseParams object.



Possible fix:


This code would add the node id into a property of the hashed params object, which would be configurable by the nodeParameter config. This behavior is also what would be expected by reading the docs.


getParams: function(node){
var buf = [], bp = this.baseParams;

if( this.directFn ) {
buf.push( node.id );

if( this.paramOrder ) {
if( bp ) {
for( var i = 0, len = this.paramOrder.length; i < len; i++ ){
buf.push( bp[ this.paramOrder[ i ] ] );
}
}

} else if( this.paramsAsHash ) {
var o = Ext.apply( {}, bp );
o[ this.nodeParameter ] = node.id;
buf = [ o ]; // Reset buf to a 1 element array containing the params object
}
return buf;

} else {
var o = Ext.apply( {}, bp );
o[ this.nodeParameter ] = node.id;
return o;
}
}
It also seems to cover all situations, which are:

paramOrder: undefined, paramsAsHash: false
- Puts the node id string in as the only argument.
paramOrder: undefined, paramsAsHash: true
- Adds the node id string into a property of the hashed params object, specified by the nodeParameter config.
paramOrder: [ array of param names ], paramsAsHash: true/false (true/false, as specifying a paramOrder is supposed to nullify any paramsAsHash value)
- Puts the node id in as the first argument, and appends base params in the paramOrder, after it. It seems that the node id should possibly be appended to the base params instead of prepended to them, but this is the current implementation's order.

[hendricd]

@Gjslick --

Nice digging on that one!
You're right. Flexible placement of the node.id in paramOrder would support some flexibility (and a fix), so I adapted yours a bit to permit:


new Ext.tree.TreeLoader({
directFn : directFn,
baseParams : { format: 'tree' },
paramOrder : 'format node',
nodeParameter : 'node', //default
paramsAsHash : false

});
while maintaining compat with the current behaviour (forcing the node.id as the first argument).

Give this a shot and let me know how things look ?



Ext.override( Ext.tree.TreeLoader, {

getParams: function(node){
var bp = Ext.apply({}, this.baseParams),
np = this.nodeParameter,
po = this.paramOrder;

np && (bp[ np ] = node.id); //add node.id to params

if(this.directFn){
var buf = [node.id];
if(po){
//Default node.id as first argument IF 'nodeParameter' not specified in paramOrder
(np && po.join('|').indexOf('|'+np) > -1) && (buf = []);

for(var i = 0, len = po.length; i < len; i++){
buf.push(bp[ po[i] ]);
}
}else if(this.paramsAsHash){
buf = [bp];
}
return buf;
}else{
return bp;
}
}
});

[Gjslick]

Hey, great idea Doug! I wasn't even thinking in terms of customizing where the node can be placed in the parameters array when using paramOrder. That's the best of both worlds: default behavior consistent with what it's been, and the ability to move it anywhere. Good call! =D>

The only problem with your implementation is if we specify the nodeParameter as the first element in the paramOrder, we end up with two array elements of the node's ID. B)

I changed it just a bit from your implementation.



Ext.override( Ext.tree.TreeLoader, {

getParams: function( node ) {
var params = Ext.apply( {}, this.baseParams ),
nodeParameter = this.nodeParameter,
paramOrder = this.paramOrder;

params[ nodeParameter ] = node.id;

if( this.directFn ) {
var buf = [ node.id ];

if( paramOrder ) {
if( paramOrder.indexOf( nodeParameter ) > -1 )
buf = []; // reset 'buf' if the nodeParameter was included in paramOrder

for( var i = 0, len = paramOrder.length; i < len; i++ ) {
buf.push( params[ paramOrder[ i ] ] );
}

} else if( this.paramsAsHash ) {
buf = [ params ];
}
return buf;

} else {
return params;
}
}

} );
The side-effecting logical AND's we're killin' me btw, had to change them back to if statements. :">:)

Also, I don't think we need to test for the existence of the nodeParameter (or even for it to be a non-blank string) before adding it as a property of the params. Although, it could benefit the paramsAsHash case where a user could set the nodeParameter to a blank string, to basically dis-include the node's id from the hashed params altogether. Seems like this might be a bit of inconsistent behavior though.

Let me know whatcha think. And btw, great work on the ManagedIFrame extension. :) It's one of the cornerstones of my system.

Greg

[hendricd]

The only problem with your implementation is if we specify the nodeParameter as the first element in the paramOrder, we end up with two array elements of the node's ID. B) ...
Greg

With the exception of the unnecessary 'join fat', both versions already account for that.

Committed to SVN - 5820.

[Gjslick]

Nice, thanks dude, and keep up the great work!

Greg