Skip to content

feat: add repeat subtasks for repeating tasks #427 #2659

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

feat: add repeat subtasks for repeating tasks #427 #2659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0696c37
feat: added base for subtask being automatically added
Jun 25, 2023
88c0787
feat:add repeat subtasks for repeating tasks #427
Jun 30, 2023
9713138
removed comment and console log
Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
180 changes: 171 additions & 9 deletions src/app/features/task-repeat-cfg/store/task-repeat-cfg.effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
delay,
filter,
first,
map,
mergeMap,
take,
tap,
Expand All @@ -22,11 +23,16 @@ import {
import { selectTaskRepeatCfgFeatureState } from './task-repeat-cfg.reducer';
import { PersistenceService } from '../../../core/persistence/persistence.service';
import { Task, TaskArchive, TaskCopy } from '../../tasks/task.model';
import { updateTask } from '../../tasks/store/task.actions';
import { addSubTask, updateTask } from '../../tasks/store/task.actions';
import { TaskService } from '../../tasks/task.service';
import { TaskRepeatCfgService } from '../task-repeat-cfg.service';
import { TaskRepeatCfg, TaskRepeatCfgState } from '../task-repeat-cfg.model';
import { forkJoin, from, merge, of } from 'rxjs';
import {
DEFAULT_TASK_REPEAT_CFG,
TaskRepeatCfg,
TaskRepeatCfgCopy,
TaskRepeatCfgState,
} from '../task-repeat-cfg.model';
import { Observable, forkJoin, from, merge, of } from 'rxjs';
import { setActiveWorkContext } from '../../work-context/store/work-context.actions';
import { SyncTriggerService } from '../../../imex/sync/sync-trigger.service';
import { SyncProviderService } from '../../../imex/sync/sync-provider.service';
Expand All @@ -38,6 +44,8 @@ import { Update } from '@ngrx/entity';
import { getDateTimeFromClockString } from '../../../util/get-date-time-from-clock-string';
import { isToday } from '../../../util/is-today.util';
import { DateService } from 'src/app/core/date/date.service';
import { getWorklogStr } from 'src/app/util/get-work-log-str';
import { getTaskById } from '../../tasks/store/task.reducer.util';

@Injectable()
export class TaskRepeatCfgEffects {
Expand All @@ -58,6 +66,131 @@ export class TaskRepeatCfgEffects {
{ dispatch: false },
);

/**
* Updates the repeatCfg of a task, if the task was updated.
*/
updateRepeatCfgWhenTaskUpdates: any = createEffect(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this (honestly I am unsure :D)? Are there potential edge cases and problems that this might cause?

Copy link
Author

@StartAGarden StartAGarden Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

() =>
this._actions$.pipe(
ofType(updateTask),
tap(async (aAction) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

const allTasks = await this._taskService.allTasks$.pipe(first()).toPromise();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to directly use taskService.getByIdOnce$ here.

const task = allTasks.find((aTask) => aTask.id === aAction.task.id)!;

if (task.repeatCfgId !== null) {
const repeatCfgForTask = await this._taskRepeatCfgService
.getTaskRepeatCfgById$(task.repeatCfgId)
.pipe(first())
.toPromise();

const taskChanges = aAction.task.changes;

// TODO: is there a better way to do this? Is there anything missing?
const repeatCfgChanges: Partial<TaskRepeatCfgCopy> = {
projectId: taskChanges.projectId ?? repeatCfgForTask.projectId,
title: taskChanges.title ?? repeatCfgForTask.title,
tagIds: taskChanges.tagIds ?? repeatCfgForTask.tagIds,
notes: taskChanges.notes ?? repeatCfgForTask.notes,
};

// TODO: Do we need to do this for all instances??
this._taskRepeatCfgService.updateTaskRepeatCfg(
task.repeatCfgId,
repeatCfgChanges,
);
}
}),
),
{ dispatch: false }, // Question: What exactly does this do?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

);

/**
* When a main task is made repeatable, this function checks if there are subtasks.
* If that is the case, a repeat-cfg gets added for each subtask, too.
*/
addTaskRepeatCfgForSubTasksOf: any = createEffect(
() =>
this._actions$.pipe(
ofType(addTaskRepeatCfgToTask),
tap(async (aAction) => {
// TODO: is there an easier way to get to the parent task?
const allTasks = await this._taskService.allTasks$.pipe(first()).toPromise();
const parentTask = allTasks.find((aTask) => aTask.id === aAction.taskId);
const parentTaskRepeatCfg = aAction.taskRepeatCfg;

if (parentTask !== undefined && parentTask.subTaskIds.length > 0) {
for (const aSubTaskId of parentTask.subTaskIds) {
const task = allTasks.find((aTask) => aTask.id === aSubTaskId)!;

const repeatCfg = {
...parentTaskRepeatCfg,
// TODO: anything missing in this list that should not be overwritten by the parent?
title: task.title,
notes: task.notes,
defaultEstimate: task.timeEstimate, // is this correct?
parentId: parentTask.repeatCfgId,
};

this._taskRepeatCfgService.addTaskRepeatCfgToTask(
task.id,
task.projectId,
repeatCfg,
);
}
}
}),
),
{ dispatch: false }, // Question: What exactly does this do?
);

/**
* 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(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imho subtasks should not be repeatable tasks, but just subtasks of a task with a repeatable config.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

() =>
this._actions$.pipe(
ofType(addSubTask),
tap(async (aAction) => {
const task = aAction.task;

// we only want to continue if the task doesn't already have a repeatCfgId
if (task.repeatCfgId === null) {
console.log('A TASKKK', task);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]


// TODO: is there an easier way to get to the parent task?
const allTasks = await this._taskService.allTasks$.pipe(first()).toPromise();
const parentTask = allTasks.find((aTask) => aTask.id === aAction.parentId);

console.log('PARENT TASK', parentTask);

if (parentTask !== undefined && parentTask.repeatCfgId !== null) {
const parentRepeatCfg = await this._taskRepeatCfgService
.getTaskRepeatCfgById$(parentTask.repeatCfgId)
.pipe(first())
.toPromise();

const repeatCfg = {
...parentRepeatCfg,
// TODO: anything missing in this list that should not be overwritten by the parent?
title: task.title,
notes: task.notes || undefined,
defaultEstimate: task.timeEstimate, // is this correct?
parentId: parentRepeatCfg.id,
};

this._taskRepeatCfgService.addTaskRepeatCfgToTask(
task.id,
task.projectId,
repeatCfg,
);
}
}
}),
),
{ dispatch: false }, // Question: What exactly does this do?
);

private triggerRepeatableTaskCreation$ = merge(
this._syncTriggerService.afterInitialSyncDoneAndDataLoadedInitially$,
this._actions$.pipe(
Expand Down Expand Up @@ -85,8 +218,14 @@ export class TaskRepeatCfgEffects {

// existing tasks with sub tasks are loaded, because need to move them to the archive
mergeMap(([taskRepeatCfgs, currentTaskId]) => {
// we only want to work with parent tasks, so filter out sub tasks
const parentTasksRepeatCfgs = taskRepeatCfgs.filter(
(aTask) => aTask.parentId === null,
);

// NOTE sorting here is important
const sorted = taskRepeatCfgs.sort(sortRepeatableTaskCfgs);
const sorted = parentTasksRepeatCfgs.sort(sortRepeatableTaskCfgs);

return from(sorted).pipe(
mergeMap((taskRepeatCfg: TaskRepeatCfg) =>
this._taskRepeatCfgService.getActionsForTaskRepeatCfg(
Expand All @@ -106,24 +245,47 @@ export class TaskRepeatCfgEffects {
ofType(deleteTaskRepeatCfg),
concatMap(({ id }) => this._taskService.getTasksByRepeatCfgId$(id).pipe(take(1))),
filter((tasks) => tasks && !!tasks.length),
mergeMap((tasks: Task[]) =>
tasks.map((task) =>
concatMap((value: TaskCopy[], index) => {
const tasks: Readonly<TaskCopy>[] = value;
const allSubIds = tasks.flatMap((aTask) => aTask.subTaskIds);

return this._taskService
.getByIdsLive$(allSubIds)
.pipe(map((aAllSubTasks: TaskCopy[]) => ({ aAllSubTasks, tasks })));
}),
mergeMap(({ aAllSubTasks, tasks }) => {
return [...aAllSubTasks, ...tasks].map((task) =>
updateTask({
task: {
id: task.id,
changes: { repeatCfgId: null },
},
}),
),
),
);
}),
),
);

removeConfigIdFromTaskArchiveTasks$: any = createEffect(
() =>
this._actions$.pipe(
ofType(deleteTaskRepeatCfg),
tap(({ id }) => {
tap(async ({ id }) => {
const subTasks = await this._taskRepeatCfgService
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment below

.getTaskRepeatCfgsByParentId$(id)
.pipe(first())
.toPromise();

if (subTasks.length > 0) {
const subTaskIds = subTasks.map((aTask) => aTask.id);

// remove repeat cfgs from sub tasks
for (const aId of subTaskIds) {
this._removeRepeatCfgFromArchiveTasks(aId);
}
}

// remove repeat cfg from main task
this._removeRepeatCfgFromArchiveTasks(id);
}),
),
Expand Down
18 changes: 17 additions & 1 deletion src/app/features/task-repeat-cfg/store/task-repeat-cfg.reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,17 @@ export const selectTaskRepeatCfgById = createSelector(
},
);

export const selectTaskRepeatCfgByParentId = createSelector(
selectAllTaskRepeatCfgs,
(taskRepeatCfgs: TaskRepeatCfg[], props: { id: string }): TaskRepeatCfg[] => {
const cfgs = taskRepeatCfgs.filter(
(aTaskRepeatCfg) => aTaskRepeatCfg.parentId === props.id,
);

return cfgs;
},
);

export const selectTaskRepeatCfgsWithStartTime = createSelector(
selectAllTaskRepeatCfgs,
(taskRepeatCfgs: TaskRepeatCfg[]): TaskRepeatCfg[] => {
Expand All @@ -55,7 +66,12 @@ export const selectTaskRepeatCfgsWithStartTime = createSelector(
export const selectTaskRepeatCfgsSortedByTitleAndProject = createSelector(
selectAllTaskRepeatCfgs,
(taskRepeatCfgs: TaskRepeatCfg[]): TaskRepeatCfg[] => {
return taskRepeatCfgs.sort((a, b) => {
// we only want main tasks, no sub tasks
const mainTaskRepeatCfgs = taskRepeatCfgs.filter(
(aTaskRepeatCfg) => aTaskRepeatCfg.parentId === null,
);

return mainTaskRepeatCfgs.sort((a, b) => {
if (a.projectId !== b.projectId) {
if (a.projectId === null) {
return -1;
Expand Down
2 changes: 2 additions & 0 deletions src/app/features/task-repeat-cfg/task-repeat-cfg.model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export type RepeatQuickSetting =

export interface TaskRepeatCfgCopy {
id: string;
parentId: string | null;
projectId: string | null;
lastTaskCreation: number;
title: string | null;
Expand Down Expand Up @@ -66,6 +67,7 @@ export const DEFAULT_TASK_REPEAT_CFG: Omit<TaskRepeatCfgCopy, 'id'> = {
defaultEstimate: undefined,

// id: undefined,
parentId: null,
projectId: null,
// lastTaskCreation: Date.now() - 24 * 60 * 60 * 1000,

Expand Down
Loading