Refresh tasks lists after a task has been updated #3068

ThatHurleyGuy · 2023-02-06T08:25:03Z

ThatHurleyGuy commented

2023-02-06 08:25:03 +00:00

Repro Steps

Create a repeating task that repeats every week
Filter the task list by due date To of now+12h
Mark the repeating task as completed

Expected

The task is completed and is removed from the list since the next due date is greater than now+12h

Observed Behavior

The task stays in the list, but is just updated to show the next due date. The filters aren't reapplied

Proposed Solution

When a task is updated, reload the list. This will ensure that any filters (due date, search, etc) are properly applied.

## Repro Steps 1. Create a repeating task that repeats every week 2. Filter the task list by due date `To` of `now+12h` 2. Mark the repeating task as completed ## Expected The task is completed and is removed from the list since the next due date is greater than `now+12h` ## Observed Behavior The task stays in the list, but is just updated to show the next due date. The filters aren't reapplied ## Proposed Solution When a task is updated, reload the list. This will ensure that any filters (due date, search, etc) are properly applied.

ThatHurleyGuy added 1 commit 2023-02-06 08:25:05 +00:00

continuous-integration/drone/pr Build is passing Details

984bad6f38 Refresh tasks lists after a task has been updated

ThatHurleyGuy reviewed 2023-02-06 08:29:00 +00:00

src/views/list/ListList.vue

						
				@ -272,1 +272,4 @@

					sortTasks(tasks.value)

					// After a task has been updated, reload tasks to ensure filters are applied to any changes

					loadTasks()

ThatHurleyGuy commented

2023-02-06 08:29:00 +00:00

An extra API call isn't ideal, but I would say it's worth the better user experience. Could try to make it smarter and only reload if the updated tasks changed a field that is currently being filtered on (e.g. if the task title is updated and the only active filter is on due date, don't bother reloading), but that seemed like it would be a little overly complicated.

Also, I just put this in ListList since it was the easiest spot to shim in. Might make more sense to have some sort of watch in useTaskList.ts to see if a task is updated and reloading there. But not being super familiar with vue, I wasn't sure the best way to tackle that sort of watch without getting into infinite loop trouble. Happy to try and work that out if you'd prefer that and are willing to give me a few pointers.

An extra API call isn't ideal, but I would say it's worth the better user experience. Could try to make it smarter and only reload if the updated tasks changed a field that is currently being filtered on (e.g. if the task title is updated and the only active filter is on due date, don't bother reloading), but that seemed like it would be a little overly complicated. Also, I just put this in `ListList` since it was the easiest spot to shim in. Might make more sense to have some sort of `watch` in `useTaskList.ts` to see if a task is updated and reloading there. But not being super familiar with vue, I wasn't sure the best way to tackle that sort of `watch` without getting into infinite loop trouble. Happy to try and work that out if you'd prefer that and are willing to give me a few pointers.

dpschen commented

2023-02-07 12:51:47 +00:00

It's not perfect, but I think it's a good position for this kind of fix. We have some updates that need to get rebased and merged that should improve this overall mechanism.

Could you prefix the comment with a FIXME: ?
Then I'm happy to merge it :)

It's not perfect, but I think it's a good position for this kind of fix. We have some updates that need to get rebased and merged that should improve this overall mechanism. Could you prefix the comment with a `FIXME: `? Then I'm happy to merge it :)

konrad commented

2023-02-07 13:09:26 +00:00

I think this is not great for UX, as it shows a full loading spinner every time the tasks are refreshed. A better idea would be to only do this if tasks changed, where any of the dates changed. (that would be a lot more complicated though)

👍 1

konrad commented

2023-02-07 13:09:48 +00:00

Or maybe only if a task was changed which is repeating 🤔

dpschen commented

2023-02-07 13:40:17 +00:00

Probably it would then make sense to rebase that old branch.

ThatHurleyGuy commented

2023-02-07 18:54:51 +00:00

Really good catch on the loading spinner. Since I'm hosting it locally and the backend you built is so fast, I never even noticed that :)

A better idea would be to only do this if tasks changed, where any of the dates changed

Or maybe only if a task was changed which is repeating 🤔

Yeah we could make this a more targeted check, but since you can filter on assignee/labels/taskname/etc I think the check basically turns into "Reload the list if any filterable field on the task changed, which is basically any field". So maybe the better tactic would be "check for any active filters, and reload if the task updated a field covered by one of those filters", which is even more complicated :( My only use case is for repeating tasks though, so could just ignore the other fields and only update if it's a recurring task

Does seem like the spirit of that other PR to move more of the filtering into the client would be a good solution for this though

Really good catch on the loading spinner. Since I'm hosting it locally and the backend you built is so fast, I never even noticed that :) > A better idea would be to only do this if tasks changed, where any of the dates changed > Or maybe only if a task was changed which is repeating 🤔 Yeah we could make this a more targeted check, but since you can filter on assignee/labels/taskname/etc I think the check basically turns into "Reload the list if any filterable field on the task changed, which is basically any field". So maybe the better tactic would be "check for any active filters, and reload if the task updated a field covered by one of those filters", which is even more complicated :( My only use case is for repeating tasks though, so could just ignore the other fields and only update if it's a recurring task Does seem like the spirit of that [other PR](https://kolaente.dev/vikunja/frontend/pulls/2474/files) to move more of the filtering into the client would be a good solution for this though

konrad commented

2023-02-14 10:52:30 +00:00

I think the check basically turns into "Reload the list if any filterable field on the task changed, which is basically any field"

At the end of the day, that's what we should do. It should avoid doing that if the default filters are active though.

Does seem like the spirit of that other PR to move more of the filtering into the client would be a good solution for this though

That's on hold as we plan to change how filters work in general.

> I think the check basically turns into "Reload the list if any filterable field on the task changed, which is basically any field" At the end of the day, that's what we should do. It should avoid doing that if the default filters are active though. > Does seem like the spirit of that other PR to move more of the filtering into the client would be a good solution for this though That's on hold as we plan to change how filters work in general.

👍 2

ThatHurleyGuy commented

2023-02-17 05:09:17 +00:00

Gotcha, that all makes sense. Thanks for the detail!

If this little bug isn't bothering anyone else and there's a plan to rework filters in general, I can just keep my hack on a separate fork and we can close this PR

I'll keep an eye on updates to the codebase, but in the meantime if you ever want an extra set of hands on anything feel free to @ me and I would be happy to try and help out

Gotcha, that all makes sense. Thanks for the detail! If this little bug isn't bothering anyone else and there's a plan to rework filters in general, I can just keep my hack on a separate fork and we can close this PR I'll keep an eye on updates to the codebase, but in the meantime if you ever want an extra set of hands on anything feel free to @ me and I would be happy to try and help out

❤️ 1

konrad commented

2023-02-17 17:30:52 +00:00

Great, will do!

frederick commented

2023-02-06 10:14:22 +00:00

Hi ThatHurleyGuy!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3068-reload-tasks-updates--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi ThatHurleyGuy! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3068-reload-tasks-updates--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

ThatHurleyGuy closed this pull request

2023-02-17 05:09:23 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers