[bug] DynamicForm.valuesHaveChanged() returns always true after saveData() call

[antanas.arvasevicius]

Hi,
I've noticed that DynamicForm.valuesHaveChanged() always returns true then i call saveData().

Problem is that I have a field in datasource which is only generated by server side. `added_at` datetime.

I've declared that field in frontend DataSource as

                    {
                        name: "addedAt",
                        type: "datetime",
                        disabled: true,
                        useTextField: true,
                        shouldSaveValue: false
                    },

Problem is in DynamicForm._saveDataReply : function (request, response, data)

                var compareVal = submittedValues[i] === undef ? this._oldValues[i] : submittedValues[i];
                var field = this.getField(i);   // <- (1) HERE WE COMPARING FIELD VALUES WITH SERVER RETURNED VALUE
                // check whether the form item has changed since submission
                if (this.fieldValuesAreEqual(field, currentValues[i], compareVal)) {
                    // if not, check whether the server changed the submitted value to
                    // something else
                    if (!this.fieldValuesAreEqual(field, compareVal, data[i])) {
                        currentValues[i] = data[i];
                        hasChanges = true;
                    }

                } else {
                    // value in the form has changed since being submitted
                    rememberValues = false;  // <-- (2) HERE, REMEMBER VALUES ARE SET TO FALSE 
                }

You can see from code that (1) is comparing current dynamic field value with server response, and do not check is this field is even disabled or shouldSaveValue: false;
so then server returns a response with `added_at` value, this logic acts like dynamic form `added_at` is changed and (2) logic applies, so after save success, rememberValues() is not called, and I always getting
DynamicForm.valuesHaveChanged() true. And all field aren't remembered.

Please add into (1) a bypass if field has shouldSaveValue:false or disabled:true, maybe should bypass if field do not exists on dynamicform at all.

Actually it's deeper problem, because all fields which was edited is now treated like as not persisted, and
DynamicForm.valuesHaveChanged(true) returns all edited fields which was saved.

Suggestion:
I suggest not to call rememberValues() for all fields at once, but use one by one field values to set to _oldValues[] array if it was correctly persisted.

Tested on: v10.0p_2015-07-25/LGPL Deployment
But I think all versions are affected by this.

Thanks.

[Isomorphic]

It looks like you've misread this part of the code. What's actually being compared is the current values in the form vs the data that was sent to the server. This is designed to detect that a user has made further edits to the form values while waiting for the server to reply (for cases where this is allowed).

You can break this comparison if you've added logic that makes a call to editRecord() or to setValue()/setValues() after calling saveData(), and before the server has responded. That's most likely your actual problem.

[antanas.arvasevicius]

