IDEMSInternational / open-app-builder

PLH App Frontend
GNU General Public License v3.0
5 stars 24 forks source link

Feat!: standalone task group progress bar component #2176

Closed jfmcquade closed 6 months ago

jfmcquade commented 7 months ago

PR Checklist

Description

Fleshes out the standalone version of the task-progress-bar component (i.e. the component as declared in a template as opposed to the version instantiated implicitly within the task-card component), to be used in #2150.

The component takes the following params:

Task group data lists can store their completion status in one of two ways:

  1. A "completed" column, with values updated dynamically
    • This is a new option that should mean that a page refresh is not required in order for the progress status to update
  2. A "completed_field" column, referencing a field that stores the completion status
    • This is how we have hitherto tracked task completion

See below for demos of each.

Breaking changes

The task service has been updated to extract some hardcoded variables out to the deployment config level, namely highlightedTaskField, with default value "_task_highlighted_group_id" and taskGroupsListName, with default value "workshop_tasks". Additionally, deployments can set the value of config.app_config.TASKS.enabled. The default is false, meaning that deployments which do not make use of the task feature will, without changing their config, no longer get the ubiquitous error message workshop_tasks not found.

Any deployment that does use the tasks feature that targets this code release must update its deployment config to include the following: config.app_config.TASKS.enabled = true

In order solve the issue brought up in this mattermost thread, after merge, the WASH deployment repo should be updated to point to latest code release, and simultaneously the WASH deployment config should be updated to include the following (accomplishing the equivalent of this commit): config.app_config.TASKS.taskGroupsListName = "module_tasks"

Testing

Explore the functionality of the templates comp_task_prog_bar_dynamic and comp_task_progress_bar and assess the authoring and proposed authoring syntaxes.

I've also added a new template, feature_task_group, with the aim of allowing us to functionally test that the task group functionality including setting a highlighted task etc (as described in this future RFC) is working as expected. It would be good to experiment with this template a little.

See screenshots/videos below for demonstrations of functionality.

Git Issues

Closes #2150

Screenshots/Videos

Case 1: dynamic data list, completion status stored in completed column

This configuration uses the dynamic data feature, and a reload is not required for the progress bar to reflect the state of the source data. The completed value of each subtask should be set using the set_item action. It may be that we still need a separate completed_field column and associated completed fields in order to store the user data.

comp_task_prog_bar_dynamic template

comp_dynamic

comp_task_prog_bar_dyn_data data list

data_dynamic

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/1b8ae2e1-1fce-46eb-9514-f04ed15002de

Case 2: static data list, completion status stored in fields

In this case, a force_reload action is required in order for the changes to propagate.

comp_task_progress_bar template

comp

comp_task_progress_bar_data data list

data

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/0911f489-fc07-4e9b-9988-0913a60cf434

Task Group functionality

feature_task_group

https://github.com/IDEMSInternational/parenting-app-ui/assets/64838927/d026700d-c7d2-4a94-baf0-d1bfd566b7d0

jfmcquade commented 7 months ago

Thanks @chrismclarke. I'm still yet to write unit tests, but I think I've addressed your other comments

chrismclarke commented 7 months ago

Thanks @jfmcquade I've gone through each of the comments and I think looking good (few minor clarifications requested where I found myself a bit confused).

I'll wait until there are some tests in to do a second review focused more on the data flow and operation side. Are you planning on adding these as part of this PR or as a follow-up?

Thanks also for adding the RFC doc and example templates also, useful to have for sure and makes sense that we migrate more of the refactoring tasks there

esmeetewinkel commented 7 months ago

When testing feature_task_group, my _task_highlighted_group_id is set to blank whenever I toggle any task completion (potentially the same as issue highlighted above). I tried resetting the app / empty cache and hard reload / clear storage, but the issue persists

image
jfmcquade commented 7 months ago

@chrismclarke I am planning on including unit tests in this PR, but I am still having issues with running tests, we can discuss tomorrow if I haven't been able to resolve.

Thanks for the review, @esmeetewinkel, my response is below:

show_text: Show the text associated with progress bar, e.g. "8 sections". Default false Assuming that in practice this means it doesn't show i.e. default = blank

That is correct

config.app_config.TASKS.enabled = "false" (the default value is true)

I think it would make more sense for the default to be false, and a deployment to opt-in to using features rather than opt-out. In that way someone trying to make a simple app, with the simplest possible config, doesn't need to know what tasks are and make sure to disable them.

I agree, I have update the PR with that change. NB, that now means that the PR contains breaking changes, I have updated the description accordingly.

When testing feature_task_group, my _task_highlighted_group_id is set to blank whenever I toggle any task completion (potentially the same as issue highlighted above). I tried resetting the app / empty cache and hard reload / clear storage, but the issue persists

Ah, I should have predicted this. Because the debug templates are using a non-default task group for config.app_config.TASKS.taskGroupsListName (namely feat_task_groups), the feature requires an update to the deployment config. I've included those updates in this PR on the debug repo. After merging, syncing, re-setting your deployment, and restarting your local app, you should find that the feature behaves as expected.

esmeetewinkel commented 7 months ago

I agree, I have update the PR with that change. NB, that now means that the PR contains breaking changes, I have updated the description accordingly.

  1. I tested that indeed on a deployment without tasks (plh_facilitator_mx) the error "workshop_tasks not found" has disappeared.
  2. I also updated the configs of the two deployments that use tasks:

    1. debug (post merge of config update PR) feature_task_group working as expected (only issue found that a refresh is needed for the active deployment to update correctly when uncompleting tasks, but I don't think addressing this is essential as the only use case for this is the WASH deployment). comp_task_prog_bar_dynamic seems to have stopped working at all (apart from the button click animation nothing happens when I press the buttons?) comp_task_progress_bar works as expected.

    2. plh_teens: When running this deployment on this PR code branch (with or without deployment config update) I'm getting the error

      image
    3. wash Can only test properly after PR was merged (when testing on relevant content & code branch it appears that highlighted task group is not set).

jfmcquade commented 7 months ago

comp_task_prog_bar_dynamic seems to have stopped working at all (apart from the button click animation nothing happens when I press the buttons?)

It seems this was due to cell D9 of comp_task_prog_bar_dynamic. You pointed out a discrepancy in the way I'd authored the set_item action, and I made a change to bring them into alignment. However, I opted for the wrong syntax: it turns the set_item action must be authored with a pipe | after the action name, e.g. click | set_item | completed:!@item.completed. The template should be working now if you sync your debug sheets.

  1. plh_teens: When running this deployment on this PR code branch (with or without deployment config update) I'm getting the error

Good thing you tested that deployment. The issue was arising because the task progress bars in the task cards on the homepage were erroneously trying to use the dynamic data feature. I've fixed this in the latest commit.

wash Can only test properly after PR was merged (when testing on relevant content & code branch it appears that highlighted task group is not set).

You should be able to test the wash deployment by manually adding these 2 lines to the deployment config (and re-setting the deployment):

config.app_config.TASKS.enabled = true;
config.app_config.TASKS.taskGroupsListName = "module_tasks";

When I do this locally, the highlighted task seems to be set correctly:

Screenshot 2024-01-22 at 17 28 59
esmeetewinkel commented 6 months ago

Merging this since feature is needed for content deployments now, @jfmcquade will work on tests as a follow-up PR