feat: add repeat subtasks for repeating tasks #427 #2659
Conversation
StartAGarden
changed the title
|
@johannesjo Looking forward to your review! I was struggling a lot with rxjs and observables, this project is probably perfect for getting better in using them... There are some questions in comments in there, maybe you can answer them? Also if there are structural things that are missing or that could be optimized please let me know, I would love to improve the code. |
| ), | ||
| ); | ||
|
|
||
| removeConfigIdFromTaskArchiveTasks$: any = createEffect( | ||
| () => | ||
| this._actions$.pipe( | ||
| ofType(deleteTaskRepeatCfg), | ||
| tap(({ id }) => { | ||
| tap(async ({ id }) => { | ||
| const subTasks = await this._taskRepeatCfgService |
There was a problem hiding this comment.
I don't think that this is necessary, since only parent tasks should have a repeat config linked to it, but not their sub tasks.
| return this._taskService.createNewTaskWithDefaults({ | ||
| title: taskRepeatCfg.title, | ||
| additional: { | ||
| repeatCfgId: taskRepeatCfg.id, |
There was a problem hiding this comment.
I would just leave out the repeatCfgId part
| this._actions$.pipe( | ||
| ofType(updateTask), | ||
| tap(async (aAction) => { | ||
| const allTasks = await this._taskService.allTasks$.pipe(first()).toPromise(); |
There was a problem hiding this comment.
Better to directly use taskService.getByIdOnce$ here.
| /** | ||
| * Updates the repeatCfg of a task, if the task was updated. | ||
| */ | ||
| updateRepeatCfgWhenTaskUpdates: any = createEffect( |
There was a problem hiding this comment.
Do we really need this (honestly I am unsure :D)? Are there potential edge cases and problems that this might cause?
There was a problem hiding this comment.
Not sure about edge cases, but without this you have no way of changing contents of a repeatable task as far as I was able to see.
What I did: I tried to change the title of a repeatable task, but since each day the new tasks are generated based on the repeatCfg (where the title was not updated), the old title would appear. So if I wanted to actually change the title, I would have to remove the repeat-cfg first, then change the title and then add the repeat-config back.
There was a problem hiding this comment.
Currently you can change existing repeatable task instances by editing the repeatable config. If you edit the title there, the app asks you if you want to update all existing tasks. The idea behind it is, that you might want to have different notes and even tags for different instances. But I can see how it might be more intuitive to ditch this behavior and do it like this. Let's try it out!
| * When adding a sub task, this function checks if the parent is a repeatable task and therefore the sub-task also has to be. | ||
| * If that is the case, a repeat-cfg gets added for each subtask, too. | ||
| */ | ||
| addTaskRepeatCfgForSubTask: any = createEffect( |
There was a problem hiding this comment.
Imho subtasks should not be repeatable tasks, but just subtasks of a task with a repeatable config.
There was a problem hiding this comment.
Since finished tasks get deleted (at least that's what it seemed like when I tried to observe how the app works?) and I am not confident enough at this point to try and change things on such a fundamental level, I based this approach on the fact that repeatCfgs are persisted and can be used to store the information of the sub tasks. I basically mirrored that structure with the repeatCfs:
All Tasks (sub + parent) are stored as tasks (all together) in the store (only difference is if they have subtask-ids or a parent-Id or not)
All repeatCfgs (sub + parent) are stored as repeatCfs (all together) in the store (only difference is that they have a parent id or not)
Since all information of a repeatable task gets generated from a repeatable config, I only see 2 ways of storing the information for the sub tasks:
- storing all information about all subtasks in the parent repeat config (which would go against the structure that is currently implemented for normal tasks and therefore would feel like bad practise to me, but that might just be my view)
- storing the information about sub tasks as a subtask-repeatable-config
Definitely let me know if I missed something.
There was a problem hiding this comment.
Afaics subtasks don't need to "know" that they are a child task of a repeatable task and I also think different parent repeatable tasks should be allowed to have different sub tasks. To solve this I would not reference "real" sub tasks but rather provide a simple input that allows to create a list of sub tasks with no more information than maybe the title.
Using real "subtasks" as reference point creates all sorts of messy problems, when moving them around or deleting them or their parent.
|
Thank you very much! The code looks very clean! Much appreciated! I am a bit pressed for time, so I wasn't able to provide a complete feedback yet. Sending the incomplete feedback anyway since I am not sure I will find more time before next Friday. |
No problem, just take your time! I added my thoughts to your comments already so you can have a better insight into my thought progress for the review. |
|
I think I understand now my main misunderstanding :D The current approach for repeatable tasks is to take the repeatCfg as template to create new instances. Existing instances are not touched unless the the repeatCfg is updated by using the dedicated dialog for it. This allows an instance to be used relatively independent from others, e.g. I could create different sub tasks or notes every time. The changes you are providing take a different approach by making changes to a task directly change the repeatCfg and by making changes to sub tasks also reflect on new instances. This might be more intuitive to new users (and thus ultimately desirable, but So I think I might have to test this a bit more to understand all the implications of this changes. Or alternatively we can go with the "simpler" change that I proposed in the comment above and stick with the current approach of doing things. What do you think? |
|
@CodiumAI-Agent /review |
PR Analysis
PR Feedback
How to use
|
|
|
||
| // we only want to continue if the task doesn't already have a repeatCfgId | ||
| if (task.repeatCfgId === null) { | ||
| console.log('A TASKKK', task); |
There was a problem hiding this comment.
Consider removing the console.log statements as they are generally not recommended for production code. They can expose sensitive information and clutter the console output. [important]
| } | ||
| }), | ||
| ), | ||
| { dispatch: false }, // Question: What exactly does this do? |
There was a problem hiding this comment.
The 'dispatch: false' in the createEffect function means that the effect will not dispatch any new actions. If you want to dispatch new actions from within the effect, you should remove this option or set it to true. [medium]
| () => | ||
| this._actions$.pipe( | ||
| ofType(updateTask), | ||
| tap(async (aAction) => { |
There was a problem hiding this comment.
The 'await' inside the 'tap' operator may lead to unexpected behavior. It's better to use 'mergeMap' or 'concatMap' to handle asynchronous operations in an Observable chain. [important]
First of all: Sorry for the late answer! Once I have a quiet minute I will definitely look into your suggested simpler version, as I generally agree with trying to avoid possible problems or confusion. I am just really pressed for time these days so I am not quite sure when I will be able to pick it up again, but I hope to find time soon and I will contact you again, if I have further questions :) |
|
How is this doing? |
Since there has been a lot of things happening for me, I don't have time at all to work on this. If anyone wants to pick this up, feel free. If not I hope to maybe get to it at the end of the year, but currently I don't have any capacity to work on it.... :( |
|
@StartAGarden no worries! Thank you very much for your effort! Any volunteers to pick this up? |
|
Hi @StartAGarden, how are you doing? |
Description
The repeating tasks are updated so they include their sub-tasks.
Example Use Case: The user wants to add a task for a daily "Morning Routine" including several sub-tasks (steps of the routine), without having to re-add the sub-tasks every day.
There is no way to individually decide for certain sub tasks to be repeatable or not in this implementation. All sub tasks of a repeatable parent task are automatically repeatable, too.
Issues Resolved
#427
Check List