[Bug] ComboBoxItem: wrong criteria is sent to server on item pick up.

[antanas.arvasevicius]

Hi,

I've found that ComboBoxItem which is bounded to DataSource is behaving somehow incorrectly.

Theory:
valueMap             - a dictionary how to map input string to ID (with data bound it's implemented differently, but logical the same)
getValue()           - real value of ComboBoxItem will return ID if found in valueMap or the same value as getEnteredValue()
getEnteredValue() - value of ComboBoxItem which is entered or selected by user (string)
displayFieldName   - is field name which will be searched by if input is not found in picklist
valueFieldName     - is field name which stores real value for ComboBoxItem  (basically it`s primary key of datasource)

If ComboBox will find `getEnteredValue()` in it's `valueMap` then resolved ID will be used,
but if not found then ComboBox will trigger fetchData on DataSource with criteria := {displayFieldName: getEnteredValue()}.

The problem is that ComboBox now is triggering fetchData with criteria := {displayFieldName: getValue()} instead and our datasource can't find any records with "name"=2

Firstly I cannot find out why ComboBox triggers that fetch at all because I've picked up item from drop down.

Bug is here:

//ComboBoxItem.js at handleEditorExit method:

                // Otherwise, consider the value to be a display value. If we have an ODS and
                // display field, check whether we have a data value for it. If not, try to fetch
                // a data value for this display value using _checkForValueFieldValue().
                } else if (ods != null && displayField != null
                           && elementValue != null && elementValue != "" &&
                           this.shouldFetchMissingDisplayValue(elementValue)
                          )
                {
                    this._checkForValueFieldValue(value); // <- Here must be an elementValue not a value.
                }

Please fix this then you'll have time.

PS.

I've analyzed why ComboBox triggers that fetch at all and found that `this.shouldFetchMissingDisplayValue(elementValue)`
is returning true.

FormItem._shouldFetchMissingValue(..): in this._displayFieldCache object I see only one record instead of full picklist record set.
Shouldn't be this method related with `mapDisplayToValue()` method? because input value mapping to ID is done by this method not by some _displayFieldCache object? if _displayFieldCache is cleared `mapDisplayToValue()` works correctly but that _shouldFetchMissingValue() returns true - not logical.

Thanks.



Affected versions:
all versions including SmartClient_v100p_2015-05-21

[Isomorphic]

Thanks for the report. Our developers will take a look at this and we'll follow up when we have more information for you.

Regards
Isomorphic Software

[antanas.arvasevicius]

Hi,
I've made further investigation and what to share with my findings.

1.
FormItem._displayFieldCache is populated only with the record which ID's value is current ComboBoxItem's value.
The _displayFieldCache wont store any records from pick list, tried to find maybe there is some "dataArrived" or other handlers which populates _displayFieldCache on first pick list load or something, but haven't find anything.
So my conlusion is that FormItem._displayFieldCache is used only to cache some information which relates to missing values.

2.
FormItem.shouldFetchMissingDisplayValue(newValue) method will call:
2.1. `_shouldFetchMissingValue` method which checks various caches including _displayFieldCache to know is this display value 'newValue' is already fetched if not found in cache then returns null.
2.2. because of null, we further call FormItem._unmapKey(value, dontReturnValue) to check is this 'newValue' is in valueMap.
2.2.1 then var map = this.getValueMap(); is called, it return a map of a single record, not an items from picklist..
2.2.2 check whether value is in a map, if not then returns false
2.3. as previously _unmapKey returned false we are returning true (as should fetch missing value)


meanwhile, ComboBoxItem.mapDisplayToValue(value) is returning ID from picklist.

mapDisplayToValue(..): will call PickList._translateValueFieldValue(...) which correctly returns ID, as it traverses picklist datasource cache.


I'm thinking that FormItem.shouldFetchMissingDisplayValue() should incorporate that _translateValueFieldValue method in should fetch
missing value calculations. Should override shouldFetchMissingDisplayValue in ComboBoxItem and add additional logic?

As now I'm understanding _displayFieldCache is not related to this problem.

[Isomorphic]

Thanks for your analysis.
Before we dig into this, can we back up to the basic steps to get this unexpected fetch.

If you have a simple ComboBoxItem bound to the sample supplyItem dataSource - something like this:

isc.DynamicForm.create({
   ID:"testForm",
    width: 500,
    fields : [{
        name: "itemName", title: "Item Name", editorType: "comboBox", 
        optionDataSource: "supplyItem", pickListWidth: 250,
        displayField:"itemName",
        valueField:"itemID"
    }]
});

The steps of clicking the icon to show the drop down, then selecting an item from it do not cause an unexpected fetch of the kind you describe.
What steps are your taking to see the problem? If necessary, please show us a small runnable snippet of code similar to this so we can see exactly how this bad state is being reached.

Thanks
Isomorphic Software

[antanas.arvasevicius]

Hi,

yes in that simple case is just working, but try to add one more time the same dynamic form.

isc.VLayout.create({
	members: [
		isc.DynamicForm.create({
		   ID:"testForm",
			width: 500,
			fields : [{
				name: "itemName", title: "Item Name", editorType: "comboBox", 
				optionDataSource: "supplyItem", pickListWidth: 250,
				displayField:"itemName",
				valueField:"itemID"
			}]
		}),
		isc.DynamicForm.create({
		   ID:"testForm",
			width: 500,
			fields : [{
				name: "itemName", title: "Item Name", editorType: "comboBox", 
				optionDataSource: "supplyItem", pickListWidth: 250,
				displayField:"itemName",
				valueField:"itemID"
			}]
		})
	]
});

Steps to reproduce:

1. Click on first combobox picker and let menu to drop down
2. Click on the second combobox and select a some value
3. move focus and cursor away from the second combo box (the request should be triggered)
------------

Now I understand why is that not working.

It's because second combobox picklist is somehow rendered from a ds cache and datasource won't trigger dataArrived handler, and
PickList.handleDataArrived() is not triggered on second combobox.
In good scenario `handleDataArrived` will be triggered and all received data will be placed in `FormItem._displayFieldCache`

As mentioned in previous post `FormItem.shouldFetchMissingDisplayValue(newValue)` will look firstly at `_displayFieldCache` (look at this._shouldFetchMissingValue method) to determine whether is not needed to fetch, if not found in cache it will look further by this._unmapKey(...) to determine is value is in valueMap.

But as I said mapDisplayToValue(..) correctly returns an ID, so it's strange that shouldFetchMissingDisplayValue is not strictly related to mapDisplayToValue(..).

Maybe a simple solution would be extend shouldFetchMissingDisplayValue in ComboBoxItem class and incorporate `_translateValueFieldValue` method in determining whether to fetch or not, because that method traverses picklist's listgrid cache directly.


Real case for problem was: that we are using ListGrid with filter editor, and combo boxes in edit forms and listgrid's filter, and they are bound to the same datasource with the same field name.

Hope that helps you :)

[antanas.arvasevicius]

Hello,

Any news on this issue? Have you any plans to go on github? It would be easier to contribute.

[Isomorphic]

We have made a change to resolve this issue. Please try the next nightly build dated June 3 (10.0 / 10.1 branches)

Regards
Isomorphic Software

[antanas.arvasevicius]

Thank you!

But I still getting problems, two ComboBoxItems seems working at first, but if I am using SelectItem and ComboBoxItem then ComboBoxItem do not have a correct `_displayFieldCache` as before.

and in SmartClient_v100p_2015-06-03_LGPL.zip\smartclientSDK\isomorphic\system\modules-debug\ISC_Forms.js:54746

               } else if (ods != null && displayField != null
                           && elementValue != null && elementValue != "" &&
                           this.shouldFetchMissingDisplayValue(elementValue)
                          )
                {
                    this._checkForValueFieldValue(value); // <- must be elementValue not a value.
                }

This also is wrong, as if that bug occurs then I am getting and ID to server and server returns zero records and that f*uk ups everything, because ComboBoxItem stucks in forever loading state.


I'll write back more info about SelectItem and ComboBoxItem which are bound to the same ds.

[antanas.arvasevicius]

Hi again!

Clarification, everything seems alright, but in my case

in ISC_Forms.js: _shouldFetchMissingValue(...):

this.optionDataSource is undefined
but this.getOptionDataSource() returns an object

    _shouldFetchMissingValue : function (value, fieldName) {

        if (this.fetchMissingValues == false) return false;
        if (this.getOptionDataSource() == null) return false;

        var superShouldFetch = this.Super("_shouldFetchMissingValue", arguments);
        if (superShouldFetch == false) return false;

        if (value != null && this.optionDataSource) { // <- this should be this.getOptionDataSource() instead direct reference.
            var recordFromPickList = this.getPickListRecordForValue(value, fieldName);
            if (recordFromPickList != null) {
                // Store the record from the pickList in our displayField cache

                this._addDataToDisplayFieldCache([recordFromPickList]);
                this._updateSelectedRecord();
                return false;
            }
        }
        return superShouldFetch;
    },

Yes looks `getOptionDataSource()` function do more work than just returning this.optionDataSource property, looks in form's default option value and more.

Hope this information will help you :)

[antanas.arvasevicius]

As waiting too long from smartclient to fix that bug, I've made a patch by myself:

Everyone can use this bugfix till smartclient will release proper fix:

isc.ComboBoxItem.addMethods({

	shouldFetchMissingDisplayValue : function (newValue) {
		var displayField = this.getDisplayFieldName();
		if (displayField == null) return false;

		var superShouldFetch = this.Super("shouldFetchMissingDisplayValue", arguments);
		if (superShouldFetch == false) return false;

		if (newValue != null && this.getOptionDataSource()) {
			var recordFromPickList = this.getPickListRecordForValue(newValue, displayField);
			if (recordFromPickList != null) {
				return false;
			}
		}

		return superShouldFetch;
	},

	shouldFetchMissingValue : function (newValue) {

		if (this.fetchMissingValues == false) return false;
		if (this.getOptionDataSource() == null) return false;

		var superShouldFetch = this.Super("shouldFetchMissingValue", arguments);
		if (superShouldFetch == false) return false;

		if (newValue != null && this.getOptionDataSource()) {
			var recordFromPickList = this.getPickListRecordForValue(newValue, this.getValueFieldName());
			if (recordFromPickList != null) {
				return false;
			}
		}

		return superShouldFetch;
	},

	// Given a value, reach into the pickList and find the associated record.
	getPickListRecordForValue : function (value, fieldName) {
		var record;
		if (this.pickList == null || this.pickList.destroyed) {
			if (this.progressiveLoading === true || this.progressiveLoading === false) {
				if (this.pickListProperties == null) this.picklistProperties = {};
				this.picklistProperties.progressiveLoading = this.progressiveLoading;
			}
			this.makePickList(false);
		}
		if (fieldName == null) fieldName = this.getValueFieldName();

		if (this.pickList && this.pickList.data) {
			record = this.pickList.data.find(fieldName, value);
		}
		return record;
	}

});

[antanas.arvasevicius]

Hi again,

it seems that we cannot use `getPickListRecordForValue()` in patch as it will call makePickList()
and makePickList() sets this._fetchingPickListData, and then `_checkForDisplayFieldValue` will set this._checkDisplayFieldValueOnFilterComplete to true, and then `_updateValueForFilterComplete` will call `_checkForDisplayFieldValue` again. But if datasource contains 1000 elements, only 75 records are fetched.
So if valueid is not in that 75 record set it never be loaded. so ComboBoxItem in DynamicForm with bounded to some record is displaying foreign key ID instead of real value on the first load.

After so many days debugging that ComboBoxItem component I could say that implementation is really screwed, looks like a bug patching over patching over many years, with many global states to skip/run logic somewhere else, really complicated stuff. Maybe component rewrite using some structures as future/promises would be better investment than patching around.

[Isomorphic]

Thanks for keeping us abreast of what you've been working on here. Indeed this is a complicated subsystem which we are likely to rework and simplify at some point in the future. (But obviously any such rework would not be being backported into existing release branches).

We have an engineer looking at the issues you've reported here (as well as your suggested changes) and will follow up when we have more information for you.

Regards
Isomorphic Software