yes, I do understand this. But actually field `added_at` is intended to be not editable by user.
But that logic do comparision on that field. that field is not sent to server either. But our backend datasource will return that field `added_at` with timestamp (because it's autogenerated).

And that logic I will see that like: we've got `added_at` value but our form do not have that field at all. that value so lets skip all rememberValues logic.

By that all other fields which was edited before save treats like they are not saved (because rememberValues wasn't called, and _oldValues do not reflect changes).

And `added_at` field must be not visible in form, so it is not included in it.

Note: this case is in record editing, so `added_at` exists in record.


My urgent fix is to call rememberValues() manually on save callback.

I don't get it why don't remember any values if at least one is changed by user, because other values was sent and persisted to server.

Note2: then field do not exists on DynamicForm then all datetime fields will be not equal, and fieldValuesAreEqual() return false. so no values will be remembered, and do saveData as much as you want, but valuesHaveChanged() always notifies that there is changes in form.

[Isomorphic]

Try re-reading our previous response. Again, the values you see compared in this code do not involve the server's response.

It may also clarify things to take one of the samples and add a bit of logic to add a server-generated field. You shouldn't see the issue you have in your application. Then you can re-examine your application code to find the spot where you're updating the form values during the server turnaround.

[antanas.arvasevicius]

Oh, sorry! you are right it compares only submitted vs value which is currently in form.
But actually I'm not doing any edits on `added_at` it not exists in DynamicForm at all.
So it compares submitted value with non existent form field. I see there is a simple logic in fieldValuesAreEqual() if field do not exists then

//snippet: DataBoundComponent.js: fieldValuesAreEqual() 

    // no matter what the type, if we get this far, the field type had no custom comparison mechanism,
// so, if the values are '==', always return true
    if (value1 == value2) return true;

    // return false
    return false;

And I think Date's in js is compared by reference only and maybe references do not match, because there was a copies made using clone before submitting to server.

I'll check that, but don't you think this also maybe a case?

[Isomorphic]

fieldValuesAreEqual() does not compare Dates by reference, but by value. It handles several other cases in which the JavaScript == would not be right as well.

[antanas.arvasevicius]

Only if first parameter field is not null, in my case field do not exists in DynamicForm, so it compares only by simple ==

So I must add `added_at` field into DynamicForm.fields, and make a showIf: function () { return false; } to work that logic as you describe.
As I said I do not need field in DynamicForm. Maybe that part should check field definition with form's datasource instead of using
DynamicForm.getField() ?

                var compareVal = submittedValues[i] === undef ? this._oldValues[i] : submittedValues[i];
                var field = this.getField(i); // <-- (1) here I get null because `added_at` is not in form.
                // check whether the form item has changed since submission
                if (this.fieldValuesAreEqual(field, currentValues[i], compareVal)) { // <- this will use js == comparision

[antanas.arvasevicius]

I can confirm that then I'm adding a field in DynamicForm everything is okay, as fieldValuesAreEqual() will then compare dates by values not by references.

[Isomorphic]

Yes, in the absence of a field definition to provide type information, fieldValuesAreEqual would use "==" comparison.

However, as we keep explaining, the code you are looking at compares submitted values to current values in the form. It runs before the response from the server is even processed. So it has nothing to do with your problem with valuesHaveChanged() and your server-generated date field.

Instead, you need to look over your code and find out how you are accidentally changing form values between the moment you call saveData(), and *before* _saveDataReply() has run.

And again we'd recommend:

It may also clarify things to take one of the samples and add a bit of logic to add a server-generated field. You shouldn't see the issue you have in your application. Then you can re-examine your application code to find the spot where you're updating the form values during the server turnaround.

[antanas.arvasevicius]

Hi,
yes, I have misread the code that I've pasted to you, but it do not change the problem either.

As I said, I do not do any changes or modifications for fields before during after saveData() no changes at all.


I am just saying that if field do not exists in DynamicForm.fields then your code is not working,
"Yes, in the absence of a field definition to provide type information, fieldValuesAreEqual would use "==" comparison."

So yes, there is no field definition in DynamicField because I do not need that field "added_at" in DynamicForm, that field is definitely defined in DataSource and I don't see the point to define that field "added_at" on DynamicForm just for fix smartclient's internal logic.

So now I need to do like this

isc.DynamicForm.create({
// ...
  fields: [ 
          { name: "added_at",
             showIf: function () { return false; }
          }
  ]
});

just to fix the fieldValuesAreEqual() because without that , this function always return false; because it compares dates by reference ("==")
So if I've a "datetime" field in DataSource, your forms do not work properly (I'm calling saveData() and after success the valuesHaveChanged() returns true, whatever I do)

Because that "added_at" field by that fieldValuesAreEqual() is magically NOT EQUAL. but it was not changed because it not exist in DynamicForm, and there is no manipulations.

It's non sense, why I must define field in DynamicForm which I do not need at all, is this magic it clearly written in smartclient's docs or if it's not a bug I definitely do not understand the point to do so?

[antanas.arvasevicius]

Sorry, that this talk was so long during my misunderstanding of code first time, but
I've made a patch to fix this issue:

// ISC_DataBinding.js: _saveDataReply : function (request, response, data) {
            for (var i in data) {
                // If the value for this field is undefined in the submitted data, that probably
                // means it was stripped by the sparseUpdates logic, so we can't compare it to
                // the current value.  However, we can compare it to the corresponding member of
                // _oldValues - the fact that it was stripped by sparseUpdates means that it was
                // unchanged, so if it is different now, it has changed since we sent the update
                // to the server
                var compareVal = submittedValues[i] === undef ? this._oldValues[i] : submittedValues[i];
                var field = this.getField(i);
                if (!field) {  // (1) if field is not in DynamicForm
                    field = this.getDataSource() ? this.getDataSource().getField(i) : null; // (2) we'll try to get field definition from DS          
                }
                // check whether the form item has changed since submission
                if (this.fieldValuesAreEqual(field, currentValues[i], compareVal)) {

maybe this will clear out our minds of miscommunication :)

[Isomorphic]

Yes, an == comparison would be used by this code.

But it cannot actually cause your issue unless you are modifying the values after saveData() and before _saveDataReply(), for the simple reason we've already covered multiple times - the server data has not even been processed at the point _saveDataReply() is invoked, so there is no possible way the "added_at" field can influence what's going on.

And for that reason, it doesn't matter whether an "==" comparison or different approach is used in this particular codepath.

Note that the fact you can "fix" this by adding a field declaration is not proof of a bug - it's typical that a hack can fix a problem elsewhere in your code.

Disagree? Create a minimal, standalone test case that demonstrates that a framework bug is causing your valuesHaveChanged().

Apologies in advance, but we won't be responding to any further discussion here unless we see a test case as described. We have other, clearly real issues to address first.

[Isomorphic]

Just a quick note that your attempted patch is just another variation on adding an extra field - it alters framework behavior to work around an application-level bug.

A test case, as previously described, is still the only way to show that you've found an actual bug.

[antanas.arvasevicius]

Understand, ok, I'll make a small test case for you.

[antanas.arvasevicius]

Hi, again, here you go, a test case for bug: no need any server side:

    <script type="text/javascript">
        isc.setAutoDraw(false);

        isc.DataSource.create({
            ID: 'test',
            clientOnly: true,
            fields: [
                {
                    hidden: true,
                    primaryKey: true,
                    name: "id",
                    type: "sequence",
                    required: true
                },
                {
                    title: "Name",
                    name: "name",
                    length: 50,
                    type: "text",
                },
                {
                    title: 'added_at',
                    name: 'added_at',
                    type: 'datetime'
                }
            ],
            testData: [
                {id: 1, name: 'change me', added_at: new Date()}
            ]
        });

        isc.VLayout.create({
           autoDraw: true,
           members: [
               isc.DynamicForm.create({
                   ID: 'form',
                   dataSource: 'test',
                   useAllDataSourceFields: false,
                   fields: [
                       {name: 'name'}
                       /*,{name: 'added_at'}*/
                   ]
               }),
               isc.Button.create({
                   title: 'Save',
                   click: function () {
                       form.saveData(function () {
                           if (form.valuesHaveChanged()) {
                               isc.say('still not saved, has changed values: ' + JSON.stringify(form.valuesHaveChanged(true)));
                           } else {
                               isc.say('everything ok');
                           }
                       });
                   }
               })
           ]
        });

        form.fetchData({'id':1});

    </script>

How to proceed:

1. change name field to any value, click save button.
2. should see 'everything ok', but you can see that we still have a changes, even after save.

How to fix:

1. uncomment field in DynamicForm, /*,{name: 'added_at'}*/
2. open example, edit 'name' field value
3. click save, should see 'everything ok'

so you can see that this example do not edit 'added_at' field but as I said the code cannot get field definition so dates are compared by references ("==")
My patch can be seen in previous post, it just retrieve field declaration from bound datasource if not exist in dynamicform.

cheers:)