5710 add breadcrumbs to tasks lists #7757
Conversation
|
@elvisdorkenoo is this ready to review or is it waiting on #7732? (Just not exactly sure what do to when assigned to a draft PR....) 😄 |
leave draft reviews 😅. |
elvisdorkenoo
changed the base branch from
master
to
5697-update-breadcrumbs-displayed-levels
…:medic/cht-core into 5710-add-breadcrumbs-to-tasks-lists
| const hydratedTasks = await this.hydrateEmissions(taskDocs) || []; | ||
| const subjects = await this.getLineagesFromTaskDocs(hydratedTasks); | ||
| if (subjects?.size) { | ||
| for (const task of hydratedTasks) { |
There was a problem hiding this comment.
In theory the this.currentLevel promise should resolve immediately the following iterations and the performance impact of awaiting for every task shouldn't be big....
Another option is to await 1 time first and send over the currentLevel to the getTaskLineage, something like this:
... other code...
const subjects = await this.getLineagesFromTaskDocs(hydratedTasks);
if (subjects?.size) {
const userLineageLevel = await this.currentLevel;
hydratedTasks.forEach(task => task.lineage = this.getTaskLineage(subjects, task, userLineageLevel));
}
...continue code...
And the cleanAndRemoveCurrentLineage wont' be async anymore.
@jkuester @elvisdorkenoo what do you think?
There was a problem hiding this comment.
Yeah I like the second option, it would make me passing over userLineageLevel parameter, but it's fine.
…:medic/cht-core into 5710-add-breadcrumbs-to-tasks-lists
| if (!subjects?.size) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Minor, but this check is unnecessary since you have done it already above.
| if (subjects?.size) { | ||
| const userLineageLevel = await this.currentLevel; | ||
| for (const task of hydratedTasks) { | ||
| task.lineage = await this.getTaskLineage(subjects, task, userLineageLevel); |
There was a problem hiding this comment.
Pretty sure we don't need to await this call.
There was a problem hiding this comment.
and you can use for each as in the sample here.
Also please remove the async in cleanAndRemoveCurrentLineage since you don't need it anymore
| const dueDate = moment(emission.dueDate, 'YYYY-MM-DD'); | ||
| emission.date = new Date(dueDate.valueOf()); | ||
| emission.overdue = dueDate.isBefore(moment()); | ||
| emission.owner = taskDoc.owner; | ||
|
|
||
| emission.forId = taskDoc.forId ? taskDoc.forId : taskDoc.owner; |
There was a problem hiding this comment.
Are we sure that we are not going to inadvertently trigger some additional logic by setting the forId to owner?
I am not sure exactly what the difference is between the two fields anyway, but I am a little worried that this could have unintended consequences...
There was a problem hiding this comment.
@elvisdorkenoo - To play safe, you can:
- Line 135 has
emission.owneralready defined and available. - Line 136 - Add
emission.forIdand don't default it totask.ownerbecause it's already expose inemission.owner. - In your
getLineagesFromTaskDocscheck for both properties:const ids = [...new Set(taskDocs.map(task => task.forId || task.owner))];
There was a problem hiding this comment.
task => task.forId || task.owner is actually what I've before changing it this way when I got the confirmation that when it's set, task.forId always equals task.owner. But yeah let's play it safe.
…:medic/cht-core into 5710-add-breadcrumbs-to-tasks-lists
|
@elvisdorkenoo is it ready for review again? |
…:medic/cht-core into 5710-add-breadcrumbs-to-tasks-lists
…:medic/cht-core into 5710-add-breadcrumbs-to-tasks-lists
|
@elvisdorkenoo Looks like the e2e is failing consistently in the same place, can you please have a look? |
I tried to take a look, even reverted my changes, it is also failing when I run that e2e locally, it looks mostly a timeout issue. |
elvisdorkenoo
changed the base branch from
5697-update-breadcrumbs-displayed-levels
to
master
…dcrumbs-to-tasks-lists
…dcrumbs-to-tasks-lists
This commit adds a third line of text to tasks to display the additional contextual information. This new line of text should display breadcrumbs showing where the person/place the task is about belongs to. The functionality should match how we display breadcrumbs elsewhere on the app.
Description
This PR adds a third line of text to tasks to display the additional contextual information.
This new line of text should display breadcrumbs showing where the person/place the task is about belongs to.
The functionality should match how we display breadcrumbs elsewhere on the app. Note that we are updating breadcrumbs across the app so that they do not list out the level the user themselves belongs to repetitively (i.e. CHW Janet will not see “CHW Janet’s Area” repeated all over the place, ticket #5697).
The text should be styled just like breadcrumbs elsewhere on the app (Noto Sans 14px).
#5710
Code review checklist
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.