Slow calculate can overwrite later entered data

[torbjokv]

Describe the bug

When calculate is slow, the user may experience loosing entered data when calculate returns its answer.

To Reproduce

Steps to reproduce the behavior:

1. Enter a address and postal code. Leave acative focus in the postal code.
2. Click directly on the next yes option. (it will not register) This step will trigger the slow calculate and validation (both).
3. Very fast, try to click again (it will register)
4. Observe that your registered yes answer disappear

Expected behavior

The click should register right away and not get overridden, or the calculate should lock the GUI in a predictable manner while calculate/validation is running.

In my use case it could maybe be useful with a button (with loading spinner) to trigger the calculate so that the user understands there are external requests they have to wait for.

Screenshots

Additional info

Add any other relevant context info about the problem here. For example OS (Windows, MacOS, iOS), browser (Chrome, Firefox, Safari), device (iPhone, laptop), screen-size, etc.

app: dat/innkvartering

[TheTechArch]

First figure out how this should behave

[nkylstad]

I think we have to lock the GUI while we wait for the calculation. Result from server-calculation will always be the "correct" result, and to make sure we don't then overwrite values in the GUI that have been entered in the meantime, we should prevent user from making changes while waiting for the calculation. @lorang92 @Febakke do you agree?

[altinnadmin]

This is a "dangerous path" and a slippery slope. Locking the GUI gives a bad user experience, and gives us no incentive to improve the performance.

There must be a better way.

[lorang92]

@altinnadmin just a note that this particular case is caused by a slow external api-call.

[TheTechArch]

If we where able to just update one field with the response we might solve this, but then we would need some "mode settings" that knows that the response would be single field response. And the response only would be mapped to the target field/fields.

Other would be to keep track of all values changed after post and merge when you get the full form in response. Could be very complex.

What about a button to "get value"

[torbjokv]

I like the idea with the "get value" button or something like that.

Could one alternative be to do selective locking defined in the layout json?

{
  "id": "innkvarteringsadresse-gateadresse",
  "type": "Input",
  "textResourceBindings": {
    "title": "OppholdstedadresseLedetekst",
    "description": "OppholdstedadresseBeskrivelse"
  },
  "dataModelBindings": {
    "simpleBinding": "Innhold.Innkvarteringsadresse.Gateadresse"
  },
  "required": true,
  "readOnly": false,
################ something like this
  "calculationLock": [ 
    "Innhold.Innkvarteringsadresse.Poststed",
    "Innhold.Innkvarteringsadresse.Kommune",
    "Innhold.Innkvarteringsadresse.Gardsnummer",
    "Innhold.Innkvarteringsadresse.Bruksnummer"
  ],
################
  "triggers": [
    "validation"
  ]
},

Each calculate(save) would only be allowed to change these values.

Edit: this would still need some single field update as TheTechArch suggested.

[altinnadmin]

@altinnadmin just a note that this particular case is caused by a slow external api-call.

Yup, in real life this could be cause by a lot of things, like slow network, complex calculations, slow code, many external calls, etc.

@TheTechArch Yes, those three are the possible solutions I was contemplating initially as well...

How about something in between? Could we return just the changed elements fra the calculations and update the react state with those? Would lead to less data being returned, less javascript that needs to run, and for most cases (including this) it will "just work" without defining locks.

Will that be possible?

[nkylstad]

We could probably do something like that, yes. Or at least a list of the fields to update in addition to the form data, that should be quite easy, and could be defined by the app developer in the code. That would also solve the issue of having multiple fields updated from a calculation triggered by a single field. If the user has to click "get values" for all fields that are updated, that is probably not the best user experience either.

[TheTechArch]

I guess the most difficult areas is where there is added complex nodes based on input (eg new person with all info is added based on ssn). Would that be difficult to merge @nkylstad

[altinnadmin]

Wouldn't a case like that require clicking an "Add" button or something?

[nkylstad]

In any case, as long as we have a list of which fields are to be updated, I don't think it should be a problem. Form data in frontend is in a flat structure, we just need the data model fields to know what to update.

[torbjokv]

Could we return just the changed elements fra the calculations and update the react state with those? Would lead to less data being returned, less javascript that needs to run, and for most cases (including this) it will "just work" without defining locks.

We still may want to lock stuff since the user may edit fields that are changed in calculate while waiting for the calculate response?

Edit: but in my case no, so good enough for me ;-)

[altinnadmin]

We still may want to lock stuff since the user may edit fields that are changed in calculate while waiting for the calculate response?

Wouldn't that be a "bad" form design? Having a non-readonly field that is calculated, located right after a field that is triggering a server-side calculation for that field, seems like asking for trouble? 😊 Anyway, is something that can be added later if a need arises.

[altinnadmin]

Seems like we have a plan? All onboard? 😄

[TheTechArch]

The question is then, Are we suddenly making a specialized API only for frontend? @altinnadmin

[altinnadmin]

The question is then, Are we suddenly making a specialized API only for frontend? @altinnadmin

The only thing we're changing is making it possible to also return a list of elements that is changed by the calculation. Couldn't that be relevant for any API client?

[TheTechArch]

Ok. So same API, but with some specific parameter we can return fields insteads of dataElement. We would need to make some code to compare objects. So we know which fields that is changed. https://github.com/Altinn/altinn-studio/blob/master/src/Altinn.Apps/AppTemplates/AspNet/Altinn.App.Api/Controllers/DataController.cs#L554

[nkylstad]

Ok. So same API, but with some specific parameter we can return fields insteads of dataElement. We would need to make some code to compare objects. So we know which fields that is changed. https://github.com/Altinn/altinn-studio/blob/master/src/Altinn.Apps/AppTemplates/AspNet/Altinn.App.Api/Controllers/DataController.cs#L554

Could we make that up to the app developer to populate such a list?

[altinnadmin]

Could we make that up to the app developer to populate such a list?

Yes, that was the solution I thought we had landed on. Optional, of course, but a tool that can be used to avoid the kind of problem that this issue describes.

[altinnadmin]

But of course, if it's super-easy to do this automatically it's worth considering.

[TheTechArch]

Ok. So same API, but with some specific parameter we can return fields insteads of dataElement. We would need to make some code to compare objects. So we know which fields that is changed. https://github.com/Altinn/altinn-studio/blob/master/src/Altinn.Apps/AppTemplates/AspNet/Altinn.App.Api/Controllers/DataController.cs#L554

Could we make that up to the app developer to populate such a list?

Yes we could but we would then need to change the signature of that method, There would possible be scenarios where this is prefered over automatic if they want to hide changes to the clients. (hidden fields used for other stuff)

[acn-sbuad]

digdir/godkjenn-bruksvilkår should be upgraded if the fix includes changes to app backend.

[nkylstad]

@jeevananthank Updated API endpoint (DataController PUT) can now be tested, and should return a new property ChangedFields along with data element when a calculation has changed the form data.

Frontend is not ready for test yet, test need for frontend will just be regression testing, as well as re-test of scenario described in bug.

[jeevananthank]

Tested with an app having nuget 4.1.0-alpha and a logic in CalculationHandler.cs, and could see that PUT update form data api calls the calculation and updates the data and changedFields parameter in the response with response code 303.

A subsequent call to GET form data also runs the logic and updated the formdata again, not sure if the get call should be running the logic defined in calculationHandler.cs @nkylstad

app: ttd/level1-app

[nkylstad]

@jeevananthank Frontend is now updated (pending QA), and GET call should no longer run when the response is 303 with a dict of the updated fields. If ChangedFields property is missing from response, the GET call will be made.

[jeevananthank]

There are two issues seen with dat/innkvartering app

1. with the steps described in the issue, when the focus is active on postal code and when one click on yes radio button, the value is registered but the value from the calculation is not updated.

2. Enter postal code -> tab -> click on yes radio button -> the radio button is selected after the slow calculation is complete. @nkylstad

This bug on overwrite of values is not experienced anymore.

[nkylstad]

There are two issues seen with dat/innkvartering app

1. with the steps described in the issue, when the focus is active on postal code and when one click on yes radio button, the value is registered but the value from the calculation is not updated.
2. Enter postal code -> tab -> click on yes radio button -> the radio button is selected after the slow calculation is complete. @nkylstad

This bug on overwrite of values is not experienced anymore.

The original issue of overwriting data is solved. PUT of form data is updated to automatically return any changed fields in the response, and frontend updates these individually regardless of other changes to the form. This also avoids an extra calculate-operation that is triggered in the GET operation that was previously used to get the updated data. Closing this issue. Following up the issues from the referenced comment above in Altinn/app-frontend-react#208.