valtimo-platform / valtimo-issues

1 stars 0 forks source link

Intermediate save on User tasks #1

Closed Tom-Ritense closed 4 months ago

Tom-Ritense commented 4 months ago

Why? When filling out a Usertask form, the user wants to save their changes and complete the task later without losing any data.

Background Building forms can result in large sets of fields, making it difficult to complete all inputs in one go. Therefore, users may need to view and fill in fields multiple times. To support this behavior, changes between editing sessions need to be stored intermittently.

Conditions In a custom implementation, a solution already exists to store submission data. It would be wise to replicate most of that code and integrate it into the core. Users can trigger the save function through a dedicated button (e.g., labeled "Intermediate Save") at the bottom of the screen.

ThomasMinkeRitense commented 4 months ago

@Tom-Ritense is the submission task specific or user-specific? When you hit the 'intermediate save' button, do I see your data when I open the same task?

@peetritense any thoughts?

Tom-Ritense commented 4 months ago

I would suggest that a user task can be opened by multiple people and therefore share the intermediate save results. Discussions with TU Delft confirmed this is desired.

ThomasMinkeRitense commented 4 months ago

Do we want to show the user who has saved the task before? So I can see you filled in half of the form fields this morning?

peetritense commented 4 months ago

As for the name of the save button, "Save current progress" would be the better option.

Regarding the data shown when someone else hit the intermediate save button: If a user is allowed to see/execute the task, they should be included in the intermediate save results.

peetritense commented 4 months ago

Do we want to show the user who has saved the task before? So I can see you filled in half of the form fields this morning?

A "last edited/opened by" would be a helpful addition. It would also help to direct questions about filled fields to the appropriate person.

Tom-Ritense commented 4 months ago

I would suggest that a user task can be opened by multiple people and therefore share the intermediate save results. Discussions with TU Delft confirmed this is desired.

After looking into ValueResolver application there are scenarios where a value can be retrieved and filtered by the user permission level. That would then possibly lead to data sharing between users with different permissions levels via this intermediate save. @ThomasMinkeRitense so should we use the user specific approach then to avoid that?

ThomasMinkeRitense commented 4 months ago

I would suggest that a user task can be opened by multiple people and therefore share the intermediate save results. Discussions with TU Delft confirmed this is desired.

After looking into ValueResolver application there are scenarios where a value can be retrieved and filtered by the user permission level. That would then possibly lead to data sharing between users with different permissions levels via this intermediate save. @ThomasMinkeRitense so should we use the user specific approach then to avoid that?

If I remember correctly the access control framework only checks the first request. Every successive request will be executed without permissions check. That would mean if a user has access to the task, they have access to all the data that is being prefilled, and unauthorized data sharing is not possible. @marijnritense is that correct?

Tom-Ritense commented 4 months ago

@ThomasMinkeRitense Yes there are pbac permissions checks for each task but more fine grained filtering is also applied. Let's say user A may only see 1,2,3 and user B may only see 4,5,6. If user B loads the form and populates the form with 4,5,6 and hits intermediate save. Then when user A opens the form is sees data it should not see. The data will be overruled by the values in the intermediate save. Unless you make it user specific.

laurens-ritense commented 4 months ago

@ThomasMinkeRitense I have spoken with TU Delft, and they have said that if someone has the permission(s) to view a task they can see all data in that task and no further filtering takes place.

ThomasMinkeRitense commented 4 months ago

@Tom-Ritense the point is, we don't filter 1 to 6 if you have access to the task. If there are 6 fields in the task, you can retrieve all of them if you have access to the task.

laurens-ritense commented 4 months ago

@ThomasMinkeRitense I spoke with Tom we are on the same page. This was originally a concern of mine.

petervanmanenritense commented 4 months ago

In my opinion user tasks should be handled by one user; and a midway stored user task should not be shared between users. If thats a requirement in my opinion you're misusing usertasks. Either you have some sort of proces within one usertask or your usertask is way too long. So I'd suggest keep it simple and dont share intermediate saved tasks between users

laurens-ritense commented 4 months ago

@petervanmanenritense Two points

  1. There is one task Instance, one prefill, one submission and one permission, so I would say there is only one intermediate save.
  2. TU Delft really wants to use this feature to enable collaboration.
petervanmanenritense commented 4 months ago

@laurens-ritense

  1. Ok
  2. If its a form of collaboration that appears to be something like a review process, so maybe we should model the process instead of "hiding" it in a usertask

Risks I see with intermediate saves stored between users:

laurens-ritense commented 4 months ago

@petervanmanenritense We have discussed data leakage and it is a non-issue.

ThomasMinkeRitense commented 4 months ago

@petervanmanenritense Thanks for the feedback, I think these are points we should consider.

  1. Complexity: I think sharing data doesn't add more complexity. Keeping a user-specific 'version' of the data might even be more complex to build
  2. Concurrency problems: How would this look like if users 'share' the data?
  3. Auditing can be done. We have events for completing a task, we can add events for the intermediate save. @Tom-Ritense please include that.
  4. As Laurens mentioned, data leakage doesn't seem to be an issue.
petervanmanenritense commented 4 months ago
  1. For me the complexity is in the UI for the casemanager. When a casemanager opens a usertask that was saved by someone else does it directly open the saved version or a new version? Does it open at the beginning or where save was saved? Can you clear it?
  2. Concurrency: what happens
    • if 2 casemanagers start the usertask at the same time and save it?
    • if 2 casemanagers continue with a saved usertask?
    • if 1 casemanager completes and another casemanager intermediate saves?

I would suggest to create either a feature toggle or configuration per usertask to enable/disable this

Tom-Ritense commented 4 months ago
  1. When reopening an intermediate saved form, it displays the last saved values. However, there is no reset button to revert to the default data as it appears the first time. I would like to add a reset button so users can return to the default state. @peetritense, are we adding two buttons: one for saving and another for resetting?

  2. Last one wins: If User Task A is closed by Person A, Person B cannot complete it, resulting in an error.

  3. Will add the auditing of the intermediate save action.

  4. No issue.

  5. A feature toggle or configuration per usertask to enable/disable the sharing option. @ThomasMinkeRitense what do you think?

peetritense commented 4 months ago

We should indeed have a "clear/reset fields" button if they can be pre-filled by another user. This button should however not be on the same hierarchy level as the "save progress" and "next" buttons.

Regarding the concurrency: Would it be possible to disable opening a task when another user has the task window already open? (like a in-editing status). This would prevent data conflicts. If the task is disabled it would be nice to show a "user X is currently working on this task" tooltip when hovering

ThomasMinkeRitense commented 4 months ago

@Tom-Ritense regarding the feature toggle: I'd say one application-level toggle to enable or disable the intermediate save.

ThomasMinkeRitense commented 4 months ago

@peetritense the question with 'locking' task is when the lock should be removed, e.g. when the user closes the browser window.

peetritense commented 4 months ago

@peetritense the question with 'locking' task is when the lock should be removed, e.g. when the user closes the browser window.

Preferably when the task modal is closed (either with or without an intermediate save). This would eliminate the issue of 2 people doing the same thing, but create the issue of someone having opened a task and then forget to close the modal. We could look into a "force takeover" functionality that becomes available after a couple of minutes of inactivity perhaps.

laurens-ritense commented 4 months ago

@peetritense Would a confirm dialog for the “reset” button be satisfactory?

peetritense commented 4 months ago

@peetritense Would a confirm dialog for the “reset” button be satisfactory?

A confirmation dialog is a must yes, otherwise you could lose hours worth of work with the click of only one button.

Tom-Ritense commented 4 months ago

Decisions summary:

Out of scope: