Feat/task group completion action

PR Checklist

[x] PR title descriptive (can be used in release notes)

Description

Adds a new template action, task: evaluate, to enable the evaluation of the completion status of a task with child tasks. This action is intended as something of an intermediary workaround: as part of a V2 task system, we will likely want to author this functionality somewhere other than in template actions, instead leveraging data actions (see Data Actions RFC) to trigger the logic automatically based on the relationships defined on the data itself.

A "task" is taken to essentially be a row in a data list that has a boolean "completed" value. A "task group" is not explicitly referred to, and should ultimately be defined outside of this action as part of the reworked task system, but this action expects the task row to be evaluated to have a column, task_child, containing the name of the data list that contains its child tasks, i.e. the list of tasks which, when all have been completed, should cause the parent task to be evaluated as completed.

The new action takes up to 2 parameters:

data_list_name, the name of the data list that contains the row corresponding to the task to evaluate
row_id (optional), the ID of the row within the data list corresponding to the task to evaluate. If not provided, every row of the data list will be evaluated

In order to evaluate multiple levels of nested "task groups", the action should be called multiple times, for example:

click | task: evaluate | data_list_name: data_list_b, row_id: b_2;
click | task: evaluate | data_list_name: data_list_a, row_id: a_1;

See debug_task_group_completion for a working example.

Git Issues

Closes #

Screenshots/Videos

Demo

The template debug_task_group_completion demonstrates the desired functionality. When the task group completion status is evaluated:

if and only if all tasks in debug_task_group_c are completed, then the row debug_task_group_b.b_2 is set to completed
- else it is set to false
if and only if all tasks in debug_task_group_b are completed, then the row debug_task_group_a.a_1 is set to completed
- else it is set to false

https://github.com/IDEMSInternational/open-app-builder/assets/64838927/c26d7b01-fd43-4460-b185-b679d867e8f0

debug_task_group_completion:

debug_task_group_a:

debug_task_group_b:

debug_task_group_c:

Passing tests

Just to clarify, by calling the action we are actually creating the hierarchy

debug_task_group_a.a_1 debug_task_group_b.b_2 debug_task_group_c

which says task_group_a has row a_1 which should be the completed status of task_group b row b_2 which in turn is the completed status of the entire task_group_c (?)

That's right.

I think defining this relationship as part of the action perhaps a bit confusing, although from what I understand you just want a very quick workaround to refer to those values?

Yes, I accept it is quite confusing – it was intended as a quick workaround, but obviously quick workarounds may end up staying in the code indefinitely so it'd be good if it wasn't too unpalatable.

I think minimally I would suggest being more explicit with the naming, and consider building each relationship one at a time. I might also consider using an _completed property on a task group to represent whether the entire group is completed. E.g. click | task: set_from_child | debug_task_group_b.b_2 : debug_task_group_c._completed click | task: set_from_child | debug_task_group_a.a_1 : debug_task_group_b._completed

I hadn't thought of separating out the actions like that, that is certainly clearer. I think I do prefer this proposed syntax, a few comments:

Our existing template action parser treats the third section (the section after the second | separator) as a list of key-value pairs that represent parameter names and their values. With this in mind, I think the third section of this new syntax could be handled differently to be clearer.
- Having .s in the param name does seem to work with the current parser, but it's worth noting how it gets interpreted: debug_task_group_b.b_2 : debug_task_group_c._completed would be parsed and added to action.params as debug_task_group_b: {b_2: debug_task_group_c._completed}
- I'm not sure about how about @task would function when used in the name of a "parameter"
"set_from_child" doesn't carry a lot of meaning as an action name, since it's the nature of task groups, as currently defined, to have a parent row and a child data list
- If we incorporate "completed" into the action name then we don't need to use it in the parameter value
  - Otherwise, it might make sense to generalise the action further, to be setting a named property of a task row (or even general data row) based on some property of the child data list. E.g. click | data: set_property | row: debug_task_group_b.b_2, property: completed, value: debug_task_group_c._completed
- If we're limiting the action to tasks, then as "completed" is the fundamental property of tasks, I think we probably don't need the completed or _completed reference

I agree with your main point that we don't need to try and define the whole nested group at once, and can build the relationship via one action at a time. The advantage of defining the nesting all at once is that it binds the task groups together so they can't be "out of sync" (which could happen if the separate actions weren't all authored to trigger at the same point, or were authored in the wrong order). But as using an action to define this relationship isn't the right approach long term, I think I agree that separate actions is clearer and more explicit, and also more likely to be extendable to the kind of authoring syntaxes we're likely to want in future. I was quite pleased with the recursive task group evaluation that I achieved with this PR, but perhaps that idea could be useful in the future implementation of task groups.

So with all that in mind, I have some proposals for versions of the new authoring syntax:

Referencing "task group"

Keeping the idea of "task group" as an entity consisting of a data list of subtasks and a task row.

click | task: evaluate_group_completion | row_reference: debug_task_group_b.b_2, task_group_data: debug_task_group_c

Or more explicit:

click | task: evaluate_group_completion | parent_data_list: debug_task_group_b, row_id: b_2, subtasks_data: debug_task_group_c
- These parameter names make sense to me because I've been working on this code, but perhaps wouldn't be so clear to authors

Generalising to "task"

Whilst we will want to think about task groups qua task groups at some point (possibly via a new flow type), if we're just defining them in an action like this rather than more fundamentally then it probably does make sense to avoid defining them at all. This is more in keeping with your proposed syntax, Chris, where we're just setting the completion status of a given task (which happens to be a task group row)

click | task: evaluate_completion | data_list: debug_task_group_b, row_id: b_2, condition: debug_task_group_c._completed
- Where debug_task_group_c._completed is interpreted as meaning that every row of debug_task_group_c is completed.

Or,

click | task: set_completion | data_list: debug_task_group_b, row_id: b_2, value: debug_task_group_c._completed

I think one of these last two options would probably my preferred syntax, but I've included the other suggestions for discussion. In this case, authoring the task completion over multiple nested task groups would look something like:

click | task: set_completion | data_list: debug_task_group_b, row_id: b_2, value: debug_task_group_c._completed;
click | task: set_completion | data_list: debug_task_group_a, row_id: a_1, value: debug_task_group_b._completed;

Next steps

After deciding on a preferred syntax, there would still be an open question of whether it's worth reworking this PR to support it, or seeing this as part of a more significant overhaul. I think it would require substantial changes to this PR, but wouldn't be too difficult to implement, maybe a day of my time. @esmeetewinkel perhaps we could discuss this tomorrow morning: is the syntax proposed in this PR description acceptable or would it be worth reworking to make the action syntax more explicit, as suggested?

Update

Me and @esmeetewinkel met and decided that the following syntax might represent the best balance of being explicit whilst not implying more general functionality that is not actually implemented (whilst we will likely want to have an action for setting the value of an arbitrary property of an arbitrary row in the dynamic data in the future, handling the current use case is more of a priority at this moment):

click | task: set_group_completion | parent_data_list_name: debug_task_group_b, row_id: b_2, child_data_list_name: debug_task_group_c;
click | task: set_group_completion | parent_data_list_name: debug_task_group_a, row_id: a_1, child_data_list_name: debug_task_group_b;

Update

Me and @esmeetewinkel met and decided that the following syntax might represent the best balance of being explicit whilst not implying more general functionality that is not actually implemented (whilst we will likely want to have an action for setting the value of an arbitrary property of an arbitrary row in the dynamic data in the future, handling the current use case is more of a priority at this moment):
click | task: set_group_completion | parent_data_list_name: debug_task_group_b, row_id: b_2, child_data_list_name: debug_task_group_c;
click | task: set_group_completion | parent_data_list_name: debug_task_group_a, row_id: a_1, child_data_list_name: d

Not to throw a complete spanner in the works, but could it also be worth just using a column in the data_list to define the relationships, i.e.

top_task_list	id	task_child	completed
task_1	child_list_1
task_2	child_list_2

child_list_1	id	task_child	completed
child_task_1	granchild_list_1
child_task_2	granchild_list_2

granchild_list_1	id	completed
grandchild_task_1

In doing so any parent task that reference a task_child list can then just track the completion of that child (recursively as deeper nested children added). We could still keep the action to force evaluation of all child tasks but omit the need to define the relationship

Not to throw a complete spanner in the works, but could it also be worth just using a column in the data_list to define the relationships

It could be a useful spanner – I think I was wary of proposing any data_list-level changes because I thought it could conflict with how we want to eventually implement a more substantial V2 tasks system. E.g., is it right to specify the child on the parent and not the other way round?

But potentially this could be a first step towards Tasks V2 rather than in conflict with it.

With the additional data list columns as you've suggested above, is the idea that the task group completion evaluation action would look something like the following?

click | task: set_completion_from_child | data_list_name: debug_task_group_b, row_id: b_2;
click | task: set_completion_from_child | data_list_name: debug_task_group_a, row_id: a_1

Here the column task_child data list column would hold special significance (as completed already does). Its name could be handed to the above action as an additional parameter, but that complexity is probably best avoided.

It could be a useful spanner – I think I was wary of proposing any data_list-level changes because I thought it could conflict with how we want to eventually implement a more substantial V2 tasks system. E.g., is it right to specify the child on the parent and not the other way round?

The main idea is that the code would not impose anything at the data_list level itself, just more of an implicit expectation checked from within the task service - indeed as a bridge towards v2. The reason for denoting child on parent is that it could be easily inlined into the existing parent data_list (if referencing the parent from the child we'd either need to include it in the contents sheet somehow or repeat the same entry on every row, whereas the parent row can just make a single reference to the child list).

With the additional data list columns as you've suggested above, is the idea that the task group completion evaluation action would look something like the following?
click | task: set_completion_from_child | data_list_name: debug_task_group_b, row_id: b_2;
click | task: set_completion_from_child | data_list_name: debug_task_group_a, row_id: a_1

Pretty much, although I might change the keyword to click | task: evaluate and also allow omission of row_id if wanting to just evaluate all tasks with child references

Here the column task_child data list column would hold special significance (as completed already does). Its name could be handed to the above action as an additional parameter, but that complexity is probably best avoided.

Yes it would essentially be an optional column used to extend data_lists for support within task service. We no longer need to include direct references within the action code as the data linkages are already setup from the list itself

IDEMSInternational / open-app-builder